893298 - Misalignment label of input[type=submit/reset/button/file]

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Alice0775 White]

Attachment: 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]

Attachment: screenshot

Comment 2

[Boris Zbarsky [:bzbarsky]]

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]

Will take a look at this later today.

Comment 4

[betravis]

Attachment: Initial patch

Comment 5

[betravis]

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 [:bzbarsky]]

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 9

[betravis]

Attachment: 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 [:bzbarsky]]

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 12

[betravis]

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 [:bzbarsky]]

Ah, I see.  Does that test fail without the patch, given that the button is also getting a 100px line-height?

Comment 14

[betravis]

Attachment: 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]

Attachment: 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 [:bzbarsky]]

betravis, is that last patch just waiting on try results?  Or on something else?

Comment 18

[betravis]

Attachment: 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]

Attachment: Corrected patch

Accidentally included a merge in the last patch.

Comment 20

[betravis]

Try job successfully completed, should be ready to land.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/319da88f992e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Attachment: 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 25

[betravis]

Attachment: 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 27

[betravis]

Attachment: 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]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/123821ef07b4

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Attachment: 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]

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]

Attachment: 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 34

[Ryan VanderMeulen [:RyanVM]]

(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]]

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]

Third time's a charm (I hear).

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8536fd1da6

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c8536fd1da6