Invalidation issue when creating backing layer for fixed positioned element.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

Root layer is not properly invalidated when layer backing is created for fixed positioned element as a result of scrolling.

Chromium bugs:
http://code.google.com/p/chromium-os/issues/detail?id=27225
http://code.google.com/p/chromium/issues/detail?id=116436

[David Reveman]

Created attachment 130920 [details]
Patch

[Build Bot]

Comment on attachment 130920 [details]
Patch

Attachment 130920 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11949264

[David Reveman]

The problem is that the root layer area for fixed positioned elements is not invalidated when a backing layer is created for the element as a result of a ScrollView::scrollTo call.

This happens when the fixed positioned element is not overlapping a compositor layer prior to the ScrollView::scrollTo call. If the ScrollView::scrollTo call causes the fixed positioned element to overlap a compositor layer, a backing layer needs to be created for it as it can't otherwise be displayed above the compositor layer. The area of the root layer where the fixed positioned element was previously located need to be invalidated when this happens and that is currently not always taking place. The location of the fixed positioned element prior to the scrollTo call is instead invalidated. By moving the creation of the backing layer to after we update fixed positioned elements I avoid this issue.

The layout test included with the patch should make it clear what's going on.

Not sure that the patch is the proper solution. I don't know this code well enough.

[Simon Fraser (smfr)]

Comment on attachment 130920 [details]
Patch

Patch looks OK but it would be better if the testcase were changed to more obviously pass or fail in visual inspection. e.g. use 200x200px green and red boxes, and make it so that no red is visible on success, but is on failure.

[David Reveman]

Created attachment 132310 [details]
Patch

[David Reveman]

(In reply to comment #4)
> (From update of attachment 130920 [details])
> Patch looks OK but it would be better if the testcase were changed to more obviously pass or fail in visual inspection. e.g. use 200x200px green and red boxes, and make it so that no red is visible on success, but is on failure.

Good idea, I've uploaded new patch with an improved layout test.

Btw, the layout test doesn't always fail unless --run-singly is used. Is there a way to mark a test as needing to be run in a separate DumpRenderTree?

[Simon Fraser (smfr)]

Comment on attachment 132310 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132310&action=review

> Source/WebCore/platform/ScrollView.h:366
>      virtual void repaintFixedElementsAfterScrolling() {}
> +    virtual void updateFixedElementsAfterScrolling() { }

No space between the parens (match the line above).

[Simon Fraser (smfr)]

> Btw, the layout test doesn't always fail unless --run-singly is used.

Hmm, this indicates that the test should use waitUntilDone() with a short setTimeout(). Please try to fix this before landing.

> Is there a way to mark a test as needing to be run in a separate DumpRenderTree?

No.

[Adrienne Walker]

(In reply to comment #8)
> > Btw, the layout test doesn't always fail unless --run-singly is used.
> 
> Hmm, this indicates that the test should use waitUntilDone() with a short setTimeout(). Please try to fix this before landing.

I'm confused--how will that fix things? What state needs to be cleared between tests to make this fail consistently?

[David Reveman]

Created attachment 132324 [details]
Patch

Add missing space so decleration of updateFixedElementsAfterScrolling matches repaintFixedElementsAfterScrolling without breaking style guidelines

[Shawn Singh]

Just a side comment - in the past I was suggested not to include text on the pixel tests... I guess that applies here, too =)

[David Reveman]

(In reply to comment #9)
> (In reply to comment #8)
> > > Btw, the layout test doesn't always fail unless --run-singly is used.
> > 
> > Hmm, this indicates that the test should use waitUntilDone() with a short setTimeout(). Please try to fix this before landing.
> 
> I'm confused--how will that fix things? What state needs to be cleared between tests to make this fail consistently?

I get a significantly different set of invalidation rectangles when a test has been run prior to this test in the same DumpRenderTree instance. I haven't looked at the details of why that is. I assume that is expected.

These invalidation differences are enough to prevent the test from failing as it relies on a series of invalidations and repaints happening in a specific way.

I'm not sure how to make the invalidations predictable without relying on a fresh DumpRenderTree instance being used. Any ideas?

The bug was tricky to reproduce in a layout test but it's not hard to reproduce in a normal browser instance. Load up the html test file, scroll up and down a bit and you should quickly see some artifacts.

[David Reveman]

(In reply to comment #11)
> Just a side comment - in the past I was suggested not to include text on the pixel tests... I guess that applies here, too =)

ok, someone needs to update http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree in that case.

[James Robinson]

(In reply to comment #13)
> (In reply to comment #11)
> > Just a side comment - in the past I was suggested not to include text on the pixel tests... I guess that applies here, too =)
> 
> ok, someone needs to update http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree in that case.

http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Fonts

[David Reveman]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Just a side comment - in the past I was suggested not to include text on the pixel tests... I guess that applies here, too =)
> > 
> > ok, someone needs to update http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree in that case.
> 
> http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Fonts

oops, someone needs to read not just part of a tutorial :)

I'll update the test.

[David Reveman]

Created attachment 132369 [details]
Patch

Move layout test text outside of the dumped area and disable render tree dumps

[James Robinson]

Comment on attachment 132369 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132369&action=review

R=me

> LayoutTests/compositing/layer-creation/fixed-position-scroll.html:51
> +          }, 500); // Needs to be enough for repaint to happen.

use fast/repaint/resources/repaint.js and you won't need to do all this by hand or duplicate the test logic

[David Reveman]

Created attachment 132503 [details]
Patch

Use fast/repaint/resources/repaint.js

[James Robinson]

Comment on attachment 132503 [details]
Patch

R=me on the tests, carrying Simon Fraser's review+ forward on the code (sadly no way to do that in the ChangeLogs while using the commit queue, I'm afraid).

[WebKit Review Bot]

Comment on attachment 132503 [details]
Patch

Clearing flags on attachment: 132503

Committed r111139: <http://trac.webkit.org/changeset/111139>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.