1832880 - Pull-to-refresh activates despite onTouchMove.preventDefault()

Status: RESOLVED FIXED

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

Description

[Martin]

Attachment: screen-recording.webm

Steps to reproduce:

1. Go to https://touchmove-preventdefault.glitch.me/
2. Click the button "Enable preventDefault() on the touchmove event"
3. Try to pull down to refresh

Actual results:

The pull down to refresh gesture activated and the page refreshed.

Expected results:

The pull down to refresh gesture should not have activated.

The expected result occurs in Chrome for Android and Safari for iOS.

This bug impacts swiping down to close the mobile menu on https://mconverter.eu - causing the page to refresh.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I believe this is a symptom that pull-to-refresh machinery doesn't wait the prevenDefaulted response.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Wrong bug number. :/

Comment 3

[BugBot [:suhaib / :marco]]

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

For more information, please visit BugBot documentation.

Comment 4

[Lorand Janos]

I can confirm this issue is reproducible on the current Nightly build (118.0a1 from 2023-08-01).
Not sure if the fix for any of the previously filed pull to refresh issues would fix this one as well, making this a duplicate of one of them.
Devices used:

• Oppo Find X5 (Android 13).
• Google Pixel 7 (Android 14).

Comment 5

[Botond Ballo [:botond]]

There is something strange going on in this test case.

After the button is pressed (such that preventDefault() is enabled), for downward scrolls APZ correctly sends an InputResult of HANDLED_CONTENT, but for upwards scrolls it sends an InputResult of HANDLED.

Comment 6

[Botond Ballo [:botond]]

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

After the button is pressed (such that preventDefault() is enabled), for downward scrolls APZ correctly sends an InputResult of HANDLED_CONTENT, but for upwards scrolls it sends an InputResult of HANDLED.

Ah, what's happening here is that for upward scrolls (more specifically, every other upward scroll), GeckoView sends us a touch-cancel event immediately after the touch-start (just like in bug 1830805 and bug 1847305). The touch-cancel event cancels the touch gesture before we get a chance to dispatch the first touch-move event to the page, and thus the event listener which would call preventDefault() (which would cause the result to change to HANDLED_CONTENT) doesn't get a chance to run.

Comment 7

[Botond Ballo [:botond]]

For completeness, the mechanism by which we produce the HANDLED InputResult in this scenario is:

• The touch-cancel is dispatched to the content process
• APZEventState has a fallthrough from the touch-cancel case to the case where it would send a content response after the first touch-move. Since the touch-cancel itself was not prevented, isTouchPrevented == false, so we tell the compositor the input block can be processed. (The InputResult is then produced using the usual "delayed result" codepath.)

Comment 8

[Botond Ballo [:botond]]

While the root cause here (like in bug 1830805 and bug 1847305) is the fact that GeckoView is sending a touch-cancel in the first place when it shouldn't be, unlike in those bugs here we could work around the symptoms in APZ. I'm not sure how advisable that is, but I'll post a tentative patch for discussion.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1832880 - Treat a touch-cancel before the first touch-move as a preventDefault(). r=hiro

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Okay, I did double check that what Botond saw with adding a debug printf here in HandleMotionEvent on my Pixel 3.

Indeed, the event sequence I saw is; an ACTION_DOWN (= touch-start), one or two ACTION_MOVEs (= touch-move) and then an ACTION_CANCEL (= touch-cancel) events.

And I've confirmed that disabling pull-to-refresh DOES fix the weird sequence.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

From a document about Android SDK;

The onInterceptTouchEvent() method gives a parent the chance to see touch events before its children do. If you return true from onInterceptTouchEvent(), the child view that was previously handling touch events receives an ACTION_CANCEL

And as far as I can tell we return true from a onInterceptTouchEvent at this line.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

And the line ends up calling SwipeRefreshLayout::onInterceptTouchEvent and reaching to this line, so whether to return true or false depends on mIsBeingDragged value.

And the mIsBeingDragged is set to true in startDragging, and startDragging gets called in here during handling ACTION_MOVE.

So, I am not sure whether we need to call the super class' onInterceptTouchEvent or not, but at least we shouldn't use the returned value to avoid the unexpected ACTION_CANCELs.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I dug into a bit more detail.

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

From a document about Android SDK;

The onInterceptTouchEvent() method gives a parent the chance to see touch events before its children do. If you return true from onInterceptTouchEvent(), the child view that was previously handling touch events receives an ACTION_CANCEL

And as far as I can tell we return true from a onInterceptTouchEvent at this line.

The true case is caused by this canScrollToTop function. If canScrollToTop returns false, then we will see the touch-cancel event.

Comment 14

[Botond Ballo [:botond]]

Per discussion with Hiro, I moved the APZ patch to a separate bug, bug 1849407, so that this bug can track the Fenix issue.

Comment 15

[Phabricator Automation]

Comment on attachment 9348676 [details]
Bug 1832880 - Treat a touch-cancel before the first touch-move as a preventDefault(). r=hiro

Revision D186064 was moved to bug 1849407. Setting attachment 9348676 [details] to obsolete.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: A possible fix

I happened to realize that when Fenix received a new MotionEvent.ACTION_DOWN event, Fenix keeps using the old inputResultDetail, it looks clearly wrong to me.

With re-initializing inputResultDetail on every ACTION_DOWN, it seems to me that at least the test case in comment 0 disappears.

I haven't eagerly tested the change and I am unfamiliar with Android component staff, so I am not 100% sure whether there's any side effects or not. I believe this is a right step to do there.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: A slightly better fix than the previous one

I found there's an API named requestDisallowInterceptTouchEvent which prevents intercepting touch events in ancestor components (in our case it's SwipeRefresh) looks promising.

Attaching patch uses requestDisallowInterceptTouchEvent. I tried all "See Also" bugs with this patch, so far it works fine.

One thing I am still wondering is that after receiving a input result from Gecko whether we should invoke onInterceptTouchEvent explicitly to trigger pull-to-refresh in the case where Gecko didn't consumed the event.

Comment 18

[Jonathan Almeida [:jonalmeida]]

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

One thing I am still wondering is that after receiving a input result from Gecko whether we should invoke onInterceptTouchEvent explicitly to trigger pull-to-refresh in the case where Gecko didn't consumed the event.

Titouan and I were discussing this today: we were wondering what do you mean by "Gecko didn't consumed the event."? If it means that Gecko responds that the event is is unhandled, then we do not need to call onInterceptTouchEvent because we are handle those events correctly.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

If there's an active touchmove event listener Gecko responds the result after the event listener processed the event regardless whether the event was preventDefaulted or not. Then if the event was NOT preventDefaulted, and it was NOT used to scroll, the result is unhandled, so yes. But it's too late. The sequence is;

1. onInterceptTouchEvent with ACTION_DOWN
2. onTouchEvent with ACTION_DOWN
   2-1) requestDisallowInterceptTouchEvent(true) is called
3. onTouchEvent with ACTION_MOVE
   3-1) requestDisallowInterceptTouchEvent(false) is called

So, onInterceptTouchEvent with ACTION_MOVE is totally skipped. That's what I mean.

That being said, I'd hope this scenario is not a big problem practically because once after requestDisallowInterceptTouchEvent(false) was called, subsequent ACTION_MOVEs would be properly used in subsequent onInterceptTouchEvent calls.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Confirmed that this bug was fixed by bug 1847305.