Bug 101303 - Fixed position elements that are out of view still end up forcing non-threaded scrolling
Summary: Fixed position elements that are out of view still end up forcing non-threade...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
: 98144 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-11-05 22:22 PST by Beth Dakin
Modified: 2012-12-06 11:10 PST (History)
12 users (show)

See Also:


Attachments
Patch (3.96 KB, patch)
2012-11-06 13:49 PST, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (7.57 KB, patch)
2012-11-06 17:29 PST, Beth Dakin
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (10.03 KB, patch)
2012-11-07 11:38 PST, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-11-05 22:22:57 PST
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;
Comment 1 Radar WebKit Bug Importer 2012-11-05 22:23:46 PST
<rdar://problem/12642222>
Comment 2 Beth Dakin 2012-11-06 13:49:05 PST
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?
Comment 3 Simon Fraser (smfr) 2012-11-06 14:04:22 PST
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.
Comment 4 Beth Dakin 2012-11-06 14:58:38 PST
(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.
Comment 5 Beth Dakin 2012-11-06 17:29:53 PST
Created attachment 172682 [details]
Patch
Comment 6 Simon Fraser (smfr) 2012-11-06 17:39:33 PST
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?
Comment 7 EFL EWS Bot 2012-11-06 19:30:14 PST
Comment on attachment 172682 [details]
Patch

Attachment 172682 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14757235
Comment 8 Beth Dakin 2012-11-07 11:38:52 PST
Created attachment 172850 [details]
Patch
Comment 9 Beth Dakin 2012-11-07 13:42:45 PST
My most recent patch resolved https://bugs.webkit.org/show_bug.cgi?id=98144 as well.
Comment 10 Simon Fraser (smfr) 2012-11-07 13:56:27 PST
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.
Comment 11 Beth Dakin 2012-11-07 14:28:19 PST
(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
Comment 12 Beth Dakin 2012-11-07 14:29:03 PST
*** Bug 98144 has been marked as a duplicate of this bug. ***
Comment 13 Alexey Proskuryakov 2012-11-16 13:44:53 PST
This has caused bug 102144.
Comment 14 Xianzhu Wang 2012-11-16 16:27:23 PST
*** Bug 98144 has been marked as a duplicate of this bug. ***
Comment 15 John Knottenbelt 2012-11-21 10:07:31 PST
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.
Comment 16 Alexey Proskuryakov 2012-12-06 10:55:51 PST
This has also caused bug 104276.
Comment 17 Alexey Proskuryakov 2012-12-06 10:59:07 PST
Beth, will you have a chance to look into the regressions soon?
Comment 18 Beth Dakin 2012-12-06 11:08:45 PST
(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.
Comment 19 Beth Dakin 2012-12-06 11:10:11 PST
(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.