1893306 - Unexpected horizontal scrolling at the end of a vertical pan - lifting the finger breaks the axis lock

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Attachment: screen recording

On Android, when panning to the bottom of a page that's scrollable on both axes, the page sometimes scrolls horizontally once the finger is lifted.

Steps to reproduce:

1. Go to https://phabricator.services.mozilla.com/D208427
2. Scroll to the bottom with a series of flings.

Sometimes the page ends up being scrolled horizontally once you reach the bottom, because the scroll gesture happened to be slightly diagonal.

This is unexpected - if we only scroll vertically while the fingers are on the screen, then we should continue to only scroll vertically once the fingers are no longer on the screen.

---

I think what happens is that, when we initiate the fling at the end of the pan, we don't respect the current axis lock, so we can initiate a horizontal fling even if the pan was locked vertically.

Comment 1

[Markus Stange [:mstange]]

Attachment: easier to reproduce screen recording

It's often easier to reproduce this in the other direction - horizontal pans starting vertical flings.

In this recording, I did a horizontal pan, and then started moving my fingers vertically, before I lifted them. The vertical motion is ignored while the fingers are on the screen, but then gets translated into a vertical fling once the fingers are lifted.

Comment 2

[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 3

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1893306 - Fling animations should respect the gesture axis lock. r=botond

Ensure that a fling animation following a touch scroll respects the axis
lock used durring the gesture.

Comment 4

[Frank Griffith Jr]

Are we waiting for additional changes or reviews on these patches? Just checking in. Thanks.

Comment 5

[Dan Robertson (:dlrobertson)]

Just got some extra info on the patch... I'll figure out next steps today.

Comment 6

[Dan Robertson (:dlrobertson)]

Attachment: Bug 1893306 - Do not hand off fling velocity to a parent APZC if a child APZC has scrolled. r=botond

Not all flings should allow for handoff. A overscroll fling should be
able to handoff residual velocity to a APZC earlier in the overscroll
handoff chain, but a fling that starts in an APZC that is not scrollable
in the given direction should not handoff.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98b15c826948
Do not hand off fling velocity to a parent APZC if a child APZC has scrolled. r=botond

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1893306 - Tweak name and description of PartialFlingHandoff gtest. r=dlrobertson

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages @ TestScrollHandoff.cpp

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

Push with failures

Failure log -> ERROR - gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:676: Unified_cpp_apz_test_gtest1.o] Error 1

Comment 10

[Botond Ballo [:botond]]

(In reply to Sandor Molnar[:smolnar] from comment #9)

Failure log -> ERROR - gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:676: Unified_cpp_apz_test_gtest1.o] Error 1

The error is:

> ERROR -  /builds/worker/checkouts/gecko/gfx/layers/apz/test/gtest/TestScrollHandoff.cpp:545:3: error: Unused "kungFuDeathGrip" 'RefPtr<TestAsyncPanZoomController>' objects constructed from temporary values are prohibited
>  INFO -    545 |   RefPtr<TestAsyncPanZoomController> parent = ApzcOf(layers[0]);
>  INFO -        |   ^
>  INFO -  /builds/worker/checkouts/gecko/gfx/layers/apz/test/gtest/TestScrollHandoff.cpp:545:47: note: Please switch all accesses to this value to go through 'parent', or explicitly pass 'parent' to `mozilla::Unused`
>  INFO -    545 |   RefPtr<TestAsyncPanZoomController> parent = ApzcOf(layers[0]);
>  INFO -        |                                               ^
>  INFO -  1 error generated.

It looks like it's complaining about the local variable parent being unused (though this is not clang's built-in unused-variable diagnostic, it seems to be some sort of custom static analysis maybe specific to RefPtr).

As it happens, the follow-up patch that I posted (https://phabricator.services.mozilla.com/D242206) re-introduces a use of this variable, so as long as we re-land the patch together with the follow-up as a single patch stack, we should be fine.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8f04b55c397
Do not hand off fling velocity to a parent APZC if a child APZC has scrolled. r=botond
https://hg.mozilla.org/integration/autoland/rev/81793dcc59df
Tweak name and description of PartialFlingHandoff gtest. r=dlrobertson

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/d8f04b55c397
https://hg.mozilla.org/mozilla-central/rev/81793dcc59df