Last Comment Bug 835873 - Misalignment (incorrect baseline?) when using min-height compared to height
: Misalignment (incorrect baseline?) when using min-height compared to height
: testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla25
Assigned To: betravis
: Jet Villegas (:jet)
Depends on: 893298
  Show dependency treegraph
Reported: 2013-01-29 09:22 PST by Mounir Lamouri (:mounir)
Modified: 2013-07-13 09:41 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Testcase (378 bytes, text/html)
2013-01-29 09:22 PST, Mounir Lamouri (:mounir)
no flags Details
Initial patch (4.54 KB, patch)
2013-07-08 16:15 PDT, betravis
no flags Details | Diff | Splinter Review
Updated patch (4.96 KB, patch)
2013-07-09 10:16 PDT, betravis
no flags Details | Diff | Splinter Review
Updating reftest (4.94 KB, patch)
2013-07-09 10:49 PDT, betravis
bzbarsky: review+
Details | Diff | Splinter Review
Incorporating review feedback (4.85 KB, patch)
2013-07-09 13:24 PDT, betravis
no flags Details | Diff | Splinter Review

Description User image Mounir Lamouri (:mounir) 2013-01-29 09:22:06 PST
Created attachment 707678 [details]

Two buttons, one with height: 50px and the other with min-height: 50px have the same height as expected but have a different baseline.
Comment 1 User image Mounir Lamouri (:mounir) 2013-01-29 09:22:39 PST
The test case and the issue are coming from:
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2013-01-29 09:27:03 PST
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?
Comment 3 User image Jeremy Heintz 2013-02-12 17:19:16 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-02-12 17:51:03 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2013-03-27 22:06:24 PDT
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 ) down into ReflowButtonContents, somewhere between the ReflowChild call (which computes the height of the child) and the "center child vertically" bit.
Comment 6 User image Usurelu Catalin 2013-03-28 08:12:06 PDT
Hi, can I take this bug ?
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2013-03-28 08:15:15 PDT
Comment 8 User image Josh Matthews [:jdm] 2013-04-25 02:36:31 PDT
Usurelu, could you confirm that you're still working on this?
Comment 9 User image Usurelu Catalin 2013-04-25 05:18:36 PDT
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 :( .
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2013-04-25 07:53:25 PDT
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 User image Usurelu Catalin 2013-05-13 14:58:42 PDT
Hi, I'm sorry but I won't be able to work on this bug for some time, please unassign me. Sorry :(
Comment 12 User image Boris Zbarsky [:bz] (still a bit busy) 2013-05-13 21:26:35 PDT
Not a problem.  Just let me know when you have time again!
Comment 13 User image betravis 2013-07-08 16:15:31 PDT
Created attachment 772365 [details] [diff] [review]
Initial patch

An initial patch based on bz's suggestion in comment 5 to move the aDesiredSize height calculation into ReflowButtonContents.
Comment 14 User image David Baron :dbaron: ⌚️UTC-8 2013-07-08 16:35:05 PDT
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.
Comment 15 User image betravis 2013-07-09 10:16:22 PDT
Created attachment 772713 [details] [diff] [review]
Updated patch

Adding the test to the reftest.list file
Comment 16 User image betravis 2013-07-09 10:49:48 PDT
Created attachment 772740 [details] [diff] [review]
Updating reftest

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.
Comment 17 User image David Baron :dbaron: ⌚️UTC-8 2013-07-09 11:32:42 PDT
Pushed attachment 772740 [details] [diff] [review] to try:
Comment 18 User image Boris Zbarsky [:bz] (still a bit busy) 2013-07-09 11:37:32 PDT
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!
Comment 19 User image betravis 2013-07-09 13:24:00 PDT
Created attachment 772842 [details] [diff] [review]
Incorporating review feedback

Thanks for the review!

Added newlines to the test files, fixed the comment in nsHTMLButtonControlFrame.cpp, and shortened the log comment.
Comment 20 User image Ryan VanderMeulen [:RyanVM] 2013-07-10 07:11:19 PDT
Comment 21 User image Ed Morley [:emorley] 2013-07-11 03:07:36 PDT

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