Bug 101303 – Fixed position elements that are out of view still end up forcing non-threaded scrolling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

http://trac.webkit.org/changeset/133536 made it so that pages with position:fixed elements can scroll on the scrolling thread. However, pages with out-of-viewport fixed position elements are still failing to scroll on the scrolling thread because we don't create layers for the out-of-view elements.

This issues is tracked by these tests:
platform/mac/tiled-drawing/fixed/fixed-position-out-of-view.html
platform/mac/tiled-drawing/fixed/fixed-position-out-of-view-negative-zindex.html

If you comment out these lines in RenderLayerCompositor::requiresCompositingForPosition(), then we do create the layers and the bug goes away:
    // Fixed position elements that are invisible in the current view don't get their own layer.
    FrameView* frameView = m_renderView->frameView();
    if (frameView && !layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize())))
        return false;

[Radar WebKit Bug Importer]

<rdar://problem/12642222>

[Beth Dakin]

Created attachment 172642 [details]
Patch

Here's one simple way to fix this bug. Simon, I am wondering if you think this is okay or if you have another fix in mind?

[Simon Fraser (smfr)]

Comment on attachment 172642 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:279
> +        bool layerIsInsideViewport = layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()));

Checking absoluteBoundingBox() isn't sufficient: you can have a 0x0 fixedpos with an abspos child.

Is hasNonLayerFixedObjects() called after every layout, so it can update at the right time?

I also think this should share some code with RenderLayerCompositor::requiresCompositingForPosition(), since if that's correct, we should never have a visible fixedpos element that is visible.

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 172642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172642&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:279
> > +        bool layerIsInsideViewport = layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()));
> 

Ah, yes, good call.

> Checking absoluteBoundingBox() isn't sufficient: you can have a 0x0 fixedpos with an abspos child.
> 
> Is hasNonLayerFixedObjects() called after every layout, so it can update at the right time?
> 

Yes.

> I also think this should share some code with RenderLayerCompositor::requiresCompositingForPosition(), since if that's correct, we should never have a visible fixedpos element that is visible.

Yes, I will look into that.

[Beth Dakin]

Created attachment 172682 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 172682 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1937
> +bool RenderLayerCompositor::layerIsOutsideView(const RenderLayer* layer) const
> +{
> +    IntRect boundingBox = layer->absoluteBoundingBox();
> +    return !boundingBox.isEmpty() && !layerIsVisibleInView(layer);
> +}
> +
> +bool RenderLayerCompositor::layerIsVisibleInView(const RenderLayer* layer) const
> +{
> +    // FIXME: This test should be much more sophisticated and consider descendants.
> +    // https://bugs.webkit.org/show_bug.cgi?id=98144
> +    FrameView* frameView = m_renderView->frameView();
> +    return frameView && layer->absoluteBoundingBox().intersects(IntRect(IntPoint(frameView->scrollOffsetForFixedPosition()), frameView->layoutSize()));
> +}

I'm confused about why we need both of these?

[EFL EWS Bot]

Comment on attachment 172682 [details]
Patch

Attachment 172682 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14757235

[Beth Dakin]

Created attachment 172850 [details]
Patch

[Beth Dakin]

My most recent patch resolved https://bugs.webkit.org/show_bug.cgi?id=98144 as well.

[Simon Fraser (smfr)]

Comment on attachment 172850 [details]
Patch

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

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:95
> +    virtual bool hasNonLayerFixedObjects(FrameView*) const { return false; }

I think this function name is wrong. I think it should be hasVisibleSlowRepaintFixedObjects() or something.

> LayoutTests/platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html:40
> +          if (window.internals) {
> +            document.getElementById('results').innerText = internals.scrollingStateTreeAsText(document);
> +            testRunner.notifyDone();

I think it would also be good for this test to dump the layer tree, since you're changing compositing behavior. Or add another test under compositing/ that does this, and isn't tied to tiled drawing.

[Beth Dakin]

(In reply to comment #10)
> (From update of attachment 172850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172850&action=review
> 
> > Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:95
> > +    virtual bool hasNonLayerFixedObjects(FrameView*) const { return false; }
> 
> I think this function name is wrong. I think it should be hasVisibleSlowRepaintFixedObjects() or something.
> 
> > LayoutTests/platform/mac/tiled-drawing/fixed/absolute-inside-out-of-view-fixed.html:40
> > +          if (window.internals) {
> > +            document.getElementById('results').innerText = internals.scrollingStateTreeAsText(document);
> > +            testRunner.notifyDone();
> 
> I think it would also be good for this test to dump the layer tree, since you're changing compositing behavior. Or add another test under compositing/ that does this, and isn't tied to tiled drawing.

Thanks, Simon! I addressed both of these things. http://trac.webkit.org/changeset/133807

[Beth Dakin]

*** Bug 98144 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

This has caused bug 102144.

[Xianzhu Wang]

*** Bug 98144 has been marked as a duplicate of this bug. ***

[John Knottenbelt]

I noticed that this patch has also introduced an assertion failure while running the ACID3 CSS tests:

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r135405%20%283886%29/results.html

( Also reported in https://bugs.webkit.org/show_bug.cgi?id=102599 )

I tried the test without this patch reverted and it worked fine. Any ideas on how to fix much appreciated.

[Alexey Proskuryakov]

This has also caused bug 104276.

[Alexey Proskuryakov]

Beth, will you have a chance to look into the regressions soon?

[Beth Dakin]

(In reply to comment #17)
> Beth, will you have a chance to look into the regressions soon?

I have been working on a reduction for one of them this yes. So, yes.

[Beth Dakin]

(In reply to comment #18)
> (In reply to comment #17)
> > Beth, will you have a chance to look into the regressions soon?
> 
> I have been working on a reduction for one of them this yes. So, yes.

*This week.