463516 - SNAV: _focusNextUsingCmdDispatcher() can make bad assumption, snav can leave content to go to chrome.

Status: RESOLVED INCOMPLETE

(Toolkit Graveyard :: Spatial Navigation, defect)

Description

[Antonio Gomes (tonikitoo)]

Attachment: testcase

1) Open the attached testcase.
2) Put focus on "option 2".
3) Press ArrowDown

ACTUAL OUTCOME:
Snav detects that there is no a valid focusable element (correct) and calls '_focusNextUsingCmdDispatcher' which move focus forward: tabindex moves focus out of content. It goes to browser urlbar (chrome).

EXPECTED OUTCOME:
As there is no focusable element below "option 1" focus should just stay where it is (or maybe try to scrollDown() instead ?).


http://mxr.mozilla.org/mozilla-central/source/toolkit/spatial-navigation/SpatialNavigation.js#231

Comment 1

[Antonio Gomes (tonikitoo)]

if we are thinking about making SNAV more usable to ship fennec 1.0 w/ it on, we should consider this bug, imho.

bad assumption usually comes from these lines below in SNAV code, whereas when we do not find any good candidate to move focus to, we do FocusMove (forward/backward) which can be potential bad and "dangerous" ... try attached test case on fennec.

(...) 
  } else {
    // couldn't find anything.  just advance and hope.
    _focusNextUsingCmdDispatcher(key, callback);
  }
(...)

MoveFocus can even jump through different documents trees, e.g. from last link in the bottom content to URLbar in chrome ...

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Antonio - the new focus manager has landed since you guys first worked on this code. Maybe we should try using nsIFocusManager.moveFocus

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIFocusManager.idl#133

Comment 3

[Antonio Gomes (tonikitoo)]

(In reply to comment #2)
> Antonio - the new focus manager has landed since you guys first worked on this
> code. Maybe we should try using nsIFocusManager.moveFocus
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIFocusManager.idl#133

@mark, the problem is not actually what method we are using to move the focus forward/backward. Actually, we are indirectly using nsIFocusManager.moveFocus method you suggested: see from snav we calling

1) _focusNextUsingCmdDispatcher (internal method) which calls

2) window.document.commandDispatcher.advanceFocus(); 

from http://mxr.mozilla.org/mozilla-central/source/content/xul/document/src/nsXULCommandDispatcher.cpp#248 and #255 we see both calling nsIfocusmanager.moveFocus()

the problem from my perspective is do .moveFocus ("shift focus") when SNAV has no option to focus. MoveFocus rely on tabIndex which has nothing to do w/ spatial position.

thoughts ?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

based on bug 492409, we should disable SNAV for 1.0

vote for not blocking-fennec1.0

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

We disabled SNAV for Fennec 1.0

Comment 6

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This bug's parent component is going away. Should this bug be moved to an equivalent component in Android or iOS?

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

This is a Gecko bug thus irrelevant for Firefox for iOS. I don't know of an Android device with a directional pad that we support (Android 4.0+).