position:sticky should stick for the enclosing overflow ancestor

Source/core/rendering/RenderLayerCompositor.cpp

Index: Source/core/rendering/RenderLayerCompositor.cpp
diff --git a/Source/core/rendering/RenderLayerCompositor.cpp b/Source/core/rendering/RenderLayerCompositor.cpp
index 09084ed814cd94e77220d8a4c2544b9a43907d86..26d8c846eb6560e214bb06873b9fc0af28fecf57 100644
--- a/Source/core/rendering/RenderLayerCompositor.cpp
+++ b/Source/core/rendering/RenderLayerCompositor.cpp
@@ -1796,6 +1796,24 @@ bool RenderLayerCompositor::requiresCompositingForOutOfFlowClipping(const Render
     return layer->compositorDrivenAcceleratedScrollingEnabled() && layer->isUnclippedDescendant();
 }
 
+static bool isViewportConstrainedFixedOrStickyLayer(const RenderLayer* layer)
+{
+    if (layer->renderer()->isStickyPositioned())
+        return !layer->enclosingOverflowClipLayer(false);
+
+    if (layer->renderer()->style()->position() != FixedPosition)
+        return false;
+
+    for (const RenderLayerStackingNode* stackingContainer = layer->stackingNode(); stackingContainer;
+        stackingContainer = stackingContainer->ancestorStackingContainerNode()) {
+        if (stackingContainer->layer()->compositingState() != NotComposited

[Julien - ping for review, 2014/01/06 17:22:07]

The old code was checking
stackingContainer->layer()->hasCompositedLayerMapping(), I think this check
should be kept instead of changed.

[shawnsingh, 2014/01/06 20:02:48]

Actually what ostap@ has done here is reasonable.  We should be relying less on
hasCompositedLayerMapping() to indicate compositingState, and when we want to
query compositingState, do it directly.

A side comment - don't need to block this patch:
There is a red flag here, even before this patch.  This function is being used
within computeCompositingRequirements() - and we should not have any code in
computeCompositingRequirements that depends on any layer's compositingState() or
hasCompositedLayerMapping().  The compositingState() will be from a previous
update or be in an inconsistent state here.  compositingState() is only fully
correct after assignLayersToBackings().  I created bug
https://code.google.com/p/chromium/issues/detail?id=331866 for this.

I suspect the webkit code might have an error about doing this, too, but webkit
and blink are subtly different these days about the data flow in the compositing
code, so I'm not sure.

[ostap, 2014/01/08 01:46:40]

The original code in webkit patch calls stackingContainer->isComposited() .
Since in blink stacking container is separated from renderLayer and
isComposited() doesn't exist any more, I thought compositingState() check would
be valid replacement.

[Julien - ping for review, 2014/01/08 15:31:45]

I stand corrected by Shawn. I was more comfortable matching the original intent
of the code but we are moving away from that idiom so we may as well update the
code.

+            && stackingContainer->layer()->renderer()->style()->position() == FixedPosition)
+            return false;
+    }
+
+    return true;
+}
+
 bool RenderLayerCompositor::requiresCompositingForPosition(RenderObject* renderer, const RenderLayer* layer, RenderLayer::ViewportConstrainedNotCompositedReason* viewportConstrainedNotCompositedReason) const
 {
     // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
@@ -1820,7 +1838,7 @@ bool RenderLayerCompositor::requiresCompositingForPosition(RenderObject* rendere
     }
 
     if (isSticky)
-        return true;
+        return isViewportConstrainedFixedOrStickyLayer(layer);
 
     RenderObject* container = renderer->container();
     // If the renderer is not hooked up yet then we have to wait until it is.
@@ -2346,25 +2364,9 @@ bool RenderLayerCompositor::layerHas3DContent(const RenderLayer* layer) const
     return false;
 }
 
-static bool isRootmostFixedOrStickyLayer(RenderLayer* layer)
-{
-    if (layer->renderer()->isStickyPositioned())
-        return true;
-
-    if (layer->renderer()->style()->position() != FixedPosition)
-        return false;
-
-    for (RenderLayerStackingNode* stackingContainerNode = layer->stackingNode()->ancestorStackingContainerNode(); stackingContainerNode; stackingContainerNode = stackingContainerNode->ancestorStackingContainerNode()) {
-        if (stackingContainerNode->layer()->hasCompositedLayerMapping() && stackingContainerNode->layer()->renderer()->style()->position() == FixedPosition)
-            return false;
-    }
-
-    return true;
-}
-
 void RenderLayerCompositor::updateViewportConstraintStatus(RenderLayer* layer)
 {
-    if (isRootmostFixedOrStickyLayer(layer))
+    if (isViewportConstrainedFixedOrStickyLayer(layer))
         addViewportConstrainedLayer(layer);
     else
         removeViewportConstrainedLayer(layer);
@@ -2424,6 +2426,8 @@ FixedPositionViewportConstraints RenderLayerCompositor::computeFixedViewportCons
 StickyPositionViewportConstraints RenderLayerCompositor::computeStickyViewportConstraints(RenderLayer* layer) const
 {
     ASSERT(layer->hasCompositedLayerMapping());
+    // We should never get here for stickies constrained by an enclosing clipping layer.
+    ASSERT(!layer->enclosingOverflowClipLayer(false));
 
     FrameView* frameView = m_renderView->frameView();
     LayoutRect viewportRect = frameView->viewportConstrainedVisibleContentRect();