1709334 - [Proton] Vertical scroll animation can get cut after the page is in horizontal/vertical overscroll animations

Status: VERIFIED FIXED

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

Description

[Timea Cernea [:tbabos][inactive]]

Affected Versions:
Nightly 90.0a1
Beta 89.0b7

Affected Platforms:
MacOS

Steps to reproduce:
(intermittent or needs multiple tries to reproduce)

1. Go to https://planet.mozilla.org/
2. Swipe up (for vertical overscroll)
3. Swipe right (for horizontal overscoll)
4. Swipe down multiple times

Expected Results:
The vertical scroll animation should be smooth and fluent.

Actual Results:
The vertical scroll animation is cut, making it hard to scroll on the page.
Recording: https://drive.google.com/file/d/1yOwp8dFM8edaOnDOvfdRhOsaWLykOMJm/view?usp=sharing

Notes:
Could be a leftover of Bug 1700215, reproducible on a build after that bug was fixed.

Comment 1

[Botond Ballo [:botond]]

Tentatively marking as release blocker as Timothy could trigger this relatively easily.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Just a quick update, I am still investigating this though. When the issue starts happening, the horizontal overscroll amount remains, it's very small, smaller than 1.0, say -0.320298 I saw. I am not yet sure how this situation happens.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Okay, so the problem is here in OverscrollAnimation::DoSample function. If the overscroll amount is pretty small where the value is considered as "finish" by the physics model (i.e. Axis::IsOverscrollAnimationRunning returns true), we skip calling Axis::SampleOverscrollAnimation() which will clobber the small overscrolled amount to zero. Then we repeatedly try to start an OverscrollAnimation while scrolling. We should check Axis::IsOverscrolled() instead.

There are a couple of call sites of IsOverscrollAnimationRunning, looks like in HandlePanMomentum we should also use IsOverscrolled, the ones in OverscrollAnimation::IsManaging[XY]Axis (which are used in OnPan) are probably OK as it is since when IsManagingXAxis returns false in the case of still overscrolling, it means it's still overscrolling but with very small amount, it's about to finish, thus setting pan displacements to zero will not have much impact, we will have a new overscroll animation in the next frame if the next pan moment is big enough to overscroll, or nothing will happen if the next pan momenum is not going to be a source of overscrolling.

Anyway, I will make a change for this fix as small as possible in this bug, so leave the rest of the call site as it is.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1709334 - Ensure calling SampleOverscrollAnimation for overscroll animations which are about to finish. r?botond

In cases where the overscroll amount is pretty small, the animation's
physics model considers it's finished, which exacly means it's about
to finish, so if we don't call SampleOverscrollAnimation in such cases,
the small amount of overscrolling value remains there, then we try to
create overscroll animations repeatedly even if the overscroll amount
is visually noticeablt.

Comment 5

[Botond Ballo [:botond]]

Marking as P1 as this is a release blocker for overscroll and we'll be looking to uplift it to 89.

Comment 6

[Botond Ballo [:botond]]

[Tracking Requested - why for this release]: Release blocker for the overscroll feature we are hoping to ship in 89.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd803536f7b7
Ensure calling SampleOverscrollAnimation for overscroll animations which are about to finish. r=botond

Comment 8

[Sandor Molnar[:smolnar]]

Backed out for causing Gtest failures in APZCOverscrollTester.FlingIntoOverscroll

Backout link: https://hg.mozilla.org/integration/autoland/rev/b626672b391b9899d93d29c50676dbed3e91cae2

Push with failures

Failure log

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a94094bf2bb9
Ensure calling SampleOverscrollAnimation for overscroll animations which are about to finish. r=botond

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a94094bf2bb9

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9220517 [details]
Bug 1709334 - Ensure calling SampleOverscrollAnimation for overscroll animations which are about to finish. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: [Required for MR1 / Proton] Swipe scrolling will be quite choppy on sites where the site can be scrollable both horizontally and vertically once after overscrolling on both directions has happened
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is small and the code path is only used for overscrolling and is well covered by some amount of automated tests (gtests).
• String changes made/needed: none

EDITED: I forgot adding "Required for MR1 / Proton".

Comment 12

[Timea Cernea [:tbabos][inactive]]

We verified the fix on the latest Nightly 90.0a1 (2021-05-07) (64-bit) on MacOS 10.14 and 11.0.1 with the following results:

• the initial issue is no longer reproducible
• After following the STR, the very first swipe-down gesture will end the scroll animation suddenly. It does not flow naturally at the end of the animation like the normal scroll would do.
  This is happening only for the first swipe down/up gesture after toggling the overscroll animations (vertical/horizontal). Any further scroll down or up will work as expected. Recording for a better view of this issue: https://drive.google.com/file/d/1o21NnMW5q6qwZlyn8s-y7PlA_v6_atT-/view?usp=sharing

Hey Hiroyuki, can you please also share your opinion about this fix? From a QA perspective, this fix can go as it is now given the initial issue which was way more visible and now is no longer reproducible. Further fixes for the minor inconvenience I just described and recorded could be handled in the future.

Please let me know what is the decision for this one and if we should go further with the Uplift process. Thanks!

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I think that's bug 1702949, which is a left over issue from bug 1700215.

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9220517 [details]
Bug 1709334 - Ensure calling SampleOverscrollAnimation for overscroll animations which are about to finish. r?botond

Flagged as a major bug to fix before shipping by QA, approved for 89 beta 11, thanks.

Comment 15

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/615b92b631ff

Comment 16

[Andrei Purice]

Thanks for the answer Hirouki! Marking this as Verified fixed on Nightly 90.0a1 (2021-05-09) (20210509213623) - tested on MacOS 10.15 and 11.2.3. This fix can go as it is, based on Comment 12 that seems to be Bug 1702949. Waiting for the fix to land in Beta 11 for further verification.

Comment 17

[Andrei Purice]

Verified fixed on Beta 89.0b11 (20210511190154) on MacOS 10.14 and 11.2.3.