1483946 - Google News is unscrollable if you have CSS Containment enabled

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Set layout.css.contain.enabled = true
 1. Visit https://news.google.com/
 2. Try to scroll the main content area.

ACTUAL RESULTS:
Cannot scroll. No vertical scrollbar.

EXPECTED RESULTS:
I'd like to be able to scroll through the headlines.

Chrome gives EXPECTED RESULTS. Firefox nightly gives ACTUAL RESULTS.


The scrollable element (an element called "<c-wiz ...>" has this styling:

  overflow-y: auto;
  contain: layout style;

I think we're doing the right thing, actually, per this requirement in the spec:
> If the contents of the element overflow the element,
> they must be treated as ink overflow.

(This is why we're not creating a scrollbar - we're creating ink overflow rather than scrollable overflow, for the overflowing descendants.)

We should figure out what's going on differently in Firefox vs. Chrome here, and either:
 - file a Chrome bug (and do outreach to Google News team)
 - ...or fix Firefox, if we happen to end up being wrong.
to get alignment.

Comment 1

[Daniel Holbert [:dholbert]]

Reduced testcase: http://jsfiddle.net/knao5wu3/

CC Geral

Comment 2

[Daniel Holbert [:dholbert]]

CC'ing Gérard -- Gérard, do you know if there are any "ink overflow" web platform testcases that were meant to cover this?

Comment 3

[Daniel Holbert [:dholbert]]

Reduced testcase: https://jsfiddle.net/knao5wu3/

In Firefox with containment preffed on, there's no scrollbar on the orange area.
In Chrome, there is a scrollbar on the orange area.

Comment 4

[Daniel Holbert [:dholbert]]

Maybe this is just us having misunderstood the intent of spec...  I guess you could argue that the children of a scrollable element don't "overflow the element" in the way that the spec means here.

They do overflow *from the perspective of that element* (and they create a scrollbar as a result), but they don't overflow the scroll-area's content box into the surrounding area of the page -- i.e. they don't create any overflow for that element to propagate to its scrollable ancestors.

Comment 5

[Daniel Holbert [:dholbert]]

I filed https://github.com/w3c/csswg-drafts/issues/3023 to ask for spec clarification.  I tend to think Chrome's behavior seems more useful here (where, if I'm interpreting it correctly, contain:layout basically prevents child overflow from *contributing to the parent's overflow areas* -- i.e. it only prevents scrollbars from being created for *ancestors* of the contain:layout element.)

Comment 6

[Gérard Talbot]

I went to 

https://news.google.com/?hl=en-US&gl=US&ceid=US:en

with Firefox 63.0a1 buildID=20180812220618 and with Chromium 68.0.3440.75 and, indeed, there is no vertical scrollbar for the whole content area in Firefox 63 but there is for Chromium 68.
 
> The scrollable element (an element called "<c-wiz ...>" has this styling:
> 
>   overflow-y: auto;
>   contain: layout style;

I see that <c-wiz class="zQTmif SSPGKf eejsDc">

.zQTmif {
    height: 100%;
}
.eejsDc {
    overflow-y: auto;
}
.SSPGKf {
    display: block;
    overflow-y: hidden;
    z-index: 1;
}

c-wiz {
    contain: layout style;
}

The contain test suite currently has no tests involving 'overflow-y' or involving 'overflow-x' with regards to ink overflow. This is probably a weak area of the test suite.

Comment 7

[Gérard Talbot]

4 quick tests based on your jsfiddle.net/knao5wu3/ :

Test with 'contain: layout' and 'overflow-y: auto'
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-023.html

Test with 'contain: layout' and 'overflow-x: auto'
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-024.html

Test with 'contain: layout' and 'overflow-y: scroll'
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-025.html

Test with 'contain: layout' and 'overflow-x: scroll'
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-026.html

Comment 8

[Daniel Holbert [:dholbert]]

Tab confirmed on the github issue that Chrome's behavior is what the spec meant to call for.

Quoting him:
> Ah, we intended that statement ... to refer to contents
> that actually spilled out of the element, aka overflow:visible.
> The intention is just that the containing element doesn't
> project its contents' geometry outside of itself (and thus
> you don't need to care about the contents when figuring out the
> overflow bounds of the element).
> 
> If the containing element is scrollable, it already traps the
> geometry of its contents, so nothing needs to be done.

Comment 9

[Gérard Talbot]

Test with 'contain: layout' and 'overflow-y: hidden' (with text)
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-027.html

Test with 'contain: layout' and 'overflow-y: hidden' (with images)
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-028.html

Test with 'contain: layout' and 'overflow-x: hidden' (with contiguous words)
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-029.html

Test with 'contain: layout' and 'overflow: hidden' (with contiguous images causing line wrapping)
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-ink-overflow-030.html

Comment 10

[Daniel Holbert [:dholbert]]

CC'ing Dan/Gerald (not to be confused with Gérard), who I think are both going to be helping finish off 'contain' work.

I suspect we need to revert the scrolledContent specific stuff that we added in Bug 1476495, here:
https://hg.mozilla.org/mozilla-central/rev/be0650692abc#l1.66

We probably want to remove that whole top chunk (with the conditional "aDisplay = mParent->StyleDisplay()" assignment), and then we should edit the comment below to remove this snippet which is no longer applicable:
>   // [...] (or, per above, we are a scrollframe's
>   // inner anonymous box and our parent has layout containment)


And we should make the same change in nsFrame::ConsiderChildOverflow:
https://hg.mozilla.org/mozilla-central/rev/be0650692abc#l2.15


With those changes, we *might* start passing the test here.  I suspect we'll start failing some of Bug 1476495's testcases, but probably only because those testcases were based on a misunderstanding of the spec authors' intentions, and they need to be updated per comment 8.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Following Daniel's suggestion in comment 10 fixed the issue (in Google News and all testcases given in this bug).

I'll verify further and fix testcases if needed, before pushing for review.

Interestingly, test 29 from comment 9 fails on Chrome the same way as unpatched Firefox, but then it passes on patch Firefox (good) -- even with layout.css.contain.enabled = false!?

Comment 12

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1483946 - Fix CSS containment issue with scrollable elements - r?dholbert

Scrollable elements already trap all of their contents, nothing should spill
out, so there is no need for special handling of the `contain` CSS property.

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36a4850a8ba78be8952520bfcb2bbab1ff800912

Comment 14

[Gérard Talbot]

In development:

Scroller with 'contain: layout' and with various overflow-[ x | y ] values and a too wide or too tall descendant
http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-overflow-x-y-dhtml.html

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try with the test:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]

And I tested locally that the test alone (without the fix) revealed the bug.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 9002664 [details]
Bug 1483946 - Fix CSS containment issue with scrollable elements - r?dholbert

Daniel Holbert [:dholbert] has approved the revision.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/833c2ea661a6
Fix CSS containment issue with scrollable elements - r=dholbert

Comment 18

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/833c2ea661a6

Comment 19

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce this issue using Firefox 63.0a1 (BuildId:20180816220128) on Windows 10 64bit.

This issue is verified fixed using Firefox 63.0b7 (BuildId:20180917143811) on Windows 10 64bit, macOS 10.13.6 and Ubuntu 16.04 64bit.