1817330 - Hiding the toolbar and pull to refresh can result in two gestures to trigger PTR if the address bar remains hidden

Status: RESOLVED FIXED

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

Description

[Kevin Brosnan [Ex-Mozilla]]

Steps to reproduce

1. Visit a website ex https://mozilla.org
2. Scroll down (address bar hides)
3. before the fling finishes scroll up (address bar still hidden)
4. Pull to refresh (address bar shown)
5. Pull to refresh actually happens

Expected behavior

One gesture to activate pull to refresh

Actual behavior

Two gestures to activate pull to refresh

Device information

• Firefox version: Nightly 112.0a1
• Android device model: Galaxy Note 9
• Android OS version: 10

Any additional information?

Comment 1

[Petru-Mugurel Lingurar [:petru]]

In this scenario I think the toolbar also shows a bug: it should be expanded into view flinging up.
So in the scenario of:

• fling down
• continue immediately with fling up

What should happen is:

• fling down
  -> the toolbar is collapsed - hidden from view
• continue immediately with fling up
  -> the toolbar is expanded - shown into view
• a new scroll down gesture with the website now scrolled to the top
  -> the pull to refresh functionality starts

---

Looked into why this isn't happening right now and saw that:

• the toolbar isn't expanded on the fling up gesture because the InputResultDetail api from GeckoView doesn't allow this, it sending the following details (seen with a log here[1]):

    inputResult == INPUT_HANDLING_UNKNOWN (-1)
    scrollDirections == SCROLL_DIRECTIONS_NONE (0)
    overscrollDirections == OVERSCROLL_DIRECTIONS_NONE (0)

• and then with the toolbar still hidden the first scroll down does not trigger the pull to refresh functionality and only shows back the toolbar because the InputResultDetail api from GeckoView now sends the following details (seen with a log here[2]):

    inputResult == INPUT_HANDLED (1)
    scrollDirections == SCROLL_DIRECTIONS_TOP & SCROLL_DIRECTIONS_BOTTOM (5)
    overscrollDirections == OVERSCROLL_DIRECTIONS_VERTICAL & OVERSCROLL_DIRECTIONS_HORIZONTAL (3)

This seems to be then another issue to be fixed from APZ.

[1] https://searchfox.org/mozilla-mobile/rev/0f0559be2bac6d10b7cdfb06fff57f62c779353d/firefox-android/android-components/components/browser/toolbar/src/main/java/mozilla/components/browser/toolbar/behavior/BrowserToolbarBehavior.kt#215
[2] https://searchfox.org/mozilla-mobile/rev/4c640a0da38ce181ea89e71d20b90a7f881d65b9/firefox-android/android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/InputResultDetail.kt#258-261

Comment 2

[Chris Peterson [:cpeterson]]

Frank, this bug looks like another APZ issue with Firefox Android's pull-to-refresh gesture.

Comment 3

[Frank Griffith Jr]

ok.....will bring to our APZ meeting to discuss....I don't have an Android device handy to reproduce....

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I believe this is an issue in Fenix. See bug 1724755.

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

Looked into why this isn't happening right now and saw that:

• the toolbar isn't expanded on the fling up gesture because the InputResultDetail api from GeckoView doesn't allow this, it sending the following details (seen with a log here[1]):

    inputResult == INPUT_HANDLING_UNKNOWN (-1)
    scrollDirections == SCROLL_DIRECTIONS_NONE (0)
    overscrollDirections == OVERSCROLL_DIRECTIONS_NONE (0)

Gecko never sends the INPUT_HANDLING_UNKNOWN, it's an uninitialzied state of InputResultDetail in Fenix.

Comment 5

[Petru-Mugurel Lingurar [:petru]]

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

I believe this is an issue in Fenix. See bug 1724755.
Gecko never sends the INPUT_HANDLING_UNKNOWN, it's an uninitialzied state of InputResultDetail in Fenix.

I don't agree that this is an issue in Fenix.
Indeed I used our mapping of values above, so I'll refer to the values from PanZoomController.java now:

I tested on the today's featured article from wikipedia.com.
With logs added to onTouchEventForDetailResult[1] I see that with the page scrolled to the middle a normal scroll down to get to the top of the page will have APZ send the following details:

    inputResult == INPUT_RESULT_HANDLED (1)
    scrollDirections == SCROLLABLE_FLAG_TOP & SCROLLABLE_FLAG_BOTTOM (5)
    overscrollDirections == OVERSCROLL_FLAG_HORIZONTAL & OVERSCROLL_FLAG_VERTICAL (3)

 

But when I do a fast fling up and then down I see:

    inputResult == INPUT_RESULT_HANDLED (1)
    scrollDirections ==  SCROLLABLE_FLAG_BOTTOM (4)
    overscrollDirections == OVERSCROLL_FLAG_HORIZONTAL & OVERSCROLL_FLAG_VERTICAL (3)

folllowed by

    inputResult == INPUT_RESULT_IGNORED (3)
    scrollDirections ==  SCROLLABLE_FLAG_NONE (0)
    overscrollDirections == OVERSCROLL_FLAG_NONE (0)

So you are right that because of the uninitialized state of InputResultDetail Fenix will not update the toolbar as expected.
But this is only happening because of the data GeckoView sends (or does not send) so in my opinion this is a APZ -> GeckoView bug.
According to the documentation for INPUT_RESULT_IGNORED - Specifies that an input event was consumed by a PanZoomController internally and browsers should do nothing in response to the event[2]

 

Continuing from the above scenario (a fling up followed by a fling down to get the page scrolled to it's top and the toolbar not getting expanded) the next immediate gesture which we'd expect to trigger pull to refresh - a scroll down will - not get do that because if looking at the response from onTouchEventForDetailResult[1] we get:

    inputResult == INPUT_RESULT_HANDLED (1)
    scrollDirections == SCROLLABLE_FLAG_TOP & SCROLLABLE_FLAG_BOTTOM (5)
    overscrollDirections == OVERSCROLL_FLAG_HORIZONTAL & OVERSCROLL_FLAG_VERTICAL (3)

based on which we will just now expand the toolbar but not also start pull to refresh since GeckoView says that the page can still be scrolled to the top.

[1] https://searchfox.org/mozilla-mobile/rev/93c7fc093c6fd5abe635740b4043d8b041025535/firefox-android/android-components/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/NestedGeckoView.kt#122-132
[2] https://searchfox.org/mozilla-central/rev/75da1dd5d4b9b991f919a41594194eab93cdef62/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/PanZoomController.java#80-84

Comment 6

[Botond Ballo [:botond]]

Thanks for the analysis, Petru! I agree that there are underlying issues here on the APZ side.

The reason for the INPUT_RESULT_IGNORED when panning upward during an existing downward fling is this logic which causes the APZ status to be set to eConsumeNoDefault, which is then translated to INPUT_RESULT_IGNORED here.

The motivation behind this code is that if the browser is currently flinging at a speed above a threshold, a touch in that state should not be delivered to page content because the intent of the touch is likely to be to interrupt the fling rather than to interact with whatever content happens to be under the finger. I think we want to keep the "touch event is not delivered to page content" part of the behaviour, but the "touch event cannot trigger dynamic toolbar movement" part is an unintended side effect which we should disentangle.

I haven't evaluated the second scenario very deeply; if the first issue is fixed, this state ("page is scrolled to the top and toolbar is hidden") may not arise in the first place.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

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

I haven't evaluated the second scenario very deeply; if the first issue is fixed, this state ("page is scrolled to the top and toolbar is hidden") may not arise in the first place.

Agree.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1817330 - Inform a reasonable APZHandledResult to GeckoView even if the APZ's result is nsEventStatus_eConsumeNoDefault. r?botond

Note that for touch events there are two possibilities where
InputQueue::ReceiveInputEvent() returns nsEventStatus_eConsumeNoDefault, a) the
touch input block is in a state of fast fling or b) the touch input block is in
a state of internal slop. But in the case of b) APZ never informs
nsEventStatus_eConsumeNoDefault to Android widget (APZ will inform one of the
other nsEventStatuses once after the block was escaped from the slop state), so
we just need to care a) case here.

In the case of a) nsEventStatus_eConsumeNoDefault means the event doesn't need
to be sent to content, it doesn't mean any APZC didn't scroll by the event.

Also note that there's an automated test exercising the scenario a). It was
flaky before (bug 1687842), but now it appears to be stable.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e98c4aeacc4f
Inform a reasonable APZHandledResult to GeckoView even if the APZ's result is nsEventStatus_eConsumeNoDefault. r=botond,geckoview-reviewers,owlish

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e98c4aeacc4f

Comment 11

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

The patch landed in nightly and beta is affected.
:hiro, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9332311 [details]
Bug 1817330 - Inform a reasonable APZHandledResult to GeckoView even if the APZ's result is nsEventStatus_eConsumeNoDefault. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: pull-to-refresh is unexpectedly triggered
• 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: This change is Android specific, it doesn't affect any other platforms, and the change affects only pull-to-refresh feature.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed: none
• Is Android affected?: Yes

Comment 13

[Donal Meehan [:dmeehan]]

Comment on attachment 9332311 [details]
Bug 1817330 - Inform a reasonable APZHandledResult to GeckoView even if the APZ's result is nsEventStatus_eConsumeNoDefault. r?botond

Approved for Fenix/Focus 114.0b7.

Comment 14

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3436c899285c