1114536 - Support basic spatial navigation for web pages on non-touch device

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Evelyn Hung]

For devices using arrows keys or directional gesture to control focused element, we need a simple logic in Gecko to deal with navigation for web pages or applications. Navigating according to elements' spatial location is intuitive to user, so the focus can be easily move to the nearest element of the current one by the direction given by user.

Comment 1

[Evelyn Hung]

Writing down on offline discussion with Jeff.

For elements which want to be focusable, adding tabIndex as a hint to Gecko, so spatial navigation calculation algorithm will take them into account for a best match.

Comment 2

[Marco Chen [:mchen]]

One issue we came out during the discussion with Partner is 
  "How to access the XUL components of control panel inside video element".

  1. nsIFocusManager::elementIsFocusable(...) can be used to check focus so maybe we avoid to check possible elements one by one like what we are doing now. [1]

  2. In current version, video element can be focusable so it can be set focus by focus manager.

  3. Then the space and left/right can be used to control play/pause and seek.

  4. One issue is that once control panel is hidden (be shown by touch event / hover), then there is no way to show up the panel then access it.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/SpatialNavigation.jsm?from=spatialnavigation.jsm#331

Comment 3

[Marco Chen [:mchen]]

And the description [1] here shows basically XUL element can be focusable or tabable depent on tabindex.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/xul/nsXULElement.cpp?from=nsxulelement.cpp#535

Comment 4

[Changbin Park]

Attachment: [WIP] SpatialNavigationB2G.patch

I'm attaching a patch of SpatialNavigationB2G that is in WIP to share current status.

Comment 5

[Changbin Park]

Attachment: [WIP]SpatialNavigationB2G.patch

I just know the patch uploaded last time was corrupted.
I'm reattaching the patch.

Comment 6

[Changbin Park]

Hi Evelyn.
I'm wondering who can review patch "[WIP]SpatialNavigationB2G.patch".
Is there anyone you recommend?

It's not final patch but I want to get review and opinions on that.

Comment 7

[Evelyn Hung]

Comment on attachment 8592596 [details] [diff] [review]
[WIP]SpatialNavigationB2G.patch

Kershaw, would you like to take a look first and give some feedback? Thanks!

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Evelyn Hung [:evelyn] from comment #7)
> Comment on attachment 8592596 [details] [diff] [review]
> [WIP]SpatialNavigationB2G.patch
> 
> Kershaw, would you like to take a look first and give some feedback? Thanks!

I've already started looking into this patch. I'll provide my comments before the end of this week.
Thanks.

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Changbin Park from comment #4)
> Created attachment 8591504 [details] [diff] [review]
> [WIP] SpatialNavigationB2G.patch
> 
> I'm attaching a patch of SpatialNavigationB2G that is in WIP to share
> current status.

Hi Changbin,
Could you please briefly introduce the basic idea of you patch?
I also want to know what issues that your patch are addressed what issues are not. For example, does your patch solve all cross-iframe cases?
Thanks.

Comment 10

[Changbin Park]

Hi Kershaw,

 SpatialNavigationB2G has been evolved from SpatialNavigation.jsm to support it on B2G. As you may know SpatialNavigation is used only for Firefox for Android. To use it on B2G, it must support inter-frame and inter-process problem since E10S. So, we cloned it as SpatialNavigationB2G and has made changes.

 SpatialNavigationB2G has modules and addons. Consider modules are necessary and addons are optional functions. FYI, I have few more addons net necessary yet. While it's still in WIP, if there's any unclear, please let me know.

1. Preferences
  * b2g/app/b2g.js
    - Preferences used for SpatialNavigationB2G and its modules/addons

2. Loading
  * b2g/chrome/content/shell.js
    - Load SpatialNavigationB2G into parent process

  * dom/browser-element/BrowserElementChild.js
    - Load SpatialNavigationB2G into chlid process

3. Main Module
  * toolkit/components/spatialnavigation/SpatialNavigationB2G.jsm
    - Begin with 'keydown' event handler at capturing phase
    - loaded only once per process
    - Call click() of focused element on Ok key.
    - Inter-iframe focus move via docshell
    - Inter-process focus move via IPC(ppmm/cpmm)
    - Scroll when following cases
      # Just focused element is out of viewport
      # There's a scrollable element between currently focused element and next
         focusable element
    - Finding next focusable element in two steps, i.e. narrow to wide
    - Offer hook points for Addons

4. Submodules
 Submodules offers neccessary functionalities for SpatialNavigationB2G and its
 addons.
  * toolkit/components/spatialnavigation/modules/Addon.jsm
    - Make addons working
    - Manage callbacks
  * toolkit/components/spatialnavigation/modules/DirectedFocusRect.jsm
    - Abstracting calculation of coordinates and directions
  * toolkit/components/spatialnavigation/modules/PrefObserver.jsm
    - Cache preferences values
    - Notify given preference change as callback
      e.g. PrefObserver['key-to-watch'] = function(value_of_key_to_watch) {}
  * toolkit/components/spatialnavigation/modules/Scroll.jsm
    - Handle scrolls
  * toolkit/components/spatialnavigation/modules/StyleSheetsManager.jsm
    - Register/Unregister given css as author or user sheet

5. Addons
 Basically, "Addons" are not necessary for spatialnavigation. But, it could
 offer better UX.
  * toolkit/components/spatialnavigation/res/spatialnavigation.css
   - Style definition for focused element

  * toolkit/components/spatialnavigation/addons/StyleFocused.jsm
   - Apply spatialnavigation.css as author sheet via StyleSheetsManager.jsm

  * toolkit/components/spatialnavigation/addons/StyleShadowDOM.jsm
   - Force spatialnavigation.css for an element within shadow DOM

  * toolkit/components/spatialnavigation/addons/TextField.jsm
   - From legacy SpatialNavigation
   - Just refactored as addon

Comment 11

[Kershaw Chang [:kershaw]]

Comment on attachment 8592596 [details] [diff] [review]
[WIP]SpatialNavigationB2G.patch

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

Overall it looks good to me. However, I still found that sometimes the focus is missing or is set to an invisible element in my test.
I am not sure whether your patch is able to handle all kinds of iframe case. It would be great to have some test cases to verify.

::: b2g/chrome/content/shell.js
@@ +66,5 @@
> +try {
> +  Cu.import('resource://gre/modules/SpatialNavigationB2G.jsm');
> +} catch (e) {
> +  dump('SpatialNavigation : import failed - '  + e + '\n');
> +}

Why we need this try catch?
In what case does it fail?

@@ +276,4 @@
>            cr.annotateCrashReport("B2G_OS_Version", value);
>          } catch(e) { }
>        });
> +      SpatialNavigationB2G.init(window, null, true);

If failed to import the jsm, we cannot use SpatialNavigationB2G here.

::: toolkit/components/spatialnavigation/SpatialNavigationB2G.jsm
@@ +21,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["SpatialNavigationB2G"];
> +
> +this.SpatialNavigationB2G = {
> +  init: function(browser, callback, force) {

forceToRunInParentProcess would be clearer.

@@ +24,5 @@
> +this.SpatialNavigationB2G = {
> +  init: function(browser, callback, force) {
> +
> +    // Do not initialize this module if it is running on ParentProcess
> +    // unless the force value is true.

Is this module supposed not to run in parent process? Please explain.

@@ +42,5 @@
> +  },
> +  enabled : function() {
> +    return PrefObserver['enabled'];
> +  },
> +  canFocus : _canFocus,

Why export canFocus and enabled? It seems these are not used.

@@ +77,5 @@
> +      _consoleWin = getDocShellItem(focusManager.activeWindow)
> +                        .rootTreeItem
> +                        .QueryInterface(Ci.nsIDocShell)
> +                        .contentViewer.DOMDocument
> +                        .defaultView;

nit: Always brace controlled statements, even a single-line consequent of an if.

@@ +97,5 @@
> +Cu.import("resource://gre/modules/spatialnavigation/Scroll.jsm");
> +Cu.import("resource://gre/modules/spatialnavigation/Addon.jsm");
> +Cu.import("resource://gre/modules/spatialnavigation/StyleSheetsManager.jsm");
> +const { DIR_UNKNOWN, DIR_LEFT, DIR_RIGHT, DIR_UP, DIR_DOWN }
> +    = DirectedFocusRect.prototype;

Can you also export these constants in DirectedFocusRect.jsm? So you don't have to declare again here.

@@ +134,5 @@
> +    let document = elm.contentDocument;
> +
> +    if (document && document.designMode !== 'on') {
> +      if (document.activeElement !== document.body) {
> +        _setFocusTo(document.activeElement);

It is possible that the activeElement is not most close to the previous focused element.
Is this the expected behavior?

@@ +137,5 @@
> +      if (document.activeElement !== document.body) {
> +        _setFocusTo(document.activeElement);
> +      } else {
> +        // document doesn't have focused element;
> +        let e = searchFirstElementToFocus(elm.contentWindow, getStartRect());

In my test, sometimes I see that the element from searchFirstElementToFocus is not most close to aFromRect.
Could you please check?

@@ +152,5 @@
> + * @return {[type]} [description]
> + */
> +function _registerListenerOnParent() {
> +  let ppmm = Cc['@mozilla.org/parentprocessmessagemanager;1']
> +            .getService(Ci.nsIMessageBroadcaster);

It would be better to use frame message manager rather than ppmm and cpmm, because it is possible that the embedder iframe is in another content process. Details about this case can be found in https://wiki.mozilla.org/Nested_Content_Processes.
To do this, you may also need to import SpatialNavigationB2G.jsm in BrowserElementParent.js.

@@ +202,5 @@
> + * @param  {String} aType
> + * @param  {JSON - Data} aDtata
> + * @param  {JSON - Objects} aObject
> + */
> +function sendMessageToParent(...a) {

...args would be more clearer.

@@ +206,5 @@
> +function sendMessageToParent(...a) {
> +  cpmm.sendAsyncMessage(...a);
> +}
> +
> +function sendMessageToChild(aIFrame, ...a) {

ditto.

@@ +242,5 @@
> +
> +    let docShellItem = getDocShellItem(window);
> +    while (docShellItem = docShellItem.parent) {
> +      yield docShellItem.QueryInterface(Ci.nsIDocShell)
> +                        .contentViewer.DOMDocument.defaultView;

nit: .contentViewer
     .DOMDocument
     .defaultView;

@@ +295,5 @@
> +      if (isScrollable(elem, direction)) {
> +        if(!isRemoteBrowserFrame(focused)) {
> +          let offsets = scrollOffset(window, direction);
> +          if (offsets) {
> +            scrollBy(elem, ...offsets);

If the focused element is changed to parent window's activeElement, does it make sense to check scroll here?
It looks like the loop of parentElementChain should move out of loop getParentWindowChain.

@@ +384,5 @@
> +                              !aExcludeSet.has(n) && _canFocus(n)));
> +}
> +
> +function isRemoteBrowserFrame(aElem) {
> +  if (isParentProcess &&

As mentioned above, it is possible to have a nested content process in b2g.
So, you may remove isParentProcess.

@@ +432,5 @@
> +  if (!event.altKey && PrefObserver['modifierAlt'] ||
> +      !event.shiftKey && PrefObserver['modifierShift'] ||
> +      !event.crtlKey && PrefObserver['modifierCtrl']) {
> +    return;
> +  }

It seems this check can be moved to the top of this function.

@@ +442,5 @@
> +  if (handledByAddon("begin", event)) return;
> +
> +  if (isRemoteBrowserFrame(focused)) {
> +    return;
> +  }

Can we move this up to return more earlier?

@@ +490,5 @@
> +  if (typeof callback === 'function') {
> +    callback(bestElementToFocus);
> +  }
> +
> +  return;

nit: we don't need this return.

@@ +517,5 @@
> +    return true;
> +  }
> +
> +  // The element's tabindex focus flag is set.
> +  if (!focusManager.elementIsFocusable(node, FLAG_BYMOUSE)) {

Why FLAG_BYMOUSE?

@@ +553,5 @@
> +
> +  let { innerHeight, innerWidth} = aElem.ownerDocument.defaultView;
> +
> +  return new FocusRect(0, 0, innerWidth, innerHeight)
> +                .isCovering(getFocusRect(aElem), aFully?100:1);

nit: add space after '?' and ':'.

::: toolkit/components/spatialnavigation/modules/DirectedFocusRect.jsm
@@ +29,5 @@
> +  }
> +  throw aSource;
> +}
> +
> +this.FocusRect =

Actually, this object has nothing to do with focus. It's basically a helper object for encapsulating the calculation of rectangle.
Would be better to have a more proper name.

::: toolkit/components/spatialnavigation/modules/PrefObserver.jsm
@@ +75,5 @@
> +
> +    return aCache[aKey];
> +  },
> +  set(aCache, aKey, aCallback) {
> +    if (typeof aCallback !== "function") return;

Need to return a boolean.

Comment 12

[Kershaw Chang [:kershaw]]

Comment on attachment 8592596 [details] [diff] [review]
[WIP]SpatialNavigationB2G.patch

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

::: toolkit/components/spatialnavigation/SpatialNavigationB2G.jsm
@@ +112,5 @@
> +  if (!elm) {
> +    console.error("trying to focus null");
> +    return;
> +  }
> +  if (aFromRect && !(aFromRect instanceof DirectedFocusRect)) {

Why we need to check the type of aFromRect?
You always pass the expected parameter, don't you?

@@ +121,5 @@
> +  //dump('Set Focus to', elm/*, 'from', aFromRect*/);
> +  if (handledByAddon("prefocus", elm)) return;
> +  focusManager.setFocus(elm, FLAG_SHOWRING | FLAG_NOSCROLL);
> +  if (handledByAddon("postfocus", elm)) return;
> +  scrollIntoViewIfNeeded(elm);

We should not scroll here. If you do this, elm's position might be changed but aFromRect's position remain the same.

@@ +127,5 @@
> +  function getStartRect() {
> +    return aFromRect ?
> +              aFromRect.getRelativeRectFrom(elm.getBoundingClientRect()) :
> +              kPseudoFocusedRect;
> +  }

getStartRect may create a wrong rectangle if the scroll position is changed.

@@ +141,5 @@
> +        let e = searchFirstElementToFocus(elm.contentWindow, getStartRect());
> +        if (e) _setFocusTo(e);
> +      }
> +    } else if (!document && isRemoteBrowserFrame(elm)) {
> +      sendMessageToChild(elm, getStartRect());

In parent frame, we already set the focus to child frame. If there is no element in child frame can be focused, we may lose the focus.

Comment 13

[Changbin Park]

Attachment: [WIP]SpatialNavigationB2G.patch

I'm attaching new WIP patch that contains what we agreed at last meeting in Seoul.

Comment 14

[Changbin Park]

Attachment: [WIP]SpatialNavigationB2G.pdf

This depicts overall structure of the attachment 8663934 [details] [diff] [review]

Comment 15

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

ni DJF as well

Comment 16

[David Flanagan [:djf]]

Doug Turner suggested that we ask someone from the DOM team to take a look at this patch. I can't set needinfo for Doug, so I don't know who he had in mind, though.

Comment 17

[Rachelle Yang [:ryang][[e-mail]]]

Hi Kershaw,

Would you please kindly help check the patch in comment 13 (with the description of the overall structure  in comment 14)? 

Thank you very much!

Comment 18

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Wesly, per Comment#16, could you help to check if someone from the DOM team can also help to look at this patch?

Comment 19

[Kershaw Chang [:kershaw]]

(In reply to Rachelle Yang [:ryang][[e-mail]] from comment #17)
> Hi Kershaw,
> 
> Would you please kindly help check the patch in comment 13 (with the
> description of the overall structure  in comment 14)? 
> 
> Thank you very much!

I already started looking into this patch. I think I can provide some feedback next week.

Comment 20

[Rachelle Yang [:ryang][[e-mail]]]

Thank you very much, dear Kershaw . Thanks !

Comment 21

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

NI to DOM team to review this patch.

Hi Andrew/Olli,
This work is essential prerequisite for Mozilla's smart feature phone initiative.
Could you assign someone to review this?
Seems to be a big patch, and we do really want it landed asap.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Oh, this lives outside Gecko and is all JS. I'm somewhat surprised (but perhaps it is a good thing that this is a separate thing).
But that also means that technically this doesn't need any DOM review, and I rarely review JS code.
I wonder if Enn had time to look at this. If not, I guess I could take a look, but what kind of review do you actually need/want here?



(random thing I noticed, you probably want to use addSystemEventListener, or capturing event listeners, when dealing with events from a web page. Otherwise event listeners in the page may affect to the event handling.)


Do we have any tests for this?

Comment 23

[Kershaw Chang [:kershaw]]

Comment on attachment 8663934 [details] [diff] [review]
[WIP]SpatialNavigationB2G.patch

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

It looks very good to me.
I think we can let our DOM module peer to have a look.

::: toolkit/components/spatialnavigation/addons/AudioVideoElements.js
@@ +250,5 @@
> +      return false;
> +    }
> +  });
> +
> +  return true;

This is an unreachable return statement.

::: toolkit/components/spatialnavigation/addons/LongKey.js
@@ +14,5 @@
> +
> +Cu.import("resource://spatialnavigation/InputManager.jsm");
> +
> +InputManager.on("userinput:long:direction", (aKind, aDirection) => {
> +  Navigator.moveToward(aDirection);

Can you just call Navigator.moveToward directly when emit userinput:long:direction instead of loading LongKey.js?

::: toolkit/components/spatialnavigation/addons/TextField.js
@@ +13,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "Direction",
> +  "resource://spatialnavigation/Direction.jsm");
> +
> +on("premove",
> +  function TextFieldPreMove(aKind, aFocused, aDirection) {

Is this able to handle a document with designMode and a node's contenteditable is true?

::: toolkit/components/spatialnavigation/modules/Addon.jsm
@@ +67,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "InputManager",
> +  "resource://spatialnavigation/InputManager.jsm");
> +XPCOMUtils.defineLazyGetter(this, "systemPrincipal",
> +  Services.scriptSecurityManager.getSystemPrincipal);
> +XPCOMUtils.defineLazyGetter(this, "loadSubScript", () => 

nit: trailing space

::: toolkit/components/spatialnavigation/modules/DOMInfo.jsm
@@ +186,5 @@
> +                 .DOMDocument
> +                 .defaultView;
> +
> +  if (!aIncludeChromeWindow && window instanceof nsIDOMChromeWindow) {
> +    window = window.content || window.getContentWindow();

I got the error below.
TypeError: window.getContentWindow is not a function.

::: toolkit/components/spatialnavigation/modules/IPC.jsm
@@ +58,5 @@
> +  console.debug(aDirection, aStartRect);
> +  aIFrame.QueryInterface(Ci.nsIFrameLoaderOwner)
> +         .frameLoader
> +         .messageManager
> +         .sendAsyncMessage(SearchMessage.sendToChild, aDirection, aStartRect);

I got a warning at this line.
Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?"

::: toolkit/components/spatialnavigation/modules/InputManager.jsm
@@ +29,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +  "resource://spatialnavigation/Console.jsm");
> +
> +const { EventEmitter } =
> +  Cu.import("resource://gre/modules/devtools/event-emitter.js", {});

I am not sure if it's a good idea to use event-emitter.js. It seems that no one uses this in b2g.

::: toolkit/components/spatialnavigation/modules/Navigator.jsm
@@ +135,5 @@
> +        aDirection = Direction.down;
> +      }
> +
> +      if (aDirection === Direction.up) {
> +        rect.translate(1, 1);

I don't understand why we need this translation.

@@ +204,5 @@
> +    setFocus(bestElement);
> +  } else {
> +    // we don't find any element to focus on Child side
> +    // send a message to parent to keep searching on parent side.
> +    IPC.searchFocusOnParent(aDirection, aStartRect);

We have to consider two cases here:
1. The previous focused element is in child frame and can not find any element.
   Send a message to parent and keep searching.
2. The previous focused element is in parent frame and can not find any element.
   Send a message to parent, but the direction should be reversed. So, the focus can be returned to the previous focused element in parent. Otherwise, the focus is at the child's frame element. Users may fell that the focus is missing.

Comment 24

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi David, 
Per comment 22, and since we're planning to use it for 2.2R.
Would you suggest someone (other than DOM peer) to review?
Usually I don't like to say this but timing is really important for this case.

Comment 25

[Neil Deakin]

If this is only needed for FirefoxOS and you need it checked in quickly, why do you need to add it to toolkit? Why not under b2g for now?

This is a very large patch and would take me a while to review it.

Comment 26

[David Flanagan [:djf]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #24)
> Hi David, 
> Per comment 22, and since we're planning to use it for 2.2R.
> Would you suggest someone (other than DOM peer) to review?
> Usually I don't like to say this but timing is really important for this
> case.

I don't know anything about this area of the platform, so I really don't know who should review it. My suggestion that we get a DOM peer to look at it was based on a suggestion by Doug Turner (who worked on spatial navigation long long ago and knows how hard it is to do well).  If Kershaw is comfortable reviewing it, then that may be sufficient. Since it is gecko code, though, I really don't know.

Neil's point in comment #25 about putting this feature under the b2g/ directory is an excellent one. I think we should do that. 

Olli's point in comment #22 about needing tests is another good point, and if we can't get the original author to write gecko tests for the feature, we should probably write some gaia integration tests to demonstrate that it works.

I'd also really like to see someone who understands this code (the author, or the reviewer) write up a page or two of documentation explaining how it works so that someone like me can understand it well enough to work on it myself. There is no top-level overview comment in the code and the PDF attached to this bug does not help me understand it at all.

Comment 27

[Changbin Park]

Comment on attachment 8663934 [details] [diff] [review]
[WIP]SpatialNavigationB2G.patch

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

Thank you for your feedback Kershaw.
Please check my comments on your feedback.
I'll upload next (fixed) patch, if there's no disagreement.

::: toolkit/components/spatialnavigation/addons/AudioVideoElements.js
@@ +93,5 @@
> +}
> +
> +const xulController = new WeakMap();
> +function prepareXULController(aAudioVideoElem) {
> +  if (xulController.has(aAudioVideoElem)) return true;

To keep consistency, I'll change return value to xulController.get(aAudioVideoElem).

@@ +250,5 @@
> +      return false;
> +    }
> +  });
> +
> +  return true;

Got it.

::: toolkit/components/spatialnavigation/addons/LongKey.js
@@ +14,5 @@
> +
> +Cu.import("resource://spatialnavigation/InputManager.jsm");
> +
> +InputManager.on("userinput:long:direction", (aKind, aDirection) => {
> +  Navigator.moveToward(aDirection);

Yes. It's possible and may be more efficient (shorter call stack).
But, I want to keep it as it is if there's no problem.

I made "addons" to make it possible to offer some functionalities for better UX.
And, I think, handling long key is not an essential part of Spatial Navigation.
So, I considered handling long key is one of the functionalities.

::: toolkit/components/spatialnavigation/addons/TextField.js
@@ +13,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "Direction",
> +  "resource://spatialnavigation/Direction.jsm");
> +
> +on("premove",
> +  function TextFieldPreMove(aKind, aFocused, aDirection) {

It's not properly handled yet.
I hope it will be supported soon.

::: toolkit/components/spatialnavigation/modules/Addon.jsm
@@ +67,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "InputManager",
> +  "resource://spatialnavigation/InputManager.jsm");
> +XPCOMUtils.defineLazyGetter(this, "systemPrincipal",
> +  Services.scriptSecurityManager.getSystemPrincipal);
> +XPCOMUtils.defineLazyGetter(this, "loadSubScript", () => 

OK.

::: toolkit/components/spatialnavigation/modules/DOMInfo.jsm
@@ +186,5 @@
> +                 .DOMDocument
> +                 .defaultView;
> +
> +  if (!aIncludeChromeWindow && window instanceof nsIDOMChromeWindow) {
> +    window = window.content || window.getContentWindow();

Function getContentWindow were defined in b2g/chrome/content/shell.js but is removed by patch from Bug 1160923.
I'll change this, no matter how the patch is applied only in master branch yet.

::: toolkit/components/spatialnavigation/modules/IPC.jsm
@@ +58,5 @@
> +  console.debug(aDirection, aStartRect);
> +  aIFrame.QueryInterface(Ci.nsIFrameLoaderOwner)
> +         .frameLoader
> +         .messageManager
> +         .sendAsyncMessage(SearchMessage.sendToChild, aDirection, aStartRect);

The warning was not shown in 2.2r branch.
The message is added recently to avoid XPCOM object to be sent via message managers.
I'll trying to avoid the message anyway.

::: toolkit/components/spatialnavigation/modules/InputManager.jsm
@@ +29,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "console",
> +  "resource://spatialnavigation/Console.jsm");
> +
> +const { EventEmitter } =
> +  Cu.import("resource://gre/modules/devtools/event-emitter.js", {});

I also not that sure it good idea or not.
But, It is already used in places outside of devtools.
And, if I have not to use EventEmitter, I have to make my own similar one.
So, I want to keep this unless there's explicit reason to avoid it.

::: toolkit/components/spatialnavigation/modules/Navigator.jsm
@@ +135,5 @@
> +        aDirection = Direction.down;
> +      }
> +
> +      if (aDirection === Direction.up) {
> +        rect.translate(1, 1);

If given direction to find next focusable element is down, x and y of "search rect" will be 0, 0 because "rect" has -1 as it x and y.
When the direction is up, point (-1, -1) have to be contained in search rect.
So, I moved x and y of "rect" to (0, 0) via rect.translate(1, 1);
Let me know if this is not enough to understand.

@@ +204,5 @@
> +    setFocus(bestElement);
> +  } else {
> +    // we don't find any element to focus on Child side
> +    // send a message to parent to keep searching on parent side.
> +    IPC.searchFocusOnParent(aDirection, aStartRect);

First case is performed by IPC.searchFocusOnParent().
For second case, it's already done in function getBestElementOnWindow() above.
The function find focusable element from given window to all nested iframes those are in "Search rect" from aStartRect and aDirection.
Still, we need a way to indicate iframe is focused while its contents' not.(In this case, iframe is scrollable but has not focusable element within its viewport)

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

(In reply to David Flanagan [:djf] from comment #26)
> I don't know anything about this area of the platform, so I really don't
> know who should review it. My suggestion that we get a DOM peer
Ok, that is then probably me, but I need some help about the setup here.
Which events go where and what is this "Addon.jsm" and "Navigator.jsm", and how is "Console.jsm" related to this at all? But I'm really not a js developer so this needs also a review from someone hacking JS commonly.


> Olli's point in comment #22 about needing tests is another good point, and
> if we can't get the original author to write gecko tests for the feature, we
> should probably write some gaia integration tests to demonstrate that it
> works.
that is only partially what the tests are for. More important part is to prevent regressions.
We can't really land this massive amount of code and new functionality without some tests.


> I'd also really like to see someone who understands this code (the author,
> or the reviewer) write up a page or two of documentation explaining how it
> works so that someone like me can understand it well enough to work on it
> myself. There is no top-level overview comment in the code and the PDF
> attached to this bug does not help me understand it at all.
Indeed. comment 10 explains some, but not quite enough.

Comment 29

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Olli Pettay [:smaug] from comment #22)
> Oh, this lives outside Gecko and is all JS. I'm somewhat surprised (but

BTW, from my understanding this is still part of Gecko change. 
Please correct me if I'm wrong.

Comment 30

[Kershaw Chang [:kershaw]]

> ::: toolkit/components/spatialnavigation/addons/LongKey.js
> @@ +14,5 @@
> > +
> > +Cu.import("resource://spatialnavigation/InputManager.jsm");
> > +
> > +InputManager.on("userinput:long:direction", (aKind, aDirection) => {
> > +  Navigator.moveToward(aDirection);
> 
> Yes. It's possible and may be more efficient (shorter call stack).
> But, I want to keep it as it is if there's no problem.
> 
> I made "addons" to make it possible to offer some functionalities for better
> UX.
> And, I think, handling long key is not an essential part of Spatial
> Navigation.
> So, I considered handling long key is one of the functionalities.
> 
Thanks for the explanation. However, you are loading 7 script files every time you open a new web page. I am worried about if user will be happy with the latency or not. If you think the latency is acceptable, I think you can keep it.

> ::: toolkit/components/spatialnavigation/modules/InputManager.jsm
> @@ +29,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this, "console",
> > +  "resource://spatialnavigation/Console.jsm");
> > +
> > +const { EventEmitter } =
> > +  Cu.import("resource://gre/modules/devtools/event-emitter.js", {});
> 
> I also not that sure it good idea or not.
> But, It is already used in places outside of devtools.
> And, if I have not to use EventEmitter, I have to make my own similar one.
> So, I want to keep this unless there's explicit reason to avoid it.
> 
Let our DOM peer to decide this.

> ::: toolkit/components/spatialnavigation/modules/Navigator.jsm
> @@ +135,5 @@
> > +        aDirection = Direction.down;
> > +      }
> > +
> > +      if (aDirection === Direction.up) {
> > +        rect.translate(1, 1);
> 
> If given direction to find next focusable element is down, x and y of
> "search rect" will be 0, 0 because "rect" has -1 as it x and y.
> When the direction is up, point (-1, -1) have to be contained in search rect.
> So, I moved x and y of "rect" to (0, 0) via rect.translate(1, 1);
> Let me know if this is not enough to understand.
> 
Please add some comment in code to explain this. It would be great to have a test case to test the code here.

> @@ +204,5 @@
> > +    setFocus(bestElement);
> > +  } else {
> > +    // we don't find any element to focus on Child side
> > +    // send a message to parent to keep searching on parent side.
> > +    IPC.searchFocusOnParent(aDirection, aStartRect);
> 
> First case is performed by IPC.searchFocusOnParent().
> For second case, it's already done in function getBestElementOnWindow()
> above.
> The function find focusable element from given window to all nested iframes
> those are in "Search rect" from aStartRect and aDirection.
> Still, we need a way to indicate iframe is focused while its contents'
> not.(In this case, iframe is scrollable but has not focusable element within
> its viewport)

The case I observed is like below:
+-------------------------------------+
|          parent frame               |
|           +-----------+             |
|           |           |             |
|           | Element A |             |
|           |           |             |
|           +-----------+             |
|                                     |
|  +------------------------------+   |
|  |    Remote child frame        |   |
|  |                              |   |
|  |                              |   |
|  +------------------------------+   |
|                                     |
+-------------------------------------+

The current focused element is A. When receiving the down key, the focus is set to remote child frame. In child frame, there is no element can be focused, so child frame send a message back to ask parent to search. But, the focus can not be moved back to element A, it's stayed on child frame. It looks like that the focus is missing in this case.
To help people understand how your code is running, we really need some test cases.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #29)
> BTW, from my understanding this is still part of Gecko change. 
> Please correct me if I'm wrong.
toolkit/ or browser/ in general aren't part of Gecko core, but it is indeed vague what is part of Gecko and what isn't.

Comment 32

[Changbin Park]

(In reply to Kershaw Chang [:kershaw] from comment #30)
> Thanks for the explanation. However, you are loading 7 script files every
> time you open a new web page. I am worried about if user will be happy with
> the latency or not. If you think the latency is acceptable, I think you can
> keep it.
I think the latency is not that much. Because, it is not loaded per every new web page but loaded once per process when it's created. As I observed, the scripts used to be loaded before content of new process. So, I think, the latency is acceptable.
It can be changed easily, if there's explicit reason to load them faster.
Because, it is not that hard that getting them out from Sandbox and load as normal JS module.

> Let our DOM peer to decide this.
OK

> > ::: toolkit/components/spatialnavigation/modules/Navigator.jsm
> > @@ +135,5 @@
> > > +        aDirection = Direction.down;
> > > +      }
> > > +
> > > +      if (aDirection === Direction.up) {
> > > +        rect.translate(1, 1);
> > 
> > If given direction to find next focusable element is down, x and y of
> > "search rect" will be 0, 0 because "rect" has -1 as it x and y.
> > When the direction is up, point (-1, -1) have to be contained in search rect.
> > So, I moved x and y of "rect" to (0, 0) via rect.translate(1, 1);
> > Let me know if this is not enough to understand.
> > 
> Please add some comment in code to explain this. It would be great to have a
> test case to test the code here.
OK

> The case I observed is like below:
> +-------------------------------------+
> |          parent frame               |
> |           +-----------+             |
> |           |           |             |
> |           | Element A |             |
> |           |           |             |
> |           +-----------+             |
> |                                     |
> |  +------------------------------+   |
> |  |    Remote child frame        |   |
> |  |                              |   |
> |  |                              |   |
> |  +------------------------------+   |
> |                                     |
> +-------------------------------------+
> 
> The current focused element is A. When receiving the down key, the focus is
> set to remote child frame. In child frame, there is no element can be
> focused, so child frame send a message back to ask parent to search. But,
> the focus can not be moved back to element A, it's stayed on child frame. It
> looks like that the focus is missing in this case.
> To help people understand how your code is running, we really need some test
> cases.
I thought, we discussed about this case in last meeting in Seoul.
Anyway, for case described above, surely keeping focus at "Element A" looks natural.
So, It's already considered between "in process" frames.
But, when it comes with "out of process" case, it's getting complicated.
Because, remote child frame in parent process have to know it is (or contains) focusable element.
There might be a latency to know that and I'm afraid that makes UX worse.

So, I want to keep this if it's not that critical for UX.
Still, I agree that if it can be achieved with no or acceptable latency, it'll be better UX.
It can be a good item to improve UX of spatial navigation in multi process environment.

BTW, because, I think, it is not a good UX that giving focus to remote child frame and restore it to previous when there's no focusable in remote child frame, I don't think about implementation of that way.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Kershaw Chang [:kershaw] from comment #30)
> > ::: toolkit/components/spatialnavigation/modules/InputManager.jsm
> > @@ +29,5 @@
> > > +XPCOMUtils.defineLazyModuleGetter(this, "console",
> > > +  "resource://spatialnavigation/Console.jsm");
> > > +
> > > +const { EventEmitter } =
> > > +  Cu.import("resource://gre/modules/devtools/event-emitter.js", {});
> > 
> > I also not that sure it good idea or not.
> > But, It is already used in places outside of devtools.
> > And, if I have not to use EventEmitter, I have to make my own similar one.
> > So, I want to keep this unless there's explicit reason to avoid it.
> > 
> Let our DOM peer to decide this.
Sorry, as a DOM peer I have no idea whether toolkit stuff can use devtools modules.


You need to ask toolkit and/or devtools devs.

Comment 34

[Kershaw Chang [:kershaw]]

> > The case I observed is like below:
> > +-------------------------------------+
> > |          parent frame               |
> > |           +-----------+             |
> > |           |           |             |
> > |           | Element A |             |
> > |           |           |             |
> > |           +-----------+             |
> > |                                     |
> > |  +------------------------------+   |
> > |  |    Remote child frame        |   |
> > |  |                              |   |
> > |  |                              |   |
> > |  +------------------------------+   |
> > |                                     |
> > +-------------------------------------+
> > 
> > The current focused element is A. When receiving the down key, the focus is
> > set to remote child frame. In child frame, there is no element can be
> > focused, so child frame send a message back to ask parent to search. But,
> > the focus can not be moved back to element A, it's stayed on child frame. It
> > looks like that the focus is missing in this case.
> > To help people understand how your code is running, we really need some test
> > cases.
> I thought, we discussed about this case in last meeting in Seoul.
> Anyway, for case described above, surely keeping focus at "Element A" looks
> natural.
> So, It's already considered between "in process" frames.
> But, when it comes with "out of process" case, it's getting complicated.
> Because, remote child frame in parent process have to know it is (or
> contains) focusable element.
> There might be a latency to know that and I'm afraid that makes UX worse.
> 
> So, I want to keep this if it's not that critical for UX.
> Still, I agree that if it can be achieved with no or acceptable latency,
> it'll be better UX.
> It can be a good item to improve UX of spatial navigation in multi process
> environment.
> 
> BTW, because, I think, it is not a good UX that giving focus to remote child
> frame and restore it to previous when there's no focusable in remote child
> frame, I don't think about implementation of that way.

Your current code flow in this case looks like:
1. In parent process: moveFocus -> IPC.searchFocusOnChild
2. In child process: moveFocusFromParent -> moveFocus -> IPC.searchFocusOnParent
3. In parent process: moveFocusFromChild -> moveFocus

So, you already search in parent again if there is no focusable element in child. If you want to reduce the latency, you should avoid step 3.

Comment 35

[Changbin Park]

(In reply to Kershaw Chang [:kershaw] from comment #34)
> Your current code flow in this case looks like:
> 1. In parent process: moveFocus -> IPC.searchFocusOnChild
> 2. In child process: moveFocusFromParent -> moveFocus ->
> IPC.searchFocusOnParent
> 3. In parent process: moveFocusFromChild -> moveFocus
> 
> So, you already search in parent again if there is no focusable element in
> child. If you want to reduce the latency, you should avoid step 3.

To make the step 3 happened, it requires certain condition that there's no focusable element in remote iframe and the iframe is not scrollable. So, the step 3 seldom occurs since the condition is not met in usual. (This is just from my experience)
Moreover, no matter how the step 3 is performed, user might not realize unless there's focusable element is found in the step like below. (Remote child frame is not focusable at all)
+-------------------------------------+
|          parent frame               |
|           +-----------+             |
|           | Element A |             |
|           +-----------+             |
|                                     |
|  +------------------------------+   |
|  |    Remote child frame        |   |
|  |                              |   |
|  |                              |   |
|  +------------------------------+   |
|                                     |
|           +-----------+             |
|           | Element B |             |
|           +-----------+             |
+-------------------------------------+

Nevertheless, to move focus more precisely in that case, parent process always need to check remote iframe that is focusable or not before giving focus to it. And it must comes with more latency for every remote iframe cases because we can't know that it is focusable or not before checking it via IPC.

For that reason, as I remember, we agreed that we'll do best effort in process but remote iframe will be treated as always focusable. And, I still believe that it is not that weird and critical.

Do you have any idea to achieve it with no or minimum latency?
IMO, the point is that how parent process determine remote iframe is focusable or not.

Comment 36

[Kershaw Chang [:kershaw]]

(In reply to Changbin Park from comment #35)
> (In reply to Kershaw Chang [:kershaw] from comment #34)
> > Your current code flow in this case looks like:
> > 1. In parent process: moveFocus -> IPC.searchFocusOnChild
> > 2. In child process: moveFocusFromParent -> moveFocus ->
> > IPC.searchFocusOnParent
> > 3. In parent process: moveFocusFromChild -> moveFocus
> > 
> > So, you already search in parent again if there is no focusable element in
> > child. If you want to reduce the latency, you should avoid step 3.
> 
> To make the step 3 happened, it requires certain condition that there's no
> focusable element in remote iframe and the iframe is not scrollable. So, the
> step 3 seldom occurs since the condition is not met in usual. (This is just
> from my experience)
> Moreover, no matter how the step 3 is performed, user might not realize
> unless there's focusable element is found in the step like below. (Remote
> child frame is not focusable at all)
> +-------------------------------------+
> |          parent frame               |
> |           +-----------+             |
> |           | Element A |             |
> |           +-----------+             |
> |                                     |
> |  +------------------------------+   |
> |  |    Remote child frame        |   |
> |  |                              |   |
> |  |                              |   |
> |  +------------------------------+   |
> |                                     |
> |           +-----------+             |
> |           | Element B |             |
> |           +-----------+             |
> +-------------------------------------+
> 
> Nevertheless, to move focus more precisely in that case, parent process
> always need to check remote iframe that is focusable or not before giving
> focus to it. And it must comes with more latency for every remote iframe
> cases because we can't know that it is focusable or not before checking it
> via IPC.
> 
> For that reason, as I remember, we agreed that we'll do best effort in
> process but remote iframe will be treated as always focusable. And, I still
> believe that it is not that weird and critical.
> 
> Do you have any idea to achieve it with no or minimum latency?
> IMO, the point is that how parent process determine remote iframe is
> focusable or not.

I don't have any good idea to achieve this, since things get more complicated when involving remote iframe.
As I said in previous comment, I think you should just avoid to send message back to parent in this case.

Comment 37

[Changbin Park]

(In reply to Kershaw Chang [:kershaw] from comment #36)
> I don't have any good idea to achieve this, since things get more
> complicated when involving remote iframe.
> As I said in previous comment, I think you should just avoid to send message
> back to parent in this case.
OK. Then, I'll remove "send message back" case.

Comment 38

[Changbin Park]

Attachment: [WIP]SpatialNavigationB2G.patch

I'm uploading new patch that contains what we've talked about.

Comment 39

[Kershaw Chang [:kershaw]]

(In reply to Changbin Park from comment #38)
> Created attachment 8673478 [details] [diff] [review]
> [WIP]SpatialNavigationB2G.patch
> 
> I'm uploading new patch that contains what we've talked about.

Could you please also upload the interdiff between two versions?
Thanks.

Comment 40

[Changbin Park]

Attachment: [InterDiff]SpatialNavigationB2G.patch

I'm uploading interdiff between attachment 8663934 [details] [diff] [review] and attachment 8663978 [details] [diff] [review].
Thanks for your efforts.

Comment 41

[Kershaw Chang [:kershaw]]

Comment on attachment 8673478 [details] [diff] [review]
[WIP]SpatialNavigationB2G.patch

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

As comment#26 said, it would be great if you could also do things below:
1. Move these jsm files to b2g/components for landing on 2.2R. We can move it to tookit later.
2. Since your code includes many different kind of cases, we need tests to verify it.
3. For speeding up the review process, please provide more detailed document.

::: toolkit/components/spatialnavigation/modules/Navigator.jsm
@@ +74,5 @@
> +      console.debug("Navigation is prevented by addon");
> +      return;
> +    }
> +
> +    const handledElem = moveFocus(aTopWindow, aRectFrom, aDirection, true);

I think this is wrong. If this search request is coming from parent, we don't want to ask parent to search again.

@@ +145,5 @@
> +      if (aDirection === Direction.up) {
> +        rect.translate(0, rect.height);
> +      }
> +
> +      return rect;

You have many different kind of ways to get the rect. Could you add tests for each ways?

@@ +211,5 @@
> +                             aStartRect.translate(-x, -y));
> +    }
> +
> +    setFocus(bestElement);
> +  } else if (aWithinProcessBoundary) {

aWithinProcessBoundary looks odd to me. Should this be something like aSearchComingFromParent?

And the code below would look more clear.

else {
    if (!aSearchComingFromParent) {
        IPC.searchFocusOnParent(aDirection, aStartRect);
    }
}

Please also add some comments here to describe why we are doing this.

Comment 42

[Andrew Overholt [:overholt]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #21)
> NI to DOM team to review this patch.
> 
> [...]
> 
> Could you assign someone to review this?

There's an email thread going on about reviews for this so I'm removing the needinfo request of me.

Comment 43

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Changbin,

Could you provide the test cases? Those are mandatory when it comes to review process.

Comment 44

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

As per email discussion, we'll focus on landing this patch to 2.2R first and Kershaw will act as reviewer for this on 2.2R.

Changbin, now the missing piece is test case. Can you add that?

Comment 45

[John Chin]

The user needs to have control of navigating the user interface by being able to identify and traverse the UI objects on the screen one at a time.
MUST BE DONE.

This will allow the user to learn and utilize the user interface at their own rate of speed.

Screen reader shall provide the user the means to easily traverse the User Interface (UI) one UI object at a time with the ability to navigate to the previous, current and next UI Object at their own pace.


If the device has a touch screen, these UI objects need to be identifiable by touch (i.e. explore by touch).

Comment 46

[David Flanagan [:djf]]

John: yes, this is one of our highest priority patches to get landed in the 2.2r branch, and we know that it is urgent. Note, however, that we are waiting for a response to comment #41 and assume that the patch is still being worked on by the author.

John: What is Verizon's interest in this patch? My understanding is that your Red-Tai code already has focus navigation implemented on a per-app basis in Gaia. I suspect that landing this patch in Gecko will break your existing apps. Have you investigated that on your end?

Note that I have no idea whether this patch will integrate in any way with our screen reader code.

Wesley: do you know if we have any bugs tracking screen reader integration with keypad-based navigation?

Eitan: do you know anything about our screen reader support for non-touch screens? Do we already have that? Does our screen reader already have some kind of spatial navigation baked in to it?

Comment 47

[Changbin Park]

(In reply to Kershaw Chang [:kershaw] from comment #41)
> As comment#26 said, it would be great if you could also do things below:
> 1. Move these jsm files to b2g/components for landing on 2.2R. We can move
> it to tookit later.
> 2. Since your code includes many different kind of cases, we need tests to
> verify it.
No problem.
> 3. For speeding up the review process, please provide more detailed document.
OK. I'm preparing it.
> ::: toolkit/components/spatialnavigation/modules/Navigator.jsm
> @@ +74,5 @@
> > +      console.debug("Navigation is prevented by addon");
> > +      return;
> > +    }
> > +
> > +    const handledElem = moveFocus(aTopWindow, aRectFrom, aDirection, true);
> 
> I think this is wrong. If this search request is coming from parent, we
> don't want to ask parent to search again.
That's why I add true as 4th parameter - that's default value is false.
Plz, check below.
> @@ +145,5 @@
> > +      if (aDirection === Direction.up) {
> > +        rect.translate(0, rect.height);
> > +      }
> > +
> > +      return rect;
> 
> You have many different kind of ways to get the rect. Could you add tests
> for each ways?
I also think that part is pretty messy. I want to add tests for it soon but hardly now.
> @@ +211,5 @@
> > +                             aStartRect.translate(-x, -y));
> > +    }
> > +
> > +    setFocus(bestElement);
> > +  } else if (aWithinProcessBoundary) {
> 
> aWithinProcessBoundary looks odd to me. Should this be something like
> aSearchComingFromParent?
"aWithinProcessBoundary" means that moving focus within boundary of process, that used to be content process.
"aSearchComingFromParent" is also clear but it need some comment to understand its effect.
Because, "aSearchComingFromParent" describes environment of moveFocus() while "aWithinProcessBoundary" describes what to do in moveFocus().
So, I think, "aWithinProcessBoundary" is more intuitive.
> 
> And the code below would look more clear.
"if (!aSearchComingFromParent)" and "if (aWithinProcessBoundary)" are perfectly same.
> 
> else {
>     if (!aSearchComingFromParent) {
>         IPC.searchFocusOnParent(aDirection, aStartRect);
>     }
> }
> 
> Please also add some comments here to describe why we are doing this.
OK. I'll comment why aWithinProcessBoundary is used here.

Kershaw: Please, give some comment about this.
If you agree, I'll upload new patch including below.

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #44)
> As per email discussion, we'll focus on landing this patch to 2.2R first and
> Kershaw will act as reviewer for this on 2.2R.
> 
> Changbin, now the missing piece is test case. Can you add that?
I'm sorry for late.
As you saw in the email, we're preparing some testcases.
But, we're experiencing trouble with mochitest on our target devices. And, now, we're considering gaia ui test.
Since, we don't have much experience with test suites, I want to get some help, if possible.

Comment 48

[Ravi Dandu]

Hi Kershaw, Would you be able to provide help for Changbin asap, so the code lands. This spatial navigation code is big dependency for various other development.

Comment 49

[Kershaw Chang [:kershaw]]

> > @@ +211,5 @@
> > > +                             aStartRect.translate(-x, -y));
> > > +    }
> > > +
> > > +    setFocus(bestElement);
> > > +  } else if (aWithinProcessBoundary) {
> > 
> > aWithinProcessBoundary looks odd to me. Should this be something like
> > aSearchComingFromParent?
> "aWithinProcessBoundary" means that moving focus within boundary of process,
> that used to be content process.
> "aSearchComingFromParent" is also clear but it need some comment to
> understand its effect.
> Because, "aSearchComingFromParent" describes environment of moveFocus()
> while "aWithinProcessBoundary" describes what to do in moveFocus().
> So, I think, "aWithinProcessBoundary" is more intuitive.
> > 

If I understand your code correctly, you set aWithinProcessBoundary to true only when child frame receiving a request from parent, which means that the focusable element could be inside the child frame.
The code below says that if we don't find any element on child, we will send a message back to parent.
> +  } else if (aWithinProcessBoundary) {
> +    // we don't find any element to focus on Child side
> +    // send a message to parent to keep searching on parent side.
> +    IPC.searchFocusOnParent(aDirection, aStartRect);
> +  }

But, as you said in comment#37, we agreed to avoid sending a message back to parent?

> 
> Kershaw: Please, give some comment about this.
> If you agree, I'll upload new patch including below.
> 
> (In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #44)
> > As per email discussion, we'll focus on landing this patch to 2.2R first and
> > Kershaw will act as reviewer for this on 2.2R.
> > 
> > Changbin, now the missing piece is test case. Can you add that?
> I'm sorry for late.
> As you saw in the email, we're preparing some testcases.
> But, we're experiencing trouble with mochitest on our target devices. And,
> now, we're considering gaia ui test.
> Since, we don't have much experience with test suites, I want to get some
> help, if possible.

As [1] described, running mochitests on B2G device is not supported now. However, I think running mochitests on an emulator should be enough. You can also refer to [2] for creating a test case to test spatial navigation. BTW, I've tried to run [2] with your patch, but it just not worked. I am still figuring out what's wrong.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Automated_testing/Mochitests#Running_the_tests_2
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/mochitest/test_spatial_navigation.html

Comment 50

[Changbin Park]

(In reply to Kershaw Chang [:kershaw] from comment #49)
> If I understand your code correctly, you set aWithinProcessBoundary to true
> only when child frame receiving a request from parent, which means that the
> focusable element could be inside the child frame.
> The code below says that if we don't find any element on child, we will send
> a message back to parent.
> > +  } else if (aWithinProcessBoundary) {
> > +    // we don't find any element to focus on Child side
> > +    // send a message to parent to keep searching on parent side.
> > +    IPC.searchFocusOnParent(aDirection, aStartRect);
> > +  }
> 
> But, as you said in comment#37, we agreed to avoid sending a message back to
> parent?
Now, I'm seeing what I'm wrong. It's totally my mistake. I'm sorry for bothering you with it.
It must be like below
> > +  } else if (!aWithinProcessBoundary) {

> As [1] described, running mochitests on B2G device is not supported now.
> However, I think running mochitests on an emulator should be enough. You can
> also refer to [2] for creating a test case to test spatial navigation. BTW,
> I've tried to run [2] with your patch, but it just not worked. I am still
> figuring out what's wrong.
I'll also check [2] on emulator.

Comment 51

[Kershaw Chang [:kershaw]]

Attachment: testcase.patch

Hi Changbin, are you able to pass test_spatial_navigation.html at your side?
As the attached patch shows, I found that if I add some delay before start testing, test_spatial_navigation.html can pass with your patch.

Hope this can help you to create mochitests. If you have any trouble, don't hesitate to let me know.

Comment 52

[Kershaw Chang [:kershaw]]

Hi smaug,

I met a weird problem and I would like to know your advice.
I was running mochitest on B2G x86 emulator and in my test, I want to know if an iframe is focusable after the document is loaded immediately. But, I found that the iframe is not focusable at this moment because it has a zombie viewer[1]. After a while, the iframe can become to be focusable!

Is this an expected behavior? Do you know the reason behind this?

Thanks.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#6352-6358

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

So that can happen when we're loading already a new document, yet still showing the old one.
How do you detect that the iframe is loaded?

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Are you perhaps handling 'load' event from the previous load or something?

Comment 55

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #54)
> Are you perhaps handling 'load' event from the previous load or something?

Yes, I am handling 'load' event. But, I am still not clear about the root cause of this. Anyway, I found a workaround to make this work. Thanks.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

So are you sure you were dealing with the right load event? Say, if you have load listener on
<iframe> element, make sure you're not handling load for possible initial about:blank load, but the actual page.

Comment 57

[Eitan Isaacson [:eeejay]]

(In reply to David Flanagan [:djf] from comment #46)
> Eitan: do you know anything about our screen reader support for non-touch
> screens? Do we already have that? Does our screen reader already have some
> kind of spatial navigation baked in to it?

Some history: The screen reader was initially started to provide a11y in earlier version of Android Firefox. The main navigation mode at the time was not via touch screen, but instead of the "dpad". I looked into reviving the very old spatial navigation module, but we ended up not using it because spatial navigation does not make sense for 90% of screen reader users. Instead, we made the arrow keys analogous to swipe actions so swipe right=arrow right, swipe left = arrow left.

That is currently the behavior we still support. If you attach a BT keyboard to android and run Firefox with Talkback, you should get that behavior. Theoretically that should be the same case with B2G, the arrow keys should behave like swipe actions.

Does that answer the question?

Comment 58

[Changbin Park]

Attachment: Patch1 - SpatialNavigationB2G.patch

I'm attaching a new patch that contains no test cases.
Test cases still under preparing by my colleagues.

So, I think, it's better to get review this code first.

Comment 59

[Changbin Park]

Attachment: SpatialNavigationB2G.pdf

I hope this will be helpful some more.

Comment 60

[Kershaw Chang [:kershaw]]

(In reply to Changbin Park from comment #58)
> Created attachment 8682347 [details] [diff] [review]
> Patch1 - SpatialNavigationB2G.patch
> 
> I'm attaching a new patch that contains no test cases.
> Test cases still under preparing by my colleagues.
> 
> So, I think, it's better to get review this code first.

Please also upload the interdiff. Thanks.

Comment 61

[Changbin Park]

Attachment: [InterDiff]SpatialNavigaionB2G.patch

I'm uploading interdiff.

Notice that, I moved directory of "spatialnavigation", the interdiff doesn't show the moving but changes within the directory.

Comment 62

[Kershaw Chang [:kershaw]]

Comment on attachment 8682347 [details] [diff] [review]
Patch1 - SpatialNavigationB2G.patch

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

::: b2g/app/b2g.js
@@ +1161,5 @@
> +// Enable snav
> +pref("snav.enabled", true);
> +pref("snav.loglevel", "error");
> +
> +pref("snav.addon.baseURI", "resource://spatialnavigation/addons");

Can we reduce the preference settings here?
e.g., "resource://spatialnavigation/addons" should not be supposed to change?

::: b2g/components/spatialnavigation/SpatialNavigationB2G.jsm
@@ +55,5 @@
> +                         .getRootWindowOfProcess(null, true);
> +  if (chromeWindow) {
> +    console.log("Injecting SpatialNavigationB2G into all children frames");
> +    loadFrameScript(chromeWindow.messageManager
> +                                .QueryInterface(Ci.nsIFrameScriptLoader));

We don't need to QI again.

@@ +87,5 @@
> +  }
> +}
> +
> +function isPermittedMozBrowserFrame(aMozBrowserFrame) {
> +  if (!(aMozBrowserFrame instanceof Ci.nsIMozBrowserFrame)) return false;

Already checked ownerIsBrowserOrAppFrame above.

::: b2g/components/spatialnavigation/addons/InputRange.js
@@ +58,5 @@
> +
> +function deactivate(aElem) {
> +  inIDOMUtils.removePseudoClassLock(aElem, kPseudoClass);
> +}
> +

nit: remove newline

::: b2g/components/spatialnavigation/addons/OkKey.js
@@ +47,5 @@
> +                                .getInterface(Ci.nsIDOMWindowUtils);
> +
> +    if (aFocused === windowUtils.elementFromPoint(cx, cy, true, true)) {
> +      if(PrefObserver['allow.touchevent']) {
> +        fireTouchEvent(windowUtils, cx, cy);

Add a test case to show why we need to fire a touch event.

@@ +92,5 @@
> +  for (let evtType of ['mousedown', 'mouseup']) {
> +    aWindowUtils.sendMouseEvent(evtType, aX, aY, 0, 1 , 0);
> +  }
> +}
> +

nit: remove newline

::: b2g/components/spatialnavigation/addons/StyleFocused.js
@@ +36,5 @@
> +function processAuthorStyle(aKind, aEnable = PrefObserver[kPrefAuthor]) {
> +  aEnable ? StyleSheetsManager.registerStyleSheet(kURI):
> +            StyleSheetsManager.unregisterStyleSheet(kURI);
> +}
> +

nit: remove newline

::: b2g/components/spatialnavigation/addons/TextField.js
@@ +54,5 @@
> +  return aElem instanceof Ci.nsIDOMHTMLTextAreaElement ||
> +         (aElem instanceof Ci.nsIDOMHTMLInputElement &&
> +            aElem.mozIsTextField(false));
> +}
> +

nit: remove newline

::: b2g/components/spatialnavigation/modules/DOMInfo.jsm
@@ +231,5 @@
> +
> +    if (!isFocusable(node)) {
> +      aBlackList.add(node);
> +      continue;
> +    }

Move this after extracting focusable nodes in child frame. I think we don't have to pass parent's aBlackList into child.

@@ +260,5 @@
> +        if (DOMInfo(iframeContentHtml).isScrollable()) {
> +          nodeInfo.set(iframeContentHtml, contentRect.translate(aDx, aDy));
> +        }
> +      }
> +    } else {

Move isFocusable(node) here.

@@ +299,5 @@
> +            }
> +          }
> +        } = aWindow;
> +
> +  let [x, y] = [innerWidth, innerHeight].map(v => Math.round(v * .2));

Why this magic number?
Adding a test case would be great!

@@ +521,5 @@
> +  return windowRect.clone()
> +                   .translate(...DOMInfo.scrollOffset(aWindow, aDirection))
> +                   .union(windowRect);
> +}
> +

nit: remove newline

::: b2g/components/spatialnavigation/modules/InputManager.jsm
@@ +75,5 @@
> +  eventInProcessing = Cu.getWeakReference(null);
> +}
> +
> +InputManager.preventDefault = function InputManager_preventDefault() {
> +  const event = eventInProcessing.get();

Check eventInProcessing is null before using it?

::: b2g/components/spatialnavigation/modules/Navigator.jsm
@@ +192,5 @@
> +function moveFocus(aWindow,
> +                   aStartRect,
> +                   aDirection,
> +                   aWithinProcessBoundary = false) {
> +  let topWindow = aWindow;

When running the test case at my side on v2.2r, aWindow here is null. On master, aWindow is not null. I think this is due to the changes of testing framework between v2.2r and master. However, I think you should make sure your code can handle different cases. So, please consider what to do if aWindow is null.

@@ +201,5 @@
> +    }
> +    const {x, y} = DOMInfo(iframe).getContentRect();
> +    aStartRect.translate(x, y);
> +    topWindow = iframe.ownerDocument.defaultView;
> +  }

Can we cache the window that already traversed here? I think the window chain would not change very often, so you don't have to do this every time.

@@ +327,5 @@
> +
> +  focusManager.setFocus(aElem, FLAG_SHOWRING | FLAG_NOSCROLL);
> +
> +  if (!(aElem instanceof nsIDOMHTMLHtmlElement)) {
> +    DOMInfo(aElem).scrollIntoViewIfNeeded();

Need test case here.

@@ +439,5 @@
> +
> +  function vBorderDistance() {
> +    return  aRectTo.bottom <= aRectFrom.top ? aRectFrom.top - aRectTo.bottom :
> +            aRectFrom.bottom <= aRectTo.top ? aRectTo.top - aRectFrom.bottom :
> +            0;

Can we use math.abs here?

@@ +451,5 @@
> +
> +  return diagonalDistance(hBorderDistance(), vBorderDistance(), true);
> +}
> +
> +function diagonalDistance(aHdist, aVdist, aSkipSqrt = false) {

It seems this function is used with setting sSkipSqrt to always true. If so, can you just remove this parameter?

@@ +457,5 @@
> +
> +  return aSkipSqrt ? squaredDist : Math.sqrt(squaredDist);
> +}
> +
> +function *iframesChainFrom(aWindow) {

This function is somehow like DOMInfo.getRootWindowOfProcess, can you try to combine these two function?

::: b2g/components/spatialnavigation/modules/moz.build
@@ +15,5 @@
> +    'Navigator.jsm',
> +    'PrefObserver.jsm',
> +    'StyleSheetsManager.jsm'
> +]
> +

nit: remove newline

Comment 63

[Kershaw Chang [:kershaw]]

Attachment: test case modification for v2.2r

Modify test case for passing on v2.2r branch.

Comment 64

[Joonseok Yang]

Attachment: test_cases.patch

I made a patch that contains basic test cases for the spatial navigation. The test cases were successfully run on master branch (applied last gecko commit is c544f0d43095324dee33a8f02acea4497c57a232). There was no problem when I tried to run the tests SEPARATELY. But, when I ran all the tests at once (by passing the test directory as an argument), the test results were different from my expectations. The problem would be caused because the mochitest UI contains anchor tags, which is detected by the spatial navigation as focusable.

Although the patch only covers buttons in the grid (one type of html tag and very simple design), it is one of the most used designs. Test cases that covers more complex use cases will be uploaded later. 
The patch consists of four test cases. In each test case, a focus moves to focusable elements and the mochitest checks whether the results and the expectations are matched. 
 1) test_cross: cross-shaped 3x3 grid
 2) test_grid: 3x3 grid
 3) test_x_grid: x-shaped 3x3 grid (this case is going to be discussed internally and will be patched if necessary.)
 4) test_hollow: no focusable element in the center of the grid.

Comment 65

[Changbin Park]

Comment on attachment 8682347 [details] [diff] [review]
Patch1 - SpatialNavigationB2G.patch

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

Hi Kershaw,
I made answers to your review.
Please check it and let me know if there's anything to talk about.
New patches will be uploaded soon.

::: b2g/app/b2g.js
@@ +1161,5 @@
> +// Enable snav
> +pref("snav.enabled", true);
> +pref("snav.loglevel", "error");
> +
> +pref("snav.addon.baseURI", "resource://spatialnavigation/addons");

OK.

::: b2g/components/spatialnavigation/SpatialNavigationB2G.jsm
@@ +55,5 @@
> +                         .getRootWindowOfProcess(null, true);
> +  if (chromeWindow) {
> +    console.log("Injecting SpatialNavigationB2G into all children frames");
> +    loadFrameScript(chromeWindow.messageManager
> +                                .QueryInterface(Ci.nsIFrameScriptLoader));

OK

@@ +87,5 @@
> +  }
> +}
> +
> +function isPermittedMozBrowserFrame(aMozBrowserFrame) {
> +  if (!(aMozBrowserFrame instanceof Ci.nsIMozBrowserFrame)) return false;

You're right it's already checked.
I just want to check type of parameter.
I'll remove the statement and add comment about the type.

::: b2g/components/spatialnavigation/addons/InputRange.js
@@ +58,5 @@
> +
> +function deactivate(aElem) {
> +  inIDOMUtils.removePseudoClassLock(aElem, kPseudoClass);
> +}
> +

OK

::: b2g/components/spatialnavigation/addons/OkKey.js
@@ +47,5 @@
> +                                .getInterface(Ci.nsIDOMWindowUtils);
> +
> +    if (aFocused === windowUtils.elementFromPoint(cx, cy, true, true)) {
> +      if(PrefObserver['allow.touchevent']) {
> +        fireTouchEvent(windowUtils, cx, cy);

Firing touch event is for some specific cases those were found in early stage development.
I don't remember exactly what it was. But, as I remember, there's some elements those have listeners only for touch events.
I'll set preference "snav.allow.touchevent" to false by default.

@@ +92,5 @@
> +  for (let evtType of ['mousedown', 'mouseup']) {
> +    aWindowUtils.sendMouseEvent(evtType, aX, aY, 0, 1 , 0);
> +  }
> +}
> +

OK

::: b2g/components/spatialnavigation/addons/StyleFocused.js
@@ +36,5 @@
> +function processAuthorStyle(aKind, aEnable = PrefObserver[kPrefAuthor]) {
> +  aEnable ? StyleSheetsManager.registerStyleSheet(kURI):
> +            StyleSheetsManager.unregisterStyleSheet(kURI);
> +}
> +

OK

::: b2g/components/spatialnavigation/addons/TextField.js
@@ +54,5 @@
> +  return aElem instanceof Ci.nsIDOMHTMLTextAreaElement ||
> +         (aElem instanceof Ci.nsIDOMHTMLInputElement &&
> +            aElem.mozIsTextField(false));
> +}
> +

OK

::: b2g/components/spatialnavigation/modules/DOMInfo.jsm
@@ +231,5 @@
> +
> +    if (!isFocusable(node)) {
> +      aBlackList.add(node);
> +      continue;
> +    }

I think, iframe must be checked and determined focusable before extracting nodes from its content.
For example, if non-remote iframe has negative integer as it attribute tabIndex, the iframe must be excluded from focusable nodes.

An object for aBlackList is created and kept by Navigation module. And, it'll be used when DOMInfo.extractFocusableNodesInfo() is invoked from Navigator second time, so called "wide search".
Nodes, added into aBlackList while recursion of DOMInfo.extractFocusableNodesInfo(), are not meaningful within the recursion.

@@ +260,5 @@
> +        if (DOMInfo(iframeContentHtml).isScrollable()) {
> +          nodeInfo.set(iframeContentHtml, contentRect.translate(aDx, aDy));
> +        }
> +      }
> +    } else {

Explained above.

@@ +299,5 @@
> +            }
> +          }
> +        } = aWindow;
> +
> +  let [x, y] = [innerWidth, innerHeight].map(v => Math.round(v * .2));

There's no explicit reason for that number.
I just thought it might be good if view port of an window fully scrolled with 5 times of key press.
For our devices, it's pretty good to use.
I think, it's better to be configurable via preferences.
The value must be set depends on device.

@@ +521,5 @@
> +  return windowRect.clone()
> +                   .translate(...DOMInfo.scrollOffset(aWindow, aDirection))
> +                   .union(windowRect);
> +}
> +

OK

::: b2g/components/spatialnavigation/modules/InputManager.jsm
@@ +75,5 @@
> +  eventInProcessing = Cu.getWeakReference(null);
> +}
> +
> +InputManager.preventDefault = function InputManager_preventDefault() {
> +  const event = eventInProcessing.get();

Then, I'm gonna remove Cu.getWeakReference(null) also.

::: b2g/components/spatialnavigation/modules/Navigator.jsm
@@ +192,5 @@
> +function moveFocus(aWindow,
> +                   aStartRect,
> +                   aDirection,
> +                   aWithinProcessBoundary = false) {
> +  let topWindow = aWindow;

It doesn't make sense moveFocus is called with null as aWindow because aStartRect is a relative coordinate belongs to aWindow.
I'll add error message here to get noticed such a case and remove that kind of function call.

@@ +201,5 @@
> +    }
> +    const {x, y} = DOMInfo(iframe).getContentRect();
> +    aStartRect.translate(x, y);
> +    topWindow = iframe.ownerDocument.defaultView;
> +  }

If it's possible to cache with precise data, it'll be better as you said.
But, I'm afraid it seems hard to make it.
Because, it's not only traverse iframe and it's embedder but also calculate their relative coordination.
If there's any change of layout between "aWindow" and new "topWindow", the caching could lead to unexpected focus moving.

@@ +439,5 @@
> +
> +  function vBorderDistance() {
> +    return  aRectTo.bottom <= aRectFrom.top ? aRectFrom.top - aRectTo.bottom :
> +            aRectFrom.bottom <= aRectTo.top ? aRectTo.top - aRectFrom.bottom :
> +            0;

Can you give a hint how can I use it?

@@ +451,5 @@
> +
> +  return diagonalDistance(hBorderDistance(), vBorderDistance(), true);
> +}
> +
> +function diagonalDistance(aHdist, aVdist, aSkipSqrt = false) {

Yes, it's always true so far.
I just want to mark it is squared distance for further use of the function.
I'll replace that with renaming of functions.

@@ +457,5 @@
> +
> +  return aSkipSqrt ? squaredDist : Math.sqrt(squaredDist);
> +}
> +
> +function *iframesChainFrom(aWindow) {

They looks similar but used for different purpose.
As a result, both getRootWindowOfProcess() and iframesChainFrom() reaches top most window in the end. However, they use totally different way to get it.
The former always find top most window at once via docshell no matter how first parameter is given or not while the latter can't.
On the other hand, since the latter's an generator, it can stop traversing nested iframes in DOM tree before reaching top most window while the former can't.

::: b2g/components/spatialnavigation/modules/moz.build
@@ +15,5 @@
> +    'Navigator.jsm',
> +    'PrefObserver.jsm',
> +    'StyleSheetsManager.jsm'
> +]
> +

OK

Comment 66

[Kershaw Chang [:kershaw]]

> I think, iframe must be checked and determined focusable before extracting
> nodes from its content.
> For example, if non-remote iframe has negative integer as it attribute
> tabIndex, the iframe must be excluded from focusable nodes.
> 
I agree with you that we should filter the iframe with a negative tabIndex.
 
> An object for aBlackList is created and kept by Navigation module. And,
> it'll be used when DOMInfo.extractFocusableNodesInfo() is invoked from
> Navigator second time, so called "wide search".
> Nodes, added into aBlackList while recursion of
> DOMInfo.extractFocusableNodesInfo(), are not meaningful within the recursion.
> 
Thanks for your explanation. But, passing the black list which contains the nodes in parent frame into child frame does not make sense. Can you try to avoid that?
 
> ::: b2g/components/spatialnavigation/modules/Navigator.jsm
> @@ +192,5 @@
> > +function moveFocus(aWindow,
> > +                   aStartRect,
> > +                   aDirection,
> > +                   aWithinProcessBoundary = false) {
> > +  let topWindow = aWindow;
> 
> It doesn't make sense moveFocus is called with null as aWindow because
> aStartRect is a relative coordinate belongs to aWindow.
> I'll add error message here to get noticed such a case and remove that kind
> of function call.
> 
I met this situation when I ran mochitest on v2.2r before. Sometimes the focusWindow in focus manager could be null. 

> @@ +201,5 @@
> > +    }
> > +    const {x, y} = DOMInfo(iframe).getContentRect();
> > +    aStartRect.translate(x, y);
> > +    topWindow = iframe.ownerDocument.defaultView;
> > +  }
> 
> If it's possible to cache with precise data, it'll be better as you said.
> But, I'm afraid it seems hard to make it.
> Because, it's not only traverse iframe and it's embedder but also calculate
> their relative coordination.
> If there's any change of layout between "aWindow" and new "topWindow", the
> caching could lead to unexpected focus moving.
> 
Considering that two consecutive moves, the window hierarchy should be the same. Maybe we don't have to waste time on traversing to the top window. If it's difficult to do that, we can keep the current way at this moment.


> @@ +439,5 @@
> > +
> > +  function vBorderDistance() {
> > +    return  aRectTo.bottom <= aRectFrom.top ? aRectFrom.top - aRectTo.bottom :
> > +            aRectFrom.bottom <= aRectTo.top ? aRectTo.top - aRectFrom.bottom :
> > +            0;
> 
> Can you give a hint how can I use it?
> 
Oops, I was wrong. We really have to check which one is larger in this case.

> 
> @@ +457,5 @@
> > +
> > +  return aSkipSqrt ? squaredDist : Math.sqrt(squaredDist);
> > +}
> > +
> > +function *iframesChainFrom(aWindow) {
> 
> They looks similar but used for different purpose.
> As a result, both getRootWindowOfProcess() and iframesChainFrom() reaches
> top most window in the end. However, they use totally different way to get
> it.
> The former always find top most window at once via docshell no matter how
> first parameter is given or not while the latter can't.
> On the other hand, since the latter's an generator, it can stop traversing
> nested iframes in DOM tree before reaching top most window while the former
> can't.
> 
Ok.

Comment 67

[Changbin Park]

(In reply to Kershaw Chang [:kershaw] from comment #66)
> > An object for aBlackList is created and kept by Navigation module. And,
> > it'll be used when DOMInfo.extractFocusableNodesInfo() is invoked from
> > Navigator second time, so called "wide search".
> > Nodes, added into aBlackList while recursion of
> > DOMInfo.extractFocusableNodesInfo(), are not meaningful within the recursion.
> > 
> Thanks for your explanation. But, passing the black list which contains the
> nodes in parent frame into child frame does not make sense. Can you try to
> avoid that?
Yes, it's possible but, I'm afraid that it makes unnecessary processes.
I don't think moving code position is enough to avoid that. Because, if there's any element before iframe while iterating "nodes", it'll be passed into child frame.

I have 2 possible ways to avoid that.
First way is sorting "nodes" to make iframes be first before iterating. Second is create new object for every aBlackList parameter of the recursive call an merge it to current(parent) aBlackList. Obviously, both ways requires additional processes.

Can you tell me what's your worrying about?
IMO, it's not that problem to passing an WeakSet object, contains elements from parent frame, into child frame. Because, child frame can't get any information of parent frame from the WeakSet.

> > ::: b2g/components/spatialnavigation/modules/Navigator.jsm
> > @@ +192,5 @@
> > > +function moveFocus(aWindow,
> > > +                   aStartRect,
> > > +                   aDirection,
> > > +                   aWithinProcessBoundary = false) {
> > > +  let topWindow = aWindow;
> > 
> > It doesn't make sense moveFocus is called with null as aWindow because
> > aStartRect is a relative coordinate belongs to aWindow.
> > I'll add error message here to get noticed such a case and remove that kind
> > of function call.
> > 
> I met this situation when I ran mochitest on v2.2r before. Sometimes the
> focusWindow in focus manager could be null. 
I also found a possibility occuring the situation when "focusedWindow" of focus manager is null.
It comes from moveToward() function. So, I'll change it to use DOMInfo.getRootWindowOfProcess() when "focusedWindow" is null.

> > @@ +201,5 @@
> > > +    }
> > > +    const {x, y} = DOMInfo(iframe).getContentRect();
> > > +    aStartRect.translate(x, y);
> > > +    topWindow = iframe.ownerDocument.defaultView;
> > > +  }
> > 
> > If it's possible to cache with precise data, it'll be better as you said.
> > But, I'm afraid it seems hard to make it.
> > Because, it's not only traverse iframe and it's embedder but also calculate
> > their relative coordination.
> > If there's any change of layout between "aWindow" and new "topWindow", the
> > caching could lead to unexpected focus moving.
> > 
> Considering that two consecutive moves, the window hierarchy should be the
> same. Maybe we don't have to waste time on traversing to the top window. If
> it's difficult to do that, we can keep the current way at this moment.
Nobody can't guarantee that there's no change in DOM between two consecutive moves. There might be some other event/handler between them and it could make some change in DOM. That's why I said it's hard.
So, it'll be kept as it is.(If you have any idea to achieve "precise caching", let me know plz)

By the way, we're still experiencing trouble with trying to make an environment to running mochitest via emulator. So, test cases might be delayed some more.

Comment 68

[Kershaw Chang [:kershaw]]

(In reply to Changbin Park from comment #67)
> (In reply to Kershaw Chang [:kershaw] from comment #66)
> > > An object for aBlackList is created and kept by Navigation module. And,
> > > it'll be used when DOMInfo.extractFocusableNodesInfo() is invoked from
> > > Navigator second time, so called "wide search".
> > > Nodes, added into aBlackList while recursion of
> > > DOMInfo.extractFocusableNodesInfo(), are not meaningful within the recursion.
> > > 
> > Thanks for your explanation. But, passing the black list which contains the
> > nodes in parent frame into child frame does not make sense. Can you try to
> > avoid that?
> Yes, it's possible but, I'm afraid that it makes unnecessary processes.
> I don't think moving code position is enough to avoid that. Because, if
> there's any element before iframe while iterating "nodes", it'll be passed
> into child frame.
> 
> I have 2 possible ways to avoid that.
> First way is sorting "nodes" to make iframes be first before iterating.
> Second is create new object for every aBlackList parameter of the recursive
> call an merge it to current(parent) aBlackList. Obviously, both ways
> requires additional processes.
> 
> Can you tell me what's your worrying about?
> IMO, it's not that problem to passing an WeakSet object, contains elements
> from parent frame, into child frame. Because, child frame can't get any
> information of parent frame from the WeakSet.
> 
My only concern is how to make the code more clear. aBlackList now has two purposes: 1. Collecting the unfocusable nodes in frames. 2. Black list for filtering nodes in wide search. In my opinion, it's not a good design to combine these two purposes into one variable. I think it would be better to separate into two lists.

> > > @@ +201,5 @@
> > > > +    }
> > > > +    const {x, y} = DOMInfo(iframe).getContentRect();
> > > > +    aStartRect.translate(x, y);
> > > > +    topWindow = iframe.ownerDocument.defaultView;
> > > > +  }
> > > 
> > > If it's possible to cache with precise data, it'll be better as you said.
> > > But, I'm afraid it seems hard to make it.
> > > Because, it's not only traverse iframe and it's embedder but also calculate
> > > their relative coordination.
> > > If there's any change of layout between "aWindow" and new "topWindow", the
> > > caching could lead to unexpected focus moving.
> > > 
> > Considering that two consecutive moves, the window hierarchy should be the
> > same. Maybe we don't have to waste time on traversing to the top window. If
> > it's difficult to do that, we can keep the current way at this moment.
> Nobody can't guarantee that there's no change in DOM between two consecutive
> moves. There might be some other event/handler between them and it could
> make some change in DOM. That's why I said it's hard.
> So, it'll be kept as it is.(If you have any idea to achieve "precise
> caching", let me know plz)
> 
I think the top window in both parent and content process should be always the same. Would it be helpful if you just keep the reference to the top window?

> By the way, we're still experiencing trouble with trying to make an
> environment to running mochitest via emulator. So, test cases might be
> delayed some more.

I've replied your questions via email. However, I'd suggest you to post your questions here. There are more experienced people can help to answer.

Comment 69

[Changbin Park]

(In reply to Kershaw Chang [:kershaw] from comment #68)
> My only concern is how to make the code more clear. aBlackList now has two
> purposes: 1. Collecting the unfocusable nodes in frames. 2. Black list for
> filtering nodes in wide search. In my opinion, it's not a good design to
> combine these two purposes into one variable. I think it would be better to
> separate into two lists.
I think, "aBlackList" might not be a good name. It's purpose is caching unfocusable elements, indeed. For that, it works as collecting and filtering as you said. ("Black list" in purpose 2 comes from "Collecting" in 1)

I think, most simple way to get rid of confusion from aBlackList is just remove it. Because, even if it'll be removed from the function, it'll works if currently focused element is properly filtered at getBestElementOnWindow(). Of course it'll be slower since re-determining known unfocusable elements as unfocusable.

To separate those steps, we have to increase times of iteration and replace WeakSet with Set that uses strong reference to its elements for merging elements from child iframe. I don't think, this is a best way for both purpose (caching and clear code).

How about some comment and/or renaming for aBlackList? I think, it's not that bad choice.

> I think the top window in both parent and content process should be always
> the same. Would it be helpful if you just keep the reference to the top
> window?
In here, just keeping reference to the top window is not that helpful. If we use cache here, key will be 2 windows and data will be relative coordinate between them.

> I've replied your questions via email. However, I'd suggest you to post your
> questions here. There are more experienced people can help to answer.
We'll file a new bug for that and make it blocker of this if necessary.
Thanks for the helps.

Comment 70

[Kershaw Chang [:kershaw]]

(In reply to Changbin Park from comment #69)
> (In reply to Kershaw Chang [:kershaw] from comment #68)
> > My only concern is how to make the code more clear. aBlackList now has two
> > purposes: 1. Collecting the unfocusable nodes in frames. 2. Black list for
> > filtering nodes in wide search. In my opinion, it's not a good design to
> > combine these two purposes into one variable. I think it would be better to
> > separate into two lists.
> I think, "aBlackList" might not be a good name. It's purpose is caching
> unfocusable elements, indeed. For that, it works as collecting and filtering
> as you said. ("Black list" in purpose 2 comes from "Collecting" in 1)
> 
> I think, most simple way to get rid of confusion from aBlackList is just
> remove it. Because, even if it'll be removed from the function, it'll works
> if currently focused element is properly filtered at
> getBestElementOnWindow(). Of course it'll be slower since re-determining
> known unfocusable elements as unfocusable.
> 
> To separate those steps, we have to increase times of iteration and replace
> WeakSet with Set that uses strong reference to its elements for merging
> elements from child iframe. I don't think, this is a best way for both
> purpose (caching and clear code).
> 
> How about some comment and/or renaming for aBlackList? I think, it's not
> that bad choice.
> 
I think renaming is fine. Also adding some comment would be great.

> > I think the top window in both parent and content process should be always
> > the same. Would it be helpful if you just keep the reference to the top
> > window?
> In here, just keeping reference to the top window is not that helpful. If we
> use cache here, key will be 2 windows and data will be relative coordinate
> between them.
> 
OK, I am fine with this right now. Maybe we can think about how to improve the performance later.

Comment 71

[BMO Automation]

Firefox OS is not being worked on