Closed Bug 893298 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout: Form Controls, defect)

25 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- unaffected
firefox25 + fixed

People

(Reporter: alice0775, Assigned: betravis)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files, 9 obsolete files)

Attached file 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
Attached image screenshot
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
Hardware: x86_64 → All
Will take a look at this later today.
Attached patch Initial patch (obsolete) — Splinter Review
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
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?
Duplicate of this bug: 894720
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?
Attached patch Updated patch (obsolete) — Splinter Review
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
Attached patch Correcting for focus padding (obsolete) — Splinter Review
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
Attached patch button-height.patch (obsolete) — Splinter Review
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
Attached patch Corrected patch (obsolete) — Splinter Review
Accidentally included a merge in the last patch.
Attachment #778051 - Attachment is obsolete: true
Try job successfully completed, should be ready to land.
Keywords: checkin-needed
Duplicate of this bug: 895278
Duplicate of this bug: 895984
Attached patch Updated patch (obsolete) — Splinter Review
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
Duplicate of this bug: 895321
Attached patch Transparent Background (obsolete) — Splinter Review
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
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
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
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
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
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!
Third time's a charm (I hear).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c8536fd1da6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Duplicate of this bug: 903566
You need to log in before you can comment on or make changes to this bug.