Closed Bug 547722 Opened 14 years ago Closed 14 years ago

Get rid of the 400ms waiting phase if we're sure that the click is not a part of a double click

Categories

(Firefox for Android Graveyard :: General, defect)

Fennec 1.1
x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
Each time a click happen on the system we're waiting 400ms to see if it is part of a double click.

If the pressure is long enough we're sure that the click is not part of a double click.

I think the patch is a nice improvement.
Attachment #428219 - Flags: review?(mark.finkle)
Attachment #428219 - Flags: review?(webapps)
Comment on attachment 428219 [details] [diff] [review]
Patch


>+// threshold in ms to detect if the click is possibly a dblClick
>+const kDoubleClickThreshold = 170;

Why 170ms? Is this based on tests you made on an N900 or desktop?

>-InputHandler.EventInfo = function EventInfo(aEvent, timestamp) {
>+InputHandler.EventInfo = function EventInfo(aEvent) {
>   this.event = aEvent;
>-  this.time = timestamp || Date.now();
>+  this.time = Date.now();

I assume this is just some code cleanup, since we do not seem to pass "timestamp" into the EventInfo anymore?

>+      let time = this._downUpEvents[1].time - this._downUpEvents[0].time;

Hopefully we are protecting the _downUpEvents array well enough now so we don't get an exception if _downUpEvents[1] does not exist.

>+    mouseDown: function mouseDown(cX, cY) {
>+      if (!this._overlayTimeout)
>+        this._overlayTimeout = setTimeout(function(self) { self._showCanvas(cX, cY); }, kDoubleClickThreshold, this);
>+    },

Hmm, I thought we were using this canvas overlay trick to avoid the 400ms delay? So we could show the link click as soon as possible.

No r+ yet, until we answer this question.

Let's get Ben to take a look at this too.
(In reply to comment #1)
> (From update of attachment 428219 [details] [diff] [review])
> 
> >+// threshold in ms to detect if the click is possibly a dblClick
> >+const kDoubleClickThreshold = 170;
> 
> Why 170ms? Is this based on tests you made on an N900 or desktop?

That's pretty much random since I don't own a n900 anymore (grrr). But Paul has dog-fooding my patch this morning and "think" he see an improvement. It also looks like he is using stylus more than finger so maybe there is a need to tweak this value.
> 
> >-InputHandler.EventInfo = function EventInfo(aEvent, timestamp) {
> >+InputHandler.EventInfo = function EventInfo(aEvent) {
> >   this.event = aEvent;
> >-  this.time = timestamp || Date.now();
> >+  this.time = Date.now();
> 
> I assume this is just some code cleanup, since we do not seem to pass
> "timestamp" into the EventInfo anymore?

yep, just code cleanup.

> 
> >+      let time = this._downUpEvents[1].time - this._downUpEvents[0].time;
> 
> Hopefully we are protecting the _downUpEvents array well enough now so we don't
> get an exception if _downUpEvents[1] does not exist.

:)

> 
> >+    mouseDown: function mouseDown(cX, cY) {
> >+      if (!this._overlayTimeout)
> >+        this._overlayTimeout = setTimeout(function(self) { self._showCanvas(cX, cY); }, kDoubleClickThreshold, this);
> >+    },
> 
> Hmm, I thought we were using this canvas overlay trick to avoid the 400ms
> delay? So we could show the link click as soon as possible.
> 

I think we were doing that not to avoid the 400ms delay but to give an immediate feedback to the user (I guess this is what you mean), and this code change is not needed for workaround the 400ms. But I also think that showing it immediatly is not really useful and didn't make sense when you're panning fast (we don't see the blue glow now when panning fast for in-content). Also there is an association between the blue glow and the click feedback, when the user see the blue glow he is sure that the a click is dispatched "right now" (or more or less)

> Let's get Ben to take a look at this too.

I will be happy to have his input, also I think that Madhava can have some thoughts on the delay before the blue glow.
Comment on attachment 428219 [details] [diff] [review]
Patch

>+// threshold in ms to detect if the click is possibly a dblClick
>+const kDoubleClickThreshold = 170;

Double clicking to zoom on the N900 is pretty inconsistent with this value.  Sometimes it works, other times it doesn't unless I tap really fast.  I think this is because sometimes those events get crammed really close together, and other times they don't.  Double click threshold needs to be larger IMO (or we need to get reliable timestamps from platform about when these events occurred).

>+    mouseDown: function mouseDown(cX, cY) {
>+      if (!this._overlayTimeout)
>+        this._overlayTimeout = setTimeout(function(self) { self._showCanvas(cX, cY); }, kDoubleClickThreshold, this);
>+    },
>+

Nit: it's a little worrisome to see kDoubleClickThreshold here.  It would be easy to accidentally forget to change this line if we make this a preference in InputHandler someday.  Either don't use this constant in browser.js or add a comment to InputHandler noting that we use the constant in other places?

r+ with higher threshold.
Attachment #428219 - Flags: review?(webapps) → review+
Hmm.  But if the threshold is higher there is more delay to painting.  With 170, it feels OK, but I'm sure 400 would feel laggy.

Perhaps two different values is the best way to go actually.
Attached patch Patch v0.2Splinter Review
I've set up the time to 200ms and use an other constant. I'll like to see this bug landed and have feedback from people.
Assignee: nobody → 21
Attachment #428219 - Attachment is obsolete: true
Attachment #431852 - Flags: review?(mark.finkle)
Attachment #428219 - Flags: review?(mark.finkle)
Attachment #431852 - Flags: review?(mark.finkle) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/697f4cd1f2af
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This patch makes me sad/happy. I can't find any problems with it. verified FIXED on builds:

ozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2.2pre) Gecko/20100311 Namoroka/3.6.2pre Fennec/1.1a1

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.2pre) Gecko/20100311 Namoroka/3.6.2pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a3pre) Gecko/20100311 Namoroka/3.7a3pre Fennec/1.1a2pre

and

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a3pre) Gecko/20100311 Namoroka/3.7a3pre Fennec/1.1a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: