1586496 - Second click is ignored if fast and in a different spot in Fenix

Status: RESOLVED FIXED

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

Description

[Jaume Delclòs Coll]

Attachment: ff.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

Given a mobile friendly site (<meta name="viewport" content="width=device-width, initial-scale=1"> or touch-action: manipulation;), when two taps happen in different parts of the screen in a small time frame, the second one is ignored and not treated as a click.

I have attached a minimal test html file (which is also uploaded at https://www.cosarara.me/jaume/ff.html ), where you can tap any of the two elements as fast as you want and clicks will always register (as long as you keep tapping the same spot), but if you alternate between the two elements, only the taps on one of them register.

You can also use the same element with different fingers to the same effect, proving this is not a matter of different elements but simply distance.

If you slow down leaving over 350ms between clicks, both will register.

Actual results:

touchstart and touchend events always fire, but the second click event does not.

Expected results:

Both clicks should fire, like it would happen if both were done on the same spot.

Ignoring the event makes firefox seem slow and unresponsive.

This works as expected on desktop firefox using a mouse, and also tapping on firefox focus (8.0.16) and chrome for android 77.0.
It fails the same way on Firefox Preview v2.0.0 package 12.0.0 gecko 7.0.

Comment 1

[Laurentiu Apahidean]

I have managed to reproduce your issue on Release 68.1.1 and Nightly 68.2a1 (2019-10-05) using a Samsung Galaxy S8+ (Android 8.0.0). I will set this issue as new.

Comment 2

[Laurentiu Apahidean]

I have also managed to reproduce the issue on Firefox Preview and submitted a separate issue: https://github.com/mozilla-mobile/fenix/issues/5844.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Asking the GV team to triage this as this may be platform related.

Comment 4

[Emily Toop (:fluffyemily)]

Randall says that is an APZ issue and should be moved to the graphics component.

Comment 5

[Chris Peterson [:cpeterson]]

Moving to APZ component

Comment 6

[Jaume Delclòs Coll]

Just to clarify, this is in both Fenix and Fennec (although maybe the fix will never reach the later).

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Looks like we unconditionally go into the one-touch-pinch code which doesn't do anything on this page (since it cannot be zoomed), and probably prevents us from firing the click.

Not sure what the best way to handle this is, maybe we need to check if zooming is possible and if not, just handle the second touch as a regular touch down without one-touch-pinch.

Comment 8

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

The priority flag is not set for this bug.
:botond, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Botond Ballo [:botond]]

I haven't had a chance to confirm the diagnosis or investigate further yet, but I plan to. Will park the bug with me in the meantime.

Comment 10

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (away until Feb-2020) from comment #7)

Looks like we unconditionally go into the one-touch-pinch code which doesn't do anything on this page (since it cannot be zoomed), and probably prevents us from firing the click.

I don't think this is the problem, because the linked code is in HandleInputTouchMove(), but the issue occurs even if only touch-star / touch-end events are involved.

Comment 11

[Botond Ballo [:botond]]

Rather, it appears the problem is that if a second tap is close enough in time to be a double-tap (i.e. we're still in the GESTURE_FIRST_SINGLE_TOUCH_UP state) but far in distance, we bail by setting the state to GESTURE_NONE, which prevents that second tap from triggering a gesture event such as a single-tap.

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1586496 - If two taps are close in time but far in distance, still allow the second one to start a gesture. r=tnikkel

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1586496 - Add a gtest. r=tnikkel

Depends on D51479

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cec59e15970
If two taps are close in time but far in distance, still allow the second one to start a gesture. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/8aa8ed80ea43
Add a gtest. r=tnikkel

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7cec59e15970
https://hg.mozilla.org/mozilla-central/rev/8aa8ed80ea43

Comment 16

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 17

[Jaume Delclòs Coll]

I have applied the patch on fennec esr68 and it works perfectly. Will it be officially backported there?

Comment 18

[Botond Ballo [:botond]]

(In reply to Jaume Delclòs Coll from comment #17)

I have applied the patch on fennec esr68 and it works perfectly.

Great, thanks for checking!

Will it be officially backported there?

I think the patch is small and low-risk enough to backport. I would like to give it a chance to bake on nightly for a week or two first.

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 9105919 [details]
Bug 1586496 - If two taps are close in time but far in distance, still allow the second one to start a gesture. r=tnikkel

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: To make the fix available to users of Fennec which is on the 68esr branch
• User impact if declined: A tap may be ignored if it's close in time to a previous tap (but far enough in distance not to be a double-tap)
• Fix Landed on Version: 72
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small, localize changed to APZ gesture handling code
• String or UUID changes made by this patch:

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9105919 [details]
Bug 1586496 - If two taps are close in time but far in distance, still allow the second one to start a gesture. r=tnikkel

OK for uplift, useful fix for Fennec.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/46bd3ef84ba8
https://hg.mozilla.org/releases/mozilla-esr68/rev/462b5dbfb4c9

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Had landed the test with a=test-only and there were bustages after the header conflict got 'fixed'. Backout:

https://hg.mozilla.org/releases/mozilla-esr68/rev/4e0eded29ce39f15d1d40463d15ba13ed9468dda

Push with bustages: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=46e80be3afbbc631575ff327a312bcc17222cffe

Comment 23

[Botond Ballo [:botond]]

It looks like the test would need to be rebased across the prefs refactor to apply cleanly. We could do that, but I think the incremental value of having the test run on the esr68 branch is low enough that we can just leave it backed out. Thanks!

Comment 24

[Laurentiu Apahidean]

I have tested the issue on 68.3.0 RC using a Google Pixel 3a (Android 9) and Google Pixel 3 XL (Android 9) and the issue no longer occurs.