Adapt and reland old position sticky implementation

Adapt and reland old position sticky implementation from https://codereview.chromium.org/346603007

This adds support for position sticky, and plumbs the necessary information to the compositor by which it can maintain the effect during accelerated scrolls.

TEST=fast/css/sticky/*
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/846f623cb1ef78d90e620c2a7a6557544a86b692
Cr-Commit-Position: refs/heads/master@{#384231}

[flackr, 2015-09-15 20:41:26]

Patchset #1 (id:1) has been deleted

[flackr, 2015-09-15 20:41:35]

Patchset #1 (id:20001) has been deleted

[flackr, 2015-09-15 21:13:10]

flackr@chromium.org changed reviewers:
+ esprehn@chromium.org, skobes@chromium.org

[flackr, 2015-09-15 21:13:12]

Hey,

This patch implements sticky positioning for the main thread and should plumb
the necessary information for doing the same on the compositor (modulo undoing
the positioning that the main thread has done). I've fixed this up to the point
where all of the old tests of position sticky pass and made sure that the
geometry layout map assertions aren't firing, though I'm not entirely sure the
interaction with fixed position elements is correct.

Also, previously the frameview did not seem to trigger layer position updates so
I've added this to update sticky position elements in response to the frame view
scrolling.

Please have a look when you get a chance, thanks!

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1579:
layer->updateLayerPositionsAfterOverflowScroll(scrollDelta);
+skobes, does this seem like the right way / place to update the viewport
constrained sticky elements after a frame view scroll?

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutGeometryMap.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutGeometryMap.cpp:78: // TODO(flackr): Verify how fixed
and sticky elements should interact with each other.
My assumption is that this code should only apply to fixed position containers,
I'll have to add an extra flag for fixed vs sticky positioned to track this.

https://codereview.chromium.org/1308273010/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:324: // as a compositing reason.
Is there an easy way to check the scroll ancestor before the ancestor depending
compositing inputs?

[pdr., 2015-09-15 22:24:14]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, wangxianzhu@chromium.org

[pdr., 2015-09-15 22:24:30]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2015-09-15 22:24:31]

+cc chrishtr, wangxianzhu

[skobes, 2015-09-16 00:07:57]

I am vaguely concerned about adding all this complexity where everything needs
to know about position:sticky instead of building it out of simpler primitives. 
But I am not sure what the right primitives are.  Wasn't there a proposal for a
LayoutObserver that would help here?  (+vollick)

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1579:
layer->updateLayerPositionsAfterOverflowScroll(scrollDelta);
On 2015/09/15 21:13:11, flackr wrote:
> +skobes, does this seem like the right way / place to update the viewport
> constrained sticky elements after a frame view scroll?

Possibly, but keep in mind that FrameView scrolls are going away.  With root
layer scrolling you will have to hook
DeprecatedPaintLayerScrollableArea::setScrollOffset.

Related: can sticky-positioned elements be sticky inside overflow scrollers?  Or
do they only stick to the viewport?

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.h:278: virtual LayoutRect
frameRectForStickyPositioning() const = 0;
Can you add a comment explaining what this is and why it is different from
frameRect()?  (Why doesn't frameRect() simply return the correct position for
sticky-positioned elements?)

[chrishtr, 2015-09-17 01:18:56]

I wonder if we really need the compositing integration for a first version.

I think that compositing sticky will be significantly easier in slimming paint
phase 2.

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1579:
layer->updateLayerPositionsAfterOverflowScroll(scrollDelta);
On 2015/09/16 at 00:07:57, skobes wrote:
> On 2015/09/15 21:13:11, flackr wrote:
> > +skobes, does this seem like the right way / place to update the viewport
> > constrained sticky elements after a frame view scroll?
> 
> Possibly, but keep in mind that FrameView scrolls are going away.  With root
layer scrolling you will have to hook
DeprecatedPaintLayerScrollableArea::setScrollOffset.

This method of implementation is not performant enough. Iterating over all of
the position objects will take a long time, and has been a performance
bottleneck in the past.

> 
> Related: can sticky-positioned elements be sticky inside overflow scrollers? 
Or do they only stick to the viewport?

The nearest scrolling ancestor:
https://drafts.csswg.org/css-position/Overview.html#sticky-pos

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.cpp:606: LayoutRect
absoluteStickyBoxRect(LayoutPoint(absContainerFrame.location()) +
stickyLocation, flippedStickyBoxRect.size());
This is bad for performance. Do we really need to store this rect? Why?

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.cpp:678: constrainingRect =
LayoutRect(enclosingClippingBox->localToContainerQuad(FloatRect(clipRect),
view()).boundingBox());
Does clippedOverflowRectForPaintInvalidation do this for you already?

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.h:81: void
computeStickyPositionConstraints(StickyPositionViewportConstraints&, const
LayoutRect& constrainingRect) const;
What are the return values from this? What space is the rect in?

[enne (OOO), 2015-09-17 16:57:39]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2015-09-17 16:57:40]

On 2015/09/17 at 01:18:56, chrishtr wrote:
> I wonder if we really need the compositing integration for a first version.

I'm not sure I understand what the goal of landing this would be without
compositor integration.

https://codereview.chromium.org/1308273010/diff/40001/public/platform/WebLaye...
File public/platform/WebLayerPositionConstraint.h (right):

https://codereview.chromium.org/1308273010/diff/40001/public/platform/WebLaye...
public/platform/WebLayerPositionConstraint.h:55: // TODO(flackr): Combine with
the fixed position constraint above?
Yeah, I think this should probably be merged with the other constraints.

https://codereview.chromium.org/1308273010/diff/40001/public/platform/WebLaye...
public/platform/WebLayerPositionConstraint.h:68: WebRect absoluteStickyBoxRect;
Are these values redundant with WebLayer's bounds and the offsets above? Or can
the sticky part be some subrect of a layer?

[trchen, 2015-09-17 21:23:17]

On 2015/09/17 16:57:40, enne wrote:
> On 2015/09/17 at 01:18:56, chrishtr wrote:
> > I wonder if we really need the compositing integration for a first version.
> 
> I'm not sure I understand what the goal of landing this would be without
> compositor integration.

It is certainly doable if we treat all of them as slow-repaint objects. :p

The compositing integration doesn't look correct though. In particular I'm
worrying about this case:
<div style="position:sticky;">
    <div style="position:relative; z-index:2;"></div>
</div>
<div style="position:absolute; z-index:1;">In between</div>

My understanding is that the rel-pos element belongs to the root stacking
context, not the pseudo stacking context created by sticky-pos, that means the
painting order will be discontiguous. I couldn't find any code in CDPLM that
positions the rel-pos layer in such case.

Another thing I'm worrying about is that there is no specification on stick-pos.
The W3C draft still contains many unresolved issues, and is too unclear about
the interaction with other components. (e.g. transform, multicol.) It is pretty
much implementation-defined by WebKit.

My opinion is NOT to ship stick-pos at all. Did you submit an intent to
implement already? However if we did decide to ship stick-pos, +1 to chrishtr
that forget about compositing integration at this moment in favor of proper
support in SPv2.

[flackr, 2015-09-22 22:25:32]

On 2015/09/17 21:23:17, trchen wrote:
> On 2015/09/17 16:57:40, enne wrote:
> > On 2015/09/17 at 01:18:56, chrishtr wrote:
> > > I wonder if we really need the compositing integration for a first
version.
> > 
> > I'm not sure I understand what the goal of landing this would be without
> > compositor integration.
> 
> It is certainly doable if we treat all of them as slow-repaint objects. :p
> 
> The compositing integration doesn't look correct though. In particular I'm
> worrying about this case:
> <div style="position:sticky;">
>     <div style="position:relative; z-index:2;"></div>
> </div>
> <div style="position:absolute; z-index:1;">In between</div>
> 
> My understanding is that the rel-pos element belongs to the root stacking
> context, not the pseudo stacking context created by sticky-pos, that means the
> painting order will be discontiguous. I couldn't find any code in CDPLM that
> positions the rel-pos layer in such case.

I tried this case, and both Firefox and Safari treat the sticky element as a new
stacking context putting both the sticky div and the relative div behind the
absolute div. I think we should get this added to the spec and avoid these
complex stacking context scenarios.
> 
> Another thing I'm worrying about is that there is no specification on
stick-pos.
> The W3C draft still contains many unresolved issues, and is too unclear about
> the interaction with other components. (e.g. transform, multicol.) It is
pretty
> much implementation-defined by WebKit.
> 
> My opinion is NOT to ship stick-pos at all. Did you submit an intent to
> implement already? However if we did decide to ship stick-pos, +1 to chrishtr
> that forget about compositing integration at this moment in favor of proper
> support in SPv2.

I had mistakenly assumed that given we already had a launch bug for sticky we
had just hit a roadblock but still intended to implement sticky. I've just
submitted an intent to implement.

Given that sticky forms a stacking context in both Firefox and Safari, the
compositor integration is actually pretty simple.

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1579:
layer->updateLayerPositionsAfterOverflowScroll(scrollDelta);
On 2015/09/17 01:18:56, chrishtr wrote:
> On 2015/09/16 at 00:07:57, skobes wrote:
> > On 2015/09/15 21:13:11, flackr wrote:
> > > +skobes, does this seem like the right way / place to update the viewport
> > > constrained sticky elements after a frame view scroll?
> > 
> > Possibly, but keep in mind that FrameView scrolls are going away.  With root
> layer scrolling you will have to hook
> DeprecatedPaintLayerScrollableArea::setScrollOffset.
> 
> This method of implementation is not performant enough. Iterating over all of
> the position objects will take a long time, and has been a performance
> bottleneck in the past.

Agreed, my intention is to only add those sticky objects whose nearest ancestor
scroller is the FrameView to the FrameView's viewport constrained objects.
Currently, this information is not available when the style changes but I
believe Ojan was okay with propagating nearest ancestor scroller down during the
style recalc. Do you think this needs to be resolved before landing anything?
> 
> > 
> > Related: can sticky-positioned elements be sticky inside overflow scrollers?

> Or do they only stick to the viewport?
> 
> The nearest scrolling ancestor:
> https://drafts.csswg.org/css-position/Overview.html#sticky-pos
> 
>

https://codereview.chromium.org/1308273010/diff/40001/public/platform/WebLaye...
File public/platform/WebLayerPositionConstraint.h (right):

https://codereview.chromium.org/1308273010/diff/40001/public/platform/WebLaye...
public/platform/WebLayerPositionConstraint.h:68: WebRect absoluteStickyBoxRect;
On 2015/09/17 16:57:40, enne wrote:
> Are these values redundant with WebLayer's bounds and the offsets above? Or
can
> the sticky part be some subrect of a layer?

I don't think so, but please correct me if I am mistaken about the layer's
bounds. The sticky position depends on the element's in flow position. Is it
possible to determine where the layer was before it was transformed /
positioned? (e.g. if it's anchored to the top it must not move above it's in
flow position)

[chrishtr, 2015-10-06 17:08:33]

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:468: // background
painting through columns / column groups
Is this recorded in a bug?

According to this spec:

https://drafts.csswg.org/css-position-3/#valdef-position-sticky

position: sticky is not supposed to apply to any table elements.

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

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1564: // Update sticky
positioned elements which scroll with the viewport.
Are position: sticky elements spec'ed to stick to the viewport only, or to the
containing scroller?

https://drafts.csswg.org/css-position-3/#sticky-pos

indicates it should be the containing scroller. Just the viewport would be very
problematic, and introduce another way in which containing block and paint
properties disagree, when in fact the paint team is pushing to go in the
opposite direction.

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3173:
DisableCompositingQueryAsserts disabler;
What is the code path that is now active which leads to querying compositing
state at the wrong time?

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3181:
DisableCompositingQueryAsserts disabler;
Same question here.

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:868: LayoutRect
frameRectForStickyPositioning() const override final { return frameRect(); }
final implies override

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

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:623: LayoutUnit
horizontalOffsets = constraints.rightOffset() + constraints.leftOffset();
You're reading leftOffset() here and then setting it possibly below on line 635?
Why is that correct?

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:681: LayoutRect
LayoutBoxModelObject::computeStickyConstrainingRect() const
Is this part of implementing the 6-step algorithm spec'ed here?

https://drafts.csswg.org/css-position-3/#sticky-pos

[flackr, 2015-10-07 20:38:12]

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.cpp:606: LayoutRect
absoluteStickyBoxRect(LayoutPoint(absContainerFrame.location()) +
stickyLocation, flippedStickyBoxRect.size());
On 2015/09/17 01:18:56, chrishtr wrote:
> This is bad for performance. Do we really need to store this rect? Why?

We need to save this rect because sticky objects only begin to stick once they
are past their static position (e.g. a top sticky element should only ever be
pushed down, see the first note in
https://drafts.csswg.org/css-position-3/#sticky-pos)

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.cpp:678: constrainingRect =
LayoutRect(enclosingClippingBox->localToContainerQuad(FloatRect(clipRect),
view()).boundingBox());
On 2015/09/17 01:18:56, chrishtr wrote:
> Does clippedOverflowRectForPaintInvalidation do this for you already?

Unless I'm not understanding / using this correctly, this doesn't seem to remove
the padding or border.

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.h:81: void
computeStickyPositionConstraints(StickyPositionViewportConstraints&, const
LayoutRect& constrainingRect) const;
On 2015/09/17 01:18:56, chrishtr wrote:
> What are the return values from this? What space is the rect in?

Added a comment.

https://codereview.chromium.org/1308273010/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBoxModelObject.h:278: virtual LayoutRect
frameRectForStickyPositioning() const = 0;
On 2015/09/16 00:07:57, skobes (OOO until Oct 9) wrote:
> Can you add a comment explaining what this is and why it is different from
> frameRect()?  (Why doesn't frameRect() simply return the correct position for
> sticky-positioned elements?)

Done.

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:468: // background
painting through columns / column groups
On 2015/10/06 17:08:33, chrishtr wrote:
> Is this recorded in a bug?
> 
> According to this spec:
> 
> https://drafts.csswg.org/css-position-3/#valdef-position-sticky
> 
> position: sticky is not supposed to apply to any table elements.

Hmm, doesn't look like it, just the bug for a crash allowing sticky in this
context used to cause. See the original review:
https://codereview.chromium.org/196413002/

According to https://bugs.webkit.org/show_bug.cgi?id=112024 table rows should be
able to stick. Perhaps I'm mistaken, but my reading of the spec was that for
table elements it should have the same ability to "offset" as relative does
(https://drafts.csswg.org/css-position-3/#valdef-position-relative) - which I
thought meant that columns and column groups will not be offset but rows and row
groups can be offset. There are even a couple tests for this:
fast/css/sticky/sticky-table-row-top.html
fast/css/sticky/sticky-table-thead-top.html 

It is strange that this doesn't seem to be consistent with the code above which
suggests that header groups, row groups, footer groups and rows do not offset.
It also looks like there are known bugs with table rows being absolute position
containing blocks (http://crbug.com/443190)...

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

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1564: // Update sticky
positioned elements which scroll with the viewport.
On 2015/10/06 17:08:33, chrishtr wrote:
> Are position: sticky elements spec'ed to stick to the viewport only, or to the
> containing scroller?
> 
> https://drafts.csswg.org/css-position-3/#sticky-pos
> 
> indicates it should be the containing scroller. Just the viewport would be
very
> problematic, and introduce another way in which containing block and paint
> properties disagree, when in fact the paint team is pushing to go in the
> opposite direction.

They're spec'ed to stick to the viewport if no ancestor has a scrolling box.
Otherwise, they just stick to that containing scroller. In the latest patch I've
updated this to only add sticky elements with no ancestor scrolling box to the
viewport constrained objects.

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3173:
DisableCompositingQueryAsserts disabler;
On 2015/10/06 17:08:33, chrishtr wrote:
> What is the code path that is now active which leads to querying compositing
> state at the wrong time?

The stack trace is here:
#0  0x00007fdaea9281b1 in blink::PaintLayer::compositingState
(this=0x12df96c10e80) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2213
#1  0x00007fdaea9294a9 in blink::PaintLayer::isPaintInvalidationContainer
(this=0x12df96c10e80) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:878
#2  0x00007fdaea9293a8 in
blink::PaintLayer::updateLayerPositionsAfterOverflowScroll (this=0x12df96c10e80,
scrollDelta=...) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:330
#3  0x00007fdaea6f8515 in
blink::FrameView::updateLayersAndCompositingAfterScrollIfNeeded
(this=0x1fffcd344010, scrollDelta=...) at
../../third_party/WebKit/Source/core/frame/FrameView.cpp:1547
#4  0x00007fdaea6fa212 in blink::FrameView::scrollTo (this=0x1fffcd344010,
newPosition=...) at
../../third_party/WebKit/Source/core/frame/FrameView.cpp:2104

Basically we try to query if we're scrolling a paint invalidation container and
update paint invalidation for the scroll when we update layer positions in
response to the scroll.

What do you think about adding PaintLayer::updateLayerPositionAfterFrameScroll
which shouldn't need to recurse and (not sure how correct this is) doesn't
update paint invalidation? (See latest patch) This doesn't require disabling
compositing query asserts.

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3181:
DisableCompositingQueryAsserts disabler;
On 2015/10/06 17:08:33, chrishtr wrote:
> Same question here.

Same as above.

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:868: LayoutRect
frameRectForStickyPositioning() const override final { return frameRect(); }
On 2015/10/06 17:08:33, chrishtr wrote:
> final implies override

Done.

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

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:623: LayoutUnit
horizontalOffsets = constraints.rightOffset() + constraints.leftOffset();
On 2015/10/06 17:08:33, chrishtr wrote:
> You're reading leftOffset() here and then setting it possibly below on line
635?
> Why is that correct?

This is strange, I had lifted this from the old sticky code which seems to have
done the same. I've updated this to use the style right and left / top and
bottom.

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:681: LayoutRect
LayoutBoxModelObject::computeStickyConstrainingRect() const
On 2015/10/06 17:08:33, chrishtr wrote:
> Is this part of implementing the 6-step algorithm spec'ed here?
> 
> https://drafts.csswg.org/css-position-3/#sticky-pos

No, this rectangle is the "viewport" used to determine if and by how much the
sticky element "projects" outside of the sticky constraint rectangle.

[flackr, 2015-10-30 19:30:54]

ping?

[chrishtr, 2015-10-30 20:37:11]

Sorry, got bogged down in other stuff and this CL got starved. Will address it
Monday at the latest.

[chrishtr, 2015-11-04 01:40:44]

Some more comments, still reading...

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:468: // background
painting through columns / column groups
On 2015/10/07 at 20:38:12, flackr wrote:
> On 2015/10/06 17:08:33, chrishtr wrote:
> > Is this recorded in a bug?
> > 
> > According to this spec:
> > 
> > https://drafts.csswg.org/css-position-3/#valdef-position-sticky
> > 
> > position: sticky is not supposed to apply to any table elements.
> 
> Hmm, doesn't look like it, just the bug for a crash allowing sticky in this
context used to cause. See the original review:
> https://codereview.chromium.org/196413002/
> 
> According to https://bugs.webkit.org/show_bug.cgi?id=112024 table rows should
be able to stick. Perhaps I'm mistaken, but my reading of the spec was that for
table elements it should have the same ability to "offset" as relative does
(https://drafts.csswg.org/css-position-3/#valdef-position-relative) - which I
thought meant that columns and column groups will not be offset but rows and row
groups can be offset. There are even a couple tests for this:
> fast/css/sticky/sticky-table-row-top.html
> fast/css/sticky/sticky-table-thead-top.html 
> 
> It is strange that this doesn't seem to be consistent with the code above
which suggests that header groups, row groups, footer groups and rows do not
offset. It also looks like there are known bugs with table rows being absolute
position containing blocks (http://crbug.com/443190)...

The spec says: "The effect of position: sticky on table elements is the same as
for position: relative". Doesn't that mean that table elements can't be sticky?

As crbug.com/443190 indicates (I duped it into another bug BTW), tables are
special and some of the subclasses can't be containing
blocks in our current implementation. This may actually be a constraint of the
way they are spec'ed, I never got to the bottom of looking into that.

See https://code.google.com/p/chromium/issues/detail?id=477495#c9

I think we should likely only support sticky for LayoutTable, but not any of the
other table-related classes. Maybe also LayoutTableCell?

It appears the lines in this function implement that restriction? Please confirm
and add a comment explaining.

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

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3173:
DisableCompositingQueryAsserts disabler;
On 2015/10/07 at 20:38:12, flackr wrote:
> On 2015/10/06 17:08:33, chrishtr wrote:
> > What is the code path that is now active which leads to querying compositing
> > state at the wrong time?
> 
> The stack trace is here:
> #0  0x00007fdaea9281b1 in blink::PaintLayer::compositingState
(this=0x12df96c10e80) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2213
> #1  0x00007fdaea9294a9 in blink::PaintLayer::isPaintInvalidationContainer
(this=0x12df96c10e80) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:878
> #2  0x00007fdaea9293a8 in
blink::PaintLayer::updateLayerPositionsAfterOverflowScroll (this=0x12df96c10e80,
scrollDelta=...) at
../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:330
> #3  0x00007fdaea6f8515 in
blink::FrameView::updateLayersAndCompositingAfterScrollIfNeeded
(this=0x1fffcd344010, scrollDelta=...) at
../../third_party/WebKit/Source/core/frame/FrameView.cpp:1547
> #4  0x00007fdaea6fa212 in blink::FrameView::scrollTo (this=0x1fffcd344010,
newPosition=...) at
../../third_party/WebKit/Source/core/frame/FrameView.cpp:2104
> 
> Basically we try to query if we're scrolling a paint invalidation container
and update paint invalidation for the scroll when we update layer positions in
response to the scroll.
> 
> What do you think about adding PaintLayer::updateLayerPositionAfterFrameScroll
which shouldn't need to recurse and (not sure how correct this is) doesn't
update paint invalidation? (See latest patch) This doesn't require disabling
compositing query asserts.

Why is the compositing data stale when setScrollOffset is called here? Accessing
that data is only illegal when the document lifecycle has not yet been fully
advanced.

[chrishtr, 2015-11-04 01:55:18]

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1568: for (const auto&
viewportConstrainedObject : *m_viewportConstrainedObjects) {
Shouldn't this go in PaintLayerScrollableArea? This kind of code needs to run
for scrollers that are not the view also..

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

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:181: static
inline LayoutBox* findScrollAncestor(const Node* startNode)
enclosingLayer()->ancestorScrollingLayer()

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:658: // Map to
the view to avoid including page scale factor.
Why would this avoid including page scale factor?

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

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:67: // If container
is the root LayoutView (step 0) we want to apply its viewport constrained
position offset.
Sticky elements are not quite viewport constrained, they are containing scroller
constrained. Doesn't this mean that the changes here are not correct in those
cases?

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:78: // TODO(flackr):
Verify how fixed and sticky elements should interact with each other.
Meaning: what should happen with a sticky element below a fixed one?

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.h (right):

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.h:248: LayoutRect
frameRectForStickyPositioning() const override final { return
LayoutRect(linesBoundingBox()); }
Rather than adding a new virtual API method to handle differences between block
and inline layout, I think it would be better
to just do type inspection in LayoutBoxModelObject and call either
linesBoundingBox() or frameRect() as appropriate.

[flackr, 2015-11-25 20:47:53]

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1568: for (const auto&
viewportConstrainedObject : *m_viewportConstrainedObjects) {
On 2015/11/04 01:55:18, chrishtr wrote:
> Shouldn't this go in PaintLayerScrollableArea? This kind of code needs to run
> for scrollers that are not the view also..

The root frame doesn't use PaintLayerScrollableArea as far as I can tell. The
ScrollableArea for the root frame is a RootFrameViewport.

Scrollers that are not the root frame view are actually updated by
PaintLayerScrollableArea::setScrollOffset when it calls
updateLayerPositionsAfterOverflowScroll(scrollDelta).

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

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:181: static
inline LayoutBox* findScrollAncestor(const Node* startNode)
On 2015/11/04 01:55:18, chrishtr wrote:
> enclosingLayer()->ancestorScrollingLayer()

ancestorScrollingLayer uses ancestorDependentCompositingInputs but we're using
it in styleDidChange and layout related functions so these haven't been created
/ recalculated yet.

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:658: // Map to
the view to avoid including page scale factor.
On 2015/11/04 01:55:18, chrishtr wrote:
> Why would this avoid including page scale factor?

The page scale is applied above the view().

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

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:67: // If container
is the root LayoutView (step 0) we want to apply its viewport constrained
position offset.
On 2015/11/04 01:55:18, chrishtr wrote:
> Sticky elements are not quite viewport constrained, they are containing
scroller
> constrained. Doesn't this mean that the changes here are not correct in those
> cases?

Yes, I rethought this and it seems we can get by with simply passing the sticky
offset. This field can be re-used if and when other cases occur.

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutGeometryMap.cpp:78: // TODO(flackr):
Verify how fixed and sticky elements should interact with each other.
On 2015/11/04 01:55:18, chrishtr wrote:
> Meaning: what should happen with a sticky element below a fixed one?

Correct. FWIW I have now tested both of these and they have reasonable behavior
(what you'd expect to happen) under Firefox and this patch matches Firefox's
behavior so I've removed the comment.

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutInline.h (right):

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutInline.h:248: LayoutRect
frameRectForStickyPositioning() const override final { return
LayoutRect(linesBoundingBox()); }
On 2015/11/04 01:55:18, chrishtr wrote:
> Rather than adding a new virtual API method to handle differences between
block
> and inline layout, I think it would be better
> to just do type inspection in LayoutBoxModelObject and call either
> linesBoundingBox() or frameRect() as appropriate.

Done.

[flackr, 2015-11-25 21:03:01]

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:468: // background
painting through columns / column groups
On 2015/11/04 01:40:43, chrishtr wrote:
> On 2015/10/07 at 20:38:12, flackr wrote:
> > On 2015/10/06 17:08:33, chrishtr wrote:
> > > Is this recorded in a bug?
> > > 
> > > According to this spec:
> > > 
> > > https://drafts.csswg.org/css-position-3/#valdef-position-sticky
> > > 
> > > position: sticky is not supposed to apply to any table elements.
> > 
> > Hmm, doesn't look like it, just the bug for a crash allowing sticky in this
> context used to cause. See the original review:
> > https://codereview.chromium.org/196413002/
> > 
> > According to https://bugs.webkit.org/show_bug.cgi?id=112024 table rows
should
> be able to stick. Perhaps I'm mistaken, but my reading of the spec was that
for
> table elements it should have the same ability to "offset" as relative does
> (https://drafts.csswg.org/css-position-3/#valdef-position-relative) - which I
> thought meant that columns and column groups will not be offset but rows and
row
> groups can be offset. There are even a couple tests for this:
> > fast/css/sticky/sticky-table-row-top.html
> > fast/css/sticky/sticky-table-thead-top.html 
> > 
> > It is strange that this doesn't seem to be consistent with the code above
> which suggests that header groups, row groups, footer groups and rows do not
> offset. It also looks like there are known bugs with table rows being absolute
> position containing blocks (http://crbug.com/443190)...
> 
> The spec says: "The effect of position: sticky on table elements is the same
as
> for position: relative". Doesn't that mean that table elements can't be
sticky?
> 
> As crbug.com/443190 indicates (I duped it into another bug BTW), tables are
> special and some of the subclasses can't be containing
> blocks in our current implementation. This may actually be a constraint of the
> way they are spec'ed, I never got to the bottom of looking into that.
> 
> See https://code.google.com/p/chromium/issues/detail?id=477495#c9
> 
> I think we should likely only support sticky for LayoutTable, but not any of
the
> other table-related classes. Maybe also LayoutTableCell?
> 
> It appears the lines in this function implement that restriction? Please
confirm
> and add a comment explaining.

There are many public examples where sticky is used on a table row to produce an
always visible table header. I do believe it is intended to work on table rows.

Under position: relative on
https://drafts.csswg.org/css-position/#valdef-position-relative it says that
only table-column-group and table-column do not offset.

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

https://codereview.chromium.org/1308273010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3173:
DisableCompositingQueryAsserts disabler;
On 2015/11/04 01:40:44, chrishtr wrote:
> On 2015/10/07 at 20:38:12, flackr wrote:
> > On 2015/10/06 17:08:33, chrishtr wrote:
> > > What is the code path that is now active which leads to querying
compositing
> > > state at the wrong time?
> > 
> > The stack trace is here:
> > #0  0x00007fdaea9281b1 in blink::PaintLayer::compositingState
> (this=0x12df96c10e80) at
> ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2213
> > #1  0x00007fdaea9294a9 in blink::PaintLayer::isPaintInvalidationContainer
> (this=0x12df96c10e80) at
> ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:878
> > #2  0x00007fdaea9293a8 in
> blink::PaintLayer::updateLayerPositionsAfterOverflowScroll
(this=0x12df96c10e80,
> scrollDelta=...) at
> ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:330
> > #3  0x00007fdaea6f8515 in
> blink::FrameView::updateLayersAndCompositingAfterScrollIfNeeded
> (this=0x1fffcd344010, scrollDelta=...) at
> ../../third_party/WebKit/Source/core/frame/FrameView.cpp:1547
> > #4  0x00007fdaea6fa212 in blink::FrameView::scrollTo (this=0x1fffcd344010,
> newPosition=...) at
> ../../third_party/WebKit/Source/core/frame/FrameView.cpp:2104
> > 
> > Basically we try to query if we're scrolling a paint invalidation container
> and update paint invalidation for the scroll when we update layer positions in
> response to the scroll.
> > 
> > What do you think about adding
PaintLayer::updateLayerPositionAfterFrameScroll
> which shouldn't need to recurse and (not sure how correct this is) doesn't
> update paint invalidation? (See latest patch) This doesn't require disabling
> compositing query asserts.
> 
> Why is the compositing data stale when setScrollOffset is called here?
Accessing
> that data is only illegal when the document lifecycle has not yet been fully
> advanced.

We have the same disabler in PaintLayerScrollableArea::setScrollOffset
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
It's a known bug that we're processing scroll updates before compositing update:
http://crbug.com/420741

[flackr, 2015-12-03 19:05:53]

ping?

[chrishtr, 2015-12-09 00:37:38]

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1568: for (const auto&
viewportConstrainedObject : *m_viewportConstrainedObjects) {
On 2015/11/25 at 20:47:52, flackr wrote:
> On 2015/11/04 01:55:18, chrishtr wrote:
> > Shouldn't this go in PaintLayerScrollableArea? This kind of code needs to
run
> > for scrollers that are not the view also..
> 
> The root frame doesn't use PaintLayerScrollableArea as far as I can tell. The
ScrollableArea for the root frame is a RootFrameViewport.
> 
> Scrollers that are not the root frame view are actually updated by
PaintLayerScrollableArea::setScrollOffset when it calls
updateLayerPositionsAfterOverflowScroll(scrollDelta).

What about iframes?

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1573:
layer->updateLayerPositionAfterFrameScroll(scrollDelta);
So on a scroll, first the positions of the sticky elements will be updated, then
they will invalidate paint for themselves?
Does the invalidation happen in scrollContentsFastPath (for the root)?

I'm trying to understand in particular how this will work correctly with
position-sticky elements
on top of a composited scroller.

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

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:635: void
LayoutBoxModelObject::computeStickyPositionConstraints(StickyPositionViewportConstraints&
constraints, const LayoutRect& constrainingRect) const
I have a bunch of questions about this method. The best way forward though is to
do a VC to discuss in person, which will make the review much faster. Could you
schedule a meeting for tomorrow?

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:328: void
PaintLayer::updateLayerPositionAfterFrameScroll(const DoubleSize& scrollDelta)
What's the point of declaring a new method with an argument that does nothing
but call another method?

[chrishtr, 2015-12-09 19:12:21]

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1573:
layer->updateLayerPositionAfterFrameScroll(scrollDelta);
On 2015/12/09 at 00:37:38, chrishtr wrote:
> So on a scroll, first the positions of the sticky elements will be updated,
then they will invalidate paint for themselves?
> Does the invalidation happen in scrollContentsFastPath (for the root)?
> 
> I'm trying to understand in particular how this will work correctly with
position-sticky elements
> on top of a composited scroller.

I think PaintLayerScrollableArea, line 384, needs to remove the
layer()->isRootLayer() condition, since sticky elements may not be composited
even if they are within a composited scroller.

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

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:666: LayoutSize
scrollOffset(containingBlock->layer()->scrollableArea()->adjustedScrollOffset());
Is this code useless? Can the containing block be a scroller without also being
the sticky scroll container?

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:727:
clipRect.contract(LayoutSize(enclosingClippingBox->paddingLeft() +
enclosingClippingBox->paddingRight(),
Why is special code needed to remove padding?

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.cpp
(right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.cpp:95:
LayoutPoint
StickyPositionViewportConstraints::layerPositionForViewportRect(const
LayoutRect& viewportRect) const
Remove, not called

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(left):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:91:
FloatPoint layerPositionForViewportRect(const FloatRect& viewportRect) const;
Not called, remove.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:93: const
FloatRect& viewportRectAtLastLayout() const { return m_viewportRectAtLastLayout;
}
Maybe a bunch of these are never called?

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:112:
FloatRect m_viewportRectAtLastLayout;
Remove these if not called.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:139: const
LayoutSize stickyOffsetAtLastLayout() const { return m_stickyOffsetAtLastLayout;
}
Remove the unused accessor functions for now.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:157:
LayoutRect absoluteContainingBlockRect() const { return
m_absoluteContainingBlockRect; }
Remove the accesssor function, not used.

[flackr, 2015-12-10 23:43:15]

Thanks for the review. In the interest of keeping this review fresh, responding
to most of your comments early. I still need to look into how the position is
updated / sticky objects invalidated correctly for composted / noncomposited
scroller / sticky objects. I will follow up with a response on that comment
hopefully tomorrow.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1573:
layer->updateLayerPositionAfterFrameScroll(scrollDelta);
On 2015/12/09 19:12:21, chrishtr wrote:
> On 2015/12/09 at 00:37:38, chrishtr wrote:
> > So on a scroll, first the positions of the sticky elements will be updated,
> then they will invalidate paint for themselves?
> > Does the invalidation happen in scrollContentsFastPath (for the root)?
> > 
> > I'm trying to understand in particular how this will work correctly with
> position-sticky elements
> > on top of a composited scroller.
> 
> I think PaintLayerScrollableArea, line 384, needs to remove the
> layer()->isRootLayer() condition, since sticky elements may not be composited
> even if they are within a composited scroller.

Actually, sticky elements which have an ancestor scroller (e.g. not the
FrameView) are not even in the viewportConstrainedObjects.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:1573:
layer->updateLayerPositionAfterFrameScroll(scrollDelta);
On 2015/12/09 00:37:38, chrishtr wrote:
> So on a scroll, first the positions of the sticky elements will be updated,
then
> they will invalidate paint for themselves?
> Does the invalidation happen in scrollContentsFastPath (for the root)?

> I'm trying to understand in particular how this will work correctly with
> position-sticky elements
> on top of a composited scroller.


All of these cases do seem to be working, but I need to instrument and figure
out how these paths work to be able to explain why. I will get back to you on
this.

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

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:635: void
LayoutBoxModelObject::computeStickyPositionConstraints(StickyPositionViewportConstraints&
constraints, const LayoutRect& constrainingRect) const
On 2015/12/09 00:37:38, chrishtr wrote:
> I have a bunch of questions about this method. The best way forward though is
to
> do a VC to discuss in person, which will make the review much faster. Could
you
> schedule a meeting for tomorrow?

Acknowledged.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:666: LayoutSize
scrollOffset(containingBlock->layer()->scrollableArea()->adjustedScrollOffset());
On 2015/12/09 19:12:21, chrishtr wrote:
> Is this code useless? Can the containing block be a scroller without also
being
> the sticky scroll container?

No, the containing block can't be a scroller without also being the sticky
scroll container afaik, but this is necessary. If the containing block is also
the scroller, absContainerFrame.location() is the location of the scroller which
does not include the scroll offset. When we go to set the absoluteStickyBoxRect
it then thinks that it's always at the same "absolute" position regardless of
how much the container has been scrolled.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:727:
clipRect.contract(LayoutSize(enclosingClippingBox->paddingLeft() +
enclosingClippingBox->paddingRight(),
On 2015/12/09 19:12:21, chrishtr wrote:
> Why is special code needed to remove padding?

The padding is not clipped from an overflow box, so we need to remove it here.
In the spec it says that we stick to the flow box rect which I think means the
rect in which the scroller's content flows in, and this is consistent with how
its implemented by Firefox and Safari.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.cpp
(right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.cpp:95:
LayoutPoint
StickyPositionViewportConstraints::layerPositionForViewportRect(const
LayoutRect& viewportRect) const
On 2015/12/09 19:12:21, chrishtr wrote:
> Remove, not called

Done.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(left):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:91:
FloatPoint layerPositionForViewportRect(const FloatRect& viewportRect) const;
On 2015/12/09 19:12:21, chrishtr wrote:
> Not called, remove.

Done.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:93: const
FloatRect& viewportRectAtLastLayout() const { return m_viewportRectAtLastLayout;
}
On 2015/12/09 19:12:21, chrishtr wrote:
> Maybe a bunch of these are never called?

Done.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:112:
FloatRect m_viewportRectAtLastLayout;
On 2015/12/09 19:12:21, chrishtr wrote:
> Remove these if not called.

Done.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:139: const
LayoutSize stickyOffsetAtLastLayout() const { return m_stickyOffsetAtLastLayout;
}
On 2015/12/09 19:12:21, chrishtr wrote:
> Remove the unused accessor functions for now.

Done.

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:157:
LayoutRect absoluteContainingBlockRect() const { return
m_absoluteContainingBlockRect; }
On 2015/12/09 19:12:21, chrishtr wrote:
> Remove the accesssor function, not used.

Done, as well as absoluteStickyBoxRect().

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:328: void
PaintLayer::updateLayerPositionAfterFrameScroll(const DoubleSize& scrollDelta)
On 2015/12/09 00:37:38, chrishtr wrote:
> What's the point of declaring a new method with an argument that does nothing
> but call another method?

I was trying to be consistent with the other methods identifying the purpose or
time for updating the layer position, but I guess this is silly, made
updateLayerPosition public instead.

[chrishtr, 2015-12-11 02:06:12]

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

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:666: LayoutSize
scrollOffset(containingBlock->layer()->scrollableArea()->adjustedScrollOffset());
On 2015/12/10 at 23:43:15, flackr wrote:
> On 2015/12/09 19:12:21, chrishtr wrote:
> > Is this code useless? Can the containing block be a scroller without also
being
> > the sticky scroll container?
> 
> No, the containing block can't be a scroller without also being the sticky
scroll container afaik, but this is necessary. If the containing block is also
the scroller, absContainerFrame.location() is the location of the scroller which
does not include the scroll offset. When we go to set the absoluteStickyBoxRect
it then thinks that it's always at the same "absolute" position regardless of
how much the container has been scrolled.

Ok please add a comment explaining this then.

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

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:186: while
(curBox && !curBox->isLayoutView()) {
Use enclosingLayer()->scrollParent() instead.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:671: LayoutRect
absContainerFrame =
LayoutRect(containingBlock->localToContainerQuad(FloatRect(FloatPoint(),
FloatSize(containingBlock->size())), view()).boundingBox());
I don't think this should be in LayoutView coordinates, it should be in the
coordinates of the scroller.
Do you know of a testcase where page scale comes into play?

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747: LayoutRect
constrainingRect = computeStickyConstrainingRect();
This will compute these rects from scratch on every scroll. You should be able
to get it set up so that the only thing that happens on each scroll is to add
the scroll offset to the sticky bounds and compare with the already-computed
containing block and scroller viewport.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(left):

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:66:
FloatSize alignmentOffset() const { return m_alignmentOffset; }
Why the change from float to layout units here? This affects fixed-position
also, if it needs to be changed please break it out into another patch and
commit that first.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(right):

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:26: #ifndef
ScrollingConstraints_h
Rename this file ViewportConstraints and add a new one for
StickyPositionViewportConstraints.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:223: bool
updateLayerPosition();
It appears no one cares about the return value of this method, change to void.

[flackr, 2016-01-19 15:40:48]

PTAL, and let me know if you have any suggestions for when/how and where to
cache the computed rects. I'll keep thinking about it too.

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

https://codereview.chromium.org/1308273010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:666: LayoutSize
scrollOffset(containingBlock->layer()->scrollableArea()->adjustedScrollOffset());
On 2015/12/11 02:06:11, chrishtr wrote:
> On 2015/12/10 at 23:43:15, flackr wrote:
> > On 2015/12/09 19:12:21, chrishtr wrote:
> > > Is this code useless? Can the containing block be a scroller without also
> being
> > > the sticky scroll container?
> > 
> > No, the containing block can't be a scroller without also being the sticky
> scroll container afaik, but this is necessary. If the containing block is also
> the scroller, absContainerFrame.location() is the location of the scroller
which
> does not include the scroll offset. When we go to set the
absoluteStickyBoxRect
> it then thinks that it's always at the same "absolute" position regardless of
> how much the container has been scrolled.
> 
> Ok please add a comment explaining this then.

Done.

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

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:186: while
(curBox && !curBox->isLayoutView()) {
On 2015/12/11 02:06:12, chrishtr wrote:
> Use enclosingLayer()->scrollParent() instead.

ancestorScrollingLayer (and scrollParent, which is something different) relies
on ancestorDependentCompositingInputs which are not available during layout when
we ask for this. I've added a check to see if we can query the compositing
inputs state and use that if so. This is the case during scrolling when we're
asking for the position from the compositing update step.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:671: LayoutRect
absContainerFrame =
LayoutRect(containingBlock->localToContainerQuad(FloatRect(FloatPoint(),
FloatSize(containingBlock->size())), view()).boundingBox());
On 2015/12/11 02:06:12, chrishtr wrote:
> I don't think this should be in LayoutView coordinates, it should be in the
> coordinates of the scroller.
> Do you know of a testcase where page scale comes into play?

So this is specifically excluding page scale, which is if I understand
correctly, above the view(), which means relative to the scroller should be just
fine. I've refactored this to be relative to the scroll ancestor.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747: LayoutRect
constrainingRect = computeStickyConstrainingRect();
On 2015/12/11 02:06:12, chrishtr wrote:
> This will compute these rects from scratch on every scroll. You should be able
> to get it set up so that the only thing that happens on each scroll is to add
> the scroll offset to the sticky bounds and compare with the already-computed
> containing block and scroller viewport.

I've been trying to wrap my head around how we can do this correctly, updating
at the right times.

It seems that we would need to make layout updates on the scroll ancestor /
containing block descend to sticky position children. One caveat is that
LayoutInline::layout() is not supposed to be reached but sticky objects can be
inline.

Once we did have the update of these rects happening at the right time, would I
then then add a rare data member to LayoutBoxModelObject to save these details?

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(left):

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:66:
FloatSize alignmentOffset() const { return m_alignmentOffset; }
On 2015/12/11 02:06:12, chrishtr wrote:
> Why the change from float to layout units here? This affects fixed-position
> also, if it needs to be changed please break it out into another patch and
> commit that first.

Undone. At the time I thought it'd made for fewer conversions but I'm not sure.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h
(right):

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ScrollingConstraints.h:26: #ifndef
ScrollingConstraints_h
On 2015/12/11 02:06:12, chrishtr wrote:
> Rename this file ViewportConstraints and add a new one for
> StickyPositionViewportConstraints.

Done.

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:223: bool
updateLayerPosition();
On 2015/12/11 02:06:12, chrishtr wrote:
> It appears no one cares about the return value of this method, change to void.

Done.

[flackr, 2016-01-28 22:30:24]

Description was changed from

==========
Adapt and reland old position sticky implementation from
https://codereview.chromium.org/346603007

This adds support for position sticky, and plumbs the necessary information to
the compositor by which it can maintain the effect during accelerated scrolls.

TEST=fast/css/sticky/*
==========

to

==========
Adapt and reland old position sticky implementation from
https://codereview.chromium.org/346603007

This adds support for position sticky, and plumbs the necessary information to
the compositor by which it can maintain the effect during accelerated scrolls.

TEST=fast/css/sticky/*
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[chrishtr, 2016-01-30 01:49:03]

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

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747: LayoutRect
constrainingRect = computeStickyConstrainingRect();
On 2016/01/19 at 15:40:48, flackr wrote:
> On 2015/12/11 02:06:12, chrishtr wrote:
> > This will compute these rects from scratch on every scroll. You should be
able
> > to get it set up so that the only thing that happens on each scroll is to
add
> > the scroll offset to the sticky bounds and compare with the already-computed
> > containing block and scroller viewport.
> 
> I've been trying to wrap my head around how we can do this correctly, updating
at the right times.
> 
> It seems that we would need to make layout updates on the scroll ancestor /
containing block descend to sticky position children. One caveat is that
LayoutInline::layout() is not supposed to be reached but sticky objects can be
inline.

What do you mean exactly that "LayoutInline::layout() is not supposed to be
reached but sticky objects can be inline" ?

> 
> Once we did have the update of these rects happening at the right time, would
I then then add a rare data member to LayoutBoxModelObject to save these
details?

Yes. It'd be: run the lifecycle code in FrameView up to compositing, then update
all sticky constraints for elements which
need it based on dirty bits. Store these constraints in their LayoutObject's
rare data.

This would make position sticky offsets something that is computed after layout
and just before paint, similar to how transforms are updated after layout and
are (mostly) not affecting layout. But this would mean that position:sticky
elements would have an updated in-flow offset that layout wouldn't know about,
so we need to make sure that doesn't change
anything about layout. I think it should not, in fact I don't know why these
constraints would have to be applied in that
way. Is there some reason?

Alternatively, we can think of it as a purely paint effect that has nothing to
do with layout, which is spec'ed to
work out. I hope the spec was set up to make this work?

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

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:186: if (layer &&
!layer->needsCompositingInputsUpdate()) {
The computations for position:sticky should be done after the compositing update
is done, which means you can rely on this.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:195: //
TODO(flackr): We could add scroll ancestor as an input to layout so that
PaintLayer's scrollParent ancestor-dependent input does this.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:648: while
(containingBlock->isAnonymous()) {
Why exactly? Is there a test for this?

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:676: // FIXME:
sucks to call localToAbsolute again, but we can't just offset from the
previously computed rect if there are transforms.
s/localToAbsolute/localToAncestorQuad

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:678: FloatRect
absContainerFrame = containingBlock->localToAncestorQuad(FloatRect(FloatPoint(),
FloatSize(containingBlock->size())), scrollAncestor).boundingBox();
s/absContainerFrame/stickyBoxRect/

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:687: // We can't
call localToAbsolute on |this| because that will recur. FIXME: For now, assume
that |this| is not transformed.
What is this comment about again? Why a need to map coordinates?
absContainerFrame is already in scroller space.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:694: if
(!style()->left().isAuto() && !style()->right().isAuto()) {
Document this code and what it's for.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:713: //
FIXME(ostap): Exclude top or bottom edge offset depending on the writing mode
when related
s/FIXME/TODO/ throughout, and replace names with flakr

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h
(right):

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h:12:
class StickyPositionViewportConstraints final : public ViewportConstraints {
On further reflection, StickyPositionScrollingConstraints is more accurate.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h:43:
void setAbsoluteContainingBlockRect(const FloatRect& rect) {
m_absoluteContainingBlockRect = rect; }
setScrollContainerRelativeContainingBlockRect

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h:45:
void setAbsoluteStickyBoxRect(const FloatRect& rect) { m_absoluteStickyBoxRect =
rect; }
setScrollContainerRelativeStickyBoxRect

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/scrolling/ViewportConstraints.h (left):

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/ViewportConstraints.h:27: #define
ScrollingConstraints_h
Rename to ScrollingConstraints? Sorry for going in circles on this name with
you. :(

[flackr, 2016-02-03 22:48:38]

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

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747: LayoutRect
constrainingRect = computeStickyConstrainingRect();
On 2016/01/30 at 01:49:02, chrishtr wrote:
> On 2016/01/19 at 15:40:48, flackr wrote:
> > On 2015/12/11 02:06:12, chrishtr wrote:
> > > This will compute these rects from scratch on every scroll. You should be
able
> > > to get it set up so that the only thing that happens on each scroll is to
add
> > > the scroll offset to the sticky bounds and compare with the
already-computed
> > > containing block and scroller viewport.
> > 
> > I've been trying to wrap my head around how we can do this correctly,
updating at the right times.
> > 
> > It seems that we would need to make layout updates on the scroll ancestor /
containing block descend to sticky position children. One caveat is that
LayoutInline::layout() is not supposed to be reached but sticky objects can be
inline.
> 
> What do you mean exactly that "LayoutInline::layout() is not supposed to be
reached but sticky objects can be inline" ?

My original thinking was that I may need to call layout on sticky objects to
update their saved sticky constraints, but LayoutInline::layout() has an
ASSERT_NOT_REACHED() so I assumed that this would not be correct.
> 
> > 
> > Once we did have the update of these rects happening at the right time,
would I then then add a rare data member to LayoutBoxModelObject to save these
details?
> 
> Yes. It'd be: run the lifecycle code in FrameView up to compositing, then
update all sticky constraints for elements which
> need it based on dirty bits. Store these constraints in their LayoutObject's
rare data.

So to be clear, this would be somewhere in PaintLayerCompositor::updateIfNeeded
after the compositing input update?

As for detecting dirtiness, I notice there is LayoutBox::frameRectChanged which
could be the right place to detect the containing block / ancestor scroller size
change and set some flag on sticky descendants at that point.

If the ancestor scroller changes I imagine this could be detected during the
compositing inputs update and mark the sticky object dirty then.

> 
> This would make position sticky offsets something that is computed after
layout and just before paint, similar to how transforms are updated after layout
and are (mostly) not affecting layout. But this would mean that position:sticky
elements would have an updated in-flow offset that layout wouldn't know about,
so we need to make sure that doesn't change
> anything about layout. I think it should not, in fact I don't know why these
constraints would have to be applied in that
> way. Is there some reason?

I don't think there's any problems with this. As long the offset will be correct
if JS queries element.offsetTop after updating some sticky property / making an
element sticky it should be fine. I had been assuming (perhaps incorrectly) that
querying offset from JS only advanced the lifecycle past the layout stage.
> 
> Alternatively, we can think of it as a purely paint effect that has nothing to
do with layout, which is spec'ed to
> work out. I hope the spec was set up to make this work?

Is LayoutObject::invalidateTreeIfNeeded perhaps a good hook for when to dirty
sticky constraints? It seems like this happens whenever sizes change or things
move and that this should happen after compositing inputs have been updated:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

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

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:186: if (layer &&
!layer->needsCompositingInputsUpdate()) {
On 2016/01/30 at 01:49:02, chrishtr wrote:
> The computations for position:sticky should be done after the compositing
update is done, which means you can rely on this.

Currently this is also called during styleChanged to determine if the sticky
object needs to be added to the viewportconstrained objects. I will move
updating viewport constrained objects to where we compute the sticky constraints
so that we can rely on this.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:648: while
(containingBlock->isAnonymous()) {
On 2016/01/30 at 01:49:02, chrishtr wrote:
> Why exactly? Is there a test for this?

I was matching firefox and safari on
http://flackr.github.io/compositor-worker/sticky/sticky.html. In particular, the
top sticky element is contained by an anonymous block above the spacer div,
which on other browsers does not constrain it. That being said, I don't see any
specific language in the spec about this. I will add a test for it.

[chrishtr, 2016-02-08 21:46:55]

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

https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747: LayoutRect
constrainingRect = computeStickyConstrainingRect();
On 2016/02/03 at 22:48:38, flackr wrote:
> On 2016/01/30 at 01:49:02, chrishtr wrote:
> > On 2016/01/19 at 15:40:48, flackr wrote:
> > > On 2015/12/11 02:06:12, chrishtr wrote:
> > > > This will compute these rects from scratch on every scroll. You should
be able
> > > > to get it set up so that the only thing that happens on each scroll is
to add
> > > > the scroll offset to the sticky bounds and compare with the
already-computed
> > > > containing block and scroller viewport.
> > > 
> > > I've been trying to wrap my head around how we can do this correctly,
updating at the right times.
> > > 
> > > It seems that we would need to make layout updates on the scroll ancestor
/ containing block descend to sticky position children. One caveat is that
LayoutInline::layout() is not supposed to be reached but sticky objects can be
inline.
> > 
> > What do you mean exactly that "LayoutInline::layout() is not supposed to be
reached but sticky objects can be inline" ?
> 
> My original thinking was that I may need to call layout on sticky objects to
update their saved sticky constraints, but LayoutInline::layout() has an
ASSERT_NOT_REACHED() so I assumed that this would not be correct.
> > 
> > > 
> > > Once we did have the update of these rects happening at the right time,
would I then then add a rare data member to LayoutBoxModelObject to save these
details?
> > 
> > Yes. It'd be: run the lifecycle code in FrameView up to compositing, then
update all sticky constraints for elements which
> > need it based on dirty bits. Store these constraints in their LayoutObject's
rare data.
> 
> So to be clear, this would be somewhere in
PaintLayerCompositor::updateIfNeeded after the compositing input update?

No, in FrameView::updateLifecyclePhasesInternal, after compositing update. Just
before the call to scrollContentsIfNeededRecursive(), probably.

> 
> As for detecting dirtiness, I notice there is LayoutBox::frameRectChanged
which could be the right place to detect the containing block / ancestor
scroller size change and set some flag on sticky descendants at that point.
> 
> If the ancestor scroller changes I imagine this could be detected during the
compositing inputs update and mark the sticky object dirty then.

Updating layout which may affect the sticky-positioned element's positioning
should require re-computation of it. It's not just that the scroller's rect
changed; there could be other changes of intermediate elements which affect the
sticky element constraint rects.

The simplest way I can think of to be correct is to clear out the sticky
position constraints for a scroller if anything about it or its contents changes
(maybe if any style or layout on any element in the scroller, including itself,
changed?), and then in the part where you update those constraints, look to see
if they were cleared out, and if so recompute.

> 
> > 
> > This would make position sticky offsets something that is computed after
layout and just before paint, similar to how transforms are updated after layout
and are (mostly) not affecting layout. But this would mean that position:sticky
elements would have an updated in-flow offset that layout wouldn't know about,
so we need to make sure that doesn't change
> > anything about layout. I think it should not, in fact I don't know why these
constraints would have to be applied in that
> > way. Is there some reason?
> 
> I don't think there's any problems with this. As long the offset will be
correct if JS queries element.offsetTop after updating some sticky property /
making an element sticky it should be fine. I had been assuming (perhaps
incorrectly) that querying offset from JS only advanced the lifecycle past the
layout stage.

Element::offsetTop currently only advances to layout clean, and only for the
current frame, unfortunately. :( It's really spec'ed to require this?

Options I can think of:

1. Change the spec?
2. Change Element::offsetTop to do more work. This may make some microbenchmarks
slower.
3. #2, but only if there is a position:sticky element present. This hacks around
the benchmarks.
4. Run the computation of ancestor- and descendant-dependent PaintLayer
properties (i.e. CompositingInputsUpdater) inside of the sticky position update
code, if needed, in its own
lifecycle phases with or separate from sticky.

I think I like 4 or 1 the best. 4 shouldn't be super hard, I can walk you
through it as a preparatory CL.

> > 
> > Alternatively, we can think of it as a purely paint effect that has nothing
to do with layout, which is spec'ed to
> > work out. I hope the spec was set up to make this work?
> 
> Is LayoutObject::invalidateTreeIfNeeded perhaps a good hook for when to dirty
sticky constraints? It seems like this happens whenever sizes change or things
move and that this should happen after compositing inputs have been updated:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

See my comment above. I don't think it would be a good idea to conflate the
dirtiness you would be tracking here with invalidation.

[flackr, 2016-02-10 17:02:36]

Thanks for your comments, I'm starting to get a much better big picture.

On 2016/02/08 at 21:46:55, chrishtr wrote:
>
https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747: LayoutRect
constrainingRect = computeStickyConstrainingRect();
> On 2016/02/03 at 22:48:38, flackr wrote:
> > On 2016/01/30 at 01:49:02, chrishtr wrote:
> > > On 2016/01/19 at 15:40:48, flackr wrote:
> > > > On 2015/12/11 02:06:12, chrishtr wrote:
> > > > > This will compute these rects from scratch on every scroll. You should
be able
> > > > > to get it set up so that the only thing that happens on each scroll is
to add
> > > > > the scroll offset to the sticky bounds and compare with the
already-computed
> > > > > containing block and scroller viewport.
> > > > 
> > > > I've been trying to wrap my head around how we can do this correctly,
updating at the right times.
> > > > 
> > > > It seems that we would need to make layout updates on the scroll
ancestor / containing block descend to sticky position children. One caveat is
that LayoutInline::layout() is not supposed to be reached but sticky objects can
be inline.
> > > 
> > > What do you mean exactly that "LayoutInline::layout() is not supposed to
be reached but sticky objects can be inline" ?
> > 
> > My original thinking was that I may need to call layout on sticky objects to
update their saved sticky constraints, but LayoutInline::layout() has an
ASSERT_NOT_REACHED() so I assumed that this would not be correct.
> > > 
> > > > 
> > > > Once we did have the update of these rects happening at the right time,
would I then then add a rare data member to LayoutBoxModelObject to save these
details?
> > > 
> > > Yes. It'd be: run the lifecycle code in FrameView up to compositing, then
update all sticky constraints for elements which
> > > need it based on dirty bits. Store these constraints in their
LayoutObject's rare data.
> > 
> > So to be clear, this would be somewhere in
PaintLayerCompositor::updateIfNeeded after the compositing input update?
> 
> No, in FrameView::updateLifecyclePhasesInternal, after compositing update.
Just before the call to scrollContentsIfNeededRecursive(), probably.
> 
> > 
> > As for detecting dirtiness, I notice there is LayoutBox::frameRectChanged
which could be the right place to detect the containing block / ancestor
scroller size change and set some flag on sticky descendants at that point.
> > 
> > If the ancestor scroller changes I imagine this could be detected during the
compositing inputs update and mark the sticky object dirty then.
> 
> Updating layout which may affect the sticky-positioned element's positioning
should require re-computation of it. It's not just that the scroller's rect
changed; there could be other changes of intermediate elements which affect the
sticky element constraint rects.
> 
> The simplest way I can think of to be correct is to clear out the sticky
position constraints for a scroller if anything about it or its contents changes
(maybe if any style or layout on any element in the scroller, including itself,
changed?), and then in the part where you update those constraints, look to see
if they were cleared out, and if so recompute.
> 
> > 
> > > 
> > > This would make position sticky offsets something that is computed after
layout and just before paint, similar to how transforms are updated after layout
and are (mostly) not affecting layout. But this would mean that position:sticky
elements would have an updated in-flow offset that layout wouldn't know about,
so we need to make sure that doesn't change
> > > anything about layout. I think it should not, in fact I don't know why
these constraints would have to be applied in that
> > > way. Is there some reason?
> > 
> > I don't think there's any problems with this. As long the offset will be
correct if JS queries element.offsetTop after updating some sticky property /
making an element sticky it should be fine. I had been assuming (perhaps
incorrectly) that querying offset from JS only advanced the lifecycle past the
layout stage.
> 
> Element::offsetTop currently only advances to layout clean, and only for the
current frame, unfortunately. :( It's really spec'ed to require this?
> 
> Options I can think of:
> 
> 1. Change the spec?
> 2. Change Element::offsetTop to do more work. This may make some
microbenchmarks slower.
> 3. #2, but only if there is a position:sticky element present. This hacks
around the benchmarks.
> 4. Run the computation of ancestor- and descendant-dependent PaintLayer
properties (i.e. CompositingInputsUpdater) inside of the sticky position update
code, if needed, in its own
> lifecycle phases with or separate from sticky.
> 
> I think I like 4 or 1 the best. 4 shouldn't be super hard, I can walk you
through it as a preparatory CL.
> 

I like option 4, and in fact I was wondering if we couldn't compute ancestor
dependent input properties after style, but it looks like the scroll ancestor
chain only includes layers which actually overflow (requiring layout).
Firefox and Safari consider the sticky ancestor scrolling box as the first one
to not have "overflow: visible" regardless of whether the content overflows.
This makes it knowable after style.
The spec says it should be the "nearest ancestor with a scrolling box"
(https://www.w3.org/TR/css3-positioning/#sticky-pos) where scrolling box is
defined in CSSOM View as "Elements and viewports have an associated scrolling
box if has a scrolling mechanism or it overflows its content area and the used
value of the 'overflow-x' or 'overflow-y' property is 'hidden'."
(https://www.w3.org/TR/cssom-view/#scrolling-box).
Unless I'm misreading, this seems to be inconsistent, but necessary given "if
sticky positioning causes an 'overflow: auto' or 'overflow: scroll' box to have
overflow, the user agent must allow the user to access this content (at its
offset position), which, through the creation of a scrolling mechanism, may
affect layout." (https://www.w3.org/TR/css3-positioning/#sticky-pos).

I'm wondering if we should also seek to clarify the spec on this matter?

> > > 
> > > Alternatively, we can think of it as a purely paint effect that has
nothing to do with layout, which is spec'ed to
> > > work out. I hope the spec was set up to make this work?

I believe this line from the spec requires it to be part of layout: "if sticky
positioning causes an 'overflow: auto' or 'overflow: scroll' box to have
overflow, the user agent must allow the user to access this content (at its
offset position), which, through the creation of a scrolling mechanism, may
affect layout." (https://www.w3.org/TR/css3-positioning/#sticky-pos).

> > Is LayoutObject::invalidateTreeIfNeeded perhaps a good hook for when to
dirty sticky constraints? It seems like this happens whenever sizes change or
things move and that this should happen after compositing inputs have been
updated:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> See my comment above. I don't think it would be a good idea to conflate the
dirtiness you would be tracking here with invalidation.

[chrishtr, 2016-02-10 19:32:54]

On 2016/02/10 at 17:02:36, flackr wrote:
> Thanks for your comments, I'm starting to get a much better big picture.
> 
> On 2016/02/08 at 21:46:55, chrishtr wrote:
> >
https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> > 
> >
https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747:
LayoutRect constrainingRect = computeStickyConstrainingRect();
> > On 2016/02/03 at 22:48:38, flackr wrote:
> > > On 2016/01/30 at 01:49:02, chrishtr wrote:
> > > > On 2016/01/19 at 15:40:48, flackr wrote:
> > > > > On 2015/12/11 02:06:12, chrishtr wrote:
> > > > > > This will compute these rects from scratch on every scroll. You
should be able
> > > > > > to get it set up so that the only thing that happens on each scroll
is to add
> > > > > > the scroll offset to the sticky bounds and compare with the
already-computed
> > > > > > containing block and scroller viewport.
> > > > > 
> > > > > I've been trying to wrap my head around how we can do this correctly,
updating at the right times.
> > > > > 
> > > > > It seems that we would need to make layout updates on the scroll
ancestor / containing block descend to sticky position children. One caveat is
that LayoutInline::layout() is not supposed to be reached but sticky objects can
be inline.
> > > > 
> > > > What do you mean exactly that "LayoutInline::layout() is not supposed to
be reached but sticky objects can be inline" ?
> > > 
> > > My original thinking was that I may need to call layout on sticky objects
to update their saved sticky constraints, but LayoutInline::layout() has an
ASSERT_NOT_REACHED() so I assumed that this would not be correct.
> > > > 
> > > > > 
> > > > > Once we did have the update of these rects happening at the right
time, would I then then add a rare data member to LayoutBoxModelObject to save
these details?
> > > > 
> > > > Yes. It'd be: run the lifecycle code in FrameView up to compositing,
then update all sticky constraints for elements which
> > > > need it based on dirty bits. Store these constraints in their
LayoutObject's rare data.
> > > 
> > > So to be clear, this would be somewhere in
PaintLayerCompositor::updateIfNeeded after the compositing input update?
> > 
> > No, in FrameView::updateLifecyclePhasesInternal, after compositing update.
Just before the call to scrollContentsIfNeededRecursive(), probably.
> > 
> > > 
> > > As for detecting dirtiness, I notice there is LayoutBox::frameRectChanged
which could be the right place to detect the containing block / ancestor
scroller size change and set some flag on sticky descendants at that point.
> > > 
> > > If the ancestor scroller changes I imagine this could be detected during
the compositing inputs update and mark the sticky object dirty then.
> > 
> > Updating layout which may affect the sticky-positioned element's positioning
should require re-computation of it. It's not just that the scroller's rect
changed; there could be other changes of intermediate elements which affect the
sticky element constraint rects.
> > 
> > The simplest way I can think of to be correct is to clear out the sticky
position constraints for a scroller if anything about it or its contents changes
(maybe if any style or layout on any element in the scroller, including itself,
changed?), and then in the part where you update those constraints, look to see
if they were cleared out, and if so recompute.
> > 
> > > 
> > > > 
> > > > This would make position sticky offsets something that is computed after
layout and just before paint, similar to how transforms are updated after layout
and are (mostly) not affecting layout. But this would mean that position:sticky
elements would have an updated in-flow offset that layout wouldn't know about,
so we need to make sure that doesn't change
> > > > anything about layout. I think it should not, in fact I don't know why
these constraints would have to be applied in that
> > > > way. Is there some reason?
> > > 
> > > I don't think there's any problems with this. As long the offset will be
correct if JS queries element.offsetTop after updating some sticky property /
making an element sticky it should be fine. I had been assuming (perhaps
incorrectly) that querying offset from JS only advanced the lifecycle past the
layout stage.
> > 
> > Element::offsetTop currently only advances to layout clean, and only for the
current frame, unfortunately. :( It's really spec'ed to require this?
> > 
> > Options I can think of:
> > 
> > 1. Change the spec?
> > 2. Change Element::offsetTop to do more work. This may make some
microbenchmarks slower.
> > 3. #2, but only if there is a position:sticky element present. This hacks
around the benchmarks.
> > 4. Run the computation of ancestor- and descendant-dependent PaintLayer
properties (i.e. CompositingInputsUpdater) inside of the sticky position update
code, if needed, in its own
> > lifecycle phases with or separate from sticky.
> > 
> > I think I like 4 or 1 the best. 4 shouldn't be super hard, I can walk you
through it as a preparatory CL.
> > 
> 
> I like option 4, and in fact I was wondering if we couldn't compute ancestor
dependent input properties after style, but it looks like the scroll ancestor
chain only includes layers which actually overflow (requiring layout).
> Firefox and Safari consider the sticky ancestor scrolling box as the first one
to not have "overflow: visible" regardless of whether the content overflows.
This makes it knowable after style.
> The spec says it should be the "nearest ancestor with a scrolling box"
(https://www.w3.org/TR/css3-positioning/#sticky-pos) where scrolling box is
defined in CSSOM View as "Elements and viewports have an associated scrolling
box if has a scrolling mechanism or it overflows its content area and the used
value of the 'overflow-x' or 'overflow-y' property is 'hidden'."
(https://www.w3.org/TR/cssom-view/#scrolling-box).
> Unless I'm misreading, this seems to be inconsistent, but necessary given "if
sticky positioning causes an 'overflow: auto' or 'overflow: scroll' box to have
overflow, the user agent must allow the user to access this content (at its
offset position), which, through the creation of a scrolling mechanism, may
affect layout." (https://www.w3.org/TR/css3-positioning/#sticky-pos).
> 
> I'm wondering if we should also seek to clarify the spec on this matter?

Possibly, but there would still be a performance issue walking up the tree
multiple times (in the presence of multiple sticky elements) which would need to
be dealt with.

> 
> > > > 
> > > > Alternatively, we can think of it as a purely paint effect that has
nothing to do with layout, which is spec'ed to
> > > > work out. I hope the spec was set up to make this work?
> 
> I believe this line from the spec requires it to be part of layout: "if sticky
positioning causes an 'overflow: auto' or 'overflow: scroll' box to have
overflow, the user agent must allow the user to access this content (at its
offset position), which, through the creation of a scrolling mechanism, may
affect layout." (https://www.w3.org/TR/css3-positioning/#sticky-pos).

Interesting. Why would stickly positioning cause more overflow?

> 
> > > Is LayoutObject::invalidateTreeIfNeeded perhaps a good hook for when to
dirty sticky constraints? It seems like this happens whenever sizes change or
things move and that this should happen after compositing inputs have been
updated:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > See my comment above. I don't think it would be a good idea to conflate the
dirtiness you would be tracking here with invalidation.

[flackr, 2016-02-10 20:36:07]

On 2016/02/10 at 19:32:54, chrishtr wrote:
> On 2016/02/10 at 17:02:36, flackr wrote:
> > Thanks for your comments, I'm starting to get a much better big picture.
> > 
> > On 2016/02/08 at 21:46:55, chrishtr wrote:
> > >
https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/1308273010/diff/160001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:747:
LayoutRect constrainingRect = computeStickyConstrainingRect();
> > > On 2016/02/03 at 22:48:38, flackr wrote:
> > > > On 2016/01/30 at 01:49:02, chrishtr wrote:
> > > > > On 2016/01/19 at 15:40:48, flackr wrote:
> > > > > > On 2015/12/11 02:06:12, chrishtr wrote:
> > > > > > > This will compute these rects from scratch on every scroll. You
should be able
> > > > > > > to get it set up so that the only thing that happens on each
scroll is to add
> > > > > > > the scroll offset to the sticky bounds and compare with the
already-computed
> > > > > > > containing block and scroller viewport.
> > > > > > 
> > > > > > I've been trying to wrap my head around how we can do this
correctly, updating at the right times.
> > > > > > 
> > > > > > It seems that we would need to make layout updates on the scroll
ancestor / containing block descend to sticky position children. One caveat is
that LayoutInline::layout() is not supposed to be reached but sticky objects can
be inline.
> > > > > 
> > > > > What do you mean exactly that "LayoutInline::layout() is not supposed
to be reached but sticky objects can be inline" ?
> > > > 
> > > > My original thinking was that I may need to call layout on sticky
objects to update their saved sticky constraints, but LayoutInline::layout() has
an ASSERT_NOT_REACHED() so I assumed that this would not be correct.
> > > > > 
> > > > > > 
> > > > > > Once we did have the update of these rects happening at the right
time, would I then then add a rare data member to LayoutBoxModelObject to save
these details?
> > > > > 
> > > > > Yes. It'd be: run the lifecycle code in FrameView up to compositing,
then update all sticky constraints for elements which
> > > > > need it based on dirty bits. Store these constraints in their
LayoutObject's rare data.
> > > > 
> > > > So to be clear, this would be somewhere in
PaintLayerCompositor::updateIfNeeded after the compositing input update?
> > > 
> > > No, in FrameView::updateLifecyclePhasesInternal, after compositing update.
Just before the call to scrollContentsIfNeededRecursive(), probably.
> > > 
> > > > 
> > > > As for detecting dirtiness, I notice there is
LayoutBox::frameRectChanged which could be the right place to detect the
containing block / ancestor scroller size change and set some flag on sticky
descendants at that point.
> > > > 
> > > > If the ancestor scroller changes I imagine this could be detected during
the compositing inputs update and mark the sticky object dirty then.
> > > 
> > > Updating layout which may affect the sticky-positioned element's
positioning should require re-computation of it. It's not just that the
scroller's rect changed; there could be other changes of intermediate elements
which affect the sticky element constraint rects.
> > > 
> > > The simplest way I can think of to be correct is to clear out the sticky
position constraints for a scroller if anything about it or its contents changes
(maybe if any style or layout on any element in the scroller, including itself,
changed?), and then in the part where you update those constraints, look to see
if they were cleared out, and if so recompute.
> > > 
> > > > 
> > > > > 
> > > > > This would make position sticky offsets something that is computed
after layout and just before paint, similar to how transforms are updated after
layout and are (mostly) not affecting layout. But this would mean that
position:sticky elements would have an updated in-flow offset that layout
wouldn't know about, so we need to make sure that doesn't change
> > > > > anything about layout. I think it should not, in fact I don't know why
these constraints would have to be applied in that
> > > > > way. Is there some reason?
> > > > 
> > > > I don't think there's any problems with this. As long the offset will be
correct if JS queries element.offsetTop after updating some sticky property /
making an element sticky it should be fine. I had been assuming (perhaps
incorrectly) that querying offset from JS only advanced the lifecycle past the
layout stage.
> > > 
> > > Element::offsetTop currently only advances to layout clean, and only for
the current frame, unfortunately. :( It's really spec'ed to require this?
> > > 
> > > Options I can think of:
> > > 
> > > 1. Change the spec?
> > > 2. Change Element::offsetTop to do more work. This may make some
microbenchmarks slower.
> > > 3. #2, but only if there is a position:sticky element present. This hacks
around the benchmarks.
> > > 4. Run the computation of ancestor- and descendant-dependent PaintLayer
properties (i.e. CompositingInputsUpdater) inside of the sticky position update
code, if needed, in its own
> > > lifecycle phases with or separate from sticky.
> > > 
> > > I think I like 4 or 1 the best. 4 shouldn't be super hard, I can walk you
through it as a preparatory CL.
> > > 
> > 
> > I like option 4, and in fact I was wondering if we couldn't compute ancestor
dependent input properties after style, but it looks like the scroll ancestor
chain only includes layers which actually overflow (requiring layout).
> > Firefox and Safari consider the sticky ancestor scrolling box as the first
one to not have "overflow: visible" regardless of whether the content overflows.
This makes it knowable after style.
> > The spec says it should be the "nearest ancestor with a scrolling box"
(https://www.w3.org/TR/css3-positioning/#sticky-pos) where scrolling box is
defined in CSSOM View as "Elements and viewports have an associated scrolling
box if has a scrolling mechanism or it overflows its content area and the used
value of the 'overflow-x' or 'overflow-y' property is 'hidden'."
(https://www.w3.org/TR/cssom-view/#scrolling-box).
> > Unless I'm misreading, this seems to be inconsistent, but necessary given
"if sticky positioning causes an 'overflow: auto' or 'overflow: scroll' box to
have overflow, the user agent must allow the user to access this content (at its
offset position), which, through the creation of a scrolling mechanism, may
affect layout." (https://www.w3.org/TR/css3-positioning/#sticky-pos).
> > 
> > I'm wondering if we should also seek to clarify the spec on this matter?
> 
> Possibly, but there would still be a performance issue walking up the tree
multiple times (in the presence of multiple sticky elements) which would need to
be dealt with.

Oh, I was suggesting we clarify in the spec that it should be the ancestor with
used value of overflow not 'visible'. This is consistent with what Safari and
Firefox do and allows it to be evaluated based on style. We could change
ancestorDependentCompositingInputs to include non overflowing scroll nodes in
the scrolling chain, and have a function which skips scroll nodes that do not
actually have overflow content (presuming that this would be the exceptional
case rather than most nodes).
> 
> > 
> > > > > 
> > > > > Alternatively, we can think of it as a purely paint effect that has
nothing to do with layout, which is spec'ed to
> > > > > work out. I hope the spec was set up to make this work?
> > 
> > I believe this line from the spec requires it to be part of layout: "if
sticky positioning causes an 'overflow: auto' or 'overflow: scroll' box to have
overflow, the user agent must allow the user to access this content (at its
offset position), which, through the creation of a scrolling mechanism, may
affect layout." (https://www.w3.org/TR/css3-positioning/#sticky-pos).
> 
> Interesting. Why would stickly positioning cause more overflow?

I think this is the same as the reason relative would cause overflow, it's
offset to the point of being outside the visible scroll content. e.g. here's a
particularly terrible example: https://jsfiddle.net/1ewchLdm/ . On firefox this
results in growing the scroller as you scroll down :(. On safari this is
positioned as low as it can be within the scroller, though if you add a border
it causes overflow. I'm really not sure when this behavior would be desirable
though.

Regardless of this though, the spec reads like this is spec'd to be a layout
effect. In particular, it is "positioned similarly to a relatively positioned
box, but the offset is computed with reference to ...". Since relative position
affects layout (can cause scrolling), and offsetTop / offsetLeft return the
relative offset position, it seems like sticky should follow suit. Offset top
and left are supposed to return the "border edge of the first CSS layout box
associated with the element, relative to the initial containing block origin,
ignoring any transforms that apply to the element and its ancestors."
(https://www.w3.org/TR/cssom-view/#dom-htmlelement-offsettop) so I guess if we
changed the sticky spec such that its layout box was not moved we could do this,
but it feels very inconsistent IMO.
> 
> > 
> > > > Is LayoutObject::invalidateTreeIfNeeded perhaps a good hook for when to
dirty sticky constraints? It seems like this happens whenever sizes change or
things move and that this should happen after compositing inputs have been
updated:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > See my comment above. I don't think it would be a good idea to conflate
the dirtiness you would be tracking here with invalidation.

[chrishtr, 2016-02-10 22:28:55]

Ok yeah I see how adding position:sticky can change overflow. But it is the case
that
scrolling a sticky element cannot cause layout to be dirty, therefore it's a
post-layout effect.
Right?

Is it true that offsetTop requires the position:sticky element to report its
correct position,
including the sticky adjustment? If so that can be done but requires recomputing
the sticky
constraints and painted output position in that case...

[flackr, 2016-02-11 16:26:14]

On 2016/02/10 at 22:28:55, chrishtr wrote:
> Ok yeah I see how adding position:sticky can change overflow. But it is the
case that
> scrolling a sticky element cannot cause layout to be dirty, therefore it's a
post-layout effect.
> Right?

Right, there is no way I can imagine for it to dirty layout during the scroll.
> 
> Is it true that offsetTop requires the position:sticky element to report its
correct position,
> including the sticky adjustment? If so that can be done but requires
recomputing the sticky
> constraints and painted output position in that case...

Yes, this seems to be required by the spec and is reported correctly on Safari
and Firefox's implementations. So we'll trigger this "paint" step somehow even
if the element is far outside the viewport?

[flackr, 2016-02-11 16:33:28]

On 2016/02/11 at 16:26:14, flackr wrote:
> On 2016/02/10 at 22:28:55, chrishtr wrote:
> > Ok yeah I see how adding position:sticky can change overflow. But it is the
case that
> > scrolling a sticky element cannot cause layout to be dirty, therefore it's a
post-layout effect.
> > Right?
> 
> Right, there is no way I can imagine for it to dirty layout during the scroll.

Sorry, I abbreviated this point a bit, Firefox does update layout as a result of
scrolling (making the scroller get longer in that fiddle I sent you:
https://jsfiddle.net/1ewchLdm/ . Safari does not seemingly by not letting the
sticky element project below its containing scroller's current bounds. I think
you can interpret the spec such that Safari's behavior is correct (i.e. not
projecting outside of its containing block being the scroller's current scroll
content size).
> > 
> > Is it true that offsetTop requires the position:sticky element to report its
correct position,
> > including the sticky adjustment? If so that can be done but requires
recomputing the sticky
> > constraints and painted output position in that case...
> 
> Yes, this seems to be required by the spec and is reported correctly on Safari
and Firefox's implementations. So we'll trigger this "paint" step somehow even
if the element is far outside the viewport?

[chrishtr, 2016-02-11 18:36:03]

On 2016/02/11 at 16:33:28, flackr wrote:
> On 2016/02/11 at 16:26:14, flackr wrote:
> > On 2016/02/10 at 22:28:55, chrishtr wrote:
> > > Ok yeah I see how adding position:sticky can change overflow. But it is
the case that
> > > scrolling a sticky element cannot cause layout to be dirty, therefore it's
a post-layout effect.
> > > Right?
> > 
> > Right, there is no way I can imagine for it to dirty layout during the
scroll.
> 
> Sorry, I abbreviated this point a bit, Firefox does update layout as a result
of scrolling (making the scroller get longer in that fiddle I sent you:
https://jsfiddle.net/1ewchLdm/ . Safari does not seemingly by not letting the
sticky element project below its containing scroller's current bounds. I think
you can interpret the spec such that Safari's behavior is correct (i.e. not
projecting outside of its containing block being the scroller's current scroll
content size).

I think we should not let scroll affect layout in this way, either by adopting
an interpretation of the spec or changing the spec.

> > > 
> > > Is it true that offsetTop requires the position:sticky element to report
its correct position,
> > > including the sticky adjustment? If so that can be done but requires
recomputing the sticky
> > > constraints and painted output position in that case...
> > 
> > Yes, this seems to be required by the spec and is reported correctly on
Safari and Firefox's implementations. So we'll trigger this "paint" step somehow
even if the element is far outside the viewport?

Do you mean we'll trigger it even if the scroller and the sticky element in it
are both far off the viewport?

[flackr, 2016-02-11 20:44:55]

On 2016/02/11 at 18:36:03, chrishtr wrote:
> On 2016/02/11 at 16:33:28, flackr wrote:
> > On 2016/02/11 at 16:26:14, flackr wrote:
> > > On 2016/02/10 at 22:28:55, chrishtr wrote:
> > > > Ok yeah I see how adding position:sticky can change overflow. But it is
the case that
> > > > scrolling a sticky element cannot cause layout to be dirty, therefore
it's a post-layout effect.
> > > > Right?
> > > 
> > > Right, there is no way I can imagine for it to dirty layout during the
scroll.
> > 
> > Sorry, I abbreviated this point a bit, Firefox does update layout as a
result of scrolling (making the scroller get longer in that fiddle I sent you:
https://jsfiddle.net/1ewchLdm/ . Safari does not seemingly by not letting the
sticky element project below its containing scroller's current bounds. I think
you can interpret the spec such that Safari's behavior is correct (i.e. not
projecting outside of its containing block being the scroller's current scroll
content size).
> 
> I think we should not let scroll affect layout in this way, either by adopting
an interpretation of the spec or changing the spec.

Agreed.
> 
> > > > 
> > > > Is it true that offsetTop requires the position:sticky element to report
its correct position,
> > > > including the sticky adjustment? If so that can be done but requires
recomputing the sticky
> > > > constraints and painted output position in that case...
> > > 
> > > Yes, this seems to be required by the spec and is reported correctly on
Safari and Firefox's implementations. So we'll trigger this "paint" step somehow
even if the element is far outside the viewport?
> 
> Do you mean we'll trigger it even if the scroller and the sticky element in it
are both far off the viewport?

Yes. The page could be querying the offsetTop / offsetLeft from script - not
sure of a practical example for this but it should work.

[flackr, 2016-02-22 22:55:24]

Hi Chris, this still has some crashes to be worked out and some cleaning up of
the cache to do, but if you have time can you check if the general idea of this
seems reasonable? It's caching sticky constraints during the compositing inputs
update, I should probably enforce that they are only generated at that time, and
then using those when asked for the stickyPositionOffset.

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

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:676: // FIXME:
sucks to call localToAbsolute again, but we can't just offset from the
previously computed rect if there are transforms.
On 2016/01/30 at 01:49:02, chrishtr wrote:
> s/localToAbsolute/localToAncestorQuad

Done.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:678: FloatRect
absContainerFrame = containingBlock->localToAncestorQuad(FloatRect(FloatPoint(),
FloatSize(containingBlock->size())), scrollAncestor).boundingBox();
On 2016/01/30 at 01:49:02, chrishtr wrote:
> s/absContainerFrame/stickyBoxRect/

This is the container's frame rect which we add to the sticky box's location
within it in order to get the sticky box's absolute location within the
scroller. I was just playing with Firefox and Safari to see how they handle
transforms and it looks like Safari accounts for the transformed position of the
container while in firefox sticky seems to apply before transforms and then
things are transformed afterwards.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:687: // We can't
call localToAbsolute on |this| because that will recur. FIXME: For now, assume
that |this| is not transformed.
On 2016/01/30 at 01:49:02, chrishtr wrote:
> What is this comment about again? Why a need to map coordinates?
absContainerFrame is already in scroller space.

This actually seems unnecessary. If there is a transform on the sticky element
it is expected to be offset from its stuck position so we don't need to account
for that.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:694: if
(!style()->left().isAuto() && !style()->right().isAuto()) {
On 2016/01/30 at 01:49:02, chrishtr wrote:
> Document this code and what it's for.

Done.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:713: //
FIXME(ostap): Exclude top or bottom edge offset depending on the writing mode
when related
On 2016/01/30 at 01:49:02, chrishtr wrote:
> s/FIXME/TODO/ throughout, and replace names with flakr

Done.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h
(right):

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h:12:
class StickyPositionViewportConstraints final : public ViewportConstraints {
On 2016/01/30 at 01:49:03, chrishtr wrote:
> On further reflection, StickyPositionScrollingConstraints is more accurate.

Done.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h:43:
void setAbsoluteContainingBlockRect(const FloatRect& rect) {
m_absoluteContainingBlockRect = rect; }
On 2016/01/30 at 01:49:03, chrishtr wrote:
> setScrollContainerRelativeContainingBlockRect

Done.

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/scrolling/StickyPositionViewportConstraints.h:45:
void setAbsoluteStickyBoxRect(const FloatRect& rect) { m_absoluteStickyBoxRect =
rect; }
On 2016/01/30 at 01:49:03, chrishtr wrote:
> setScrollContainerRelativeStickyBoxRect

Done.

[chrishtr, 2016-02-23 16:31:51]

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

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:648: while
(containingBlock->isAnonymous()) {
On 2016/02/03 at 22:48:38, flackr wrote:
> On 2016/01/30 at 01:49:02, chrishtr wrote:
> > Why exactly? Is there a test for this?
> 
> I was matching firefox and safari on
http://flackr.github.io/compositor-worker/sticky/sticky.html. In particular, the
top sticky element is contained by an anonymous block above the spacer div,
which on other browsers does not constrain it. That being said, I don't see any
specific language in the spec about this. I will add a test for it.

And file a spec bug?

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

https://codereview.chromium.org/1308273010/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:682: FloatRect
scrollContainerRelativeContainerFrame =
containingBlock->localToAncestorQuad(FloatRect(FloatPoint(),
FloatSize(containingBlock->size())), scrollAncestor).boundingBox();
For these localToAncestor* methods, you should consider the MapCoordinatesFlags
parameter and set it accordingly.
e.g. include transforms? flip for container block?

[flackr, 2016-02-25 22:10:26]

https://codereview.chromium.org/1308273010/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):

https://codereview.chromium.org/1308273010/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:118:
properties.clippedAbsoluteBoundingBox =
enclosingIntRect(m_geometryMap.absoluteRect(FloatRect(layer->boundingBoxForCompositingOverlapTest())));
There seems to be a bit of a problem here. Seems we need the bounding box for
the AncestorDependentCompositingInputs here, but in
LayoutBoxModelObject::offsetForStickyPosition it will use the old
ancestorOverflowLayer. I could separately set the ancestorOverflowLayer and
sticky constraints before this.

[chrishtr, 2016-03-04 20:05:41]

https://codereview.chromium.org/1308273010/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):

https://codereview.chromium.org/1308273010/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:118:
properties.clippedAbsoluteBoundingBox =
enclosingIntRect(m_geometryMap.absoluteRect(FloatRect(layer->boundingBoxForCompositingOverlapTest())));
On 2016/02/25 at 22:10:26, flackr wrote:
> There seems to be a bit of a problem here. Seems we need the bounding box for
the AncestorDependentCompositingInputs here, but in
LayoutBoxModelObject::offsetForStickyPosition it will use the old
ancestorOverflowLayer. I could separately set the ancestorOverflowLayer and
sticky constraints before this.

Are you blocked on this? Or did your proposed approach work?

[flackr, 2016-03-07 19:07:33]

Sorry this took so long - adding an ancestor pointer with sticky information
which is cleared correctly and having correct lifetime semantics was tricky. I
had to add the ancestor overflow pointer to PaintLayer as it was needed before
updating the layout geometry map and computing the position for ancestor
dependent compositing inputs.

Can you have a look? Thanks.

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

https://codereview.chromium.org/1308273010/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:648: while
(containingBlock->isAnonymous()) {
On 2016/02/23 at 16:31:51, chrishtr wrote:
> On 2016/02/03 at 22:48:38, flackr wrote:
> > On 2016/01/30 at 01:49:02, chrishtr wrote:
> > > Why exactly? Is there a test for this?
> > 
> > I was matching firefox and safari on
http://flackr.github.io/compositor-worker/sticky/sticky.html. In particular, the
top sticky element is contained by an anonymous block above the spacer div,
which on other browsers does not constrain it. That being said, I don't see any
specific language in the spec about this. I will add a test for it.
> 
> And file a spec bug?

Done: bug is here https://www.w3.org/Bugs/Public/show_bug.cgi?id=29510
and test is
third_party/WebKit/LayoutTests/fast/css/sticky/inline-sticky-anonymous-container.html

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

https://codereview.chromium.org/1308273010/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:682: FloatRect
scrollContainerRelativeContainerFrame =
containingBlock->localToAncestorQuad(FloatRect(FloatPoint(),
FloatSize(containingBlock->size())), scrollAncestor).boundingBox();
On 2016/02/23 at 16:31:51, chrishtr wrote:
> For these localToAncestor* methods, you should consider the
MapCoordinatesFlags parameter and set it accordingly.
> e.g. include transforms? flip for container block?

I think we actually want neither, but I will think carefully about this. If the
transform affected sticky constraints then we would have to invalidate when
updating a transform, which should be a lightweight property. The flip I believe
only needs to be accounted for in terms of which edge to stick to.

[chrishtr, 2016-03-09 03:27:00]

I'll take a look in the morning, was at an offsite today.

[chrishtr, 2016-03-09 16:53:08]

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
Is this where you handle the case of an element with the same scroller as a
position-sticky element that may affect
the sticky constraints for that scroller?

Does the CL have a test for this scenario?

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
Why is the layer position not already up-to-date at this point?

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h
(right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h:34:
explicit AncestorInfo(PaintLayer* rootLayer)
You should be able to initialize lastOverflowLayer inside of updateRecursive,
similarly to how it works for ancestorStackingContext

[flackr, 2016-03-14 19:04:26]

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
On 2016/03/09 at 16:53:08, chrishtr wrote:
> Is this where you handle the case of an element with the same scroller as a
position-sticky element that may affect
> the sticky constraints for that scroller?

Yes.
> 
> Does the CL have a test for this scenario?

It does now :-) - sticky-container-moved.html

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
On 2016/03/09 at 16:53:08, chrishtr wrote:
> Why is the layer position not already up-to-date at this point?

The overflow layer may have just changed - and the sticky position is dependent
on this.

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h
(right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h:34:
explicit AncestorInfo(PaintLayer* rootLayer)
On 2016/03/09 at 16:53:08, chrishtr wrote:
> You should be able to initialize lastOverflowLayer inside of updateRecursive,
similarly to how it works for ancestorStackingContext

Done.

[chrishtr, 2016-03-16 21:52:39]

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
On 2016/03/14 at 19:04:26, flackr wrote:
> On 2016/03/09 at 16:53:08, chrishtr wrote:
> > Is this where you handle the case of an element with the same scroller as a
position-sticky element that may affect
> > the sticky constraints for that scroller?
> 
> Yes.

Hmm. I'm trying to think of whether it is possible to not change the frame rect
but still screw things up. What if a transform is
changed on an element between the scroller and the sticky element?

How about instead always invalidating the sticky constraints for any containing
scroller whenever style or layout of any element changes? That should be
complete.

> > 
> > Does the CL have a test for this scenario?
> 
> It does now :-) - sticky-container-moved.html

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
On 2016/03/14 at 19:04:26, flackr wrote:
> On 2016/03/09 at 16:53:08, chrishtr wrote:
> > Why is the layer position not already up-to-date at this point?
> 
> The overflow layer may have just changed - and the sticky position is
dependent on this.

But layer positions should be set before the compositing step of the lifecycle,
no?

[flackr, 2016-03-22 21:44:32]

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
On 2016/03/16 at 21:52:39, chrishtr wrote:
> On 2016/03/14 at 19:04:26, flackr wrote:
> > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > Is this where you handle the case of an element with the same scroller as
a position-sticky element that may affect
> > > the sticky constraints for that scroller?
> > 
> > Yes.
> 
> Hmm. I'm trying to think of whether it is possible to not change the frame
rect but still screw things up. What if a transform is
> changed on an element between the scroller and the sticky element?

I'm fairly certain that transforms shouldn't be included in the sticky
constraints. Trying to include transforms would require also supporting
animations of transforms.

> 
> How about instead always invalidating the sticky constraints for any
containing scroller whenever style or layout of any element changes? That should
be complete.
> 
> > > 
> > > Does the CL have a test for this scenario?
> > 
> > It does now :-) - sticky-container-moved.html

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):

https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
On 2016/03/16 at 21:52:39, chrishtr wrote:
> On 2016/03/14 at 19:04:26, flackr wrote:
> > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > Why is the layer position not already up-to-date at this point?
> > 
> > The overflow layer may have just changed - and the sticky position is
dependent on this.
> 
> But layer positions should be set before the compositing step of the
lifecycle, no?

Yes, but without correct ancestor information. We can't accurately set the layer
position for sticky elements until we have the correct ancestor overflow
scroller. Perhaps we should skip sticky offsets during
updateLayerPositionRecursive?

Alternately, it seems like compositing inputs and layer position updates would
have to become part of the same walk.

[chrishtr, 2016-03-23 00:36:24]

On 2016/03/22 at 21:44:32, flackr wrote:
>
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> 
>
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
> On 2016/03/16 at 21:52:39, chrishtr wrote:
> > On 2016/03/14 at 19:04:26, flackr wrote:
> > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > Is this where you handle the case of an element with the same scroller
as a position-sticky element that may affect
> > > > the sticky constraints for that scroller?
> > > 
> > > Yes.
> > 
> > Hmm. I'm trying to think of whether it is possible to not change the frame
rect but still screw things up. What if a transform is
> > changed on an element between the scroller and the sticky element?
> 
> I'm fairly certain that transforms shouldn't be included in the sticky
constraints. Trying to include transforms would require also supporting
animations of transforms.

But what about my larger concern regarding frame rects? Relying on style/layout
dirty bits rather than artifacts of their computation (e.g. the frame rect)
sounds safe and complete, and in line with other pipeline invalidation machinery
in Blink. What do you think?
Is my proposal hard to implement? I don't think it should be.. 

> 
> > 
> > How about instead always invalidating the sticky constraints for any
containing scroller whenever style or layout of any element changes? That should
be complete.
> > 
> > > > 
> > > > Does the CL have a test for this scenario?
> > > 
> > > It does now :-) - sticky-container-moved.html
> 
>
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):
> 
>
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
> On 2016/03/16 at 21:52:39, chrishtr wrote:
> > On 2016/03/14 at 19:04:26, flackr wrote:
> > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > Why is the layer position not already up-to-date at this point?
> > > 
> > > The overflow layer may have just changed - and the sticky position is
dependent on this.
> > 
> > But layer positions should be set before the compositing step of the
lifecycle, no?
> 
> Yes, but without correct ancestor information. We can't accurately set the
layer position for sticky elements until we have the correct ancestor overflow
scroller. Perhaps we should skip sticky offsets during
updateLayerPositionRecursive?

I see. So sticky elements need a two-phase position update after layout, given
the way the current code is structured.

I'd be ok with rephrasing your comment to say that sticky constraints and
ancestor overflow scroller affect sticky position layer position, so we need to
update it again here. And add a TODO that this should be refactored in the
future to be cleaner.

> Alternately, it seems like compositing inputs and layer position updates would
have to become part of the same walk.

The latter is a good idea, but a larger project that I don't want to inflict on
you. :) I will follow up on this and see if should
be staffed on the paint team though, thanks for the good suggestion.

[flackr, 2016-03-23 20:28:45]

On 2016/03/23 at 00:36:24, chrishtr wrote:
> On 2016/03/22 at 21:44:32, flackr wrote:
> >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > 
> >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
> > On 2016/03/16 at 21:52:39, chrishtr wrote:
> > > On 2016/03/14 at 19:04:26, flackr wrote:
> > > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > > Is this where you handle the case of an element with the same scroller
as a position-sticky element that may affect
> > > > > the sticky constraints for that scroller?
> > > > 
> > > > Yes.
> > > 
> > > Hmm. I'm trying to think of whether it is possible to not change the frame
rect but still screw things up. What if a transform is
> > > changed on an element between the scroller and the sticky element?
> > 
> > I'm fairly certain that transforms shouldn't be included in the sticky
constraints. Trying to include transforms would require also supporting
animations of transforms.
> 
> But what about my larger concern regarding frame rects? Relying on
style/layout dirty bits rather than artifacts of their computation (e.g. the
frame rect) sounds safe and complete, and in line with other pipeline
invalidation machinery in Blink. What do you think?
> Is my proposal hard to implement? I don't think it should be.. 

I had thought that the frame rect changing described the exact circumstances I
was interested in (and not unnecessarily invalidating sticky constraints), but
as I thought about it, the containing block writing direction and margins can
affect the sticky constraints so I agree it's simpler (and likely to be more
correct) to just invalidate anytime style or layout changes for now.

Turns out this actually greatly simplifies things. I think I don't have to
lookup ancestor scroller, I can just invalidate the sticky constraints whenever
style or layout walks past an overflow scroller. What do you think? This may
unnecessarily invalidate sticky constraints on a parent scroller when the change
is in a nested scroller - but I imagine this case isn't very common and it saves
having to check the overflow scroller on every updated element.

> 
> > 
> > > 
> > > How about instead always invalidating the sticky constraints for any
containing scroller whenever style or layout of any element changes? That should
be complete.
> > > 
> > > > > 
> > > > > Does the CL have a test for this scenario?
> > > > 
> > > > It does now :-) - sticky-container-moved.html
> > 
> >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):
> > 
> >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
> > On 2016/03/16 at 21:52:39, chrishtr wrote:
> > > On 2016/03/14 at 19:04:26, flackr wrote:
> > > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > > Why is the layer position not already up-to-date at this point?
> > > > 
> > > > The overflow layer may have just changed - and the sticky position is
dependent on this.
> > > 
> > > But layer positions should be set before the compositing step of the
lifecycle, no?
> > 
> > Yes, but without correct ancestor information. We can't accurately set the
layer position for sticky elements until we have the correct ancestor overflow
scroller. Perhaps we should skip sticky offsets during
updateLayerPositionRecursive?
> 
> I see. So sticky elements need a two-phase position update after layout, given
the way the current code is structured.
> 
> I'd be ok with rephrasing your comment to say that sticky constraints and
ancestor overflow scroller affect sticky position layer position, so we need to
update it again here. And add a TODO that this should be refactored in the
future to be cleaner.
> 
> > Alternately, it seems like compositing inputs and layer position updates
would have to become part of the same walk.
> 
> The latter is a good idea, but a larger project that I don't want to inflict
on you. :) I will follow up on this and see if should
> be staffed on the paint team though, thanks for the good suggestion.

[chrishtr, 2016-03-23 20:39:12]

On 2016/03/23 at 20:28:45, flackr wrote:
> On 2016/03/23 at 00:36:24, chrishtr wrote:
> > On 2016/03/22 at 21:44:32, flackr wrote:
> > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > > 
> > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
> > > On 2016/03/16 at 21:52:39, chrishtr wrote:
> > > > On 2016/03/14 at 19:04:26, flackr wrote:
> > > > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > > > Is this where you handle the case of an element with the same
scroller as a position-sticky element that may affect
> > > > > > the sticky constraints for that scroller?
> > > > > 
> > > > > Yes.
> > > > 
> > > > Hmm. I'm trying to think of whether it is possible to not change the
frame rect but still screw things up. What if a transform is
> > > > changed on an element between the scroller and the sticky element?
> > > 
> > > I'm fairly certain that transforms shouldn't be included in the sticky
constraints. Trying to include transforms would require also supporting
animations of transforms.
> > 
> > But what about my larger concern regarding frame rects? Relying on
style/layout dirty bits rather than artifacts of their computation (e.g. the
frame rect) sounds safe and complete, and in line with other pipeline
invalidation machinery in Blink. What do you think?
> > Is my proposal hard to implement? I don't think it should be.. 
> 
> I had thought that the frame rect changing described the exact circumstances I
was interested in (and not unnecessarily invalidating sticky constraints), but
as I thought about it, the containing block writing direction and margins can
affect the sticky constraints so I agree it's simpler (and likely to be more
correct) to just invalidate anytime style or layout changes for now.
> 
> Turns out this actually greatly simplifies things. I think I don't have to
lookup ancestor scroller, I can just invalidate the sticky constraints whenever
style or layout walks past an overflow scroller. What do you think? This may
unnecessarily invalidate sticky constraints on a parent scroller when the change
is in a nested scroller - but I imagine this case isn't very common and it saves
having to check the overflow scroller on every updated element.

I don't think you can't depend on style or layout always starting at the root.
It is cleaner to just write

if (layer = ancestorOverflowLayer())
  layer->invalidateStickyConstraints()

in style and layout, and be done. This will be plenty performant also given
you've computed it in compositing inputs from the previous frame.
 
> 
> > 
> > > 
> > > > 
> > > > How about instead always invalidating the sticky constraints for any
containing scroller whenever style or layout of any element changes? That should
be complete.
> > > > 
> > > > > > 
> > > > > > Does the CL have a test for this scenario?
> > > > > 
> > > > > It does now :-) - sticky-container-moved.html
> > > 
> > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > > File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > >
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
> > > On 2016/03/16 at 21:52:39, chrishtr wrote:
> > > > On 2016/03/14 at 19:04:26, flackr wrote:
> > > > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > > > Why is the layer position not already up-to-date at this point?
> > > > > 
> > > > > The overflow layer may have just changed - and the sticky position is
dependent on this.
> > > > 
> > > > But layer positions should be set before the compositing step of the
lifecycle, no?
> > > 
> > > Yes, but without correct ancestor information. We can't accurately set the
layer position for sticky elements until we have the correct ancestor overflow
scroller. Perhaps we should skip sticky offsets during
updateLayerPositionRecursive?
> > 
> > I see. So sticky elements need a two-phase position update after layout,
given the way the current code is structured.
> > 
> > I'd be ok with rephrasing your comment to say that sticky constraints and
ancestor overflow scroller affect sticky position layer position, so we need to
update it again here. And add a TODO that this should be refactored in the
future to be cleaner.
> > 
> > > Alternately, it seems like compositing inputs and layer position updates
would have to become part of the same walk.
> > 
> > The latter is a good idea, but a larger project that I don't want to inflict
on you. :) I will follow up on this and see if should
> > be staffed on the paint team though, thanks for the good suggestion.

[flackr, 2016-03-23 22:30:34]

On 2016/03/23 at 20:39:12, chrishtr wrote:
> On 2016/03/23 at 20:28:45, flackr wrote:
> > On 2016/03/23 at 00:36:24, chrishtr wrote:
> > > On 2016/03/22 at 21:44:32, flackr wrote:
> > > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > > > 
> > > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/layout/LayoutBox.h:1016:
invalidateScrollAncestorConstraints();
> > > > On 2016/03/16 at 21:52:39, chrishtr wrote:
> > > > > On 2016/03/14 at 19:04:26, flackr wrote:
> > > > > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > > > > Is this where you handle the case of an element with the same
scroller as a position-sticky element that may affect
> > > > > > > the sticky constraints for that scroller?
> > > > > > 
> > > > > > Yes.
> > > > > 
> > > > > Hmm. I'm trying to think of whether it is possible to not change the
frame rect but still screw things up. What if a transform is
> > > > > changed on an element between the scroller and the sticky element?
> > > > 
> > > > I'm fairly certain that transforms shouldn't be included in the sticky
constraints. Trying to include transforms would require also supporting
animations of transforms.
> > > 
> > > But what about my larger concern regarding frame rects? Relying on
style/layout dirty bits rather than artifacts of their computation (e.g. the
frame rect) sounds safe and complete, and in line with other pipeline
invalidation machinery in Blink. What do you think?
> > > Is my proposal hard to implement? I don't think it should be.. 
> > 
> > I had thought that the frame rect changing described the exact circumstances
I was interested in (and not unnecessarily invalidating sticky constraints), but
as I thought about it, the containing block writing direction and margins can
affect the sticky constraints so I agree it's simpler (and likely to be more
correct) to just invalidate anytime style or layout changes for now.
> > 
> > Turns out this actually greatly simplifies things. I think I don't have to
lookup ancestor scroller, I can just invalidate the sticky constraints whenever
style or layout walks past an overflow scroller. What do you think? This may
unnecessarily invalidate sticky constraints on a parent scroller when the change
is in a nested scroller - but I imagine this case isn't very common and it saves
having to check the overflow scroller on every updated element.
> 
> I don't think you can't depend on style or layout always starting at the root.
It is cleaner to just write
> 
> if (layer = ancestorOverflowLayer())
>   layer->invalidateStickyConstraints()
> 
> in style and layout, and be done. This will be plenty performant also given
you've computed it in compositing inputs from the previous frame.

Done. Renamed updateScrollInfoAfter layout to updateAfterLayout and put that
there.

>  
> > 
> > > 
> > > > 
> > > > > 
> > > > > How about instead always invalidating the sticky constraints for any
containing scroller whenever style or layout of any element changes? That should
be complete.
> > > > > 
> > > > > > > 
> > > > > > > Does the CL have a test for this scenario?
> > > > > > 
> > > > > > It does now :-) - sticky-container-moved.html
> > > > 
> > > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > > > File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):
> > > > 
> > > >
https://codereview.chromium.org/1308273010/diff/280001/third_party/WebKit/Sou...
> > > >
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:117:
// Layer position needs to be updated before mapping clippedAbsoluteBoundingBox.
> > > > On 2016/03/16 at 21:52:39, chrishtr wrote:
> > > > > On 2016/03/14 at 19:04:26, flackr wrote:
> > > > > > On 2016/03/09 at 16:53:08, chrishtr wrote:
> > > > > > > Why is the layer position not already up-to-date at this point?
> > > > > > 
> > > > > > The overflow layer may have just changed - and the sticky position
is dependent on this.
> > > > > 
> > > > > But layer positions should be set before the compositing step of the
lifecycle, no?
> > > > 
> > > > Yes, but without correct ancestor information. We can't accurately set
the layer position for sticky elements until we have the correct ancestor
overflow scroller. Perhaps we should skip sticky offsets during
updateLayerPositionRecursive?
> > > 
> > > I see. So sticky elements need a two-phase position update after layout,
given the way the current code is structured.
> > > 
> > > I'd be ok with rephrasing your comment to say that sticky constraints and
ancestor overflow scroller affect sticky position layer position, so we need to
update it again here. And add a TODO that this should be refactored in the
future to be cleaner.
Done.
> > > 
> > > > Alternately, it seems like compositing inputs and layer position updates
would have to become part of the same walk.
> > > 
> > > The latter is a good idea, but a larger project that I don't want to
inflict on you. :) I will follow up on this and see if should
> > > be staffed on the paint team though, thanks for the good suggestion.

[chrishtr, 2016-03-24 17:36:03]

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:328: // Update our
scroll information if we're overflow:auto/scroll/hidden now that we know if
Move the comment into updateAfterLayout

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

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:290: if
(newStyleIsSticky != oldStyleIsSticky) {
Do you need to invalidate sticky constraints here?

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:645: void
LayoutBoxModelObject::updateStickyPositionConstraints() const
It would be good to directly unittest this a bit. Can you add a TODO?

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h
(right):

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h:46:
PaintLayer* lastOverflowLayer;
lastOverflowClipLayer

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:1265: if
(oldChild->ancestorOverflowLayer())
Do you have a layout test involving removal of paint layers, and testing whether
it correctly removes the pointer and avoids a stale pointer in the hash map?

[flackr, 2016-03-29 16:02:32]

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:328: // Update our
scroll information if we're overflow:auto/scroll/hidden now that we know if
On 2016/03/24 at 17:36:02, chrishtr wrote:
> Move the comment into updateAfterLayout

Done.

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

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:290: if
(newStyleIsSticky != oldStyleIsSticky) {
On 2016/03/24 at 17:36:02, chrishtr wrote:
> Do you need to invalidate sticky constraints here?

Yes, but we're currently invalidating sticky constraints on every style change
so it would be redundant to invalidate here as well. Think I should continue to
invalidate here?

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:645: void
LayoutBoxModelObject::updateStickyPositionConstraints() const
On 2016/03/24 at 17:36:02, chrishtr wrote:
> It would be good to directly unittest this a bit. Can you add a TODO?

Done.

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h
(right):

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h:46:
PaintLayer* lastOverflowLayer;
On 2016/03/24 at 17:36:03, chrishtr wrote:
> lastOverflowClipLayer

Done.

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:1265: if
(oldChild->ancestorOverflowLayer())
On 2016/03/24 at 17:36:03, chrishtr wrote:
> Do you have a layout test involving removal of paint layers, and testing
whether it correctly removes the pointer and avoids a stale pointer in the hash
map?

sticky-overflow-changed.html does test layer removal (I've been using it to
ensure this case was handled correctly) but I could add a more explicit test.

[chrishtr, 2016-03-29 16:28:10]

On 2016/03/29 at 16:02:32, flackr wrote:
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:328: // Update our
scroll information if we're overflow:auto/scroll/hidden now that we know if
> On 2016/03/24 at 17:36:02, chrishtr wrote:
> > Move the comment into updateAfterLayout
> 
> Done.
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:290: if
(newStyleIsSticky != oldStyleIsSticky) {
> On 2016/03/24 at 17:36:02, chrishtr wrote:
> > Do you need to invalidate sticky constraints here?
> 
> Yes, but we're currently invalidating sticky constraints on every style change
so it would be redundant to invalidate here as well. Think I should continue to
invalidate here?
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:645: void
LayoutBoxModelObject::updateStickyPositionConstraints() const
> On 2016/03/24 at 17:36:02, chrishtr wrote:
> > It would be good to directly unittest this a bit. Can you add a TODO?
> 
> Done.
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h
(right):
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.h:46:
PaintLayer* lastOverflowLayer;
> On 2016/03/24 at 17:36:03, chrishtr wrote:
> > lastOverflowClipLayer
> 
> Done.
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayer.cpp:1265: if
(oldChild->ancestorOverflowLayer())
> On 2016/03/24 at 17:36:03, chrishtr wrote:
> > Do you have a layout test involving removal of paint layers, and testing
whether it correctly removes the pointer and avoids a stale pointer in the hash
map?
> 
> sticky-overflow-changed.html does test layer removal (I've been using it to
ensure this case was handled correctly) but I could add a more explicit test.

It's ok then.

[chrishtr, 2016-03-29 16:28:19]

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

https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:290: if
(newStyleIsSticky != oldStyleIsSticky) {
On 2016/03/29 at 16:02:32, flackr wrote:
> On 2016/03/24 at 17:36:02, chrishtr wrote:
> > Do you need to invalidate sticky constraints here?
> 
> Yes, but we're currently invalidating sticky constraints on every style change

Does the code do that? Which line of code implements this? Not sure I see it
yet.

[flackr, 2016-03-29 17:39:10]

On 2016/03/29 at 16:28:19, chrishtr wrote:
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/1308273010/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:290: if
(newStyleIsSticky != oldStyleIsSticky) {
> On 2016/03/29 at 16:02:32, flackr wrote:
> > On 2016/03/24 at 17:36:02, chrishtr wrote:
> > > Do you need to invalidate sticky constraints here?
> > 
> > Yes, but we're currently invalidating sticky constraints on every style
change
> 
> Does the code do that? Which line of code implements this? Not sure I see it
yet.

My apologies, I must have lost that addition at some point. The ancestor
overflow clip layer is updated now from LayoutBoxModelObject::updateFromStyle.
Similarly, LayoutBlock::updateFromStyle handles invalidating sticky constraints
for the overflow clip layer itself.

[flackr, 2016-03-29 18:31:56]

Updated as per our discussion. Removed constraints when no longer sticky and
added a test to verify that style updates on the sticky element are correctly
updated by the resulting layout.

[chrishtr, 2016-03-29 18:47:31]

lgtm

[Ian Vollick, 2016-03-29 19:02:11]

On 2016/03/29 18:47:31, chrishtr wrote:
> lgtm

platform/ lgtm.

[flackr, 2016-03-29 19:06:27]

flackr@chromium.org changed reviewers:
+ isherman@chromium.org, tdresser@chromium.org, vollick@chromium.org

[flackr, 2016-03-29 19:06:29]

+tdresser for cc/input/main_thread_scrolling_reason.h
+isherman for tools/metrics/histograms/histograms.xml

Please take a look, thanks!

[tdresser, 2016-03-29 19:10:03]

cc/input/main_thread_scrolling_reason.h LGTM

[flackr, 2016-03-29 21:32:58]

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

[commit-bot: I haz the power, 2016-03-29 21:33:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308273010/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308273010/420001

[commit-bot: I haz the power, 2016-03-29 21:44:28]

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

[commit-bot: I haz the power, 2016-03-29 21:44:31]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[flackr, 2016-03-29 21:54:25]

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

[commit-bot: I haz the power, 2016-03-29 21:54:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308273010/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308273010/440001

[commit-bot: I haz the power, 2016-03-29 23:51:53]

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

[commit-bot: I haz the power, 2016-03-29 23:51:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ilya Sherman, 2016-03-30 01:27:44]

histograms.xml lgtm

[flackr, 2016-03-30 20:49:39]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-03-30 20:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, vollick@chromium.org, tdresser@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1308273010/#ps460001
(title: "Make sticky vertical ref tests expectations not dependent on font
size.")

[commit-bot: I haz the power, 2016-03-30 20:50:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308273010/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308273010/460001

[commit-bot: I haz the power, 2016-03-30 22:42:13]

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

[commit-bot: I haz the power, 2016-03-30 22:42:15]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[flackr, 2016-03-31 09:21:04]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-03-31 09:21:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308273010/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308273010/460001

[commit-bot: I haz the power, 2016-03-31 09:29:10]

Committed patchset #22 (id:460001)

[commit-bot: I haz the power, 2016-03-31 09:31:24]

Description was changed from

==========
Adapt and reland old position sticky implementation from
https://codereview.chromium.org/346603007

This adds support for position sticky, and plumbs the necessary information to
the compositor by which it can maintain the effect during accelerated scrolls.

TEST=fast/css/sticky/*
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Adapt and reland old position sticky implementation from
https://codereview.chromium.org/346603007

This adds support for position sticky, and plumbs the necessary information to
the compositor by which it can maintain the effect during accelerated scrolls.

TEST=fast/css/sticky/*
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/846f623cb1ef78d90e620c2a7a6557544a86b692
Cr-Commit-Position: refs/heads/master@{#384231}
==========

[commit-bot: I haz the power, 2016-03-31 09:31:27]

Patchset 22 (id:??) landed as
https://crrev.com/846f623cb1ef78d90e620c2a7a6557544a86b692
Cr-Commit-Position: refs/heads/master@{#384231}

[Sami, 2016-03-31 16:21:03]

On 2016/03/31 09:31:27, commit-bot: I haz the power wrote:
> Patchset 22 (id:??) landed as
> https://crrev.com/846f623cb1ef78d90e620c2a7a6557544a86b692
> Cr-Commit-Position: refs/heads/master@{#384231}

This patch seems to have caused crashes in a number of performance tests. See
https://bugs.chromium.org/p/chromium/issues/detail?id=599505.

I'll do a manual revert since the patch is too big to be reverted automatically.