1495470 - Google Photos' left sidebar menu items (e.g. "Albums") are unclickable, if CSS Containment is enabled

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Set the pref layout.css.contain.enabled to true
 2. Visit https://photos.google.com/ (and log in if you're not logged in)
 3. Try to hover and/or click the icons on the left sidebar (Photos, Albums, Assistant, Sharing, Photo books)

ACTUAL RESULTS:
 - The cursor doesn't change when I hover these icons.
 - Clicks on the icons have no effect.

EXPECTED RESULTS:
 - The cursor should change to reflect the fact that they're clickable.


If I right-click the icons and do "inspect" to see what's intercepting the mouse events, I can see that the clicks are being blocked by a large element <c-wiz ...>, which:
 - ...has padding-left:96px, which is the area that these icons are "shining through"
 - ...has "contain:layout style"

If I remove "layout" from its contain property, then I get "expected results". So something about contain:layout is making this element intercept clicks in its padding area.

Comment 1

[Daniel Holbert [:dholbert]]

Actually there are several <c-wiz> elements involved here, and I think its a more ancesterly one whose 'contain' styles are breaking things here.

Coming up with a reduced testcase...

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (obsolete/replaced due to typo in close tag)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 5

[Daniel Holbert [:dholbert]]

testcase 2 is pretty reduced. It shows a red box in Firefox, vs. a green box in Chrome.

Comment 6

[Daniel Holbert [:dholbert]]

So right now, containment is having an effect in the testcase. And the fact that it's having an effect is a bug.

Notes:
* "containment is having an effect": testcase 2 renders red in Nightly with layout.css.contain.enabled = true, but it renders as false if you turn off the pref or remove the 'contain:layout' styling.

* "that's a bug": The span here is a non-atomic inline-level box, and that means (per spec) the "contain:layout" styling should have no effect:
> ...if the element’s principal box is a non-atomic inline-level box, layout containment has no effect.
https://drafts.csswg.org/css-contain/#containment-layout


The testcase involves an IB split... I suspect our "should we honor layout containment" code (for determining stacking contexts perhaps) is wrong, for IB splits.

Comment 7

[Daniel Holbert [:dholbert]]

Yeah, so with content like...
   <span style="contain:layout">   <!--- (or contain:paint) -->
     <div></div>
   </span>

...we end up generating a frame tree that looks like:
>  line <
>    Inline(span)
>  >
>  line <
>    Block(span) <
>      line <
>        Block(div)
>      >
>    >
>  >
>  line <
>    Inline(span)
>  >

...and notably, the IB-split-generated block ("Block(span)") has the span's styles applied to it. So it has contain:layout, and  so since it's block-level, it thinks it should generate a stacking context.

But really, it shouldn't generating a stacking context, because the span's "principal box is a non-atomic inline-level box" (which means its 'contain' styling shouldn't matter).

Comment 8

[Daniel Holbert [:dholbert]]

It looks like the problem is just that nsIFrame::IsStackingContext() calls IsContainLayout() and IsContainPaint() without testing whether the frame supports them.

I think we need to add this check there, as a precondition for caring about IsContainLayout/IsContainPaint:
  IsFrameOfType(eSupportsContainLayoutAndPaint)


If I add that, it fixes this.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1495470: Only let 'contain:layout/paint' create stacking contexts on frames that support it.

Comment 10

[Daniel Holbert [:dholbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39af800dc57dae5c6d03729d99e0d005e01a605f

Comment 11

[Daniel Holbert [:dholbert]]

(Looks like the Try run shows that this makes us pass two annotated-as-failing WPT tests, too -- contain-layout-018.html and contain-paint-025.html. I'll remove the .ini expected-fail notations for those.)

Comment 12

[Daniel Holbert [:dholbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=607616618286d448da5820813e43d95d7e8fdce0

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b597edf0a98
Only let 'contain:layout/paint' create stacking contexts on frames that support it. r=dbaron

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3b597edf0a98

Comment 15

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

web-platform-tests PR at https://github.com/web-platform-tests/wpt/pull/13459