Closed Bug 785754 Opened 12 years ago Closed 12 years ago

Buttons on some sites are not rendered right and are like half cut off

Categories

(Core :: Layout: Form Controls, defect)

16 Branch
x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 + unaffected
firefox17 + verified

People

(Reporter: alice0775, Assigned: bzbarsky)

References

Details

(Keywords: regression, Whiteboard: [testday-20121005])

Attachments

(3 files, 1 obsolete file)

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/b3cce81fef1a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120826030526
and
http://hg.mozilla.org/releases/mozilla-aurora/rev/7d9e6b2b9db7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120826 Firefox/16.0 ID:20120826042008

Buttons on some sites are not rendered right and are like half cut off

@kilara1988 reported with screenshot in http://forums.mozillazine.org/viewtopic.php?p=12229127#p12229127

Steps To Reproduce
 1. Start Firefox with clean profile
 2. Go to www.yahoo.com and Sign in
 3. Go to Yahoo! mail

Actual Results:
 Buttons are half cut off

Expected results
 Should not


Regression wndow(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/35b8d6ef5d46
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120819111901
Bad:
http://hg.mozilla.org/mozilla-central/rev/c676b554c7bb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120819211001
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=35b8d6ef5d46&tochange=c676b554c7bb


Regression wndow(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/84bf19883685
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120819013101
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d99b258809ca
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120819090701
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=84bf19883685&tochange=d99b258809ca

Treggered by:
d99b258809ca	Charly Molter — Bug 776265 - changing the way ComputeHeightValue works to make it work just like ComputeWidthValue already does. this fixes {min,max}-height + adding reftests for {min,max}-{height,width} r=mats
Attached file reduced html
In this case the max-height is 23px, the top padding is 4px, the bottom padding is 3px, and the border is 1px.  So the content area should end up 23 - 4 - 3 - 2 == 14px tall.

But it's actually ending up 5px tall, which suggests that something is double-subtracting the border and padding here.
The problem is that the button code does the NS_CSS_MINMAX after adding the padding, not before.
Mats, Charly, did you audit consumers of mComputedHeight, in general?
I'm guessing not, since nsProgressFrame, nsMeterFrame, nsLeafBoxFrame have the exact same problem.
Er, I meant mComputedMaxHeight in comment 4.
Attachment #655489 - Flags: review?(matspal)
Alice0775 White, thank you for the (as usual!) amazing bug report!
Assignee: nobody → bzbarsky
Whiteboard: [need review]
https://tbpl.mozilla.org/?tree=Try&rev=b2fbf42fa5e9 just to make sure that adding the meter tests and changing leaf box frame don't make the world explode...  Though the leaf box frame change should just restore the old behavior there too.
Try is looking good.
I'm not entirely sure but I think that NS_CSS_MIN_MAX is not necessary anymore since this is considered in nsFrame::computeSize(). I'll have a try today and tell you. 
Anyway thanks for finding that one and sorry I missed something that important!
Ok so after looking a little bit. The NS_MIN_MAX is not necessary on progress and meter. However it seems to be necessary on button as it fails to render properly otherwise. This needs to be investigated deeper!
Also is it only me or nsLeafBoxFrame's reflow is quite weird? Why do we do we constraint the width with min and max? There might be something I don't get in nsLeafBoxFrame...
Finally bz did you see that you reftests for progress and meter (not the ref) render differently on webkit(safari and chrome) ?However, I think I remember somebody saying that the handling of {min,max} properties on webkit was wrong. Sorry if I'm saying crap Just want to make sure ;)
Finally about the reftests again I would change the name of meter-max-height.html to max-height.html and the same for the meter reftests as this seems to be the norm in the folder.
Also I do really think we should put some order in layout/reftests/forms/ it's a bit of a mess. I think the 1-folder 1-tag thing is nice. I am going to file a bug report for that.
> The NS_MIN_MAX is not necessary on progress and meter. 

Ah, because the thing they're using aReflowState.ComputedHeight()?  That's an excellent point; good catch.

> However it seems to be necessary on button

Because in that case it's being applied to the intrinsic height derived from reflowing the button contents, not to aReflowState.ComputedHeight().

> Also is it only me or nsLeafBoxFrame's reflow is quite weird?

It's not you.  ;)  It's XUL reflow, so "weird" is the name of the game....

> did you see that you reftests for progress and meter (not the ref) render differently
> on webkit(safari and chrome) ?

No, but I just looked and those simply look buggy.

> I would change the name of meter-max-height.html to max-height.html

Indeed.  Will fix.
Attachment #655489 - Attachment is obsolete: true
Attachment #655489 - Flags: review?(matspal)
One way to test nsLeafBoxFrame::Reflow is wrapping <xul:image>
inside <html:div>.  Fwiw, it appears we still get one case wrong -
height:auto with max-height and box-sizing:content-box.
I've highlighted it with red border color.  (most of the content-box
cases were wrong before though, so it's not a recent regression)
Comment on attachment 655605 [details] [diff] [review]
Fix handling of max-height for frame classes that still seem to think it's a border-box height, not a content-box height.

Looks good to me, sorry for not catching these earlier.  r=mats

I find the commit message somewhat confusing though -
the "it's" should probably refer to mComputedMinHeight/mComputedMaxHeight,
not max-height which *is* a border box height for box-sizing:border-box.
Attachment #655605 - Flags: review?(matspal) → review+
Fixed commit message and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/721ff7492145
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla18
Comment on attachment 655605 [details] [diff] [review]
Fix handling of max-height for frame classes that still seem to think it's a border-box height, not a content-box height.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 776265 
User impact if declined: Buttons on some websites are too small for their text,
   so users can't read the text.
Testing completed (on m-c, etc.): Passes automated tests, testcase in this bug.
Risk to taking this patch (and alternatives if risky): Risk is medium, as you
  can tell by the fact that this is a regression fix for a regression from a
  regression fix to start with.  Alternatives are to ship the rendering
  regression or back out bug 776265 (and then ship _that_ regression).
  Personally, I think the right fix is to take this patch, since I think the
  chance of it breaking things for users is lower than the other options.
String or UUID changes made by this patch: None
Attachment #655605 - Flags: approval-mozilla-beta?
Attachment #655605 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/721ff7492145
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 655605 [details] [diff] [review]
Fix handling of max-height for frame classes that still seem to think it's a border-box height, not a content-box height.

Thanks bz for the descriptive risk assessment, your proposal makes sense, let's uplift this instead of backing out to get back to a regression state.
Attachment #655605 - Flags: approval-mozilla-beta?
Attachment #655605 - Flags: approval-mozilla-beta+
Attachment #655605 - Flags: approval-mozilla-aurora?
Attachment #655605 - Flags: approval-mozilla-aurora+
Where can I find out reasons for backout?
(In reply to Jan Skrasek from comment #23)
> Where can I find out reasons for backout?
It's in bug 716875.
You meant "unaffected": the bug that caused this one was backed out too.  But maybe it should have stayed "fixed".  Who knows.  In any case, "affected" is wrong.
Keywords: verifyme
I can verify the bug is fixed.
Using Win 7 64-bit and today's Aurora build.
Whiteboard: [testday-20121005]
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0

Verified on Firefox 17 beta 3 that the buttons are properly displayed (verified using the attached test case from Comment 1 and on Yahoo! mail).
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.