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

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Alice0775 White]

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

Comment 1

[Alice0775 White]

Attachment: reduced html

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The problem is that the button code does the NS_CSS_MINMAX after adding the padding, not before.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mats, Charly, did you audit consumers of mComputedHeight, in general?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm guessing not, since nsProgressFrame, nsMeterFrame, nsLeafBoxFrame have the exact same problem.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Er, I meant mComputedMaxHeight in comment 4.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix handling of max-height for buttons.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Alice0775 White, thank you for the (as usual!) amazing bug report!

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Try is looking good.

Comment 11

[Charly Molter :lahabana]

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!

Comment 12

[Charly Molter :lahabana]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix handling of max-height for frame classes that still seem to think it's a border-box height, not a content-box height.

Comment 15

[Mats Palmgren (inactive)]

Attachment: test for nsLeafBoxFrame::Reflow

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 16

[Mats Palmgren (inactive)]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed commit message and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/721ff7492145

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/721ff7492145

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa19e184a64c
https://hg.mozilla.org/releases/mozilla-beta/rev/c09b15ec8fdd

Comment 22

[Scoobidiver (away)]

Backout from Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/2db84458c7c3

Comment 23

[Jan Skrasek]

Where can I find out reasons for backout?

Comment 24

[Scoobidiver (away)]

(In reply to Jan Skrasek from comment #23)
> Where can I find out reasons for backout?
It's in bug 716875.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Gabriela [:gaby2300]]

I can verify the bug is fixed.
Using Win 7 64-bit and today's Aurora build.

Comment 27

[Simona Badau, Desktop QA]

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).