Last Comment Bug 893298 - Misalignment label of input[type=submit/reset/button/file]
: Misalignment label of input[type=submit/reset/button/file]
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: 25 Branch
: All All
: -- normal with 4 votes (vote)
: mozilla25
Assigned To: betravis
:
: Jet Villegas (:jet)
Mentors:
: 894439 894449 894720 895278 895321 895610 895984 896720 (view as bug list)
Depends on:
Blocks: 835873 895289
  Show dependency treegraph
 
Reported: 2013-07-12 18:22 PDT by Alice0775 White
Modified: 2013-08-09 13:50 PDT (History)
25 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
testcase (193 bytes, text/html)
2013-07-12 18:22 PDT, Alice0775 White
no flags Details
screenshot (95.74 KB, image/png)
2013-07-12 18:28 PDT, Alice0775 White
no flags Details
Initial patch (4.59 KB, patch)
2013-07-15 17:16 PDT, betravis
bzbarsky: review+
Details | Diff | Splinter Review
Updated patch, with zero check and new test (5.07 KB, patch)
2013-07-16 17:52 PDT, betravis
no flags Details | Diff | Splinter Review
Updated patch (5.09 KB, patch)
2013-07-17 12:54 PDT, betravis
no flags Details | Diff | Splinter Review
Correcting for focus padding (5.11 KB, patch)
2013-07-17 17:43 PDT, betravis
no flags Details | Diff | Splinter Review
button-height.patch (5.59 MB, patch)
2013-07-18 14:13 PDT, betravis
no flags Details | Diff | Splinter Review
Corrected patch (5.28 KB, patch)
2013-07-18 14:29 PDT, betravis
no flags Details | Diff | Splinter Review
screenshot of failing test from log (9.88 KB, image/png)
2013-07-19 10:06 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details
Updated patch (5.32 KB, patch)
2013-07-19 12:47 PDT, betravis
no flags Details | Diff | Splinter Review
Transparent Background (5.33 KB, patch)
2013-07-19 16:08 PDT, betravis
no flags Details | Diff | Splinter Review
screenshot of failing test from log (5.64 KB, image/png)
2013-07-22 08:51 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details
button-height.patch (5.29 KB, patch)
2013-07-22 11:47 PDT, betravis
no flags Details | Diff | Splinter Review

Description Alice0775 White 2013-07-12 18:22:19 PDT
Created attachment 775056 [details]
testcase

Steps To Reproduce:
1. Open testcase

Actual Results:
The label is placed at top

Expected Results:
The label should be placed in the center to the vertical direction

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/468b35185c44
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130710 Firefox/25.0 ID:20130710064558
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b7d6458d2a3c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130710 Firefox/25.0 ID:20130710075527
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=468b35185c44&tochange=b7d6458d2a3c

Suspected:
64e0c9f89e	Bear Travis — Bug 835873 - Include minHeight calculation before vertically centering a button's children. r=bz
Comment 1 Alice0775 White 2013-07-12 18:28:28 PDT
Created attachment 775058 [details]
screenshot
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2013-07-12 21:18:40 PDT
This is definitely a regression from bug 835873.

The problem is that the vertical centering uses this expression:

  aReflowState.ComputedHeight() - aDesiredSize.height

to determine the amount of space it needs to split in two, but we've already modified aDesiredSize.height.  I'm sorry I didn't catch this during the review...

What probably needs to happen here is to:

1)  Instead of modifying aDesiredSize.height before the vertical centering, store the value we compute in actualDesiredHeight.

2)  Make the vertical centering not conditional on whether we're auto-height.

3)  Use "actualDesiredHeight - aDesiredSize.height" to compute the vertical offset bits.

4)  Set aDesiredSize.height to actualDesiredHeight after all that is done.

The minInternalHeight bits that were trying to sort of handle min-height here can go away in the process.

And I guess we need tests to catch this problem in the future...
Comment 3 betravis 2013-07-15 10:55:32 PDT
Will take a look at this later today.
Comment 4 betravis 2013-07-15 17:16:00 PDT
Created attachment 776054 [details] [diff] [review]
Initial patch
Comment 5 betravis 2013-07-15 17:19:17 PDT
Comment on attachment 776054 [details] [diff] [review]
Initial patch

Took a first stab at a fix. Is this what you were thinking? Are there any additional test cases that I should add?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2013-07-15 19:54:36 PDT
Comment on attachment 776054 [details] [diff] [review]
Initial patch

>-    if (yoff < 0) {
>-      yoff = 0;
>-    }

You need to keep that part.  Apart from that, this looks like what I was thinking about, yes.

Does the test fail without the patch?
Comment 7 Chris Peterson [:cpeterson] 2013-07-16 09:00:23 PDT
*** Bug 894439 has been marked as a duplicate of this bug. ***
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2013-07-16 09:27:52 PDT
*** Bug 894449 has been marked as a duplicate of this bug. ***
Comment 9 betravis 2013-07-16 17:52:59 PDT
Created attachment 776832 [details] [diff] [review]
Updated patch, with zero check and new test

I've added back the zero check. The previous test case did not test the desired behavior. The new test compares an input button with a simple styled div.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2013-07-16 21:04:59 PDT
The code looks good.

I'm not sure I follow the test: the button will do vertical centering, but the div won't, right?  How can they end up looking the same?
Comment 11 pqwoerituytrueiwoq 2013-07-16 21:39:15 PDT
*** Bug 894720 has been marked as a duplicate of this bug. ***
Comment 12 betravis 2013-07-17 11:16:54 PDT
The div's line-height is set to 100px, and the text is centered within the line. The test passes locally, but I'm running a try job just to be sure. Will set for commit once the try job completes.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2013-07-17 11:50:07 PDT
Ah, I see.  Does that test fail without the patch, given that the button is also getting a 100px line-height?
Comment 14 betravis 2013-07-17 12:54:27 PDT
Created attachment 777316 [details] [diff] [review]
Updated patch

The test fails without the patch as Firefox ignores line-height on form elements, as they are replaced [1]. Going to go ahead and simplify, though, to explicitly set the line-height, and do so only on divs.

[1] http://www.w3.org/TR/CSS21/visudet.html#leading
Comment 15 betravis 2013-07-17 17:43:48 PDT
Created attachment 777507 [details] [diff] [review]
Correcting for focus padding

Text should be vertically centered within the focus content area. The updated patch removes the focus border and padding before calculating yoff. The omission originally caused layout/reftests/bug/491180-2.html to fail.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2013-07-18 13:52:21 PDT
betravis, is that last patch just waiting on try results?  Or on something else?
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2013-07-18 14:03:10 PDT
*** Bug 895610 has been marked as a duplicate of this bug. ***
Comment 18 betravis 2013-07-18 14:13:07 PDT
Created attachment 778051 [details] [diff] [review]
button-height.patch

The test in the last patch was failing on a linux try job. I'm not able to reproduce the failure locally, so I tweaked the patch and am currently waiting on the following try job:

https://tbpl.mozilla.org/?tree=Try&rev=2f53b1c603d2
Comment 19 betravis 2013-07-18 14:29:24 PDT
Created attachment 778065 [details] [diff] [review]
Corrected patch

Accidentally included a merge in the last patch.
Comment 20 betravis 2013-07-18 14:33:10 PDT
Try job successfully completed, should be ready to land.
Comment 21 mjh563 2013-07-18 15:54:56 PDT
*** Bug 895278 has been marked as a duplicate of this bug. ***
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-07-19 07:35:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/319da88f992e
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-07-19 10:06:49 PDT
Created attachment 778539 [details]
screenshot of failing test from log

Backed out for B2G reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab0ba7fb3f4

https://tbpl.mozilla.org/php/getParsedLog.php?id=25493623&tree=Mozilla-Inbound
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2013-07-19 11:06:04 PDT
*** Bug 895984 has been marked as a duplicate of this bug. ***
Comment 25 betravis 2013-07-19 12:47:37 PDT
Created attachment 778631 [details] [diff] [review]
Updated patch

I can't reproduce the failure locally, but am testing out setting -moz-appearance: none and running another try job:
https://tbpl.mozilla.org/?tree=Try&rev=d70c3dc3e4f4
If that doesn't work, might be able to set the background to transparent.
Comment 26 Drew Willcoxon :adw 2013-07-19 15:50:23 PDT
*** Bug 895321 has been marked as a duplicate of this bug. ***
Comment 27 betravis 2013-07-19 16:08:14 PDT
Created attachment 778705 [details] [diff] [review]
Transparent Background

Even with appearance set to none, the browser looks to be adding a background image. Trying the method used in reftests/bugs/491180-2.html.
Comment 28 betravis 2013-07-21 22:53:16 PDT
The last patch passed through the try server, so we should be good to go.
https://tbpl.mozilla.org/?tree=Try&rev=b981c3ba1836
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-07-22 05:57:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/123821ef07b4
Comment 30 Ryan VanderMeulen [:RyanVM] 2013-07-22 08:51:13 PDT
Created attachment 779222 [details]
screenshot of failing test from log

Backed out for failing on Linux64 this time. As you can see, the button text is being cut off a bit. Note that your last Try run didn't actually run reftests outside of B2G.
https://hg.mozilla.org/integration/mozilla-inbound/rev/582a0b8558d1
Comment 31 betravis 2013-07-22 11:44:52 PDT
Sorry for that last bit. I was trying to minimize using try resources, but it looks like it's best to just be sure. I've removed the width property and kicked off another, complete, try job.

https://tbpl.mozilla.org/?tree=Try&rev=bc9e9bd4aa17
Comment 32 betravis 2013-07-22 11:47:06 PDT
Created attachment 779311 [details] [diff] [review]
button-height.patch

The font is slightly bigger on the linux systems, causing the button text to clip on that platform. Setting width to auto should fix this.
Comment 33 mjh563 2013-07-22 17:35:22 PDT
*** Bug 896720 has been marked as a duplicate of this bug. ***
Comment 34 Ryan VanderMeulen [:RyanVM] 2013-07-23 08:00:29 PDT
(In reply to betravis from comment #31)
> Sorry for that last bit. I was trying to minimize using try resources, but
> it looks like it's best to just be sure. I've removed the width property and
> kicked off another, complete, try job.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=bc9e9bd4aa17

You can run *just* reftests, you know :). Anyway, the Try run looks good. Ready for checkin-needed?
Comment 35 Ed Morley [:emorley] 2013-07-23 08:04:45 PDT
http://trychooser.pub.build.mozilla.org/ is useful for selecting which jobs to run on Tryserver, in case someone hadn't mentioned it before :-)

Thank you for the patch!
Comment 36 betravis 2013-07-23 10:01:58 PDT
Third time's a charm (I hear).
Comment 37 Ryan VanderMeulen [:RyanVM] 2013-07-23 13:44:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8536fd1da6
Comment 38 Ed Morley [:emorley] 2013-07-24 05:44:05 PDT
https://hg.mozilla.org/mozilla-central/rev/3c8536fd1da6
Comment 39 Drew Willcoxon :adw 2013-08-09 13:50:55 PDT
*** Bug 903566 has been marked as a duplicate of this bug. ***

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