Closed Bug 1483946 Opened 6 years ago Closed 6 years ago

Google News is unscrollable if you have CSS Containment enabled

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- verified

People

(Reporter: dholbert, Assigned: mozbugz)

References

Details

(Keywords: testcase)

Attachments

(1 file)

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.
CC'ing Gérard -- Gérard, do you know if there are any "ink overflow" web platform testcases that were meant to cover this?
Blocks: 1476495
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.
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.
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.)
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.
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
Priority: -- → P3
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.
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
Keywords: testcase
OS: Unspecified → All
Hardware: Unspecified → All
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.
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!?
Assignee: nobody → gsquelart
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.
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
Try with the test:
https://treeherder.mozilla.org/#/jobs?repo=try&author=gsquelart@mozilla.com

And I tested locally that the test alone (without the fix) revealed the bug.
Comment on attachment 9002664 [details]
Bug 1483946 - Fix CSS containment issue with scrollable elements - r?dholbert

Daniel Holbert [:dholbert] has approved the revision.
Attachment #9002664 - Flags: review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/833c2ea661a6
Fix CSS containment issue with scrollable elements - r=dholbert
https://hg.mozilla.org/mozilla-central/rev/833c2ea661a6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: