Handle nested position:sticky elements correctly (compositor)

Handle nested position:sticky elements correctly (compositor)

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.

On the compositor side, the goal of the sticky calculation is to adjust
the rectangle by any additional composited scroll offset on top of the
main thread scroll position (as the sticky calculation will already have
adjusted for that scroll).

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2733633002
Cr-Commit-Position: refs/heads/master@{#459419}
Committed: https://chromium.googlesource.com/chromium/src/+/29014fffc71e122447fb29f32915ae421b8eded4

[smcgruer, 2017-03-03 20:01:22]

Description was changed from

==========
Handle nested position:sticky elements correctly (compositor)

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.

On the compositor side, the goal of the sticky calculation is to adjust
the rectangle by any additional composited scroll offset on top of the
main thread scroll position (as the sticky calculation will already have
adjusted for that scroll).

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

to

==========
Handle nested position:sticky elements correctly (compositor)

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.

On the compositor side, the goal of the sticky calculation is to adjust
the rectangle by any additional composited scroll offset on top of the
main thread scroll position (as the sticky calculation will already have
adjusted for that scroll).

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

[smcgruer, 2017-03-03 20:01:23]

Description was changed from

==========
Handle nested position:sticky elements correctly (compositor)

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.

On the compositor side, the goal of the sticky calculation is to adjust
the rectangle by any additional composited scroll offset on top of the
main thread scroll position (as the sticky calculation will already have
adjusted for that scroll).

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

to

==========
Handle nested position:sticky elements correctly (compositor)

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.

On the compositor side, the goal of the sticky calculation is to adjust
the rectangle by any additional composited scroll offset on top of the
main thread scroll position (as the sticky calculation will already have
adjusted for that scroll).

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[smcgruer, 2017-03-06 22:17:35]

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

[smcgruer, 2017-03-06 22:17:46]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 22:18:30]

Dry run: 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, 2017-03-07 00:13:25]

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

[commit-bot: I haz the power, 2017-03-07 00:13:26]

Dry run: This issue passed the CQ dry run.

[flackr, 2017-03-09 20:22:09]

https://codereview.chromium.org/2733633002/diff/20002/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/layers/layer.cc#newc...
cc/layers/layer.cc:597: // scroll.
Unless we add a data channel for updating this (similar to updating the layer
position), I don't think we can pass this extra data from the blink side right
now. so it does make sense to do here.

https://codereview.chromium.org/2733633002/diff/20002/cc/layers/layer.cc#newc...
cc/layers/layer.cc:613: layer->transform_tree_index();
I don't understand this, do the constraints or the indices on the node change
without rebuilding the property trees? My understanding was that they are static
until we build a new tree.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1506: // Find nearest ancestor which is sticky,
up to the scroller.
Isn't this ancestor part of the constraints passed in?

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1508: layer = layer->parent();
Can you verify whether this chain is correct in the case of a fixed position
ancestor? (i.e. given sticky > fixed > sticky the internal sticky should not
account for the outer sticky because the fixed is not contained by it).

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1528: while (nextSticky) {
Can we cache the accumulated offset like we do on the blink side so that we can
just grab the total offset from the nearest sticky ancestor?

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree....
cc/trees/property_tree.cc:402: ancestor_sticky_box_offset.y());
nit: sticky_box_rect += ancestor_sticky_box_offset?

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree....
cc/trees/property_tree.cc:452: // TODO(smcgruer): add in the ancestor sticky box
offset here?
Yes, this should exactly mirror LayoutBoxModelObject::stickyPositionOffset().

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree....
cc/trees/property_tree.h:117: int nearest_node_shifting_sticky_box;
nit: Add a comment about these new members.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:519: return gfx::Vector2dF();
Add NOTREACHED() or equivalent in this function.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:777: // TODO(smcgruer): Maybe do this blink
side instead.
Given that we may have to do this on the cc side for layer position updates
there's probably not much to be saved avoiding it here, although we could sync
the accumulated offset over so that it doesn't need to be recalculated here.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:786: sticky_box_offset.OffsetFromOrigin();
It looks like you pass in the main thread offset in the constraint's
localStickyOffset.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:787: // Copy the ancestor nodes for later use.
It's probably worth pointing out in the comment that because these point to
ancestors they should already be set.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:178:
while (obj && obj->compositingState() == NotComposited) {
I think technically you want to find compositingState() == PaintsIntoOwnBacking,
but it's probably better / more consistent to just check if the object has a
compositedLayerMapping since that's what we intend to access.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:182:
obj = constraints.nearestStickyBoxShiftingStickyBox()
This seems wrong, don't we need to follow the same chain as the one we started
on? i.e. if we're interested in nearestStickyBoxShiftingContainingBlock then
only follow those pointers?

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:383:
webConstraint.localStickyOffset = constraints.getLocalStickyOffset();
This seems to be passed in, but unused as far as I can tell. I'm inclined to
think that we shouldn't pass this in since it changes with every scroll but we
don't update the constraints every scroll.

[flackr, 2017-03-09 20:25:21]

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-deep.html
(right):

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-deep.html:21:
will-change: transform;
I don't think you have a test of mixed composited and non composited sticky
elements yet do you?

[smcgruer, 2017-03-10 16:37:11]

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1506: // Find nearest ancestor which is sticky,
up to the scroller.
On 2017/03/09 20:22:08, flackr wrote:
> Isn't this ancestor part of the constraints passed in?

|layer| might not be sticky.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree....
cc/trees/property_tree.cc:402: ancestor_sticky_box_offset.y());
On 2017/03/09 20:22:08, flackr wrote:
> nit: sticky_box_rect += ancestor_sticky_box_offset?

Done.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree....
cc/trees/property_tree.cc:452: // TODO(smcgruer): add in the ancestor sticky box
offset here?
On 2017/03/09 20:22:08, flackr wrote:
> Yes, this should exactly mirror LayoutBoxModelObject::stickyPositionOffset().

Done. I need to figure out a test for this though.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree....
cc/trees/property_tree.h:117: int nearest_node_shifting_sticky_box;
On 2017/03/09 20:22:08, flackr wrote:
> nit: Add a comment about these new members.

Done.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:519: return gfx::Vector2dF();
On 2017/03/09 20:22:08, flackr wrote:
> Add NOTREACHED() or equivalent in this function.

Done.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:777: // TODO(smcgruer): Maybe do this blink
side instead.
On 2017/03/09 20:22:08, flackr wrote:
> Given that we may have to do this on the cc side for layer position updates
> there's probably not much to be saved avoiding it here, although we could sync
> the accumulated offset over so that it doesn't need to be recalculated here.

Acknowledged.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:786: sticky_box_offset.OffsetFromOrigin();
On 2017/03/09 20:22:08, flackr wrote:
> It looks like you pass in the main thread offset in the constraint's
> localStickyOffset.

I don't quite follow this comment; can you clarify?

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:787: // Copy the ancestor nodes for later use.
On 2017/03/09 20:22:08, flackr wrote:
> It's probably worth pointing out in the comment that because these point to
> ancestors they should already be set.

Done.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:178:
while (obj && obj->compositingState() == NotComposited) {
On 2017/03/09 20:22:09, flackr wrote:
> I think technically you want to find compositingState() ==
PaintsIntoOwnBacking,
> but it's probably better / more consistent to just check if the object has a
> compositedLayerMapping since that's what we intend to access.

Done.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:383:
webConstraint.localStickyOffset = constraints.getLocalStickyOffset();
On 2017/03/09 20:22:09, flackr wrote:
> This seems to be passed in, but unused as far as I can tell. I'm inclined to
> think that we shouldn't pass this in since it changes with every scroll but we
> don't update the constraints every scroll.

Its used in draw_property_utils::CalculateTotalStickyOffsetToScroller. We need
it to adjust the main_thread_offset.

[smcgruer, 2017-03-10 16:38:39]

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1506: // Find nearest ancestor which is sticky,
up to the scroller.
On 2017/03/10 16:37:10, smcgruer wrote:
> On 2017/03/09 20:22:08, flackr wrote:
> > Isn't this ancestor part of the constraints passed in?
> 
> |layer| might not be sticky.

Oh I think I just realized what you meant. We only call this on the parent() of
a sticky layer, so instead we can just pass in the nearest ancestor. I can fix
that.

[flackr, 2017-03-13 19:49:45]

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:786: sticky_box_offset.OffsetFromOrigin();
On 2017/03/10 16:37:10, smcgruer wrote:
> On 2017/03/09 20:22:08, flackr wrote:
> > It looks like you pass in the main thread offset in the constraint's
> > localStickyOffset.
> 
> I don't quite follow this comment; can you clarify?

I think we chatted offline about this. There is a main thread offset being
passed in as part of the constraints called localStickyOffset but this will be
stale after some scrolling happens because we shouldn't regularly update the
sticky constraints.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:383:
webConstraint.localStickyOffset = constraints.getLocalStickyOffset();
On 2017/03/10 16:37:10, smcgruer wrote:
> On 2017/03/09 20:22:09, flackr wrote:
> > This seems to be passed in, but unused as far as I can tell. I'm inclined to
> > think that we shouldn't pass this in since it changes with every scroll but
we
> > don't update the constraints every scroll.
> 
> Its used in draw_property_utils::CalculateTotalStickyOffsetToScroller. We need
> it to adjust the main_thread_offset.

As discussed, we should try to work out the local offset on the compositor side
since it will change with calls to Layer::SetPosition without the sticky
constraints being updated.

https://codereview.chromium.org/2733633002/diff/70001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/70001/cc/trees/property_tree....
cc/trees/property_tree.cc:401: sticky_box_rect +=
gfx::ToFlooredVector2d(ancestor_sticky_box_offset +
This loss of precision seems dangerous, we could end up causing jitter on a high
dpi device (e.g. android device) if we don't track the full offset that has
already been applied.

[smcgruer, 2017-03-14 22:04:06]

Large update (sorry!) ahead of our sync tomorrow. I think I've hit most of the
points you made except for the precision issues. We should chat regarding what
we need to do there.

https://codereview.chromium.org/2733633002/diff/20002/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/layers/layer.cc#newc...
cc/layers/layer.cc:597: // scroll.
On 2017/03/09 20:22:08, flackr wrote:
> Unless we add a data channel for updating this (similar to updating the layer
> position), I don't think we can pass this extra data from the blink side right
> now. so it does make sense to do here.

Acknowledged.

https://codereview.chromium.org/2733633002/diff/20002/cc/layers/layer.cc#newc...
cc/layers/layer.cc:613: layer->transform_tree_index();
On 2017/03/09 20:22:08, flackr wrote:
> I don't understand this, do the constraints or the indices on the node change
> without rebuilding the property trees? My understanding was that they are
static
> until we build a new tree.

It was mostly just copy/paste. Removed.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1506: // Find nearest ancestor which is sticky,
up to the scroller.
On 2017/03/10 16:38:38, smcgruer wrote:
> On 2017/03/10 16:37:10, smcgruer wrote:
> > On 2017/03/09 20:22:08, flackr wrote:
> > > Isn't this ancestor part of the constraints passed in?
> > 
> > |layer| might not be sticky.
> 
> Oh I think I just realized what you meant. We only call this on the parent()
of
> a sticky layer, so instead we can just pass in the nearest ancestor. I can fix
> that.

Done.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1508: layer = layer->parent();
On 2017/03/09 20:22:08, flackr wrote:
> Can you verify whether this chain is correct in the case of a fixed position
> ancestor? (i.e. given sticky > fixed > sticky the internal sticky should not
> account for the outer sticky because the fixed is not contained by it).

No longer relevant, we don't do this walk.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1528: while (nextSticky) {
On 2017/03/09 20:22:08, flackr wrote:
> Can we cache the accumulated offset like we do on the blink side so that we
can
> just grab the total offset from the nearest sticky ancestor?

No longer relevant, this code is gone.

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/20002/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:786: sticky_box_offset.OffsetFromOrigin();
On 2017/03/13 19:49:45, flackr wrote:
> On 2017/03/10 16:37:10, smcgruer wrote:
> > On 2017/03/09 20:22:08, flackr wrote:
> > > It looks like you pass in the main thread offset in the constraint's
> > > localStickyOffset.
> > 
> > I don't quite follow this comment; can you clarify?
> 
> I think we chatted offline about this. There is a main thread offset being
> passed in as part of the constraints called localStickyOffset but this will be
> stale after some scrolling happens because we shouldn't regularly update the
> sticky constraints.

Done.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-deep.html
(right):

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-deep.html:21:
will-change: transform;
On 2017/03/09 20:25:20, flackr wrote:
> I don't think you have a test of mixed composited and non composited sticky
> elements yet do you?

Tried, but everything appears to be getting composited anyway 'due to
association with an element with a "position: fixed" or "position: sticky"
style'

Uploaded that test, ptal.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:182:
obj = constraints.nearestStickyBoxShiftingStickyBox()
On 2017/03/09 20:22:08, flackr wrote:
> This seems wrong, don't we need to follow the same chain as the one we started
> on? i.e. if we're interested in nearestStickyBoxShiftingContainingBlock then
> only follow those pointers?

I believe you're correct, done. Adding a test is waiting on resolving copying
back pointers to main from cc.

https://codereview.chromium.org/2733633002/diff/20002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:383:
webConstraint.localStickyOffset = constraints.getLocalStickyOffset();
On 2017/03/13 19:49:45, flackr wrote:
> On 2017/03/10 16:37:10, smcgruer wrote:
> > On 2017/03/09 20:22:09, flackr wrote:
> > > This seems to be passed in, but unused as far as I can tell. I'm inclined
to
> > > think that we shouldn't pass this in since it changes with every scroll
but
> we
> > > don't update the constraints every scroll.
> > 
> > Its used in draw_property_utils::CalculateTotalStickyOffsetToScroller. We
need
> > it to adjust the main_thread_offset.
> 
> As discussed, we should try to work out the local offset on the compositor
side
> since it will change with calls to Layer::SetPosition without the sticky
> constraints being updated.

Done.

[smcgruer, 2017-03-16 13:55:18]

PTAL, I believe I've addressed the main points we discussed yesterday. Remaining
are just a unittest for the ancestor-layer-finding in CompositedLayerMapping,
and fixing the mixed LayoutTest to actually be mixed.

[flackr, 2017-03-17 15:01:56]

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer.cc#new...
cc/layers/layer.cc:32: #include "cc/trees/scroll_node.h"
Obsolete?

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer.cc#new...
cc/layers/layer.cc:594: // element that is not composited:
http://crbug.com/702229
Can you change this to a TODO to either pass the main thread sticky shifting
offset of the non composited element or promote the ancestor sticky element?
i.e. suggests this will be fixed soon.

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

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer_sticky...
cc/layers/layer_sticky_position_constraint.h:49: // The nearest ancestor sticky
layers that affect the sticky box constraint
Mention layer id in the comment, and -1 for no such layer.

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer_sticky...
cc/layers/layer_sticky_position_constraint.h:61: // Returns the nearest sticky
ancestor layer, or null if no such layer exists.
s/null/-1 ?

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree...
cc/trees/property_tree.cc:380: // over bottom offset.
This comment should probably stay next to the limit checks on the constraints.

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree...
cc/trees/property_tree.cc:397: gfx::RectF sticky_box_rect =
nit: Add a short comment explaining what this is doing, i.e. Calculate the
position of the constraint rects given sticky offsets from ancestor sticky
elements.

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree...
cc/trees/property_tree.cc:450: sticky_offset -
sticky_box_rect.OffsetFromOrigin();
I think we should keep the passed in constraints const, consider them strictly
inputs from the main thread layer tree, and add these calculated total offsets
to the StickyPositionNodeData.

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-inline.html
(right):

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-inline.html:7:
/* TODO(smcgruer): Appears to be a paint bug if the scroller is composited. */
What is the bug? We shouldn't be trying to do sticky position updates on the
compositor if the scroll isn't composited, are we? In this case we should just
be relying on the main thread to set the layer position of the sticky elements
and doing nothing more with them.

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

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:395:
.getTotalContainingBlockStickyOffset());
Do we not want to remove all of the sticky offset applied to the ancestor? i.e.
getTotalStickyBoxStickyOffset. Maybe add a test where the ancestor is also
shifted by non containers.

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

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:175:
FloatSize m_localStickyOffset;
This seems unused, remove it?

[smcgruer, 2017-03-17 17:36:52]

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer.cc#new...
cc/layers/layer.cc:32: #include "cc/trees/scroll_node.h"
On 2017/03/17 15:01:55, flackr wrote:
> Obsolete?

Done.

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer.cc#new...
cc/layers/layer.cc:594: // element that is not composited:
http://crbug.com/702229
On 2017/03/17 15:01:55, flackr wrote:
> Can you change this to a TODO to either pass the main thread sticky shifting
> offset of the non composited element or promote the ancestor sticky element?
> i.e. suggests this will be fixed soon.

Done.

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

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer_sticky...
cc/layers/layer_sticky_position_constraint.h:49: // The nearest ancestor sticky
layers that affect the sticky box constraint
On 2017/03/17 15:01:55, flackr wrote:
> Mention layer id in the comment, and -1 for no such layer.

Done.

https://codereview.chromium.org/2733633002/diff/230001/cc/layers/layer_sticky...
cc/layers/layer_sticky_position_constraint.h:61: // Returns the nearest sticky
ancestor layer, or null if no such layer exists.
On 2017/03/17 15:01:55, flackr wrote:
> s/null/-1 ?

Done.

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree...
cc/trees/property_tree.cc:380: // over bottom offset.
On 2017/03/17 15:01:55, flackr wrote:
> This comment should probably stay next to the limit checks on the constraints.

Done.

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree...
cc/trees/property_tree.cc:397: gfx::RectF sticky_box_rect =
On 2017/03/17 15:01:56, flackr wrote:
> nit: Add a short comment explaining what this is doing, i.e. Calculate the
> position of the constraint rects given sticky offsets from ancestor sticky
> elements.

Done.

https://codereview.chromium.org/2733633002/diff/230001/cc/trees/property_tree...
cc/trees/property_tree.cc:450: sticky_offset -
sticky_box_rect.OffsetFromOrigin();
On 2017/03/17 15:01:55, flackr wrote:
> I think we should keep the passed in constraints const, consider them strictly
> inputs from the main thread layer tree, and add these calculated total offsets
> to the StickyPositionNodeData.

Done.

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-inline.html
(right):

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/composited-nested-sticky-inline.html:7:
/* TODO(smcgruer): Appears to be a paint bug if the scroller is composited. */
On 2017/03/17 15:01:56, flackr wrote:
> What is the bug? We shouldn't be trying to do sticky position updates on the
> compositor if the scroll isn't composited, are we? In this case we should just
> be relying on the main thread to set the layer position of the sticky elements
> and doing nothing more with them.

Filed crbug.com/702637 and reverted this LayoutTest. I think we should follow up
after this CL is submitted.

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

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:395:
.getTotalContainingBlockStickyOffset());
On 2017/03/17 15:01:56, flackr wrote:
> Do we not want to remove all of the sticky offset applied to the ancestor?
i.e.
> getTotalStickyBoxStickyOffset. Maybe add a test where the ancestor is also
> shifted by non containers.

That fails compositing/overflow/composited-nested-sticky-deep.html , because
m_totalStickyBoxStickyOffset is only ancestorStickyBoxOffset + stickyOffset, so
doesn't include the ancestorContainingBlockOffset, so for all of those nested
elements its just stickyOffset.

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

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:175:
FloatSize m_localStickyOffset;
On 2017/03/17 15:01:56, flackr wrote:
> This seems unused, remove it?

Done.

[flackr, 2017-03-17 18:04:08]

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

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:395:
.getTotalContainingBlockStickyOffset());
On 2017/03/17 17:36:52, smcgruer wrote:
> On 2017/03/17 15:01:56, flackr wrote:
> > Do we not want to remove all of the sticky offset applied to the ancestor?
> i.e.
> > getTotalStickyBoxStickyOffset. Maybe add a test where the ancestor is also
> > shifted by non containers.
> 
> That fails compositing/overflow/composited-nested-sticky-deep.html , because
> m_totalStickyBoxStickyOffset is only ancestorStickyBoxOffset + stickyOffset,
so
> doesn't include the ancestorContainingBlockOffset, so for all of those nested
> elements its just stickyOffset.

Oh right, but wouldn't this would still be a bug not to include the
non-container additional offsite? Maybe we should add a method to the
constraints like, pardon the long name, 
getTotalScrollContainerRelativeStickyOffset?

https://codereview.chromium.org/2733633002/diff/250001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/layers/layer.cc#new...
cc/layers/layer.cc:593: // non-promoted ancestors, or promote all ancestor
sticky elements.
nit: Also bug link please?

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree.cc:373: if (sticky_data->nearest_node_shifting_sticky_box
>= 0) {
Also all of these comparisons can be != kInvalidNodeId

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree.h:131: : scroll_ancestor(-1),
nit: Can we change these ids to kInvalidNodeId?

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:772: int x =
sticky_data->constraints.nearest_layer_shifting_sticky_box;
nit: Use descriptive variable names

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:773: if (x >= 0) {
Compare to the invalid id constant.

[smcgruer, 2017-03-17 18:42:42]

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

https://codereview.chromium.org/2733633002/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:395:
.getTotalContainingBlockStickyOffset());
On 2017/03/17 18:04:08, flackr wrote:
> On 2017/03/17 17:36:52, smcgruer wrote:
> > On 2017/03/17 15:01:56, flackr wrote:
> > > Do we not want to remove all of the sticky offset applied to the ancestor?
> > i.e.
> > > getTotalStickyBoxStickyOffset. Maybe add a test where the ancestor is also
> > > shifted by non containers.
> > 
> > That fails compositing/overflow/composited-nested-sticky-deep.html , because
> > m_totalStickyBoxStickyOffset is only ancestorStickyBoxOffset + stickyOffset,
> so
> > doesn't include the ancestorContainingBlockOffset, so for all of those
nested
> > elements its just stickyOffset.
> 
> Oh right, but wouldn't this would still be a bug not to include the
> non-container additional offsite? Maybe we should add a method to the
> constraints like, pardon the long name, 
> getTotalScrollContainerRelativeStickyOffset?

As per discussion, we already dothis.

https://codereview.chromium.org/2733633002/diff/250001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/layers/layer.cc#new...
cc/layers/layer.cc:593: // non-promoted ancestors, or promote all ancestor
sticky elements.
On 2017/03/17 18:04:08, flackr wrote:
> nit: Also bug link please?

Done.

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree.cc:373: if (sticky_data->nearest_node_shifting_sticky_box
>= 0) {
On 2017/03/17 18:04:08, flackr wrote:
> Also all of these comparisons can be != kInvalidNodeId

Done.

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree.h:131: : scroll_ancestor(-1),
On 2017/03/17 18:04:08, flackr wrote:
> nit: Can we change these ids to kInvalidNodeId?

Done.

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:772: int x =
sticky_data->constraints.nearest_layer_shifting_sticky_box;
On 2017/03/17 18:04:08, flackr wrote:
> nit: Use descriptive variable names

Done.

https://codereview.chromium.org/2733633002/diff/250001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:773: if (x >= 0) {
On 2017/03/17 18:04:08, flackr wrote:
> Compare to the invalid id constant.

Done.

[smcgruer, 2017-03-17 19:09:17]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 19:10:12]

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

[flackr, 2017-03-17 19:15:00]

LGTM

[smcgruer, 2017-03-17 19:15:54]

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

[smcgruer, 2017-03-17 19:15:55]

+ajuma for cc OWNERS

+chrishtr for Blink OWNERS

[commit-bot: I haz the power, 2017-03-17 19:55:27]

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

[commit-bot: I haz the power, 2017-03-17 19:55:29]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[ajuma, 2017-03-17 23:59:52]

cc lgtm

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

https://codereview.chromium.org/2733633002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8005: // keep going as it hasnt
reached its position yet.
hasn't

https://codereview.chromium.org/2733633002/diff/290001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/290001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:780:
->layer_id_to_transform_node_index[shifting_sticky_box_layer_id];
This needs to use TransformTree::FindNodeIndexFromOwningLayerId.

[smcgruer, 2017-03-20 13:36:30]

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

https://codereview.chromium.org/2733633002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8005: // keep going as it hasnt
reached its position yet.
On 2017/03/17 23:59:52, ajuma wrote:
> hasn't

Done.

https://codereview.chromium.org/2733633002/diff/290001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2733633002/diff/290001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:780:
->layer_id_to_transform_node_index[shifting_sticky_box_layer_id];
On 2017/03/17 23:59:52, ajuma wrote:
> This needs to use TransformTree::FindNodeIndexFromOwningLayerId.

Done.

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

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:176:
static const LayoutBoxModelObject* nearestCompositedStickyAncestor(
I assume one of the layout tests exercises each of these methods? Could you
please dd
comments to the tests explaining the corner of the semantics it is exercising?

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:377:
// enclosing layers offset and the content offset in the composited layer
nit: "layer's"

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp
(left):

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:277:
setBodyInnerHTML(
Please re-add this test to execise the opposite condition (that nested stickies
are composited).

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:117:
return m_nearestStickyBoxShiftingStickyBox
It's not obvious to me why this is correct. Could you explain/add a comment
saying
why?

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h
(right):

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h:56:
WebFloatPoint parentRelativeStickyBoxOffset;
Are these now floating-point to account for adding subpixel sticky offsets? If
so,
how is snapping done in the compositor?

[smcgruer, 2017-03-21 13:54:55]

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:176:
static const LayoutBoxModelObject* nearestCompositedStickyAncestor(
On 2017/03/20 17:24:52, chrishtr wrote:
> I assume one of the layout tests exercises each of these methods? Could you
> please dd
> comments to the tests explaining the corner of the semantics it is exercising?

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

Starting with the first sub-question:

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

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

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

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

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

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:377:
// enclosing layers offset and the content offset in the composited layer
On 2017/03/20 17:24:52, chrishtr wrote:
> nit: "layer's"

Done.

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp
(left):

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:277:
setBodyInnerHTML(
On 2017/03/20 17:24:52, chrishtr wrote:
> Please re-add this test to execise the opposite condition (that nested
stickies
> are composited).

Done.

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:117:
return m_nearestStickyBoxShiftingStickyBox
On 2017/03/20 17:24:52, chrishtr wrote:
> It's not obvious to me why this is correct. Could you explain/add a comment
> saying
> why?

Done.

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h
(right):

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h:56:
WebFloatPoint parentRelativeStickyBoxOffset;
On 2017/03/20 17:24:52, chrishtr wrote:
> Are these now floating-point to account for adding subpixel sticky offsets? If
> so,
> how is snapping done in the compositor?

This came about due to flackr's concerns about losing precision in
total_sticky_box_sticky_offset and total_containing_block_sticky_offset in
patchset 5. Once those were float, there was then a natural progression to clean
things up, including parentRelativeStickyBoxOffset which was created from a
float (the old stickyBoxOffset in CompositedLayerMapping) and was used to set a
Vector2dF (main_thread_offset).

I guess there is a question here as to whether it is correct to remove the
rounding that was previously applied to parentRelativeStickyBoxOffset via
'roundedIntPoint'?

(The output of the cc side is and always was a Vector2dF, which is incorporated
as a translate Transform).

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

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:176:
static const LayoutBoxModelObject* nearestCompositedStickyAncestor(
Ok, flackr and I discussed this and realized that once his patch and the fix for
http://crbug.com/702229 is submitted, any sticky ancestor you have will always
be composited. (flackr's patch has evolved to just remove non-constrained sticky
elements).

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h
(right):

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h:56:
WebFloatPoint parentRelativeStickyBoxOffset;
Discussed with flackr. I overreached in my cleanup attempts. :D. Will move the
parentRelativeStickyBoxOffset back to WebPoint.

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

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

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:176:
static const LayoutBoxModelObject* nearestCompositedStickyAncestor(
On 2017/03/21 16:01:25, smcgruer wrote:
> Ok, flackr and I discussed this and realized that once his patch and the fix
for
> http://crbug.com/702229 is submitted, any sticky ancestor you have will always
> be composited. (flackr's patch has evolved to just remove non-constrained
sticky
> elements).
> 
> I will remove the search and just leave a brief early-exit for parent being
> non-composited which will be replaced in the fix for http://crbug.com/702229

Done.

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h
(right):

https://codereview.chromium.org/2733633002/diff/310001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerStickyPositionConstraint.h:56:
WebFloatPoint parentRelativeStickyBoxOffset;
On 2017/03/21 16:01:25, smcgruer wrote:
> Discussed with flackr. I overreached in my cleanup attempts. :D. Will move the
> parentRelativeStickyBoxOffset back to WebPoint.

Done.

[smcgruer, 2017-03-21 17:37:57]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 17:39:01]

Dry run: 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, 2017-03-21 19:16:40]

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

[commit-bot: I haz the power, 2017-03-21 19:16:41]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-03-21 22:18:28]

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

https://codereview.chromium.org/2733633002/diff/370001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:408:
web_constraint.nearestLayerShiftingContainingBlock;
This seems unused.

[smcgruer, 2017-03-21 22:41:36]

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

https://codereview.chromium.org/2733633002/diff/370001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:408:
web_constraint.nearestLayerShiftingContainingBlock;
On 2017/03/21 22:18:28, chrishtr wrote:
> This seems unused.

It is used. See property_tree_builder.cc line 787, where this is used to
calculate nearest_node_shifting_containing_block, which is then used to get the
ancestor total to adjust the constraint rect.

[chrishtr, 2017-03-21 23:26:05]

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

https://codereview.chromium.org/2733633002/diff/370001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.cc:408:
web_constraint.nearestLayerShiftingContainingBlock;
On 2017/03/21 at 22:41:35, smcgruer wrote:
> On 2017/03/21 22:18:28, chrishtr wrote:
> > This seems unused.
> 
> It is used. See property_tree_builder.cc line 787, where this is used to
calculate nearest_node_shifting_containing_block, which is then used to get the
ancestor total to adjust the constraint rect.

I see, sorry.

It seems weird to me to expose a "containing block" name to cc. cc shouldn't
have to care about
containing blocks. More comments below.

https://codereview.chromium.org/2733633002/diff/370001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/370001/cc/trees/property_tree...
cc/trees/property_tree.cc:398: ancestor_containing_block_offset;
Your code adds on an offset from the ancestor sticky layer, and also an offset
related to the
containing block? Can you give me an example to think through where this offset
is not directly related
or computable from the ancestor sticky layer offset? I'm sure it's the case,
just need to see an
example.

[smcgruer, 2017-03-22 15:15:03]

https://codereview.chromium.org/2733633002/diff/370001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2733633002/diff/370001/cc/trees/property_tree...
cc/trees/property_tree.cc:398: ancestor_containing_block_offset;
On 2017/03/21 23:26:05, chrishtr wrote:
> Your code adds on an offset from the ancestor sticky layer, and also an offset
> related to the
> containing block? Can you give me an example to think through where this
offset
> is not directly related
> or computable from the ancestor sticky layer offset? I'm sure it's the case,
> just need to see an
> example.


If I understand correctly, you are asking about ancestor_sticky_box_offset vs
ancestor_containing_block_offset? (And more generally, the StickyPositionData
for these two ancestor layers).

With sticky elements (layers) there are always two questions to be asked: how
much do I need to offset to be where I want to be, and how much am I *allowed*
to be offset whilst staying inside my container. The former depends on the
'sticky box rect' (which is where we are *before* sticky), the latter depends on
the 'containing block rect'. Note that these are scroll-container relative
afaik.

The positions for these rects are set in the sticky constraints at layout time,
and then we might scroll the page. In the naive non-nested case, this is fine -
we are already calculating the required/allowed offset based on the original
positions plus the current viewport position, so that offset just changes as you
scroll. However in the nested case we run into the issue that both of our
original rects might be wrong, because some ancestor may have been shifted due
to its own sticky offset.

So, we need to correct the sticky box rect (which affects how much we need to
offset) and the containing block rect (which affects how much we can offset).
This is where the ancestor_sticky_box_offset and
ancestor_containing_block_offset come into play. The former accumulates only the
sticky offset of elements between the current sticky element and its containing
block. This is only relevant for elements whose containing block is not their
parent, most commonly tables and inline-divs. The latter accumulates the entire
sticky offset of elements between the containing block and the ancestor scroller
(this includes both of these offsets for ancestor elements, if you look at the
code on line 450 of this file, or 79 of StickyPositionScrollingConstraints.cpp).

So, which of these shifts our two constraint rects? Well, the sticky box rect is
affected by all of the sticky stuff above it, so that is both the
ancestor_sticky_box_offset *and* ancestor_containing_block_offset. On the other
hand, the containing block rect obviously doesn't shift by the sticky stuff
between the current element and the containing block, only the higher-level
sticky, so it only shifts by ancestor_containing_block_offset.

Does that make sense?

With regards to computing it from only the layer that provides the
ancestor_sticky_box_offset, I don't quite see how that could be done. Open to
suggestions though.

In general btw, this code is just a replica of
StickyPositionScrollingConstraints::computeStickyOffset. The only reason it
exists is that we can scroll on the cc side and main will only have applied
sticky offset for the main-thread scroll amount, so we have to fix the offset
for the additional cc scroll position.

[chrishtr, 2017-03-23 23:15:29]

lgtm

[chrishtr, 2017-03-23 23:15:52]

Ok, thanks for the explanations. Complicated, but sticky seems to be inherently
so.

[smcgruer, 2017-03-24 12:33:22]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-03-24 12:33:23]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2733633002/#ps370001
(title: "Revert float changes to unittests too")

[commit-bot: I haz the power, 2017-03-24 12:33:37]

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, 2017-03-24 14:34:49]

CQ is committing da patch.

Bot data: {"patchset_id": 370001, "attempt_start_ts": 1490358802998520,
"parent_rev": "bf765c663d54787ab8dc3bb1b8c1748aab1d0eea", "commit_rev":
"29014fffc71e122447fb29f32915ae421b8eded4"}

[commit-bot: I haz the power, 2017-03-24 14:35:33]

Description was changed from

==========
Handle nested position:sticky elements correctly (compositor)

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.

On the compositor side, the goal of the sticky calculation is to adjust
the rectangle by any additional composited scroll offset on top of the
main thread scroll position (as the sticky calculation will already have
adjusted for that scroll).

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Handle nested position:sticky elements correctly (compositor)

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.

On the compositor side, the goal of the sticky calculation is to adjust
the rectangle by any additional composited scroll offset on top of the
main thread scroll position (as the sticky calculation will already have
adjusted for that scroll).

BUG=672710
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2733633002
Cr-Commit-Position: refs/heads/master@{#459419}
Committed:
https://chromium.googlesource.com/chromium/src/+/29014fffc71e122447fb29f32915...
==========

[commit-bot: I haz the power, 2017-03-24 14:35:35]

Committed patchset #20 (id:370001) as
https://chromium.googlesource.com/chromium/src/+/29014fffc71e122447fb29f32915...