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.

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.

(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.

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.

(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.

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

(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 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.

This was missing a replacement I had locally.

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

Waiting on tryresults