Bug 36505 - Generated run-in Content is Mistakenly Getting Deleted
Summary: Generated run-in Content is Mistakenly Getting Deleted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-23 14:17 PDT by Joseph Pecoraro
Modified: 2010-03-26 14:42 PDT (History)
3 users (show)

See Also:


Attachments
[TEST] This Just Adds a Dynamic Style Update to the Existing "fast/runin/generated.html" testgenerated (331 bytes, text/html)
2010-03-23 14:17 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (4.15 KB, patch)
2010-03-23 14:54 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Fixed Test and Expected Results (4.25 KB, patch)
2010-03-24 09:22 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Addressed Comments - New Test (8.58 KB, patch)
2010-03-24 16:36 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Addressed List Marker Comments - Another Test (11.76 KB, patch)
2010-03-25 15:59 PDT, Joseph Pecoraro
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2010-03-23 14:17:50 PDT
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.
Comment 1 Joseph Pecoraro 2010-03-23 14:54:51 PDT
Created attachment 51456 [details]
[PATCH] Proposed Fix
Comment 2 mitz 2010-03-24 08:44:51 PDT
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.
Comment 3 Joseph Pecoraro 2010-03-24 09:20:03 PDT
(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.
Comment 4 Joseph Pecoraro 2010-03-24 09:22:06 PDT
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 5 mitz 2010-03-24 10:29:51 PDT
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 :-)
Comment 6 Joseph Pecoraro 2010-03-24 10:47:43 PDT
(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!
Comment 7 Joseph Pecoraro 2010-03-24 16:36:22 PDT
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!
Comment 8 Joseph Pecoraro 2010-03-24 16:45:23 PDT
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).
Comment 9 Joseph Pecoraro 2010-03-25 15:59:25 PDT
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.
Comment 10 Joseph Pecoraro 2010-03-25 16:04:26 PDT
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.
Comment 11 Dave Hyatt 2010-03-26 12:51:18 PDT
Comment on attachment 51687 [details]
[PATCH] Addressed List Marker Comments - Another Test

r=me
Comment 12 Joseph Pecoraro 2010-03-26 14:42:38 PDT
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.