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

VERIFIED FIXED in Firefox 65

Status

()

defect
P2
normal
VERIFIED FIXED
8 months ago
4 months ago

People

(Reporter: emilio, Assigned: dholbert)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 verified, firefox66 verified)

Details

()

Attachments

(3 attachments)

Posted image 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.
Reporter

Comment 1

8 months ago
This is a paint containment issue. The scroller has contain: paint. Disabling layout.css.contain.enabled fixes it.
Component: Graphics: WebRender → Layout: Web Painting
Assignee

Updated

8 months ago
Summary: Scrolled content doesn't repaint in cs.chromium.org. → Scrolled content doesn't repaint in cs.chromium.org (with layout.css.contain.enabled=true, for 'contain:paint')
Assignee

Comment 2

8 months ago
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()".)
Flags: needinfo?(dholbert)
Reporter

Comment 3

8 months ago
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.
Priority: -- → P2
Assignee

Updated

7 months ago
Flags: needinfo?(dholbert)
Assignee

Comment 4

7 months ago
Posted file testcase 1
Assignee

Comment 5

6 months ago
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.
Assignee

Updated

6 months ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Comment 6

6 months ago
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().
Assignee

Comment 7

6 months ago
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

6 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5884c1ce6696
Don't add additional "contain:paint" clipping on scroll frames. r=mattwoodrow

Comment 9

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5884c1ce6696
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Reporter

Updated

6 months ago
Depends on: 1511757
Reporter

Updated

6 months ago
No longer depends on: 1511757
See Also: → 1511757
Assignee

Updated

6 months ago
Duplicate of this bug: 1511757
Flags: qe-verify+

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.

Flags: needinfo?(emilio)
Reporter

Comment 12

4 months ago

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

Flags: needinfo?(emilio)

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

Assignee

Comment 14

4 months ago

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

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.