Last Comment Bug 762851 - [AccessFu] Introduce single letter quick navigation keys
: [AccessFu] Introduce single letter quick navigation keys
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla16
Assigned To: Marco Zehe (:MarcoZ) on PTO until August 15
:
Mentors:
Depends on:
Blocks: 764211
  Show dependency treegraph
 
Reported: 2012-06-08 03:55 PDT by Marco Zehe (:MarcoZ) on PTO until August 15
Modified: 2012-06-12 18:32 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch, not yet tested (16.92 KB, patch)
2012-06-08 03:55 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
WIP2 (16.09 KB, patch)
2012-06-08 09:04 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
WIP3 (16.03 KB, patch)
2012-06-08 09:11 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
WIP with different technique (30.64 KB, patch)
2012-06-08 11:47 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
eitan: feedback+
Details | Diff | Splinter Review
WIP4: New approach with proper line endings (14.76 KB, patch)
2012-06-08 23:22 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
What I currently have, doesn't work (15.18 KB, patch)
2012-06-09 21:53 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
no flags Details | Diff | Splinter Review
Patch which should, in theory, be working (15.20 KB, patch)
2012-06-11 05:25 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
eitan: feedback+
Details | Diff | Splinter Review
Patch (20.91 KB, patch)
2012-06-12 03:50 PDT, Marco Zehe (:MarcoZ) on PTO until August 15
eitan: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-08 03:55:14 PDT
Created attachment 631339 [details] [diff] [review]
WIP patch, not yet tested

This patch is still untested, primarily put here for safe-keeping and possibly feedback. The keys introduced are:

a: named anchors
b: buttons
c: combobox and listbox
e: entry
f: many form control types
g: graphic
h: heading
i: list item in a ul, ol or dl
k: link
l: list (ul, ol, or dl)
p: pagetab (for example ARIA tabs)
r: radio buttons and radio menu items
s: separators
t: tables
x: checkboxes and checkable menu items

Question: I would also like to have 1 through 6 move to headings of a specific type. Eitan, can I define *one* specific rule that gets a parameter from me, and that then asks the heading accessible for its level, or do they have to be separate rules?

Not yet supported are landmarks, because we don't have utterances for them yet, either. Since they can be of *any* role, we must do some object attribute querying. This should be done in a separate bug along with the utterances.
Comment 1 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-08 05:57:11 PDT
Comment on attachment 631339 [details] [diff] [review]
WIP patch, not yet tested

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

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +46,5 @@
>  
>      switch (aEvent.keyCode) {
> +      case 0:
> +        // an alphanumeric key was pressed, handle it separately.
> +        handleQuickNavKeys(aEvent);

This has beein fixed locally to this.handleQuickNavKeys...
Comment 2 David Bolter [:davidb] 2012-06-08 07:54:47 PDT
Comment on attachment 631339 [details] [diff] [review]
WIP patch, not yet tested

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

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +228,5 @@
>      if (last) {
>        virtualCursor.moveLast(this.SimpleTraversalRule);
>      } else {
>        try {
> +        virtualCursor.moveNext((rule ? rule : this.SimpleTraversalRule));

js would let you write this as:
virtualCursor.moveNext(rule || this.SimpleTraversalRule);

(same elsewhere)
Comment 3 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-08 09:04:40 PDT
Created attachment 631415 [details] [diff] [review]
WIP2

Fixes a few issues with statements. gjslint doesn't find any errors, but somehow it still doesn't work.
Comment 4 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-08 09:11:25 PDT
Created attachment 631423 [details] [diff] [review]
WIP3

Corrected the rule definitions to be just that, not assignments.
Comment 5 Eitan Isaacson [:eeejay] 2012-06-08 09:25:37 PDT
Comment on attachment 631339 [details] [diff] [review]
WIP patch, not yet tested

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

Good start! This is what I had in mind too. Maybe it is time to factor out all the traversal rules to their own module.

As for the heading rules for 1-6, I would have one rule with a constructor that takes a heading level as an argument. And then I would do something like this in the new module:

var Heading1TraveralRule = new HeadingTraversalRule(1);
...

And export them all.

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +46,5 @@
>  
>      switch (aEvent.keyCode) {
> +      case 0:
> +        // an alphanumeric key was pressed, handle it separately.
> +        handleQuickNavKeys(aEvent);

This should return if we are in a text field so we don't consume keys when users are entering text. See case aEvent.DOM_VK_RIGHT.

@@ +114,5 @@
> +
> +    switch (aEvent.charCode) {
> +      case 97: // a
> +        this.moveForward(document, false, this.AnchorTraversalRule);
> +        break;

I would have a hashmap of keys as keys and traversal rules as values. Would be easier to read and remap at runtime.

@@ +367,5 @@
>        Ci.nsIAccessibleRole.ROLE_ENTRY
>      ]
> +  },
> +
> +  AnchorTraversalRule =

Broken JS. It needs to be AnchorTraversalRule: you can't assign in an object definition.

@@ +618,5 @@
> +
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIAccessibleTraversalRule])
> +  },
> +
> +  RadioButtonTraversalRule =

Having separate rules for buttons, check boxes, radio buttons, and combo boxes seems excessive to me, even if NVDA does that too. Does iOS do that? I am concerned about the day when we map this into a touch interface with limited gestures. I am wondering if we need more usability feedback for rules with higher granularity than "form controls".

In other words, I don't want to crowd in too many features and later have to pull some of them out.

Just thinking out loud.
Comment 6 Eitan Isaacson [:eeejay] 2012-06-08 09:27:15 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #5)
> Comment on attachment 631339 [details] [diff] [review]
> WIP patch, not yet tested
> 
> Review of attachment 631339 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +367,5 @@
> >        Ci.nsIAccessibleRole.ROLE_ENTRY
> >      ]
> > +  },
> > +
> > +  AnchorTraversalRule =
> 
> Broken JS. It needs to be AnchorTraversalRule: you can't assign in an object
> definition.
> 

Oops, commented on obsolete patch. Disregard this part.
Comment 7 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-08 11:47:55 PDT
Created attachment 631488 [details] [diff] [review]
WIP with different technique

Eitan, is this what you had in mind?
Comment 8 Eitan Isaacson [:eeejay] 2012-06-08 11:54:58 PDT
Comment on attachment 631488 [details] [diff] [review]
WIP with different technique

This introduces DOS newlines, and makes it hard for review :(
But generally, yes. I would factor out all traversal rules to their own module file as well.
Comment 9 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-08 23:22:36 PDT
Created attachment 631618 [details] [diff] [review]
WIP4: New approach with proper line endings

Sorry for the mess! Guess we'll have to thank gjsfixstyle for that. :( This is just the current patch with the line endings fixed so you can see the actual diffs more easily. I'll follow your suggestion and create a quickNav.jsm file next and factor the map and rules out.
Comment 10 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-09 09:09:54 PDT
Comment on attachment 631618 [details] [diff] [review]
WIP4: New approach with proper line endings

New patch coming soon. I turned the rules in the keyMap array into strings, I think otherwise the addressing from the other module won't work. I also fixed the states thanks to a hint from Surkov.
Comment 11 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-09 21:53:07 PDT
Created attachment 631719 [details] [diff] [review]
What I currently have, doesn't work

This *should* be according to what we discussed on IRC on Friday. But it doesn't work, the whole virtual cursor goes dead. Eitan, do you see why?

BTW: The first approach with this modularized form was that I kept the "this.AnchorTraversalRule" etc. as they were in the original with the case statement. After that didn't work, I took this stringified approach, but it also doesn't work.
Comment 12 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-11 05:25:28 PDT
Created attachment 631862 [details] [diff] [review]
Patch which should, in theory, be working

But it doesn't. A similarly structured testcase using these techniques works fine on the desktop, but both a self-built Fennec and a try-server build completely break virtual cursor nav. jslint doesn't give me any syntax errors.
Comment 13 Eitan Isaacson [:eeejay] 2012-06-11 09:48:13 PDT
Comment on attachment 631862 [details] [diff] [review]
Patch which should, in theory, be working

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

Nice! It works if you add:

const Cu = Components.utils;
Cu.import('resource://gre/modules/XPCOMUtils.jsm');

Have some questions, issues and suggestions below, once they are addressed this is ready.

::: accessible/src/jsat/QuickNavigation.jsm
@@ +7,5 @@
> +const Ci = Components.interfaces;
> +
> +var EXPORTED_SYMBOLS = ['QuickNavigation'];
> +
> +var QuickNavigation = {

Originally when I suggested factoring it out, I thought of taking all the traversal rules (including SimpleStraversalRule) and putting them as individual objects in a module of their own. I still think that is not a bad idea.

An alternative would be encapsulating them in one separate object like you did here (QuickNavigation), but keeping them in the VirtualCursorController file. But I don't think both are necessary.

I don't have a strong opinion either way, but I do think that now that there is more than one traversal rule, they all need to be bunched up in one place and separate from the vc controller object.

@@ +8,5 @@
> +
> +var EXPORTED_SYMBOLS = ['QuickNavigation'];
> +
> +var QuickNavigation = {
> +  keyMap: {

The keymap belongs in the VirtualCursorController object.

@@ +9,5 @@
> +var EXPORTED_SYMBOLS = ['QuickNavigation'];
> +
> +var QuickNavigation = {
> +  keyMap: {
> +    a: ['moveForward', 'AnchorTraversalRule'],

If you take this out of this object, the traversal rule could be a direct reference again.

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +49,5 @@
>      switch (aEvent.keyCode) {
> +      case 0:
> +        // an alphanumeric key was pressed, handle it separately.
> +        if (this._isEditableText(target))
> +          return;

I would also return if a modifier besides shift was pressed. In case the user is in a webapp that captures key shortcuts.

On a side note, maybe we should have a rerouting key. In other words a key like meta that when pressed sends the key event to the document (after stripping the modifier from the event). This would be useful for webapps that use the keys we are capturing here.

@@ +53,5 @@
> +          return;
> +
> +        let key = String.fromCharCode(aEvent.charCode);
> +        if (!(key in QuickNavigation.keyMap))
> +          return;

You are doing the lookup in keyMap twice, maybe there is a way to reorder this to only have the lookup happen once?

@@ +129,4 @@
>      return this.NOT_EDITABLE;
>    },
>  
> +  moveForward: function moveForward(document, last, rule) {

Since you touched this, do you mind changing these arguments to aDocument, aLast, aRule? Same with moveBackward.

@@ +139,4 @@
>        } catch (x) {
> +        if (!rule)
> +          this.moveCursorToObject(
> +            gAccRetrieval.getAccessibleFor(document.activeElement));

This should only happen when there is no rule argument? Why not pass the rule to moveCursorToObject? It takes a second argument.

@@ +177,5 @@
>        let objX = {}, objY = {}, objW = {}, objH = {};
>        acc.getBounds(objX, objY, objW, objH);
>  
> +      let x = Math.round((objX.value - docX.value) + objW.value / 2);
> +      let y = Math.round((objY.value - docY.value) + objH.value / 2);

Did a linter do this? It is unrelated to the patch, but I welcome whitespace fixes :)
Comment 14 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-12 03:50:29 PDT
Created attachment 632194 [details] [diff] [review]
Patch

This patch addresses all comments, implements a fall-through for when the ctrl or alt keys are pressed, and generates an event if one of the alphanumeric keys is pressed with the meta key, but filters the meta key out so the website sees the unmodified version.

I had to move the TraversalRules object to the top of the file, that's why the whole simple traversal rule is in the diff once as an addition and once as a removal. Note that I changed the return of the matchRoles function in TraversalRules.Simple to return the length of the value rather than the length of the constant array for consistency. Yes, I know it's the same, but the others return the value length, too, and I wanted to be consistent.

This patch works, and is blazingly fast on my systems. :)
Comment 15 Eitan Isaacson [:eeejay] 2012-06-12 10:09:14 PDT
Comment on attachment 632194 [details] [diff] [review]
Patch

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

This looks great. Please take out the passthrough bits, we should figure that out in a separate bug.

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +17,4 @@
>  var gAccRetrieval = Cc['@mozilla.org/accessibleRetrieval;1'].
>    getService(Ci.nsIAccessibleRetrieval);
>  
> +var TraversalRules = {

I would move this below VirtualCursorController.

@@ +447,5 @@
> +                                      aEvent.keyCode, aEvent.charCode);
> +
> +          document.documentElement.dispatchEvent(generatedEvent);
> +          return;
> +        }

Thanks for giving this a shot. I am not sure if it is correct. I would prefer seeing this as a separate patch. Concerns I have with this implementation:
1. It only allows pass-through for alphanumeric keys. It should work for arrows and enter as well.
2. You could probably change the event argument and it will continue to the document. I don't think there is a need to dispatch a new event. And if there was, the propagation of this event would need to be stopped and its default would need to be prevented.
3. I am not sure if meta is a common key in most slider phones. I know I threw it out there earlier, but I don't see it on my Droid 2. Modifiers seem to be sparse on mobile keyboards so we may need to do a staggered passthrough. In other words the user presses a passthrough key, and then afterwards presses the key for the web app.
Comment 16 Marco Zehe (:MarcoZ) on PTO until August 15 2012-06-12 10:35:34 PDT
Landed with the PassThrough code removed and the metaKey being now part of the regular fall-through:
http://hg.mozilla.org/integration/mozilla-inbound/rev/eac2f63bc414
Comment 17 Matt Brubeck (:mbrubeck) 2012-06-12 18:32:18 PDT
https://hg.mozilla.org/mozilla-central/rev/eac2f63bc414

Note You need to log in before you can comment on or make changes to this bug.