:after selector displays in wrong place with nested div

[Marius]

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.

[Valters Švābe]

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.

[Valters Švābe]

Created attachment 46523 [details]
The patch.

[Valters Švābe]

Created attachment 46525 [details]
fix the changelog

[Dave Hyatt]

Comment on attachment 46525 [details]
fix the changelog

afterChild is a very confusing name.  Can you just rename it to lastRenderer or something?

[Valters Švābe]

Sure.

[Valters Švābe]

Created attachment 46605 [details]
Patch v3

[Dave Hyatt]

Comment on attachment 46605 [details]
Patch v3

r=me

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[Valters Švābe]

Created attachment 46730 [details]
Patch v4

Adds test to the skipped list for platforms that need result files added.

[Valters Švābe]

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..

[Eric Seidel (no email)]

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.

[Adam Barth]

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.)

[Valters Švābe]

Created attachment 50349 [details]
Patch v5.

Updated patch to make the test platform-independent. The already reviewed code changes stay the same.

[Darin Adler]

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?

[Valters Švābe]

Created attachment 50357 [details]
Patch v6

Updated patch to address the last comment.

[Darin Adler]

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

[Valters Švābe]

(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.

[WebKit Commit Bot]

Comment on attachment 50357 [details]
Patch v6

Clearing flags on attachment: 50357

Committed r56033: <http://trac.webkit.org/changeset/56033>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.