1785754 - Consider revising APZ's choice of APZHandledResult values to better suit pull-to-refresh

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

GeckoView uses information provided by APZ (which takes the form of an APZHandledResult value returned via APZInputBridge::ReceiveInputEvent(), or a "delayed" version thereof) to help determine whether a touch event should be allowed to perform certain actions such as moving the dynamic toolbar or pull-to-refresh.

While working on bug 1746336, I realized there are some open questions about what value should be returned in certain cases.

The values reflect whether the touch event has caused scrolling of the root scrollable element on the page, and if not, why not. Here are the main scenarios, and associated values:

• (A) Did scroll the root ==> HandledByRoot
• (B) Did not scroll the root because:
  • (B1) There was no room to scroll (e.g. page is already at the top for an upward scroll, or page is not scrollable at all) ==> Unhandled
  • (B2) A touch event listener consumed the event by calling preventDefault() on it ==> HandledByContent
  • (B3) A descendant scrollable element was scrolled instead ==> also HandledByContent

I believe the impact on dynamic toolbar movement and pull-to-refresh is as follows:

• HandledByRoot allows dynamic toolbar movement but not pull-to-refresh
• Unhandled allows pull-to-refresh but not dynamic toolbar movement
• HandledByContent allows neither

The open questions are:

1. What value should be returned if (B1) and (B2) both apply, i.e. a page is not scrollable and has a touch event listener consuming touch events? Bug 1746336 had the side effect of changing the value here from HandledByContent to Unhandled, thereby effectively opting such pages into pull-to-refresh. We should confirm if that's desirable.

   • Note that the choice here has performance implications as well. If, having detected that the page is not scrollable, we can just return Unhandled without caring about touch event listeners, APZ can provide the result to GeckoView "eagerly". If, in cases of a prevent-defaulting touch listener we need to return HandledByContent, APZ needs to wait for the event to actually be dispatched to web content and for the listener to run, and can only then provide the answer as part of a "delayed" result.

2. What value should be returned on pages that use touch-action: none on the region that was touched? APZ currently treats these as scenario (B1) ("no room to scroll", i.e. pull-to-refresh allowed), but one could argue that using touch-action: none is more akin to consuming the events via a listener (indeed, using touch-action: none combined with a passive event listener is an alternative to using a listener that always prevent-defaults that is considered a better practice because the page is expressing the fact that it consumes all touch events declaratively) and therefore should be treated as (B2) (i.e. pull-to-refresh not allowed).

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Thank you for considering this!
As a side note a few months ago I was talking with Agi and Amedyne about the future of these dynamic features and rearchitecting them so that they are entirely driven from GeckoView - they should react in response to the distance the page is scrolled / overscrolled and GeckoView could send this data to clients instead of clients (like Fenix) differentiating between gestures and calculating the distance on their own - duplicated what GeckoView/Gecko already does to then reconcile through InputResultDetail.
This was the general flow in Fennec where GeckoView would actually translate a screenshot of the toolbar when that needed to be animated and this seems to be the general approach used on Chrome also.
Opened a small RFC about that here - https://github.com/mozilla-mobile/android-components/pull/12198.

Comment 2

[Botond Ballo [:botond]]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #1)

As a side note a few months ago I was talking with Agi and Amedyne about the future of these dynamic features and rearchitecting them so that they are entirely driven from GeckoView - they should react in response to the distance the page is scrolled / overscrolled and GeckoView could send this data to clients instead of clients (like Fenix) differentiating between gestures and calculating the distance on their own - duplicated what GeckoView/Gecko already does to then reconcile through InputResultDetail.
This was the general flow in Fennec where GeckoView would actually translate a screenshot of the toolbar when that needed to be animated and this seems to be the general approach used on Chrome also.
Opened a small RFC about that here - https://github.com/mozilla-mobile/android-components/pull/12198.

Thanks; I read over the RFC again, and I think I understand parts of it (it sounds like InputResultDetail would need to be extended with some fields related to distance which APZ would populate?), but I'm not very familiar with the details of the Android and GeckoView APIs so I'm less sure about some of the details (e.g. to what extent this information would need to be provided synchronously vs. asynchronously).

At some point, I think it would be helpful to have a discussion about the RFC, with someone more familiar with the GeckoView layer also present.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

The main idea there is that the app - Fenix only needs to know about the distance the page is actually scrolled and would be great if GeckoView would inform about this.
InputResultDetail is a good way to be able to reconcile how GV scrolls the webpage and how Fenix infers the scrolling distance on it's own but I'd say ideally we shouldn't need that.
I understand this is a bigger discussion and a bigger refactoring, just wanted to bring it out as an idea.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #3)

The main idea there is that the app - Fenix only needs to know about the distance the page is actually scrolled and would be great if GeckoView would inform about this.

IIRC, I've commented somewhere (probably in a github issue), I believe the distance Fenix wants to know is not the actual scroll distance in the page. So for example, if the page gets scaled by 0.5 for whatever reasons, the scroll distance in the scaled page is different from the distance in non-scaled page. I've commented the distance in the screen unit from the last touch point to the current touch point can be calculated in GeckoView or A-C or Fenix.

Comment 5

[Botond Ballo [:botond]]

(By the way, if I'm not mistaken, the RFC discussed in comments 1-4 is largely orthogonal to the open questions in comment 0. It may be better to take additional discussion of the RFC to a new bug.)

Comment 6

[Botond Ballo [:botond]]

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

1. What value should be returned if (B1) and (B2) both apply, i.e. a page is not scrollable and has a touch event listener consuming touch events? Bug 1746336 had the side effect of changing the value here from HandledByContent to Unhandled, thereby effectively opting such pages into pull-to-refresh. We should confirm if that's desirable.

   • Note that the choice here has performance implications as well. If, having detected that the page is not scrollable, we can just return Unhandled without caring about touch event listeners, APZ can provide the result to GeckoView "eagerly". If, in cases of a prevent-defaulting touch listener we need to return HandledByContent, APZ needs to wait for the event to actually be dispatched to web content and for the listener to run, and can only then provide the answer as part of a "delayed" result.

2. What value should be returned on pages that use touch-action: none on the region that was touched? APZ currently treats these as scenario (B1) ("no room to scroll", i.e. pull-to-refresh allowed), but one could argue that using touch-action: none is more akin to consuming the events via a listener (indeed, using touch-action: none combined with a passive event listener is an alternative to using a listener that always prevent-defaults that is considered a better practice because the page is expressing the fact that it consumes all touch events declaratively) and therefore should be treated as (B2) (i.e. pull-to-refresh not allowed).

After thinking about this a bit, my opinion on these questions is:

• Touching a touch-action: none region should result in HandledByContent. Otherwise, if you have e.g. a drawing app implemented using touch-action: none combined with a passive touch event listener, every downward touch-drag would gratuitously trigger pull-to-refresh.
• preventDefault() and touch-action: none should take precedence over "no room to scroll", i.e. should result in HandledByContent and not Unhandled even if there is no room to scroll. (Bug 1746336 regressed this.) The motivation for this is similar to the previous bullet: an app which has a region that consumes touch events in one of these ways should not trigger pull-to-refresh over those regions, regardless of whether the enclosing page is scrollable.
  • The mentioned performance concern, which only applies to the preventDefault() case, can be addressed by encouraging page authors to prefer touch-action: none over non-passive event listeners. (The performance concern does not apply to touch-action: none -- we have touch-action information available in the compositor, so we can return HandledByContent eagerly in that case).

I also think that it's important to fix these scenarios before shipping pull-to-refresh to a wider audience, and therefore this bug should be a blocker for shipping pull-to-refresh on beta.

Other opinions / thoughts are welcome.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Yeah, it sounds reasonable. I guess to be precise for pull-to-refresh feature, checking touch-action: pan-y would be more reasonable, but I think it's rarely used?

Comment 8

[Botond Ballo [:botond]]

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

Yeah, it sounds reasonable. I guess to be precise for pull-to-refresh feature, checking touch-action: pan-y would be more reasonable, but I think it's rarely used?

Yep, good point! In the touch-action case what we really want to know is "is the page consuming touch movement in the vertical direction", which can be done with touch-action: none but also with a more targeted value like touch-action: pan-x.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1785754 - Report HANDLED_CONTENT on pages with a preventDefault()-ing event listener even if the page is not scrollable. r=hiro!,dlrobertson

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1785754 - Split the result of ArePointerEventsConsumable() into two flags. r=hiro!,dlrobertson

The function would combine "has room to scroll (or zoom)" and
"touch action allows scroll (or zoom)", but in future patches
APZ will behave differently in the two cases.

Depends on D164114

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1785754 - Compute an APZHandledResult based on the TargetConfirmationFlags and PointerEventsConsumableFlags in the eIgnore case too. r=hiro!,dlrobertson

This ensures we have an eager Unhandled result when pointer events are
not consumable.

Depends on D164581

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1785754 - Support a null target in SetStatusForTouchEvent(). r=hiro!,dlrobertson

Depends on D164582

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1785754 - Simplify branch structure in InputQueue::ReceiveTouchInput(). r=hiro!,dlrobertson

Depends on D164583

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1785754 - Simplify Android widget code for handling the eIgnore status. r=hiro!,dlrobertson

APZ now ensures that GetHandledResult() is Unhandled when appropriate,
so the widget code doesn't need to special-case eIgnore any more.

Depends on D164584

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1785754 - Return HandledByContent (eagerly) for touches prevented by touch-action. r=hiro!,dlrobertson

Note, changes to the delayed-result codepath are not required because
touched prevented by touch-action always get an eager HandledByContent
result.

Depends on D164585

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5f42ea8d4de
Report HANDLED_CONTENT on pages with a preventDefault()-ing event listener even if the page is not scrollable. r=geckoview-reviewers,hiro,dlrobertson,m_kato
https://hg.mozilla.org/integration/autoland/rev/224edcccae94
Split the result of ArePointerEventsConsumable() into two flags. r=dlrobertson,hiro
https://hg.mozilla.org/integration/autoland/rev/ea50d6573f66
Compute an APZHandledResult based on the TargetConfirmationFlags and PointerEventsConsumableFlags in the eIgnore case too. r=hiro
https://hg.mozilla.org/integration/autoland/rev/6609201006ba
Support a null target in SetStatusForTouchEvent(). r=hiro
https://hg.mozilla.org/integration/autoland/rev/0acb045d9a40
Simplify branch structure in InputQueue::ReceiveTouchInput(). r=hiro
https://hg.mozilla.org/integration/autoland/rev/b9d179f8f92b
Simplify Android widget code for handling the eIgnore status. r=dlrobertson,hiro,geckoview-reviewers,m_kato
https://hg.mozilla.org/integration/autoland/rev/e05ed4cedf9f
Return HandledByContent (eagerly) for touches prevented by touch-action. r=dlrobertson,hiro,geckoview-reviewers,m_kato

Comment 17

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/b5f42ea8d4de
https://hg.mozilla.org/mozilla-central/rev/224edcccae94
https://hg.mozilla.org/mozilla-central/rev/ea50d6573f66
https://hg.mozilla.org/mozilla-central/rev/6609201006ba
https://hg.mozilla.org/mozilla-central/rev/0acb045d9a40
https://hg.mozilla.org/mozilla-central/rev/b9d179f8f92b
https://hg.mozilla.org/mozilla-central/rev/e05ed4cedf9f