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

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

Sticky position constraints were being computed relative to the current scroll position which led to incorrect constraints when updated after the ancestor scroller had been scrolled. This fixes that as well as updating descendant layer positions after scroll to fix 606732.

BUG=606732
TEST=fast/css/sticky/sticky-clip-rel-child.html

Committed: https://crrev.com/6b83939340b453504f8d5d38ba774d4f07324dd5
Cr-Commit-Position: refs/heads/master@{#403828}

[flackr, 2016-05-30 20:40:39]

flackr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[flackr, 2016-05-30 20:40:39]

Can you take a look? We have to update all descendant layers of the sticky layer
when updating layer positions after a scroll of the frame. Thanks.

[flackr, 2016-05-31 14:29:03]

Actually, since the scroll call can happen while compositing inputs are stale,
perhaps it makes more sense to mark needing positioned layout as I've done in
patch 2?

[chrishtr, 2016-05-31 20:19:53]

https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1616:
layoutObject->setNeedsPositionedMovementLayout();
What about sticky objects that are not stuck to the viewport?

[flackr, 2016-05-31 20:38:52]

https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1616:
layoutObject->setNeedsPositionedMovementLayout();
On 2016/05/31 at 20:19:53, chrishtr wrote:
> What about sticky objects that are not stuck to the viewport?

This is already handled by PaintLayerScrollableArea::setScrollOffset calling
updateLayerPositionsAfterOverflowScroll which is used for overflow scrollers.
It's probably best to be consistent with that so I'll use this update method. It
updates the layer positions recursively as needed though does require the same
compositing assert disabler used there:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Should I add a second test for an overflow scroller?

[chrishtr, 2016-05-31 21:06:29]

On 2016/05/31 at 20:38:52, flackr wrote:
>
https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:1616:
layoutObject->setNeedsPositionedMovementLayout();
> On 2016/05/31 at 20:19:53, chrishtr wrote:
> > What about sticky objects that are not stuck to the viewport?
> 
> This is already handled by PaintLayerScrollableArea::setScrollOffset calling
updateLayerPositionsAfterOverflowScroll which is used for overflow scrollers.
It's probably best to be consistent with that so I'll use this update method. It
updates the layer positions recursively as needed though does require the same
compositing assert disabler used there:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Should I add a second test for an overflow scroller?

Yes, definitely. Thanks.

[flackr, 2016-06-02 17:34:59]

On 2016/05/31 at 21:06:29, chrishtr wrote:
> On 2016/05/31 at 20:38:52, flackr wrote:
> >
https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> > 
> >
https://codereview.chromium.org/2020103002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/FrameView.cpp:1616:
layoutObject->setNeedsPositionedMovementLayout();
> > On 2016/05/31 at 20:19:53, chrishtr wrote:
> > > What about sticky objects that are not stuck to the viewport?
> > 
> > This is already handled by PaintLayerScrollableArea::setScrollOffset calling
updateLayerPositionsAfterOverflowScroll which is used for overflow scrollers.
It's probably best to be consistent with that so I'll use this update method. It
updates the layer positions recursively as needed though does require the same
compositing assert disabler used there:
> > 
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Should I add a second test for an overflow scroller?
> 
> Yes, definitely. Thanks.

So, it turns out writing this test exposed some other existing bugs with the
constraints for sticky within overflow scrollers. I've added several unit tests
to verify the sticky position constraints are exactly what is expected for the
various cases now as well as adding to the layout test an overflow scroller with
a clipped relative position box. Please take a look, thanks.

[chrishtr, 2016-06-06 22:18:40]

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-clip-rel-child-expected.html
(right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-clip-rel-child-expected.html:46:
<div class="relative">
Was the bug that it wasn't including the bounds of positioned descendants?

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:653:
containerContentRect.contractEdges(
Contract for padding and then margin? Is this trying to subtract off both? If
so, why aren't they
added?

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:659: // Sticky
positioned element ignore any override logical width on the containing block (as
they don't call
Does this comment belong above line 651?

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:670: FloatSize
scrollOffset(scrollAncestor ?
toFloatSize(scrollAncestor->getScrollableArea()->adjustedScrollOffset()) :
FloatSize());
Why should this rect include scroll offset?

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:673: if
(containingBlock != scrollAncestor)
Why conditional? What if the containing block is above the scroll ancestor?

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:687:
scrollContainerRelativeContainerFrame.move(scrollOffset);
localToAncestorQuad should already be adding in the scroll offset. Not sure what
you are trying to do here.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:39:
ASSERT_EQ(IntRect(15, 115, 170, 370),
enclosingIntRect(constraints.scrollContainerRelativeContainingBlockRect()));
Why 115 and not 15?

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:59:
ASSERT_EQ(IntRect(0, 0, 400, 1100),
enclosingIntRect(constraints.scrollContainerRelativeContainingBlockRect()));
Why should it include the overflow (and hence height=1100)?

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:68:
"#container { box-sizing: border-box; position: relative; top: 100px; height:
400px; width: 200px; padding: 10px; border: 5px solid black; }"
Where is the anonymous containing block in this example? Don't see a case of a
block inside an
inline..am I missing it?

[flackr, 2016-06-09 17:12:27]

Description was changed from

==========
Update layer positions recursively on updating sticky position after scroll.

BUG=606732
TEST=fast/css/sticky/sticky-clip-rel-child.html
==========

to

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

Sticky position constraints were being computed relative to the current scroll
position which led to incorrect constraints when updated after the ancestor
scroller had been scrolled. This fixes that as well as updating descendant layer
positions after scroll to fix 606732.

BUG=606732
TEST=fast/css/sticky/sticky-clip-rel-child.html
==========

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

Hey, let me know if you have any suggestions for comments to make it easier for
others to understand what's going on here. Thanks.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-clip-rel-child-expected.html
(right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-clip-rel-child-expected.html:46:
<div class="relative">
On 2016/06/06 at 22:18:39, chrishtr wrote:
> Was the bug that it wasn't including the bounds of positioned descendants?

Because the relative positioned element is clipped by sticky element, it gets a
separate PaintLayer. The bug was that the relative position element's position
was not being updated on scroll. This was basically the minimal repro from the
original bug but I suspect there may be other ways to reproduce this problem.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:651: LayoutRect
containerContentRect = containingBlock->layoutOverflowRect();
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.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:653:
containerContentRect.contractEdges(
On 2016/06/06 at 22:18:39, chrishtr wrote:
> Contract for padding and then margin? Is this trying to subtract off both? If
so, why aren't they
> added?

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.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:659: // Sticky
positioned element ignore any override logical width on the containing block (as
they don't call
On 2016/06/06 at 22:18:39, chrishtr wrote:
> Does this comment belong above line 651?

Done.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:670: FloatSize
scrollOffset(scrollAncestor ?
toFloatSize(scrollAncestor->getScrollableArea()->adjustedScrollOffset()) :
FloatSize());
On 2016/06/06 at 22:18:39, chrishtr wrote:
> Why should this rect include scroll offset?

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

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:673: if
(containingBlock != scrollAncestor)
On 2016/06/06 at 22:18:39, chrishtr wrote:
> Why conditional?

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.

> What if the containing block is above the scroll ancestor?

This should be impossible, position sticky's containing block is the first block
ancestor (which a scroll ancestor must be).

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:687:
scrollContainerRelativeContainerFrame.move(scrollOffset);
On 2016/06/06 at 22:18:39, chrishtr wrote:
> localToAncestorQuad should already be adding in the scroll offset. Not sure
what you are trying to do here.

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.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:39:
ASSERT_EQ(IntRect(15, 115, 170, 370),
enclosingIntRect(constraints.scrollContainerRelativeContainingBlockRect()));
On 2016/06/06 at 22:18:40, chrishtr wrote:
> Why 115 and not 15?

Because it's 115 from the top of the scrollable area (see the top: 100px in
#container)

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:59:
ASSERT_EQ(IntRect(0, 0, 400, 1100),
enclosingIntRect(constraints.scrollContainerRelativeContainingBlockRect()));
On 2016/06/06 at 22:18:39, chrishtr wrote:
> Why should it include the overflow (and hence height=1100)?

Because the #sticky element is constrained to the containing block, which is in
this case the entire scrollable area / overflow. This allows it to move down as
we scroll without recomputing sticky constraints.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:68:
"#container { box-sizing: border-box; position: relative; top: 100px; height:
400px; width: 200px; padding: 10px; border: 5px solid black; }"
On 2016/06/06 at 22:18:40, chrishtr wrote:
> Where is the anonymous containing block in this example? Don't see a case of a
block inside an
> inline..am I missing it?

Inside .container, .header is a block and #sticky is inline.

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

Sorry for the slow reply. Last week was kind of busy..

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:670: FloatSize
scrollOffset(scrollAncestor ?
toFloatSize(scrollAncestor->getScrollableArea()->adjustedScrollOffset()) :
FloatSize());
On 2016/06/09 at 17:13:33, flackr wrote:
> On 2016/06/06 at 22:18:39, chrishtr wrote:
> > Why should this rect include scroll offset?
> 
> Because we want the container's rect with respect to the scrollable area of
the scroll ancestor.

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.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:673: if
(containingBlock != scrollAncestor)
On 2016/06/09 at 17:13:32, flackr wrote:
> On 2016/06/06 at 22:18:39, chrishtr wrote:
> > Why conditional?
> 
> 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.
> 
> > What if the containing block is above the scroll ancestor?
> 
> This should be impossible, position sticky's containing block is the first
block ancestor (which a scroll ancestor must be).

Ah. Yes you are right.

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:687:
scrollContainerRelativeContainerFrame.move(scrollOffset);
On 2016/06/09 at 17:13:32, flackr wrote:
> On 2016/06/06 at 22:18:39, chrishtr wrote:
> > localToAncestorQuad should already be adding in the scroll offset. Not sure
what you are trying to do here.
> 
> 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.

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

https://codereview.chromium.org/2020103002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:654: //
containingBlockLogicalWidthForContent). It's unclear whether this is totally
fine.
Is this comment obsolete now that you are calling
containingBlockLogicalWidthForContent?

https://codereview.chromium.org/2020103002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:656: LayoutUnit
maxWidth = containingBlock->availableLogicalWidth();
Add a comment explaining that this is padding of containing block plus margin of
sticky element,
with link to spec.

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

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:670: FloatSize
scrollOffset(scrollAncestor ?
toFloatSize(scrollAncestor->getScrollableArea()->adjustedScrollOffset()) :
FloatSize());
On 2016/06/13 at 19:46:39, chrishtr wrote:
> On 2016/06/09 at 17:13:33, flackr wrote:
> > On 2016/06/06 at 22:18:39, chrishtr wrote:
> > > Why should this rect include scroll offset?
> > 
> > Because we want the container's rect with respect to the scrollable area of
the scroll ancestor.
> 
> 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.

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

https://codereview.chromium.org/2020103002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:654: //
containingBlockLogicalWidthForContent). It's unclear whether this is totally
fine.
On 2016/06/13 at 19:46:39, chrishtr wrote:
> Is this comment obsolete now that you are calling
containingBlockLogicalWidthForContent?

No, we're only calling containingBlockLogicalWidthForContent on the container
(for resolving sizes on the container), but not on the sticky element's
container (for resolving relative sizes on the sticky element).

https://codereview.chromium.org/2020103002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:656: LayoutUnit
maxWidth = containingBlock->availableLogicalWidth();
On 2016/06/13 at 19:46:39, chrishtr wrote:
> Add a comment explaining that this is padding of containing block plus margin
of sticky element,
> with link to spec.

Done.

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

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:687:
scrollContainerRelativeContainerFrame.move(scrollOffset);
On 2016/06/13 at 19:46:39, chrishtr wrote:
> On 2016/06/09 at 17:13:32, flackr wrote:
> > On 2016/06/06 at 22:18:39, chrishtr wrote:
> > > localToAncestorQuad should already be adding in the scroll offset. Not
sure what you are trying to do here.
> > 
> > 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.
> 
> See my comment above. Curious why it is not working as I expected..

Any response to this comment?

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

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:687:
scrollContainerRelativeContainerFrame.move(scrollOffset);
On 2016/06/15 at 16:13:46, chrishtr wrote:
> On 2016/06/13 at 19:46:39, chrishtr wrote:
> > On 2016/06/09 at 17:13:32, flackr wrote:
> > > On 2016/06/06 at 22:18:39, chrishtr wrote:
> > > > localToAncestorQuad should already be adding in the scroll offset. Not
sure what you are trying to do here.
> > > 
> > > 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.
> > 
> > See my comment above. Curious why it is not working as I expected..
> 
> Any response to this comment?

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

[chrishtr, 2016-06-16 11:03:59]

I think I understand better why you are removing scroll offset - to make the
rects independent of scroll, right? Sounds legit. If that is right, it's not so
clear to me from the code that that
is the point. Add some comments making this explicit?

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:670: if
(containingBlock != scrollAncestor)
If containingBlock == scrollAncestor, scrollContainerRelativeContainingBlockRect
will not include a scroll offset, right?
IOW, I don't think the conditional on line 670 should be necessary, you can just
put line 671.

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:58:
const FloatRect& scrollContainerRelativeContainingBlockRect() const { return
m_scrollContainerRelativeContainingBlockRect; }
These new methods are only used in tests. Make them private and add the test as
a friend?

[flackr, 2016-06-29 13:21:06]

> I think I understand better why you are removing scroll offset - to make the
rects independent of scroll, right? Sounds legit. If that is right, it's not so
clear to me from the code that that
> is the point. Add some comments making this explicit?

Yes, this is correct. I've added some comments that I hope makes this more
obvious.

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:670: if
(containingBlock != scrollAncestor)
On 2016/06/16 at 11:03:59, chrishtr wrote:
> If containingBlock == scrollAncestor,
scrollContainerRelativeContainingBlockRect will not include a scroll offset,
right?

Correct.

> IOW, I don't think the conditional on line 670 should be necessary, you can
just put line 671.

Not true, because we don't want the containing block's rect to be shifted by its
scroll offset - it's box should be rooted at the top of its scroll at (0, 0).

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2020103002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:58:
const FloatRect& scrollContainerRelativeContainingBlockRect() const { return
m_scrollContainerRelativeContainingBlockRect; }
On 2016/06/16 at 11:03:59, chrishtr wrote:
> These new methods are only used in tests. Make them private and add the test
as a friend?

Done.

[chrishtr, 2016-06-29 22:40:55]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-06-29 22:40:56]

lgtm

[commit-bot: I haz the power, 2016-06-29 22:42:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 23:37:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 23:37:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[flackr, 2016-07-05 20:53:37]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-07-05 20:53:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 22:04:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-05 22:06:34]

Description was changed from

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

Sticky position constraints were being computed relative to the current scroll
position which led to incorrect constraints when updated after the ancestor
scroller had been scrolled. This fixes that as well as updating descendant layer
positions after scroll to fix 606732.

BUG=606732
TEST=fast/css/sticky/sticky-clip-rel-child.html
==========

to

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

Sticky position constraints were being computed relative to the current scroll
position which led to incorrect constraints when updated after the ancestor
scroller had been scrolled. This fixes that as well as updating descendant layer
positions after scroll to fix 606732.

BUG=606732
TEST=fast/css/sticky/sticky-clip-rel-child.html

Committed: https://crrev.com/6b83939340b453504f8d5d38ba774d4f07324dd5
Cr-Commit-Position: refs/heads/master@{#403828}
==========

[commit-bot: I haz the power, 2016-07-05 22:06:35]

Patchset 8 (id:??) landed as
https://crrev.com/6b83939340b453504f8d5d38ba774d4f07324dd5
Cr-Commit-Position: refs/heads/master@{#403828}