721076 - Prevent default on touch events should prevent long taps and double taps

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

If you enable multitouch in Android, you'll find out that tapping on things like links results in longtap far to often, and clicks not very often. I think there's a bug in (my) code that isn't sending ACTION_UP motion events to the Java handlers correctly, but I'm not sure exactly where or why at this point.

Comment 1

[Wesley Johnston (:wesj)]

I think this is also the root cause of issues with the current pan location jumping ahead sometimes. i.e. you lift you finger but the panzoomcontroller gets no action up. When you put your finger down again it thinks you have jumped to some new location.

Comment 2

[Wesley Johnston (:wesj)]

I think this may be fixed by the patch in bug 721079 or the one in bug 721484. I am still seeing LongTaps fire on elements even when preventDefault is called though. Bug 721597 deals with long tap, but the bug being seen is really a result of us firing mouse events. Changing summary to be a bit more specific.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

Aha! Two parts here.

Sometimes we are trying to dispatch an event where there are no changedTouches. This probably means Android is detecting some change on a property that isn't in the spec (not sure what...). We return early, but we don't set that status bit, leading to us occasionally releasing panning.

This ensures we always return the same status (after touch start and the first touch move have established it).

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

The second part is also stupidity on my part. We aren't setting up the initial value for allowDefaultActions early enough here, instead waiting for it to be set in a runnable. That means that the initial touchdown event is leaking through sometimes, and when the GestureListener gets nothing else, it eventually fires a longtap.

Comment 5

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

Comment on attachment 593312 [details] [diff] [review]
Patch 2/2

Review of attachment 593312 [details] [diff] [review]:
-----------------------------------------------------------------

Now that I'm re-reading this code to figure it out what the patch does, it seems much harder to follow. We should try to make it clearer at some point, although I'm not sure exactly yet how.

r+ on the patch, look ok.

Comment 6

[Wesley Johnston (:wesj)]

As I was leaving last night I realized that, I don't know that double tap zoom has any hope of working on touchevents enabled pages that DON'T call prevent default. At least, there's a scenario where the user taps down and up, doesn't call prevent default. We don't send setPreventPanning(false) back from Gecko because we only do that on touchmove events. The user then taps down again and we clear the queue.

I think maybe I should move the timer code up so that it starts on touch down, try to ensure that it runs longer than the double tap timeout, and don't clear the queue unless setPreventDefault(true) happens.... Although that may result in the queue never clearing on pages that do preventDefault... Have to think/play with it a bit more I guess.

(In reply to Wesley Johnston (:wesj) from comment #4)
> Created attachment 593312 [details] [diff] [review]
> Patch 2/2
> 
> The second part is also stupidity on my part. We aren't setting up the
> initial value for allowDefaultActions early enough here, instead waiting for
> it to be set in a runnable. That means that the initial touchdown event is
> leaking through sometimes, and when the GestureListener gets nothing else,
> it eventually fires a longtap.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 593311 [details] [diff] [review]
Patch 1/2

I hope there will be some Fennec specific tests for this all.

Comment 8

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c1a8889bac
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c027f9fc627

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 593312 [details] [diff] [review]
Patch 2/2

Regression caused by (bug #): Required for touch events
User impact if declined: No multitouch
Testing completed (on m-c, etc.): Landed on mc 2/1/12
Risk to taking this patch (and alternatives if risky): Low risk. Mobile only fixes to broken code.
String changes made by this patch: None.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 593311 [details] [diff] [review]
Patch 1/2

Regression caused by (bug #): Required for touch events
User impact if declined: No multitouch
Testing completed (on m-c, etc.): Landed on mc 2/1/12
Risk to taking this patch (and alternatives if risky): Low risk. Mobile only fixes to broken code.
String changes made by this patch: None.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 593311 [details] [diff] [review]
Patch 1/2

If/When we move this to Aurora, we would likely also want it on beta.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 593312 [details] [diff] [review]
Patch 2/2

If/When we move this to Aurora, we would likely also want it on beta.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 593311 [details] [diff] [review]
Patch 1/2

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8c027f9fc627
https://hg.mozilla.org/mozilla-central/rev/57c1a8889bac

Comment 15

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-beta/rev/861a56e415eb

Comment 16

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-beta/rev/973e5111299e

Comment 17

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-03-06)
20120306031101
http://hg.mozilla.org/mozilla-central/rev/7d0d1108a14e

--
Device: HTC Desire
OS: Android 2.2