Bug 69796

Created attachment 110427 [details] Patch

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.

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

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

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

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

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

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.

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

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?

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

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.

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

Created attachment 110600 [details] Patch

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.

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.

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.

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

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

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.

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.

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.

Created attachment 111876 [details] Patch

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.

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.

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

Created attachment 112035 [details] Patch

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?

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

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

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

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

Created attachment 112616 [details] Patch

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

Created attachment 112628 [details] Patch

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

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

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