Bug 4860 – div align="center" rendering problem

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexei Svitkine]

This is easier to see with two examples:

Example1: http://projectmagma.net/~myrdred/flirt1.html
Example2: http://projectmagma.net/~myrdred/flirt2.html

Example1 renders identically under Firefox, IE and Safari.
Example2 renders differently under Safari than under Firefox and IE.

The only difference between the source code of those two examples, is that in example two, there's 
extra text inside the div tag, "Hello!".

Basically, the error is that the second string of text is only being affected by its parent's div's 
align="center" only when there's some other text in the div tag as well, but when there isn't - like in 
example2, it is unaffected by its parent's div tag. This should not be the case!

[Joost de Valk (AlthA)]

Confirmed, good testcases too. Making it p3, but if you can hand me sites that break because of this, i'll 
make it a p2 for you :)

[Eric Seidel (no email)]

I screened a bug earlier today about align=center not working for a table, perhaps that was enclosing a 
table in a <div align="center"> and seeing that fail to align correctly.  Either way, this sounds to me like a 
serious bug which is likely very common.

[Eric Seidel (no email)]

It would be best if we had a layout-test compatible test case attached to this bug.  Although there might 
already be one made for a dup of this bug (as I expect it has dups).

[Joost de Valk (AlthA)]

*** Bug 6262 has been marked as a duplicate of this bug. ***

[Alexei Svitkine]

Created attachment 17292 [details]
This is the "flirt1.html" page that was linked in the original summary (no longer available online).

[Alexei Svitkine]

Created attachment 17293 [details]
This is the "flirt2.html" page that was linked in the original summary (no longer available online).

[David Kilzer (:ddkilzer)]

Is the align="center" attribute in the <div> overriding the "margin-left: -150px;" in the <font> tag, but only when there is text inside the <div>?

[Robert Hogan]

Created attachment 100239 [details]
Patch

[Simon Fraser (smfr)]

Related to bug 64030?

[Robert Hogan]

(In reply to comment #9)
> Related to bug 64030?

No, seems to be a different issue. The test case on that bug still fails with my patch.

[Dave Hyatt]

Comment on attachment 100239 [details]
Patch

Nice patch. I think these updateLogicalWidth functions are horribly named. I know you're just following the convention someone else already used, but we should really fix the names.  They make it sound like you're changing the logical width of the block itself, when that isn't what they're doing at all.

Also, why only patch the inline case? Doesn't the block case needs to be patched as well? What do other browsers do with the block case? (e.g., try a div with a fixed width instead of a font tag).

[Robert Hogan]

Created attachment 101891 [details]
Patch

[Robert Hogan]

(In reply to comment #11)
> (From update of attachment 100239 [details])
> Nice patch. I think these updateLogicalWidth functions are horribly named. I know you're just following the convention someone else already used, but we should really fix the names.  They make it sound like you're changing the logical width of the block itself, when that isn't what they're doing at all.

I agree but have left this alone for now. It would introduce too much noise into the patch I think - I will change them in a later one if that's ok? 

> Also, why only patch the inline case? Doesn't the block case needs to be patched as well? What do other browsers do with the block case? (e.g., try a div with a fixed width instead of a font tag).

I've addressed this in the updated patch.

[Dave Hyatt]

Comment on attachment 101891 [details]
Patch

r=me on this although we need to fix the awful updateLogicalWidth function names, since nobody's logicalWidth is actually being updated.

[Robert Hogan]

Committed r93616: <http://trac.webkit.org/changeset/93616>

[Peter Kasting]

This was rolled out in r93621, so reopening.

I have a request for future versions of this patch's tests.  Whenever possible, could you avoid printing text to the screen?  For example, the description of what the test should do can go in comments inside the HTML file instead of being printed.  This sort of change reduces the need to make new pixel baselines for e.g. every different flavor of Chromium due to font rendering differences between OSes or OS versions.

For absolute-positioned-inline-in-centred-block.html , perhaps you could use some kind of <span> instead of the <font> element to achieve a similar result?

Thanks for the consideration.

[Robert Hogan]

Created attachment 105441 [details]
Patch

[Dave Hyatt]

Comment on attachment 105441 [details]
Patch

Note that startOffsetForLine is doing the wrong thing for RTL and returning an offset from the left when it should be from the right. I broke this a while back. startAlignedOffsetForLine has the same mistake. I'm cool with landing as is, but your next patch will hopefully fix both of these methods to do the right thing for RTL (before you tackle the parent vs. containing block issue with RTL).

[Robert Hogan]

Committed r94492: <http://trac.webkit.org/changeset/94492>

Created attachment 119617 [details]
Test case showing incorrect and unstable layout

(In reply to comment #19)
> Committed r94492: <http://trac.webkit.org/changeset/94492>

This change was incorrect. The attached test case shows two problems with the behavior after the change:
1. After this change, I would expect the two lines in the test to look identical, like they do in Firefox and in Opera. But they do not.
2. Resizing the window after opening the test, which causes relayout, changes the layout of the first line. That means that the layout isn’t stable. The reason is apparently that startAlignedOffsetForLine() calls logicalWidthForChild() on the positioned child at a time where it doesn’t have up-to-date layout yet.

This change broke some Mac App Store content. We should either complete the fix or revert the change.

[Robert Hogan]

(In reply to comment #20)

> 2. Resizing the window after opening the test, which causes relayout, changes the layout of the first line. That means that the layout isn’t stable. The reason is apparently that startAlignedOffsetForLine() calls logicalWidthForChild() on the positioned child at a time where it doesn’t have up-to-date layout yet.
> 
> This change broke some Mac App Store content. We should either complete the fix or revert the change.

I can't recreate the instability on Qt or Chromium - the layout of the first line stays wrong for me all the time.

I believe the problem is that positioned objects with no leading characters are skipped over by skipLeadingWhiteSpace() and positioned there and then. They're not included in the line run so their aligned or unaligned position can't take account of subsequent objects in the line run on first positioning or even later.

So reverting would mean that instead of getting centred, the blue box in the test case would sit on the extreme left of the silver box rather than in the centre (as it currently does) or over the 'Lorem' as it does in Firefox or Opera.

I'm not sure if the rendering in the Mac App store after reversion would still appear broken, i.e. with the absolute element was positioned on the extreme left, as I can't access it. I imagine positioning too far to the left rather than too far to the centre is probably not an issue for that particular page.

As to fixing this, I am looking at it now. I think the place to fix it is in computeInlineDirectionPositionsForLine() where the positioned object could learn of the total logical width of the run.

(In reply to comment #21)
> (In reply to comment #20)
> 
> > 2. Resizing the window after opening the test, which causes relayout, changes the layout of the first line. That means that the layout isn’t stable. The reason is apparently that startAlignedOffsetForLine() calls logicalWidthForChild() on the positioned child at a time where it doesn’t have up-to-date layout yet.
> > 
> > This change broke some Mac App Store content. We should either complete the fix or revert the change.
> 
> I can't recreate the instability on Qt or Chromium - the layout of the first line stays wrong for me all the time.

You are probably seeing the result of two layout passes, since those ports have always-on scrollbars. You can try setting the height property of the body element to something taller than the viewport to observe the result of a single layout pass. Or perhaps enclose the test in a fixed-width, fixed-height relative-positioned container.

> I believe the problem is that positioned objects with no leading characters are skipped over by skipLeadingWhiteSpace() and positioned there and then. They're not included in the line run so their aligned or unaligned position can't take account of subsequent objects in the line run on first positioning or even later.
> 
> So reverting would mean that instead of getting centred, the blue box in the test case would sit on the extreme left of the silver box rather than in the centre (as it currently does) or over the 'Lorem' as it does in Firefox or Opera.

Right. Your change did not replace correct behavior with incorrect one, it replaced incorrect behavior with different incorrect and unstable behavior.

> I'm not sure if the rendering in the Mac App store after reversion would still appear broken, i.e. with the absolute element was positioned on the extreme left, as I can't access it. I imagine positioning too far to the left rather than too far to the centre is probably not an issue for that particular page.
> 
> As to fixing this, I am looking at it now. I think the place to fix it is in computeInlineDirectionPositionsForLine() where the positioned object could learn of the total logical width of the run.