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

Status: RESOLVED FIXED

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

Description

[Alex R.]

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

Comment 1

[Peter van der Woude [:Peter6]]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hrmph.  This used to work; the tests at http://lxr.mozilla.org/seamonkey/source/layout/html/tests/formctls/base/button_overconstrained.html  http://lxr.mozilla.org/seamonkey/source/layout/html/tests/formctls/base/button_overconstrained_strict.html http://lxr.mozilla.org/seamonkey/source/layout/html/tests/formctls/base/input_button_overconstrained_strict.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/formctls/base/input_button_overconstrained.html
used to pass...

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This was fixed by switching ComputeSize to use aAvailableWidth.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 247372 [details] [diff] [review]
Fix

minusing since patch is no longer needed

Comment 7

[Boris Zbarsky [:bzbarsky]]

Added tests for this to reftest.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Round to the nearest pixel

David, does that make your tests pass?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why do you expect this to help?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And no, it does not make the test pass.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 253974 [details] [diff] [review]
Round to the nearest pixel

OK.  Which way is the test image text offset from the ref?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The Area frame is at x=75 in both test and reference.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, and that's all *with* your patch above.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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?

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.)

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm seeing this failure in my Linux build too.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Set text-align:left

> That said, adding text-align:left to both the reference *and* the test

Checked in this change.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...which makes 359903-1 pass and makes 359903-2 fail.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ugh.  Let me look into that.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Er, yeah.  That change to -2 was just wrong; I backed it out.  I should figure out why it was passing here...