1789930 - Allow scroll anchoring during APZ scrolling triggered by user

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

We skip scroll anchoring if there's any async scrolling is in progress. The check was added in bug 1584035 to fix unexpected scroll anchoring when a programmatic scroll operation is invoked. But if the async scrolling was triggered by user we should allow scroll anchoring. I believe this is one of the sources of bug 1779404.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

This is more complicated than I thought initially. So for example, if the user is trying to scroll with two fingers gestures, and if the user's fingers still keep touching, then we should NOT allow scroll anchoring. Yeah, we can tell the situation by looking mInScrollingGesture but we can't tell pan momentum phase as of now. A more fundamental question is whether we should allow scroll anchoring during pan momentum. It seems to me we should NOT allow scroll anchoring either. What about the case of async smooth animation triggered by wheel events? If we should NOT allow in such cases, the original code in question is probably correct. But I am pretty sure the code in question is one of the sources of the Facebook jumpy scrolling issue.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I am now 50% sure we need to do a kinda relative scroll update in the case of scroll anchoring.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

I am now 50% sure we need to do a kinda relative scroll update in the case of scroll anchoring.

Okay, great. We have done it actually (thanks to Ryan Hunt).

So what we need to do here is; skip applying scroll anchoring if there's any programmatic async scroll operation. In the case of programmatic scroll operation cases, the script itself should know the scroll amount thus the script can do scroll anchoring-ish code when they change the DOM structures, e.g. insert/delete elements or some such.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1789930 - Propagate |aTriggerByScript| argument properly. r?botond

The new test case added in this change is a variant test case for bug 1584035.
The test will be failed by the next change without this change.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1789930 - Allow scroll anchoring during async scrolling animation triggered by user is in progress. r?emilio!,botond!

Scroll anchoring updates the scroll positions with ScrollOrigin::Relative, thus
it's properly merged into the in-progress animation.

So what the new function named IsAnimationTriggeredByScriptInProgress does is
basically same as what ScrollAnimationState does but with an additional check
whether the animation in question was triggered by script or not.

Depends on D156931

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I'm confused. Bug 1779404 repros without scroll anchoring enabled, so it's unclear to me how would this fix it?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I'm confused. Bug 1779404 repros without scroll anchoring enabled, so it's unclear to me how would this fix it?

As far as I can tell the Facebook jumpy scrolling is caused by scroll position changes triggered by inserting/deleting elements inside the scroll container. So without scroll anchoring it happens. To fix the jump we need to apply scroll anchoring where we currently don't.

Comment 8

[Botond Ballo [:botond]]

Marking as S2 because we believe it's one of the reasons for bug 1779404 which is an S2.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cabc1ffe5247
Propagate |aTriggerByScript| argument properly. r=botond
https://hg.mozilla.org/integration/autoland/rev/b611f1f2f251
Allow scroll anchoring during async scrolling animation triggered by user is in progress. r=emilio,botond

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/cabc1ffe5247
https://hg.mozilla.org/mozilla-central/rev/b611f1f2f251