Do not promote position sticky or fixed elements unless they move with scroll.

Do not promote position sticky or fixed elements unless they move with scroll.

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

[flackr, 2017-02-24 20:38:00]

Description was changed from

==========
Do not promote position sticky or fixed elements unless they move with scroll.

BUG=None
==========

to

==========
Do not promote position sticky or fixed elements unless they move with scroll.

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

[flackr, 2017-02-24 22:59:43]

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

[flackr, 2017-02-24 22:59:44]

Hey, I left a few comments throughout to explain the changes. I'm sure there are
more bugs that existed before (i.e. through referencing the wrong scroll
ancestor, or assuming that two sticky things scroll with respect to each other)
than I have tested but I'm not sure exactly how to cause them in a real page
(I'm assuming sticky layers may have incorrectly been squashed together?).

https://codereview.chromium.org/2716583005/diff/1/third_party/WebKit/Source/c...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(left):

https://codereview.chromium.org/2716583005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:270:
directReasons |= CompositingReasonScrollDependentPosition;
This was adding the scroll dependent position reason regardless of whether the
sticky / fixed element actually moves.

https://codereview.chromium.org/2716583005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left):

https://codereview.chromium.org/2716583005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:350:
->findEnclosingScrollNode();
Do we still need to something like this for sticksToScroller?

https://codereview.chromium.org/2716583005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:362:
(!ancestorScrollingLayer() || ancestorScrollingLayer() == root()));
ancestorScrollingLayer was not the correct "scroller" for sticky because it
excludes non-visible overflow which isn't actually scrollable.

[flackr, 2017-02-24 23:08:44]

Description was changed from

==========
Do not promote position sticky or fixed elements unless they move with scroll.

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

to

==========
Do not promote position sticky or fixed elements unless they move with scroll.

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

[flackr, 2017-03-14 21:59:01]

ping?

[pdr., 2017-03-15 22:22:57]

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

[pdr., 2017-03-15 22:23:44]

On 2017/03/14 at 21:59:01, flackr wrote:
> ping?

Sorry, I'm behind on reviews this week.

Chris, I think you're more familiar with this area than I am. Could you take a
look?

[chrishtr, 2017-03-16 00:24:04]

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

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:253:
style()->hasStickyConstrainedPosition())
This was a bug before?

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h
(right):

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h:29:
CompositingReasons directReasons(const PaintLayer*, bool ignoreLCDText) const;
Please add a comment explaining what ignoreLCDText does here.

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

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:88:
document().frame()->settings()->setPreferCompositingToLCDTextEnabled(true);
Does this just composite the class='scroller' element? Alternatively
you can put will-change: transform on it.

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:620: bool sticksToScroller()
const;
Please add comments documenting these.

[flackr, 2017-03-20 21:04:58]

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

[flackr, 2017-03-20 21:04:59]

I can probably split this up into three patches:
- Walk inline container chain for nested sticky
- Don't composite non-anchored sticky elements
- Don't compute constraints for non-anchored sticky elements
The 2nd and 3rd are fairly small distributed changes, but this might make it
easier to review?

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

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:253:
style()->hasStickyConstrainedPosition())
On 2017/03/16 00:24:03, chrishtr wrote:
> This was a bug before?

Not so much a bug, just hasStickyConstrainedPosition now also checks that it is
actually constrained by one of top/bottom/left/right. If it's not we should
treat position sticky as if it is position relative (i.e. not cause slow
scrolling, not promote it, etc). I realized this also needs to be updated in
CompositingInputsUpdater, so I've updated it there and added some tests.
Interestingly, as far as I can tell the SPv2 version never adds sticky position
to the frameview viewport constrained set (but with root layer scrolling we
don't need this anymore).

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h
(right):

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h:29:
CompositingReasons directReasons(const PaintLayer*, bool ignoreLCDText) const;
On 2017/03/16 00:24:04, chrishtr wrote:
> Please add a comment explaining what ignoreLCDText does here.

Done.

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

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:88:
document().frame()->settings()->setPreferCompositingToLCDTextEnabled(true);
On 2017/03/16 00:24:04, chrishtr wrote:
> Does this just composite the class='scroller' element? Alternatively
> you can put will-change: transform on it.

Done.

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:620: bool sticksToScroller()
const;
On 2017/03/16 00:24:04, chrishtr wrote:
> Please add comments documenting these.

Done.

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

https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:83: ?
maybeStickyAncestor->container()
Stephen I noticed when I added an unanchored sticky parent that if you had
deeply nested inlines this code used to skip straight past the first parent to
the containing block.

[smcgruer, 2017-03-21 14:06:20]

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

https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:83: ?
maybeStickyAncestor->container()
On 2017/03/20 21:04:59, flackr wrote:
> Stephen I noticed when I added an unanchored sticky parent that if you had
> deeply nested inlines this code used to skip straight past the first parent to
> the containing block.

Good catch, this should not have been jumping to the containing block. Can you
add a test for exactly that scenario? (I.e. multiple inline elements between the
inline sticky ancestor and the current sticky)?

[chrishtr, 2017-03-21 16:43:10]

On 2017/03/20 at 21:04:59, flackr wrote:
> I can probably split this up into three patches:
> - Walk inline container chain for nested sticky
> - Don't composite non-anchored sticky elements
> - Don't compute constraints for non-anchored sticky elements
> The 2nd and 3rd are fairly small distributed changes, but this might make it
easier to review?

Yes please, that would be great.

> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:253:
style()->hasStickyConstrainedPosition())
> On 2017/03/16 00:24:03, chrishtr wrote:
> > This was a bug before?
> 
> Not so much a bug, just hasStickyConstrainedPosition now also checks that it
is actually constrained by one of top/bottom/left/right. If it's not we should
treat position sticky as if it is position relative (i.e. not cause slow
scrolling, not promote it, etc). I realized this also needs to be updated in
CompositingInputsUpdater, so I've updated it there and added some tests.
Interestingly, as far as I can tell the SPv2 version never adds sticky position
to the frameview viewport constrained set (but with root layer scrolling we
don't need this anymore).
> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h
(right):
> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h:29:
CompositingReasons directReasons(const PaintLayer*, bool ignoreLCDText) const;
> On 2017/03/16 00:24:04, chrishtr wrote:
> > Please add a comment explaining what ignoreLCDText does here.
> 
> Done.
> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp
(right):
> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:88:
document().frame()->settings()->setPreferCompositingToLCDTextEnabled(true);
> On 2017/03/16 00:24:04, chrishtr wrote:
> > Does this just composite the class='scroller' element? Alternatively
> > you can put will-change: transform on it.
> 
> Done.
> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayer.h (right):
> 
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayer.h:620: bool sticksToScroller()
const;
> On 2017/03/16 00:24:04, chrishtr wrote:
> > Please add comments documenting these.
> 
> Done.
> 
>
https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:83: ?
maybeStickyAncestor->container()
> Stephen I noticed when I added an unanchored sticky parent that if you had
deeply nested inlines this code used to skip straight past the first parent to
the containing block.

[chrishtr, 2017-03-21 16:43:16]

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

https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:83: ?
maybeStickyAncestor->container()
On 2017/03/21 at 14:06:19, smcgruer wrote:
> On 2017/03/20 21:04:59, flackr wrote:
> > Stephen I noticed when I added an unanchored sticky parent that if you had
> > deeply nested inlines this code used to skip straight past the first parent
to
> > the containing block.
> 
> Good catch, this should not have been jumping to the containing block. Can you
add a test for exactly that scenario? (I.e. multiple inline elements between the
inline sticky ancestor and the current sticky)?

container() is the containing block. containingBlock() is something slightly
different.

[flackr, 2017-03-21 16:56:33]

Okay, will split this up and post new reviews.

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

https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:83: ?
maybeStickyAncestor->container()
On 2017/03/21 16:43:16, chrishtr wrote:
> On 2017/03/21 at 14:06:19, smcgruer wrote:
> > On 2017/03/20 21:04:59, flackr wrote:
> > > Stephen I noticed when I added an unanchored sticky parent that if you had
> > > deeply nested inlines this code used to skip straight past the first
parent
> to
> > > the containing block.
> > 
> > Good catch, this should not have been jumping to the containing block. Can
you
> add a test for exactly that scenario? (I.e. multiple inline elements between
the
> inline sticky ancestor and the current sticky)?
> 
> container() is the containing block. containingBlock() is something slightly
> different.

FYI, container() isn't equivalent to CSS containing block. It returns parent()
for non absolute / fixed, but CSS defines the containing block to be the nearest
block container parent (i.e. skipping inlines)
https://www.w3.org/TR/CSS2/visudet.html#containing-block-details.

[flackr, 2017-03-24 04:51:03]

On 2017/03/21 16:43:10, chrishtr wrote:
> On 2017/03/20 at 21:04:59, flackr wrote:
> > I can probably split this up into three patches:
> > - Walk inline container chain for nested sticky
> > - Don't composite non-anchored sticky elements
> > - Don't compute constraints for non-anchored sticky elements
> > The 2nd and 3rd are fairly small distributed changes, but this might make it
> easier to review?
> 
> Yes please, that would be great.

Split into three separate reviews and closed this one:
Walk inline container chain for nested sticky:
https://codereview.chromium.org/2776643002/
Don't composite non-anchored sticky elements:
https://codereview.chromium.org/2776563003/
Don't compute constraints for non-anchored sticky:
https://codereview.chromium.org/2769353002/ (dependent on previous patch)
> 
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:253:
> style()->hasStickyConstrainedPosition())
> > On 2017/03/16 00:24:03, chrishtr wrote:
> > > This was a bug before?
> > 
> > Not so much a bug, just hasStickyConstrainedPosition now also checks that it
> is actually constrained by one of top/bottom/left/right. If it's not we should
> treat position sticky as if it is position relative (i.e. not cause slow
> scrolling, not promote it, etc). I realized this also needs to be updated in
> CompositingInputsUpdater, so I've updated it there and added some tests.
> Interestingly, as far as I can tell the SPv2 version never adds sticky
position
> to the frameview viewport constrained set (but with root layer scrolling we
> don't need this anymore).
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> > File
> third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h
> (right):
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.h:29:
> CompositingReasons directReasons(const PaintLayer*, bool ignoreLCDText) const;
> > On 2017/03/16 00:24:04, chrishtr wrote:
> > > Please add a comment explaining what ignoreLCDText does here.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> > File
>
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinderTest.cpp:88:
> document().frame()->settings()->setPreferCompositingToLCDTextEnabled(true);
> > On 2017/03/16 00:24:04, chrishtr wrote:
> > > Does this just composite the class='scroller' element? Alternatively
> > > you can put will-change: transform on it.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintLayer.h (right):
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintLayer.h:620: bool
sticksToScroller()
> const;
> > On 2017/03/16 00:24:04, chrishtr wrote:
> > > Please add comments documenting these.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2716583005/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:83: ?
> maybeStickyAncestor->container()
> > Stephen I noticed when I added an unanchored sticky parent that if you had
> deeply nested inlines this code used to skip straight past the first parent to
> the containing block.