1807073 - [Pull To Refresh][Bug] Pull to refresh should only trigger at the start of a pan gesture, not during a gesture which already resulted in scrolling

Status: VERIFIED FIXED

(Firefox for Android :: Browser Engine, defect, P2)

Description

[Christian Sadilek [:csadilek]]

From github: https://github.com/mozilla-mobile/fenix/issues/16577.

The current implementation of the pull-to-refresh gesture handling works differently than in Chrome, and causes unintended page refreshes because it triggers too easily.

Steps to reproduce

1. On any page, in Firefox Nightly for Android, scroll to the top.
2. Scroll down a bit, for example by 20% of the viewport height.
3. In one continuous panning motion, scroll up to the top and down again.

Expected behavior

You should be able to scroll up and down again, if you didn't start out at the very top.

Actual behavior

As soon as you hit the top edge, the scroll gesture is "swallowed" and turned into a pull to refresh gesture.

Pull to refresh should only be triggered when the user is intending to trigger it. Requiring the overscroll to be at the very start of the gesture is a good extra hurdle to weed out unintentional gestures.

Device information

• Android device: Moto G5
• Fenix version: Nightly 201112

┆Issue is synchronized with this Jira Task

Change performed by the Move to Bugzilla add-on.

Comment 1

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

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

For more information, please visit auto_nag documentation.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

This works for me using Nightly 111

• load https://planet.mozilla.org or https://en.wikipedia.org/wiki/Main_Page
• scroll down a small amount
• In one continuous motion scroll to the top of the page and then back down.

QA: Please test on a wider range of devices and mark the bug works for me if not able to reproduce.

Comment 3

[Lorand Janos]

I checked on the following devices where the behavior is as expected:

• Huawei P9 Lite (Android 7).
• Samsung Galaxy Tab S3 (Android 9).
• Redmi 9C NFC (Android 10).
• Oppo Find X5 (Android 12).
• Google Pixel 7 (Android 13).

Tested on the latest Nightly 111.0a1 (2023-01-31) build.

Comment 4

[Petru-Mugurel Lingurar [:petru]]

Attachment: edgeGlowOrPullToRefresh.mp4

I can reproduce the issue and seems like it is stemming from GeckoView - see the missing edge glow that GeckoView controls and does not show when the pull to refresh throbber shows. Checking the data that we sync on with GeckoView I see that in that case the InputResultDetail API sends us the same data as when the gesture should be enabled (same as for the expected scenario - dragging the wikipedia page down from the very top in one continuous gesture):

• inputResult == INPUT_HANDLING_UNKNOWN
• scrolLDirections == SCROLL_DIRECTIONS_NONE
• overscrollDirections == OVERSCROLL_DIRECTIONS_VERTICAL

That would mean that if we are to ignore INPUT_HANDLING_UNKNOWN here[1] then pull to refresh would not have the issue described in this ticket but it would also not work in the expected scenario. And the glow effect would also not be showing.

cc Botond for recommendations since we're in an active process of polishing the feature.

[1] 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 5

[Botond Ballo [:botond]]

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

That would mean that if we are to ignore INPUT_HANDLING_UNKNOWN here[1]

Petru, did you mean to include a code link here?

Comment 6

[Petru-Mugurel Lingurar [:petru]]

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

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

That would mean that if we are to ignore INPUT_HANDLING_UNKNOWN here[1]

Petru, did you mean to include a code link here?

Thank you, wanted to link to where we sync on the inputResultDetail - 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 7

[Hiroyuki Ikezoe (:hiro)]

I am pretty sure this is the same root cause of bug 1724755. See bug 1724755 comment 9.

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

• inputResult == INPUT_HANDLING_UNKNOWN

Gecko never returns this value. It was set in here, and will be used until Gecko returns the correct value. Please keep in mind, Gecko (GeckoView) returns InputResultDetail asynchronously.

Comment 8

[Botond Ballo [:botond]]

I can't reproduce this with the exact STR provided, but I think it's plausible that the issue discussed in bug 1724755 can cause these symptoms intermittently.

However, I've noticed another issue with slightly modified STR that seems independent of bug 1724755:

STR

1. Load en.wikipedia.org
2. Make sure the page is scrolled all the way to the top
3. In a single movement, scroll down and then back up

Actual results

When you reach the top of the page scrolling back up, pull-to-refresh is activated.

Expected results

Since the gesture started by scrolling down, it should not cause pull-to-refresh. I verified that this is what happens in Chrome.

This seems like another issue at the application layer unrelated to bug 1724755.

Comment 9

[Markus Stange [:mstange]]

This bug seemed like it should have been a blocker for shipping Pull-to-refresh in Fenix. Why was it shipped with this bug still present?

Comment 10

[killercontact1.7.4.0]

This includes pinch to zoom as well. Doing a pinch and lifting one finger to follow up with a scroll gesture using the remaining one on the touchscreen, this is sometimes recognised as pull to refresh. This happens on outlook.live.com (account required) in the inbox with desktop mode enabled.

Comment 12

[Botond Ballo [:botond]]

(In reply to killercontact1.7.4.0 from comment #10)

This includes pinch to zoom as well. Doing a pinch and lifting one finger to follow up with a scroll gesture using the remaining one on the touchscreen, this is sometimes recognised as pull to refresh. This happens on outlook.live.com (account required) in the inbox with desktop mode enabled.

Could I ask you to file a separate issue for this please? I think it may have a different underlying cause from this one (and even if it's the same, having a separate issue will make sure that we remember to re-test the scenario after this is fixed).

Comment 13

[killercontact1.7.4.0]

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

(In reply to killercontact1.7.4.0 from comment #10)

This includes pinch to zoom as well. Doing a pinch and lifting one finger to follow up with a scroll gesture using the remaining one on the touchscreen, this is sometimes recognised as pull to refresh. This happens on outlook.live.com (account required) in the inbox with desktop mode enabled.

Could I ask you to file a separate issue for this please? I think it may have a different underlying cause from this one (and even if it's the same, having a separate issue will make sure that we remember to re-test the scenario after this is fixed).

Dear, I will investigate as much as possible, possibly with an edited screencast, because it is difficult to post my private inbox on the bugzilla. Hope you understand. Will report back soon.

Comment 14

[Jonathan Almeida [:jonalmeida]]

Leaving priority empty for now if the APZ team are interested in working on this (then I'll leave it up to them to assign their preferred value).

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch to Fenix to fix the issue

So here is a patch to fix the issue Botond commented in comment 8, but there's no automated test in this patch since it looks like there's no way to write tests involving user's inputs such as the scenario in comment 8?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Jon, what's the right course to proceed this bug? Just sending a PR to the Fenix repo without tests is reasonable?

Comment 17

[Jonathan Almeida [:jonalmeida]]

Thanks for taking this on! We were looking to pick this up next but you got to it first. :)

Yes, I think we should definitely have some test coverage. We learnt that in writing the tests, it would find flaws in the implementation. Happy to help provide support if needed to do this.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch to Fenix to fix the issue

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test cases

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Jon, with the way you told me on #Fenix channel creating a new instance of PanZoomController.InputResultDetail gets compiled properly, but still it doesn't work as expected, the values returned by thenReturn are not used at all.

Attachment 9368349 [details] [diff] has two test cases I wrote, to make the returned values properly used, I had to add an override function of onTouchEventForDetailResult to NestedGeckoView class directly. I think I am doing something wrong, but I couldn't figure out any ways to make the tests work. What's the right way to make them work? Thanks!

Comment 21

[Jonathan Almeida [:jonalmeida]]

Will look into this tomorrow with Titouan when we sync. Thanks for the draft patch!

NI Titouan in case you want to take a peek at this earlier.

Comment 22

[Titouan Thibaud [:titouan]]

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

Jon, with the way you told me on #Fenix channel creating a new instance of PanZoomController.InputResultDetail gets compiled properly, but still it doesn't work as expected, the values returned by thenReturn are not used at all.

Attachment 9368349 [details] [diff] has two test cases I wrote, to make the returned values properly used, I had to add an override function of onTouchEventForDetailResult to NestedGeckoView class directly. I think I am doing something wrong, but I couldn't figure out any ways to make the tests work. What's the right way to make them work? Thanks!

Hey :hiro! Thanks a lot for your work on this bug!

From what I understand, your patch doesn't change the current behavior yet, right? 
This would explain why the test doesn't pass. I would expect the 3rd assertEquals to be assertEquals(2, viewParentInterceptCounter) and the 4th one to be assertEquals(3, viewParentInterceptCounter) (and the tests indeed pass with those changes), just because for now, as far as I know, we don't requestDisallowInterceptTouch when the gesture is a swipe down after a gesture that resulted in scrolling.

Does it make sense?

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Titouan Thibaud [:tthibaud] from comment #22)

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

Jon, with the way you told me on #Fenix channel creating a new instance of PanZoomController.InputResultDetail gets compiled properly, but still it doesn't work as expected, the values returned by thenReturn are not used at all.

Attachment 9368349 [details] [diff] has two test cases I wrote, to make the returned values properly used, I had to add an override function of onTouchEventForDetailResult to NestedGeckoView class directly. I think I am doing something wrong, but I couldn't figure out any ways to make the tests work. What's the right way to make them work? Thanks!

Hey :hiro! Thanks a lot for your work on this bug!

From what I understand, your patch doesn't change the current behavior yet, right? 
This would explain why the test doesn't pass. I would expect the 3rd assertEquals to be assertEquals(2, viewParentInterceptCounter) and the 4th one to be assertEquals(3, viewParentInterceptCounter) (and the tests indeed pass with those changes), just because for now, as far as I know, we don't requestDisallowInterceptTouch when the gesture is a swipe down after a gesture that resulted in scrolling.

Does it make sense?

I am sorry I didn't mention that the test is based upon attachment 9368348 [details] [diff] [review]. With that the assertEqualss should be passed if the thenReturn worked as expected.

You can see the issue I am seeing without these changes, changing the current requestDisallowInterceptTouchEvent doesn't pass touch events to parents until engineView responds test case with whenever().thenReturn() will have the same issue. Thanks!

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Okay, I figured out a way to make the test works properly, the way is to use spy for the NestedGeckoView. I've sent a PR; https://github.com/mozilla-mobile/firefox-android/pull/4870

Comment 25

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1807073 - Prevent pull-to-refresh after scrolling down (#5075)

Comment 26

[BMO Github Automation]

Authored by https://github.com/hiikezoe
https://github.com/mozilla-mobile/firefox-android/commit/f8d084c92345a69cdef64c33ae9e92122648c75b
[main] Bug 1807073 - Defer allowing touch event interception in NestedGeckoView.

Authored by https://github.com/hiikezoe
https://github.com/mozilla-mobile/firefox-android/commit/d2602c39ef5a2e22861b9687ea053f9f80ed6439
[main] Bug 1807073 - Tests for bug 1807073.

Authored by Titouan Thibaud
https://github.com/mozilla-mobile/firefox-android/commit/8b08fc8934089d0efc2bdb2832d8bd8058d116ed
[main] Bug 1807073 - Prevent pull-to-refresh after scrolling down

Comment 27

[Lorand Janos]

Verified as fixed on the latest Nightly build (124.0a1 from 2024-01-23).
Device used: Samsung Galaxy S23 Ultra (Android 14).
Marking the ticket as verified.

Comment 28

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1807073 - Prevent pull-to-refresh after scrolling down (backport #5075) (#5540)

Comment 29

[Titouan Thibaud [:titouan]]

Comment on attachment 9379247 [details] [review]
[mozilla-mobile/firefox-android] Bug 1807073 - Prevent pull-to-refresh after scrolling down (backport #5075) (#5540)

Beta/Release Uplift Approval Request

• User impact if declined: Pull to Refresh remains buggy, as this patch fixes the main issues this feature currently has
• 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:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It has been baked in Nightly for a 3 weeks
• String changes made/needed:
• Is Android affected?: Yes

Comment 30

[Dianna Smith [:diannaS]]

Comment on attachment 9379247 [details] [review]
[mozilla-mobile/firefox-android] Bug 1807073 - Prevent pull-to-refresh after scrolling down (backport #5075) (#5540)

Approved for Mobile 123.0rc1

Comment 31

[BMO Github Automation]

Authored by https://github.com/hiikezoe
https://github.com/mozilla-mobile/firefox-android/commit/d95531b4de3e5dad23fbd9641f2db861a1adf68b
[releases_v123] Bug 1807073 - Defer allowing touch event interception in NestedGeckoView.

Authored by https://github.com/hiikezoe
https://github.com/mozilla-mobile/firefox-android/commit/e6474daeedd59962772ae73278fd8c33c85499e2
[releases_v123] Bug 1807073 - Tests for bug 1807073.

Authored by Titouan Thibaud
https://github.com/mozilla-mobile/firefox-android/commit/62342a58893e4f816150518d7133ffa370cea90c
[releases_v123] Bug 1807073 - Prevent pull-to-refresh after scrolling down

Comment 32

[Adina Petridean QA [:apetridean]]

Verified as fixed on Firefox RC 123.0.
Device used: Samsung Galaxy S23 Ultra (Android 14) and OnePlus A3000 (Android 6).