Bug 80647

Created attachment 130920 [details] Patch

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

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.

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.

Created attachment 132310 [details] Patch

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

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

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

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

Created attachment 132324 [details] Patch Add missing space so decleration of updateFixedElementsAfterScrolling matches repaintFixedElementsAfterScrolling without breaking style guidelines

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

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

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

(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

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

Created attachment 132369 [details] Patch Move layout test text outside of the dumped area and disable render tree dumps

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

Created attachment 132503 [details] Patch Use fast/repaint/resources/repaint.js

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

Comment on attachment 132503 [details] Patch Clearing flags on attachment: 132503 Committed r111139: <http://trac.webkit.org/changeset/111139>

All reviewed patches have been landed. Closing bug.