1428432 - Add even more keyboard support for the Tree component.

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

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 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1428432 patch

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

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 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1428432.1 part 1

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1428432 part 2 v2

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Dorel Luca [:dluca]]

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

Comment 9

[Chris Mills [:cmills]]

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!

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

(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.

Comment 11

[Chris Mills [:cmills]]

(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 ;-)