1335192 - Improve accessibility of devtools/client/shared/components/tree/tree-view.js

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

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

Need to improve semantics and keyboard a11y for the tree-view component.

Comment 1

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

Attachment: 1335192 patch

Comment 2

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

Jan, would it be possible to find a different reviewer in case you are busy with other stuff, this really blocks me on things bug 1151468 which is a dev-rel P1 bug. Thanks.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b58a25f6e05365a08ca4fb2fca59842052db8f

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8831938 [details] [diff] [review]
1335192 patch

Review of attachment 8831938 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch and sorry for delay with the review!

Please see my inline comments:

Also, I noticed that moving the selection using arrow keys is quite slow. It looks like changing the selection somehow re-renders the entire Tree. I am testing it in the DOM panel.

Honza

::: devtools/client/shared/components/tree/label-cell.js
@@ +53,5 @@
>            className: "treeLabelCell",
>            key: "default",
> +          style: rowStyle,
> +          role: "presentation"},
> +          span({ className: iconClassList.join(" "), role: "presentation" }),

nit: please put individual props on separate lines as follows:

span({
  className: iconClassList.join(" "),
  role: "presentation"
}),

::: devtools/client/shared/components/tree/tree-cell.js
@@ +117,5 @@
>        }
>  
>        // Render me!
>        return (
> +        td({ className: classNames.join(" "), role: "presentation" },

nit: props should be on separate lines

::: devtools/client/shared/components/tree/tree-row.js
@@ +15,5 @@
>    const TreeCell = React.createFactory(require("./tree-cell"));
>    const LabelCell = React.createFactory(require("./label-cell"));
>  
> +  // Scroll
> +  const { scrollIntoViewIfNeeded } = require("devtools/client/shared/scroll");

This breaks JSON Viewer since "devtools/client/shared/scroll" module doesn't support AMD environment. You might see e.g. the tree-row.js file - its content is enclosed with `define` function. 

If 'scroll.js' should be included it must also support AMD.

But, the question is, should the view auto-scroll to the selected line? Is that something we want by default?

Comment 5

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

(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> This breaks JSON Viewer since "devtools/client/shared/scroll" module doesn't
> support AMD environment. You might see e.g. the tree-row.js file - its
> content is enclosed with `define` function. 
> 
> If 'scroll.js' should be included it must also support AMD.
> 
> But, the question is, should the view auto-scroll to the selected line? Is
> that something we want by default?

I want to say yes since that's how inspector works and it has good keyboard support IMO (and also based of some twitter feedback). It would be an accessibility issue too for keyboard users only (not screen reader) since they will loose context of where they are in the widget.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Yura Zenevich [:yzen] from comment #5)
> I want to say yes since that's how inspector works and it has good keyboard
> support IMO (and also based of some twitter feedback). It would be an
> accessibility issue too for keyboard users only (not screen reader) since
> they will loose context of where they are in the widget.
OK, I see the point. So, the correct solution is to ensure that 'scroll.js' supports AMD.
(its content should be enclosed by `define` function) Just like e.g. tree-view.js module itself.

Honza

Comment 7

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

(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> Comment on attachment 8831938 [details] [diff] [review]
> 1335192 patch
> 
> Review of attachment 8831938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch and sorry for delay with the review!
> 
> Please see my inline comments:
> 
> Also, I noticed that moving the selection using arrow keys is quite slow. It
> looks like changing the selection somehow re-renders the entire Tree. I am
> testing it in the DOM panel.

I did some extensive profiling and what I see is:

Only the 2 rows that have their selected prop changed are re-renered. Everything else remains. The time spent (about half a second on DOM tab when selecting a row) is inside renderRows, where we render a row for each member. Because DOM props table has soo many rows it takes that long to go through all of them and determine if they need to be rendered or not.. Only 2 get re-rendered after that. 

I think this behavior existed before the patch too. We just didn't see it because one would only click (now also select) on the row to expand it and that would take quite a bit of time, thought to be spent on fetching DOM property children.

Comment 8

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

Attachment: 1335192 patch v2

Hopefully addressed your comments. The perf one IMO is a separate bug (this one only makes it more visible).

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8848349 [details] [diff] [review]
1335192 patch v2

Review of attachment 8848349 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Yura Zenevich [:yzen] from comment #8)
> Created attachment 8848349 [details] [diff] [review]
> 1335192 patch v2
> 
> Hopefully addressed your comments. The perf one IMO is a separate bug (this
> one only makes it more visible).
Thanks for the analysis! Can you please file a follow up for it,
so it isn't forgotten.

R+ assuming try is green

Thanks!

Honza

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47dfce832d75
improving accessibility of tree-view component (keyboard and semantics). r=Honza

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47dfce832d75