698437 - Allow navigation with directional controller

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Eitan Isaacson [:eeejay]]

Currently the arrow keys pan the web view. The expected behavior in android is to directionally navigate to focusable items on the page, as described in the Android accessibility guide[1].

I think there is room for interpretation. By default, the behavior should be similar to tab focus on desktop. When accessibility services on android are turned on, i think the behavior should be similar to caret mode.

1. http://developer.android.com/guide/practices/design/accessibility.html#Navigation

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

It should be noted that we disabled this feature (actually removed the code for it). I beleive the rational was that most recently released devices had no directional controls. But, given that we're targeting a lower end of the device spectrum with the native UI, I think it becomes more important

Comment 2

[Eitan Isaacson [:eeejay]]

It should also be noted that currently Android accessibility depends on a directional controller, which is really not ideal. For devices don't have a hardware controller there is a software d-pad that comes up like the on screen keyboard and uses flick gestures. So basically the user is required to flick up/down/right/left on the bottom third of the screen to control the UI in the top two thirds.

Comment 3

[Justin Wood (:Callek)]

just for posterity, I pointed at this bug by accident on a checkin see: https://bugzilla.mozilla.org/show_bug.cgi?id=689437#c16

(cset http://hg.mozilla.org/comm-central/rev/7814c0f2732d for searching for future)

Comment 4

[Eitan Isaacson [:eeejay]]

I am starting to doubt how much this is really a bug, and what the default d-pad behaviour should be. I think it makes more sense to have the d-pad scroll like it currently does. The part that is missing is directional controller access to the chrome.

I think once the user scrolls all the way up, and presses up again the focus should be on the awesomebar. I have been looking at the current implementation, and it seems to be tough to solve. You need to figure out that an "up" keypress has not been consumed by any listeners, and that it did not result in a scroll-up. Only then should the LayerView relinquish focus in favor of the top awesomebar.

Comment 5

[Matt Brubeck (:mbrubeck)]

This is something we need to do, but it does not block the native 1.0 release.  (Notably, XUL fennec does not support this either.)

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

Brad believes this is needed for Google TV devices. Do we have a specific target or is plus good kar?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Carrying dependency chain from bug 849850. Note that there used to be code for this in toolkit/ somewhere but was deleted in bug 670880.

Comment 9

[Mina Almasry]

Attachment: WIP patch

So here is my WIP.

So far, pressing the directional keys just focuses and then skips the select elements, so the user is not stuck in them.

I am considering sending the window a mouse event on whatever is focused whenever the user hits O.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 808743 [details] [diff] [review]
WIP patch

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

SpatialNavigation.jsm needs a lot of formatting fixes. Try to follow the coding style guidelines at https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style as much as possible, specially since this is a new file so you don't need to be consistent with existing code. Definitely on the right track though.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(As discussed on IRC you probably want to call .click() on the focused element when the user hits the action key).

Comment 12

[Mina Almasry]

Attachment: WIP v2

Here is the updated patch.

Now pressing the directional keys goes from element to element, and pressing A fires a click event on whatever element is in focus.

A doesn't have an equivalent in the DOM, so I used the DOM_VK_RETURN keycode.

I left the logic that finds a new element to focus mostly unchanged so far.

I am mainly submitting for review to get feedback on the Preference stuff. I followed the lead of the code that was already there but I'm not sure I got it all right.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 808850 [details] [diff] [review]
WIP v2

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

(In reply to Mina Almasry [:mina] from comment #12)
> I am mainly submitting for review to get feedback on the Preference stuff. I
> followed the lead of the code that was already there but I'm not sure I got
> it all right.

I have some comments on this near the bottom of the review. Note that if you're only requesting feedback on a part of the patch rather than a full review you should use the feedback? flag rather than the review? flag. I did go through most of the patch anyway and pointed out a few examples of things (mostly cosmetic) that should be fixed for the final version.

::: mobile/android/chrome/content/browser.js
@@ +4103,5 @@
>      BrowserApp.deck.addEventListener("touchstart", this, true);
>      BrowserApp.deck.addEventListener("click", InputWidgetHelper, true);
>      BrowserApp.deck.addEventListener("click", SelectHelper, true);
> +
> +    SpatialNavigation.init(BrowserApp.deck, null);

Something to consider is to only import this and run this code if the device has a d-pad. We can probably determine this in Java using the Android APIs and pass that info down to browser.js pretty easily. Or we could query it through nsIAndroidBridge and implement it as a JNI-invoked function in Java.

::: toolkit/modules/SpatialNavigation.jsm
@@ +1,2 @@
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Update the license block to the MPL 2.0. You can find it at the top of browser.js or any other file in the tree. You don't need to keep any part of the existing license block.

@@ +71,5 @@
> +  init: function(browser, callback) {
> +          eventListenerService.addSystemEventListener(browser, "keydown",
> +              function (event) {
> +                _onInputKeyPress(event, callback);
> +              }, true);

Fix indentation

@@ +93,5 @@
> +  //Use whatever key value is available (either keyCode or charCode).
> +  //It might be useful for addons or whoever wants to set different
> +  //key to be used here (e.g. "a", "F1", "arrowUp", ...).
> +  var key = event.which || event.keyCode;
> +  dump("the key is " + key);

Remove the dump call for the final patch

@@ +97,5 @@
> +  dump("the key is " + key);
> +
> +  if (key == PrefObserver['keyCodeA']) {
> +    if (focusedElem) {
> +      focusedElem.click();

1. I would change the condition to (focusedElem && focusedElem.click) in case the element doesn't have a click function (I don't think all elements have a click function)
2. You can also return after calling click()

@@ +105,5 @@
> +  if (key != PrefObserver['keyCodeDown']  &&
> +      key != PrefObserver['keyCodeRight'] &&
> +      key != PrefObserver['keyCodeUp'] &&
> +      key != PrefObserver['keyCodeLeft'])
> +    return;

Here and everywhere else: braces around the body of the if condition

@@ +108,5 @@
> +      key != PrefObserver['keyCodeLeft'])
> +    return;
> +
> +   //If it is not using the modifiers it should, bail.
> +  if (!event.altKey && PrefObserver['modifierAlt'])

Line up the comment with the code (here and everywhere else)

@@ +156,5 @@
> +        if (target.textLength != target.selectionEnd)
> +          return;
> +      }
> +      else
> +      {

cuddle the else between the } {

@@ +462,5 @@
> +  },
> +
> +  observe: function(aSubject, aTopic, aData)
> +  {
> +    if(aTopic != "nsPref:changed")

nit: space before (

@@ +546,5 @@
> +        }
> +        break;
> +      case "keyCode.enter":
> +        try {
> +          this.keyCodeA = this._branch.getIntPref("keyCode.A");

The parameter you pass to getIntPref should match whatever the case statement value is. Both of these strings are the name of the pref that is being used. So if you look at keyCode.right as an example, the full pref name is snav.keyCode.right. All the operations (listening for events and getting prefs) are done on the _branch, which is already initialized with the "snav." prefix. The aData argument to this function will be the name of the pref that is being changed within the branch, so that would be "keyCode.right". And so the pref that needs to be read is also "keyCode.right". With the code you added, you will be listening for the "keyCode.enter" pref changes, but using the value of the "keyCode.A" pref, which is not really what you want. I would suggest using keyCode.enter in both places, and also renaming the "this.keyCodeA" variable to "this.keyCodeEnter" for consistency.

::: widget/android/nsWindow.cpp
@@ +1425,5 @@
>          case AKEYCODE_RO:                 return NS_VK_BACK_SLASH; // Same as other platforms.
>          case AKEYCODE_KANA:               return NS_VK_KANA;
>          case AKEYCODE_ASSIST:             return NS_VK_HELP;
>  
> +        // the A key is the action key for the OUYA controller

No need to make this comment specific to the OUYA controller; just say it is the action key for gamepad devices.

Comment 14

[Mina Almasry]

Attachment: patch v1

Patch! Ready for a full review.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 812894 [details] [diff] [review]
patch v1

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

Looks pretty good! I have just some styling nits below. Also update the commit message so it doesn't have the message in there twice.

::: toolkit/modules/SpatialNavigation.jsm
@@ +25,5 @@
> +  init: function(browser, callback) {
> +          eventListenerService.addSystemEventListener(browser, "keydown",
> +              function (event) {
> +                _onInputKeyPress(event, callback);
> +              }, true);

Fix indentation here

@@ +80,5 @@
> +  // key to be used here (e.g. "a", "F1", "arrowUp", ...).
> +  var key = event.which || event.keyCode;
> +
> +  if (key == PrefObserver['keyCodeReturn']) {
> +    if (event && event.target && event.target.click) {

You can remove "event" from the condition check here, it should always be non-null (and in fact you assume that later in the function).

@@ +113,5 @@
> +    focusManager.moveFocus(currentlyFocusedWindow, null, focusManager.MOVEFOCUS_FIRST, 0);
> +    event.stopPropagation();
> +    event.preventDefault();
> +    return;
> +  } else {

Our style guide says to avoid else-after-return. You can delete the else and un-indent all the code inside the else block.

@@ +125,5 @@
> +
> +    let searchRectOverflows = false;
> +
> +    while (!bestElementToFocus && !searchRectOverflows) {
> +      switch(key) {

nit: space before "(" (a bunch of other switch statements below have this too)

@@ +152,5 @@
> +        case PrefObserver['keyCodeLeft']:
> +        case PrefObserver['keyCodeRight']: {
> +          searchRect.top = searchRect.top - searchRect.height / 2;
> +          searchRect.height = searchRect.height * 2;
> +          searchRect.bottom = searchRect.top + searchRect.height;

For clarity I would move the height assignment below the bottom assignment, and change the bottom assignment to use height / 2. Same for the width one below

@@ +167,5 @@
> +      bestElementToFocus = _getBestToFocus(nodes, key, currentlyFocused);
> +    }
> +  }
> +
> +  if (bestElementToFocus !== null) {

I would invert this condition and do an early return, so you don't unnecessarily indent the rest of the code in this function.

@@ +267,5 @@
> +    if (curDist > bestDist) {
> +      continue;
> +    }
> +
> +    nodeMid = _getMidpoint(nodes[i]);

You already assigned nodeMid above using the same value so no need to repeat it

@@ +313,5 @@
> +// Returns true if the node is a type that we want to focus, false otherwise.
> +function _canFocus(node) {
> +  if (node instanceof Ci.nsIDOMHTMLLinkElement ||
> +      node instanceof Ci.nsIDOMHTMLAnchorElement) {
> +    return  true;

nit: one space between "return  true"

@@ +320,5 @@
> +        node instanceof Ci.nsIDOMHTMLInputElement ||
> +        node instanceof Ci.nsIDOMHTMLLinkElement ||
> +        node instanceof Ci.nsIDOMHTMLOptGroupElement ||
> +        node instanceof Ci.nsIDOMHTMLSelectElement ||
> +        node.onClick ||

Instead of .onClick you'll need to check with the event listener service, similar to the code at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4889. onClick won't return nodes which have listeners registered using addEventListener.

@@ +396,5 @@
> +    this.observe(null, "nsPref:changed", "keyCode.return");
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    if(aTopic != "nsPref:changed") {

nit: space before "("

@@ +421,5 @@
> +        break;
> +
> +      case "keyCode.modifier":
> +        try {
> +          this.keyCodeModifier = this._branch.getCharPref("keyCode.modifier");

Make keyCodeModifier a local var using "let". It's not used anywhere outside of this case block.

@@ +429,5 @@
> +          this.modifierShift = false;
> +          this.modifierCtrl = false;
> +
> +          if (this.keyCodeModifier != this.kNone) {
> +            // use are using '+' as a separator in about:config.

s/use/we/

@@ +483,5 @@
> +      case "keyCode.return":
> +        try {
> +          this.keyCodeReturn = this._branch.getIntPref("keyCode.return");
> +        } catch(e) {
> +          this.keyCodeReturn = Ci.nsIDOMKeyEvent.DOM_VK_RETURN;

Does this work now? Did you figure out why it wasn't working before?

Comment 16

[Mina Almasry]

Attachment: patch v2

Comment 17

[Mina Almasry]

Answering a few questions and making note of things. Kats if you're fine with everything here I'll mark checkin-needed:

> > +        node.onClick ||
> 
> Instead of .onClick you'll need to check with the event listener service,
> similar to the code at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#4889. onClick won't return nodes which have listeners registered
> using addEventListener.

I did that, but then I ran into a problem that some nodes have an onClick event but don't seem to accept focus (nsIFocusManager.setFocus emits a warning to the log). I tried to check if an element accepts focus, but nsIFocusManager.elementIsFocusable doesn't seem to work, and it's not referenced anywhere in the tree including tests. I'll file a bug on that.

> > +      case "keyCode.return":
> > +        try {
> > +          this.keyCodeReturn = this._branch.getIntPref("keyCode.return");
> > +        } catch(e) {
> > +          this.keyCodeReturn = Ci.nsIDOMKeyEvent.DOM_VK_RETURN;
> 
> Does this work now? Did you figure out why it wasn't working before?

Yes. The |this.observe(null, "nsPref:changed", "keyCode.return")| line was wrong before.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 813587 [details] [diff] [review]
patch v2

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

Took a quick look at the revisions - it looks good, just a few minor comments:

::: toolkit/modules/SpatialNavigation.jsm
@@ +174,5 @@
> +    // Couldn't find an element to focus.
> +    return;
> +  }
> +
> +  dumpNodeCoord("bestElementToFocus is ", bestElementToFocus);

Take out the debug logging

@@ +191,5 @@
> +  }
> +
> +  event.preventDefault();
> +  event.stopPropagation();
> +  return;

No need for the return statement here, this is the end of the function anyway.

@@ +451,5 @@
> +              }
> +            }
> +          }
> +        } catch(e) {
> +          keyCodeModifier = kNone;

Note: you don't need to bother setting keyCodeModifier to kNone ever since it's not used anywhere else after this point.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mina Almasry [:mina] from comment #17)
> > Instead of .onClick you'll need to check with the event listener service,
> > similar to the code at
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> > browser.js#4889. onClick won't return nodes which have listeners registered
> > using addEventListener.
> 
> I did that, but then I ran into a problem that some nodes have an onClick
> event but don't seem to accept focus (nsIFocusManager.setFocus emits a
> warning to the log). I tried to check if an element accepts focus, but
> nsIFocusManager.elementIsFocusable doesn't seem to work, and it's not
> referenced anywhere in the tree including tests. I'll file a bug on that.

Ok, that makes sense.

> > > +      case "keyCode.return":
> > > +        try {
> > > +          this.keyCodeReturn = this._branch.getIntPref("keyCode.return");
> > > +        } catch(e) {
> > > +          this.keyCodeReturn = Ci.nsIDOMKeyEvent.DOM_VK_RETURN;
> > 
> > Does this work now? Did you figure out why it wasn't working before?
> 
> Yes. The |this.observe(null, "nsPref:changed", "keyCode.return")| line was
> wrong before.

Cool, thanks. Feel free to mark it checkin-needed after addressing the three things above.

Comment 20

[Mina Almasry]

Oh, all other comments are addressed in patch v2.

Comment 21

[Mina Almasry]

Attachment: patch v3

This patch is checkin-needed

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f23136a1fedf

Comment 23

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out because I suspect it caused failures in several mochitests here:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=858e4ce84c04&jobname=android.*mochitest

Comment 24

[Mina Almasry]

Attachment: Patch v4

Fixed test failures and added a test.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=66014027bcbc

Comment 25

[Mina Almasry]

Attachment: Patch v5

Fixed the pref stuff.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 815866 [details] [diff] [review]
Patch v5

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

r=me with the comments addressed.

::: modules/libpref/src/init/all.js
@@ +4467,5 @@
>  pref("urlclassifier.phish_table", "goog-phish-shavar");
>  pref("urlclassifier.download_block_table", "goog-badbinurl-shavar");
>  pref("urlclassifier.download_allow_table", "goog-downloadwhite-digest256");
> +
> +// Turn of Spatial navigation by default.

"off"

::: toolkit/modules/SpatialNavigation.jsm
@@ +92,5 @@
> +    // key to the DOM. The behaviour of hitting the return key and clicking an
> +    // element is the same for some elements, but not all, so we handle the
> +    // ones we want (like the Select element) here:
> +    if (event.target instanceof Ci.nsIDOMHTMLSelectElement &&
> +        event.target &&

Don't need to check "event.target" if you're checking it as part of an instanceof on the line above.

::: toolkit/modules/tests/moz.build
@@ +9,5 @@
>  MODULE = 'test_toolkit_general'
>  
>  XPCSHELL_TESTS_MANIFESTS += ['xpcshell/xpcshell.ini']
> +
> +MOCHITEST_MANIFESTS += ['mochitest.ini']

ted said on IRC that's better to move the mochitest.ini and test_spatial_navigation.html files into toolkit/modules/tests/mochitest/ and update this to say mochitest/mochitest.ini.

Comment 27

[Mina Almasry]

Attachment: patch v6

changes made

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/972d7d7be035

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/972d7d7be035

Comment 30

[Mina Almasry]

I'd like to document a few things in case Spatial Navigation gets revisited and it needs to be reimplemented.

In this bug we implemented spatial navigation in JS using hit tests, but I suspect the correct way to do it would be to implement it in layout, where you have a display list at your disposal, which could be a little more flexible.

The way to go about that is to buid a display list (or use a valid one if there's one you could use) and then transverse all the rectangles in the list and switch focus to the best rectangle for the given keypress.

Something similar happens at (nsLayoutUtils::GetFramesForArea) for hit tests; except hit tests return all rectangles that intersect the given rectangle.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1911

This is what webkit does:

https://bug-18662-attachments.webkit.org/attachment.cgi?id=49483