Here are some additional things that would make the Tree even more awesome for accessibility: * Home key selects first node in the tree * End key selects last node in the tree * Allow specifying custom action for Space/Enter on a key.

Comment on attachment 8947606 [details] [diff] [review] 1428432 patch Review of attachment 8947606 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Yzen, these vhanges looks good. r- until there are tests, but then it will be good. Could you add a test for : - Hitting Home / End when at random position - Hitting Home when already on the first node - Hitting End when already on the last node - Hitting Enter when no onActivate prop is passed - Hitting Enter with a onActivate prop is passed ? Also, could you file a bug in https://github.com/devtools-html/devtools-core/issues so we can make the same changes in the Tree component we have there ? Thanks ! ::: devtools/client/shared/components/VirtualizedTree.js @@ +328,5 @@ > /** > * Updates the state's height based on clientHeight. > */ > _updateHeight() { > + if (this.refs.tree.clientHeight && Is this fixing a bug ? Looks like it could be its own commit if so

Comment on attachment 8948780 [details] [diff] [review] 1428432 part 2 v2 Review of attachment 8948780 [details] [diff] [review]: ----------------------------------------------------------------- That's great, thanks a lot yura. I only have a few nits and a couple of comment for checking isActivated, but this is good to land. ::: devtools/client/shared/components/test/mochitest/test_tree_12.html @@ +18,5 @@ > +<script type="application/javascript"> > + > +"use strict"; > + > +window.onload = Task.async(function* () { nit: could we switch to use a native async function (i.e. without the Task.async wrapper) @@ +27,5 @@ > + const Tree = > + createFactory(browserRequire("devtools/client/shared/components/VirtualizedTree")); > + > + function renderTree(props) { > + const treeProps = Object.assign({}, nit: Use object spread operator: ```js const treeProps = { ...TEST_TREE_INTERFACE, onFocus: x => renderTree({ focused: x }) } ``` @@ +35,5 @@ > + ); > + return ReactDOM.render(Tree(treeProps), window.document.body); > + } > + > + let isActivated; nit: change it to `isFocused` or change the argument name from `focused` to `activated` so we have consistent naming @@ +91,5 @@ > + "-N:false", > + "--O:false", > + ], "After the Home key again, A should still be focused."); > + > + // End --------------------------------------------------------------------- nit: Write "Test End key" or something similar, for 5s I was wondering what was ending here ^^ @@ +140,5 @@ > + > + // Enter ------------------------------------------------------------------- > + > + info("Press Enter to activate node, when onActivate is not passed."); > + isActivated = null; could we assign a Symbol to isActivated so we know it wasn't assigned a falsy value in mockFn ? ```js const checker = Symbol(); isActivated = checker; ... ok(isActivated === checker, ...) ``` (we use `ok` because `is` can't compare with a Symbol) @@ +193,5 @@ > + > + // Space ------------------------------------------------------------------- > + > + info("Press Space to activate node, when onActivate is not passed."); > + isActivated = null; ditto for checking with Symbol

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a06c67ea6a prevent unnecessary VirtualizedTree render when the client height does not change. r=nchevobbe https://hg.mozilla.org/integration/mozilla-inbound/rev/034c72b76050 further improve keyboard accessibility for VirtualizedTree. r=nchevobbe

Pushed by yura.zenevich@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/53218d52fdfb Fix for mochitest chrome failures at devtools/client/shared/components/test/mochitest/test_tree_12.html. CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/e1a06c67ea6a https://hg.mozilla.org/mozilla-central/rev/034c72b76050 https://hg.mozilla.org/mozilla-central/rev/53218d52fdfb

I've documented this: Added a note saying where to find the relevant keyboard shortcuts: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#HTML_tree Added the new keyboard shortcut details (search for "Firefox 60"): https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#HTML_pane Added a note to the Fx60 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/60#Developer_tools Can you let know if this is OK? I didn't really understand the bit about "Allow specifying custom action for Space/Enter on a key." Thanks!

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #9) > I've documented this: > > Added a note saying where to find the relevant keyboard shortcuts: > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/ > Examine_and_edit_HTML#HTML_tree > > Added the new keyboard shortcut details (search for "Firefox 60"): > https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#HTML_pane > > Added a note to the Fx60 rel notes: > https://developer.mozilla.org/en-US/Firefox/Releases/60#Developer_tools > > Can you let know if this is OK? I didn't really understand the bit about > "Allow specifying custom action for Space/Enter on a key." > > Thanks! Hi Chris, I these might be a different components. The tree mentioned in this bug is used in the new debugger's sidebar and I believe debugger source tree. Keyboard controls for the markup tree documented here were added a number of releases back, though have never been documented and it's great to have them now! End button does not have the expected behavior in the markup tree, as far as I can tell.

(In reply to Yura Zenevich [:yzen] from comment #10) > Hi Chris, I these might be a different components. The tree mentioned in > this bug is used in the new debugger's sidebar and I believe debugger source > tree. Ah ;-) Is that enabled yet? I can't seem to get these commends to work there. > Keyboard controls for the markup tree documented here were added a number of > releases back, though have never been documented and it's great to have them > now! At least it wasn't a wasted effort then ;-)