Bug 4860 - div align="center" rendering problem
Summary: div align="center" rendering problem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: All All
: P3 Normal
Assignee: Robert Hogan
URL:
Keywords:
: 6262 (view as bug list)
Depends on: 66796
Blocks: 31241
  Show dependency treegraph
 
Reported: 2005-09-05 19:50 PDT by Alexei Svitkine
Modified: 2012-02-21 16:49 PST (History)
7 users (show)

See Also:


Attachments
This is the "flirt1.html" page that was linked in the original summary (no longer available online). (258 bytes, text/html)
2007-11-15 07:55 PST, Alexei Svitkine
no flags Details
This is the "flirt2.html" page that was linked in the original summary (no longer available online). (250 bytes, text/html)
2007-11-15 07:55 PST, Alexei Svitkine
no flags Details
Patch (52.24 KB, patch)
2011-07-10 14:08 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (67.97 KB, patch)
2011-07-25 12:19 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (325.31 KB, patch)
2011-08-27 12:14 PDT, Robert Hogan
hyatt: review+
Details | Formatted Diff | Diff
Test case showing incorrect and unstable layout (470 bytes, text/html)
2011-12-16 09:11 PST, mitz
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Svitkine 2005-09-05 19:50:40 PDT
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!
Comment 1 Joost de Valk (AlthA) 2005-09-06 23:02:37 PDT
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 :)
Comment 2 Eric Seidel (no email) 2005-12-28 01:27:47 PST
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.
Comment 3 Eric Seidel (no email) 2005-12-28 01:29:13 PST
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).
Comment 4 Joost de Valk (AlthA) 2005-12-28 07:07:23 PST
*** Bug 6262 has been marked as a duplicate of this bug. ***
Comment 5 Alexei Svitkine 2007-11-15 07:55:30 PST
Created attachment 17292 [details]
This is the "flirt1.html" page that was linked in the original summary (no longer available online).
Comment 6 Alexei Svitkine 2007-11-15 07:55:39 PST
Created attachment 17293 [details]
This is the "flirt2.html" page that was linked in the original summary (no longer available online).
Comment 7 David Kilzer (:ddkilzer) 2007-11-15 09:29:07 PST
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>?

Comment 8 Robert Hogan 2011-07-10 14:08:46 PDT
Created attachment 100239 [details]
Patch
Comment 9 Simon Fraser (smfr) 2011-07-10 22:49:25 PDT
Related to bug 64030?
Comment 10 Robert Hogan 2011-07-11 11:56:15 PDT
(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.
Comment 11 Dave Hyatt 2011-07-21 11:55:17 PDT
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).
Comment 12 Robert Hogan 2011-07-25 12:19:11 PDT
Created attachment 101891 [details]
Patch
Comment 13 Robert Hogan 2011-08-03 14:04:50 PDT
(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.
Comment 14 Dave Hyatt 2011-08-22 12:33:24 PDT
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.
Comment 15 Robert Hogan 2011-08-23 11:12:34 PDT
Committed r93616: <http://trac.webkit.org/changeset/93616>
Comment 16 Peter Kasting 2011-08-23 13:31:51 PDT
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.
Comment 17 Robert Hogan 2011-08-27 12:14:40 PDT
Created attachment 105441 [details]
Patch
Comment 18 Dave Hyatt 2011-08-30 13:59:35 PDT
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).
Comment 19 Robert Hogan 2011-09-03 11:28:37 PDT
Committed r94492: <http://trac.webkit.org/changeset/94492>
Comment 20 mitz 2011-12-16 09:11:07 PST
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.
Comment 21 Robert Hogan 2011-12-16 12:50:37 PST
(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.
Comment 22 mitz 2011-12-16 13:01:03 PST
(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.