Bug 36505 – Generated run-in Content is Mistakenly Getting Deleted

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Created attachment 51452 [details]
[TEST] This Just Adds a Dynamic Style Update to the Existing "fast/runin/generated.html" testgenerated

Generated Content, such as `div:before { content: "generated"; display: run-in }` is mistakenly getting deleted on an update to the content it was "run into" on later layouts. Test case attached.

[Joseph Pecoraro]

Created attachment 51456 [details]
[PATCH] Proposed Fix

Comment on attachment 51456 [details]
[PATCH] Proposed Fix

I am going to review the code change soon, but I already have a comment about the test.

> +// Force a second, dynamic layout
> +setTimeout(function() {
> +    document.getElementById('inner').style.color = 'blue';
> +}, 0);

This is not a reliable (because without using LayoutTestController’s waitUntilDone()/notifyDone() the test may end before the timer fires) nor efficient way to do this. The standard way to force layout is to query a DOM property that depends on layout. Tests commonly just include this statement:
    document.body.offsetTop;
which forces layout. Then you do your style change, then when the document is done loading, it forces another layout, then the final render tree is dumped.

[Joseph Pecoraro]

(In reply to comment #2)
> This is not a reliable (because without using LayoutTestController’s
> waitUntilDone()/notifyDone() the test may end before the timer fires) nor
> efficient way to do this. The standard way to force layout is to query a DOM
> property that depends on layout. Tests commonly just include this statement:
>     document.body.offsetTop;
> which forces layout. Then you do your style change, then when the document is
> done loading, it forces another layout, then the final render tree is dumped.

Ahh, thanks. I should have realized this. I corrected the test and expected results.

[Joseph Pecoraro]

Created attachment 51513 [details]
[PATCH] Fixed Test and Expected Results

Note that the run-in content is not blue, even though it gets injected into div#inner. I believe this is the expected behavior, but I could be wrong.

Comment on attachment 51513 [details]
[PATCH] Fixed Test and Expected Results

I think this breaks the case identical to your test case except you set the style on the "target" element. It looks like in that case, with your patch, you will get two copies of “PASS”. You need to somehow distinguish between the case where the run-in is :before content from an ancestor and the case where it is your own. I don’t know exactly how you would do that, but I think beforeAfterContainer() is the function that should resolve that.

Since this patch wasn’t up for review, I will not r- it :-)

[Joseph Pecoraro]

(In reply to comment #5)
> (From update of attachment 51513 [details])
> I think this breaks the case identical to your test case except you set the
> style on the "target" element. It looks like in that case, with your patch, you
> will get two copies of “PASS”. You need to somehow distinguish between the case
> where the run-in is :before content from an ancestor and the case where it is
> your own. I don’t know exactly how you would do that, but I think
> beforeAfterContainer() is the function that should resolve that.

You're correct. Thanks for the pointers!

[Joseph Pecoraro]

Created attachment 51565 [details]
[PATCH] Addressed Comments - New Test

This patch has two tests. The one from before and the one from Dan's comment about (about updating the style on #target causing duplicate content).  The comment in the code describes how I handle the situation.

Hyatt mentioned on IRC I should probably break this up into two patches. I can do that if you want!

[Joseph Pecoraro]

hyatt's comments on IRC were:

> (1) teach the parent block not to be confused by a child run-in
> that is inline (and thus belongs to the parent block's parent)
> (2) teach the grandparent block how to find and update a successfully
> run-in child in one of its child blocks

> your bug fix is very close to fixing (1) but needs to check for inline
> display (inline renderobject type) also (2) is a separate bug and
> could be filed on its own fixing (1) won't make (2) any worse

I put them together because I believe fixing (1) did cause (2) to get worse (the duplicate rendering still happened). So I came up with the above solution, which worked as I expected it too in all the cases I could come up with (adding extra block elements, floaters, etc).

[Joseph Pecoraro]

Created attachment 51687 [details]
[PATCH] Addressed List Marker Comments - Another Test

This addressed Hyatt's comments about skipping list markers. Indeed that was necessary, and the new test (generated4.html) checks for this.

[Joseph Pecoraro]

Just a heads up, besides correctly handling listMarkers I also made an optimization:

> -        while (first && (!first->isRenderBlock() || first->isFloatingOrPositioned()))
> +        if (!first->isRenderBlock())
> +            return 0;
> +        while (first && first->isFloatingOrPositioned())

Render block sibling should only be render blocks. So I can check once (before the loop) instead of every iteration. That is what the original isRunIn() handling code did. Also, a null check for first is handled before.

[Dave Hyatt]

Comment on attachment 51687 [details]
[PATCH] Addressed List Marker Comments - Another Test

r=me

[Joseph Pecoraro]

Committed r56641
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderObjectChildList.cpp
	A	LayoutTests/platform/mac/fast/runin/generated4-expected.txt
	A	LayoutTests/platform/mac/fast/runin/generated2-expected.txt
	A	LayoutTests/platform/mac/fast/runin/generated3-expected.txt
	A	LayoutTests/fast/runin/generated2.html
	A	LayoutTests/fast/runin/generated3.html
	A	LayoutTests/fast/runin/generated4.html
	M	LayoutTests/ChangeLog
r56641 = 78029fb8a2fe47e969b9f03e90fccc9f5117245a (trunk)
http://trac.webkit.org/changeset/56641

All the bots so far have produced the correct expected results!
Windows bots haven't finished yet, but I will close this.