597159 - Double taps sometime cause single taps

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

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.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: imported patch noautosingleclick

Comment 2

[Benjamin Stover (:stechz)]

Attachment: fix Util.Timeout bugs

Comment 3

[Benjamin Stover (:stechz)]

Attachment: DragData no longer needs owner

Comment 4

[Benjamin Stover (:stechz)]

Attachment: Make cancel pending work correctly so that other modules can grab properly

Comment 5

[Benjamin Stover (:stechz)]

Attachment: Simplify click timeouts

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Use tap events in InputHandler instead of clicker

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Remove overlay timeout in content.js and ensure that tap highlight stays on the screen after tap

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Remove tap highlight delay and always display tap highlight for a little bit

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 475973 [details] [diff] [review]
imported patch noautosingleclick

You should get rid of the kDoubleClickThreshold constant too.

Comment 10

[Matt Brubeck (: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?)

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.

Comment 11

[Matt Brubeck (: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.)

Comment 12

[Matt Brubeck (:mbrubeck)]

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.

Comment 13

[Benjamin Stover (:stechz)]

Pushed
http://hg.mozilla.org/mobile-browser/rev/24032f1f041a
http://hg.mozilla.org/mobile-browser/rev/a4a39f1afbf8

Comment 14

[Aakash Desai [:aakashd]]

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

Comment 15

[Dão Gottwald [:dao]]

bugspam