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

RESOLVED FIXED in Firefox 25

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: betravis)

Tracking

({regression, testcase})

25 Branch
mozilla25
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ fixed)

Details

Attachments

(4 attachments, 9 obsolete attachments)

(Reporter)

Description

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

Comment 1

4 years ago
Created attachment 775058 [details]
screenshot
(Reporter)

Updated

4 years ago
OS: Windows 7 → All
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...
Assignee: nobody → betravis
Keywords: testcase
Hardware: x86_64 → All
(Assignee)

Comment 3

4 years ago
Will take a look at this later today.
(Assignee)

Comment 4

4 years ago
Created attachment 776054 [details] [diff] [review]
Initial patch
(Assignee)

Comment 5

4 years ago
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?
Attachment #776054 - Flags: review?(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?
Attachment #776054 - Flags: review?(bzbarsky) → review+
Duplicate of this bug: 894439
Duplicate of this bug: 894449
(Assignee)

Comment 9

4 years ago
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.
Attachment #776054 - Attachment is obsolete: true
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?

Updated

4 years ago
Duplicate of this bug: 894720
(Assignee)

Comment 12

4 years ago
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.
Ah, I see.  Does that test fail without the patch, given that the button is also getting a 100px line-height?
(Assignee)

Comment 14

4 years ago
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
Attachment #776832 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
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.
Attachment #777316 - Attachment is obsolete: true
betravis, is that last patch just waiting on try results?  Or on something else?
Duplicate of this bug: 895610
(Assignee)

Comment 18

4 years ago
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
Attachment #777507 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
Created attachment 778065 [details] [diff] [review]
Corrected patch

Accidentally included a merge in the last patch.
Attachment #778051 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Try job successfully completed, should be ready to land.
Keywords: checkin-needed

Updated

4 years ago
Duplicate of this bug: 895278
https://hg.mozilla.org/integration/mozilla-inbound/rev/319da88f992e
Flags: in-testsuite+
Keywords: checkin-needed
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
Duplicate of this bug: 895984
(Assignee)

Comment 25

4 years ago
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.
Attachment #778065 - Attachment is obsolete: true

Updated

4 years ago
status-firefox24: --- → unaffected
status-firefox25: --- → affected
tracking-firefox25: ? → +

Updated

4 years ago
Duplicate of this bug: 895321
(Assignee)

Comment 27

4 years ago
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.
Attachment #778631 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
The last patch passed through the try server, so we should be good to go.
https://tbpl.mozilla.org/?tree=Try&rev=b981c3ba1836
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/123821ef07b4
Keywords: checkin-needed
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
Attachment #778539 - Attachment is obsolete: true
(Assignee)

Comment 31

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

Comment 32

4 years ago
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.
Attachment #778705 - Attachment is obsolete: true

Updated

4 years ago
Duplicate of this bug: 896720
Blocks: 895289
(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?
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!
(Assignee)

Comment 36

4 years ago
Third time's a charm (I hear).
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8536fd1da6
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c8536fd1da6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox25: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

4 years ago
Duplicate of this bug: 903566
You need to log in before you can comment on or make changes to this bug.