1894122 - "Dead zone" during scrolling when panning back and forth in a single gesture (sticky axis locking eats vertical movement in some cases)

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Attachment: Screen recording

Steps to reproduce:

1. Go to a page that's scrollable both vertically and horizontally, for example https://loglog.games/blog/leaving-rust-gamedev/ .
2. Scroll down into the middle of the page.
3. Do an "up 40%, down 80%" scroll gesture: With your finger starting in the middle of the screen, scroll up, and, without lifting your finger, scroll back down, and past where your finger started.
4. Optionally, with your finger still on the screen, keep scrolling back and forth.

Expected results:
Scrolling should behave normally.

Actual results:
Whenever your finger goes past the position where it started, there is a "hitch" in the scrolling - some of the vertical delta of your pan movement is absorbed and doesn't result in the page moving vertically.
The further your finger has drifted horizontally away from the X position where the finger started, the longer the "hitch" - the bigger the "dead zone".

What happens here is that sticky axis locking remembers where the pan gesture started. Whenever the vector between the touch down position and the current position has a bigger horizontal component than vertical component, no vertical scrolling happens. This creates triangular zones emanating from the touch start position where vertical scrolling is blocked.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:botond, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Botond Ballo [:botond]]

This is one of the bugs we're tracking in FFXP-2856.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

The code in question is here in AsyncPanZoomController::HandlePanningUpdate;

ParentLayerPoint vector =
    ToParentLayerCoordinates(aPanDistance, mStartTouch);

float angle = atan2f(vector.y, vector.x);  // range [-pi, pi]
angle = fabsf(angle);                      // range [0, pi]

Comment 4

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1894122: Improve axis locking for touch scrolls. r=botond,hiro

For long touch scrolls that change direction, using the start point of
the touch scroll as the origin to calculate the angle of the scroll is
problematic for determine the axis that should be locked. Maintain a
list of recent touch gesture points and calculate the gesture angle from
the most recent touch gesture points.

Comment 5

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1894122: Add a minimum size parameter to the RecentEventsBuffer. r=botond

Add a minimum size parameter to the RecentEventsBuffer, for the scenario
in which we would prefer data from stale events to that of the fallback.

Comment 6

[Markus Stange [:mstange]]

I've tested a local GVE build with this patch applied and can confirm that it solves the problem! It feels much much better than before. Thank you!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1355b1b3841c
Add a minimum size parameter to the RecentEventsBuffer. r=botond
https://hg.mozilla.org/integration/autoland/rev/a1bbca68e1f7
Improve axis locking for touch scrolls. r=botond

Comment 8

[Cristina Horotan [:chorotan]]

Backed out for causing GTest failures

Backout: https://hg.mozilla.org/integration/autoland/rev/5ea40820794cd8b6a1197e612015c77f7792e7b7

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=a1bbca68e1f743b8de5353e9d6093132a942db0f&selectedTaskRun=Ep8EJIgfTcuqx81w3X-GqA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=472778516&repo=autoland&lineNumber=5009

Comment 9

[Dan Robertson (:dlrobertson)]

Found a way to reproduce this try failure locally over the weekend.

Comment 10

[Botond Ballo [:botond]]

I see these patches were pushed to Try recently and I'm not seeing any failures that look related -- is this ready to land?

Comment 11

[Dan Robertson (:dlrobertson)]

Ah rats! I forgot to make the tests only run on android

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)

I see these patches were pushed to Try recently and I'm not seeing any failures that look related -- is this ready to land?

My bad, I did overlook that the desktop gtest failure is a failing APZ test and so likely caused by the patch.

(In reply to Dan Robertson (:dlrobertson) from comment #11)

Ah rats! I forgot to make the tests only run on android

It looks like the failing test is actually a desktop test that was somehow regressed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8d077687277
Add a minimum size parameter to the RecentEventsBuffer. r=botond
https://hg.mozilla.org/integration/autoland/rev/374bd6b07bbf
Improve axis locking for touch scrolls. r=botond

Comment 14

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/d8d077687277
https://hg.mozilla.org/mozilla-central/rev/374bd6b07bbf