Eitan Isaacson [:eeejay] Assignee
	Description • 10 years ago

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.

	Eitan Isaacson [:eeejay] Assignee
	Comment 1 • 10 years ago

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?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 2 • 10 years ago

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.

	Eitan Isaacson [:eeejay] Assignee
	Comment 3 • 10 years ago

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!

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 4 • 10 years ago

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.

	Eitan Isaacson [:eeejay] Assignee
	Comment 5 • 10 years ago

Updated tests.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 6 • 10 years ago

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

	Eitan Isaacson [:eeejay] Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a940b5a1f2f6

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 10 years ago

Backed out for mochitest-a11y orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce2fc3939da

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3030798&repo=mozilla-inbound

	Eitan Isaacson [:eeejay] Assignee
	Comment 9 • 10 years ago

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

	Eitan Isaacson [:eeejay] Assignee
	Comment 10 • 10 years ago

I changed the tests a bit. You could see the specific delta by looking at the changesets in the try page above.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 11 • 10 years ago

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;
}

	Eitan Isaacson [:eeejay] Assignee
	Comment 12 • 10 years ago

(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.

	Eitan Isaacson [:eeejay] Assignee
	Comment 13 • 10 years ago

Passes try as well:
https://tbpl.mozilla.org/?tree=Try&rev=5da2cab09e71

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 14 • 10 years ago

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.

	Eitan Isaacson [:eeejay] Assignee
	Comment 15 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/30f4aa3c6790

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/30f4aa3c6790