It looks like right now there's no way to navigate away with a keyboard from the inspector toolbar. Once the user tabs into the toolbar, there is no way to forward or back tab out of it.

The way this should work is: Once tab lands on the current or first item in the tool bar, arrow keys left and right navigate to the previous and next item respectively. Tab itself should move out of the container onto the next main thing, presumably the DOM tree of the current open document, and land on the selected tree view item. It is not necessary and in fact not desirable to have tab stop on every control within the toolbar itself.

Filter on CLIMBING SHOES

--- devtools/client/framework/toolbox.js | 67 +++++++++++++++++++++++++++++ devtools/client/shared/developer-toolbar.js | 8 ++++ devtools/client/themes/toolbars.css | 11 +++++ 3 files changed, 86 insertions(+) Review commit: https://reviewboard.mozilla.org/r/37673/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37673/

--- devtools/client/inspector/breadcrumbs.js | 36 +++++++++++++++++++++++++++ devtools/client/inspector/inspector-search.js | 6 +++++ 2 files changed, 42 insertions(+) Review commit: https://reviewboard.mozilla.org/r/37675/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37675/

note this is a lot more convenient to test with a patch for bug 1242851 applied

Sorry, haven't had a chance to fully review this yet. I tried to leave this comment inline but can't figure out how to leave it without clearing the review flag: Curious, is there a way to get the same / similar behavior as FocusManager without chrome privileged APIs? We are working towards making the frontend run in a tab from a non-chrome URI

(In reply to Brian Grinstead [:bgrins] from comment #6) > Sorry, haven't had a chance to fully review this yet. I tried to leave this > comment inline but can't figure out how to leave it without clearing the > review flag: > > Curious, is there a way to get the same / similar behavior as FocusManager > without chrome privileged APIs? We are working towards making the frontend > run in a tab from a non-chrome URI I think at this point , it's the best way to determine what the next focusable element would be. The other way it could possible be implemented is by somehow declaring what those focusable elements are in relation to the current toolbar..

some comments for IRC of things to do: <@bgrins> yzen: ok, and also we'll need some test for this functionality if we want to be sure it won't regress. We have something like this already so maybe a clone of this could be used to test the tool navigation behavior: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/browser_keybindings_03.js look into: <@bgrins> yzen: one more thing: I have all the patches applied (including the styles patch) but I don't see the toolbar tabs being styled as I'm expecting from: https://reviewboard.mozilla.org/r/37673/diff/1#index_header <@bgrins> yzen: also re: breadcrumbs. There's something weird when I focus one then press 'tab' it moves me to the css rules search box. but then if I shift+tab it moves me back into the markup tree (skips the breadcrumbs) [15:31:26] <@bgrins> likewise, if i tab forward from markup view it skips crumbs and moves straight to rule view [15:31:56] <yzen> bgrins yeah ill take a look , there might be some unexpected keyboard orders [15:32:00] <yzen> thanks for pointing that out [15:32:29] <@bgrins> could somehow setting tabindex on the breadcrumbs make it work without needing any extra code to handle tabs? [15:32:41] <@bgrins> I guess it'd need to be synchronized with all other elements in the inspector then thoguh? [15:36:28] ddahl (ddahl@moz-k9mup2.ntao.hgdc.0248.2601.IP) left IRC (Connection closed) [15:37:21] <yzen> bgrins yeah so the issue here is that we want to tab between toolbars but arrow between their content wherever possible [15:37:43] <yzen> so setting tabindex everywhere makes everything tabbable, which is not very effifient [15:37:50] <yzen> efficient [15:38:01] <@bgrins> oh i see. so tab over to a new tool and arrow down to descend into the content of the tool? [15:40:46] <@bgrins> but what about for breadcrumbs? AFAICT that patch is handling only tab / shift tab key shortcuts, so could tabindex somehow fix this? What would be nice is if we could keep diff tabindex ordering among sub components within a single frame [15:41:14] <@bgrins> so breadcrumbs we could set the attributes as 1,2,3,etc without needing to know the tabindex of markup view

Hi Yura, based on our discussion are you planning to modify these patches before needing a review? I think I will forward at least the breadcrumbs review to someone else, but want to know if I should wait for another version to get pushed.

(In reply to Brian Grinstead [:bgrins] from comment #9) > Hi Yura, based on our discussion are you planning to modify these patches > before needing a review? I think I will forward at least the breadcrumbs > review to someone else, but want to know if I should wait for another > version to get pushed. Hi Brian, yes, I have almost finished writing tests for my changes. If you tell me who I should mark for review, I will do that when I push the update.

I think Gabe (:gl) should be in the loop on the breadcrumb changes so he'd be good to request for that one. I can review the toolbox tabs one.

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins Clearing review for now as discussed

Comment on attachment 8725883 [details] MozReview Request: Bug 1242852 - (part 2) making inspector toolbar keyboard accessible. Clearing review for now as discussed

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37673/diff/1-2/

Comment on attachment 8725883 [details] MozReview Request: Bug 1242852 - (part 2) making inspector toolbar keyboard accessible. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37675/diff/1-2/

Comment on attachment 8725883 [details] MozReview Request: Bug 1242852 - (part 2) making inspector toolbar keyboard accessible. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37675/diff/2-3/

OK, so I've applied the patch (along with the patch from bug 1242851), and I can can't figure out how to get focus on the toolbox tabs and navigate them with the keyboard (on OSX). Like if I tab through into the toolbox it jumps straight into a tool. Should I be able to access these tabs with the keyboard?

(In reply to Brian Grinstead [:bgrins] from comment #17) > OK, so I've applied the patch (along with the patch from bug 1242851), and I > can can't figure out how to get focus on the toolbox tabs and navigate them > with the keyboard (on OSX). Like if I tab through into the toolbox it jumps > straight into a tool. Should I be able to access these tabs with the > keyboard? Ah. Sorry, I should've mentioned... In order to be able to use keyboard fully with browser on OSX you need to set Full Keyboard Access to All Controls in Keyboard System Preferences. Completely forgot that since I have that by default.

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins https://reviewboard.mozilla.org/r/37673/#review39823 OK now that I've enabled the keyboard navigation locally this makes much more sense. I'm curious of the reason for making arrow keys move through the toolbar instead of having them be part of the 'normal' tab / shift+tab ordering (not requesting it change, just curious) ::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:31 (Diff revision 2) > + is(aDoc.activeElement.id, id, "New control is focused"); > +} > + > +add_task(function*() { > + info("Create a test tab and open the toolbox"); > + let tab = yield addTab(TEST_URL); These three lines can be replaced with: let toolbox = yield openNewTabAndToolbox(TEST_URL, "webconsole" (from framework/test/shared-head.js) ::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:37 (Diff revision 2) > + let target = TargetFactory.forTab(tab); > + let toolbox = yield gDevTools.showToolbox(target, "webconsole"); > + let doc = toolbox.doc; > + > + let toolbar = doc.querySelector(".devtools-tabbar"); > + let toolbarControls = Array.prototype.filter.call( Could alternatively use [...toolbar.querySelectorAll(".devtools-tab, toolbarbutton")].filter(...) ::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:43 (Diff revision 2) > + toolbar.querySelectorAll(".devtools-tab, toolbarbutton"), elm => > + !elm.hidden && doc.defaultView.getComputedStyle(elm).getPropertyValue( > + "display") !== "none"); > + > + // Put the keyboard focus onto the first toolbar control. > + toolbarControls[0].focus(); Will need to check this on Try (since you have additional keyboard accessibility enabled locally). The other day I ran into an issue where for some reason calling .focus() on some elements in OSX didn't actually focus them, as is ominously referenced here https://developer.mozilla.org/en-US/docs/Web/API/Document/activeElement: "Note: On Mac, elements that aren't text input elements tend not to get focus assigned to them." ::: devtools/client/framework/test/browser_toolbox_keyboard_navigation.js:91 (Diff revision 2) > + // Move the focus back to the toolbar, ensure we land on the last active > + // descendant control. > + EventUtils.synthesizeKey("VK_TAB", { shiftKey: true }); > + testFocus(doc, toolbar, expectedFocusedControl); > + > + yield toolbox.destroy(); These two cleanup lines shouldn't be needed since it's handled in a cleanupFunction in shared-head ::: devtools/client/framework/toolbox.js:948 (Diff revision 2) > + // next/previous focusable element relative to toolbar itself. > + if (event.shiftKey) { > + elm = toolbar; > + type = Services.focus.MOVEFOCUS_BACKWARD; > + } else { > + // To move focus to next element following the toolbar, relative Huh, I would have expected we could use `toolbar` as the element for movement in either direction. ::: devtools/client/shared/developer-toolbar.js:110 (Diff revision 2) > requisition.updateExec(typed); > }, false); > > + button.addEventListener("keypress", event => { > + if (event.key === " ") { > + // Ensure click is not fired (which happens in some cases). What's this a fix for? Anyway, seems unrelated to the rest of the changes since it's touching GCLI and not the toolbox, so I'd prefer handling in another bug / patch if possible. ::: devtools/client/themes/toolbars.css:693 (Diff revision 2) > > #toolbox-controls-separator { > margin: 0; > } > > +.toolbox-dock-button { There's a selector that already hits the dock buttons and close button here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.css#612 So the declaration could be put there and removed from .toolbox-dock-button and .devtools-closebutton ::: devtools/client/themes/toolbars.css:753 (Diff revision 2) > > #command-button-scratchpad > image { > background-image: url("chrome://devtools/skin/images/command-scratchpad.png"); > } > > +#command-button-pick { This element has the .command-button class on it so this rule should be unneeded

(In reply to Brian Grinstead [:bgrins] from comment #19) > Comment on attachment 8725882 [details] > MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar > keyboard accessible. > > https://reviewboard.mozilla.org/r/37673/#review39823 > > OK now that I've enabled the keyboard navigation locally this makes much > more sense. I'm curious of the reason for making arrow keys move through > the toolbar instead of having them be part of the 'normal' tab / shift+tab > ordering (not requesting it change, just curious) This is primarily for navigation efficiency. Tabbing is a good way to navigate between form fields and widgets (such as toolbars, menus etc). Where arrow keys is a good way widgets implement keyboard navigation internally. So in devtools context: You can get to a secondary toolbar or the tool itself with 1-2 tabs (once you are in devtools) instead of doing as many tab as the number of toolbar controls. Hope this helps.

https://reviewboard.mozilla.org/r/37673/#review39823 > Huh, I would have expected we could use `toolbar` as the element for movement in either direction. So the focus manager will try to move focus to the next focusable element, even if it is in the current active element's subtree.

https://reviewboard.mozilla.org/r/37673/#review39823 > What's this a fix for? Anyway, seems unrelated to the rest of the changes since it's touching GCLI and not the toolbox, so I'd prefer handling in another bug / patch if possible. For whatever reason, space does not trigger a click event, which is typical for controls. I have no idea why not tbh, but you're right, it is probably a different bug (since it's not a navigation issue).

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37673/diff/2-3/

--- devtools/client/inspector/breadcrumbs.js | 36 +++++++++++++++ devtools/client/inspector/inspector-search.js | 9 +++- devtools/client/inspector/test/browser.ini | 2 + .../browser_inspector_breadcrumbs_keyboard_trap.js | 52 ++++++++++++++++++++++ .../inspector/test/browser_inspector_search-05.js | 4 +- .../test/browser_inspector_search-navigation.js | 8 ++-- .../test/browser_inspector_search_keyboard_trap.js | 48 ++++++++++++++++++++ devtools/client/inspector/test/head.js | 15 +++++++ 8 files changed, 168 insertions(+), 6 deletions(-) create mode 100644 devtools/client/inspector/test/browser_inspector_breadcrumbs_keyboard_trap.js create mode 100644 devtools/client/inspector/test/browser_inspector_search_keyboard_trap.js Review commit: https://reviewboard.mozilla.org/r/43293/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43293/

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37673/diff/3-4/

Comment on attachment 8736447 [details] MozReview Request: Bug 1242852 - (part 2) making inspector toolbar keyboard accessible. r=gl Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43293/diff/1-2/

Comment on attachment 8736447 [details] MozReview Request: Bug 1242852 - (part 2) making inspector toolbar keyboard accessible. r=gl https://reviewboard.mozilla.org/r/43293/#review41023 ::: devtools/client/inspector/breadcrumbs.js:300 (Diff revision 2) > + } else if (event.type == "focus") { > + this.handleFocus(event); > + } > + }, > + > + handleFocus: function(event) { Add a function comment about the focus handler. ::: devtools/client/inspector/inspector-search.js:293 (Diff revision 2) > this.searchBox.value = this.searchPopup.selectedItem.label; > this.hidePopup(); > } > break; > > + case event.DOM_VK_TAB: I am not quite sure this is our desired behavior when tabbing with the autocomplete popup open. I would expect the autocomplete to fill the searchbox and the next tab will move the focus. At the very least, I am very familar with the tab to autocomplete flow when a popup is opened and it is a common pattern looking at some editors. @bgrins, do you have any feelings toward this? ::: devtools/client/inspector/test/browser_inspector_breadcrumbs_keyboard_trap.js:5 (Diff revision 2) > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > +"use strict"; > + Please add a comment here describing what this file tests for. ::: devtools/client/inspector/test/browser_inspector_breadcrumbs_keyboard_trap.js:15 (Diff revision 2) > + * [ > + * {Boolean} flag, indicating if breadcrumbs contains focus > + * {String} key event's key > + * {?Object} Optional event data such as shiftKey, etc > + * ] > + * Remove this empty line in the test comments. ::: devtools/client/inspector/test/browser_inspector_breadcrumbs_keyboard_trap.js:17 (Diff revision 2) > + * {String} key event's key > + * {?Object} Optional event data such as shiftKey, etc > + * ] > + * > + */ > +const TEST_DATA = [ This is a bit of a weird style of handling TEST_DATA. We typically define an array of objects. I would prefer if we simply did that, so that we are very explicit about the values we are passing into EventUtils.synthesizeKey when looping through the TEST_DATA. ::: devtools/client/inspector/test/browser_inspector_breadcrumbs_keyboard_trap.js:32 (Diff revision 2) > + > +add_task(function*() { > + let { inspector } = yield openInspectorForURL(TEST_URL); > + let doc = inspector.panelDoc; > + > + info("Selecting the test node"); Remove this info. It should be clear we are selecting a note from the function name. ::: devtools/client/inspector/test/browser_inspector_search-05.js:37 (Diff revision 2) > yield processingDone; > > info("Wait for search query to complete"); > yield inspector.searchSuggestions._lastQuery; > > - info("Press tab to fill the search input with the first suggestion"); > + info("Press enter to fill the search input with the first suggestion"); This change will likely be reverted if we go back to tabbing to fill the search input. ::: devtools/client/inspector/test/browser_inspector_search_keyboard_trap.js:5 (Diff revision 2) > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > +"use strict"; > + Add a comment describing the test case. ::: devtools/client/inspector/test/browser_inspector_search_keyboard_trap.js:9 (Diff revision 2) > +"use strict"; > + > +const TEST_URL = URL_ROOT + "doc_inspector_search.html"; > + > +/** > + * Test data has the format of: Use array of objects for TEST_DATA. ::: devtools/client/inspector/test/head.js:614 (Diff revision 2) > return def.promise; > } > + > +/** > + * Checks if document's active element is within the given element. > + * @param {HTMLDocument} aDoc document with active element in question We want to avoid aVariable naming style. Rename to doc and elm. ::: devtools/client/inspector/test/head.js:621 (Diff revision 2) > + * @return {Boolean} > + */ > +function containsFocus(aDoc, aElm) { > + let elm = aDoc.activeElement; > + while (elm) { > + if (elm === aElm) { return true; } Put return true; on a new line.

https://reviewboard.mozilla.org/r/43293/#review41023 > I am not quite sure this is our desired behavior when tabbing with the autocomplete popup open. I would expect the autocomplete to fill the searchbox and the next tab will move the focus. At the very least, I am very familar with the tab to autocomplete flow when a popup is opened and it is a common pattern looking at some editors. > > @bgrins, do you have any feelings toward this? I just wanted to add a little bit of context. There is now a visible concept of a focused alement within the autocomplete textbox. Focus can be seen on the intput field itself or inside of the autocomplete popup. If the focus is in the popup (the user stepped into it), tabbing will behave as you described (tab will select from popup and sequential tap will focus on the next element). The case here is when the focus is on the input field istelf. I would like to argue that an expected behaviour here would be to focus on next rather than select, but I am happy to only move focus if the popup is closed.

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins https://reviewboard.mozilla.org/r/37673/#review41459

(In reply to Yura Zenevich [:yzen] from comment #28) > https://reviewboard.mozilla.org/r/43293/#review41023 > > > I am not quite sure this is our desired behavior when tabbing with the autocomplete popup open. I would expect the autocomplete to fill the searchbox and the next tab will move the focus. At the very least, I am very familar with the tab to autocomplete flow when a popup is opened and it is a common pattern looking at some editors. > > > > @bgrins, do you have any feelings toward this? > > I just wanted to add a little bit of context. There is now a visible concept > of a focused alement within the autocomplete textbox. Focus can be seen on > the intput field itself or inside of the autocomplete popup. If the focus is > in the popup (the user stepped into it), tabbing will behave as you > described (tab will select from popup and sequential tap will focus on the > next element). The case here is when the focus is on the input field istelf. > I would like to argue that an expected behaviour here would be to focus on > next rather than select, but I am happy to only move focus if the popup is > closed. I'd say move focus only if the popup is closed. This is consistent with the rest of the autocomplete popups in the tools.

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37673/diff/4-5/

Comment on attachment 8736447 [details] MozReview Request: Bug 1242852 - (part 2) making inspector toolbar keyboard accessible. r=gl Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43293/diff/2-3/

https://hg.mozilla.org/integration/mozilla-inbound/rev/61e35a492b0d https://hg.mozilla.org/integration/mozilla-inbound/rev/16747842afff

I had to back these out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25431526&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7b395dbfca

Comment on attachment 8725882 [details] MozReview Request: Bug 1242852 - (part 1) making top dev tools toolbar keyboard accessible. r=bgrins Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37673/diff/5-6/

Comment on attachment 8736447 [details] MozReview Request: Bug 1242852 - (part 2) making inspector toolbar keyboard accessible. r=gl Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43293/diff/3-4/

OK , updated patches, should fix the test failure.

https://hg.mozilla.org/integration/mozilla-inbound/rev/09fb56bc976c https://hg.mozilla.org/integration/mozilla-inbound/rev/43a78545f93f

https://hg.mozilla.org/mozilla-central/rev/09fb56bc976c https://hg.mozilla.org/mozilla-central/rev/43a78545f93f