Closed Bug 1335192 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Shared Components, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

(Depends on 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

Need to improve semantics and keyboard a11y for the tree-view component.
Summary: Improve accessibility of devtools/client/shared/components/tree-view → Improve accessibility of devtools/client/shared/components/tree/tree-view.js
Attached patch 1335192 patch (obsolete) — Splinter Review
Attachment #8831938 - Flags: review?(odvarko)
Component: Developer Tools → Developer Tools: Shared Components
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.
Flags: needinfo?(odvarko)
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?
Attachment #8831938 - Flags: review?(odvarko)
(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.
(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
(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.
Attached patch 1335192 patch v2Splinter Review
Hopefully addressed your comments. The perf one IMO is a separate bug (this one only makes it more visible).
Attachment #8831938 - Attachment is obsolete: true
Attachment #8848349 - Flags: review?(odvarko)
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
Attachment #8848349 - Flags: review?(odvarko) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47dfce832d75
improving accessibility of tree-view component (keyboard and semantics). r=Honza
https://hg.mozilla.org/mozilla-central/rev/47dfce832d75
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1395193
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.