Closed
Bug 835873
Opened 11 years ago
Closed 11 years ago
Misalignment (incorrect baseline?) when using min-height compared to height
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: mounir, Assigned: betravis)
References
Details
(Keywords: testcase, Whiteboard: [mentor=bz][lang=c++])
Attachments
(2 files, 3 obsolete files)
378 bytes,
text/html
|
Details | |
4.85 KB,
patch
|
Details | Diff | Splinter Review |
Two buttons, one with height: 50px and the other with min-height: 50px have the same height as expected but have a different baseline.
Reporter | ||
Comment 1•11 years ago
|
||
The test case and the issue are coming from: http://stackoverflow.com/questions/14587372/misplacement-of-submit-type-input-in-firefox-when-using-min-height
Comment 2•11 years ago
|
||
This happens because the "center child vertically" bit in ReflowButtonContents happens before we adjust the height to take min-height into account (in Reflow). We should probably pass in the info needed to compute the right thing to ReflowButtonContents or something. Mounir, want to fix?
Updated•11 years ago
|
Whiteboard: DUPEME → [mentor=bz][lang=c++]
Comment 3•11 years ago
|
||
Hi, I'm new to bugzilla and would like to fix my first bug! Would this be a good one to start on? Thanks!
Comment 4•11 years ago
|
||
This one is fine, yes. Someone was looking at it at some point, but I haven't heard back from them in a while....
Comment 5•11 years ago
|
||
So to expand on comment 2, the way button reflow works right now is that we call ReflowButtonContents to reflow its contents, then set the height of the button based on the size of the contents (unless it has a non-auto height), then adjust the height based on min/max-height. The problem is that the vertical positioning of the button contents happens inside ReflowButtonContents, and uses the height available to it, which is the intrinsic height of the button unless a non-auto height is specified. But we already pass aReflowState to ReflowButtonContents, an it already writes to aDesiredSize.height. So the right thing is probably to move the application of min/max height (the entire block at http://hg.mozilla.org/mozilla-central/file/962f5293f87f/layout/forms/nsHTMLButtonControlFrame.cpp#l213 ) down into ReflowButtonContents, somewhere between the ReflowChild call (which computes the height of the child) and the "center child vertically" bit.
Comment 6•11 years ago
|
||
Hi, can I take this bug ?
Comment 8•11 years ago
|
||
Usurelu, could you confirm that you're still working on this?
Flags: needinfo?(catalin.usurelu5)
Comment 9•11 years ago
|
||
Hi Josh, yes I am, I've run into some problems and I just can't see why it isn't working; now I'm a little busy, if I don't finish it by next week I'll leave it :( .
Flags: needinfo?(catalin.usurelu5)
Comment 10•11 years ago
|
||
Please feel free to post what you have in the bug and ask me to look it over to see if anything jumps out at me. That's why I'm here!
Comment 11•11 years ago
|
||
Hi, I'm sorry but I won't be able to work on this bug for some time, please unassign me. Sorry :(
Comment 12•11 years ago
|
||
Not a problem. Just let me know when you have time again!
Assignee: catalin.usurelu5 → nobody
Assignee | ||
Comment 13•11 years ago
|
||
An initial patch based on bz's suggestion in comment 5 to move the aDesiredSize height calculation into ReflowButtonContents.
Attachment #772365 -
Flags: review?(bzbarsky)
Comment on attachment 772365 [details] [diff] [review] Initial patch You definitely need to add the reftest to the reftest.list file so that it will be run.
Assignee: nobody → betravis
Assignee | ||
Comment 15•11 years ago
|
||
Adding the test to the reftest.list file
Attachment #772365 -
Attachment is obsolete: true
Attachment #772365 -
Flags: review?(bzbarsky)
Attachment #772713 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•11 years ago
|
||
The reftests for forms were recently refactored into separate files. The updated patch takes this into account and moves the reftest to the layout/reftests/forms/button directory.
Attachment #772713 -
Attachment is obsolete: true
Attachment #772713 -
Flags: review?(bzbarsky)
Attachment #772740 -
Flags: review?(bzbarsky)
Pushed attachment 772740 [details] [diff] [review] to try: https://tbpl.mozilla.org/?tree=Try&rev=6cfc37da5696
Comment 18•11 years ago
|
||
Comment on attachment 772740 [details] [diff] [review] Updating reftest You probably want to change the "If computed use the computed value" comment to say something like "Compute our desired height before we start vertically centering our kids". And please add the missing newlines to the end of your test files. r=me with those changes. Thank you for fixing this!
Attachment #772740 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Thanks for the review! Added newlines to the test files, fixed the comment in nsHTMLButtonControlFrame.cpp, and shortened the log comment.
Attachment #772740 -
Attachment is obsolete: true
Updated•11 years ago
|
Keywords: checkin-needed
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6064e0c9f89e
Flags: in-testsuite+
Keywords: checkin-needed
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6064e0c9f89e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•