918598 - [AccessFu] Support touch typing

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Keyboards, keypads and the likes should allow the user to quickly enter data with "touch typing". This means finding the key with explore by touch and lifting the finger should activate that key.

Comment 1

[Eitan Isaacson [:eeejay]]

Bug #808596 introduced a key role just for this kind of case.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Support touch typing.

Comment 3

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

Comment on attachment 807539 [details] [diff] [review]
Support touch typing.

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

Heyo. Here are some comments/questions so that I can eventually understand this code better:

::: accessible/src/jsat/AccessFu.jsm
@@ +797,5 @@
>          break;
>        case 'swipeleft1':
>          this.moveCursor('movePrevious', 'Simple', 'gesture');
>          break;
> +      case 'exploreend1':

This doesn't match 'exploreend' which you use below...

@@ +798,5 @@
>        case 'swipeleft1':
>          this.moveCursor('movePrevious', 'Simple', 'gesture');
>          break;
> +      case 'exploreend1':
> +        this.activateCurrent(null, true);

So exploreend1 semantically hints we should be on a key?

@@ +940,5 @@
>                                          'AccessFu:MoveByGranularity';
>      mm.sendAsyncMessage(type, aDetails);
>    },
>  
> +  activateCurrent: function activateCurrent(aData, aIsKey = false) {

Should 'aIsKey' be 'aCheckIfKey'?

::: accessible/src/jsat/TouchAdapter.jsm
@@ +432,2 @@
>          (this.distanceTraveled / this.dpi) > TouchAdapter.TAP_MAX_RADIUS) {
> +      return {type: this.done ? 'exploreend' : 'explore',

This doesn't match 'exploreend1' above. Is that intended?

::: accessible/src/jsat/TraversalRules.jsm
@@ +30,5 @@
>  const ROLE_CHECK_MENU_ITEM = Ci.nsIAccessibleRole.ROLE_CHECK_MENU_ITEM;
>  const ROLE_PASSWORD_TEXT = Ci.nsIAccessibleRole.ROLE_PASSWORD_TEXT;
>  const ROLE_RADIO_MENU_ITEM = Ci.nsIAccessibleRole.ROLE_RADIO_MENU_ITEM;
>  const ROLE_TOGGLE_BUTTON = Ci.nsIAccessibleRole.ROLE_TOGGLE_BUTTON;
> +const ROLE_KEY = Ci.nsIAccessibleRole.ROLE_KEY;

neato

::: accessible/src/jsat/content-script.js
@@ +165,5 @@
>    Logger.debug('activateCurrent');
>    function activateAccessible(aAccessible) {
> +    if (aMessage.json.isKey &&
> +        aAccessible.role != Ci.nsIAccessibleRole.ROLE_KEY) {
> +      // Only activate keys, don't do anythink on other objects.

nit: spelling 'anythink'

It is a bit strange to send the flag isKey true, when it is not... ?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #3)
> Comment on attachment 807539 [details] [diff] [review]
> Support touch typing.
> 
> Review of attachment 807539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Heyo. Here are some comments/questions so that I can eventually understand
> this code better:
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +797,5 @@
> >          break;
> >        case 'swipeleft1':
> >          this.moveCursor('movePrevious', 'Simple', 'gesture');
> >          break;
> > +      case 'exploreend1':
> 
> This doesn't match 'exploreend' which you use below...
> 

the gesture name and number of fingers get combined somewhere above this to make it easier with switch statements.

> @@ +798,5 @@
> >        case 'swipeleft1':
> >          this.moveCursor('movePrevious', 'Simple', 'gesture');
> >          break;
> > +      case 'exploreend1':
> > +        this.activateCurrent(null, true);
> 
> So exploreend1 semantically hints we should be on a key?
> 

No, if we ended the exploration on a key, activate it. The argument naming is unfortunate and should be changed.

> @@ +940,5 @@
> >                                          'AccessFu:MoveByGranularity';
> >      mm.sendAsyncMessage(type, aDetails);
> >    },
> >  
> > +  activateCurrent: function activateCurrent(aData, aIsKey = false) {
> 
> Should 'aIsKey' be 'aCheckIfKey'?

Yes :)

> 
> ::: accessible/src/jsat/TouchAdapter.jsm
> @@ +432,2 @@
> >          (this.distanceTraveled / this.dpi) > TouchAdapter.TAP_MAX_RADIUS) {
> > +      return {type: this.done ? 'exploreend' : 'explore',
> 
> This doesn't match 'exploreend1' above. Is that intended?
> 

Yeah, explained above.

> ::: accessible/src/jsat/TraversalRules.jsm
> @@ +30,5 @@
> >  const ROLE_CHECK_MENU_ITEM = Ci.nsIAccessibleRole.ROLE_CHECK_MENU_ITEM;
> >  const ROLE_PASSWORD_TEXT = Ci.nsIAccessibleRole.ROLE_PASSWORD_TEXT;
> >  const ROLE_RADIO_MENU_ITEM = Ci.nsIAccessibleRole.ROLE_RADIO_MENU_ITEM;
> >  const ROLE_TOGGLE_BUTTON = Ci.nsIAccessibleRole.ROLE_TOGGLE_BUTTON;
> > +const ROLE_KEY = Ci.nsIAccessibleRole.ROLE_KEY;
> 
> neato
> 

Yes.

> ::: accessible/src/jsat/content-script.js
> @@ +165,5 @@
> >    Logger.debug('activateCurrent');
> >    function activateAccessible(aAccessible) {
> > +    if (aMessage.json.isKey &&
> > +        aAccessible.role != Ci.nsIAccessibleRole.ROLE_KEY) {
> > +      // Only activate keys, don't do anythink on other objects.
> 
> nit: spelling 'anythink'
> 
> It is a bit strange to send the flag isKey true, when it is not... ?

Yeah, that is bad naming. It should be activateIfKey, perhaps.

Comment 5

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

Comment on attachment 807539 [details] [diff] [review]
Support touch typing.

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

Thanks.

r=me if Marco approves. Please do the rename :)

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 807539 [details] [diff] [review]
Support touch typing.

I do approve! r=me with David's nits fixed. This also gives us the flexibility to control which buttons are keys and which may not be. For example on iOS, Delete and Enter are not touch-typeable. They always have to be double-tapped. Gives a bit of security as to not accidentally delete or send something.

Comment 7

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/019a8c04596b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/019a8c04596b