Last Comment Bug 785754 - Buttons on some sites are not rendered right and are like half cut off
: Buttons on some sites are not rendered right and are like half cut off
Status: VERIFIED FIXED
[testday-20121005]
: regression
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: 16 Branch
: x86 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
Mentors:
Depends on:
Blocks: 776265
  Show dependency treegraph
 
Reported: 2012-08-26 15:12 PDT by Alice0775 White
Modified: 2012-11-06 16:21 PST (History)
15 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
unaffected
+
verified


Attachments
reduced html (301 bytes, text/html)
2012-08-26 15:51 PDT, Alice0775 White
no flags Details
Fix handling of max-height for buttons. (11.36 KB, patch)
2012-08-26 19:27 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter 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. (11.14 KB, patch)
2012-08-27 08:53 PDT, Boris Zbarsky [:bz] (still a bit busy)
mats: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
test for nsLeafBoxFrame::Reflow (2.79 KB, application/vnd.mozilla.xul+xml)
2012-08-27 12:22 PDT, Mats Palmgren (:mats)
no flags Details

Description Alice0775 White 2012-08-26 15:12:00 PDT
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 2012-08-26 15:51:14 PDT
Created attachment 655466 [details]
reduced html
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-08-26 18:31:37 PDT
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 [:bz] (still a bit busy) 2012-08-26 18:42:14 PDT
The problem is that the button code does the NS_CSS_MINMAX after adding the padding, not before.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-08-26 18:43:35 PDT
Mats, Charly, did you audit consumers of mComputedHeight, in general?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-08-26 18:56:40 PDT
I'm guessing not, since nsProgressFrame, nsMeterFrame, nsLeafBoxFrame have the exact same problem.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-08-26 19:26:42 PDT
Er, I meant mComputedMaxHeight in comment 4.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-08-26 19:27:09 PDT
Created attachment 655489 [details] [diff] [review]
Fix handling of max-height for buttons.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-08-26 19:27:48 PDT
Alice0775 White, thank you for the (as usual!) amazing bug report!
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-08-26 19:31:48 PDT
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 [:bz] (still a bit busy) 2012-08-26 23:00:02 PDT
Try is looking good.
Comment 11 Charly Molter :lahabana 2012-08-27 00:49:50 PDT
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 2012-08-27 02:44:14 PDT
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 [:bz] (still a bit busy) 2012-08-27 08:40:47 PDT
> 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 [:bz] (still a bit busy) 2012-08-27 08:53:32 PDT
Created 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.
Comment 15 Mats Palmgren (:mats) 2012-08-27 12:22:40 PDT
Created attachment 655702 [details]
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 (:mats) 2012-08-27 12:31:44 PDT
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 [:bz] (still a bit busy) 2012-08-27 12:47:28 PDT
Fixed commit message and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/721ff7492145
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-08-27 12:51:03 PDT
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] 2012-08-27 19:17:17 PDT
https://hg.mozilla.org/mozilla-central/rev/721ff7492145
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-29 15:27:16 PDT
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 22 Scoobidiver (away) 2012-09-17 00:03:30 PDT
Backout from Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/2db84458c7c3
Comment 23 Jan Skrasek 2012-09-17 01:31:35 PDT
Where can I find out reasons for backout?
Comment 24 Scoobidiver (away) 2012-09-17 02:04:12 PDT
(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 [:bz] (still a bit busy) 2012-09-17 04:21:51 PDT
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] 2012-10-05 14:22:03 PDT
I can verify the bug is fixed.
Using Win 7 64-bit and today's Aurora build.
Comment 27 Simona B [:simonab ] 2012-10-29 07:31:58 PDT
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).

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