Handle nested position:sticky elements

Handle nested position:sticky elements correctly

In order to correctly calculate the sticky offset for nested sticky, we must
track the accumulated sticky offset from our ancestor sticky elements. This
accumulation must then be used to adjust the location of the two constraint
rects.

This patch adds this behavior both for the main thread and cc side. On the
compositor side, we additionally adjust the main_thread_offset to include the
enclosing layer's scroll position and sticky offset. This is not directly
related to nested sticky, but is required for it to work correctly.

Finally, we also disallow position: sticky from being applied to <thead> and <tr>
elements, only <th> and <td>. This is necessary because Chrome's current
implementation of in-flow positioned elements does not support <thead> and
<tr> as per the CSS 2.1 spec, so the container/containingBlock logic is not correct
for CSS 3 currently.

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[smcgruer, 2017-01-17 21:54:49]

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

[smcgruer, 2017-01-17 21:54:49]

As I mentioned briefly earlier today, this is the work I have so far for fixing
nested position:sticky. I'm having difficulties sorting out what we want to
accumulate in my head (and the impact), so we can either discuss here or
whiteboard it later this week.

Test cases I have been using on top of the unittests:

https://output.jsbin.com/tufizar/12
https://output.jsbin.com/sanide
https://output.jsbin.com/raqufix
https://output.jsbin.com/tiqipi/35
https://output.jsbin.com/vufuyux/8
https://output.jsbin.com/fefohad/21

[flackr, 2017-01-18 14:42:57]

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1941: if
(!constraintsMap.get(layer).hasAncestorStickyElement()) {
nit: Add condition to outer if rather than nesting if conditions.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:889: }
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.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:893:
maybeStickyAncestor != scrollAncestor->parent()) {
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.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:895:
constraints.setNearestStickyElementFromContainingBlockToViewport(
for consistency s/ToViewport/ToScrollContainer (I realize these names are
getting ridiculously long :( )

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1006: auto it =
scrollableArea->stickyConstraintsMap().find(layer());
Out of curiosity, why not stickyConstraintsMap().get(layer())?

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:15:
// moving both by both is incorrect, but currently done because I don't
I think the stickyBoxRect does need to be moved by both because both offsets
come from ancestors, but the containing block rect should only be moved by the
ToVP offset.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:21:
containingBlockRect.move(accumulatedToCB);
The containing block rect should only be moved by the accumulatedToVP, the
accumulatedToCB should be from a descendant node (i.e. not affect the containing
block).

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:75:
m_cachedAccumulatedStickyOffset.setHeight(stickyOffset.height());
I think we need to add the ToCB and ToVP offsets here, since any descendants
which rely on this offset need to know about the ancestor offsets as well.

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

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:882:
maybeStickyAncestor != containingBlock->parent()) {
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:51]

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1941: if
(!constraintsMap.get(layer).hasAncestorStickyElement()) {
On 2017/01/18 14:42:56, flackr wrote:
> nit: Add condition to outer if rather than nesting if conditions.

Done.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:882:
maybeStickyAncestor != containingBlock->parent()) {
On 2017/01/18 16:09:47, flackr wrote:
> 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.

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?

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:893:
maybeStickyAncestor != scrollAncestor->parent()) {
On 2017/01/18 14:42:56, flackr wrote:
> 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.

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

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:895:
constraints.setNearestStickyElementFromContainingBlockToViewport(
On 2017/01/18 14:42:56, flackr wrote:
> for consistency s/ToViewport/ToScrollContainer (I realize these names are
> getting ridiculously long :( )

Done.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1006: auto it =
scrollableArea->stickyConstraintsMap().find(layer());
On 2017/01/18 14:42:56, flackr wrote:
> Out of curiosity, why not stickyConstraintsMap().get(layer())?

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]

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:882:
maybeStickyAncestor != containingBlock->parent()) {
On 2017/01/18 19:10:50, smcgruer wrote:
> On 2017/01/18 16:09:47, flackr wrote:
> > 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.
> 
> Not sure I follow: no element should have itself as an ancestor as far as I
can
> tell.

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

Yes.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1006: auto it =
scrollableArea->stickyConstraintsMap().find(layer());
On 2017/01/18 19:10:51, smcgruer wrote:
> On 2017/01/18 14:42:56, flackr wrote:
> > Out of curiosity, why not stickyConstraintsMap().get(layer())?
> 
> 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.

Okay no problem.

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

Ok, we're making progress! The code now works on every example I have, although
there are some things still to discuss/clean (marked by TODOs):

1. I need to fold the walks into the earlier walks for CB/AS.

2. The naming could be much improved (let the bike-shedding begin).

3. I don't actually understand the table tests - the values appear wrong to me
but the tests render correctly (see TODOs)

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:882:
maybeStickyAncestor != containingBlock->parent()) {
On 2017/01/19 01:44:41, flackr wrote:
> On 2017/01/18 19:10:50, smcgruer wrote:
> > On 2017/01/18 16:09:47, flackr wrote:
> > > 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.
> > 
> > Not sure I follow: no element should have itself as an ancestor as far as I
> can
> > tell.
> 
> 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.
> > 
> > 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?
> 
> Yes.

Done.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:15:
// moving both by both is incorrect, but currently done because I don't
On 2017/01/18 14:42:56, flackr wrote:
> I think the stickyBoxRect does need to be moved by both because both offsets
> come from ancestors, but the containing block rect should only be moved by the
> ToVP offset.

Done.

https://codereview.chromium.org/2636253002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:21:
containingBlockRect.move(accumulatedToCB);
On 2017/01/18 14:42:56, flackr wrote:
> The containing block rect should only be moved by the accumulatedToVP, the
> accumulatedToCB should be from a descendant node (i.e. not affect the
containing
> block).

Done.

[flackr, 2017-01-20 02:42:44]

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

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1940:
constraintsMap.contains(layer) &&
Do you know if we do actualy sometimes end up with missing / dirty constraints
at this time? It seems like after scroll would be a clean part of the lifecycle.

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

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:896: if
(scrollAncestor) {
nullptr scrollAncestor just means the scroll ancestor is the FrameView - there
could still be elements between the containingblock and FrameView which would
move the containing block rect so we still need to do the walk then.

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1013: DCHECK(it
!= scrollableArea->stickyConstraintsMap().end());
Please merge this with the check on 1009 above so that we don't look up layer()
in the constraints twice.

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:72:
accumulatedToCB.height() + stickyOffset.height());
FloatSize has arithmetic operators, should be able to do:
m_accumulatedStickyOffsetToContainingBlock = accumulatedToCB + stickyOffset;
(and same below)

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:162:
m_nearestStickyElementFromContainingBlockToScrollContainer;
Maybe m_nearestStickyElementShifting(ContainingBlock|StickyBox)?

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:165:
// sticky elements to use. Because we can either be the "to containing block"
nit: explain that we offset the constraint rects of descendants by these.

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:173:
FloatSize m_accumulatedStickyOffsetFromContainingBlockToScrollContainer;
How about m_totalStickyBoxStickyOffset and m_totalContainingBlockStickyOffset?

[smcgruer, 2017-01-20 17:18:14]

Replying to the non-trivial ones first, will fix the others after these are
resolved.

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

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1940:
constraintsMap.contains(layer) &&
On 2017/01/20 02:42:44, flackr wrote:
> Do you know if we do actualy sometimes end up with missing / dirty constraints
> at this time? It seems like after scroll would be a clean part of the
lifecycle.

It appears so. I don't know the exact reason, but adding:

CHECK(layoutObject->style()->position() != StickyPosition || 
constraintsMap.contains(layer));

Causes a CHECK the second you scroll http://output.jsbin.com/lukekot

I can ping you the stack-trace off bug if you want to take a closer look.

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

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:896: if
(scrollAncestor) {
On 2017/01/20 02:42:44, flackr wrote:
> nullptr scrollAncestor just means the scroll ancestor is the FrameView - there
> could still be elements between the containingblock and FrameView which would
> move the containing block rect so we still need to do the walk then.

So I'm clear, we need to do something like:

    LayoutObject* ancestor = scrollAncestor ? scrollAncestor :
view()->frameView()->layoutView();

Or similar?

[flackr, 2017-01-23 16:45:42]

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

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:896: if
(scrollAncestor) {
On 2017/01/20 17:18:14, smcgruer wrote:
> On 2017/01/20 02:42:44, flackr wrote:
> > nullptr scrollAncestor just means the scroll ancestor is the FrameView -
there
> > could still be elements between the containingblock and FrameView which
would
> > move the containing block rect so we still need to do the walk then.
> 
> So I'm clear, we need to do something like:
> 
>     LayoutObject* ancestor = scrollAncestor ? scrollAncestor :
> view()->frameView()->layoutView();
> 
> Or similar?

Yes, that would work, but probably simpler to use the ancestorOverflowLayer from
above.

[smcgruer, 2017-01-24 15:45:22]

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:72:
accumulatedToCB.height() + stickyOffset.height());
On 2017/01/20 02:42:44, flackr wrote:
> FloatSize has arithmetic operators, should be able to do:
> m_accumulatedStickyOffsetToContainingBlock = accumulatedToCB + stickyOffset;
> (and same below)

Done.

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h
(right):

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:162:
m_nearestStickyElementFromContainingBlockToScrollContainer;
On 2017/01/20 02:42:44, flackr wrote:
> Maybe m_nearestStickyElementShifting(ContainingBlock|StickyBox)?

Done.

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:165:
// sticky elements to use. Because we can either be the "to containing block"
On 2017/01/20 02:42:44, flackr wrote:
> nit: explain that we offset the constraint rects of descendants by these.

Done.

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:173:
FloatSize m_accumulatedStickyOffsetFromContainingBlockToScrollContainer;
On 2017/01/20 02:42:44, flackr wrote:
> How about m_totalStickyBoxStickyOffset and m_totalContainingBlockStickyOffset?

Done.

[smcgruer, 2017-01-24 18:38:25]

I believe the main logic is correct now (let me know if not!), so I will begin
looking at propagating this to the compositor side as well.

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

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:896: if
(scrollAncestor) {
On 2017/01/23 16:45:42, flackr wrote:
> On 2017/01/20 17:18:14, smcgruer wrote:
> > On 2017/01/20 02:42:44, flackr wrote:
> > > nullptr scrollAncestor just means the scroll ancestor is the FrameView -
> there
> > > could still be elements between the containingblock and FrameView which
> would
> > > move the containing block rect so we still need to do the walk then.
> > 
> > So I'm clear, we need to do something like:
> > 
> >     LayoutObject* ancestor = scrollAncestor ? scrollAncestor :
> > view()->frameView()->layoutView();
> > 
> > Or similar?
> 
> Yes, that would work, but probably simpler to use the ancestorOverflowLayer
from
> above.

Done.

https://codereview.chromium.org/2636253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1013: DCHECK(it
!= scrollableArea->stickyConstraintsMap().end());
On 2017/01/20 02:42:44, flackr wrote:
> Please merge this with the check on 1009 above so that we don't look up
layer()
> in the constraints twice.

Done in PS4 (missed this comment.)

[flackr, 2017-01-24 20:28:48]

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

https://codereview.chromium.org/2636253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:908:
maybeStickyAncestor = maybeStickyAncestor->parent();
Technically we should be following the locationContainer chain. Walking the
parent chain might give us an incorrect ancestor if we have a parent that is
fixed position (which would not be contained by its parent). i.e.

<sticky>
  <fixed>
    <sticky> <!-- would think that its fixed container is shifted by the sticky
position element above it -->

[smcgruer, 2017-01-28 00:55:31]

See latest patchset for an attempt at fixing the localToAncestorQuad problem.
Unfortunately it turned out to be very invasive to try and ignore the sticky
offsets, so we may want to contemplate another approach :(

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

https://codereview.chromium.org/2636253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:908:
maybeStickyAncestor = maybeStickyAncestor->parent();
On 2017/01/24 20:28:48, flackr wrote:
> Technically we should be following the locationContainer chain. Walking the
> parent chain might give us an incorrect ancestor if we have a parent that is
> fixed position (which would not be contained by its parent). i.e.
> 
> <sticky>
>   <fixed>
>     <sticky> <!-- would think that its fixed container is shifted by the
sticky
> position element above it -->

Done.

[flackr, 2017-01-31 00:20:33]

https://codereview.chromium.org/2636253002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2076:
offsetFromContainer(container, mode & IgnoreSticky);
I think this might be cleaner if we add staticOffsetFromContainer which excludes
the in flow positioning (and called from offsetFromContainer) or if you just
directly remove offsetForInFlowPosition here if the element is sticky and
ignoresticky is set. Adding arguments to offsetFromContainer feels a bit hacky.

[smcgruer, 2017-01-31 16:07:07]

https://codereview.chromium.org/2636253002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2076:
offsetFromContainer(container, mode & IgnoreSticky);
On 2017/01/31 00:20:33, flackr wrote:
> I think this might be cleaner if we add staticOffsetFromContainer which
excludes
> the in flow positioning (and called from offsetFromContainer) or if you just
> directly remove offsetForInFlowPosition here if the element is sticky and
> ignoresticky is set. Adding arguments to offsetFromContainer feels a bit
hacky.

I've gone for the direct removal for now. It's not pretty, but when I tried to
do the staticOffsetFromContainer implementation I ran into layout test failures
I didnt understand, so leaving the ugliness in for now :).

[flackr, 2017-01-31 18:33:01]

Don't forget to add the full description :-)

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

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:908: }
Create a function for this common loop code, maybe
enclosingStickyObjectBetween(start, ancestor)?

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1043:
.get(toContainingBlock->layer())
nit: Helper function to get sticky constraints for the given layout object?

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1044:
.getTotalStickyBoxStickyOffset();
It'd probably be cleaner to get these as part of computeStickyOffset as this
also updates these total offsets. We could maybe pass references to the
StickyPositionScrollingConstraints of these ancestors.

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2077: containerOffset -=
toLayoutBoxModelObject(this)->offsetForInFlowPosition();
It is a little unfortunate to calculate this offset twice though, maybe leave a
TODO to not include it in the above call to offsetFromContainer?

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/MapCoordinatesFlags.h (right):

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/MapCoordinatesFlags.h:31: IgnoreSticky = 1
<< 5,
I'd prefer IgnoreStickyOffset.

[smcgruer, 2017-01-31 20:42:16]

> Don't forget to add the full description :-)

I have not forgotten! Waiting until I get the compositor part done, so I know
the scope of what to write.

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

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:908: }
On 2017/01/31 18:33:00, flackr wrote:
> Create a function for this common loop code, maybe
> enclosingStickyObjectBetween(start, ancestor)?

I was intended to look at folding these into the walks for finding the
containingBlock/scrollAncestor - do you think we should postpone that for a
follow-up CL and just create a common function atm?

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1043:
.get(toContainingBlock->layer())
On 2017/01/31 18:33:00, flackr wrote:
> nit: Helper function to get sticky constraints for the given layout object?

Done.

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1044:
.getTotalStickyBoxStickyOffset();
On 2017/01/31 18:33:00, flackr wrote:
> It'd probably be cleaner to get these as part of computeStickyOffset as this
> also updates these total offsets. We could maybe pass references to the
> StickyPositionScrollingConstraints of these ancestors.

Done.

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2077: containerOffset -=
toLayoutBoxModelObject(this)->offsetForInFlowPosition();
On 2017/01/31 18:33:00, flackr wrote:
> It is a little unfortunate to calculate this offset twice though, maybe leave
a
> TODO to not include it in the above call to offsetFromContainer?

Done.

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/MapCoordinatesFlags.h (right):

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/MapCoordinatesFlags.h:31: IgnoreSticky = 1
<< 5,
On 2017/01/31 18:33:00, flackr wrote:
> I'd prefer IgnoreStickyOffset.

Done.

[smcgruer, 2017-01-31 20:42:16]

> Don't forget to add the full description :-)

I have not forgotten! Waiting until I get the compositor part done, so I know
the scope of what to write.

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

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:908: }
On 2017/01/31 18:33:00, flackr wrote:
> Create a function for this common loop code, maybe
> enclosingStickyObjectBetween(start, ancestor)?

I was intended to look at folding these into the walks for finding the
containingBlock/scrollAncestor - do you think we should postpone that for a
follow-up CL and just create a common function atm?

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1043:
.get(toContainingBlock->layer())
On 2017/01/31 18:33:00, flackr wrote:
> nit: Helper function to get sticky constraints for the given layout object?

Done.

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1044:
.getTotalStickyBoxStickyOffset();
On 2017/01/31 18:33:00, flackr wrote:
> It'd probably be cleaner to get these as part of computeStickyOffset as this
> also updates these total offsets. We could maybe pass references to the
> StickyPositionScrollingConstraints of these ancestors.

Done.

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2077: containerOffset -=
toLayoutBoxModelObject(this)->offsetForInFlowPosition();
On 2017/01/31 18:33:00, flackr wrote:
> It is a little unfortunate to calculate this offset twice though, maybe leave
a
> TODO to not include it in the above call to offsetFromContainer?

Done.

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/MapCoordinatesFlags.h (right):

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/MapCoordinatesFlags.h:31: IgnoreSticky = 1
<< 5,
On 2017/01/31 18:33:00, flackr wrote:
> I'd prefer IgnoreStickyOffset.

Done.

[smcgruer, 2017-02-02 16:21:41]

Description was changed from

==========
Handle nested position:sticky elements

TODO(smcgruer): Provide a full description

BUG=672710
==========

to

==========
Handle nested position:sticky elements

TODO(smcgruer): Provide a full description

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[smcgruer, 2017-02-02 16:21:42]

Description was changed from

==========
Handle nested position:sticky elements

TODO(smcgruer): Provide a full description

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Handle nested position:sticky elements

TODO(smcgruer): Provide a full description

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[smcgruer, 2017-02-02 16:22:33]

Working on the compositor side. Having some issues, so thought best approach
would be to upload the WiP and discuss on it. There are a number of issues with
the code, mostly marked with TODOs. Simple nested test cases pass, the more
complex ones fail.


Works:
https://output.jsbin.com/doluhej
https://output.jsbin.com/mugileq

Fails:
https://output.jsbin.com/qifubex
https://output.jsbin.com/bovegil
http://output.jsbin.com/sevekej
https://output.jsbin.com/pukoso

[smcgruer, 2017-02-02 16:22:34]

Working on the compositor side. Having some issues, so thought best approach
would be to upload the WiP and discuss on it. There are a number of issues with
the code, mostly marked with TODOs. Simple nested test cases pass, the more
complex ones fail.


Works:
https://output.jsbin.com/doluhej
https://output.jsbin.com/mugileq

Fails:
https://output.jsbin.com/qifubex
https://output.jsbin.com/bovegil
http://output.jsbin.com/sevekej
https://output.jsbin.com/pukoso

[flackr, 2017-02-02 18:18:12]

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

https://codereview.chromium.org/2636253002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:908: }
On 2017/01/31 20:42:16, smcgruer wrote:
> On 2017/01/31 18:33:00, flackr wrote:
> > Create a function for this common loop code, maybe
> > enclosingStickyObjectBetween(start, ancestor)?
> 
> I was intended to look at folding these into the walks for finding the
> containingBlock/scrollAncestor - do you think we should postpone that for a
> follow-up CL and just create a common function atm?

Yes, we should just clean it up with a helper function for this patch.

https://codereview.chromium.org/2636253002/diff/240001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/2636253002/diff/240001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:381: return web_constraint;
We need to copy the ancestor pointers here too right?

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree...
cc/trees/property_tree.cc:373: // [0,-25] for the inner (that seems wrong).
Sounds like we need to update the way this is computed in
CompositedLayerMapping.cpp to be the additional main thread sticky offset from
the nearest ancestor sticky element? In your demo we want the inner ones to have
a main thread offset of 0 (as in 0 additional main thread sticky offset from the
already offset top sticky element).

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree...
cc/trees/property_tree.cc:392: // transform_tree_index of -1.
We can't access the Layers at this point, I believe they will be invalid at the
point when a new tree is committed. I think we should convert the layer pointers
to transform tree indices when generating the sticky position data for the
transform tree.

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree...
cc/trees/property_tree.cc:480: // correctly, inner is offset too far...).
Keep in mind what we are returning is the additional offset which has not
already been applied to the layer from the main thread, [0, 0] is the correct
sticky offset until we do some additional cc scrolling.

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

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:59: if (!o ||
!o->layer() || !o->layer()->ancestorOverflowLayer())
Are all of these checks necessary? If we have a sticky ancestor set I think it
should have a PaintLayer and ancestorOverflowLayer shouldn't it?

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:64: if
(!scrollableArea)
scrollableArea should never be null afaik.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp (right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:385: // There
are two scrollable areas, but we only care about the scroller itself.
You mean #scroller and the viewport? I think this comment is evident from the
code / unnecessary.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:291:
if (!obj) {
nit: no curlies { } on single line if blocks.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:371:
constraints.nearestStickyElementShiftingStickyBox());
If the nearest sticky element is not composited we have to point to the next one
(and so on) to correctly include their composited sticky offset.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:29:
stickyBoxRect.move(ancestorContainingBlockOffset);
nit: stickyBoxRect.move(ancestorStickyBoxOffset + ancestorContainingBlockOffset)

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

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:160:
// NOTE(smcgruer): The case where we can affect both the sticky box constraint
nit: No need for NOTE(smcgruer)

[smcgruer, 2017-02-02 20:21:27]

https://codereview.chromium.org/2636253002/diff/240001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/2636253002/diff/240001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:381: return web_constraint;
On 2017/02/02 18:18:11, flackr wrote:
> We need to copy the ancestor pointers here too right?

Yes, although I'm not immediately clear how to convert cc::Layer back to
WebLayer.

Added TODO so I dont forget.

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree...
cc/trees/property_tree.cc:373: // [0,-25] for the inner (that seems wrong).
On 2017/02/02 18:18:11, flackr wrote:
> Sounds like we need to update the way this is computed in
> CompositedLayerMapping.cpp to be the additional main thread sticky offset from
> the nearest ancestor sticky element? In your demo we want the inner ones to
have
> a main thread offset of 0 (as in 0 additional main thread sticky offset from
the
> already offset top sticky element).

Is there any guarantee in CompositedLayerMapping::updateStickyConstraints that
main thread stick calculation will have happened? (Or that it is being called
top-down on the sticky tree.)

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree...
cc/trees/property_tree.cc:392: // transform_tree_index of -1.
On 2017/02/02 18:18:11, flackr wrote:
> We can't access the Layers at this point, I believe they will be invalid at
the
> point when a new tree is committed. I think we should convert the layer
pointers
> to transform tree indices when generating the sticky position data for the
> transform tree.

Done.

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree...
cc/trees/property_tree.cc:480: // correctly, inner is offset too far...).
On 2017/02/02 18:18:11, flackr wrote:
> Keep in mind what we are returning is the additional offset which has not
> already been applied to the layer from the main thread, [0, 0] is the correct
> sticky offset until we do some additional cc scrolling.

Ah, makes sense.

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

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:59: if (!o ||
!o->layer() || !o->layer()->ancestorOverflowLayer())
On 2017/02/02 18:18:11, flackr wrote:
> Are all of these checks necessary? If we have a sticky ancestor set I think it
> should have a PaintLayer and ancestorOverflowLayer shouldn't it?

I can switch them to DCHECKS, if that makes more sense. It's a fair nightmare of
pointers around here:).

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:64: if
(!scrollableArea)
On 2017/02/02 18:18:11, flackr wrote:
> scrollableArea should never be null afaik.

Replaced with DCHECK then.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp (right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:385: // There
are two scrollable areas, but we only care about the scroller itself.
On 2017/02/02 18:18:11, flackr wrote:
> You mean #scroller and the viewport? I think this comment is evident from the
> code / unnecessary.

Done.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:291:
if (!obj) {
On 2017/02/02 18:18:11, flackr wrote:
> nit: no curlies { } on single line if blocks.

Done.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:371:
constraints.nearestStickyElementShiftingStickyBox());
On 2017/02/02 18:18:11, flackr wrote:
> If the nearest sticky element is not composited we have to point to the next
one
> (and so on) to correctly include their composited sticky offset.

I can have a go at implementing this behavior, but we may have to whiteboard
this one before I understand it :). Not clear how this could happen or why we
would want to end up accumulating offset from something further up the tree...

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp
(right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:29:
stickyBoxRect.move(ancestorContainingBlockOffset);
On 2017/02/02 18:18:11, flackr wrote:
> nit: stickyBoxRect.move(ancestorStickyBoxOffset +
ancestorContainingBlockOffset)

Done.

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

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:160:
// NOTE(smcgruer): The case where we can affect both the sticky box constraint
On 2017/02/02 18:18:11, flackr wrote:
> nit: No need for NOTE(smcgruer)

Done.

[flackr, 2017-02-02 23:31:15]

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2636253002/diff/240001/cc/trees/property_tree...
cc/trees/property_tree.cc:373: // [0,-25] for the inner (that seems wrong).
On 2017/02/02 20:21:26, smcgruer wrote:
> On 2017/02/02 18:18:11, flackr wrote:
> > Sounds like we need to update the way this is computed in
> > CompositedLayerMapping.cpp to be the additional main thread sticky offset
from
> > the nearest ancestor sticky element? In your demo we want the inner ones to
> have
> > a main thread offset of 0 (as in 0 additional main thread sticky offset from
> the
> > already offset top sticky element).
> 
> Is there any guarantee in CompositedLayerMapping::updateStickyConstraints that
> main thread stick calculation will have happened? (Or that it is being called
> top-down on the sticky tree.)

Yes, compositing inputs are clean at at that point and during the compositing
inputs update we update the positions of sticky position layers so we will have
already computed the sticky position offset top-down.

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

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:59: if (!o ||
!o->layer() || !o->layer()->ancestorOverflowLayer())
On 2017/02/02 20:21:27, smcgruer wrote:
> On 2017/02/02 18:18:11, flackr wrote:
> > Are all of these checks necessary? If we have a sticky ancestor set I think
it
> > should have a PaintLayer and ancestorOverflowLayer shouldn't it?
> 
> I can switch them to DCHECKS, if that makes more sense. It's a fair nightmare
of
> pointers around here:).

No need to DCHECK if it will crash on failure trying to derference a null
pointer :-).

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:371:
constraints.nearestStickyElementShiftingStickyBox());
On 2017/02/02 20:21:27, smcgruer wrote:
> On 2017/02/02 18:18:11, flackr wrote:
> > If the nearest sticky element is not composited we have to point to the next
> one
> > (and so on) to correctly include their composited sticky offset.
> 
> I can have a go at implementing this behavior, but we may have to whiteboard
> this one before I understand it :). Not clear how this could happen or why we
> would want to end up accumulating offset from something further up the tree...

On low dpi this is as simple as
<div id='outer' style='position: sticky; will-change: transform;'>
  <div id='middle' style='position: sticky;'>
    <div id='inner' style='position: sticky; will-change: transform;'>
    </div>
  </div>
</div>

#inner's constraint rects will be affected by the main thread update of #middle,
but also by the compositing position update of #outer.

[smcgruer, 2017-02-07 19:05:26]

After our discussions today, another (still hacky) approach for the cc side.
This actually passes the following list of test cases, but as discussed it
should be on the cc side not main side to be always correct.

https://output.jsbin.com/doluhej
https://output.jsbin.com/qifubex
https://output.jsbin.com/bovegil
http://output.jsbin.com/sevekej
https://output.jsbin.com/mugileq
https://output.jsbin.com/pukoso
https://output.jsbin.com/reqeniy (not nested but fails in M56)

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

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:59: if (!o ||
!o->layer() || !o->layer()->ancestorOverflowLayer())
On 2017/02/02 23:31:15, flackr wrote:
> On 2017/02/02 20:21:27, smcgruer wrote:
> > On 2017/02/02 18:18:11, flackr wrote:
> > > Are all of these checks necessary? If we have a sticky ancestor set I
think
> it
> > > should have a PaintLayer and ancestorOverflowLayer shouldn't it?
> > 
> > I can switch them to DCHECKS, if that makes more sense. It's a fair
nightmare
> of
> > pointers around here:).
> 
> No need to DCHECK if it will crash on failure trying to derference a null
> pointer :-).

Did some digging. ancestorOverflowLayer() can be null for the 'this' call in
LayoutBoxModelObject::stickyPositionOffset(). Checking the original method, this
could previously happen so I left that early out. Removed checking for the
layer() though, it should always be non-null.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:64: if
(!scrollableArea)
On 2017/02/02 20:21:27, smcgruer wrote:
> On 2017/02/02 18:18:11, flackr wrote:
> > scrollableArea should never be null afaik.
> 
> Replaced with DCHECK then.

Per advice above, removed since it'll just crash anyway if scrollableArea is
ever null.

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2636253002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:371:
constraints.nearestStickyElementShiftingStickyBox());
On 2017/02/02 23:31:15, flackr wrote:
> On 2017/02/02 20:21:27, smcgruer wrote:
> > On 2017/02/02 18:18:11, flackr wrote:
> > > If the nearest sticky element is not composited we have to point to the
next
> > one
> > > (and so on) to correctly include their composited sticky offset.
> > 
> > I can have a go at implementing this behavior, but we may have to whiteboard
> > this one before I understand it :). Not clear how this could happen or why
we
> > would want to end up accumulating offset from something further up the
tree...
> 
> On low dpi this is as simple as
> <div id='outer' style='position: sticky; will-change: transform;'>
>   <div id='middle' style='position: sticky;'>
>     <div id='inner' style='position: sticky; will-change: transform;'>
>     </div>
>   </div>
> </div>
> 
> #inner's constraint rects will be affected by the main thread update of
#middle,
> but also by the compositing position update of #outer.

Thanks for the repro case. Done except I think I need to use something other
than containing block.

[smcgruer, 2017-02-09 15:46:52]

smcgruer@chromium.org changed reviewers:
+ ajuma@chromium.org

[smcgruer, 2017-02-09 15:46:53]

flackr: Per our conversation, moved the offset adjustment from
CompositedLayerMapping to cc side. There's still some work to be done here, but
wanted your input first.

Adding ajuma as well in case I've done anything horrendous with property trees
:)

[smcgruer, 2017-02-14 15:15:16]

Description was changed from

==========
Handle nested position:sticky elements

TODO(smcgruer): Provide a full description

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Handle nested position:sticky elements correctly

In order to correctly calculate the sticky offset for nested sticky, we must
track the accumulated sticky offset from our ancestor sticky elements. This
accumulation must then be used to adjust the location of the two constraint
rects. 

This patch adds this behavior both for the main thread and cc side. On the
compositor side, we additionally adjust the main_thread_offset to include the
enclosing layer's scroll position and sticky offset. This is not directly
related to nested sticky, but is required for it to work correctly.

As an additional change, we also disallow position: sticky from being applied to
<thead> and <tr> elements, only <th> and <td>. This is necessary because
Chrome's current implementation of in-flow positioned elements does not support
<thead> and <tr> as per the CSS 2.1 spec, so the container/containingBlock logic
is not correct for CSS 3 currently.

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[smcgruer, 2017-02-14 15:16:58]

Description was changed from

==========
Handle nested position:sticky elements correctly

In order to correctly calculate the sticky offset for nested sticky, we must
track the accumulated sticky offset from our ancestor sticky elements. This
accumulation must then be used to adjust the location of the two constraint
rects. 

This patch adds this behavior both for the main thread and cc side. On the
compositor side, we additionally adjust the main_thread_offset to include the
enclosing layer's scroll position and sticky offset. This is not directly
related to nested sticky, but is required for it to work correctly.

As an additional change, we also disallow position: sticky from being applied to
<thead> and <tr> elements, only <th> and <td>. This is necessary because
Chrome's current implementation of in-flow positioned elements does not support
<thead> and <tr> as per the CSS 2.1 spec, so the container/containingBlock logic
is not correct for CSS 3 currently.

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Handle nested position:sticky elements correctly

In order to correctly calculate the sticky offset for nested sticky, we must
track the accumulated sticky offset from our ancestor sticky elements. This
accumulation must then be used to adjust the location of the two constraint
rects.

This patch adds this behavior both for the main thread and cc side. On the
compositor side, we additionally adjust the main_thread_offset to include the
enclosing layer's scroll position and sticky offset. This is not directly
related to nested sticky, but is required for it to work correctly.

Finally, we also disallow position: sticky from being applied to <thead> and
<tr>
elements, only <th> and <td>. This is necessary because Chrome's current
implementation of in-flow positioned elements does not support <thead> and
<tr> as per the CSS 2.1 spec, so the container/containingBlock logic is not
correct
for CSS 3 currently.

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[smcgruer, 2017-02-14 17:02:06]

PTAL.

https://codereview.chromium.org/2636253002/diff/460001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/2636253002/diff/460001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:381: // TODO(smcgruer): Copy over ancestor pointers
too.
flackr : Ali suggested switching from layer pointers to layer ids, as it seemed
like the only way to copy back from cc to blink. Thoughts? Another option would
be to leave this method unfulfilled, as it is only used for tests afaik.

https://codereview.chromium.org/2636253002/diff/460001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2636253002/diff/460001/cc/layers/layer.cc#new...
cc/layers/layer.cc:593: // TODO(smcgruer): Maybe rename
parent_relative_sticky_box_offset ?
flackr : Thoughts on renaming this?

https://codereview.chromium.org/2636253002/diff/460001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/2636253002/diff/460001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:7145: // TODO(smcgruer): Fails. I'm
not sure why. Value is actually [0,50].
flackr : Not sure why this fails. I'd expect it to be 0,25, but the test claims
its 0,50. The sticky offset returned from StickyPositionOffset for the inner
sticky also seems to be wrong to my eye, but I'm not yet sure if it means
there's a bug in the code or this test.

[112059:112059:0214/115758.594979:3107795674364:INFO:property_tree.cc(456)]
StickyPositionOffset for node 4: returning [0.000000 90.000000] - [0.000000
0.000000] - [0.000000 0.000000] - [0 50] = [0.000000 40.000000]

https://codereview.chromium.org/2636253002/diff/460001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:7838: gfx::Vector2dF(20.f, -15.f),
flackr : These changes are also suspicious. I just copied test output to input
to make them pass, but we need to figure out why this would be correct (or if it
isnt).

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

https://codereview.chromium.org/2636253002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:907: //
TODO(smcgruer): Refactor into the code above.
flackr : We should discuss this.

[flackr, 2017-02-21 00:32:43]

https://codereview.chromium.org/2636253002/diff/460001/cc/blink/web_layer_imp...
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/2636253002/diff/460001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:381: // TODO(smcgruer): Copy over ancestor pointers
too.
On 2017/02/14 17:02:06, smcgruer wrote:
> flackr : Ali suggested switching from layer pointers to layer ids, as it
seemed
> like the only way to copy back from cc to blink. Thoughts? Another option
would
> be to leave this method unfulfilled, as it is only used for tests afaik.

It would be nice to be able to test these constraints back on the blink side and
to not lose anything in the conversion back and forth but if it's causing
nontrivial issues, I would be okay with deferring this until later (i.e. file
and reference a bug with TODO) to keep this patch simple.

https://codereview.chromium.org/2636253002/diff/460001/cc/layers/layer_sticky...
File cc/layers/layer_sticky_position_constraint.h (right):

https://codereview.chromium.org/2636253002/diff/460001/cc/layers/layer_sticky...
cc/layers/layer_sticky_position_constraint.h:53: gfx::Vector2dF
total_containing_block_sticky_offset;
These only need to be in the StickyPositionNodeData right? Since we update and
use these during the transform tree update.

[smcgruer, 2017-03-07 15:48:12]

Closed this issue as it has been replaced by
https://codereview.chromium.org/2708883005/ and
https://codereview.chromium.org/2733633002/