Bug 14134 - REGRESSION (r25353): Whitespace nodes ignored between inline list items
Summary: REGRESSION (r25353): Whitespace nodes ignored between inline list items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://cfunited.com/
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-06-13 22:37 PDT by Elliott Sprehn
Modified: 2011-03-02 16:09 PST (History)
6 users (show)

See Also:


Attachments
Screenshot of incorrect tab link behavior (6.25 KB, image/png)
2007-09-09 10:43 PDT, David Kilzer (:ddkilzer)
no flags Details
I believe this is a test file for the same error you are seeing. (3.99 KB, text/html)
2007-11-14 07:20 PST, Barry Rader
no flags Details
Partial reduction (742 bytes, text/html)
2007-12-18 23:29 PST, David Kilzer (:ddkilzer)
no flags Details
Reduction (241 bytes, text/html)
2007-12-18 23:48 PST, David Kilzer (:ddkilzer)
no flags Details
Reduction (373 bytes, text/html)
2007-12-31 11:54 PST, mitz
no flags Details
Reduction (392 bytes, text/html)
2007-12-31 12:32 PST, mitz
no flags Details
Simpler test case (361 bytes, text/html)
2007-12-31 13:36 PST, mitz
no flags Details
Ensure that whitespace-only text nodes get a renderer when they need one (60.30 KB, patch)
2007-12-31 17:04 PST, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2007-06-13 22:37:16 PDT
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).
Comment 1 Mark Rowe (bdash) 2007-06-14 00:16:28 PDT
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.
Comment 2 Adele Peterson 2007-08-30 15:05:31 PDT
It seems this problem no longer exists in the latest version of WebKit.  Please reopen if you can still reproduce.
Comment 3 Elliott Sprehn 2007-09-09 10:21:34 PDT
I'm still seeing this even with the most recent nightly. (r25449)
Comment 4 David Kilzer (:ddkilzer) 2007-09-09 10:36:34 PDT
(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!
Comment 5 David Kilzer (:ddkilzer) 2007-09-09 10:43:08 PDT
Created attachment 16233 [details]
Screenshot of incorrect tab link behavior
Comment 6 Elliott Sprehn 2007-09-09 11:19:29 PDT
(In reply to comment #5)
> Created an attachment (id=16233) [edit]
> Screenshot of incorrect tab link behavior
> 

Looks like you beat me to it.
Comment 7 David Kilzer (:ddkilzer) 2007-09-09 11:55:22 PDT
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).

Comment 8 Antti Koivisto 2007-09-09 17:27:47 PDT
r25353 is unlikely to really cause this bug but it may have exposed problem somewhere else. 
Comment 9 Barry Rader 2007-11-14 07:20:57 PST
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.
Comment 10 David Kilzer (:ddkilzer) 2007-12-18 10:06:40 PST
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.

Comment 11 David Kilzer (:ddkilzer) 2007-12-18 10:09:05 PST
(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].

Comment 12 Elliott Sprehn 2007-12-18 21:13:07 PST
(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>
Comment 13 David Kilzer (:ddkilzer) 2007-12-18 22:20:14 PST
(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!

Comment 14 David Kilzer (:ddkilzer) 2007-12-18 22:41:21 PST
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.
Comment 15 David Kilzer (:ddkilzer) 2007-12-18 23:29:14 PST
Created attachment 17983 [details]
Partial reduction

A partial reduction of the original page.

This one is really strange!
Comment 16 David Kilzer (:ddkilzer) 2007-12-18 23:48:25 PST
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;").
Comment 17 David Kilzer (:ddkilzer) 2007-12-18 23:54:53 PST
(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.

Comment 18 mitz 2007-12-18 23:56:46 PST
(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!
Comment 19 David Kilzer (:ddkilzer) 2007-12-19 00:10:33 PST
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. 
Comment 20 mitz 2007-12-19 01:16:48 PST
<rdar://problem/5655160>
Comment 21 mitz 2007-12-31 11:54:54 PST
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.
Comment 22 mitz 2007-12-31 12:32:23 PST
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.
Comment 23 mitz 2007-12-31 13:36:35 PST
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.
Comment 24 mitz 2007-12-31 17:04:38 PST
Created attachment 18221 [details]
Ensure that whitespace-only text nodes get a renderer when they need one
Comment 25 Darin Adler 2007-12-31 17:16:16 PST
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
Comment 26 mitz 2007-12-31 17:32:31 PST
Fixed in <http://trac.webkit.org/projects/webkit/changeset/29054>.
Comment 27 Eric Seidel (no email) 2011-03-02 12:30:45 PST
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.
Comment 28 Maciej Stachowiak 2011-03-02 16:09:14 PST
(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.