1464290 - Make nsIFrame::IsVisuallyAtomic and ::IsStackingContext match the spec more closely.

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Matt Woodrow (:mattwoodrow)]

IsVisuallyAtomic doesn't appear to be mentioned in the specs that I can see, and our definition of IsStackingContext is hard to understand.

It appears that IsVisuallyAtomic returns true for frames that are both a stacking context *and* should be sorted as a positioned descendant in the outer stacking context.

IsStackingContext returns true for frames that are visually atomic, as well as frames that just establish a stacking context.

I think this is a bit confusing, since it duplicates logic. The 'transform' property for example, is documented as being a stacking context, and it's also positioned since it creates a containing blocked for absolute/fixed positioned descendants.

I think we could just make IsStackingContext return true for the set of properties that are supposed to create one, and then determine if we should be positioned independently using the actual reason for this (such as using nsStyleDisplay::IsAbsPosContaining, rather than IsTransformed in this case).

Looking at the actual properties:

Opacity: Creates a stacking context, and should be positioned because it overrides z-index:auto to be interpreted as z-index:0.

Transform, will-change:transform, perspective, filter: All create stacking contexts, and create containing blocks for absolute/fixed, so should be positioned.

mask-image: Creates a stacking context, in "the same way that CSS opacity [CSS3COLOR] does for values other than 1.". Does this mean including the override of z-index/positioned behaviour? It's how the code currently interprets it at least.

mix-blend-mode, isolation: Both just mention creating a stacking context, no mention of positioning/z-index (or comparisons to opacity). We're currently treating mix-blend-mode as positioned, and isolation:isolate as not. Probably want to at least be consistent here, I guess making mix-blend-mode non-positioned?

will-change: opacity: Spec only talks about stacking contexts, no mention of the effect on z-index interpretation. Our currently implementation just does the stacking context, and won't treat it as positioned.

David, these subtle differences in the specs are somewhat hard to interpret, and it's really not clear if the differences were intentional. Do you know if there are any plans to fix this, or should I just go ahead and fix our mismatches and clean things up?

Comment 1

[Matt Woodrow (:mattwoodrow)]

contain:paint is also supposed to create a stacking context (though we don't yet do that), but also establishes a containing block for absolute/fixed position, so is in the same group as transform.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> contain:paint is also supposed to create a stacking context (though we don't
> yet do that)

(Yusuf is working on that in bug 1463599, FWIW.)

Comment 3

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

So I tend to think that this distinction shouldn't exist, and everything that creates a stacking context should paint at the positioned descendants sort in the z-order.  I filed https://github.com/w3c/csswg-drafts/issues/2717 to request that the relevant specs be clearer.

Comment 4

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

I wrote a relatively thorough testcase for this today at https://dbaron.org/css/test/2018/stacking-context-z-order and while there's a good bit of lack of interop, it does show that Chrome and WebKit don't have this distinction -- everything that establishes a stacking context also paints at the positioned descendants level.

Comment 5

[Matt Woodrow (:mattwoodrow)]

That is an amazing testcase!

That's good though, I think we should just go ahead and do the same and simplify our code, and hope we can get the specs clarified to match what blink/webkit are doing.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

From what I understand, Hiro is waiting for this bug to land before fixing bug 1218169 (performance bug affecting google.com in various locales).

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1464290 - Merge nsIFrame::IsVisuallyAtomic and IsStackingContext since the conclusion is that we want their behaviour to be identical.

Review commit: https://reviewboard.mozilla.org/r/251558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251558/

Comment 8

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

Comment on attachment 8986097 [details]
Bug 1464290 - Merge nsIFrame::IsVisuallyAtomic and IsStackingContext since the conclusion is that we want their behaviour to be identical.

https://reviewboard.mozilla.org/r/251558/#review258942

We probably want to add some form of my test to web-platform-tests, though I won't necessarily make you do it.  (It should probably be separated into pieces to have a piece for each feature, and thus work correctly for browsers that don't implement all the relevant features.)

::: layout/generic/nsFrame.cpp:11014
(Diff revision 1)
> -    // If this is changed, the function above should be updated as well.
> -    return true;
> -  }
> -
>    const bool isPositioned = disp->IsAbsPosContainingBlock(this);
> -  return IsStackingContext(disp, StylePosition(),
> +  return IsStackingContext(EffectSet::GetEffectSet(this), disp, 

fix the trailing space here

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8986097 [details]
Bug 1464290 - Merge nsIFrame::IsVisuallyAtomic and IsStackingContext since the conclusion is that we want their behaviour to be identical.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251558/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bf5016de2d0
Merge nsIFrame::IsVisuallyAtomic and IsStackingContext since the conclusion is that we want their behaviour to be identical. r=dbaron

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6bf5016de2d0