494901 - label and textbox misaligned when using align=baseline

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Florian Quèze [:florian]]

Attachment: Screenshot

The attached screenshot was made by entering this line in the error console:
openDialog('data:application/vnd.mozilla.xul+xml,<?xml version="1.0"?><?xml-stylesheet href="chrome://global/skin/"?><window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><hbox align="baseline"><label value="test"/><textbox value="text"/></hbox></window>')

On the left with a build before the checkin of bug 481751, and on the right after.

I checked that this change comes from bug 481751 by doing a local backout of changeset http://hg.mozilla.org/mozilla-central/rev/a1b982e50678 and verifying that the bug isn't in the resulting build.

Comment 1

[Richard Marti (:Paenglab)]

I can confirm this. Some labels (textboxes) in the header pane of Thunderbird
3.1a1pre are also misaligned. See Bug 494478

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like both renderings are wrong, no?  The right rendering would line up the two pieces of text....

Comment 3

[Florian Quèze [:florian]]

I think both renderings are wrong, yes. But the old one was only a polish issue, while the new one makes the UI hard to understand when there are several lines of label/textboxes in the same dialog.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, sure.  I'm just saying we should fix this right, imo.

Comment 5

[Karl Tomlinson (:karlt)]

The misalignment before checkin of bug 481751 is bug 502553.

The change from bug 481751 is that the value returned by nsTextControlFrame::GetBoxAscent depends on the height of the box which is not set until layout.

nsSprocketLayout:Layout calls GetBoxAscent on its children before laying them out (and does not update the values after layout).

Comment 6

[Wladimir Palant]

Attachment: Testcase

Attaching proper testcase - textboxes with class="plain" used to align properly with text, they no longer do.

Comment 7

[Wladimir Palant]

Note that changing align attribute dynamically helps to "fix" this issue. I used this as work-around for Adblock Plus: http://hg.mozdev.org/adblockplus/rev/845f041f6cf4

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: remove some things from nsSprocketLayout that don't do anything

Removing a little code makes the remainder a little easier to follow.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: reftest

This tests this bug and bug 502553.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: same reftest again

This separates positioning of box children along each direction, so that they are first positioned along the axis of the box and sized appropriately (and laid out), then secondly aligned in the out-of-axis direction.
The alignment does not require multiple passes, and by moving it after sizing, nsTextControlFrame::GetBoxAscent() returns a suitable value.

The ascent cached on nsBoxFrames is invalidated before Layout, so that changes due to resizing are picked up.

Comment 11

[Florian Quèze [:florian]]

(In reply to comment #10)
> Created an attachment (id=400707) [details]
> fix

attachment 400707 [details] [diff] [review] and 400706 contain the same diff.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: fix

right patch, this time

Comment 13

[Karl Tomlinson (:karlt)]

I think it would be good to get this into beta if possible.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

You have marked this as a P1 blocker, meaning that we will hold beta for the fix. Was that your intent - your comment 13 indicates that you think it would be "good" which isn't the same as "must".

Comment 15

[Karl Tomlinson (:karlt)]

I didn't mean "must": P1 -> P2.

Comment 16

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

Comment on attachment 400705 [details] [diff] [review]
remove some things from nsSprocketLayout that don't do anything

r=dbaron.  Sorry for the delay.

Comment 17

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

Comment on attachment 400842 [details] [diff] [review]
fix

>+  // These are only calculated if needed
>+  nsIBox::Halignment halign = nsBoxFrame::hAlign_Left;
>+  nsIBox::Valignment valign = nsBoxFrame::vAlign_Top;
>+  nscoord maxAscent = 0;
>+  PRBool isLTR = PR_TRUE;

I think you're better off leaving these uninitialized here so that
valgrind will warn if they're used in the wrong case.

>+  nsIBox* child = aBox->GetChildBox();
>+  while (child) {
...
>+    child = child->GetNextBox();
>   }

I tend to prefer
  for (nsIBox *child = aBox->GetChildBox(); child;
       child = child->GetNextBox()) {
  }
but it's ok either way.


r=dbaron, and sorry for the delay.

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to comment #17)
> (From update of attachment 400842 [details] [diff] [review])
> >+  // These are only calculated if needed
> >+  nsIBox::Halignment halign = nsBoxFrame::hAlign_Left;
> >+  nsIBox::Valignment valign = nsBoxFrame::vAlign_Top;
> >+  nscoord maxAscent = 0;
> >+  PRBool isLTR = PR_TRUE;
> 
> I think you're better off leaving these uninitialized here so that
> valgrind will warn if they're used in the wrong case.

That makes sense and seems the "right" thing to do.  I had initialized to
avoid gcc's "may be used uninitialized in this function" warnings, but that's
only in optimized builds anyway.

> I tend to prefer
>   for (nsIBox *child = aBox->GetChildBox(); child;
>        child = child->GetNextBox()) {
>   }
> but it's ok either way.

I see advantages in that, but the "while" construct is used so often in this
file that if there was a single different construct readers might wonder what
trick this was trying.

Comment 19

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/f6f255d72236
http://hg.mozilla.org/mozilla-central/rev/f163afecb7c3
http://hg.mozilla.org/mozilla-central/rev/d62ab530c043

Comment 20

[[e-mail]]

Comment on attachment 400842 [details] [diff] [review]
fix

>+          // Should this center the border box?
>+          // This centers the margin box, the historical behavior.
I don't think this should center the border box.

Comment 21

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/18236b2a6b80
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e746d089d5be
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6f78943ca537