1515774 - Notify APZ of nested scrolling

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Eitan Isaacson [:eeejay]]

Android has a setup for nested scrolling[1]. Via a CoordinatorLayout, descendant views negotiate top-down which one will consume a scroll action. In the case of Focus and android components, the toolbar and the GeckoView hand-off scrolling to each other. Currently, when scrolling down with a pan both the GV and the toolbar scroll, giving a weird accelerated effect for the first part of the pan until the toolbar goes away.

Ideally, there should only be one element scrolling at a time:
1. When the user starts panning down, the toolbar should scroll, and GeckoView should not (ie. it should remain static in relation the the receding toolbar)
2. When the toolbar is completely hidden, scrolling should be handed off to GeckoView.

This is consistent with how other native apps behave with a dynamic toolbar, including Chrome.

When I started fixing this in android components, I ran into two issues:
1. If we subtract the toolbar consumed y delta from the MotionEvent before dispatching it to GeckoView, APZ will trigger a long press because it will not register any relative movement from the down tap.
2. If the user continues a pan after the toolbar hides they will experience a brief hiccup in the scrolling because APZ needs to observe a minimal movement threshold to start a pan.

..so APZ needs to know when a touch is part of a nested scroll.

I also played around with preserving velocity from a fling when the toolbar is visible (eg. HandleDynamicToolbarMovement in Fennec), but I don't see any noticeable improvement in doing so, and it just adds complexity. If the participating scrolling sibling was higher than a toolbar there might be reason to revisit.

1. https://developer.android.com/reference/android/support/v4/view/NestedScrollingChild

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1515774 - Add nested scrolling flag to APZ and expose in GeckoView API. r?snorp!,kats!

Comment 2

[Eitan Isaacson [:eeejay]]

Here is the fixed NestedGeckoView implementation that uses this flag:

https://github.com/eeejay/android-components/commit/72614c29e5f5ee4c8202b4194af9367d89bdc3e4

As me personally, and I'll give you an apk :)

Comment 3

[Mark]

See also https://github.com/mozilla-mobile/focus-android/issues/2789 and possibly https://github.com/mozilla-mobile/focus-android/issues/304

Comment 4

[Eitan Isaacson [:eeejay]]

I abandoned the previous changeset (with a comment of why). I have a new WIP that tracks absolute touch coordinates. This should allow us to use APZ's established heuristics and not reinvent them badly.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1515774 - Add screen offset to APZ for better touch interpretation.

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 9036821 [details]
Bug 1515774 - Add screen offset to APZ for better touch interpretation.

I don't know how to do this in Phabricator.. just soliciting feedback before I clean this up more. AFAIK the two things I didn't resolve here are:

1. Updating the velocity with the screen offsets. This works, often. But if the nested scroll snaps back the APZ still starts a fling and it looks bad. I kind of think this is low priority because the velocity is usually set fine when the user flings past the nested scroll.
2. One finger pinch-zoom. It is just a bit chaotic with a dynamic toolbar, but not terrible. The GeckoView might need to be explicitly "pinned", that may require a bunch of messaging from APZ. Not a priority, imho.

Comment 7

[Botond Ballo [:botond]]

It would help understand the patch better if there was an explanation somewhere (here in the bug, in the commit message, in a code comment...) of what the offset being added represents :)

Comment 8

[Eitan Isaacson [:eeejay]]

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

It would help understand the patch better if there was an explanation somewhere (here in the bug, in the commit message, in a code comment...) of what the offset being added represents :)

The offset represents the top-left screen coordinate of the GeckoView. If its parent is being actively scrolled it changes on each touch move (for example, with a disappearing toolbar, the y value will change.)

When we calculate distances, we need to factor those offsets in. We can't store the offset in the APZ's transform matrix because it would apply the same offset to all points uniformly and won't account for the position change.

Does that make sense? After sleeping on it, I think I am doing AsyncPanZoomController::TouchEventOffset wrong, but I get dizzy just thinking about all these coordinate spaces :)

Comment 9

[Eitan Isaacson [:eeejay]]

The basic goal of the patch is to calculate pan distances and gesture detection based on the physical finger interaction on the screen, and not have it be relative to the GeckoView (which could be moving under the user's finger).

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 9036821 [details]
Bug 1515774 - Add screen offset to APZ for better touch interpretation.

(I wrote the feedback in Phabricator)

Comment 11

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1515774 - Use Screen pixels for gesture detection. r=botond

We currently use ParentLayer pixels in GestureEventListener, it should
be Screen pixels because we care about physical distances and
thresholds, not layer-relative ones.

Comment 12

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1515774 - Introduce mScreenOffset for pinch and multitouch events. r=botond

Depends on D17042

Comment 13

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1515774 - Pass current GeckoView position with touch events to APZC. r=snorp

Depends on D17043

Comment 14

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1515774 - Store window's screen offset in APZ and use it for gesture interpretation. r=botond

In Android the embedded GeckoView can be in a scrolling parent, and move
along with the user's finger. In order to intepret touch movements for
gestures, we need to account for the device position of each touch.

Depends on D17044

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e17cc234256
Use Screen pixels for gesture detection. r=botond
https://hg.mozilla.org/integration/autoland/rev/69c8787c67d6
Introduce mScreenOffset for pinch and multitouch events. r=botond
https://hg.mozilla.org/integration/autoland/rev/3d97f1e46c1a
Pass current GeckoView position with touch events to APZC. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/45c6f07160d9
Store window's screen offset in APZ and use it for gesture interpretation. r=botond

Comment 16

[Cristian Brindusan [:cbrindusan]]

Backed out 4 changesets (Bug 1515774) for mochitest failures at test_group_touchevents-3.html and test_touch_action.html.

Backout: https://hg.mozilla.org/integration/autoland/rev/031ad7f5577472b87d0172d54e9dcc40dbb4a842

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=45c6f07160d9c0b7a472c68ff63d4aaa9bd00304&selectedJob=224866489

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=224866489&repo=autoland&lineNumber=4969
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=224851433&repo=autoland&lineNumber=3060

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6d635be8ffc
Use Screen pixels for gesture detection. r=botond
https://hg.mozilla.org/integration/autoland/rev/48a4ac1ba700
Introduce mScreenOffset for pinch and multitouch events. r=botond
https://hg.mozilla.org/integration/autoland/rev/2e07c6788179
Pass current GeckoView position with touch events to APZC. r=geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/422db058e1cf
Store window's screen offset in APZ and use it for gesture interpretation. r=botond

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f6d635be8ffc
https://hg.mozilla.org/mozilla-central/rev/48a4ac1ba700
https://hg.mozilla.org/mozilla-central/rev/2e07c6788179
https://hg.mozilla.org/mozilla-central/rev/422db058e1cf