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

RESOLVED INCOMPLETE

Status

()

Toolkit
Spatial Navigation
--
major
RESOLVED INCOMPLETE
9 years ago
a year ago

People

(Reporter: Antonio Gomes (tonikitoo), Assigned: Antonio Gomes (tonikitoo))

Tracking

(Blocks: 1 bug, {mobile})

Trunk
x86
Linux
mobile
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [close-me-2017-02-15])

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Created attachment 346769 [details]
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
(Assignee)

Updated

9 years ago
Depends on: 463514
(Assignee)

Comment 1

9 years ago
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 ...
Severity: normal → major
tracking-fennec: --- → ?
(Assignee)

Updated

9 years ago
Assignee: nobody → tonikitoo
Component: Keyboard: Navigation → Spatial Navigation
Product: Core → Toolkit
QA Contact: keyboard.navigation → spatial.navigation
(Assignee)

Updated

9 years ago
Keywords: mobile
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
(Assignee)

Comment 3

9 years ago
(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 ?
tracking-fennec: ? → 1.0+
based on bug 492409, we should disable SNAV for 1.0

vote for not blocking-fennec1.0
We disabled SNAV for Fennec 1.0
tracking-fennec: 1.0+ → ---
This bug's parent component is going away. Should this bug be moved to an equivalent component in Android or iOS?
Whiteboard: [close-me-2017-02-15]
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+).
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.