Closed
Bug 1289170
Opened 8 years ago
Closed 8 years ago
toolbar keyboard navigation works strangely
Categories
(DevTools :: Framework, defect, P3)
DevTools
Framework
Tracking
(firefox51 fixed)
RESOLVED
FIXED
Firefox 51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: tromey, Assigned: yzen)
References
Details
Attachments
(1 file)
As part of bug 1278473, I experimented a bit with keyboard navigation in the toolbar. This was added in bug 1242852. I'm using Linux. STR: * Open devtools * Move mouse over "Inspector" tab on toolbar * Press right arrow => Focus moves to "Console" tab, good. * Press left arrow once => Back to "Inspector", good. * Press left arrow again => Focus disappears. I was expecting it to move to the "Pick an element from the page" button; or, failing that, perhaps to somewhere else on the toolbar * Press right arrow a number of times => Focus moves smoothly through toolbar tabs. Good so far... * ... but keep pressing it and, for me, it eventually disappears. It works fine through the "normal" tabs: Inspector, Console, Debugger, Style Editor, Canvas, Performance, Memory, Network Monitor. Then it disappears. If I keep pressing the right arrow key while the focus is gone, I eventually see the focus reappear on the Toolbox Options button. If I press right arrow again after this, it disappears again for good. I was expecting this to either work a bit more like Tab, or to just cycle through the various items on the toolbar. Also, a similar toolbar in the inspector also allows arrow navigation, but works differently. With the top-level toolbar, moving focus between the buttons with the arrow key has no other effect; whereas in the rule view, moving between Rules/Computed/Animations/Forms using the arrow keys has the side effect of displaying the content corresponding to that tab. I don't know which is preferable but I do think they ought to work the same.
Comment 1•8 years ago
|
||
Yura, any opinions on the expected behavior here? Additional context: we are hoping to remove and/or shim Services.focusManager.moveFocus to make it work without chrome privileges
Flags: needinfo?(yzenevich)
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #1) > Yura, any opinions on the expected behavior here? > > Additional context: we are hoping to remove and/or shim > Services.focusManager.moveFocus to make it work without chrome privileges Yes, this is something I wanted to work on: moving from a model where toolbar controls are selected with focus to one where selection is handled with aria-activedescendant
Flags: needinfo?(yzenevich)
Priority: -- → P3
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68978/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68978/
Attachment #8777401 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb96ef5158d
Comment 5•8 years ago
|
||
Comment on attachment 8777401 [details] Bug 1289170 - improving keyboard accessibility for dev tools tabbar. https://reviewboard.mozilla.org/r/68978/#review66080 This looks great! Fixes behavior, removes chrome API, and a simpler implementation ::: devtools/client/framework/toolbox.js:886 (Diff revision 1) > - if (controlID) { > - control = this.doc.getElementById(controlID); > - } > - if (rangeParent || !control) { > - // If range parent is present, the focused is moved within the toolbar, > - // simply updating aria-activedescendant. Or if aria-activedescendant is > + * results in navigating away from the tabbar container. > + * @param {FocusEvent} event > + */ > + _onTabbarFocus: function (event) { > + this.tabbarFocusableElms.forEach(elm => > + elm.setAttribute("tabindex", event.target == elm ? "0" : "-1")); Nit: === ::: devtools/client/framework/toolbox.js:897 (Diff revision 1) > + * @param {KeyboardEvent} event > + */ > + _onTabbarArrowKeypress: function (event) { > - let { key, target } = event; > + let { key, target } = event; > - let win = this.win; > - let elm, type; > + let focusableElms = this.tabbarFocusableElms; > + let curIndex = focusableElms.indexOf(target); What if curIndex is -1? I imagine that could happen if an addon injects DOM or if we add a new non-button element but forget to change the selector. I'm thinking we should be defensive in that case and return early without throwing ::: devtools/client/framework/toolbox.js:917 (Diff revision 1) > - // Ignore all other keys. > - return; > + return; > - } > + } > + > + focusableElms.forEach(elm => > + elm.setAttribute("tabindex", newTarget == elm ? "0" : "-1")); Nit: ===
Attachment #8777401 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8777401 [details] Bug 1289170 - improving keyboard accessibility for dev tools tabbar. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68978/diff/1-2/
Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5d2e3f233977 improving keyboard accessibility for dev tools tabbar. r=bgrins
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5d2e3f233977
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•