1864554 - Smooth scroll animations incorrectly handle some sequences of Update and HandleScrollOffsetUpdate operations

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Botond Ballo [:botond]]

When an input event such as a mouse wheel triggers a smooth scroll animation, APZ supports two types of updates to the smooth scroll animation while it's running:

• Updating the destination of the smooth scroll animation (UpdateDelta or UpdateDestination). This happens when another input event is received while the animation is running.
• Applying a main thread scroll offset update to the smooth scroll animation (HandleScrollOffsetUpdate). This happens when script changes the scroll position using a scrolling API by scrollBy(), or inserting new content above the viewport or similar, while the smooth scroll animation is running.

I discovered some issues in the implementations of these operations which can result in incorrect behaviour when receiving a mixture of the above two types of updates.

This came up while investigating a failure of helper_relative_scroll_smoothness.html, which involves such a mixture of the two update types, in bug 1846935, though I'm not yet sure if these issues are responsible for the test failures.

Comment 1

[Botond Ballo [:botond]]

Adding bug 1759958 to "See Also", this is where I tried to fix the implementation of HandleScrollOffsetUpdate for MSD physics, but I don't think I got it quite right.

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1864554 - Add a test that a content shift followed by a second wheel event updates the destination of a wheel animation correctly. r=hiro

Comment 3

[Botond Ballo [:botond]]

The first posted patch is testing the behaviour of a HandleScrollOffsetUpdate followed by an UpdateDelta. This one is actually passes with MSD physics but fails with Bezier physics (so it's not the cause of the test failure in bug 1846935, but something we should fix anyways).

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1864554 - Add a test that a large content shift followed by a second wheel event does not result in a wheel animation overshooting its ultimate destination. r=hiro

Depends on D193711

Comment 5

[Botond Ballo [:botond]]

The second patch exposes a bug in the MSD physics. Still unsure if it's relevant to the test failure in bug 1846935.

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1864554 - Ensure that the destination passed to ScrollAnimationPhysics::Update() takes into account any previous content shift. r=hiro

It needs to work this way because GenericScrollAnimation can receive
an absolute destination in UpdateDestination(), and the caller will
pass in a destination which does take into account any previous
content shift.

(And the caller needs to work this way because e.g. it may adjust
the destination to nearby snap points, and we want snap points near
the actual final destination, not an intermediate value that will
have a content shift applied on top of it.)

This fixes APZCSmoothScrollTester.ContentShiftThenUpdateDeltaBezier.

Depends on D193717

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1864554 - Reimplement ScrollAnimationMSDPhysics::ApplyContentShift() to reflect the content shift in the physics models. r=hiro

In bug 1759958, we tried an implementation that tacks on the content shift
at the end, but this is not consistent with Update() being called with
a destination that takes into account the content shift.

This fixes APZCSmoothScrollTester.ContentShiftDoesNotCauseOvershootMsd.

Depends on D193978

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c5fd6bb8b6d
Add a test that a content shift followed by a second wheel event updates the destination of a wheel animation correctly. r=hiro
https://hg.mozilla.org/integration/autoland/rev/b11d7b2fdcc3
Add a test that a large content shift followed by a second wheel event does not result in a wheel animation overshooting its ultimate destination. r=hiro
https://hg.mozilla.org/integration/autoland/rev/27cd2e44192d
Ensure that the destination passed to ScrollAnimationPhysics::Update() takes into account any previous content shift. r=hiro
https://hg.mozilla.org/integration/autoland/rev/6b5c80f3fd76
Reimplement ScrollAnimationMSDPhysics::ApplyContentShift() to reflect the content shift in the physics models. r=hiro

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/1c5fd6bb8b6d
https://hg.mozilla.org/mozilla-central/rev/b11d7b2fdcc3
https://hg.mozilla.org/mozilla-central/rev/27cd2e44192d
https://hg.mozilla.org/mozilla-central/rev/6b5c80f3fd76