1704065 - [Proton] Elastic overscroll animation gets stuck upon performing right-left swipe finger gestures for "Swipe between pages" use

Status: VERIFIED FIXED

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

Description

[Timea Cernea [:tbabos][inactive]]

Affected Versions:
Nightly 89.0a1
apz.overscroll.enabled true

Tested On:
MacOS 10.15

Steps to Reproduce:

1. Have the "Swipe between page" mouse gesture enabled for your magic mouse (one finger)
2. Go to any page where the over scrolling effect is visible enough (make sure you have only 1 site opened in that tab - no tab history)
3. Perform multiple swipe down gestures
4. While the above step is in progress, perform a fast-short right or left gesture (for swipe between pages - one finger for magic mouse, 2 fingers for trackpad)

Expected Results:
The overscroll effect should finish the animation (if there is no tab history available)

Actual Result:
The rubberbanding animation is stuck and is not reset until scrolling again on that site.
Reproducible with magic mouse and trackpad gestures for "Swipe between pages".

Please see attached recordings for reference: https://drive.google.com/file/d/1oMhx3kzL1dtAaj4Ij0UsJxbIANLVEPWK/view?usp=sharing
Not reproducible on Safari.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I am not sure whether I can reproduce the issue without magic mouse.

As far as I can tell with touchpad, swiping out left (or right) during an overscroll animation is running, will stop the animation for a while, but within a second (or less), the animation will restart again without any interactions so it's restored properly.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

What's going on there when a swipe gesture starts;

1. We create a new input block when we get PANGESUTRE_START for the swipe gesture
2. And set the result as nsEventStatus_eIgnore
3. And cancel the running overscroll animation
4. Then we start a SwipeTracker
5. After that the SwipeTracker returns nsEventStatus_eConsumeNoDefault so that we don't have a change to recreate an overscroll animation.

I wonder why SwipeTracker::ProcessEvent returns nsEventStatus_eConsumeNoDefault even if there is no navigation history. Will look into tomorrow.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

There are at least two distinct cases on vertical overscrolling interuppted by a swipe gesture

1. if there's no browser navigation history in the direction where the swipe gesture goes
2. if there's browser navigation history in the direction where the swipe gesture goes

In the case of 1) both Chrome and Safari do horizontal overscrolling in the direction
In the case of 2) Safari cancels the overscroll state immediately, whereas Chrome does hold the overscroll state and restores the state once after the swipe gesture did nothing (i.e. no navigation happened)
Interestingly the beheviours of 2) is opposed what those browser do in cases where horizontal overscrolling happens during vertical pan momentum, Safari restores the horizontal overscrolling state after the momentum finished, whereas Chrome restores it immediately.

Firefox does get stuck in both cases, and never overscrolls horizontally in 1).

I am going to defer the issue that we don't overscroll horizontally in 1) and focus on the stuck issue in this bug.
What I am currently thinking to fix the stuck issue is calling this CancelAnimationsForNewBlock call in InputQueue::ReceivePanGestureInput without specifying ExcludeOverscroll flag if we decided the pan gesture event will be used for a swipe gesture above the CancelAnimationsForNewBlock call.

Comment 4

[Timea Cernea [:tbabos][inactive]]

Tested on the latest Nightly and I can confirm that the animation remains stuck for the trackpad gestures as well. It will not restart by itself unless I scroll again. Not sure if you still needed this info or not, let me know if I can help with anything else.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

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

What's going on there when a swipe gesture starts;

1. We create a new input block when we get PANGESUTRE_START for the swipe gesture
2. And set the result as nsEventStatus_eIgnore
3. And cancel the running overscroll animation
4. Then we start a SwipeTracker
5. After that the SwipeTracker returns nsEventStatus_eConsumeNoDefault so that we don't have a change to recreate an overscroll animation.

There are other scenarios we start a SwipeTracker from here in nsBaseWidget::ProcessUntransformedAPZEvent and here in BrowserChild::DispatchWheelEvent.

Anyways, this issue is very relevant with other similar issues (bug 1707605, etc.) There are various senarios which cause overscrolling stuck;

a) trigger swipe navigation gestures and cancel it during overscrolling (this bug)
b) click or tap during overscrolling (bug 1704488, bug 1707605)
c) mouse cursor position gets into positions where wheel events are preventDefaulted() during overscrolling (bug 1707217)
d) mouse wheel scrolling upwards during overscrolling by upwards pan gestures (no bug)

All senarios call InputQueue::CancelAnimationsForNewBlock and the function ends up cancelling on-going overscroll animation and leave the overscrolled position.

Solutions I was thinking are;

1. Stop leaving overscrolled positions in such cases -> NG: The new input block might keep the overscrolled positions, e.g. the user keeps touching on touchpad without moving his fingers
2. Start an overscroll animation when we call SetState(NOTHING) -> NG: This is not going to work for a), we don't change the state in the case of a) (this is actually an issue we should fix though)

As for this particular swipe navigation case (a), ways I can think of are;

1. On a pan end event if any swipe navigation didn't happen, send the pan end event to APZ -> NG: This didn't work since the corresponding PanGestureBlock has been marked as "WasInterrupted" so we convert the pan end event to a new pan start event and we will never receive subsequent pan gesture events.
2. Similar to the above 1), but send a new pan-interrupted event just like what we did in bug 1703705 -> NG?: Looks a jumpy sudden movement
3. Stop the SwipeTracker as soon as the pan gesture movement is towards the direction where the page navigation cannot happen so that subsequent pan gesture events will be handled as a new input block in APZ -> OK?: Looks like this is what Safari does (and looks what Chrome does)

I'd lean toward 3).

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1704065 - Don't start navigation swipe gestures where there is no navigation history. r?spohl

Apparently we should check each command specified by
browser.gesture.swipe.left or browser.gesture.swipe.right, it will be fixed
in bug 1707118.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1704065 - Stop consuming pan gesture events in SwipeTracker if the swiping direction is not allowed to navigate. r?mstange,spohl,botond

Depends on D113635

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a39509fabf04
Don't start navigation swipe gestures where there is no navigation history. r=spohl
https://hg.mozilla.org/integration/autoland/rev/432e93434d79
Stop consuming pan gesture events in SwipeTracker if the swiping direction is not allowed to navigate. r=spohl

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a39509fabf04
https://hg.mozilla.org/mozilla-central/rev/432e93434d79

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9218884 [details]
Bug 1704065 - Stop consuming pan gesture events in SwipeTracker if the swiping direction is not allowed to navigate. r?mstange,spohl,botond

Beta/Release Uplift Approval Request

• User impact if declined: overscroll situation gets stuck when user tries to swipe navigation during overscrolling
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Do swipe navigation to a direction where there is no history. I.e. if it's not able to go back history, then swipe leftward (from left to right) , if it's not able to go forward history, swipe rightward.

Note that this patch doesn't fix cases where the user is able to go back/forward but cancels swipe navigation

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change is pretty small and an approach which most unlikely regresses something
• String changes made/needed: none

Comment 11

[Pascal Chevrel:pascalc]

Comment on attachment 9218884 [details]
Bug 1704065 - Stop consuming pan gesture events in SwipeTracker if the swiping direction is not allowed to navigate. r?mstange,spohl,botond

Low risk improvement to an 89 feature, uplift pproved for 89 beta 8, thanks.

Comment 12

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/90c48417c977
https://hg.mozilla.org/releases/mozilla-beta/rev/0bc43cc3da4a

Comment 13

[Timea Cernea [:tbabos][inactive]]

NI? myself for QA Uplift verification and regression testing.

Comment 14

[Timea Cernea [:tbabos][inactive]]

Can confirm this is indeed fixed in the latest Nightly 90.0a1 (2021-05-03) (64-bit). Tested on macOS 10.15.
The animation is no longer paused after the swipe for navigation event.
Can also confirm that this did not fix the case when the user cancels swipe navigation, as mentioned by Hiroyuki in Comment 10.

Waiting for the fix to land on Beta for further testing.

Comment 15

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest Beta 89.0b8. Tested on MacOS 10.15 during our preliminary testing round that included regression testing around swipe navigation usage.