Closed Bug 359903 Opened 13 years ago Closed 13 years ago

[FIX][reflow branch] Form buttons have improper text alignment

Categories

(Core :: Layout: Form Controls, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: alex+list, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ComputeSize aAvailableWidth])

Attachments

(4 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061107 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061107 Minefield/3.0a1

The text inside form buttons is not centered as it should be, but aligned farther to the right.

Take a look at any button here on Bugzilla, or anywhere else.

Reproducible: Always
Attached file testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
So this is a ComputeSize regression.  nsHTMLButtonControlFrame::ReflowButtonContents munges the availSize of the child reflow state, but the child area frame no longer uses the availSize for its sizing -- it now uses the mComputedWidth.
Attached patch FixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #247372 - Flags: review?(dbaron)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: [reflow branch] Form buttons have improper text alignment → [FIX][reflow branch] Form buttons have improper text alignment
Target Milestone: --- → mozilla1.9alpha
This was fixed by switching ComputeSize to use aAvailableWidth.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [ComputeSize aAvailableWidth]
Comment on attachment 247372 [details] [diff] [review]
Fix

minusing since patch is no longer needed
Attachment #247372 - Flags: review?(dbaron) → review-
Added tests for this to reftest.
Flags: in-testsuite+
So test 1 is currently failing on Mac, and it seems like it ought to for any platforms without native-theme buttons.  The rule on forms.css line 448 gives buttons 6px of horizontal padding.

Now, what I don't understand is why the failure manifests itself only as a difference in subpixel positioning of the "M" glyph.  (It looks like Mac does subpixel positioning of text.)
The rule in forms.css is more or less ignored for too-small button widths due to the code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsHTMLButtonControlFrame.cpp&rev=3.216&mark=355-372,378#354

As for subpixel positioning, we should probably round extraleft to the nearest pixel.
David, does that make your tests pass?
Attachment #253974 - Flags: review?(dbaron)
Why do you expect this to help?
Well, the only way I can think of for the text to not end up on a pixel boundary is if extraleft is an odd number of pixels and we end up with extraleft being a non-integral pixel offset....
And no, it does not make the test pass.
Comment on attachment 253974 [details] [diff] [review]
Round to the nearest pixel

OK.  Which way is the test image text offset from the ref?
Attachment #253974 - Flags: review?(dbaron) → review-
So, the differences between test and reference according to frame dumps are:

ButtonControl (input):  width=293 (in both test and reference)

  Area (input):         test width=143  reference width=180
                        test overflow width=168;  no overflow in reference

    Text:               width=168 (in both test and reference)
                        test x=0        reference x=6
The Area frame is at x=75 in both test and reference.
Oh, and that's all *with* your patch above.
Oh.  Right.  The problem is that in the reference the area is wider than the text, so the text-align:center kicks in and (180-168)/2 == 6 is exactly how far to the right it gets moved.

Does setting "text-align: left" on the reference button fix the issue?
Yes, putting text-align: left on the input in 359903-1-ref.html makes the test pass.

(But under what conditions should things really be centered?  It seems like currently both are off-center, but by different amounts.)
(That said, adding text-align:left to both the reference *and* the test and then filing another bug for the centering might be the best path forward.)
Basically, the text is centered within the Area (whatever "text-align: center" does, including whatever it does when the text is wider than the Area).  The problems start if the Area is wider than the content-width of the button.  We attempt to center it within the button anyway by overlapping it with the left padding, up to the point where the left edge of the Area is up against the left padding edge.

So if the Area's width is wider than the padding-box width of the button the text will not really end up centered in the button overall...
I'm seeing this failure in my Linux build too.
> That said, adding text-align:left to both the reference *and* the test

Checked in this change.
...which makes 359903-1 pass and makes 359903-2 fail.
Ugh.  Let me look into that.
Er, yeah.  That change to -2 was just wrong; I backed it out.  I should figure out why it was passing here...
Blocks: 476815
You need to log in before you can comment on or make changes to this bug.