Closed
Bug 1335192
Opened 8 years ago
Closed 8 years ago
Improve accessibility of devtools/client/shared/components/tree/tree-view.js
Categories
(DevTools :: Shared Components, defect)
DevTools
Shared Components
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)
(deleted),
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
Need to improve semantics and keyboard a11y for the tree-view component.
Assignee | ||
Updated•8 years ago
|
Summary: Improve accessibility of devtools/client/shared/components/tree-view → Improve accessibility of devtools/client/shared/components/tree/tree-view.js
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8831938 -
Flags: review?(odvarko)
Component: Developer Tools → Developer Tools: Shared Components
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b58a25f6e05365a08ca4fb2fca59842052db8f
Honza
Flags: needinfo?(odvarko)
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
(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•8 years ago
|
||
(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
Assignee | ||
Comment 7•8 years ago
|
||
(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.
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•