Closed Bug 597159 Opened 9 years ago Closed 9 years ago

Double taps sometime cause single taps

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(fennec2.0b1+)

VERIFIED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: stechz, Assigned: stechz)

Details

Attachments

(2 files, 6 obsolete files)

We need to stop interpreting long taps as always a single tap, because sometimes our event loop has longish pauses between mouse events.

Removing the single tap isn't enough. We need to show tap highlights as early as possible so that tapping on links still feels responsive. I also have code that sends context menus over as soon as a mousedown occurs, so that when tap highlight should be shown, we are ready to show it.

Patches coming. The first batch is removing single tap detection code and some InputHandler cleanup.
Attached patch fix Util.Timeout bugs (obsolete) — Splinter Review
Attached patch DragData no longer needs owner (obsolete) — Splinter Review
Attached patch Simplify click timeouts (obsolete) — Splinter Review
Attachment #475974 - Flags: review?(mbrubeck)
Attachment #475975 - Flags: review?(mbrubeck)
Attachment #475976 - Flags: review?(mbrubeck)
Attachment #475974 - Flags: review?(mbrubeck) → review+
Attachment #475975 - Flags: review?(mbrubeck) → review+
Attachment #475974 - Attachment is obsolete: true
Attachment #475975 - Attachment is obsolete: true
Attachment #475976 - Attachment is obsolete: true
Attachment #475977 - Attachment is obsolete: true
Attachment #475978 - Attachment is obsolete: true
Attachment #475993 - Attachment is obsolete: true
Attachment #475976 - Flags: review?(mbrubeck)
Attachment #475973 - Flags: review?(mbrubeck)
Attachment #476126 - Flags: review?(mbrubeck)
tracking-fennec: --- → ?
Comment on attachment 475973 [details] [diff] [review]
imported patch noautosingleclick

You should get rid of the kDoubleClickThreshold constant too.
Attachment #475973 - Flags: review?(mbrubeck) → review+
Comment on attachment 476126 [details] [diff] [review]
Remove tap highlight delay and always display tap highlight for a little bit

>+  hide: function hide(guaranteeShowMsecs) {

aGuaranteeShowMsecs
(or maybe "aMinimumShowMsecs" and change the property name too?)

Either way, please add a one-line comment explaining what the parameter means.

>+      if (this._shownAt)
>+        setTimeout(function() { this._hide() }.bind(this),

setTimeout(this._hide.bind(this));

>+  handleEvent: function handleEvent(ev) {

aEvent

Can you also move this above the hide() function?  That would help me understand this code while reading through the file.

>   panBegin: function panBegin() {
>+    TapHighlightHelper.hide(200);

hide(0) would be better here - otherwise you can get a blue highlight that stays fixed while the content scrolls.

>         // We add a few milliseconds because of how the InputHandler wait before
>+        // dispatching a single click
>+        this._contextTimeout.once(700, function() {

Should we change this to 500, and remove the comment?

Clearing the review - I'd like to re-review after these comments and questions are addressed.
Attachment #476126 - Flags: review?(mbrubeck)
Comment on attachment 476126 [details] [diff] [review]
Remove tap highlight delay and always display tap highlight for a little bit

>+  hide: function hide(guaranteeShowMsecs) {

aGuaranteeShowMsecs
(or maybe "aMinimumShowMsecs" and change the property name too?)

>+        setTimeout(function() { this._hide() }.bind(this),

          setTimeout(this._hide.bind(this),

>+    else
>+      this._hide();

When the if has braces, the else should too, even if it's one line.  (At least
I think so... I haven't seen an actual mobile-browser style guide, so I'm just
recalling comments from past mfinkle reviews.)

>   panBegin: function panBegin() {
>+    TapHighlightHelper.hide(200);

hide(0) is better, otherwise you can have a blue highlight that stays fixed
while the content pans.

>         // We add a few milliseconds because of how the InputHandler wait before
>+        // dispatching a single click
>+        this._contextTimeout.once(700, function() {

I think we can change this to 500 and remove the comment.  (The comment talks
about the single click delay, but it was really the tap highlight delay that
made this necessary in my opinion.)
Attachment #476126 - Flags: review+
tracking-fennec: ? → 2.0b1+
Sorry for the mostly duplicate comment - having some crashes with Bugzilla in Minefield.  I thought my comment was lost, so I retyped it, forgot some things, and changed my mind about others.  :PIgnore comment 10, except for the part about the handleEvent function.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
verified FIXED on builds:

Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b6pre) Gecko/20100920 Namoroka/4.0b7pre Fennec/2.0b1pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100920 Namoroka/4.0b7pre Fennec/2.0b1pre
Status: RESOLVED → VERIFIED
bugspam
Assignee: nobody → ben
You need to log in before you can comment on or make changes to this bug.