label and textbox misaligned when using align=baseline

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: florian, Assigned: karlt)

Tracking

(Depends on 1 bug, {regression})

Trunk
mozilla1.9.3a1
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

()

Attachments

(5 attachments, 1 obsolete attachment)

Posted image 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.
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....
Flags: blocking1.9.2?
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).
Assignee: nobody → mozbugz
Depends on: 502553
Posted file Testcase
Attaching proper testcase - textboxes with class="plain" used to align properly with text, they no longer do.
Blocks: abp
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
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Removing a little code makes the remainder a little easier to follow.
Attachment #400705 - Flags: review?(dbaron)
Posted patch reftestSplinter Review
This tests this bug and bug 502553.
Posted patch same reftest again (obsolete) — Splinter Review
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.
Attachment #400707 - Flags: review?(dbaron)
Blocks: 502553
Component: Layout: Form Controls → XUL
No longer depends on: 502553
QA Contact: layout.form-controls → xptoolkit.widgets
(In reply to comment #10)
> Created an attachment (id=400707) [details]
> fix

attachment 400707 [details] [diff] [review] and 400706 contain the same diff.
Posted patch fixSplinter Review
right patch, this time
Attachment #400707 - Attachment is obsolete: true
Attachment #400842 - Flags: review?(dbaron)
Attachment #400707 - Flags: review?(dbaron)
Attachment #400707 - Attachment description: fix → same reftest again
Blocks: 518272
I think it would be good to get this into beta if possible.
Priority: P2 → P1
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.
Priority: 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.
Attachment #400705 - Flags: review?(dbaron) → review+
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.
Attachment #400842 - Flags: review?(dbaron) → review+
Whiteboard: [needs review]
(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
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs 192 landing]
Target Milestone: --- → mozilla1.9.3a1
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.
Depends on: 528364
Depends on: 541804
You need to log in before you can comment on or make changes to this bug.