Bug 69796 – Support creating composited layers for fixed position elements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vangelis Kokkevis]

Add an optional setting for promoting fixed position elements to become composited if they are inside a scrolling frame.

[Vangelis Kokkevis]

Created attachment 110427 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 110427 [details]
Patch

Waiting on ews results to pickup missing export symbols. I will post a new patch once I have them.

[Simon Fraser (smfr)]

You'll break z-index and clipping if you just make position:fixed do compositing.

[Adrienne Walker]

(In reply to comment #3)
> You'll break z-index and clipping if you just make position:fixed do compositing.

Why would the path in this patch be any different than a fixed position element that became composited via some other trigger?

[Simon Fraser (smfr)]

Because other triggers like -webkit-transform create stacking context. position:fixed does not.

[Gustavo Noronha (kov)]

Comment on attachment 110427 [details]
Patch

Attachment 110427 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10031026

[Vangelis Kokkevis]

(In reply to comment #3)
> You'll break z-index and clipping if you just make position:fixed do compositing.

Good catch!  Thanks, Simon. I'll update the patch and only make fixed position elements composited if they create a stacking context (e.g. have a z-index).  That still covers a number of useful cases.

[Simon Fraser (smfr)]

Or you can have position:fixed create stacking context, although that does break the web slightly.

I have a patch already for position:fixed and compositing. I'll put it up here.

[Simon Fraser (smfr)]

Actually I'll just wait until you've made another patch and make comments. I don't have any additional code to add.

[James Robinson]

I think there are 3 interesting cases:

1.) position:fixed element is a stacking context for other reasons
2.) position:fixed element is not a stacking context, and treating is as a stacking context would change the rendering
3.) position:fixed element is not a stacking context but the page would render identically is if were a stacking context.

I think (3) is actually the most common on the web - how hard would it be to detect?

[Simon Fraser (smfr)]

(In reply to comment #10)
> I think there are 3 interesting cases:
> 
> 1.) position:fixed element is a stacking context for other reasons
> 2.) position:fixed element is not a stacking context, and treating is as a stacking context would change the rendering
> 3.) position:fixed element is not a stacking context but the page would render identically is if were a stacking context.
> 
> I think (3) is actually the most common on the web - how hard would it be to detect?

I would like to do 3) for other things too (like overflow), but haven't figured out how hard it would be yet. It should be doable by analyzing the structure of the z-order tree.

[James Robinson]

I think (and double-checked with Tab) just checking for descendants of the position:fixed element that have a z-index set should be good enough to start - if there are no descendants with a z-index set, it's definitely fine to treat the element as if it established a stacking context.  If descendants do have a z-index set, it's still fine so long as the z-index values are higher than anything else in the page. We also don't have to worry about descendants of elements that establish stacking contexts of their own that are descendants of the position:fixed element.

How about we just keep a bit on RenderLayer tracking whether descendants inside this stacking context have z-index set? Should catch most cases.

[Simon Fraser (smfr)]

(In reply to comment #12)
> I think (and double-checked with Tab) just checking for descendants of the position:fixed element that have a z-index set should be good enough to start - if there are no descendants with a z-index set, it's definitely fine to treat the element as if it established a stacking context.  If descendants do have a z-index set, it's still fine so long as the z-index values are higher than anything else in the page. We also don't have to worry about descendants of elements that establish stacking contexts of their own that are descendants of the position:fixed element.

Don't forget that opacity and transform also create stacking context, so I think it's more complex than this.

> How about we just keep a bit on RenderLayer tracking whether descendants inside this stacking context have z-index set? Should catch most cases.

You're really asking whether it 'renders atomically' i think. This is hard to know, since positioned descendent are hoisted up into the z-order lists of the enclosing stacking context. You have to figure out whether the fixed thing, and its RenderLayer descendants are all adjacent siblings in that z-order list.

[Vangelis Kokkevis]

Created attachment 110600 [details]
Patch

[Vangelis Kokkevis]

Updated patch creates compositing layers only if the fixed position element has its own stacking context. From a quick survey of common pages, it appears that position:fixed is usually accompanied by a z-index which does force the creation of a new stacking context.

[Simon Fraser (smfr)]

Comment on attachment 110600 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        This CL adds the necessary WebCore setting and modifies the RLC logic to take into

"CL"?

> Source/WebCore/platform/graphics/GraphicsLayer.h:275
> +    bool fixedPositioned() const { return m_fixedPositioned; }
> +    virtual void setFixedPositioned(bool b) { m_fixedPositioned = b; }
> +
> +    // Set to true when the layer corresponds to the root layer of a Frame.
> +    bool isFrameRoot() const { return m_isFrameRoot; }
> +    virtual void setIsFrameRoot(bool b) { m_isFrameRoot = b; }

I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const

I'd call this requiresCompositingForPosition()

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> +        return false;

The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> +    // Fixed position elements get composited only when inside a frame that scrolls.
> +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> +    m_compositingDependsOnGeometry = true;
> +    if (m_renderView->needsLayout())
> +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> +
> +    ScrollView* scrollView = m_renderView->frameView();
> +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();

I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.

[Vangelis Kokkevis]

Thanks for the speedy review!

(In reply to comment #16)
> (From update of attachment 110600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110600&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        This CL adds the necessary WebCore setting and modifies the RLC logic to take into
> 
> "CL"?

CL = changelist  ... I'll remove :)
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:275
> > +    bool fixedPositioned() const { return m_fixedPositioned; }
> > +    virtual void setFixedPositioned(bool b) { m_fixedPositioned = b; }
> > +
> > +    // Set to true when the layer corresponds to the root layer of a Frame.
> > +    bool isFrameRoot() const { return m_isFrameRoot; }
> > +    virtual void setIsFrameRoot(bool b) { m_isFrameRoot = b; }
> 
> I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.
> 

To give some context about this change, the end goal of this effort is to give the compositor the ability to scroll a page/iframe without having to rely on webkit to adjust fixed position elements.  This in similar vain to being able to run animations independent of the main webkit thread.

To do that, we need some meta-information about how a layer positions itself to be stored in the GraphicsLayer. 

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> > +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const
> 
> I'd call this requiresCompositingForPosition()
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> > +        return false;
> 
> The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. 

RenderLayer::isStackingContext() returns true for layers with an explicit z-index. In CSSStyleSelector.cpp the z-index is set explicitly to 0 if the element has a transform property, mask, reflection, or opacity < 1.  It looks like it covers all the bases.
 

> Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.

Sure, will do.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> > +    // Fixed position elements get composited only when inside a frame that scrolls.
> > +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> > +    m_compositingDependsOnGeometry = true;
> > +    if (m_renderView->needsLayout())
> > +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> > +
> > +    ScrollView* scrollView = m_renderView->frameView();
> > +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();
> 
> I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.

Is the issue that I'm using the presence of scroll bars in the ScrollView to determine whether the layer is scrolling?  I'll change it to pick it up from the RenderLayer directly.

[Simon Fraser (smfr)]

(In reply to comment #17)
> Thanks for the speedy review!
> 
> (In reply to comment #16)
> > (From update of attachment 110600 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=110600&action=review
> > 
> > > Source/WebCore/ChangeLog:14
> > > +        This CL adds the necessary WebCore setting and modifies the RLC logic to take into
> > 
> > "CL"?
> 
> CL = changelist  ... I'll remove :)
> > 
> > > Source/WebCore/platform/graphics/GraphicsLayer.h:275
> > > +    bool fixedPositioned() const { return m_fixedPositioned; }
> > > +    virtual void setFixedPositioned(bool b) { m_fixedPositioned = b; }
> > > +
> > > +    // Set to true when the layer corresponds to the root layer of a Frame.
> > > +    bool isFrameRoot() const { return m_isFrameRoot; }
> > > +    virtual void setIsFrameRoot(bool b) { m_isFrameRoot = b; }
> > 
> > I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.
> > 
> 
> To give some context about this change, the end goal of this effort is to give the compositor the ability to scroll a page/iframe without having to rely on webkit to adjust fixed position elements.  This in similar vain to being able to run animations independent of the main webkit thread.
> 
> To do that, we need some meta-information about how a layer positions itself to be stored in the GraphicsLayer. 

I understand, but I think you should store this information outside of GraphicsLayer, or phrase it in such a way that it's layout-agnostic.

iOS5 now supports compositing for position:fixed, but it doesn't need to touch GraphicsLayer at all for this.

> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> > > +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const
> > 
> > I'd call this requiresCompositingForPosition()
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> > > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> > > +        return false;
> > 
> > The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. 
> 
> RenderLayer::isStackingContext() returns true for layers with an explicit z-index. In CSSStyleSelector.cpp the z-index is set explicitly to 0 if the element has a transform property, mask, reflection, or opacity < 1.  It looks like it covers all the bases.

The enclosingLayer is confusing. You want renderer->layer() (which it will have, since it's positioned).

So I guess you'll only do fast scrolling if all the fixed things on the page happen to also be stacking contexts? My guess is that won't cover the majority of pages with fixed.

> > Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.
> 
> Sure, will do.
> 
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> > > +    // Fixed position elements get composited only when inside a frame that scrolls.
> > > +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> > > +    m_compositingDependsOnGeometry = true;
> > > +    if (m_renderView->needsLayout())
> > > +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> > > +
> > > +    ScrollView* scrollView = m_renderView->frameView();
> > > +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();
> > 
> > I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.
> 
> Is the issue that I'm using the presence of scroll bars in the ScrollView to determine whether the layer is scrolling?  I'll change it to pick it up from the RenderLayer directly.

I think the decision about whether fixedpos causes compositing needs to be pushed into client code somehow.

[Vangelis Kokkevis]

> > > I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.
> > > 
> > 
> > To give some context about this change, the end goal of this effort is to give the compositor the ability to scroll a page/iframe without having to rely on webkit to adjust fixed position elements.  This in similar vain to being able to run animations independent of the main webkit thread.
> > 
> > To do that, we need some meta-information about how a layer positions itself to be stored in the GraphicsLayer. 
> 
> I understand, but I think you should store this information outside of GraphicsLayer, or phrase it in such a way that it's layout-agnostic.
> 
> iOS5 now supports compositing for position:fixed, but it doesn't need to touch GraphicsLayer at all for this.
>

Any chance that code could be shared? :)

I'll see if I can tie this somehow to the existing Animation hooks of GraphicsLayer.

 
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> > > > +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const
> > > 
> > > I'd call this requiresCompositingForPosition()
> > > 
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> > > > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> > > > +        return false;
> > > 
> > > The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. 
> > 
> > RenderLayer::isStackingContext() returns true for layers with an explicit z-index. In CSSStyleSelector.cpp the z-index is set explicitly to 0 if the element has a transform property, mask, reflection, or opacity < 1.  It looks like it covers all the bases.
> 
> The enclosingLayer is confusing. You want renderer->layer() (which it will have, since it's positioned).
> 

Ah, got it!

> So I guess you'll only do fast scrolling if all the fixed things on the page happen to also be stacking contexts? My guess is that won't cover the majority of pages with fixed.
> 

That's true, at least until we figure out how to lift the stacking context restriction.

> > > Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.
> > 
> > Sure, will do.
> > 
> > > 
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> > > > +    // Fixed position elements get composited only when inside a frame that scrolls.
> > > > +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> > > > +    m_compositingDependsOnGeometry = true;
> > > > +    if (m_renderView->needsLayout())
> > > > +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> > > > +
> > > > +    ScrollView* scrollView = m_renderView->frameView();
> > > > +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();
> > > 
> > > I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.
> > 
> > Is the issue that I'm using the presence of scroll bars in the ScrollView to determine whether the layer is scrolling?  I'll change it to pick it up from the RenderLayer directly.
> 
> I think the decision about whether fixedpos causes compositing needs to be pushed into client code somehow.

Is it because the iOS implementation for the same effect is different and somehow conflicting with this one?  I did not intend to make this code Chrome-specific.

[Vangelis Kokkevis]

I decided to split this into a few patches to separate out the issues raised by Simon's review:

Patch #1: https://bugs.webkit.org/show_bug.cgi?id=70067 : Add the necessary hooks for client code to decide whether to create a compositing layer.
Patch #2: Once #1 lands, I will move the fixed position layer creation logic into Chrome's implementation of ChromeClient.  This should only touch Chrome-specific code.
Patch #3: Deal with any necessary additions to the GraphicsLayer interface.

[Vangelis Kokkevis]

Is there a plan for the recently released iOS5 WebKit code that deals with fixed position elements to be merged into trunk? It would be nice to avoid having each platform doing its own implementation.

[Simon Fraser (smfr)]

Sorry for my lack of input here. Other stuff getting in the way.

There is very little iOS code that is sharable; mostly just requiresCompositiongForPosition(), and some ChromeClient callbacks used to register/unregister fixed position layers with platform code.

I'm not sure where the code should live that looks to see if the view is scrollable. Maybe it can be in RLC, and platforms which don't want it can #ifdef it out.

There is another wrinkle this patch doesn't deal with, which is that position:fixed inside a transform doesn't behave fixed.

[Vangelis Kokkevis]

Created attachment 111876 [details]
Patch

[Vangelis Kokkevis]

New patch uploaded. This one only deals with creating compositing layers for position:fixed elements. It doesn't modify GraphicsLayer or require elements to be in a scrolling frame to become composited.

[Simon Fraser (smfr)]

Comment on attachment 111876 [details]
Patch

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

You need to check that the

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1504
> +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> +    // opacity, transform) can get their own composited layer.
> +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && layer->isStackingContext()))
> +        return false;

This will incorrectly return true for a position:fixed element inside a transformed one. You should also consider nested position:fixed too. Both need test cases.

You want to check that the container() of the renderer is the RenderView, but beware; this method may be called before the renderers have been hooked up.

[Vangelis Kokkevis]

(In reply to comment #25)
> (From update of attachment 111876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111876&action=review
> 
> You need to check that the
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1504
> > +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> > +    // opacity, transform) can get their own composited layer.
> > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && layer->isStackingContext()))
> > +        return false;
> 
> This will incorrectly return true for a position:fixed element inside a transformed one. You should also consider nested position:fixed too. Both need test cases.
> 

Do we need to worry about it here? It seems that we can safely create a layer even if we're below a transformed element.  It's the logic that handles positioning the layer during scrolls that will need to make sure it stops at the closest transformed ancestor instead of going all the way up to the frame.  


> You want to check that the container() of the renderer is the RenderView, but beware; this method may be called before the renderers have been hooked up.

[Vangelis Kokkevis]

Created attachment 112035 [details]
Patch

[Vangelis Kokkevis]

Modified the patch to check that the container is indeed our RenderView.  I had to add a flag to bypass the early out that happens when we try to update the composited layers and we think nothing changed. Simon, please have another look?

[Vangelis Kokkevis]

(In reply to comment #28)
> Modified the patch to check that the container is indeed our RenderView.  I had to add a flag to bypass the early out that happens when we try to update the composited layers and we think nothing changed. Simon, please have another look?

Simon, any thoughts on this last patch?

[Vangelis Kokkevis]

(In reply to comment #29)
> (In reply to comment #28)
> > Modified the patch to check that the container is indeed our RenderView.  I had to add a flag to bypass the early out that happens when we try to update the composited layers and we think nothing changed. Simon, please have another look?
> 
> Simon, any thoughts on this last patch?

Looking more closely at the iOS code I'm not sure I find it to be an improvement over what I posted in my first patch.  Essentially the iOS code ends up calling into the ChromeClient every time a layer is touched to notify it whether it is fixed position or not.  It will do that for every layer in the hierarchy.

I find it much cleaner and more efficient to set the fixed position property directly on the GraphicsLayer at the same time the rest of the GL properties are being updated.  I realize that GraphicsLayer's currently don't have any concept or API's related to layout properties, but fixed positioning seems to be universally useful.

Simon, how can get this moving along?

[Simon Fraser (smfr)]

Comment on attachment 112035 [details]
Patch

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

r+ but I'd prefer m_compositingNeedsUpdate and m_compositingDependsOnGeometry be combined.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1505
> +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> +    // opacity, transform) can get their own composited layer.

Would be nice to explain here why you need the stacking context check.

> Source/WebCore/rendering/RenderLayerCompositor.h:303
> +    mutable bool m_compositingNeedsUpdate;

This has a similar role to m_compositingDependsOnGeometry; can they be combined into one "composting must be updated after layout"?

[Vangelis Kokkevis]

(In reply to comment #31)
> (From update of attachment 112035 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112035&action=review
> 
> r+ but I'd prefer m_compositingNeedsUpdate and m_compositingDependsOnGeometry be combined.
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1505
> > +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> > +    // opacity, transform) can get their own composited layer.
> 
> Would be nice to explain here why you need the stacking context check.
> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:303
> > +    mutable bool m_compositingNeedsUpdate;
> 
> This has a similar role to m_compositingDependsOnGeometry; can they be combined into one "composting must be updated after layout"?

I agree, it would be nice if they could be merged.  However, m_compositingDependsOnGeometry is unfortunately a sticky bit whereas m_compositingNeedsUpdate can be cleared after layout has occurred.

[Vangelis Kokkevis]

Created attachment 112616 [details]
Patch

[Vangelis Kokkevis]

(In reply to comment #33)
> Created an attachment (id=112616) [details]
> Patch

One more attempt to see if the win-ews bot passes this time. No review requested.

[Vangelis Kokkevis]

Created attachment 112628 [details]
Patch

[Vangelis Kokkevis]

(In reply to comment #35)
> Created an attachment (id=112628) [details]
> Patch

This time without the WebKit2 deps files.  No review requested.  Still trying to get green bots.

[Vangelis Kokkevis]

Committed r98627: <http://trac.webkit.org/changeset/98627>

[Eric Seidel (no email)]

Comment on attachment 112628 [details]
Patch

Cleared review? from attachment 112628 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).