Fix sticky constraints and update sticky layer positions recursively after scroll.

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 cd3587ad63c36c9c01f3a8c43e808791cc8e3b93..77e12b9534a0f5a1e1186913e7f0314381535f2d 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
@@ -635,7 +635,6 @@ LayoutSize LayoutBoxModelObject::relativePositionOffset() const
 
 void LayoutBoxModelObject::updateStickyPositionConstraints() const
 {
-    // TODO(flackr): This method is reasonably complicated and should have some direct unit testing.
     const FloatSize constrainingSize = computeStickyConstrainingRect().size();
 
     PaintLayerScrollableArea* scrollableArea = layer()->ancestorOverflowLayer()->getScrollableArea();
@@ -649,8 +648,13 @@ void LayoutBoxModelObject::updateStickyPositionConstraints() const
     }
     LayoutBox* scrollAncestor = layer()->ancestorOverflowLayer()->isRootLayer() ? nullptr : toLayoutBox(layer()->ancestorOverflowLayer()->layoutObject());
 
-    LayoutRect containerContentRect = containingBlock->contentBoxRect();
+    LayoutRect containerContentRect = containingBlock->layoutOverflowRect();

[flackr, 2016/06/09 17:13:32]

When the container is the scroll ancestor, we need to use layoutOverflowRect in
order to get the full scrollable rect rather than the clipped rect.

     LayoutUnit maxWidth = containingBlock->availableLogicalWidth();
+    containerContentRect.contractEdges(

[chrishtr, 2016/06/06 22:18:39]

Contract for padding and then margin? Is this trying to subtract off both? If
so, why aren't they
added?

[flackr, 2016/06/09 17:13:33]

Done, note that we remove the padding from the containing block and the margin
from the sticky element. I suspect for the container's padding I need to resolve
the lengths against the containing block's containing block's available logical
width. I've added a test to verify resolving percentage values.

+        minimumValueForLength(containingBlock->style()->paddingTop(), maxWidth),
+        minimumValueForLength(containingBlock->style()->paddingRight(), maxWidth),
+        minimumValueForLength(containingBlock->style()->paddingBottom(), maxWidth),
+        minimumValueForLength(containingBlock->style()->paddingLeft(), maxWidth));
 
     // Sticky positioned element ignore any override logical width on the containing block (as they don't call

[chrishtr, 2016/06/06 22:18:39]

Does this comment belong above line 651?

[flackr, 2016/06/09 17:13:33]

Done.

     // containingBlockLogicalWidthForContent). It's unclear whether this is totally fine.
@@ -662,7 +666,13 @@ void LayoutBoxModelObject::updateStickyPositionConstraints() const
         minimumValueForLength(style()->marginLeft(), maxWidth));
 
     // Map to the scroll ancestor.
-    constraints.setScrollContainerRelativeContainingBlockRect(containingBlock->localToAncestorQuad(FloatRect(containerContentRect), scrollAncestor).boundingBox());
+    FloatRect scrollContainerRelativeContainingBlockRect(containingBlock->localToAncestorQuad(FloatRect(containerContentRect), scrollAncestor).boundingBox());
+    FloatSize scrollOffset(scrollAncestor ? toFloatSize(scrollAncestor->getScrollableArea()->adjustedScrollOffset()) : FloatSize());

[chrishtr, 2016/06/06 22:18:39]

Why should this rect include scroll offset?

[flackr, 2016/06/09 17:13:33]

Because we want the container's rect with respect to the scrollable area of the
scroll ancestor.

[chrishtr, 2016/06/13 19:46:39]

I see, ok.

Why doesn't localToAncestorQuad handle this then? I would have expected this
line of code:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
(call to offsetFromContainer())

to include scroll offset when passing to the scrollAncestor from the object for
which it is a containing block.

[flackr, 2016/06/15 13:45:49]

offsetFromContainer is specifically removing the scroll offset to return the
offset between the two boxes visually given the current scroll.

+
+    // Include scroll offset in container position if the container is not our scroll ancestor.
+    if (containingBlock != scrollAncestor)

[chrishtr, 2016/06/06 22:18:39]

Why conditional? What if the containing block is above the scroll ancestor?

[flackr, 2016/06/09 17:13:32]

We want to compute the bounds relative to the scroll ancestor. If the containing
block is the scroll ancestor it starts at (0, 0) regardless of the current
scroll position, however if it's a descendant the scroll position needs to be
added to get it's position within the scroll ancestor.
This should be impossible, position sticky's containing block is the first block
ancestor (which a scroll ancestor must be).

[chrishtr, 2016/06/13 19:46:39]

Ah. Yes you are right.

+        scrollContainerRelativeContainingBlockRect.move(scrollOffset);
+    constraints.setScrollContainerRelativeContainingBlockRect(scrollContainerRelativeContainingBlockRect);
 
     FloatRect stickyBoxRect = isLayoutInline()
         ? FloatRect(toLayoutInline(this)->linesBoundingBox())
@@ -674,13 +684,12 @@ void LayoutBoxModelObject::updateStickyPositionConstraints() const
     // TODO(flackr): Unfortunate to call localToAncestorQuad again, but we can't just offset from the previously computed rect if there are transforms.
     // Map to the scroll ancestor.
     FloatRect scrollContainerRelativeContainerFrame = containingBlock->localToAncestorQuad(FloatRect(FloatPoint(), FloatSize(containingBlock->size())), scrollAncestor).boundingBox();
+    scrollContainerRelativeContainerFrame.move(scrollOffset);

[chrishtr, 2016/06/06 22:18:39]

localToAncestorQuad should already be adding in the scroll offset. Not sure what
you are trying to do here.

[flackr, 2016/06/09 17:13:32]

It does not seem to. If the container is at (0, 0) and you're scrolled down
200px this reports the element as being at (0, -200). We want all computed rects
with respect to the top of the scrollable area.

[chrishtr, 2016/06/13 19:46:39]

See my comment above. Curious why it is not working as I expected..

[chrishtr, 2016/06/15 16:13:46]

Any response to this comment?

[flackr, 2016/06/16 07:22:51]

Ah, sorry, I commented on the other location. offsetFromContainer is
specifically removing the scroll offset presumably to return the offset between
the two boxes visually given the current scroll.

The specific line is here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

 
     // If the containing block is our scroll ancestor, its location will not include the scroll offset which we need to include as
     // part of the sticky box rect so we include it here.
-    if (containingBlock->hasOverflowClip()) {
-        FloatSize scrollOffset(toFloatSize(containingBlock->layer()->getScrollableArea()->adjustedScrollOffset()));
+    if (containingBlock == scrollAncestor)
         stickyLocation -= scrollOffset;
-    }
 
     constraints.setScrollContainerRelativeStickyBoxRect(FloatRect(scrollContainerRelativeContainerFrame.location() + toFloatSize(stickyLocation), flippedStickyBoxRect.size()));
 
@@ -738,7 +747,7 @@ FloatRect LayoutBoxModelObject::computeStickyConstrainingRect() const
 
     LayoutBox* enclosingClippingBox = toLayoutBox(layer()->ancestorOverflowLayer()->layoutObject());
     FloatRect constrainingRect;
-    constrainingRect = FloatRect(enclosingClippingBox->overflowClipRect(LayoutPoint()));
+    constrainingRect = FloatRect(enclosingClippingBox->overflowClipRect(LayoutPoint(DoublePoint(enclosingClippingBox->getScrollableArea()->adjustedScrollOffset()))));
     constrainingRect.move(enclosingClippingBox->paddingLeft(), enclosingClippingBox->paddingTop());
     constrainingRect.contract(FloatSize(enclosingClippingBox->paddingLeft() + enclosingClippingBox->paddingRight(),
         enclosingClippingBox->paddingTop() + enclosingClippingBox->paddingBottom()));