981015 - [AccessFu] Improve TouchAdapter.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

After adding a good number of touch adapter tests we need to add some of the responsiveness that the touch adapter lacks.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 patch v1

Fully tested on Firefox OS device, will do the same for Android asap.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 8394399 [details] [diff] [review]
981015 patch v1

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

Thanks! I think this patch only makes sense if it increases our accuracy and responsiveness. Readability is nice too, but definitely not worth regressing for that :)

I first tested with desktop Firefox with not much luck. Some gestures like one explore, then exploreend appear, but that is it.
Then tested on an inari. To be honest the explore by touch seemed very unresponsive. The explore events are emitted with erratic timing.
As for swipes, they seem pretty responsive, but I often get an explore instead of a swipe, and it seems to be emitted with a delay.

I think it would be cool to have some diagnostics built into this. By that I mean having an optional logging output that shows what events, their times and coordinates made each gesture. We can then easily reproduce and add it to tests if the intention was different then the outcome.

More feedback tomorrow..

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 v2 with no tests

Eitan, this is almost the same patch but it works well on Desktop now for me. I skipped tests here as I need to fix them up but you might get an idea whether it works for you.

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 8394399 [details] [diff] [review]
981015 patch v1

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

*very partial feedback* I was working on a more complete review when you updated the patch. I'll get back to it with the new one..

::: accessible/src/jsat/Gestures.jsm
@@ +22,5 @@
> +  'resource://gre/modules/Timer.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, 'clearTimeout', // jshint ignore:line
> +  'resource://gre/modules/Timer.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, 'Promise', // jshint ignore:line
> +  'resource://gre/modules/Promise.jsm');

I think Promise may be available already in m-c.

@@ +107,5 @@
> +  /**
> +   * Maximum duration of swipe
> +   * @type {Number}
> +   */
> +  swipeMaxDuration: SWIPE_MAX_DURATION,

What is the point of copying constants? I would simply get rid of the constants altogether.

@@ +108,5 @@
> +   * Maximum duration of swipe
> +   * @type {Number}
> +   */
> +  swipeMaxDuration: SWIPE_MAX_DURATION,
> +  /**

nit: extra newline after field and before doc of next field. Here and below.

@@ +127,5 @@
> +    this.clearTimer();
> +    delete this.current;
> +  },
> +  /**
> +   * Start the gesture tracker. Clean up the current gesture.

Don't you mean stop here?

@@ +133,5 @@
> +  stop: function GestureTracker_stop() {
> +    this.reset();
> +  },
> +  /**
> +   * Stop the gesture tracker. Clean up the current gesture. Determine the

And don't you mean start here?

@@ +140,5 @@
> +  start: function GestureTracker_start() {
> +    this._dpi = Utils.win.QueryInterface(Ci.nsIInterfaceRequestor).
> +      getInterface(Ci.nsIDOMWindowUtils).displayDPI;
> +    this.reset();
> +  },

Except for getting the dpi start and stop just wrap reset(), maybe the dpi should just be a lazy getter field?

@@ +144,5 @@
> +  },
> +  /**
> +   * Create a new gesture object and attach resolution handler to it as well as
> +   * handle the incoming pointer event.
> +   * @param  {Object} aDetail A new pointer event detail.

Is the use of the phrase "pointer event" coincedental, or are you borrowing from the pointer event spec, and hopefully we will transition to it?

@@ +148,5 @@
> +   * @param  {Object} aDetail A new pointer event detail.
> +   * @param  {Number} aTimeStamp A new pointer event timeStamp.
> +   * @param  {Function} AGesture A gesture constructor (default: Tap).
> +   */
> +  init: function GestureTracker_init(aDetail, aTimeStamp, AGesture = Tap) {

nit: aGesture

@@ +155,5 @@
> +      points[0].identifier !== MOUSE_ID) {
> +      // Unmutate gestures we are getting from Android when EBT is enabled.
> +      // Two finger gestures are translated to one. Double taps are translated
> +      // to single taps.
> +      AGesture = DoubleTap;

I don't like arguments being reassigned.

@@ +232,5 @@
> +   * @param  {Object} aResult A resolution payload with the relevant gesture id
> +   * and an optional new gesture contructor.
> +   */
> +  onFulfill: function GestureTracker_onFulfill(aResult) {
> +    let {id, AGesture} = aResult;

nit: AGesture? This is not an argument (and if it were it is badly cased).

@@ +306,5 @@
> +  this.deferred = Promise.defer();
> +  // Call this.resolve or this.reject when the promise is fulfilled with either
> +  // resolve or reject.
> +  this.promise = this.deferred.promise.then(this.resolve.bind(this),
> +    this.reject.bind(this));

I think you can simply do new Promise() instead of defer().promise.

@@ +381,5 @@
> +
> +  /**
> +   * Handle the pointer up event.
> +   * @param  {Array} aPoints A new pointer up points.
> +   * @param  {Number} aTimeStamp A new pointer up timeStamp.

You don't have a timestamp argument here.

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 8394399 [details] [diff] [review]
981015 patch v1

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

Cancelling reviews on this one. I'll give feedback on v2, and we could do tests separately.

Comment 6

[Eitan Isaacson [:eeejay]]

Comment on attachment 8395046 [details] [diff] [review]
981015 v2 with no tests

As discussed yesterday, explore by touch does not work correctly. Yura is reworking those bits.

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 v3 with no tests

Improved event handling. Desktop seems responsive again.

Comment 8

[Eitan Isaacson [:eeejay]]

Comment on attachment 8396643 [details] [diff] [review]
981015 v3 with no tests

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

As we talked on IRC, this seems to work well on desktop and b2g. So f+ for that. Please flag me for review once you have a working Android version.

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 patch v2

This patch v2 includes timeout independent tests and should work across all platforms.

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8400426 [details] [diff] [review]
981015 patch v2

After Skyping with Marco, I will modify the patch slightly to make it a little more sensitive when exploring. Will have the new revision asap.

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 patch v3

Improved responsive when exploring.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 8401096 [details] [diff] [review]
981015 patch v3

Yes, this is much more responsive when exploring, in that it always gives feedback when exploring to a new item. Much better user experience! Thanks for that!

I noticed a little flakiness at one stage where I tried to do a two-finger swipe up to scroll down the page. For some reason, it was mis-interpreted as a double-tap and hold gesture instead. After I dismissed the popup via the Back button, things started to behave flaky in general. For example, double-taps would then also be interpreted to tap-and-hold, and when loading the next page, I totally lost explore by touch and swiping alltogether. I had to restart Fennec to get a working browser again, meaning I had to remove it from the list of recently running apps. I then could not reproduce it on the same page and other pages. For some reason, the two-finger swipe up for scrolling then always worked. Not sure what to make of it yet, but I'll keep poking at it.

I also tested it on the older Nexus 7 2012 edition, and it works great there, too. So this change should also be good for older devices.

Comment 13

[Marco Zehe (:MarcoZ)]

Futher observations on Patch 3: Three finger swipes on Android are harder to trigger than with Patch 2. Even on a screen as wide as the Nexus 7. Since this is a point of complaint from our users frequently, especially on mobile phones, we should not regress here. So a reactive patch 3 with the three finger swipes working from patch 2 would be good!

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 8401096 [details] [diff] [review]
981015 patch v3

Cancelling review and waiting for new patch. :)

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 patch v4

Comment 16

[Marco Zehe (:MarcoZ)]

Comment on attachment 8401382 [details] [diff] [review]
981015 patch v4

OK I realize this is tricky. What patch 4 does is:
1. If nothing had been explored before, three finger swipes always work.
2. However, exploration has a very peculiar effect on busier pages:
a) I put my finger on the display and start exploring.
b) For a second or two, TalkBack won't say anything.
c) Then, it will start speaking and interrupting itself all the past points I had explored.
d) Long after I stopped moving my finger and letting it rest on an item, TalkBack finishes catching up with everything it had in its queue. The point where I rest my finger is then the last item spoken.
3. If in the situation where TalkBack is still in catch-up mode, I start swiping down with 3 fingers, the gesture isn't recognized at all. Only if I touch a single item again and thus "reset" the touch exploration logic to a point where it is not in catch-up mode, three finger swipes start working again.
4. Peculiar observation on 3: Swiping *upwards* with 3 fingers seems to be more likely to interrupt the catch-up mode and start working immediately. Swiping down with 3 fingers never did that for me.

One page I regularly test for a lot of things is http://heise.de/newsticker.
I observed 2 a) to d) just by exploring up and down the left-hand column with all the links to news stories.

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 patch v5

Comment 18

[Eitan Isaacson [:eeejay]]

Comment on attachment 8401382 [details] [diff] [review]
981015 patch v4

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

This generally looks great. I like the state transitions for each gesture a lot.

The one overall thing I would like to see in a future patch is stronger encapsulation between object types. Specifically Gesture/GestureTracker. Also, to avoid confusion, could you put a _ as a prefix for every private method and property?

::: accessible/src/jsat/Gestures.jsm
@@ +31,5 @@
> +const SWIPE_MIN_DISTANCE = 0.4;
> +// Maximum distance the pointer could move during a tap in inches
> +const TAP_MAX_RADIUS = 0.2;
> +// Minimum distance the pointer should move during travelling.
> +const TRAVEL_THRESHOLD = 0.05;

Is this the same as TRAVEL_THRESHOLD in PointerAdapter.jsm?

@@ +170,5 @@
> +   * @param  {Array} aPoints All changed points associated with the new pointer
> +   * event.
> +   * @param  {Function} AGesture A gesture constructor.
> +   */
> +  create: function GestureTracker_create(aTimeStamp, aPoints, AGesture) {

nit: aGesture

@@ +171,5 @@
> +   * event.
> +   * @param  {Function} AGesture A gesture constructor.
> +   */
> +  create: function GestureTracker_create(aTimeStamp, aPoints, AGesture) {
> +    this.current = new AGesture(Date.now(), Utils.dpi, aPoints);

Do we really need to be passing the DPI, can't we just use Utils.dpi directly in the gesture instance?

@@ +192,5 @@
> +   */
> +  startTimer: function(aGesture, aTimeStamp) {
> +    this.clearTimer();
> +    let delay;
> +    if (aGesture instanceof Swipe) {

Why not have the timer encapsulated in the gesture instances? It seems like you are leaking logic outside of it here.

@@ +211,5 @@
> +      delete this.timer;
> +      if (!aGesture.inProgress) {
> +        aGesture.deferred.reject();
> +      } else if (aGesture.rejectToOnWait) {
> +        aGesture.deferred.reject(aGesture.rejectToOnWait);

This who handle is tmi too, I think all of this needs to be in the gesture object. The use here of the promises interface is confusing enough. The GestureTracker should only need to call then() on Gesture.promise, Gesture.deferred should not be public.

@@ +445,5 @@
> +      this.emit(detail);
> +    }
> +    return {
> +      id: this.id,
> +      AGesture: this.resolveTo

gestureType?

@@ +457,5 @@
> +   *   id: current gesture id,
> +   *   AGesture: an optional subsequent gesture constructor.
> +   * }
> +   */
> +  reject: function Gesture_reject(aRejectTo) {

It confused me for a while that this method has the same name as the deferred API. Maybe _handleReject? Same with this.resolve().

@@ +483,5 @@
> +
> +/**
> + * A base explore related object.
> + */
> +function ExploreGesture() {

Object oriented js is hard! I think this is technically a mixin, not a base class.

@@ +494,5 @@
> +
> +/**
> + * A base object that is set to being in progress by default.
> + */
> +function InProgressGesture() {

same.

@@ +495,5 @@
> +/**
> + * A base object that is set to being in progress by default.
> + */
> +function InProgressGesture() {
> +  this.inProgress = true;

I think it is a bit overkill to create a mixin to override one boolean field.

@@ +614,5 @@
> + * @param {Number} aTimeStamp An initial pointer event's timeStamp.
> + * @param {Number} aDPI A current screen DPI.
> + * @param {Object} aPoints An existing set of points (from previous events).
> + */
> +function AndroidTap(aTimeStamp, aDPI, aPoints) {

I think fixing android would work better in PointerAdapter, no?

::: accessible/src/jsat/PointerAdapter.jsm
@@ +9,5 @@
> +
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +this.EXPORTED_SYMBOLS = ['PointerRelay', 'PointerAdapter'];// jshint ignore:line

nit: space before comment.

@@ +25,5 @@
> +const MOUSE_ID = 'mouse';
> +// Synthesized touch ID.
> +const SYNTH_ID = -1;
> +// Delay to prevent firing too many pointer move events.
> +const TRAVEL_THRESHOLD = 0.05;

not a delay anymore, but distance too.

@@ +27,5 @@
> +const SYNTH_ID = -1;
> +// Delay to prevent firing too many pointer move events.
> +const TRAVEL_THRESHOLD = 0.05;
> +
> +let PointerRelay = { // jshint ignore:line

Just a thought: You could offload a lot of the complexity in the gestures module here. Specifically, the event mutations that need to happen to interpret Android events. So that mozAccessFuPointerEvent would consistently emit events that can be interpreted as "true". Then Gestures.jsm could be platform agnostic.

@@ +61,5 @@
> +          'mouseup': false,
> +          'click': false };
> +        break;
> +
> +      default:

// desktop

@@ +124,5 @@
> +    Utils.win.dispatchEvent(new Utils.win.CustomEvent(
> +      'mozAccessFuPointerEvent', {
> +        bubbles: false,
> +        cancelable: true,
> +        detail: this.formatDetail(aType, aChangedTouches)

imho formatDetail could be folded in here. but whatever.

@@ +144,5 @@
> +          // Sometimes touch object can't be accessed after page navigation.
> +        }
> +        if (lastTouch && lastTouch.target === aTouch.target &&
> +          Math.hypot(aTouch.screenX - lastTouch.screenX, aTouch.screenY -
> +            lastTouch.screenY) / Utils.dpi < TRAVEL_THRESHOLD) {

Maybe hypothetical, but what if an item is smaller than the travel threshold. It may be skipped, no? Is it worth still having occasional non-suppressed moves based on time?

@@ +159,5 @@
> +      aEvent.view.top instanceof Ci.nsIDOMChromeWindow) {
> +      return;
> +    }
> +    if (aEvent.mozInputSource === Ci.nsIDOMMouseEvent.MOZ_SOURCE_UNKNOWN) {
> +      return;

Add a comment here. We ignore those events because they are scripted or clicks from the a11y API.

@@ +198,5 @@
> +  start: function PointerAdapter_start() {
> +    Logger.debug('PointerAdapter.start');
> +    GestureTracker.reset();
> +    PointerRelay.start();
> +    Utils.win.addEventListener('mozAccessFuPointerEvent', this);

Not entirely convinced that DOM events here are useful. A simple callback would do too. I'm fine either way.

I could see this as very useful in other places of our architecture to keep mvc separation cleaner. Or where there is a higher risk of cyclical references.

::: accessible/tests/mochitest/jsat/dom_helper.js
@@ +156,2 @@
>   * An extention to AccessFuTest that adds an ability to test a sequence of
> + * pointer events events and their expected mozAccessFuGesture events.

nit: events written twice

::: accessible/tests/mochitest/jsat/gestures.json
@@ +1,5 @@
>  [
>    {
>      "events": [
> +      {"type": "pointerdown", "points": [{"x": 1, "y": 1, "identifier": 1}]},
> +      {"type": "pointerup", "points": [{"x": 1, "y": 1, "identifier": 1}]}

Nice - I like this format a lot more.

BTW, the whole idea of putting this in a json format was really more about being able to collect data via user input - and less about how the tests themselves should be written. But this works.

Comment 19

[Eitan Isaacson [:eeejay]]

Comment on attachment 8402804 [details] [diff] [review]
981015 patch v5

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

Did a quick diff with the last patch to see what changed..

::: accessible/src/jsat/Gestures.jsm
@@ +251,5 @@
> +      this.create(current.startTime, current.points, current.lastEvent,
> +        AGesture);
> +      if (lastEvent === 'pointerup' && this.current.inProgress) {
> +        this.current.test(true);
> +        this.current.deferred.resolve();

meh! Now that the lastEvent is in the gesture, can't it just do it by itself?

::: accessible/src/jsat/PointerAdapter.jsm
@@ +132,5 @@
> +  suppressPointerMove: function PointerRelay_suppressPointerMove(aChangedTouches) {
> +    if (!this.lastPointerMove) {
> +      return false;
> +    }
> +    for (let i = 0; i < aChangedTouches.length; ++i) {

glad to see find() go away..

@@ +140,5 @@
> +        lastTouch = this.lastPointerMove.identifiedTouch ?
> +          this.lastPointerMove.identifiedTouch(touch.identifier) :
> +          this.lastPointerMove[i];
> +      } catch (x) {
> +        // Sometimes touch object can't be accessed after page navigation.

So what does the error look like? this.lastPointerMove is null? Can't we just check for that?

Comment 20

[Marco Zehe (:MarcoZ)]

Comment on attachment 8402804 [details] [diff] [review]
981015 patch v5

functionality r+ for this one! This works as expected, and I didn't find any problems with it.

Note that I'm on the road and only tested the functionality. I'm leaving the actual code review to Eitan for now, and I see feedback has been good and plenty. :)

Good work Yura!

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Eitan Isaacson [:eeejay] from comment #19)
> Comment on attachment 8402804 [details] [diff] [review]
> 981015 patch v5
> 
> Review of attachment 8402804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +140,5 @@
> > +        lastTouch = this.lastPointerMove.identifiedTouch ?
> > +          this.lastPointerMove.identifiedTouch(touch.identifier) :
> > +          this.lastPointerMove[i];
> > +      } catch (x) {
> > +        // Sometimes touch object can't be accessed after page navigation.
> 
> So what does the error look like? this.lastPointerMove is null? Can't we
> just check for that?

Accessing a dead object so it looks like it's gc'ed

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 patch v6

(In reply to Eitan Isaacson [:eeejay] from comment #18)
> Comment on attachment 8401382 [details] [diff] [review]
> 981015 patch v4
> 
> Review of attachment 8401382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This generally looks great. I like the state transitions for each gesture a
> lot.
> 
> The one overall thing I would like to see in a future patch is stronger
> encapsulation between object types. Specifically Gesture/GestureTracker.

Done

> Also, to avoid confusion, could you put a _ as a prefix for every private
> method and property?

Done

> 
> ::: accessible/src/jsat/Gestures.jsm
> @@ +31,5 @@
> > +const SWIPE_MIN_DISTANCE = 0.4;
> > +// Maximum distance the pointer could move during a tap in inches
> > +const TAP_MAX_RADIUS = 0.2;
> > +// Minimum distance the pointer should move during travelling.
> > +const TRAVEL_THRESHOLD = 0.05;
> 
> Is this the same as TRAVEL_THRESHOLD in PointerAdapter.jsm?

Yes, now unified.

> 
> @@ +170,5 @@
> > +   * @param  {Array} aPoints All changed points associated with the new pointer
> > +   * event.
> > +   * @param  {Function} AGesture A gesture constructor.
> > +   */
> > +  create: function GestureTracker_create(aTimeStamp, aPoints, AGesture) {
> 
> nit: aGesture

Done

> 
> @@ +171,5 @@
> > +   * event.
> > +   * @param  {Function} AGesture A gesture constructor.
> > +   */
> > +  create: function GestureTracker_create(aTimeStamp, aPoints, AGesture) {
> > +    this.current = new AGesture(Date.now(), Utils.dpi, aPoints);
> 
> Do we really need to be passing the DPI, can't we just use Utils.dpi
> directly in the gesture instance?

Done

> 
> @@ +192,5 @@
> > +   */
> > +  startTimer: function(aGesture, aTimeStamp) {
> > +    this.clearTimer();
> > +    let delay;
> > +    if (aGesture instanceof Swipe) {
> 
> Why not have the timer encapsulated in the gesture instances? It seems like
> you are leaking logic outside of it here.

Done

> 
> @@ +211,5 @@
> > +      delete this.timer;
> > +      if (!aGesture.inProgress) {
> > +        aGesture.deferred.reject();
> > +      } else if (aGesture.rejectToOnWait) {
> > +        aGesture.deferred.reject(aGesture.rejectToOnWait);
> 
> This who handle is tmi too, I think all of this needs to be in the gesture
> object. The use here of the promises interface is confusing enough. The
> GestureTracker should only need to call then() on Gesture.promise,
> Gesture.deferred should not be public.

Done

> 
> @@ +445,5 @@
> > +      this.emit(detail);
> > +    }
> > +    return {
> > +      id: this.id,
> > +      AGesture: this.resolveTo
> 
> gestureType?

Done

> 
> @@ +457,5 @@
> > +   *   id: current gesture id,
> > +   *   AGesture: an optional subsequent gesture constructor.
> > +   * }
> > +   */
> > +  reject: function Gesture_reject(aRejectTo) {
> 
> It confused me for a while that this method has the same name as the
> deferred API. Maybe _handleReject? Same with this.resolve().

Done

> 
> @@ +483,5 @@
> > +
> > +/**
> > + * A base explore related object.
> > + */
> > +function ExploreGesture() {
> 
> Object oriented js is hard! I think this is technically a mixin, not a base
> class.

Done

> 
> @@ +494,5 @@
> > +
> > +/**
> > + * A base object that is set to being in progress by default.
> > + */
> > +function InProgressGesture() {
> 
> same.

Removed entirely.

> 
> @@ +495,5 @@
> > +/**
> > + * A base object that is set to being in progress by default.
> > + */
> > +function InProgressGesture() {
> > +  this.inProgress = true;
> 
> I think it is a bit overkill to create a mixin to override one boolean field.

Removed entirely.

> 
> @@ +614,5 @@
> > + * @param {Number} aTimeStamp An initial pointer event's timeStamp.
> > + * @param {Number} aDPI A current screen DPI.
> > + * @param {Object} aPoints An existing set of points (from previous events).
> > + */
> > +function AndroidTap(aTimeStamp, aDPI, aPoints) {
> 
> I think fixing android would work better in PointerAdapter, no?
> 
> ::: accessible/src/jsat/PointerAdapter.jsm
> @@ +9,5 @@
> > +
> > +const Ci = Components.interfaces;
> > +const Cu = Components.utils;
> > +
> > +this.EXPORTED_SYMBOLS = ['PointerRelay', 'PointerAdapter'];// jshint ignore:line
> 
> nit: space before comment.

Done

> 
> @@ +25,5 @@
> > +const MOUSE_ID = 'mouse';
> > +// Synthesized touch ID.
> > +const SYNTH_ID = -1;
> > +// Delay to prevent firing too many pointer move events.
> > +const TRAVEL_THRESHOLD = 0.05;
> 
> not a delay anymore, but distance too.

Done

> 
> @@ +27,5 @@
> > +const SYNTH_ID = -1;
> > +// Delay to prevent firing too many pointer move events.
> > +const TRAVEL_THRESHOLD = 0.05;
> > +
> > +let PointerRelay = { // jshint ignore:line
> 
> Just a thought: You could offload a lot of the complexity in the gestures
> module here. Specifically, the event mutations that need to happen to
> interpret Android events. So that mozAccessFuPointerEvent would consistently
> emit events that can be interpreted as "true". Then Gestures.jsm could be
> platform agnostic.
> 
> @@ +61,5 @@
> > +          'mouseup': false,
> > +          'click': false };
> > +        break;
> > +
> > +      default:
> 
> // desktop

Done

> 
> @@ +124,5 @@
> > +    Utils.win.dispatchEvent(new Utils.win.CustomEvent(
> > +      'mozAccessFuPointerEvent', {
> > +        bubbles: false,
> > +        cancelable: true,
> > +        detail: this.formatDetail(aType, aChangedTouches)
> 
> imho formatDetail could be folded in here. but whatever.

Done

> 
> @@ +144,5 @@
> > +          // Sometimes touch object can't be accessed after page navigation.
> > +        }
> > +        if (lastTouch && lastTouch.target === aTouch.target &&
> > +          Math.hypot(aTouch.screenX - lastTouch.screenX, aTouch.screenY -
> > +            lastTouch.screenY) / Utils.dpi < TRAVEL_THRESHOLD) {
> 
> Maybe hypothetical, but what if an item is smaller than the travel
> threshold. It may be skipped, no? Is it worth still having occasional
> non-suppressed moves based on time?

I had to make the travel threshold smaller anyways (0.025inch) and it seems to be working quite well according to Marco.

> 
> @@ +159,5 @@
> > +      aEvent.view.top instanceof Ci.nsIDOMChromeWindow) {
> > +      return;
> > +    }
> > +    if (aEvent.mozInputSource === Ci.nsIDOMMouseEvent.MOZ_SOURCE_UNKNOWN) {
> > +      return;
> 
> Add a comment here. We ignore those events because they are scripted or
> clicks from the a11y API.

Done

> 
> @@ +198,5 @@
> > +  start: function PointerAdapter_start() {
> > +    Logger.debug('PointerAdapter.start');
> > +    GestureTracker.reset();
> > +    PointerRelay.start();
> > +    Utils.win.addEventListener('mozAccessFuPointerEvent', this);
> 
> Not entirely convinced that DOM events here are useful. A simple callback
> would do too. I'm fine either way.

Done

> 
> I could see this as very useful in other places of our architecture to keep
> mvc separation cleaner. Or where there is a higher risk of cyclical
> references.
> 
> ::: accessible/tests/mochitest/jsat/dom_helper.js
> @@ +156,2 @@
> >   * An extention to AccessFuTest that adds an ability to test a sequence of
> > + * pointer events events and their expected mozAccessFuGesture events.
> 
> nit: events written twice

Done

> 
> ::: accessible/tests/mochitest/jsat/gestures.json
> @@ +1,5 @@
> >  [
> >    {
> >      "events": [
> > +      {"type": "pointerdown", "points": [{"x": 1, "y": 1, "identifier": 1}]},
> > +      {"type": "pointerup", "points": [{"x": 1, "y": 1, "identifier": 1}]}
> 
> Nice - I like this format a lot more.
> 
> BTW, the whole idea of putting this in a json format was really more about
> being able to collect data via user input - and less about how the tests
> themselves should be written. But this works.

(In reply to Eitan Isaacson [:eeejay] from comment #19)
> Comment on attachment 8402804 [details] [diff] [review]
> 981015 patch v5
> 
> Review of attachment 8402804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did a quick diff with the last patch to see what changed..
> 
> ::: accessible/src/jsat/Gestures.jsm
> @@ +251,5 @@
> > +      this.create(current.startTime, current.points, current.lastEvent,
> > +        AGesture);
> > +      if (lastEvent === 'pointerup' && this.current.inProgress) {
> > +        this.current.test(true);
> > +        this.current.deferred.resolve();
> 
> meh! Now that the lastEvent is in the gesture, can't it just do it by itself?

Done

This patch does not change Android related stuff. I really like your idea though. The problem is that the event related material (like the number of points, etc) is altered based on the output of the AndroidTap gesture not the input (e.g. if it resolves then we consider it a (single finger) double tap, if it does not it depends what we reject to - a swipe (double finger) or taphold (single finger)). So it might be difficult to infer the type of the gesture based on the input events only. However, I do have a version of this patch where I move the timeout related stuff (related to triple swipes) from Gesture.jsm and move it to PointerAdapter.jsm. Although, this means that we have Android specific code in both files.

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 981015 patch v6 android bit

This is the additional android bit.

Comment 24

[Eitan Isaacson [:eeejay]]

Comment on attachment 8404701 [details] [diff] [review]
981015 patch v6

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

Overall looks great. *Much* more readable. r++.

I glanced over the Android bits without fully getting to the bottom of all of it, it is annoying that we have to do all that. Wondering if it would be useful to have tests that mimic Android input. Definitely not worth blocking this patch for that.

You could land once try is happy and the nits below are addressed, no need for more feedback from me.

::: accessible/src/jsat/Gestures.jsm
@@ +160,5 @@
> +      GestureConstructor = AndroidTap;
> +    }
> +    // Only create a new gesture on |pointerdown| event.
> +    if (aDetail.type !== 'pointerdown') {
> +      return;

This check could go at the top of the method. No?

@@ +185,5 @@
> +   * event.
> +   * @param {?String} aLastEvent Last pointer event type.
> +   */
> +  _create: function GestureTracker__create(aGesture, aTimeStamp, aPoints, aLastEvent) {
> +    this.current = new aGesture(aTimeStamp, aPoints, aLastEvent); // jshint ignore:line

What is the jshint error here? aGesture not being a known constructor? Could you add the reason in the comment in a way that jshint will still ignore?

@@ +265,5 @@
> +}
> +
> +/**
> + * A general gesture object.
> + * @param {Number} aTimeStamp An initial pointer event's timeStamp.

This is kind of vague. You mean the timestamp of the original pointerdown that transcends Gesture instances right?

@@ +496,5 @@
> + * A mixin for an explore related object.
> + */
> +function ExploreGesture() {
> +  this.compile = () => {
> +    // Unlike most of other gesutres explore based gestures comile using the

two typos :)

@@ +503,5 @@
> +  };
> +}
> +
> +/**
> + * Check the in progress gesture for copletion.

completion

@@ +508,5 @@
> + */
> +function checkProgressGesture(aGesture) {
> +  aGesture._inProgress = true;
> +  if (aGesture.lastEvent === 'pointerup') {
> +    aGesture.test(true);

test() is not defined by every gesture, or at least it is not a base method.

@@ +514,5 @@
> +  }
> +}
> +
> +/**
> + * A common travel gesture.

Could you elaborate what you mean by "travel gesture"? I get it, but if we need to make up our own terminology, it is worth explaining.

@@ +562,5 @@
> +DwellEnd.prototype = Object.create(TravelGesture.prototype);
> +DwellEnd.prototype.type = 'dwellend';
> +
> +/**
> + * TapHoldEnd gesture.

Could you spell out what a tap-hold-end means? Is it pointerdown-pointerup-pointerdown-*wait*-pointerup? Or what used to be called dwell?

@@ +578,5 @@
> +TapHoldEnd.prototype = Object.create(TravelGesture.prototype);
> +TapHoldEnd.prototype.type = 'tapholdend';
> +
> +/**
> + * DoubleTapHoldEnd gesture.

Same, and how is it different from TapHoldEnd.

@@ +602,5 @@
> + * @param {Function} aRejectTo A constructor for the next gesture to reject to
> + * in case no pointermove or pointerup happens within the
> + * GestureSettings.dwellThreshold.
> + * @param {Function} aTravelTo An optional constuctor for the next gesture to
> + * reject to in case the the TravelGesture test fails.

I like the rejectTo/travelTo duo..

@@ +612,5 @@
> +    TAP_MAX_RADIUS);
> +}
> +
> +TapGesture.prototype = Object.create(TravelGesture.prototype);
> +TapGesture.prototype._getDelay = function TapGesture__getDelay() {

I guess the assumption here is that the timestamp getDelay recieves is irrelevant, since we are simultaneously starting the timeout?

@@ +713,5 @@
> +}
> +
> +DoubleTap.prototype = Object.create(TapGesture.prototype);
> +DoubleTap.prototype.type = 'doubletap';
> +DoubleTap.prototype.resolveTo = TripleTap;

I love how this just works..

::: accessible/tests/mochitest/jsat/a11y.ini
@@ -15,5 @@
>  [test_live_regions.html]
>  [test_output.html]
>  [test_tables.html]
> -[test_touch_adapter.html]
> -skip-if = true # disabled for a number of intermitten failures: Bug 982326

I'm assuming try is always happy?

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Eitan Isaacson [:eeejay] from comment #24)
> Comment on attachment 8404701 [details] [diff] [review]

> > +/**
> > + * A common travel gesture.
> 
> Could you elaborate what you mean by "travel gesture"? I get it, but if we
> need to make up our own terminology, it is worth explaining.

This is like when you stick your thumb up, for hitchhiking :)

Comment 26

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #25)
> (In reply to Eitan Isaacson [:eeejay] from comment #24)
> > Comment on attachment 8404701 [details] [diff] [review]
> 
> > > +/**
> > > + * A common travel gesture.
> > 
> > Could you elaborate what you mean by "travel gesture"? I get it, but if we
> > need to make up our own terminology, it is worth explaining.
> 
> This is like when you stick your thumb up, for hitchhiking :)

In Europe it's an index finger and in the US it's a thumb. That's why I am asking.

Comment 27

[Yura Zenevich [:yzen] (please use "needinfo")]

https://tbpl.mozilla.org/?tree=Try&rev=dd61d5751d77

Comment 28

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b995b3d628ed