[AccessFu] Consecutive taps should only emit one gesture

RESOLVED FIXED in mozilla36

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 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.
(Assignee)

Comment 1

3 years ago
Created attachment 8497872 [details] [diff] [review]
Consecutive taps should only emit one gesture

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)
(Assignee)

Updated

3 years ago
Blocks: 1069598
(Assignee)

Comment 3

3 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!
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)
(Assignee)

Comment 5

3 years ago
Created attachment 8505644 [details] [diff] [review]
Consecutive taps should only emit one gesture

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+
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a940b5a1f2f6
Assignee: nobody → eitan
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
(Assignee)

Comment 9

3 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
(Assignee)

Comment 10

3 years ago
Created attachment 8506504 [details] [diff] [review]
Consecutive taps should only emit one gesture. r=yzen

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)
(Assignee)

Comment 12

3 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.
(Assignee)

Comment 13

3 years ago
Created attachment 8507975 [details] [diff] [review]
Consecutive taps should only emit one gesture. r=yzen

Passes try as well:
https://tbpl.mozilla.org/?tree=Try&rev=5da2cab09e71
Attachment #8506504 - Attachment is obsolete: true
Attachment #8507975 - Flags: review?(yzenevich)
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+
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f4aa3c6790
https://hg.mozilla.org/mozilla-central/rev/30f4aa3c6790
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.