Handle nested position:sticky elements

third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp

Index: third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
index 486a11a43eb50a95648e8c40706d57d7d6d7f796..f57e76c1574e0dfd078f4889ed45c0a22abe757c 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
@@ -873,6 +873,32 @@ void LayoutBoxModelObject::updateStickyPositionConstraints() const {
                     toFloatSize(stickyLocation),
                 flippedStickyBoxRect.size()));
 
+  // To correctly compute the offsets, the constraints need to know about any
+  // nested position:sticky between themselves and their containingBlock, and
+  // between the containingBlock and the viewport.
+
+  LayoutObject* maybeStickyAncestor = parent();
+  while (maybeStickyAncestor &&
+         maybeStickyAncestor != containingBlock->parent()) {

[flackr, 2017/01/18 16:09:47]

We should stop before the containing block, the containing block's to viewport
ancestor should be able to point to itself if the containing block is sticky.

[smcgruer, 2017/01/18 19:10:50]

Not sure I follow: no element should have itself as an ancestor as far as I can
tell.

Are you saying that you would want to change the bounds to be inclusive
different, e.g. so that m_nearestStickyElementToContainingBlock *cannot* be the
containingBlock, and m_nearestStickyElementFromContainingBlockToScrollContainer
*can* be the containingBlock?

[flackr, 2017/01/19 01:44:41]

Not true. For the purposes of sticky position, the containing block can be its
own nearest sticky ancestor to the viewport because all that means is you have
to offset the containing block rect by its sticky offset for its sticky
descendants.
Yes.

[smcgruer, 2017/01/19 19:28:01]

Done.

+    if (maybeStickyAncestor->isStickyPositioned()) {
+      constraints.setNearestStickyElementToContainingBlock(
+          toLayoutBoxModelObject(maybeStickyAncestor));
+      break;
+    }
+    maybeStickyAncestor = maybeStickyAncestor->parent();
+  }

[flackr, 2017/01/18 14:42:56]

We should try combining this with the walk to find the containing block. I.e.
walk up looking for containing block criteria and/or sticky.

+
+  maybeStickyAncestor = containingBlock->parent();
+  while (maybeStickyAncestor &&
+         maybeStickyAncestor != scrollAncestor->parent()) {

[flackr, 2017/01/18 14:42:56]

I don't think we need to track if the ancestor scroller is sticky, do we? If
it's moved the positions in the constraints are all relative to that ancestor so
they should be unchanged.

[smcgruer, 2017/01/18 19:10:51]

Good spot! Fixed and added a test. Also had to wrap this in a check to make sure
there is an ancestor sticky.

+    if (maybeStickyAncestor->isStickyPositioned()) {
+      constraints.setNearestStickyElementFromContainingBlockToViewport(

[flackr, 2017/01/18 14:42:56]

for consistency s/ToViewport/ToScrollContainer (I realize these names are
getting ridiculously long :( )

[smcgruer, 2017/01/18 19:10:50]

Done.

+          toLayoutBoxModelObject(maybeStickyAncestor));
+      break;
+    }
+    maybeStickyAncestor = maybeStickyAncestor->parent();
+  }
+
   // We skip the right or top sticky offset if there is not enough space to
   // honor both the left/right or top/bottom offsets.
   LayoutUnit horizontalOffsets =
@@ -976,9 +1002,35 @@ LayoutSize LayoutBoxModelObject::stickyPositionOffset() const {
   // compositing inputs.
   if (!scrollableArea->stickyConstraintsMap().contains(layer()))
     return LayoutSize();
-  return LayoutSize(
-      scrollableArea->stickyConstraintsMap().get(layer()).computeStickyOffset(
-          constrainingRect));
+
+  auto it = scrollableArea->stickyConstraintsMap().find(layer());

[flackr, 2017/01/18 14:42:56]

Out of curiosity, why not stickyConstraintsMap().get(layer())?

[smcgruer, 2017/01/18 19:10:51]

As far as I can tell, WTF::HashMap::get() returns a 'MappedPeekType', which
doesn't allow you to take a non-const reference to it. And the reference has to
be non-const to allow the call to computeStickyOffset to work.

See WTF::GenericHashTraits in HashTraits.h.

[flackr, 2017/01/19 01:44:41]

Okay no problem.

+  DCHECK(it != scrollableArea->stickyConstraintsMap().end());
+
+  StickyPositionScrollingConstraints& constraints = it->value;
+
+  FloatSize accumulatedOffsetToContainingBlock;
+  FloatSize accumulatedOffsetToViewport;
+
+  const LayoutBoxModelObject* toContainingBlock =
+      constraints.nearestStickyElementToContainingBlock();
+  if (toContainingBlock) {
+    accumulatedOffsetToContainingBlock =
+        scrollableArea->stickyConstraintsMap()
+            .get(toContainingBlock->layer())
+            .getCachedAccumulatedStickyOffset();
+  }
+
+  const LayoutBoxModelObject* toViewport =
+      constraints.nearestStickyElementFromContainingBlockToViewport();
+  if (toViewport) {
+    accumulatedOffsetToViewport = scrollableArea->stickyConstraintsMap()
+                                      .get(toViewport->layer())
+                                      .getCachedAccumulatedStickyOffset();
+  }
+
+  return LayoutSize(constraints.computeStickyOffset(
+      constrainingRect, accumulatedOffsetToContainingBlock,
+      accumulatedOffsetToViewport));
 }
 
 LayoutPoint LayoutBoxModelObject::adjustedPositionRelativeTo(