1854113 - Pull-to-refresh triggered after pinch-to-zoom and swipe gesture

Status: RESOLVED FIXED

(Fenix :: General, defect)

Description

[killercontact1.7.4.0]

Attachment: bz-1807073-ptr-outlook.webm

Steps to reproduce:

As requested by :botond in https://bugzilla.mozilla.org/show_bug.cgi?id=1807073#c12

Perform pinch-to-zoom, lift one finger to try and swipe with remaining finger.

On outlook.live.com in desktop mode.

Fenix 119.0a1 2015974627

Actual results:

It triggers pull-to-refresh and we're not even on the top of the page.

Expected results:

Perform swipe direction downwards, panning upwards.

Comment 1

[Botond Ballo [:botond]]

Thanks for the report! I can reproduce this.

A minimal testcase that demonstrates the issue is https://theres-waldo.github.io/mozilla-testcases/primary-sub.html. It happens fairly reliably with the following STR:

1. Ensure the yellow scrollable element is scrolled to the top
2. Put down two fingers one after the other
3. Pinch-zoom in slightly
4. Release one finger
5. Move the remaining finger downwards

When the first finger goes down, we send an InputResult of UNHANDLED, due to bug 1815713 (a one-finger downward movement in that situation should trigger pull-to-refresh).

At step 5, the application then seems to act on that UNHANDLED, without regard to the fact that a second finger has gone down and back up in the intervening time.

I believe this needs to be fixed at the application layer (at the time of sending the UNHANDLED, APZ does not know that this will be a two-finger gesture, and it does not have any other levers for controlling the activation of pull-to-refresh).

It would make sense to me to make a touch gesture ineligible to perform pull-to-refresh as soon as the gesture involves a second finger, for the remainder of the gesture's lifetime (i.e. until all fingers are lifted).

Comment 2

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

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

For more information, please visit BugBot documentation.

Comment 3

[Botond Ballo [:botond]]

We discussed this in our APZ team meeting, and added it to FFXP-2563, which tracks the set of issues we'd like to fix before re-enabling the pull-to-refresh feature by default on the release channel.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch

As Botond commented in comment 1, this bug should/needs to be fixed in Fenix. And it turns out that the fix should be straight-forward. (That being said I was initially trying to fix it in NestedGeckoView and then realized there's a piece of the code preventing pull-to-refresh on multi touch events in VerticalSwipeRefreshLayout).

Attaching file is a way (I can think of) to fix this bug, but not including any automated tests.

Jon or Titouan, would either of you guys mind taking over the rest of the work? Thanks!

Comment 5

[Titouan Thibaud [:tthibaud]]

Thanks a lot for this patch! I and Jon tested and reviewed the code, we added the tests and put a PR.

We added a small change that fixes a bug in a very specific situation: if a dialog appears on top of Fenix (in case of an ANR for example), hadMultitouch wasn't reset and the next gesture would be considered as mutlitouch even if it's not.

The fix is changing the condition to reset hadMultitouch from MotionEvent.ACTION_CANCEL == event.action || MotionEvent.ACTION_UP == event.action into MotionEvent.ACTION_DOWN == event.action.

Would you mind taking a look at the PR and confirming that everything looks good to you?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thank you Titouan and Jon!

Yeah, it looks pretty reasonable to me.

Comment 7

[BMO Github Automation]

Authored by Titouan Thibaud
https://github.com/mozilla-mobile/firefox-android/commit/5c047023107599782a554723a13961fe32355a4f
[main] Bug 1854113 - Fix pull-to-refresh triggered after pinch-to-zoom and swipe gesture