Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 13
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, firefox13 verified, fennec+)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.

Updated

6 years ago
Blocks: 603008
tracking-fennec: --- → +
(Assignee)

Comment 2

6 years ago
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.
Summary: ACTION_UP is not handled correctly in Java with touch events enabled → Prevent default on touch events should prevent long taps and double taps
(Assignee)

Comment 3

6 years ago
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).
Assignee: nobody → wjohnston
Attachment #593311 - Flags: review?(bugs)
(Assignee)

Comment 4

6 years ago
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.
Attachment #593312 - Flags: review?(bugmail.mozilla)
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.
Attachment #593312 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 6

6 years ago
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

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

I hope there will be some Fennec specific tests for this all.
Attachment #593311 - Flags: review?(bugs) → review+
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c1a8889bac
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c027f9fc627
(Assignee)

Comment 9

6 years ago
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.
Attachment #593312 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

6 years ago
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.
Attachment #593311 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 11

6 years ago
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.
Attachment #593311 - Flags: approval-mozilla-beta?
(Assignee)

Comment 12

6 years ago
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.
Attachment #593312 - Flags: approval-mozilla-beta?
Comment on attachment 593311 [details] [diff] [review]
Patch 1/2

[Triage Comment]
Mobile only - approved for Aurora 12 and Beta 11.
Attachment #593311 - Flags: approval-mozilla-beta?
Attachment #593311 - Flags: approval-mozilla-beta+
Attachment #593311 - Flags: approval-mozilla-aurora?
Attachment #593311 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Attachment #593312 - Flags: approval-mozilla-beta?
Attachment #593312 - Flags: approval-mozilla-beta+
Attachment #593312 - Flags: approval-mozilla-aurora?
Attachment #593312 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/8c027f9fc627
https://hg.mozilla.org/mozilla-central/rev/57c1a8889bac
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/releases/mozilla-beta/rev/861a56e415eb
status-firefox11: --- → fixed
status-firefox12: --- → fixed
status-firefox13: --- → fixed
status-firefox11: fixed → affected
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/973e5111299e
status-firefox11: affected → fixed
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
status-firefox13: fixed → verified
You need to log in before you can comment on or make changes to this bug.