Fix position:sticky location bug when scroller is not a stacking context

Fix position:sticky location bug when scroller is not a stacking context

This was broken in http://crrev.com/e481938 when I added code to correct
for scroll offset in intermediate layers - that logic is invalid if we
arent a LayerTree descendent of the scroller.

The reason that we didn't spot that this was broken is that we keep
using will-change:transform to force-enable compositing of elements, but
this also creates a stacking context. For now I have switched this to
backface-visibility:hidden, which isn't perfect but at least doesn't
create a stacking context.

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

Review-Url: https://codereview.chromium.org/2765503003
Cr-Commit-Position: refs/heads/master@{#459546}
Committed: https://chromium.googlesource.com/chromium/src/+/d5514f08a923485a8ef2a58cab355893d0e55fcb

[smcgruer, 2017-03-20 18:55:02]

Description was changed from

==========
Fix position:sticky location bug when scroller is not a stacking context

This was broken in http://crrev.com/e481938 when I added code to correct
for scroll offset in intermediate layers - that logic is invalid if we
arent a LayerTree descendent of the scroller.

The reason that we didn't spot that this was broken is that we keep
using will-change:transform to force-enable compositing of elements, but
this also creates a stacking context. For now I have switched this to
backface-visibility:hidden, which isn't perfect but at least doesn't
create a stacking context.

BUG=699137
==========

to

==========
Fix position:sticky location bug when scroller is not a stacking context

This was broken in http://crrev.com/e481938 when I added code to correct
for scroll offset in intermediate layers - that logic is invalid if we
arent a LayerTree descendent of the scroller.

The reason that we didn't spot that this was broken is that we keep
using will-change:transform to force-enable compositing of elements, but
this also creates a stacking context. For now I have switched this to
backface-visibility:hidden, which isn't perfect but at least doesn't
create a stacking context.

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

[smcgruer, 2017-03-20 18:55:48]

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

[smcgruer, 2017-03-20 18:56:06]

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

[smcgruer, 2017-03-20 18:56:07]

I plan to also update the unittests, just throwing this up for initial
discussion.

[commit-bot: I haz the power, 2017-03-20 18:56:35]

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

[commit-bot: I haz the power, 2017-03-20 19:19:54]

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

[commit-bot: I haz the power, 2017-03-20 19:19:54]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[smcgruer, 2017-03-21 12:46:03]

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

[commit-bot: I haz the power, 2017-03-21 12:46:14]

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

[commit-bot: I haz the power, 2017-03-21 14:09:14]

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

[commit-bot: I haz the power, 2017-03-21 14:09:15]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-03-21 14:09:45]

lgtm

https://codereview.chromium.org/2765503003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html
(right):

https://codereview.chromium.org/2765503003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html:8:
/* Triggers promotion without creating stacking context. */
Is it worth having a stacking context version of this test too?

[smcgruer, 2017-03-21 14:53:10]

https://codereview.chromium.org/2765503003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html
(right):

https://codereview.chromium.org/2765503003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html:8:
/* Triggers promotion without creating stacking context. */
On 2017/03/21 14:09:45, ajuma wrote:
> Is it worth having a stacking context version of this test too?

After discussion, yes we should do a version of this test since it actually
tests the 'apply scroll offset' case when its a stacking context. Done.

[smcgruer, 2017-03-21 14:53:11]

https://codereview.chromium.org/2765503003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html
(right):

https://codereview.chromium.org/2765503003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/compositing/overflow/composited-sticky-element-enclosing-layers.html:8:
/* Triggers promotion without creating stacking context. */
On 2017/03/21 14:09:45, ajuma wrote:
> Is it worth having a stacking context version of this test too?

After discussion, yes we should do a version of this test since it actually
tests the 'apply scroll offset' case when its a stacking context. Done.

[flackr, 2017-03-23 16:58:03]

LGTM with nits.

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

https://codereview.chromium.org/2765503003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322:
if (!scrollParent() && compositingContainer != ancestorOverflowLayer) {
It's probably worth "documenting" why we do this with a DCHECK to show that if
we have a scroll parent it must be our ancestor overflow layer:
DCHECK(!scrollParent() || scrollParent() == ancestorOverflowLayer);

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

https://codereview.chromium.org/2765503003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1555:
"<div class='composited container'>"
nit: indent.

[smcgruer, 2017-03-23 17:22:48]

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

https://codereview.chromium.org/2765503003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:322:
if (!scrollParent() && compositingContainer != ancestorOverflowLayer) {
On 2017/03/23 16:58:03, flackr wrote:
> It's probably worth "documenting" why we do this with a DCHECK to show that if
> we have a scroll parent it must be our ancestor overflow layer:
> DCHECK(!scrollParent() || scrollParent() == ancestorOverflowLayer);

Done.

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

https://codereview.chromium.org/2765503003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1555:
"<div class='composited container'>"
On 2017/03/23 16:58:03, flackr wrote:
> nit: indent.

Done.

[smcgruer, 2017-03-23 17:22:54]

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

[commit-bot: I haz the power, 2017-03-23 17:23:20]

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

[commit-bot: I haz the power, 2017-03-23 17:34:02]

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

[commit-bot: I haz the power, 2017-03-23 17:34:03]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[smcgruer, 2017-03-23 17:52:51]

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

[commit-bot: I haz the power, 2017-03-23 17:53:29]

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

[commit-bot: I haz the power, 2017-03-23 19:12:26]

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

[commit-bot: I haz the power, 2017-03-23 19:12:27]

Dry run: This issue passed the CQ dry run.

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

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-03-23 19:19:21]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/2765503003/#ps60001
(title: "Add DCHECK, whitespace fix")

[commit-bot: I haz the power, 2017-03-23 19:19:46]

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

[commit-bot: I haz the power, 2017-03-23 19:28:26]

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

[commit-bot: I haz the power, 2017-03-23 19:28:27]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[smcgruer, 2017-03-23 19:30:32]

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

[smcgruer, 2017-03-23 19:30:33]

Whoops, I need Blink OWNERS for this. +schenney for Blink OWNERS.

[smcgruer, 2017-03-24 17:29:20]

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

[smcgruer, 2017-03-24 17:29:20]

schenney is out today, so +pdr instead for Blink OWNERS :)

[pdr., 2017-03-24 18:32:43]

On 2017/03/24 at 17:29:20, smcgruer wrote:
> schenney is out today, so +pdr instead for Blink OWNERS :)

LGTM

[smcgruer, 2017-03-24 18:34:12]

The CQ bit was checked by smcgruer@chromium.org

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

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

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

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

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

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[smcgruer, 2017-03-24 19:33:05]

The failing test appears to run fine on my desktop. I'm going to assume flake
for this first try, and re-commit.

[smcgruer, 2017-03-24 19:33:11]

The CQ bit was checked by smcgruer@chromium.org

[commit-bot: I haz the power, 2017-03-24 19:34:04]

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

[commit-bot: I haz the power, 2017-03-24 20:55:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490383991365800,
"parent_rev": "bef2b02dde4315ab50b0b6ba89e5954d6ab85750", "commit_rev":
"d5514f08a923485a8ef2a58cab355893d0e55fcb"}

[commit-bot: I haz the power, 2017-03-24 20:56:31]

Description was changed from

==========
Fix position:sticky location bug when scroller is not a stacking context

This was broken in http://crrev.com/e481938 when I added code to correct
for scroll offset in intermediate layers - that logic is invalid if we
arent a LayerTree descendent of the scroller.

The reason that we didn't spot that this was broken is that we keep
using will-change:transform to force-enable compositing of elements, but
this also creates a stacking context. For now I have switched this to
backface-visibility:hidden, which isn't perfect but at least doesn't
create a stacking context.

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

to

==========
Fix position:sticky location bug when scroller is not a stacking context

This was broken in http://crrev.com/e481938 when I added code to correct
for scroll offset in intermediate layers - that logic is invalid if we
arent a LayerTree descendent of the scroller.

The reason that we didn't spot that this was broken is that we keep
using will-change:transform to force-enable compositing of elements, but
this also creates a stacking context. For now I have switched this to
backface-visibility:hidden, which isn't perfect but at least doesn't
create a stacking context.

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

Review-Url: https://codereview.chromium.org/2765503003
Cr-Commit-Position: refs/heads/master@{#459546}
Committed:
https://chromium.googlesource.com/chromium/src/+/d5514f08a923485a8ef2a58cab35...
==========

[commit-bot: I haz the power, 2017-03-24 20:56:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d5514f08a923485a8ef2a58cab35...