Closed
Bug 1075253
Opened 10 years ago
Closed 10 years ago
[AccessFu] Consecutive taps should only emit one gesture
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: eeejay, Assigned: eeejay)
References
Details
Attachments
(1 file, 3 obsolete files)
15.80 KB,
patch
|
yzen
:
review+
|
Details | Diff | Splinter Review |
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•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?
Attachment #8497872 -
Flags: feedback?(yzenevich)
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.
Attachment #8497872 -
Flags: feedback?(yzenevich)
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!
Attachment #8497872 -
Flags: review?(yzenevich)
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.
Attachment #8497872 -
Flags: review?(yzenevich)
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
Attachment #8505644 -
Flags: review?(yzenevich) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a940b5a1f2f6
Assignee: nobody → eitan
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
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
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.
Attachment #8497872 -
Attachment is obsolete: true
Attachment #8505644 -
Attachment is obsolete: true
Attachment #8506504 -
Flags: review?(yzenevich)
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; }
Attachment #8506504 -
Flags: review?(yzenevich)
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.
Assignee | ||
Comment 13•10 years ago
|
||
Passes try as well: https://tbpl.mozilla.org/?tree=Try&rev=5da2cab09e71
Attachment #8506504 -
Attachment is obsolete: true
Attachment #8507975 -
Flags: review?(yzenevich)
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.
Attachment #8507975 -
Flags: review?(yzenevich) → review+
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f4aa3c6790
https://hg.mozilla.org/mozilla-central/rev/30f4aa3c6790
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•