Bug 32288 - :after selector displays in wrong place with nested div
Summary: :after selector displays in wrong place with nested div
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://dl.dropbox.com/u/87458/webkit-...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-08 14:30 PST by Marius
Modified: 2010-03-15 21:41 PDT (History)
3 users (show)

See Also:


Attachments
The patch. (4.74 KB, patch)
2010-01-13 16:12 PST, Valters Švābe
no flags Details | Formatted Diff | Diff
fix the changelog (4.73 KB, patch)
2010-01-13 16:30 PST, Valters Švābe
hyatt: review-
Details | Formatted Diff | Diff
Patch v3 (4.75 KB, patch)
2010-01-14 14:34 PST, Valters Švābe
no flags Details | Formatted Diff | Diff
Patch v4 (6.14 KB, patch)
2010-01-15 20:07 PST, Valters Švābe
abarth: review-
Details | Formatted Diff | Diff
Patch v5. (4.35 KB, patch)
2010-03-09 14:24 PST, Valters Švābe
no flags Details | Formatted Diff | Diff
Patch v6 (4.31 KB, patch)
2010-03-09 15:34 PST, Valters Švābe
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marius 2009-12-08 14:30:19 PST
Consider the following HTML:

  <style>
    .t:before { content: '{'; }
    .t:after { content: '}'; }
  </style>
  <div id="t1" class="t">1
    <div id="t2" class="t">2</div>
    <div id="t3" class="t">3</div>
  </div>


Expected behavior: the '}' for the <div id="t1"> should appear after the end of <div id="t3">:
{1
{2}
{3}
}

Actual behavior: the '}' for the <div id="t1"> appears between the end of <div id="t2"> and the beginning of <div id="t3">.
{1
{2}
}
{3}

I have reproduced this with both Safari 4.0.4 and Google Chrome 4.0.249.30.
Comment 1 Valters Švābe 2010-01-13 16:09:30 PST
If a block element a) contains more than one child block element, and b) has :after generated content which is inline, then the generated content is displayed at the wrong place.

Reason - if :after content is contained in an anonymous RenderBlock, then RenderBlock::addChild doesn't notice it and appends a new child block _after_ the :after-generated content.

Proposed patch adds a check for this case.
Comment 2 Valters Švābe 2010-01-13 16:12:13 PST
Created attachment 46523 [details]
The patch.
Comment 3 Valters Švābe 2010-01-13 16:30:42 PST
Created attachment 46525 [details]
fix the changelog
Comment 4 Dave Hyatt 2010-01-14 14:06:49 PST
Comment on attachment 46525 [details]
fix the changelog

afterChild is a very confusing name.  Can you just rename it to lastRenderer or something?
Comment 5 Valters Švābe 2010-01-14 14:32:59 PST
Sure.
Comment 6 Valters Švābe 2010-01-14 14:34:10 PST
Created attachment 46605 [details]
Patch v3
Comment 7 Dave Hyatt 2010-01-14 14:47:25 PST
Comment on attachment 46605 [details]
Patch v3

r=me
Comment 8 WebKit Commit Bot 2010-01-14 18:27:48 PST
Comment on attachment 46605 [details]
Patch v3

Rejecting patch 46605 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11934 test cases.
fast/css-generated-content/block-after.html -> new (results generated in /Users/eseidel/Projects/CommitQueue/LayoutTests/platform/mac/fast/css-generated-content)

Exiting early after 1 failures. 5062 tests run.
74.57s total testing time

5061 test cases (99%) succeeded
1 test case (<1%) was new
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/187789
Comment 9 Eric Seidel (no email) 2010-01-14 18:44:18 PST
Looks like this is missing Mac results.  We can't make the bots red while landing, so you'll need to add the new test to the skipped list of every platform which needs results added.

It's unclear to me if this really needs platform specific results.  an AHEM based test shouldn't need platform specific results.
Comment 10 Valters Švābe 2010-01-15 20:07:51 PST
Created attachment 46730 [details]
Patch v4

Adds test to the skipped list for platforms that need result files added.
Comment 11 Valters Švābe 2010-01-15 23:50:16 PST
Regarding the comment about the platform specific results of the test.. I'm not sure what an Ahem based, platform independent test for this case should look like..

I can't use plaintext output, because css generated content (as far as I can tell) can't be accessed from javascript and it does not show up in plaintext output. Also, I can't just make all of the text use Ahem font, because I assume that tests are required to be human-readable / easy to inspect visually. The other tests in fast/css-generated-content also seem to be using platform specific rendering tree dumps as results..
Comment 12 Eric Seidel (no email) 2010-02-08 15:09:36 PST
Comment on attachment 46605 [details]
Patch v3

Cleared David Hyatt's review+ from obsolete attachment 46605 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 13 Adam Barth 2010-03-08 10:43:25 PST
Comment on attachment 46730 [details]
Patch v4

dhyatt has already r+ed this code change, but the testing here is insufficient.  Please either create a platform-independent test or generate baselines for the other platforms.  (The baseline requirement is like a tax on having a platform-specific test.)
Comment 14 Valters Švābe 2010-03-09 14:24:57 PST
Created attachment 50349 [details]
Patch v5.

Updated patch to make the test platform-independent. The already reviewed code changes stay the same.
Comment 15 Darin Adler 2010-03-09 14:45:50 PST
Comment on attachment 50349 [details]
Patch v5.

> +        else if (lastRenderer && lastRenderer->isAnonymousBlock() && isAfterContent(lastRenderer->lastChild())) {
> +            addChild(newChild, lastRenderer->lastChild());
> +            return;
> +        }

Since this is the addChild function, this is a loop written as recursion. Maybe we should instead make the loop explicit?
Comment 16 Valters Švābe 2010-03-09 15:34:46 PST
Created attachment 50357 [details]
Patch v6

Updated patch to address the last comment.
Comment 17 Darin Adler 2010-03-09 15:36:51 PST
Comment on attachment 50357 [details]
Patch v6

> +        if (isAfterContent(lastRenderer))
> +            beforeChild = lastRenderer;
> +        else if (lastRenderer && lastRenderer->isAnonymousBlock() && isAfterContent(lastRenderer->lastChild()))
> +            beforeChild = lastRenderer->lastChild();

Is there ever a case with more than one level of anonymous block? If so, we might need a loop instead.

r=me
Comment 18 Valters Švābe 2010-03-09 17:01:39 PST
(In reply to comment #17)
> (From update of attachment 50357 [details])
> > +        if (isAfterContent(lastRenderer))
> > +            beforeChild = lastRenderer;
> > +        else if (lastRenderer && lastRenderer->isAnonymousBlock() && isAfterContent(lastRenderer->lastChild()))
> > +            beforeChild = lastRenderer->lastChild();
> 
> Is there ever a case with more than one level of anonymous block? If so, we
> might need a loop instead.
> 
> r=me

AFAICT there isn't. Anyway, these lines don't cause any further searching down the tree. After beforeChild is set, it is clear where to insert newChild.
Comment 19 WebKit Commit Bot 2010-03-15 21:41:34 PDT
Comment on attachment 50357 [details]
Patch v6

Clearing flags on attachment: 50357

Committed r56033: <http://trac.webkit.org/changeset/56033>
Comment 20 WebKit Commit Bot 2010-03-15 21:41:41 PDT
All reviewed patches have been landed.  Closing bug.