Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED in Firefox 17

Status

()

Core
Layout: Form Controls
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Alice0775 White, Assigned: bz)

Tracking

({regression})

16 Branch
mozilla18
x86
Windows 7
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16+ unaffected, firefox17+ verified)

Details

(Whiteboard: [testday-20121005])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 655466 [details]
reduced html
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
The problem is that the button code does the NS_CSS_MINMAX after adding the padding, not before.
(Assignee)

Comment 4

5 years ago
Mats, Charly, did you audit consumers of mComputedHeight, in general?
(Assignee)

Comment 5

5 years ago
I'm guessing not, since nsProgressFrame, nsMeterFrame, nsLeafBoxFrame have the exact same problem.
(Assignee)

Comment 6

5 years ago
Er, I meant mComputedMaxHeight in comment 4.
(Assignee)

Comment 7

5 years ago
Created attachment 655489 [details] [diff] [review]
Fix handling of max-height for buttons.
Attachment #655489 - Flags: review?(matspal)
(Assignee)

Comment 8

5 years ago
Alice0775 White, thank you for the (as usual!) amazing bug report!
(Assignee)

Updated

5 years ago
Assignee: nobody → bzbarsky
Whiteboard: [need review]
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 13

5 years ago
> 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.
(Assignee)

Comment 14

5 years ago
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.
Attachment #655605 - Flags: review?(matspal)
(Assignee)

Updated

5 years ago
Attachment #655489 - Attachment is obsolete: true
Attachment #655489 - Flags: review?(matspal)
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 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+
(Assignee)

Comment 17

5 years ago
Fixed commit message and pushed as https://hg.mozilla.org/integration/mozilla-inbound/rev/721ff7492145
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla18
(Assignee)

Comment 18

5 years ago
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?

Updated

5 years ago
tracking-firefox16: ? → +
tracking-firefox17: ? → +
https://hg.mozilla.org/mozilla-central/rev/721ff7492145
Status: NEW → RESOLVED
Last Resolved: 5 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+
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa19e184a64c
https://hg.mozilla.org/releases/mozilla-beta/rev/c09b15ec8fdd
status-firefox16: --- → fixed
status-firefox17: --- → fixed

Comment 22

5 years ago
Backout from Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/2db84458c7c3
status-firefox16: fixed → affected

Comment 23

5 years ago
Where can I find out reasons for backout?

Comment 24

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

Comment 25

5 years ago
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.
status-firefox16: affected → unaffected
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-firefox17: fixed → verified
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.