Bug 14134 – REGRESSION (r25353): Whitespace nodes ignored between inline list items

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

[Elliott Sprehn]

Sometimes when loading this page (or any of the pages in the navigation), the menu renders with the links right up against each other and no space between their borders.

Other times the page renders correctly, with space between them, as there's a new line between each <li>.

I can't seem to figure out what conditions cause this. The page itself and the css are identical on every request, it just renders differently sometimes.

This doesn't happen in Safari 2.

Reproducible in the latest nightly  (r22084, 10 June 2007).

[Mark Rowe (bdash)]

Confirmed with ToT.  I can't see any obvious reason for the different layout after spending a few minutes poking around the relevant portions with the web inspector.

[Adele Peterson]

It seems this problem no longer exists in the latest version of WebKit.  Please reopen if you can still reproduce.

[Elliott Sprehn]

I'm still seeing this even with the most recent nightly. (r25449)

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> I'm still seeing this even with the most recent nightly. (r25449)

Elliott, could you attach a screenshot of the bug (use Cmd-Shift-4 and just capture the problematic area) so we know what we're looking for?  Thanks!

[David Kilzer (:ddkilzer)]

Created attachment 16233 [details]
Screenshot of incorrect tab link behavior

[Elliott Sprehn]

(In reply to comment #5)
> Created an attachment (id=16233) [edit]
> Screenshot of incorrect tab link behavior
> 

Looks like you beat me to it.

[David Kilzer (:ddkilzer)]

This goes against the original report, but an internal autospade of built revisions (using Safari 2.0.4 (419.3)) finds:

Works: r25352  Fails: r25353

I tried testing WebKit Nightly r22084 as well, but couldn't reproduce the issue (which worries me a bit that the autospade results may not be the whole story).

[Antti Koivisto]

r25353 is unlikely to really cause this bug but it may have exposed problem somewhere else. 

[Barry Rader]

Created attachment 17268 [details]
I believe this is a test file for the same error you are seeing.

I think this test case I am attaching might be related to the same issue.

[David Kilzer (:ddkilzer)]

The web site has changed, so this may not be reproducible any more.  Here's what it looked like from the Internet Archive.

http://web.archive.org/web/20070619090529rn_1/cfunited.com/

I can't get the incorrect behavior to occur with the archived page, though.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Created an attachment (id=17268) [edit]
> I believe this is a test file for the same error you are seeing.
> 
> I think this test case I am attaching might be related to the same issue.

I don't think it's the same issue.  The cfunited.com page would wrap differently on different page loads.  This attachment clearly has a layout/rendering difference that happens much more consistently (on every page load).  Please file a new bug for Attachment #17268 [details].

[Elliott Sprehn]

(In reply to comment #10)
> The web site has changed, so this may not be reproducible any more.  Here's
> what it looked like from the Internet Archive.
> 
> http://web.archive.org/web/20070619090529rn_1/cfunited.com/
> 
> I can't get the incorrect behavior to occur with the archived page, though.
> 

I can see about getting a link to the original page.

This issue also happens on the <http://cfunited.com/go/schedule> schedule page and others with the year links down the side. Sometimes the years are right up against each other, but others there's space between them.

There is also,a new issue that might be related. The login box renders too tall sometimes on page loads. On first load the "Forgot Password" button will be below the year select drop down, but if you click a link then it'll show up correctly next to the "Login" button.

<http://cfunited.com>

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> I can see about getting a link to the original page.
> 
> This issue also happens on the <http://cfunited.com/go/schedule> schedule page
> and others with the year links down the side. Sometimes the years are right up
> against each other, but others there's space between them.

A reduction would be most helpful, but if the above page still reproduces the issue, that's sufficient for now.  Thanks!

> There is also,a new issue that might be related. The login box renders too tall
> sometimes on page loads. On first load the "Forgot Password" button will be
> below the year select drop down, but if you click a link then it'll show up
> correctly next to the "Login" button.
> 
> <http://cfunited.com>

Please file a new bug.  Thanks!

[David Kilzer (:ddkilzer)]

Comment on attachment 17268 [details]
I believe this is a test file for the same error you are seeing.

(In reply to comment #11)
> (In reply to comment #9)
> > Created an attachment (id=17268) [edit]
> > I believe this is a test file for the same error you are seeing.
> > 
> > I think this test case I am attaching might be related to the same issue.
> 
> I don't think it's the same issue.  The cfunited.com page would wrap
> differently on different page loads.  This attachment clearly has a
> layout/rendering difference that happens much more consistently (on every page
> load).  Please file a new bug for Attachment #17268 [details] [edit].

I filed Bug 16513 for this test case.

[David Kilzer (:ddkilzer)]

Created attachment 17983 [details]
Partial reduction

A partial reduction of the original page.

This one is really strange!

[David Kilzer (:ddkilzer)]

Created attachment 17984 [details]
Reduction

Further reduction.  Strangely, the <link> tag, the <input> tag, and the <script> tag that focuses the <input> tag are all necessary to reproduce this bug!

Swapping the <link> tag for a <style> tag breaks the reduction, as does replacing the JavaScript that focuses the <input> field with something less interesting (like "var a = 1;").

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> Created an attachment (id=17984) [edit]
> Reduction

Note that you may have to hit Reload (Cmd-R) once or twice to see the bug.  On the initial load, it usually renders correctly.

(In reply to comment #16)
> Strangely, the <link> tag, the <input> tag, and the
> <script> tag that focuses the <input> tag are all necessary to reproduce this
> bug!

The <link> tag means that the <script> is executing while the document still has pending style sheets (even data: URLs are loaded asynchronously), and focusing the <input> probably triggers layout that has to ignore pending style sheets... these are apparently a very bug-friendly conditions.

Thanks for the reduction!

[David Kilzer (:ddkilzer)]

Rerunning autospade on internal build with the new reduction confirms that the regression occurred with r25353.  Note that Antti points out in Comment #8:

> r25353 is unlikely to really cause this bug but it may have exposed problem
> somewhere else. 

<rdar://problem/5655160>

Created attachment 18215 [details]
Reduction

Nothing specific to pending-style layout. This is a problem in updating layout when the display property changes from list-item to inline.

Created attachment 18216 [details]
Reduction

WebCore does not create renderers for whitespace-only text nodes in block flows (there are exceptions to this rule). When the block flow changes to an inline flow after the fact, it becomes a problem.

Created attachment 18217 [details]
Simpler test case

In this case there is no change in flow type, but again a renderer for a whitespace-only text node that was omitted at first is not created later when it is needed.

Created attachment 18221 [details]
Ensure that whitespace-only text nodes get a renderer when they need one

[Darin Adler]

Comment on attachment 18221 [details]
Ensure that whitespace-only text nodes get a renderer when they need one

+        Node* next = nextSibling();
+        while (next) {
+            if (next->renderer())
+                break;
+            if (!next->attached())
+                break;  // Assume this means none of the following siblings are attached.
+            if (next->isTextNode())
+                next->createRendererIfNeeded();
+            next = next->nextSibling();
+        }

That should be a for loop.

r=me

Fixed in <http://trac.webkit.org/projects/webkit/changeset/29054>.

[Eric Seidel (no email)]

This bug added a renderer walk in attach().  Looking at peacekeeper shark data, this accounts for some 7% of total time out of all the peacekeeper DOM benchmarks combined. :)

I suspect there is some way to limit the number of times we do this walk w/o regressing this bug?

Specifically it spends all its time checking node->renderer().  I suspect the walk often misses caches for some reason.  Still investigating.

[Maciej Stachowiak]

(In reply to comment #27)
> This bug added a renderer walk in attach().  Looking at peacekeeper shark data, this accounts for some 7% of total time out of all the peacekeeper DOM benchmarks combined. :)
> 
> I suspect there is some way to limit the number of times we do this walk w/o regressing this bug?
> 
> Specifically it spends all its time checking node->renderer().  I suspect the walk often misses caches for some reason.  Still investigating.

I don't think it is a cache miss. I think the problem is that you get O(N^2) behavior when replacing all of an element's contents using innerHTML. Note that in this case, the walk should be completely unnecessary. It's only necessary when dynamically inserting before pre-existing children.