1703705 - Overscroll gets stuck by pressing Ctrl (Command) during pan (or pan momentum maybe)

Status: VERIFIED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

This is the item 1 in bug 1703645 comment 1 Botond found.

STR;

1. Overscroll by pan
2. Keep your fingers on touchpad and press Ctrl (or Command) key
3. Keep pressing Ctrl key and leave your fingers from the touchpad

Comment 1

[Hiroyuki Ikezoe (:hiro)]

A problem is here in APZCTreeManager::ReceiveInputEvent, when we receive a PanEnd event during pressing Ctrl, WillHandleInput call returns Nothing() since with the Ctrl modifier the WeelPrefs::Action is ACTION_ZOOM, thus it's not one of actions we need to handle in APZC.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I also noticed that overscrolling gets stuck on double tapping by two fingers during overscrolling;

A STR is;

1. Overscroll by panning (keep your fingers on the touchpad) near the edge of the container
2. Leave your fingers and do double tap quickly in the overscroll gutter

We receive a PANGESTURE_CANCELLED event in this case, I think we should call StartOverscrollAnimation in OnPanCancelled too.

Also to fix the original issue in comment 0, I suppose we should send a fake PANGESTURE_CANCELLED event when we fall into this if branch on a PANGESTURE_END event.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

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

I also noticed that overscrolling gets stuck on double tapping by two fingers during overscrolling;

A STR is;

1. Overscroll by panning (keep your fingers on the touchpad) near the edge of the container
2. Leave your fingers and do double tap quickly in the overscroll gutter

We receive a PANGESTURE_CANCELLED event in this case, I think we should call StartOverscrollAnimation in OnPanCancelled too.

This is wrong. Actually we receive a PANGESTURE_CANCELLED event, but we also receive PANGESTURE_END event before the PANGESTURE_CANCELLED event. What I did actually was clicking the content. With the clicking event, we call CancelAnimationsForNewBlock, then it ends up calling CancelAnimation with ExcludeOverscroll flag, thus the overscroll state gets stuck there. I don't see any reasons we need to keep overscroll state on mouse events because overscroll will never happen with mouse events as far as I know.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP: Bug 1703705 - Don't keep overscrolled state when receiving mouse events.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Just pushed a patch to fix the case in comment 3.

I am still thinking how to fix the original case in comment 0. Two approaches I am thinking;

1. Call AsyncPanZoomController::ClearOverscroll directly from APZCTreeManager here if panInput.mHandledByAPZ is Nothing and the pan event is PAN_END and if we've already confirmed the target APZC
2. Add a new PanGestureType PANGESTURE_INTERRUPTED (or some such) and call ReceiveInputEvent with the new type and handle it in the target APZC to clear overscroll state (or start an OverscrollAnimation)

Comment 6

[Botond Ballo [:botond]]

Per discussion with Martin, this is a release blocker but not necessarily a nightly blocker.

Comment 7

[Botond Ballo [:botond]]

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

This is wrong. Actually we receive a PANGESTURE_CANCELLED event, but we also receive PANGESTURE_END event before the PANGESTURE_CANCELLED event. What I did actually was clicking the content. With the clicking event, we call CancelAnimationsForNewBlock, then it ends up calling CancelAnimation with ExcludeOverscroll flag, thus the overscroll state gets stuck there. I don't see any reasons we need to keep overscroll state on mouse events because overscroll will never happen with mouse events as far as I know.

One use case to keep in mind is the "click link while overscrolled" use case:

• Position the cursor slightly below a link
• Without moving the cursor, overscroll until the until transform moves the link to be under the cursor
• Click the link

We should avoid breaking this case.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thank you Botond, I am going to focus on the issue with Ctrl modifier case in this bug. I will defer the case that pan gestures are intrrupted by other events which will be handled by APZ to bug 1704488.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1703705 - Drop APZCTreeManager argument from the ctor of ScopedLayerTreeRegistration. r?botond

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1703705 - Add another variant ctor of ScopedLayerTreeRegistration for WebRender. r?botond

Depends on D112309

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1703705 - Add PanInterruped type pan gesture and send it to restore overscrolled state, etc. r?botond

The gtest in this change unfortunately runs only with WebRender because the test
needs to be run through APZCTreeManager's hit testing code path which requires
either WebRender scroll data structures or layer structures. Though we already
have the machinery to create layer structures in gtests, the WebRender backend
is more valuable in these days, this test would be a small first step to provide
a generic way to work with both the WebRender/Layers backends.

Depends on D112310

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de93bec5a450
Drop APZCTreeManager argument from the ctor of ScopedLayerTreeRegistration. r=botond
https://hg.mozilla.org/integration/autoland/rev/e0ddabdbfd9c
Add another variant ctor of ScopedLayerTreeRegistration for WebRender. r=botond
https://hg.mozilla.org/integration/autoland/rev/c2e446814a27
Add PanInterruped type pan gesture and send it to restore overscrolled state, etc. r=botond

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/de93bec5a450
https://hg.mozilla.org/mozilla-central/rev/e0ddabdbfd9c
https://hg.mozilla.org/mozilla-central/rev/c2e446814a27

Comment 14

[Andrei Purice]

Verified fixed on the latest version of Nightly 90.0a1 (2021-04-21) (20210421095627) on MacOS 11.2.3 (MacBook Pro).

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9216305 [details]
Bug 1703705 - Add PanInterruped type pan gesture and send it to restore overscrolled state, etc. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: [Required for MR1 / Proton] overscroll state gets stuck
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: It's already done
• 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 itself is relatively straight forward and will unlikely cause any side effects. Also note that relevant patches are only for testing purpose, not for the fix itself.
• String changes made/needed: none

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9216305 [details]
Bug 1703705 - Add PanInterruped type pan gesture and send it to restore overscrolled state, etc. r?botond

Approved for 89 beta 3, thanks.

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d805f61c0822
https://hg.mozilla.org/releases/mozilla-beta/rev/027c7c0d2604
https://hg.mozilla.org/releases/mozilla-beta/rev/b3c7781c114b

Comment 18

[Andrei Purice]

Verified fixed on Beta 98.0b3 (20210422190146) on MacOS 11.2.3.