Closed Bug 823483 Opened 12 years ago Closed 8 years ago

Percentage max-width does not seem to affect contributions to intrinsic min-width

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox46 --- verified
firefox47 --- verified

People

(Reporter: T.J.Hunt, Assigned: dbaron)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(16 files)

workaround.htm
668 bytes, text/html
Details
some tests with <img>
1.69 KB, text/html
Details
some tests with empty <input type="button">
1.62 KB, text/html
Details
some tests with <input type="button">
1.82 KB, text/html
Details
First testcase rendered in Edge
10.09 KB, image/png
Details
Second testcase rendered in Edge
10.77 KB, image/png
Details
Third testcase rendered in Edge
12.82 KB, image/png
Details
img-testcase.html
2.17 KB, text/html
Details
img-testcase-Firefox.png
178.31 KB, image/png
Details
img-testcase-Chrome.png
56.97 KB, image/png
Details
MozReview Request: Bug 823483 patch 1 - Check for percentage max-width in addition to percentage width when deciding to ignore intrinsic min-width of replaced elements. r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
lizzard
: approval-mozilla-aurora+
Details
MozReview Request: Bug 823483 patch 2 - Add frame state bit to indicate frame classes that do replaced-element-like sizing. r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
lizzard
: approval-mozilla-aurora+
Details
MozReview Request: Bug 823483 patch 3 - Limit effect of percentage width and max-width on intrinsic size to elements with replaced element sizing. r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
lizzard
: approval-mozilla-aurora+
Details
MozReview Request: Bug 823483 patch 4 - Make a percentage max-width override a fixed width for replaced element intrinsic size computation. r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
lizzard
: approval-mozilla-aurora+
Details
MozReview Request: Bug 823483 patch 5 - Make (again) percentage width on text inputs make intrinsic minimum width be 0. r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
lizzard
: approval-mozilla-aurora+
Details
MozReview Request: Bug 823483 patch 6 - Tests r?dholbert
58 bytes, text/x-review-board-request
dholbert
: review+
lizzard
: approval-mozilla-aurora+
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232

Steps to reproduce:

I am experiencing this problem with a complex form, but there is a nice explanation at http://stackoverflow.com/questions/10672586/how-to-make-select-elements-shrink-to-max-width-percent-style-within-fieldset, see the "Clarification:" bit, and there is a minimal test-case at http://jsfiddle.net/e2H82/.

Applying min-width: X% to a select drop-down works fine, unless the select is nested somewhere inside a fieldset.

That is, with HTML like
<fieldset style="background:blue;">
 <select name=countries style="max-width:90%;">
  <option value=gs>South Georgia and the South Sandwich Islands</option>
 </select>
</fieldset>
 <select name=countries style="max-width:90%;">
  <option value=gs>South Georgia and the South Sandwich Islands</option>
 </select>


Actual results:

Make the browser window very narrow. The second select starts to shrink when the max-width:90%; kicks in. The first select never gets any narrower.


Expected results:

Both selects should size in the same way.

I think the same bug also affects textareas, but I have not made the same effort to cleanly reproduce that.

As an additional verification, if I use firebug to edit the fieldset tag in my original complex form to a div, then the max-width style rules on the select and textareas start working.

I don't think I have created a duplicate here. Searching, it took ma a long time to find anything (so, if I have created a duplicate, at least it will help people searching). The only vaguely close things I found were:

https://bugzilla.mozilla.org/show_bug.cgi?id=261037
https://bugzilla.mozilla.org/show_bug.cgi?id=504622

Those might possibly be duplicated, but determining that requires more knowledge of the Gecko rendering engine than I posess.
The problem is that percentage max-width on the <select> is a percentage of the width of the <fieldset>.

So the real question is what the width of that <fieldset> is.

And the problem you're running into is that a fieldset will never become narrower than the claimed min-intrinsic width of its kids (bug 504622, as you noted).

That said, it's a bit odd that setting max-width to a percentage doesn't affect the min-intrinsic width the same way that setting width to a percentage does...
Depends on: 504622
David, should we try to have percentage max-width effectively set intrinsic min-width to 0 like percentage width does?  At least as long as the min-width is not set...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dbaron)
Summary: select inside fieldset ignores max-width → Percentage max-width does not seem to affect contributions to intrinsic min-width
Component: Layout: Form Controls → Layout: Block and Inline
Hmm.  Maybe percentage width doesn't affect intrinsic min-width either in some cases?  Certainly this testcase:

  <!DOCTYPE html>
  <body style="width: 0">
    <div style="float: left; background: green">
      <span style="display: inline-block; width: 100%;
                   background: rgba(255, 0, 0, 0.5)">
        SomeLongText
      </span>
    </div>
  </body>

does not show it doing so.  But it does seem to for <select>; if I replace the <span> there with a <select> the float width collapses to 0...  I have no idea what's going on there.
(In reply to Boris Zbarsky (:bz) from comment #2)
> David, should we try to have percentage max-width effectively set intrinsic
> min-width to 0 like percentage width does?  At least as long as the
> min-width is not set...

Seems reasonable.  Presumably you'd split the middle part of this && in nsLayoutUtils::IntrinsicForContainer into an ||:

  else if (aType == MIN_WIDTH &&
           // The only cases of coord-percent-calc() units that
           // GetAbsoluteCoord didn't handle are percent and calc()s
           // containing percent.
           styleWidth.IsCoordPercentCalcUnit() &&
           aFrame->IsFrameOfType(nsIFrame::eReplaced)) {
    // A percentage width on replaced elements means they can shrink to 0.
    result = 0; // let |min| handle padding/border/margin
  }


I think the min-width overriding that would be taken care of by the code about 15 lines below.
Flags: needinfo?(dbaron)
Blocks: 441046
Hmm.  If I do it that way, then the percentage max-width will only set min-intrinsic to 0 if there is no explicit width set, right?  What behavior do we want in cases like:

  <img style="width: 100px; max-width: 100%">

?  For what it's worth, it looks like WebKit has the max-width win in that case, while for Opera the behavior is different for <select> and <img> at first glance (for <select> the max-width wins, but for <img> the width wins)...
Flags: needinfo?(dbaron)
In either case, fixing this bug really should fix bug 441046 and bug 434230 if I'm not screwing up my testing.
(In reply to Boris Zbarsky (:bz) from comment #5)
> Hmm.  If I do it that way, then the percentage max-width will only set
> min-intrinsic to 0 if there is no explicit width set, right?

Ah, true.

> What behavior
> do we want in cases like:
> 
>   <img style="width: 100px; max-width: 100%">
> 
> ?

Hmmm.  I'm not entirely sure.

And, actually, I'm now reconsidering my answer in comment 4, since when 'width' is set we've explicitly thrown away the intrinsic width, whereas when 'max-width' is set, we haven't explicitly thrown it away unless there's a value of 'width', in which case we might want to honor that.

I suppose I should think about this more...
Blocks: 434230
Blocks: 932996
Blocks: 936088
Blocks: 1047886
Blocks: 1023118
Attached file workaround.htm — 
For me (Win7, FF 36) wrapping the select within an element styled like this
{
 display: table;
 table-layout: fixed;
 width: 100%;
}
fixes the issue (compare with the test case at http://jsfiddle.net/e2H82/).

See also bug 504622.
Blocks: 1170774
and https://webcompat.com/issues/1343 Our monthly intrinsic width webcompat issue.
See Also: → https://webcompat.com/issues/1343
Right now I simply assume reports about elements that have max-width:n% set and end up wider than expected are instances of this bug. I volunteer to do some retesting of the dupes I've closed when there is a fix to make sure there isn't some other bug with the same symptom ;)
https://webcompat.com/issues/1533 has *something* to do with max-height/max-width, but I'm not quite sure if it's about this bug or something else..
See Also: → https://webcompat.com/issues/1533
Flags: needinfo?(dbaron)
Attachment #8690287 - Attachment description: some tests with <input type="button"> → some tests with empty <input type="button">
So Gecko and IE11 actually match pretty closely right now; the only differences I see are on the width:% (not min-width or max-width %) cases for the non-empty button, and on the intrinsic width of the empty button.

Chromium is substantially different for anything involving % max-width on image.  But Chromium and IE appear to match exactly on all the <input type="button"> cases.

I'm curious whether WebKit is any different from Chromium, and whether Edge is any different from IE.  (Ubuntu's Webkit-GTK matches Chromium on intrinsic widths but not final widths for the images, and has differences on the empty button, maybe related to margin.)  I can't test Edge.



Do all the testcases duped against this involve max-width on *images*, or are there other sorts of elements involved?
So Edge changed (relative to IE11) on the image testcase so that Edge and Chromium now match on all 3 testcases.
Attached file img-testcase.html — 
Almost most of the layout will be affected, I hope to be able to repair as soon as possible.

    
    

    
  

      
      
      
      

        Attached image
          
        img-testcase-Chrome.png
        — 
      

    


  

    
    

    
  
I looked into the Chromium behavior a bit more closely, and now I have patches for this, but I still need to write some tests.
Assignee: nobody → dbaron
Flags: needinfo?(dbaron)
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 17 Branch → Trunk

    
    

    
  
(In reply to David Baron [:dbaron] ⌚UTC-5 from comment #39)
> I looked into the Chromium behavior a bit more closely, and now I have
> patches for this, but I still need to write some tests.

Thank you for your work.

    
    

    
  
Another one.
https://webcompat.com/issues/2028

img with max-width: 100% inside a table. So probably related to Bug 434230
See Also:  → https://webcompat.com/issues/2028
Blocks: 1095359

    
    

    
  
Sorry for the delay; picked this up again after holidays, standards meetings, and vacation.

Finished up with one more patch needed to avoid a compatibility regression (for text inputs, which have their own special behavior), and now a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44bb0fcc6332

    
    

    
  


  
This (modulo changes in later patches) matches the behavior of Chromium
and Edge.  It increases the set of elements to which this quirky
behavior applies.

Review commit: https://reviewboard.mozilla.org/r/32887/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32887/

        Attachment #8713919 -
        Flags: review?(dholbert)

    
    

    
  


  
This just reorders the if-else chain to change which conditions are
tested first.  Prior to patch 1, the order didn't matter, but with patch
1, the order does matter, and the order that we happened to have was the
opposite of the one that matches Chromium and Edge.

Review commit: https://reviewboard.mozilla.org/r/32893/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32893/

        Attachment #8713922 -
        Flags: review?(dholbert)

    
    

    
  


  
This restores the quirky behavior for text inputs, which was removed by
patch 2, but only halfway (for width but not max-width), which matches
Chromium and Edge.

Review commit: https://reviewboard.mozilla.org/r/32895/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32895/

        Attachment #8713923 -
        Flags: review?(dholbert)

    
    

    
  


  
Our behavior on these tests is reasonably close to matching Chromium
thanks to the combination of patches 1, 3, and 4, and 5.

FIXME: Make min-intrinsic-with-percents-across-elements less platform-specific.

Review commit: https://reviewboard.mozilla.org/r/32897/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32897/

        Attachment #8713924 -
        Flags: review?(dholbert)

    
    

    
  
(In reply to David Baron [:dbaron] ⌚️UTC+11 (busy, returning 8 February) from comment #53)
> FIXME: Make min-intrinsic-with-percents-across-elements less
> platform-specific.

Er, I did that (with the visibility:hidden stuff), but forgot to remove the FIXME.
Blocks: 1085106

    
    

    
  
Comment on attachment 8713919 [details]
MozReview Request: Bug 823483 patch 1 - Check for percentage max-width in addition to percentage width when deciding to ignore intrinsic min-width of replaced elements.  r?dholbert

https://reviewboard.mozilla.org/r/32887/#review30165

        Attachment #8713919 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8713920 [details]
MozReview Request: Bug 823483 patch 2 - Add frame state bit to indicate frame classes that do replaced-element-like sizing.  r?dholbert

https://reviewboard.mozilla.org/r/32889/#review30167

        Attachment #8713920 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8713921 [details]
MozReview Request: Bug 823483 patch 3 - Limit effect of percentage width and max-width on intrinsic size to elements with replaced element sizing.  r?dholbert

https://reviewboard.mozilla.org/r/32891/#review30171

        Attachment #8713921 -
        Flags: review?(dholbert) → review+

        Attachment #8713922 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8713922 [details]
MozReview Request: Bug 823483 patch 4 - Make a percentage max-width override a fixed width for replaced element intrinsic size computation.  r?dholbert

https://reviewboard.mozilla.org/r/32893/#review30175

        Attachment #8713923 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8713923 [details]
MozReview Request: Bug 823483 patch 5 - Make (again) percentage width on text inputs make intrinsic minimum width be 0.  r?dholbert

https://reviewboard.mozilla.org/r/32895/#review30177

    
    

    
  
Comment on attachment 8713924 [details]
MozReview Request: Bug 823483 patch 6 - Tests  r?dholbert

https://reviewboard.mozilla.org/r/32897/#review30179

(Remember to drop the FIXME from your extended commit message before landing; you've probably already done that locally.)

::: layout/reftests/css-sizing/min-intrinsic-with-percents-across-elements-ref.html:2
(Diff revision 1)
> +<title>Tests for bug 823483</title>

Consider s/Tests/Reference/ in the reference case title, so that it's easier to tell which file is which when viewing the testcase & reference case alongside each other (by simply looking at the titlebar).

(I can't quickly tell which is which from the URL bar, because the "-ref.html" suffix is pushed off the end of the URL bar.)

::: layout/reftests/css-sizing/min-intrinsic-with-percents-across-elements-ref.html:43
(Diff revision 1)
> +  <td>canvas, unstyled</td>

Observation: from diffing the testcase & reference case, it looks like the "unstyled"/"almost-unstyled" cases use identical markup between the testcase & reference case.

So, I'm not sure those chunks are technically testing anything, since the identical markup will clearly render the same between testcase & reference case.

I guess you have them there for completeness, which makes some sense. If you can think of a way to make them more usefully tested, great; otherwise, not a big deal I suppose.

::: layout/reftests/css-sizing/min-intrinsic-with-percents-across-elements.html:122
(Diff revision 1)
> +

Nit: There's a stray blank line before the close-brace of the "for" loop in each file here.

That probably wants to be removed, though it doesn't matter much.

        Attachment #8713924 -
        Flags: review?(dholbert) → review+

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #60)
> So, I'm not sure those chunks are technically testing anything, since the
> identical markup will clearly render the same between testcase & reference
> case.
> 
> I guess you have them there for completeness, which makes some sense. If you
> can think of a way to make them more usefully tested, great; otherwise, not
> a big deal I suppose.

Yeah, not sure I can think of anything useful to do with them, and probably not worth spending time on it.

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee0ff2c8be1bf45c2431b4010ff1fe068a286400
Bug 823483 patch 1 - Check for percentage max-width in addition to percentage width when deciding to ignore intrinsic min-width of replaced elements.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/3705a4614be0c45f3850deee4f1266ec92521fc7
Bug 823483 patch 2 - Add frame state bit to indicate frame classes that do replaced-element-like sizing.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/d68daa674f403de8093ea36b03825bbff493fe25
Bug 823483 patch 3 - Limit effect of percentage width and max-width on intrinsic size to elements with replaced element sizing.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/b34ebd7afb7811db1ef5ba36dc5c22153c99d5ce
Bug 823483 patch 4 - Make a percentage max-width override a fixed width for replaced element intrinsic size computation.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6a5f77ef94e26e74a19268b0c8b69bb39d3b55
Bug 823483 patch 5 - Make (again) percentage width on text inputs make intrinsic minimum width be 0.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/623934e9db8ab60eb605fba84e03e882651e8f02
Bug 823483 patch 6 - Tests  r=dholbert

    
    

    
  
David, Daniel, 
once you have done the review dance, I volunteer to go through all sites with the issues and see how we fix (and hopefully not break things).

Thanks a lot. <3

    
    

    
  
(In reply to percyley from comment #64)
> There are still problems in Flexbox.

I'm pretty sure that's bug 1030952, not this bug.

    
    

    
  
It seems that bug 1227140 is also not related to this.

    
    

    
  
Comment on attachment 8713919 [details]
MozReview Request: Bug 823483 patch 1 - Check for percentage max-width in addition to percentage width when deciding to ignore intrinsic min-width of replaced elements.  r?dholbert

Approval Request Comment for all 6 patches
[Feature/regressing bug #]: none
[User impact if declined]: bad layout on Web pages; this is a high-priority Web compatibility fix
[Describe test coverage new/current, TreeHerder]: a few reftests in the tree
[Risks and why]: I think the changes are relatively low risk since they change our behavior in a way that moves us closer to other browsers.  However, there's always some risk in changing behavior.  In this case, however, the changes seem likely to help.
[String/UUID change made/needed]: no

        Attachment #8713919 -
        Flags: approval-mozilla-aurora?

        Attachment #8713920 -
        Flags: approval-mozilla-aurora?

        Attachment #8713921 -
        Flags: approval-mozilla-aurora?

        Attachment #8713922 -
        Flags: approval-mozilla-aurora?

        Attachment #8713923 -
        Flags: approval-mozilla-aurora?

        Attachment #8713924 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8713919 [details]
MozReview Request: Bug 823483 patch 1 - Check for percentage max-width in addition to percentage width when deciding to ignore intrinsic min-width of replaced elements.  r?dholbert

Improves web compatibility, please uplift to aurora.

        Attachment #8713919 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8713920 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8713921 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8713922 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8713923 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8713924 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
  
Blocks: 1060131

    
  
Depends on: 1247929
Depends on: 1248174

    
  
Depends on: 1248749
Blocks: 1308929

    
  
Depends on: 1377440
See Also:  → 1613576

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: