Markus Stange [:mstange] Reporter
	Description • 8 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.

	Botond Ballo [:botond] Assignee
	Comment 1 • 8 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.

	Botond Ballo [:botond] Assignee
	Comment 2 • 8 years ago

The patch in bug 1293125 does not fix this, either.

	Botond Ballo [:botond] Assignee
	Comment 3 • 8 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.

	Botond Ballo [:botond] Assignee
	Comment 4 • 8 years ago

(The presence of position:sticky in the testcase turned out to be a red herring.)

	Botond Ballo [:botond] Assignee
	Comment 5 • 8 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.

	Nathan Froyd [:froydnj]
	Comment 6 • 7 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 7 • 7 years ago

Sounds reasonable to me.

	Botond Ballo [:botond] Assignee
	Comment 8 • 7 years 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.

	Botond Ballo [:botond] Assignee
	Comment 11 • 7 years 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.

	Markus Stange [:mstange] Reporter
	Comment 12 • 7 years ago
mozreview-review

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

https://reviewboard.mozilla.org/r/203274/#review209490

	Markus Stange [:mstange] Reporter
	Comment 13 • 7 years 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?

	Botond Ballo [:botond] Assignee
	Comment 16 • 7 years 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.

	Pulsebot
	Comment 17 • 7 years 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

	Cristian Brindusan [:cbrindusan]
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8d8e3ed0b5a8
https://hg.mozilla.org/mozilla-central/rev/dd40ae15d5bc

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 7 years ago

Is this something you wanted to be considered for Beta uplift or can it ride the 59 train?

	Botond Ballo [:botond] Assignee
	Comment 20 • 7 years 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.

	Botond Ballo [:botond] Assignee
	Comment 21 • 7 years 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.

	Botond Ballo [:botond] Assignee
	Comment 22 • 7 years ago

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

See previous comment.

	Gerry Chang [:gchang]
	Comment 23 • 7 years ago

Hi :botond,
We are in late beta cycle and this issue has been there since FF48. Can we let it ride the train?

	Botond Ballo [:botond] Assignee
	Comment 24 • 7 years ago

Ok, let's have it ride the trains.

	Botond Ballo [:botond] Assignee
	Comment 25 • 6 years ago

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.)