817869 - [AccessFu] Consolidate Android and B2G touch adapters

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

By using the same touch adapter we get gesture detection in Android. This is required if we want to do gesture quick navigation as we discussed but this adds the *huge* benefit of back-porting all of this touch niceness to Ice Cream Sandwich and even Gingerbread devices.

So the patch I am about to submit does the following on each platform:

Jelly Bean (4.1/4.2):
- Adds paginated scrolling. If you do a two finger swipe you could scroll by pages instead of panning with two fingers (the default behavior on Android). Paginated scrolling makes a lot more sense for VI use cases.

Ice Cream Sandwich (4.0):
- Paginated scrolling (see above).
- Double tap anywhere to activate item, Jelly Bean style. Weird single tap activation remains in place, so it is still consistent with other ICS apps.
- Swipe left or right to navigate by item, like in Jelly Bean.

Gingerbread (2.3):
- Paginated scrolling, double tap activation, and swipe navigation (see above).
- Explore by touch!

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Consolidate Android and B2G touch adapter.

Comment 2

[Hubert Figuiere [:hub]]

Comment on attachment 688030 [details] [diff] [review]
Consolidate Android and B2G touch adapter.

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

::: accessible/src/jsat/AccessFu.jsm
@@ +337,5 @@
>        case 'mozAccessFuGesture':
> +        let gesture = aEvent.detail;
> +        Logger.info('Gesture', gesture.type,
> +                    '(fingers: ' + gesture.touches.length + ')');
> +        if (Utils.MozBuildApp == 'mobile/android')

Couldn't we make that a build time define? I have seen that done for .jsm

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to Hub Figuiere [:hub] from comment #2)
> Comment on attachment 688030 [details] [diff] [review]
> Consolidate Android and B2G touch adapter.
> 
> Review of attachment 688030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +337,5 @@
> >        case 'mozAccessFuGesture':
> > +        let gesture = aEvent.detail;
> > +        Logger.info('Gesture', gesture.type,
> > +                    '(fingers: ' + gesture.touches.length + ')');
> > +        if (Utils.MozBuildApp == 'mobile/android')
> 
> Couldn't we make that a build time define? I have seen that done for .jsm

Yeah, I looked in to that, but I got stuck trying to do it. Something to do with the different ways of putting the jsm's in a Makefile. Might be worth revisiting.

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 688030 [details] [diff] [review]
Consolidate Android and B2G touch adapter.

>-Cu.import('resource://gre/modules/accessibility/TouchAdapter.jsm');

Why can this be removed and still work in referencing the touch adapter?

Comment 5

[Marco Zehe (:MarcoZ)]

I just built with this patch plus the patches from bug 817873, bug 817684 and bug 817671 and put the build on both a Jelly Bean and a Gingerbread device. I am blown away! This works absolutely great, and I have a feeling the gesture recognition even works better than in the rest of Jelly Bean, where I sometimes have cases where touches aren't recognized, but with this build, I never hit this case in the web content in Fennec.

Absolutely great job!

Comment 6

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

(In reply to Hub Figuiere [:hub] from comment #2)
> Comment on attachment 688030 [details] [diff] [review]
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +337,5 @@
> >        case 'mozAccessFuGesture':
> > +        let gesture = aEvent.detail;
> > +        Logger.info('Gesture', gesture.type,
> > +                    '(fingers: ' + gesture.touches.length + ')');
> > +        if (Utils.MozBuildApp == 'mobile/android')
> 
> Couldn't we make that a build time define? I have seen that done for .jsm

Cool, I didn't know you could do that...

Comment 7

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

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> Comment on attachment 688030 [details] [diff] [review]
> Consolidate Android and B2G touch adapter.
> 
> >-Cu.import('resource://gre/modules/accessibility/TouchAdapter.jsm');
> 
> Why can this be removed and still work in referencing the touch adapter?

It gets imported later in the this.AccessFu initialization area.

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Bug 817869 - Consolidate Android and B2G touch adapter.

Some fixes on the original patch. Make tap events in android a bit less sensitive since they are easy to misinterpret.

Comment 9

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

Comment on attachment 688547 [details] [diff] [review]
Bug 817869 - Consolidate Android and B2G touch adapter.

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

Nice to see this convergence. Pretty nice that you can just do chromeWin.addEventListener now and we don't need to check FF version or anything since we are bundled which turns out to be a handy thing.

::: accessible/src/jsat/AccessFu.jsm
@@ +340,5 @@
> +                    '(fingers: ' + gesture.touches.length + ')');
> +        if (Utils.MozBuildApp == 'mobile/android')
> +          this._handleAndroidGesture(aEvent.detail);
> +        else
> +          this._handleB2gGesture(aEvent.detail);

aside: it is interesting our MDN coding style guide doesn't say anything about if/else vs ternary.

@@ +354,5 @@
>  
> +    switch (gestureName) {
> +      case 'explore1':
> +        this.moveCursor('moveToPoint', 'Simple', 'gesture',
> +                          aGesture.x, aGesture.y);

nit: formatting looks off by a space or two here.

@@ +387,3 @@
>  
> +  _handleB2gGesture: function _handleB2gGesture(aGesture) {
> +    let gestureName = aGesture.type + aGesture.touches.length;

It is a bit weird to have so much duplicated between the two gesture handlers. I'd consider have a default: common_gesture_handler or something but I'll trust what you have seemed cleaner? Please put the cases in the same order in each handler. It would also be good to add a comment about missing gestures and why they aren't included. (Although maybe you add them in a later bug/patch)

::: accessible/src/jsat/TouchAdapter.jsm
@@ +88,5 @@
> +
> +    if (this._delayedEvent) {
> +      this.chromeWin.clearTimeout(this._delayedEvent);
> +      delete this._delayedEvent;
> +    }

nit: I'd put this at the top of the function.

@@ +136,5 @@
> +
> +      case 'mouseenter':
> +      {
> +        let touchPoint = new TouchPoint(aEvent, timeStamp, this._dpi);
> +        this._touchPoints['-1'] = touchPoint;

Interesting. Should we use something more descriptive than '-1'?

@@ +152,5 @@
> +        if (touchPoint)
> +          touchPoint.update(aEvent, timeStamp);
> +
> +        if (aEvent.timeStamp - this._lastExploreTime >= EXPLORE_THROTTLE) {
> +          if (Utils.AndroidSdkVersion >= 16) {

Why the version check?

@@ +235,5 @@
>              details.type = 'taphold';
> +          if (details.type == 'explore' && prevGesture.type == 'explore') {
> +            details.deltaX = details.x - prevGesture.x;
> +            details.deltaY = details.y - prevGesture.y;
> +          }

You might want to switch on details.type here. Might look cleaner.

@@ +260,5 @@
> +        aDetails.touches[0] != '-1') {
> +      if (aDetails.touches.length == 1) {
> +        if (aDetails.type == 'tap') {
> +          emitDelay = 50;
> +          aDetails.type = 'doubletap';

(makes me think of IndieUI)

Comment 10

[Marco Zehe (:MarcoZ)]

While we're here, could we try to fix the following issue I'm seeing in both a current nightly build as well as the local build with all these seven patches applied:
1. Load any web page. I was on the AMO page for Adblock Plus.
2. Lift your finger from the touch screen, then put it down on any web content, but don't move it.
Expected result: Talkback should speak the item under the finger.
Actual result: A slight beep is heard, indicating accessibility focus was set, but nothing gets spoken.
3. Move your finger slightly.
Result: Talkback will speak the item you dragged the finger to.
4. Move finger back to the previous position.
Result: This time, the item gets spoken.
6. Repeat this with a different spot. The result will always be that the initial touch receives a beep, but no speech. The finger must be put down and kept there, not moved, to see and hear the effect.

I did this on the NEXUS 7.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #9)
> Comment on attachment 688547 [details] [diff] [review]
> Bug 817869 - Consolidate Android and B2G touch adapter.
> 
> Review of attachment 688547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice to see this convergence. Pretty nice that you can just do
> chromeWin.addEventListener now and we don't need to check FF version or
> anything since we are bundled which turns out to be a handy thing.
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +340,5 @@
> > +                    '(fingers: ' + gesture.touches.length + ')');
> > +        if (Utils.MozBuildApp == 'mobile/android')
> > +          this._handleAndroidGesture(aEvent.detail);
> > +        else
> > +          this._handleB2gGesture(aEvent.detail);
> 
> aside: it is interesting our MDN coding style guide doesn't say anything
> about if/else vs ternary.
> 

What is a ternary?

> @@ +354,5 @@
> >  
> > +    switch (gestureName) {
> > +      case 'explore1':
> > +        this.moveCursor('moveToPoint', 'Simple', 'gesture',
> > +                          aGesture.x, aGesture.y);
> 
> nit: formatting looks off by a space or two here.
> 

Fixed.

> @@ +387,3 @@
> >  
> > +  _handleB2gGesture: function _handleB2gGesture(aGesture) {
> > +    let gestureName = aGesture.type + aGesture.touches.length;
> 
> It is a bit weird to have so much duplicated between the two gesture
> handlers. I'd consider have a default: common_gesture_handler or something
> but I'll trust what you have seemed cleaner? Please put the cases in the
> same order in each handler. It would also be good to add a comment about
> missing gestures and why they aren't included. (Although maybe you add them
> in a later bug/patch)
> 

I considered having a table like we do for quicknav keys. But the switch blocks seem to be very readable. Reordered them. As for missing gestures, some are not relevant for the other platform, and some will be added.

> ::: accessible/src/jsat/TouchAdapter.jsm
> @@ +88,5 @@
> > +
> > +    if (this._delayedEvent) {
> > +      this.chromeWin.clearTimeout(this._delayedEvent);
> > +      delete this._delayedEvent;
> > +    }
> 
> nit: I'd put this at the top of the function.
> 

Fixed.

> @@ +136,5 @@
> > +
> > +      case 'mouseenter':
> > +      {
> > +        let touchPoint = new TouchPoint(aEvent, timeStamp, this._dpi);
> > +        this._touchPoints['-1'] = touchPoint;
> 
> Interesting. Should we use something more descriptive than '-1'?
> 

Good idea, maybe 'hover'?

> @@ +152,5 @@
> > +        if (touchPoint)
> > +          touchPoint.update(aEvent, timeStamp);
> > +
> > +        if (aEvent.timeStamp - this._lastExploreTime >= EXPLORE_THROTTLE) {
> > +          if (Utils.AndroidSdkVersion >= 16) {
> 
> Why the version check?
> 

There is no need to do gesture interpretation on a hover event in Jelly Bean, since the one finger gestures are done by TalkBack. The updated patch does not have this anymore, and consolidates the mouse hover gesture detection with touch events.


> @@ +235,5 @@
> >              details.type = 'taphold';
> > +          if (details.type == 'explore' && prevGesture.type == 'explore') {
> > +            details.deltaX = details.x - prevGesture.x;
> > +            details.deltaY = details.y - prevGesture.y;
> > +          }
> 
> You might want to switch on details.type here. Might look cleaner.
> 

Good idea.

> @@ +260,5 @@
> > +        aDetails.touches[0] != '-1') {
> > +      if (aDetails.touches.length == 1) {
> > +        if (aDetails.type == 'tap') {
> > +          emitDelay = 50;
> > +          aDetails.type = 'doubletap';
> 
> (makes me think of IndieUI)

Yeah?

Comment 12

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

Comment on attachment 688547 [details] [diff] [review]
Bug 817869 - Consolidate Android and B2G touch adapter.

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

I'll check the new patch. \o/

Comment 13

[Eitan Isaacson [:eeejay]]

Attachment: Bug 817869 - Consolidate Android and B2G touch adapter.

Comment 14

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

Thanks for the Skype. I'll need a bit of time - I'm rusty on parts of AccessFu.

Comment 15

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

Comment on attachment 688891 [details] [diff] [review]
Bug 817869 - Consolidate Android and B2G touch adapter.

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

Had a quick look to see if I could find the cause of Marco's issue but no luck...

::: accessible/src/jsat/AccessFu.jsm
@@ +334,5 @@
>        case 'keypress':
>          this._handleKeypress(aEvent);
>          break;
>        case 'mozAccessFuGesture':
> +        let gesture = aEvent.detail;

nit: you only use this variable in the Logger. Get rid of it?

Comment 16

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

Comment on attachment 688891 [details] [diff] [review]
Bug 817869 - Consolidate Android and B2G touch adapter.

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

r=me, given you fixed marco's issue with this patch :)

(I'm a little worried about how long it will take to understand the Android nuances when/if I come back to some of this code)

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: Bug 817869 - Consolidate Android and B2G touch adapter.

OK. Sorry for this, but I needed to change this patch a bit after some recent testing with B2G. I figured it is best to just resubmit for review. The main changes from the previous patch are:

1. Consolidated the gesture->function mapping for both B2G and Android. Turns out that budget hardware may not allow more than 2 finger gestures, so 2 finger scrolling makes more sense anyway.
2. Reintroduced the "glass" element for B2G, it is the only way to consume touch events, and allow "clicking" elements under the glass with acc.doAction() without doing a weird passthrough dance. On Android it is not an issue for some reason.

Comment 18

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bdbb9387541

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7bdbb9387541