1050383 - [AccessFu] Support gaia edge gestures

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

When a user swipes with two fingers from an edge, we should send a mozChromeEvent to gaia to initiate an edge gesture.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Send edge gesture events on two finger edge swipes.

Comment 2

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

Comment on attachment 8470224 [details] [diff] [review]
Send edge gesture events on two finger edge swipes.

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

Looks good, I left some comments and could you add a test case to make sure we capture edge correctly.

::: accessible/jsat/AccessFu.jsm
@@ +527,5 @@
>      }
>    },
>  
>    B2G: function B2G(aDetails) {
> +    Utils.dispatchChromeEvent('accessfu-output', aDetails);

Utils.dispatchChromeEvent should take 1 arg, see below.

@@ +695,5 @@
>          this.activateCurrent(null, true);
>          break;
>        case 'swiperight2':
> +        if (aGesture.edge) {
> +          Utils.dispatchChromeEvent('accessibility-edge-gesture', 'right');

Here and below: I think we should only fire 'accessfu-output' event to keep the API consistent. Details of the event (similar to what's in the Presentation.jsm) have eventType field where 'edge-gesture' should go, and data field which should contain 'right', 'down' etc. So here we would call:
Utils.dispatchChromeEvent({eventType: 'edge-gesture', data: 'right'});

::: accessible/jsat/Gestures.jsm
@@ +78,5 @@
>  const ANDROID_TRIPLE_SWIPE_DELAY = 50;
>  // The virtual touch ID generated by a mouse event.
>  const MOUSE_ID = 'mouse';
> +// Amount of pixels from the edges of the screen for it to be an edge swipe
> +const EDGE = 10;

Should we take DPI into account?

@@ +944,5 @@
>    let deltaX = detail.deltaX;
>    let deltaY = detail.deltaY;
>    if (Math.abs(deltaX) > Math.abs(deltaY)) {
>      // Horizontal swipe.
> +    let startPoints = [t.x1 for (t of detail.touches)];

Nit: t -> touch

@@ +947,5 @@
>      // Horizontal swipe.
> +    let startPoints = [t.x1 for (t of detail.touches)];
> +    if (deltaX > 0) {
> +      detail.type = type + 'right';
> +      detail.edge = Math.min.apply(null, startPoints) <= EDGE;

Here and below, I think we should only consider it edge iff all startPoints are within the EDGE. Otherwise, we can have a 2 finger swipe side by side where one finger was at the edge and the other in the middle of the screen marked as edge.

::: accessible/jsat/Utils.jsm
@@ +423,5 @@
>      return parent.role === Roles.LISTITEM && parent.childCount > 1 &&
>        aStaticText.indexInParent === 0;
> +  },
> +
> +  dispatchChromeEvent: function dispatchChromeEvent(aType, aDetails) {

Should only take aDetails. Type should probably always be 'accessfu-output'.

@@ +426,5 @@
> +
> +  dispatchChromeEvent: function dispatchChromeEvent(aType, aDetails) {
> +    let details = {
> +      type: aType,
> +      details: typeof aDetails === 'string' ?

If you are OK with changes in AccessFu.jsm aDetails should be expected to be a JSON.

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #2)
> Comment on attachment 8470224 [details] [diff] [review]
> Send edge gesture events on two finger edge swipes.
> 
> Review of attachment 8470224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, I left some comments and could you add a test case to make sure
> we capture edge correctly.

I'll get on the test cases, indeed.

> 
> ::: accessible/jsat/AccessFu.jsm
> @@ +527,5 @@
> >      }
> >    },
> >  
> >    B2G: function B2G(aDetails) {
> > +    Utils.dispatchChromeEvent('accessfu-output', aDetails);
> 
> Utils.dispatchChromeEvent should take 1 arg, see below.
> 
> @@ +695,5 @@
> >          this.activateCurrent(null, true);
> >          break;
> >        case 'swiperight2':
> > +        if (aGesture.edge) {
> > +          Utils.dispatchChromeEvent('accessibility-edge-gesture', 'right');
> 
> Here and below: I think we should only fire 'accessfu-output' event to keep
> the API consistent. Details of the event (similar to what's in the
> Presentation.jsm) have eventType field where 'edge-gesture' should go, and
> data field which should contain 'right', 'down' etc. So here we would call:
> Utils.dispatchChromeEvent({eventType: 'edge-gesture', data: 'right'});

"accessfu-output" is not very descriptive of the event type, if it isn't actually output. Maybe the prefix could be the same? "accessfu-" or "accessibility-"?

> 
> ::: accessible/jsat/Gestures.jsm
> @@ +78,5 @@
> >  const ANDROID_TRIPLE_SWIPE_DELAY = 50;
> >  // The virtual touch ID generated by a mouse event.
> >  const MOUSE_ID = 'mouse';
> > +// Amount of pixels from the edges of the screen for it to be an edge swipe
> > +const EDGE = 10;
> 
> Should we take DPI into account?

I just did some measuring. The reported dpi by Utils.dpi is 254, yet the flame screen with a horizontal CSS pixel count of 320 is 2 3/16", this comes out to about 146 dpi. If you multiply that by window.devicePixelRatio (1.5) you get about 219.

So it doesn't really all add up. We should look into this.

> 
> @@ +944,5 @@
> >    let deltaX = detail.deltaX;
> >    let deltaY = detail.deltaY;
> >    if (Math.abs(deltaX) > Math.abs(deltaY)) {
> >      // Horizontal swipe.
> > +    let startPoints = [t.x1 for (t of detail.touches)];
> 
> Nit: t -> touch
> 
> @@ +947,5 @@
> >      // Horizontal swipe.
> > +    let startPoints = [t.x1 for (t of detail.touches)];
> > +    if (deltaX > 0) {
> > +      detail.type = type + 'right';
> > +      detail.edge = Math.min.apply(null, startPoints) <= EDGE;
> 
> Here and below, I think we should only consider it edge iff all startPoints
> are within the EDGE. Otherwise, we can have a 2 finger swipe side by side
> where one finger was at the edge and the other in the middle of the screen
> marked as edge.

It is already pretty hard to get an edge gesture right, I'll play with that.

I'll deal with the nits!

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Send edge gesture events on two finger edge swipes.

Comment 5

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

Comment on attachment 8470978 [details] [diff] [review]
Send edge gesture events on two finger edge swipes.

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

Nice!

Comment 6

[Eitan Isaacson [:eeejay]]

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

Comment 7

[Ed Morley [:emorley]]

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