[AccessFu] Consecutive taps should only emit one gesture

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

For example, a double tap, should only emit doubletap, and not a tap followed by a doubletap. Also, a triple tap should not be: tap, doubletap, tripletap. But, simply tripletap at the end.
This is as far as I got. I know I reviewed your gesture rewrite, but I still find it hard to follow :(

What do you think of these changes? Are they crazy?
Attachment #8497872 - Flags: feedback?(yzenevich)
Comment on attachment 8497872 [details] [diff] [review]
Consecutive taps should only emit one gesture

Review of attachment 8497872 [details] [diff] [review]:
-----------------------------------------------------------------

I was wondering if we can simplify this by delaying what happens in Gesture.prototype._emit for the amount of time (== GestureSettings.maxConsecutiveGestureDelay) and checking if there's a consequent gesture that has _inProgress already set to 'true'. Then we would only dispatch and event if the condition was false.
Attachment #8497872 - Flags: feedback?(yzenevich)
Comment on attachment 8497872 [details] [diff] [review]
Consecutive taps should only emit one gesture

We discussed this in person, and it looks like this is the right direction. Now I guess all we need is an actual review!
Attachment #8497872 - Flags: review?(yzenevich)
Comment on attachment 8497872 [details] [diff] [review]
Consecutive taps should only emit one gesture

Review of attachment 8497872 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, gesture tests need to pass though.
Attachment #8497872 - Flags: review?(yzenevich)
Updated tests.
Attachment #8505644 - Flags: review?(yzenevich)
Comment on attachment 8505644 [details] [diff] [review]
Consecutive taps should only emit one gesture

Review of attachment 8505644 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with one nit.I could not apply the patch, but if it tests are passing - r=me

::: accessible/jsat/Gestures.jsm
@@ +680,3 @@
>   * GestureSettings.dwellThreshold.
>   * @param {Function} aTravelTo An optional constuctor for the next gesture to
>   * reject to in case the the TravelGesture test fails.

Comment about aRejectToOnPointerDown
Attachment #8505644 - Flags: review?(yzenevich) → review+
I tweaked the tests a bit so they don't rely on setTimeout, and try is happy:

https://tbpl.mozilla.org/?tree=Try&rev=b4faefc9a4a6
I changed the tests a bit. You could see the specific delta by looking at the changesets in the try page above.
Attachment #8497872 - Attachment is obsolete: true
Attachment #8505644 - Attachment is obsolete: true
Attachment #8506504 - Flags: review?(yzenevich)
Comment on attachment 8506504 [details] [diff] [review]
Consecutive taps should only emit one gesture. r=yzen

Review of attachment 8506504 [details] [diff] [review]:
-----------------------------------------------------------------

Added some comments about the test and also regarding timeout.

::: accessible/jsat/Gestures.jsm
@@ +707,5 @@
> +        if (GestureSettings.maxConsecutiveGestureDelay) {
> +          this._pointerUpTimer = setTimeout(() => {
> +            delete this._pointerUpTimer;
> +            this._deferred.resolve();
> +          }, GestureSettings.maxConsecutiveGestureDelay);

I keep thinking that the correct interval should be:

GestureSettings.maxConsecutiveGestureDelay - aTimeStamp + this.startTime;

In other words: we since the previous tap gesture already started we expect the next pointer down from the initial pointer down not pointer up.

::: accessible/tests/mochitest/jsat/dom_helper.js
@@ +101,5 @@
>  };
>  
>  var originalDwellThreshold = GestureSettings.dwellThreshold;
>  var originalSwipeMaxDuration = GestureSettings.swipeMaxDuration;
> +var originalConsecutiveGestureDelay = GestureSettings.maxConsecutiveGestureDelay;

Nit: line too long

@@ +118,5 @@
>        // The is not the event of interest.
>        return;
>      }
>      is(!!aEvent.detail.edge, !!types[0].edge);
> +    ok(true, 'Received correct mozAccessFuGesture: ' + JSON.stringify(types.shift()) + '.');

Nit: line too long

@@ +167,5 @@
>  AccessFuTest.addSequence = function AccessFuTest_addSequence(aSequence) {
>    AccessFuTest.addFunc(function testSequence() {
>      testMozAccessFuGesture(aSequence.expectedGestures);
>      var events = aSequence.events;
> +    function fireEvent(aEvent, aLast) {

I would prefer not to alter the settings for all sequences being tested. Can we add:
"removeMaxConsecutiveGestureDelay": true for the last pointerup events in sequences where we expect tap, doubletap and trippletap ? This will be similar to the rest of remove... options in gestures.json. The place where it should be overridden though will be resetTimers, since we want it to be set before:

function resetTimers(aRemoveMaxConsecutiveGestureDelay) {
  GestureSettings.dwellThreshold = originalDwellThreshold;
  GestureSettings.swipeMaxDuration = originalSwipeMaxDuration;
  GestureSettings.maxConsecutiveGestureDelay =
    aRemoveMaxConsecutiveGestureDelay ? 0 : originalConsecutiveGestureDelay;
}
Attachment #8506504 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #11)
> Comment on attachment 8506504 [details] [diff] [review]
> Consecutive taps should only emit one gesture. r=yzen
> 
> Review of attachment 8506504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Added some comments about the test and also regarding timeout.
> 
> ::: accessible/jsat/Gestures.jsm
> @@ +707,5 @@
> > +        if (GestureSettings.maxConsecutiveGestureDelay) {
> > +          this._pointerUpTimer = setTimeout(() => {
> > +            delete this._pointerUpTimer;
> > +            this._deferred.resolve();
> > +          }, GestureSettings.maxConsecutiveGestureDelay);
> 
> I keep thinking that the correct interval should be:
> 
> GestureSettings.maxConsecutiveGestureDelay - aTimeStamp + this.startTime;
> 
> In other words: we since the previous tap gesture already started we expect
> the next pointer down from the initial pointer down not pointer up.
> 

I first considered that too, but I don't think it is right. Consecutive gestures should be measured with the time between them. Measuring it from the start of the gesture just adds a whole bunch of variables. What if it was an exceptionally long tap (borderline dwell)? Unless the user puts their finger back down in lightening speed, they will never manage to double tap. A user who has a slow tap, will invariably have a slow follow-up tap as well. That is just for a double tap, a triple tab will exacerbate the inaccuracy even further.

> ::: accessible/tests/mochitest/jsat/dom_helper.js
> @@ +101,5 @@
> >  };
> >  
> >  var originalDwellThreshold = GestureSettings.dwellThreshold;
> >  var originalSwipeMaxDuration = GestureSettings.swipeMaxDuration;
> > +var originalConsecutiveGestureDelay = GestureSettings.maxConsecutiveGestureDelay;
> 
> Nit: line too long
> 
> @@ +118,5 @@
> >        // The is not the event of interest.
> >        return;
> >      }
> >      is(!!aEvent.detail.edge, !!types[0].edge);
> > +    ok(true, 'Received correct mozAccessFuGesture: ' + JSON.stringify(types.shift()) + '.');
> 
> Nit: line too long
> 
> @@ +167,5 @@
> >  AccessFuTest.addSequence = function AccessFuTest_addSequence(aSequence) {
> >    AccessFuTest.addFunc(function testSequence() {
> >      testMozAccessFuGesture(aSequence.expectedGestures);
> >      var events = aSequence.events;
> > +    function fireEvent(aEvent, aLast) {
> 
> I would prefer not to alter the settings for all sequences being tested. Can
> we add:
> "removeMaxConsecutiveGestureDelay": true for the last pointerup events in
> sequences where we expect tap, doubletap and trippletap ? This will be
> similar to the rest of remove... options in gestures.json. The place where
> it should be overridden though will be resetTimers, since we want it to be
> set before:
> 
> function resetTimers(aRemoveMaxConsecutiveGestureDelay) {
>   GestureSettings.dwellThreshold = originalDwellThreshold;
>   GestureSettings.swipeMaxDuration = originalSwipeMaxDuration;
>   GestureSettings.maxConsecutiveGestureDelay =
>     aRemoveMaxConsecutiveGestureDelay ? 0 : originalConsecutiveGestureDelay;
> }

Sounds good, I'll do that.
Comment on attachment 8507975 [details] [diff] [review]
Consecutive taps should only emit one gesture. r=yzen

Review of attachment 8507975 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thank you.
Attachment #8507975 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/30f4aa3c6790
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.