Handle nested position:sticky elements correctly (compositor)

third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp

Index: third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
diff --git a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
index c715a2954ec4c2eafbe10594703a5a5c310ac30e..e34268e928cce57005d2c49bae3203151ef8eb1a 100644
--- a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
+++ b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
@@ -173,6 +173,61 @@ static ScrollingCoordinator* scrollingCoordinatorFromLayer(PaintLayer& layer) {
   return (!page) ? nullptr : page->scrollingCoordinator();
 }
 
+static const LayoutBoxModelObject* nearestCompositedStickyAncestor(

[chrishtr, 2017/03/20 17:24:52]

I assume one of the layout tests exercises each of these methods? Could you
please dd
comments to the tests explaining the corner of the semantics it is exercising?

[smcgruer, 2017/03/21 13:54:54]

Hrm, this is a good question to raise. There are basically two sub-questions
here; are these functions necessary, and is it necessary that they search for
composited layers?

Starting with the first sub-question:

1. |nearestCompositedStickyAncestor| is used to correct the enclosing layer
offset. Removing this function will make all of the LayoutTests fail, so I'm not
sure they should reference it explicitly.

2. |nearestCompositedStickyLayerShiftingStickyBox| is needed to correct for
sticky ancestor when inline/anonymous layers exist. Unfortunately the LayoutTest
that triggers this had to be removed from this CL due to
http://crbug.com/702637.

3. |nearestCompositedStickyLayerShiftingContainingBlock| is needed to generally
correct for ancestor sticky elements. Again removing this will make all of the
LayoutTests fail.

So I think there's definitely an argument that we're missing a test atm for
nearestCompositedStickyLayerShiftingStickyBox, or it could even be dropped and
re-added as part of http://crbug.com/702637. 

Now, the second sub-question - do we need this search for a composited layer?
(And do the tests exercise that). Due to http://crbug.com/702229, we plan to
promote all sticky that have a descendant promoted sticky. That would imply that
your ancestor layers would automatically have composited layers and this search
won't be necessary. However, I think flackr also wants to not promote sticky
layers that have no constraints, so that may re-necessitate the search - flackr?

As for the questions of do the LayoutTests exercise this search, I believe they
currently do not and any that tried would break due to http://crbug.com/702229.
So perhaps these searches should just be removed? 

[smcgruer, 2017/03/21 16:01:25]

Ok, flackr and I discussed this and realized that once his patch and the fix for
http://crbug.com/702229 is submitted, any sticky ancestor you have will always
be composited. (flackr's patch has evolved to just remove non-constrained sticky
elements).

I will remove the search and just leave a brief early-exit for parent being
non-composited which will be replaced in the fix for http://crbug.com/702229

[smcgruer, 2017/03/21 17:37:48]

Done.

+    const StickyConstraintsMap& constraintsMap,
+    const StickyPositionScrollingConstraints& constraints) {
+  const LayoutBoxModelObject* obj = constraints.nearestStickyAncestor();
+  while (obj && !obj->layer()->compositedLayerMapping()) {
+    DCHECK(constraintsMap.contains(obj->layer()));
+    obj = constraintsMap.at(obj->layer()).nearestStickyAncestor();
+  }
+  return obj;
+}
+
+// Return the id of the nearest composited sticky layer that shifts
+// |constraints| sticky box, or kInvalidLayerId if no such layer exists.
+static int nearestCompositedStickyLayerShiftingStickyBox(
+    const StickyConstraintsMap& constraintsMap,
+    const StickyPositionScrollingConstraints& constraints) {
+  LayoutBoxModelObject* obj = constraints.nearestStickyBoxShiftingStickyBox();
+  while (obj && !obj->layer()->compositedLayerMapping()) {
+    DCHECK(constraintsMap.contains(obj->layer()));
+    obj = constraintsMap.at(obj->layer()).nearestStickyBoxShiftingStickyBox();
+  }
+
+  if (!obj)
+    return WebLayer::kInvalidLayerId;
+
+  return obj->layer()
+      ->compositedLayerMapping()
+      ->mainGraphicsLayer()
+      ->platformLayer()
+      ->id();
+}
+
+// Return the id of the nearest composited sticky layer that shifts
+// |constraints| containing block, or kInvalidLayerId if no such layer exists.
+static int nearestCompositedStickyLayerShiftingContainingBlock(
+    const StickyConstraintsMap& constraintsMap,
+    const StickyPositionScrollingConstraints& constraints) {
+  LayoutBoxModelObject* obj =
+      constraints.nearestStickyBoxShiftingContainingBlock();
+  while (obj && !obj->layer()->compositedLayerMapping()) {
+    DCHECK(constraintsMap.contains(obj->layer()));
+    obj = constraintsMap.at(obj->layer())
+              .nearestStickyBoxShiftingContainingBlock();
+  }
+
+  if (!obj)
+    return WebLayer::kInvalidLayerId;
+
+  return obj->layer()
+      ->compositedLayerMapping()
+      ->mainGraphicsLayer()
+      ->platformLayer()
+      ->id();
+}
+
 CompositedLayerMapping::CompositedLayerMapping(PaintLayer& layer)
     : m_owningLayer(layer),
       m_contentOffsetInCompositingLayerDirty(false),
@@ -308,14 +363,24 @@ void CompositedLayerMapping::updateStickyConstraints(
 
   WebLayerStickyPositionConstraint webConstraint;
   if (sticky) {
+    const StickyConstraintsMap& constraintsMap =
+        ancestorOverflowLayer->getScrollableArea()->stickyConstraintsMap();
     const StickyPositionScrollingConstraints& constraints =
-        ancestorOverflowLayer->getScrollableArea()->stickyConstraintsMap().at(
-            &m_owningLayer);
+        constraintsMap.at(&m_owningLayer);
 
-    // Find the layout offset of the unshifted sticky box within its
-    // compositingContainer. If the enclosing layer is not the scroller, then
-    // the offset must be adjusted to include the scroll offset to keep it
-    // relative to compositingContainer.
+    // Find the layout offset of the unshifted sticky box within its parent
+    // composited layer. This information is used by the compositor side to
+    // compute the additional offset required to keep the element stuck under
+    // compositor scrolling.
+    //
+    // Starting from the scroll container relative location, removing the
+    // enclosing layers offset and the content offset in the composited layer

[chrishtr, 2017/03/20 17:24:52]

nit: "layer's"

[smcgruer, 2017/03/21 13:54:54]

Done.

+    // results in the parent-layer relative offset.
+    FloatPoint parentRelativeStickyBoxOffset =
+        constraints.scrollContainerRelativeStickyBoxRect().location();
+
+    // The enclosing layers offset returned from |convertToLayerCoords| must be
+    // adjusted for both scroll and ancestor sticky elements.
     LayoutPoint enclosingLayerOffset;
     compositingContainer->convertToLayerCoords(ancestorOverflowLayer,
                                                enclosingLayerOffset);
@@ -323,12 +388,17 @@ void CompositedLayerMapping::updateStickyConstraints(
       enclosingLayerOffset += LayoutSize(
           ancestorOverflowLayer->getScrollableArea()->getScrollOffset());
     }
+    if (const LayoutBoxModelObject* ancestor =
+            nearestCompositedStickyAncestor(constraintsMap, constraints)) {
+      enclosingLayerOffset -=
+          roundedIntSize(constraintsMap.at(ancestor->layer())
+                             .getTotalContainingBlockStickyOffset());
+    }
 
-    FloatPoint stickyBoxOffset =
-        constraints.scrollContainerRelativeStickyBoxRect().location();
     DCHECK(!m_contentOffsetInCompositingLayerDirty);
-    stickyBoxOffset.moveBy(FloatPoint(-enclosingLayerOffset) -
-                           FloatSize(contentOffsetInCompositingLayer()));
+    parentRelativeStickyBoxOffset.moveBy(
+        FloatPoint(-enclosingLayerOffset) -
+        FloatSize(contentOffsetInCompositingLayer()));
 
     webConstraint.isSticky = true;
     webConstraint.isAnchoredLeft =
@@ -347,13 +417,17 @@ void CompositedLayerMapping::updateStickyConstraints(
     webConstraint.rightOffset = constraints.rightOffset();
     webConstraint.topOffset = constraints.topOffset();
     webConstraint.bottomOffset = constraints.bottomOffset();
-    webConstraint.parentRelativeStickyBoxOffset =
-        roundedIntPoint(stickyBoxOffset);
+    webConstraint.parentRelativeStickyBoxOffset = parentRelativeStickyBoxOffset;
     webConstraint.scrollContainerRelativeStickyBoxRect =
-        enclosingIntRect(constraints.scrollContainerRelativeStickyBoxRect());
-    webConstraint.scrollContainerRelativeContainingBlockRect = enclosingIntRect(
-        constraints.scrollContainerRelativeContainingBlockRect());
-    // TODO(smcgruer): Copy fields for nested sticky in cc (crbug.com/672710)
+        constraints.scrollContainerRelativeStickyBoxRect();
+    webConstraint.scrollContainerRelativeContainingBlockRect =
+        constraints.scrollContainerRelativeContainingBlockRect();
+    webConstraint.nearestLayerShiftingStickyBox =
+        nearestCompositedStickyLayerShiftingStickyBox(constraintsMap,
+                                                      constraints);
+    webConstraint.nearestLayerShiftingContainingBlock =
+        nearestCompositedStickyLayerShiftingContainingBlock(constraintsMap,
+                                                            constraints);
   }
 
   m_graphicsLayer->setStickyPositionConstraint(webConstraint);