1891094 - Pull to refresh is triggered when we scroll down an options table

Status: RESOLVED FIXED

(Fenix :: Browser Engine, defect, P2)

Description

[eclaudiu64]

Attachment: mobizen_20240411_204002.mp4

User Agent: Mozilla/5.0 (Android 11; Mobile; rv:125.0) Gecko/125.0 Firefox/125.0

Steps to reproduce:

Pull to refresh is triggered while we are opening a table where we can select an option. It happens at this address: https://github.com/issues and I selected "Sort" and the table is displayed and after scrolling.

Actual results:

Pull to refresh is triggered when scrolling the table.

Expected results:

Pull to refresh should not fire when scrolling a table.

Comment 1

[eclaudiu64]

Attachment: mobizen_20240411_230639.mp4

An example would be this, when I change the data of a video from an error on bugzilla, I scroll up with my finger on the area where it is on the brown color and after pulling down with my finger, Pull to refresh is triggered. Pull to refresh does not trigger in Chrome. Maybe it's unrelated like this error, but you should have open tables not trigger Pull to refresh.

Comment 2

[Chris Peterson [:cpeterson]]

@ Titouan, did you create a new meta bug for pull-to-refresh bugs? If so, we should link this bug to that one (instead of the old meta bug 1807071).

In bug 1807071 comment 12, you said:

I like the idea of a new Meta bug, I can create one.

Comment 3

[Titouan Thibaud [:titouan]]

Thanks for reporting this bug, it's very helpful. I'll link it to the Meta bug for pull-to-refresh bugs (Bug 1882722)

Comment 4

[Jonathan Almeida [:jonalmeida]]

Fixed linking.

Comment 5

[Chris Peterson [:cpeterson]]

Titouan, did you intend to remove both the [group4] and [fxdroid] whiteboard tags from this bug?

Comment 6

[Chris Peterson [:cpeterson]]

Setting UX Fundamentals bugs to priority P2 because we want to fix them this year.

Comment 7

[Titouan Thibaud [:titouan]]

Thanks for the heads up Chris! I don't know what happened, I'm putting them back.

Comment 8

[Chris Peterson [:cpeterson]]

Clearing Priority on "pull to refresh" bugs until we decide whether to disable the feature by default.

Comment 9

[Botond Ballo [:botond]]

I looked at this a bit, to try and see if the InputResult value provided by Gecko is the expected one.

What I'm seeing is, we're actually providing multiple InputResult values per touch gesture (and sometimes they contradict each other). It happens whenever the bug occurs, and also on some other occasions.

I'm having a hard time reasoning about whether the values are expected without understanding why we are getting multiple values, so I think that needs to be investigated first.

Comment 10

[Botond Ballo [:botond]]

It looks like the reason Gecko is providing multiple InputResult values per touch gesture is that GeckoView is asking for multiple ones.

GeckoView asks for an InputResult for every touch-start event, but it looks like it sometimes also asks for one or more subsequent touch-move events.

APZ does not expect to be asked for an InputResult for a touch-move event (though there is nothing enforcing this, so it will return something).

Comment 11

[Botond Ballo [:botond]]

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

GeckoView asks for an InputResult for every touch-start event, but it looks like it sometimes also asks for one or more subsequent touch-move events.

I think this may be a regression from bug 1807073.

Prior to that change, the onTouchEventForDetailResult API was only called for a MotionEvent.ACTION_DOWN event, but since that change, it can now sometimes be called for a MotionEvent.ACTION_MOVE event as well.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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

It looks like the reason Gecko is providing multiple InputResult values per touch gesture is that GeckoView is asking for multiple ones.

GeckoView asks for an InputResult for every touch-start event, but it looks like it sometimes also asks for one or more subsequent touch-move events.

APZ does not expect to be asked for an InputResult for a touch-move event (though there is nothing enforcing this, so it will return something).

It's kinda expected result from bug 1807073. In bug 1807073 we (at least I) expected that the second call of onTouchEventForDetailResult for the first MotionEvent.ACTION_MOVE doesn't interfere the delayed result of the first onTouchEventForDetailResult call (which is for the initial MotionEvent.ACTION_DOWN).

Comment 13

[Polly McEldowney [:pollymce]]

Attachment: Simple html form with long scrollable options dialog

just to note that I tried making a long options table in a simple html form, with pull-to-refresh enabled, and didn't see any issues with scrolling that list. So maybe this bug is specific to github.com/issues.

Comment 14

[eclaudiu64]

(In reply to Polly McEldowney [:pollymce] from comment #13)

Created attachment 9402226 [details]
Simple html form with long scrollable options dialog

just to note that I tried making a long options table in a simple html form, with pull-to-refresh enabled, and didn't see any issues with scrolling that list. So maybe this bug is specific to github.com/issues.

I mention that I also noticed this error when I wrote a comment with a very long text and after scrolling that written comment, from the table and the pull to refresh was triggered. In addition, there is a site, which you can directly change the size of the video on the screen, but this triggers pull to refresh, compared to Chrome. I'm saying that there are still similar errors on all sites and not just isolated on some sites.

Comment 15

[Titouan Thibaud [:titouan]]

Could you please share a link for this website where you can reproduce the issue with a very long text in a comment, and for the one where you can change the size of the video? 

Thanks a lot for your help on this investigation!

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Okay an odd behavior I noticed is that while the option table is showing, trying to scroll the table sometimes scrolls the root content. A reliable way I found to see this odd behavior is;

1. Open https://github.com/w3c/csswg-drafts/issues
2. Scroll down a bit
3. Tap "Sort"
4. Scroll down the table to the bottom edge
5. Try to scroll the table starting with touching "Most reactions"

Sometimes you will able to see the root content is scrolling. A caveat here is that this odd behavior is also observable on Chrome.

Anyways, I am sure that pull-to-refresh is triggered by this odd behavior since if the root content's scroll position it at the top and if trying to scroll on the table in fact scrolls the root content, it will trigger pull-to-refresh.

Comment 17

[Polly McEldowney [:pollymce]]

thanks eclaudiu64! As noted above, it would be great if you could share more info on the specific websites where you have experienced issues! It would be ideal if you could raise those as separate bugzilla entries and add 1882722 as a blocker. Then we can keep this bug focused on solving the github behaviour & investigate and fix your other observed issues separately.

Comment 18

[Botond Ballo [:botond]]

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

Okay an odd behavior I noticed is that while the option table is showing, trying to scroll the table sometimes scrolls the root content. A reliable way I found to see this odd behavior is;

1. Open https://github.com/w3c/csswg-drafts/issues
2. Scroll down a bit
3. Tap "Sort"
4. Scroll down the table to the bottom edge
5. Try to scroll the table starting with touching "Most reactions"

Sometimes you will able to see the root content is scrolling. A caveat here is that this odd behavior is also observable on Chrome.

Anyways, I am sure that pull-to-refresh is triggered by this odd behavior since if the root content's scroll position it at the top and if trying to scroll on the table in fact scrolls the root content, it will trigger pull-to-refresh.

It seems to me that the dialog is exhibiting scroll handoff behaviour:

1. Scroll downward over the dialog: the dialog contents scroll down
2. After the bottom of the dialog is reached, scroll downward further: now the page contents (behind the dialog) scroll down
3. Scroll upward over the dialog: the dialog contents scroll up
4. After the top of the dialog is reached, scroll upward further: now the page contents (behind the dialog) scroll up

Given this, I would say it's expected to get pull-to-refresh if both the dialog and the page are scrolled to the top.

However, I'm observing pull-to-refresh in the following scenario:

1. Start with the page scrolled to the top, and tap "Sort" to show the dialog
2. Scroll the dialog down to the bottom with a quick flick
3. Without waiting too long, scroll upwards over the dialog

In this scenario, step (3) intermittently triggers pull-to-refresh, which definitely seems like a bug (the dialog should scroll instead).

Comment 19

[Botond Ballo [:botond]]

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

I looked at this a bit, to try and see if the InputResult value provided by Gecko is the expected one.

What I'm seeing is, we're actually providing multiple InputResult values per touch gesture (and sometimes they contradict each other). It happens whenever the bug occurs, and also on some other occasions.

I'm having a hard time reasoning about whether the values are expected without understanding why we are getting multiple values, so I think that needs to be investigated first.

Based on some additional investigation, it seems to me that the multiple InputResults are not the cause of this bug. In the scenario where pull-to-refresh unexpectedly happens, the first InputResult is HANDLED, which does green-light the pull-to-refresh gesture, and is already wrong. The value of subsequent results is irrelevant to the bug.

That said, I think getting multiple results is still an issue (even if it's not the cause of this bug), and I filed bug 1897567 about it.

Comment 20

[Botond Ballo [:botond]]

I further confirmed that the dialog is scrolling natively (i.e. not simulating scrolling with touch event listeners or some such). I believe that tell us that producing an InputResult of HANDLED for the touchdown event of a touch gesture over the dialog is an APZ bug -- if you're hitting a nested scroller, and it's not scrolled to the top, the answer should always be HANDLED_CONTENT.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: A reduced test case the the issue is relavitly easy to reproducible

STR:

1. Scroll down to the bottom edge on the blue area (you can see "world" at the bottom edge)
2. Try to scroll up on the blue area

Comment 22

[Hiroyuki Ikezoe (:hiro)]

I think this has been fixed bug 1897567.