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.

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

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

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.

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

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

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

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

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

This tests this bug and bug 502553.

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.

(In reply to comment #10) > Created an attachment (id=400707) [details] > fix attachment 400707 [details] [diff] [review] and 400706 contain the same diff.

right patch, this time

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

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

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

Comment on attachment 400705 [details] [diff] [review] remove some things from nsSprocketLayout that don't do anything r=dbaron. Sorry for the delay.

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.

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

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

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