964885 - Track which CSS properties create a stacking context and make will-change on such properties create a stacking context

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This is branched off bug 940842 comment 79:

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #79)
> Also, it seems like implementing this part of the spec:
> 
> ----
> 	If any non-initial value of a property would create a stacking context on
> the element, specifying that property in will-change must create a stacking
> context on the element.
> 
> If a non-initial value of a property would cause rendering differences on
> the element (such as using a different anti-aliasing strategy for text), the
> user agent should use that alternate rendering when the property is
> specified in will-change, to avoid sudden rendering differences when the
> property is eventually changed. 
> ----
> 
> requires that you track quite a few more CSS properties than you're
> currently tracking.  I suspect it means you'll want to track enough
> properties that you'll want to add bits in nsCSSPropList.h to say which
> properties trigger stacking contexts.  I'd prefer fixing that in a followup
> patch, but it should happen before the property gets enabled.

Comment 1

[Mats Palmgren (inactive)]

Wouldn't it be better if the CSSWG maintained this list of properties somewhere?
Preferably in a machine readable format so that we could automate a check that
we implement it correctly.

Comment 2

[Benoit Girard (:BenWa)]

(In reply to Mats Palmgren (:mats) from comment #1)
> Wouldn't it be better if the CSSWG maintained this list of properties
> somewhere?
> Preferably in a machine readable format so that we could automate a check
> that
> we implement it correctly.

I'd be happy to compile the list based on what we currently do in Layout and submit that to the CSSWG.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the list should be:

z-index, position (for sticky!), opacity, transform, transform-style, perspective, clip-path, filter, mask, mix-blend-mode, plus the aliases -moz-transform, -moz-transform-style, -moz-perspective.

But our implementation looks like it doesn't match specs:  we seem to look at backface-visibility (incorrectly) and not look at perspective (which we're supposed to).  See isStackingContext in nsIFrame::BuildDisplayListForChild.  I suspect we should fix that (separate bug, though).

I'd prefer not to bug the CSS WG about this at this stage.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #3)
> But our implementation looks like it doesn't match specs:  we seem to look
> at backface-visibility (incorrectly) and not look at perspective (which
> we're supposed to).  See isStackingContext in
> nsIFrame::BuildDisplayListForChild.  I suspect we should fix that (separate
> bug, though).

I made this bug 968555.

Comment 5

[Benoit Girard (:BenWa)]

Useful test for stacking context:
http://codepen.io/philipwalton/pen/dfCtb

If div:first-child forces a stacking context the z-index for the red span will be ignored causing it to behind.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8371232 [details] [diff] [review]
patch

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>   case eCSSUnit_List:
>   case eCSSUnit_ListDep: {
>     display->mWillChange.Clear();
>     display->mWillChangeBitField = 0;
>+    display->mWillChangeStackingContext = false;
>     for (const nsCSSValueList* item = willChangeValue->GetListValue();
>          item; item = item->mNext)
>     {
>       if (item->mValue.UnitHasStringValue()) {
>         nsAutoString buffer;
>         item->mValue.GetStringValue(buffer);
>         if (buffer.EqualsLiteral("transform")) {
>           display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_TRANSFORM;
>         }
>         if (buffer.EqualsLiteral("opacity")) {
>           display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_OPACITY;
>         }
>         if (buffer.EqualsLiteral("scroll-position")) {
>           display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_SCROLL;
>         }
>+        if (buffer.EqualsLiteral("z-index") ||
>+          buffer.EqualsLiteral("position") || // position: sticky
>+          buffer.EqualsLiteral("opacity") ||
>+          buffer.EqualsLiteral("transform") ||
>+          buffer.EqualsLiteral("transform-style") ||
>+          buffer.EqualsLiteral("perspective") ||
>+          buffer.EqualsLiteral("clip-path") ||
>+          buffer.EqualsLiteral("filter") ||
>+          buffer.EqualsLiteral("mask") ||
>+          buffer.EqualsLiteral("mix-blend-mode") ||
>+          buffer.EqualsLiteral("-moz-transform") ||
>+          buffer.EqualsLiteral("-moz-transform-style") ||
>+          buffer.EqualsLiteral("-moz-perspective")) {
>+          display->mWillChangeStackingContext = true;
>+        }
>         display->mWillChange.AppendElement(buffer);

Instead of hard-coding a list here, please add a bit to nsCSSProps.h (CSS_PROPERTY_CREATES_STACKING_CONTEXT, 1<<22 unless somebody beats you to it), add that bit to the above list of properties (you shouldn't need to touch the aliases), and use nsCSSProps::LookupProperty and nsCSSProps:PropHasFlags

>   case eCSSUnit_Inherit:
>     display->mWillChange = parentDisplay->mWillChange;

You need to copy this in the eCSSUnit_Inherit case, and reset it in the Initial/Unset/Auto case.  (But see comment below about using the bitfield.)

>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
>@@ -2462,17 +2464,18 @@ nsChangeHint nsStyleDisplay::CalcDiffere
>     if (mChildPerspective != aOther.mChildPerspective ||
>         mTransformStyle != aOther.mTransformStyle)
>       NS_UpdateHint(hint, kUpdateOverflowAndRepaintHint);
> 
>     if (mBackfaceVisibility != aOther.mBackfaceVisibility)
>       NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
>   }
> 
>-  if (mWillChangeBitField != aOther.mWillChangeBitField) {
>+  if (mWillChangeBitField != aOther.mWillChangeBitField ||
>+    mWillChangeStackingContext != aOther.mWillChangeStackingContext) {
>     NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
>   }

I think this is overkill in both cases (the existing BitField case, and your new case).  I believe RepaintFrame should be sufficient for mWillChangeStackingContext.

(The existing case is a little more complicated, but we should probably have a bug on file on improving it.  The whole thing about this being a property for performance optimization doesn't work out so well if it performs badly.)

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
>   uint8_t mWillChangeBitField;  // [reset] see nsStyleConsts.h. Stores a bitfield
>                                 // representation of the property that
>                                 // are frequently queried. This should match
>                                 // mWillChange
>+  bool mWillChangeStackingContext; // Do any of the will-change property in
>+                                   // the list require a stacking context

Please:
 * add [reset] to the start of the comment
 * "the will-change property in the list" -> "the properties in the will-change list"

However... why not just use a bit in the bitfield instead of adding a whole separate boolean (which will probably take up 32/64 bits!).  That also simplifies the above comments about the Inherit and Initial cases.


Sorry for the delay getting to this.

Comment 8

[Benoit Girard (:BenWa)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #7)
> Instead of hard-coding a list here, please add a bit to nsCSSProps.h
> (CSS_PROPERTY_CREATES_STACKING_CONTEXT, 1<<22 unless somebody beats you to
> it), add that bit to the above list of properties (you shouldn't need to
> touch the aliases), and use nsCSSProps::LookupProperty and
> nsCSSProps:PropHasFlags

Thanks, I was hoping you would suggest something better in the review!

> (The existing case is a little more complicated, but we should probably have
> a bug on file on improving it.  The whole thing about this being a property
> for performance optimization doesn't work out so well if it performs badly.)

Ok, I'll get that file. I haven't spent much time trying to understand that hoping this would be easy for you to catch. With some guidance I can look at the follow up.

> boolean (which will probably take up 32/64 bits!).

8 bits size/align on all the ABIs I know, but good point.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=927dd9ff02c2

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 8377920 [details] [diff] [review]
patch

Review of attachment 8377920 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStruct.cpp
@@ +2467,5 @@
>        NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
>    }
>  
>    if (mWillChangeBitField != aOther.mWillChangeBitField) {
> +    if ((mWillChangeBitField & ~(uint32_t)NS_WILL_CHANGE_STACKING_CONTEXT) ==

Not a fan of how this turned out. bjacob thinks we can make this simpler with templates. There's a way to write this with XOR but I think it's a bit more complicated. Let me know if you're happy.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch

This was missing a replacement I had locally.

Comment 12

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=5fb90de24640

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8378461 [details] [diff] [review]
patch

Could you add the comment about the position annotation being for sticky to
nsCSSPropList.h?


>+        if (nsCSSProps::PropHasFlags(prop,
>+          CSS_PROPERTY_CREATES_STACKING_CONTEXT)) {

 (1) you need to check prop != eCSSProperty_UNKNOWN && nsCSSProps::PropHasFlags

 (2) please line up the second param with the first, or if that goes over 80,
     then 2 spaces in from the "nsCSSProps" (not 2 spaces in from the "if")

>+                                // bitfield representation of the property that

property that -> properties that


>+    if ((mWillChangeBitField & ~(uint32_t)NS_STYLE_WILL_CHANGE_STACKING_CONTEXT) ==
>+      (aOther.mWillChangeBitField & ~(uint32_t)NS_STYLE_WILL_CHANGE_STACKING_CONTEXT)) {
>+      NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
>+    } else {
>+      // Bug 974125 - Don't reconstruct the frame
>+      NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
>+    }

Please write the fixme comment as "FIXME (bug 974125):  Don't ...".

Also, I think it would be a lot simpler and clearer to do:

  uint8_t willChangeBitsChanged =
    mWillChangeBitField ^ aOther.mWillChangeBitField;
  if (willChangeBitsChanged & NS_STYLE_WILL_CHANGE_STACKING_CONTEXT) {
    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
  }
  if (willChangeBitsChanged & ~uint8_t(NS_STYLE_WILL_CHANGE_STACKING_CONTEXT)) {
    // FIXME (Bug 974125): Don't reconstruct the frame
    NS_UpdateHint(hint, nsChangeHint_ReconstructFrame);
  }

r=dbaron with that

Comment 14

[Benoit Girard (:BenWa)]

Attachment: Implement stacking context behavior of CSS will-change. r=dbaron

Waiting on tryresults

Comment 15

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1c76b3a931

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f1c76b3a931