Avoid touching z-index in StyleAdjuster by using an isStackingContext flag instead

Avoid touching z-index in StyleAdjuster by using an isStackingContext flag instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we compute and inherit the correct z-index value
almost all of the time (see new test failure expectations).
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
Committed: https://crrev.com/a46d892723fe86a7f49113c46e3c40c2343724ce
Cr-Commit-Position: refs/heads/master@{#403844}

[alancutter (OOO until 2018), 2016-06-08 09:58:45]

Description was changed from

==========
Make isStackingContext a flag on ComputedStyle, cease adjusting z-index

BUG=616674
==========

to

==========
Make isStackingContext a flag on ComputedStyle, cease adjusting z-index [WIP]

BUG=616674
==========

[alancutter (OOO until 2018), 2016-06-08 09:59:32]

Description was changed from

==========
Make isStackingContext a flag on ComputedStyle, cease adjusting z-index [WIP]

BUG=616674
==========

to

==========
Make isStackingContext a flag on ComputedStyle and cease adjusting z-index [WIP]

BUG=616674
==========

[alancutter (OOO until 2018), 2016-06-09 04:46:27]

Description was changed from

==========
Make isStackingContext a flag on ComputedStyle and cease adjusting z-index [WIP]

BUG=616674
==========

to

==========
Avoid touching z-index in StyleAdjuster and use an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value and by updating
the flag outside of style resolution ensures new CSS animations that
affect stacking are accounted for.

BUG=616674, 375982
==========

[alancutter (OOO until 2018), 2016-06-09 04:46:27]

alancutter@chromium.org changed reviewers:
+ chrishtr@chromium.org, esprehn@chromium.org

[alancutter (OOO until 2018), 2016-06-09 04:47:17]

Description was changed from

==========
Avoid touching z-index in StyleAdjuster and use an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value and by updating
the flag outside of style resolution ensures new CSS animations that
affect stacking are accounted for.

BUG=616674, 375982
==========

to

==========
Avoid touching z-index in StyleAdjuster and use an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value and by updating
the flag outside of style resolution ensures new CSS animations that
affect stacking are accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

[alancutter (OOO until 2018), 2016-06-09 04:49:17]

Description was changed from

==========
Avoid touching z-index in StyleAdjuster and use an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value and by updating
the flag outside of style resolution ensures new CSS animations that
affect stacking are accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

to

==========
Avoid touching z-index in StyleAdjuster and use an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value.
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

[alancutter (OOO until 2018), 2016-06-09 04:49:26]

+esprehn for ComputedStyle & StyleRareNonInheritedData.
+christhtr for everything else.

[alancutter (OOO until 2018), 2016-06-09 04:49:32]

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

[commit-bot: I haz the power, 2016-06-09 04:49:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047283002/40001

[alancutter (OOO until 2018), 2016-06-09 05:25:09]

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

[commit-bot: I haz the power, 2016-06-09 05:25:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047283002/100001

[commit-bot: I haz the power, 2016-06-09 06:55:00]

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

[commit-bot: I haz the power, 2016-06-09 06:55:01]

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_...)

[alancutter (OOO until 2018), 2016-06-09 08:02:29]

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

[commit-bot: I haz the power, 2016-06-09 08:03:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047283002/120001

[alancutter (OOO until 2018), 2016-06-09 08:05:24]

Description was changed from

==========
Avoid touching z-index in StyleAdjuster and use an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value.
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

to

==========
Avoid touching z-index in StyleAdjuster by using an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value.
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

[rune, 2016-06-09 09:10:24]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2016-06-09 09:10:25]

375982 is not fixed since we're still adjusting z-index to auto for
non-positioned elements:

<!DOCTYPE html>
<style>
    div { z-index: 1 }
    span { z-index: inherit; position: relative }
</style>
<div>
    <span></span>
</div>
<script>
    console.log("should be 1:
"+getComputedStyle(document.querySelector("span")).zIndex);
</script>

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css3/blending/isolation-isolate-simple.html
(right):

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css3/blending/isolation-isolate-simple.html:24:
assert_true(internals.isStackingContext(isolator_accelerated), 'hardware path');
I think we should avoid adding new internals APIs.

It's possible to test if the element establishes a stacking context by putting a
negative z-index element inside it which would end up behind the element if it
didn't establish a stacking context, otherwise in front, and make it a ref-test.

A unit test is also an alternative.

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

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutFullScreen.cpp:101:
fullscreenStyle->updateIsStackingContext(false, false);
Multiple bools like this hurts readability.

[rune, 2016-06-09 09:13:43]

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (left):

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:184:
style.setHasAutoZIndex();
This also incorrectly affects computed style.

[commit-bot: I haz the power, 2016-06-09 11:05:04]

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

[commit-bot: I haz the power, 2016-06-09 11:05:06]

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

[alancutter (OOO until 2018), 2016-06-10 04:59:31]

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/css3/blending/isolation-isolate-simple.html
(right):

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/css3/blending/isolation-isolate-simple.html:24:
assert_true(internals.isStackingContext(isolator_accelerated), 'hardware path');
On 2016/06/09 at 09:10:25, rune wrote:
> I think we should avoid adding new internals APIs.
> 
> It's possible to test if the element establishes a stacking context by putting
a negative z-index element inside it which would end up behind the element if it
didn't establish a stacking context, otherwise in front, and make it a ref-test.
> 
> A unit test is also an alternative.

Done.

Removed the stacking context check from the animation test since it's really
testing for the debug crash.

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

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutFullScreen.cpp:101:
fullscreenStyle->updateIsStackingContext(false, false);
On 2016/06/09 at 09:10:25, rune wrote:
> Multiple bools like this hurts readability.

Replaced with enums though it looks a bit ugly.

[alancutter (OOO until 2018), 2016-06-10 05:36:47]

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (left):

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:184:
style.setHasAutoZIndex();
On 2016/06/09 at 09:13:43, rune wrote:
> This also incorrectly affects computed style.

Thanks for the catch. Updated test and added setIsStackingContext() which
removed the need for adding enums in place of bools.

[alancutter (OOO until 2018), 2016-06-10 08:24:47]

Description was changed from

==========
Avoid touching z-index in StyleAdjuster by using an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value.
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

to

==========
Avoid touching z-index in StyleAdjuster by using an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value almost all of the time
(see new test failure expectations).
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

[alancutter (OOO until 2018), 2016-06-10 08:25:21]

Description was changed from

==========
Avoid touching z-index in StyleAdjuster by using an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we inherit the correct z-index value almost all of the time
(see new test failure expectations).
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

to

==========
Avoid touching z-index in StyleAdjuster by using an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we compute and inherit the correct z-index value
almost all of the time (see new test failure expectations).
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

[alancutter (OOO until 2018), 2016-06-10 08:26:00]

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (left):

https://codereview.chromium.org/2047283002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:184:
style.setHasAutoZIndex();
On 2016/06/10 at 05:36:47, alancutter wrote:
> On 2016/06/09 at 09:13:43, rune wrote:
> > This also incorrectly affects computed style.
> 
> Thanks for the catch. Updated test and added setIsStackingContext() which
removed the need for adding enums in place of bools.

I spoke too soon, looks like layering depends on this code path resetting the
z-index to 0 as a side effect of setting it to auto
(fast/layers/opacity-stacking.html fails otherwise). Fixing this would require
yet another flag which I'm not too comfortable with adding.
Preserved setting z-index to 0 if not already auto and added TODO and text
failure expectations.

[alancutter (OOO until 2018), 2016-06-10 08:46:14]

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

[commit-bot: I haz the power, 2016-06-10 08:46:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047283002/220001

[commit-bot: I haz the power, 2016-06-10 10:15:27]

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

[commit-bot: I haz the power, 2016-06-10 10:15:28]

Dry run: This issue passed the CQ dry run.

[rune, 2016-06-10 12:25:52]

lgtm with comment nits.

https://codereview.chromium.org/2047283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2047283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:156: //
TODO(alancutter): Avoid altering z-index here.
Could you add the crbug issue here as well? I've attached another case to the
issue which shows the problem with this adjustment.

https://codereview.chromium.org/2047283002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (left):

https://codereview.chromium.org/2047283002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:814: // StyleAdjuster has
ensured that zIndex is non-auto only if it's applicable.
This comment is still valid with the last patchset.

[chrishtr, 2016-06-10 22:09:04]

PaintLayerStackingNode also has stale references to non-auto z-index.

[alancutter (OOO until 2018), 2016-06-30 06:36:45]

On 2016/06/10 at 22:09:04, chrishtr wrote:
> PaintLayerStackingNode also has stale references to non-auto z-index.

Are you referring to the comments in PaintLayerStackingNode.h and
core/paint/README.md?
As far as I can tell they appear to be referencing pre-adjustment z-index for
which the stacking context logic isn't being changed in this patch rather it's
being moved into ComputedStyle::isStackingContext() instead of being baked into
the adjusted z-index.

Sorry for the delayed response, I was OOO for BlinkOn and vacation.

[chrishtr, 2016-06-30 20:04:52]

On 2016/06/30 at 06:36:45, alancutter wrote:
> On 2016/06/10 at 22:09:04, chrishtr wrote:
> > PaintLayerStackingNode also has stale references to non-auto z-index.
> 
> Are you referring to the comments in PaintLayerStackingNode.h and
core/paint/README.md?
> As far as I can tell they appear to be referencing pre-adjustment z-index for
which the stacking context logic isn't being changed in this patch rather it's
being moved into ComputedStyle::isStackingContext() instead of being baked into
the adjusted z-index.
> 
> Sorry for the delayed response, I was OOO for BlinkOn and vacation.


In PaintLayerStackingNode.h, it says:

"PaintLayerStackingNode represents a stacked element which is either a stacking
context (i.e. an element with non-auto z-index) ..."

Doesn't that refer directly to the old way of messing with z-index to induce a
stacking context?

[alancutter (OOO until 2018), 2016-07-05 04:57:55]

On 2016/06/30 at 20:04:52, chrishtr wrote:
> On 2016/06/30 at 06:36:45, alancutter wrote:
> > On 2016/06/10 at 22:09:04, chrishtr wrote:
> > > PaintLayerStackingNode also has stale references to non-auto z-index.
> > 
> > Are you referring to the comments in PaintLayerStackingNode.h and
core/paint/README.md?
> > As far as I can tell they appear to be referencing pre-adjustment z-index
for which the stacking context logic isn't being changed in this patch rather
it's being moved into ComputedStyle::isStackingContext() instead of being baked
into the adjusted z-index.
> > 
> > Sorry for the delayed response, I was OOO for BlinkOn and vacation.
> 
> 
> In PaintLayerStackingNode.h, it says:
> 
> "PaintLayerStackingNode represents a stacked element which is either a
stacking context (i.e. an element with non-auto z-index) ..."
> 
> Doesn't that refer directly to the old way of messing with z-index to induce a
stacking context?

Updated comments in PaintLayerStackingNode.h and core/paint/README.md. I'm not
entirely sure about what it was saying about positioned elements so I hope you
can verify that it's still correct.
I've erred on the side of talking less about implementation details since that's
what's necessitated the additional maintenance on comments.

[chrishtr, 2016-07-05 19:37:27]

lgtm

[alancutter (OOO until 2018), 2016-07-06 02:22:48]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-07-06 02:22:49]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/2047283002/#ps240001
(title: "Update comments")

[commit-bot: I haz the power, 2016-07-06 02:22:59]

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, 2016-07-06 03:57:57]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-07-06 03:59:58]

Description was changed from

==========
Avoid touching z-index in StyleAdjuster by using an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we compute and inherit the correct z-index value
almost all of the time (see new test failure expectations).
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982
==========

to

==========
Avoid touching z-index in StyleAdjuster by using an isStackingContext flag
instead

This change moves logic in StyleAdjuster for changing the computed
z-index out into ComputedStyle::updateIsStackingContext().

This ensures we compute and inherit the correct z-index value
almost all of the time (see new test failure expectations).
Also by updating the flag outside of style resolution any new
CSS animations that affect stacking are now accounted for.

This patch is an alternative to https://codereview.chromium.org/2035793007.

BUG=616674, 375982

Committed: https://crrev.com/a46d892723fe86a7f49113c46e3c40c2343724ce
Cr-Commit-Position: refs/heads/master@{#403844}
==========

[commit-bot: I haz the power, 2016-07-06 04:00:00]

Patchset 13 (id:??) landed as
https://crrev.com/a46d892723fe86a7f49113c46e3c40c2343724ce
Cr-Commit-Position: refs/heads/master@{#403844}