Disable paint-skipping for scroll frames that contain a fixed element inside a CSS filter

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: mstange, Assigned: botond)

Tracking

(Blocks: 2 bugs, {regression})

48 Branch
mozilla59
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

(Whiteboard: [gfx-noted], URL)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
This could just be bug 1288210, but I'm filing it just to be sure.

STR:
 1. Go to http://codepen.io/Matori/full/BoaZeV/
 2. Scroll the pink slide away by scrolling down a little.
 3. Slowly scroll down some more and watch as the blue curtain image moves with the slide instead of staying fixed in place.
 4. Observe the same thing for the other slides.
 5. Scroll back up and notice jiggly images and white parts where there shouldn't be any white.
status-firefox48: --- → wontfix
status-firefox49: --- → wontfix
status-firefox50: --- → fix-optional
Keywords: regression
Priority: -- → P3
See Also: → bug 1288210
Whiteboard: [gfx-noted]
Version: Trunk → 48 Branch
status-firefox52: --- → affected
(Assignee)

Comment 1

2 years ago
The patches in bug 1288210 do not fix this.

There is another known issue with APZ handling of position:sticky: bug 1293125, which has to do with how Layout computes the sticky scroll ranges that are sent to APZ via the Layers API. I'd like to fix that next, and if that doesn't fix this bug either, I'll investigate this test case directly.
(Assignee)

Comment 2

2 years ago
The patch in bug 1293125 does not fix this, either.
(Assignee)

Comment 3

2 years ago
diagnosis
I investigated this test case, and with Markus' help arrived at a diagnosis.

The test case uses |filter: hue-rotate()| with a |background-attachment: fixed| image inside. The filter is an effect that's not supported in the compositor, so it creates an inactive layer tree that's flattened during main-thread painting. As a result, we don't create a compositor layer for the |background-attachment: fixed| image. Yet, that image is supposed to stay fixed with respect to the root scroll frame, which is async-scrolled. As a result, we get incorrect rendering.

The only proper solution that gives us both async-scrolling and correct rendering is supporting the effect in the compositor. This is possible, and has been discussed recently, but there is are no immediate plans for doing so.

Until such a proper solution is in place, we can employ certain mitigations:

 (1) Disable paint-skipping for affected scroll frames (roughly, "scroll frames
     that contain inactive layer trees that contain elements fixed w.r.t. the
     scroll frame").

     This would allow us to composite a correctly-rendered page as fast as
     we're able to paint, but intermediate composites will still be incorrect.

 (2) Disable APZ altogether for affected scroll frames. We could use the same
     mechanism we use to disable APZ for scroll-linked effects (which is
     currently behind a pref).

     In this case, the scroll frame wouldn't async-scroll, and every composite
     will be correct (but scrolling won't be as responsive).

It's fairly clear we want to do (1). (2) is less obvious - I'm open to input on this.

Implementing either (1) or (2) requires identifying the affected scroll frames, which will be significantly easier with bug 1298218 in place, so I'm going to wait for bug 1298218 to land before doing either.
Depends on: 1298218
(Assignee)

Comment 4

2 years ago
(The presence of position:sticky in the testcase turned out to be a red herring.)
Summary: Rendering glitches on this testcase, with position:sticky and background-attachment:fixed → Rendering glitches on this testcase, with filter:hue-rotate and background-attachment:fixed
(Assignee)

Comment 5

2 years ago
(In reply to Botond Ballo [:botond] from comment #3)
> Implementing either (1) or (2) requires identifying the affected scroll
> frames, which will be significantly easier with bug 1298218 in place, so I'm
> going to wait for bug 1298218 to land before doing either.

I should also write down the procedure Markus and I discussed for identifying affected scroll frames, so I don't forget:

  - During display list building, when entering a frame that has a filter or a mask
    (masks also create inactive layer trees and thus are similarly affected), take
    note of the current active scrolled root C.

  - When entering a frame that has an active scrolled root R that's an ancestor of C
    (such as anything that's fixed or sticky w.r.t. C or an ancestor of C), mark
    scroll frames from C up to but not including R as being affected.

The tracking of active scrolled roots is introduced in bug 1298218, hence the dependency.
status-firefox50: fix-optional → wontfix
status-firefox51: affected → fix-optional
status-firefox53: --- → affected
status-firefox51: fix-optional → wontfix
status-firefox52: affected → fix-optional
status-firefox52: fix-optional → wontfix
status-firefox53: affected → wontfix
status-firefox54: --- → affected
Kats, it looks like we've spent a couple of releases opting not to fix this.  Should we just mark this fix-optional for 54 and officially classify this bug as a backlog bug?
Flags: needinfo?(bugmail)
Sounds reasonable to me.
status-firefox54: affected → fix-optional
status-firefox55: --- → fix-optional
Flags: needinfo?(bugmail)

Updated

a year ago
See Also: → bug 1350663
(Assignee)

Updated

a year ago
Blocks: 1404218
(Assignee)

Comment 8

a year ago
I think we should start with approach (1) from comment 3. We can do approach (2) if we want in a follow-up bug.

Adjusting bug title accordingly.
Summary: Rendering glitches on this testcase, with filter:hue-rotate and background-attachment:fixed → Disable paint-skipping for scroll frames that contain a fixed element inside a CSS filter
Blocks: 1418923
(Assignee)

Updated

10 months ago
Assignee: nobody → botond
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

10 months ago
Implement approach (1) as outlined in comment 3 and comment 5.

I tested the example pages on this bug and the dependent bugs, and the change seems to be working as intended. Scrolling is still choppy due to painting lagging behind async scrolling, but at least we do not get "stuck" with incorrect rendering due to paint skipping; the correct thing is rendered much more quickly after scrolling.
(Reporter)

Comment 12

10 months ago
mozreview-review
Comment on attachment 8932238 [details]
Bug 1300864 - Define AutoCurrentActiveScrolledRootSetter::SetCurrentActiveScrolledRoot() out of line.

https://reviewboard.mozilla.org/r/203274/#review209490
Attachment #8932238 - Flags: review?(mstange) → review+
(Reporter)

Comment 13

10 months ago
mozreview-review
Comment on attachment 8932239 [details]
Bug 1300864 - Disable paint skipping for scroll frames that contain out-of-flow content inside a CSS filter.

https://reviewboard.mozilla.org/r/203276/#review209510

::: layout/painting/nsDisplayList.h:1139
(Diff revision 1)
> +  public:
> +    AutoFilterASRSetter(nsDisplayListBuilder* aBuilder,
> +                        const ActiveScrolledRoot* aFilterASR)
> +      : mBuilder(aBuilder), mOldValue(aBuilder->mFilterASR)
> +    {
> +      if (!aBuilder->mFilterASR && aFilterASR) {

Usually, nullptr is a valid value for an ASR - it refers to the root ASR. For example, position:fixed elements usually have an ASR of nullptr.

Here you're treating nullptr to mean "don't change the ASR". I don't think this is going to cause any problems, because if the filter scrolls with the root ASR (i.e. doesn't scroll), you can't enter any content inside it that scrolls even less. However, the way it's written might be confusing to the reader, because it makes it look like nullptr is not a valid value for an ASR, ever. Maybe add a comment?
Attachment #8932239 - Flags: review?(mstange) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

10 months ago
(In reply to Markus Stange [:mstange] from comment #13)
> Comment on attachment 8932239 [details]
> Bug 1300864 - Disable paint skipping for scroll frames that contain
> out-of-flow content inside a CSS filter.
> 
> https://reviewboard.mozilla.org/r/203276/#review209510
> 
> ::: layout/painting/nsDisplayList.h:1139
> (Diff revision 1)
> > +  public:
> > +    AutoFilterASRSetter(nsDisplayListBuilder* aBuilder,
> > +                        const ActiveScrolledRoot* aFilterASR)
> > +      : mBuilder(aBuilder), mOldValue(aBuilder->mFilterASR)
> > +    {
> > +      if (!aBuilder->mFilterASR && aFilterASR) {
> 
> Usually, nullptr is a valid value for an ASR - it refers to the root ASR.
> For example, position:fixed elements usually have an ASR of nullptr.
> 
> Here you're treating nullptr to mean "don't change the ASR". I don't think
> this is going to cause any problems, because if the filter scrolls with the
> root ASR (i.e. doesn't scroll), you can't enter any content inside it that
> scrolls even less. However, the way it's written might be confusing to the
> reader, because it makes it look like nullptr is not a valid value for an
> ASR, ever. Maybe add a comment?

I opted to instead pass in |usingFilter| to the AutoFilterASRSetter constructor as a more accurate signal of whether we should change the ASR; that seemed conceptually cleaner.

Comment 17

10 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d8e3ed0b5a8
Define AutoCurrentActiveScrolledRootSetter::SetCurrentActiveScrolledRoot() out of line. r=mstange
https://hg.mozilla.org/integration/autoland/rev/dd40ae15d5bc
Disable paint skipping for scroll frames that contain out-of-flow content inside a CSS filter. r=mstange

Comment 18

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8d8e3ed0b5a8
https://hg.mozilla.org/mozilla-central/rev/dd40ae15d5bc
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is this something you wanted to be considered for Beta uplift or can it ride the 59 train?
status-firefox54: fix-optional → wontfix
status-firefox55: fix-optional → wontfix
status-firefox57: --- → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → wontfix
Flags: needinfo?(botond)
(Assignee)

Comment 20

10 months ago
I think this is a good candidate for uplift. It's fairly low risk, and we're relatively early in the beta cycle.

I'll give it a few more days of bake time and then request uplift. Leaving needinfo on me until then.
(Assignee)

Comment 21

10 months ago
Comment on attachment 8932239 [details]
Bug 1300864 - Disable paint skipping for scroll frames that contain out-of-flow content inside a CSS filter.

Approval Request Comment
[Feature/Bug causing the regression]:
  Paint skipping (bug 1253860).

[User impact if declined]:
  On pages that contain out-of-flow content inside a
  CSS filter, scrolling can result in incorrect 
  rendering that can persist for an arbitrarily long
  period of time, until something (e.g. a click)
  forces a repaint.

[Is this code covered by automated tests?]:
  No; the absence of paint skipping is a tricky thing
  to test for in an automated test.

[Has the fix been verified in Nightly?]:
  Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
  I don't think that's necessary.

[List of other uplifts needed for the feature/fix]:
  No.

[Is the change risky?]:
  Low risk.

[Why is the change risky/not risky?]:
  Paint skipping is an optimization. Disabling paint
  skipping should have no adverse impact on correctness.

[String changes made/needed]:
  None.
Flags: needinfo?(botond)
Attachment #8932239 - Flags: approval-mozilla-beta?
(Assignee)

Comment 22

10 months ago
Comment on attachment 8932238 [details]
Bug 1300864 - Define AutoCurrentActiveScrolledRootSetter::SetCurrentActiveScrolledRoot() out of line.

See previous comment.
Attachment #8932238 - Flags: approval-mozilla-beta?

Comment 23

10 months ago
Hi :botond,
We are in late beta cycle and this issue has been there since FF48. Can we let it ride the train?
Flags: needinfo?(botond)
(Assignee)

Comment 24

10 months ago
Ok, let's have it ride the trains.
Flags: needinfo?(botond)
(Assignee)

Updated

10 months ago
Attachment #8932238 - Flags: approval-mozilla-beta?
(Assignee)

Updated

10 months ago
Attachment #8932239 - Flags: approval-mozilla-beta?
status-firefox58: affected → wontfix
(Assignee)

Comment 25

8 months ago
Created attachment 8944077 [details]
Testcase

Codepen seems to be experiencing some issues, so I prepared a pure HTML/CSS version of the testcase, attached.

(If you're wondering why I'm bothering to do this for a closed bug, it's because we'd still like to make this testcase and others like it work better, as described in bug 1404218 comment 12.)
You need to log in before you can comment on or make changes to this bug.