1494801 - Scrolled content doesn't repaint in cs.chromium.org (with layout.css.contain.enabled=true, for 'contain:paint')

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: chromium-scroll.png

STR:

 * Go to https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chromium/paper-fab/paper-fab.html
 * Try to scroll down.

Expected:

 * Content is immediately available.

Actual result:

 * Content doesn't get repainted until you force it by, e.g., clicking on the page.

I suspect this is a recent regression.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is a paint containment issue. The scroller has contain: paint. Disabling layout.css.contain.enabled fixes it.

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I'll try to look into this sometime in the next ~week, if nobody gets to it first.

(Note that our 'contain:paint' impl is pretty trivial -- it's just an early-return in nsFrame.h "ShouldApplyOverflowClipping()" and in nsFrame.cpp "IsStackingContext()".)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Looks like ShouldApplyOverflowClipping is just wrong for scrollable frames, see bug 1178382.

Markus explained the reason for that in the patch for that bug, probably it's still up-to-date.

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: testcase 1

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I think we don't need to bother returning true from ShouldApplyOverflowClipping() here, actually!  (Or for any "contain:paint; overflow: [non-visible]" frames).

I'm pretty sure we don't need the scrollport to do any "bonus" contain:paint clipping (via ShouldApplyOverflowClipping), because:
 - the scrollport will already clip any overflowing content (and turn it into scrollable overflow)
 - contain:paint makes us a fixed-pos containing block, so elements can't escape that scrollport by reparenting themselves to a further-up-the-tree containing block.

There may also be some general stuff to fix for scrollable frames with ShouldApplyOverflowClipping()==true, as hinted at in comment 3.  But I suspect the simplest solution here is just to make scrollable frames "opt out" of the IsContainPaint() logic in that function, since it's unnecessary.

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: Bug 1494801: Don't add additional "contain:paint" clipping on scroll frames. r?mattwoodrow

The contain:paint clipping would be redundant and hence unnecessary in this
scenario, because:
- Scroll frames already clip their descendant frames.
- contain:paint has other (non-clipping-related) effects that prevent
descendant frames from escaping the scrollframe ancestor.

So, no further clipping is required.

This is a behavior change - it works around an issue that makes us fail to
repaint mousewheel-scrolled content inside of any scrollframe that returns true
from ShouldApplyOverflowClipping().

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Green try run (though I've got a typo'd bug number in the commit message, which I've since fixed on phabricator):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10dab11b49328c5d0190e419793054874c59ca28

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5884c1ce6696
Don't add additional "contain:paint" clipping on scroll frames. r=mattwoodrow

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/5884c1ce6696

Comment 11

[Oana Botisan, Desktop QA]

Can you please give me some additional info?

I can't seem to reproduce the issue and I used the steps from comment 0 using Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64. The scroll went smoothly every time.

I even enabled layout.css.contain.enabled to see if I can reproduce it, but I still couldn't.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

In which version of Firefox was this? You need to refresh the page after setting the pref.

I can easily repro this using:

mozregression --launch 64 --pref layout.css.contain.enabled:true -a https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chromium/paper-fab/paper-fab.html

Comment 13

[Oana Botisan, Desktop QA]

I used an older version of Nightly (2018-09-27). I restarted the browser after I changed the pref.
I will try again using the new info. Thanks

Comment 14

[Daniel Holbert [:dholbert] (PTO until 2/24)]

For what it's worth, I can reproduce the bug in that specific Nightly version 2018-09-27, on linux, using this mozregression command:

mozregression --launch 2018-09-27 --pref layout.css.contain.enabled:true -a https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chromium/paper-fab/paper-fab.html

Comment 15

[Oana Botisan, Desktop QA]

I know why I couldn't reproduce the issue, even though I was using mozregression, after the restart (changing the pref), the browser updated to the last version of Nightly (where the bug is fixed).

I finally managed to reproduce the issue using the method from comment 12 on Ubuntu 16.04 x64. I retested everything using the latest Nightly 66.0a1 and beta 65.0b9 on Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.13. The bug is not reproducing anymore.