Last Comment Bug 721076 - Prevent default on touch events should prevent long taps and double taps
: Prevent default on touch events should prevent long taps and double taps
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
Depends on:
Blocks: 603008
  Show dependency treegraph
 
Reported: 2012-01-25 09:07 PST by Wesley Johnston (:wesj)
Modified: 2012-03-07 02:46 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
verified
+


Attachments
Patch 1/2 (922 bytes, patch)
2012-01-31 21:13 PST, Wesley Johnston (:wesj)
bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Patch 2/2 (1.35 KB, patch)
2012-01-31 21:17 PST, Wesley Johnston (:wesj)
bugmail: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Wesley Johnston (:wesj) 2012-01-25 09:07:24 PST
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) 2012-01-25 09:11:11 PST
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) 2012-01-30 17:08:46 PST
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) 2012-01-31 21:13:43 PST
Created attachment 593311 [details] [diff] [review]
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) 2012-01-31 21:17:05 PST
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 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-01 07:16:06 PST
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) 2012-02-01 08:27:39 PST
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] (TPAC) 2012-02-01 08:31:53 PST
Comment on attachment 593311 [details] [diff] [review]
Patch 1/2

I hope there will be some Fennec specific tests for this all.
Comment 9 Wesley Johnston (:wesj) 2012-02-01 10:47:23 PST
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) 2012-02-01 10:47:55 PST
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) 2012-02-01 11:27:44 PST
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) 2012-02-01 11:28:01 PST
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] 2012-02-02 06:41:31 PST
Comment on attachment 593311 [details] [diff] [review]
Patch 1/2

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.
Comment 16 Wesley Johnston (:wesj) 2012-02-06 18:25:12 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/973e5111299e
Comment 17 Cristian Nicolae (:xti) 2012-03-07 02:46:58 PST
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

Note You need to log in before you can comment on or make changes to this bug.