Closed
Bug 1286283
Opened 8 years ago
Closed 8 years ago
HTML ToolSidebar should support ARIA
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox51 verified)
Tracking | Status | |
---|---|---|
firefox51 | --- | verified |
People
(Reporter: Honza, Assigned: rickychien)
References
Details
(Whiteboard: [reserve-html][good taipei bug])
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
bgrins
:
review+
yzen
:
a11y-review+
|
Details |
This is a follow up for bug 1259819
From Yura:
https://bugzilla.mozilla.org/show_bug.cgi?id=1259819#c35
Just managed to play around with the patch and wanted to give some suggestions regarding accessibility that is now missing since xul had it build in. Some things to watch for:
* .tabs-menu must have a role="tablist" attribute
* .tabs-menu-item must have a role="tab" attribute
* .tabs-menu-item must have an aria-controls attribute with a value referencing an id of the corresponding tab panel
* active .tabs-menu-item must have an aria-selected attribute set to "true" while the other ones have it set to "false"
* .tab-panel must have a role="tabpanel" attribute
* .tab-panel must have an aria-labelledby attribute with a value referencing an id of the corresponding tab element
For more details please see:
https://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/#tabpanel
https://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/#dualfocus
Honza
Reporter | ||
Updated•8 years ago
|
Whiteboard: [devtools-html]
Severity: normal → enhancement
Updated•8 years ago
|
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [reserve-html]
Reporter | ||
Updated•8 years ago
|
Whiteboard: [reserve-html] → [devtools-html] [triage][good taipei bug]
Reporter | ||
Updated•8 years ago
|
Whiteboard: [devtools-html] [triage][good taipei bug] → [reserve-html][good taipei bug]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rchien
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/65440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65440/
Attachment #8772701 -
Flags: review?(yzenevich)
Assignee | ||
Comment 3•8 years ago
|
||
Hi Yura, here my patch for supporting ARIA attributes as you mentioned!
(In reply to Jan Honza Odvarko [:Honza] from comment #0)
> * .tab-panel must have a role="tabpanel" attribute
>
> * .tab-panel must have an aria-labelledby attribute with a value referencing
> an id of the corresponding tab element
>
However, I'm confused about this part you said. The .tab-panel doesn't make sense to have an aria-labelleby on itself since you already moved it to an independent Panel component [1]. So I suspect that you were saying is adding aria-labelledby attributes in dev.panels [2]. If I'm wrong please correct me. :)
[1] https://hg.mozilla.org/mozilla-central/diff/f249501590e6/devtools/client/shared/components/tabs/tabs.js#l1.323.
[2] https://hg.mozilla.org/mozilla-central/diff/f249501590e6/devtools/client/shared/components/tabs/tabs.js#l1.288
Comment 4•8 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #3)
> (In reply to Jan Honza Odvarko [:Honza] from comment #0)
> > * .tab-panel must have a role="tabpanel" attribute
> >
> > * .tab-panel must have an aria-labelledby attribute with a value referencing
> > an id of the corresponding tab element
> >
>
> However, I'm confused about this part you said. The .tab-panel doesn't make
> sense to have an aria-labelleby on itself since you already moved it to an
> independent Panel component [1]. So I suspect that you were saying is adding
> aria-labelledby attributes in dev.panels [2]. If I'm wrong please correct
> me. :)
Yes, the aria-labelledby that goes onto the tab-panel references the ID of the tab. Your patch does the right thing, I just looked. :-) I'll leave the actual review to Yura, but wanted to clear this up right away. :-)
One thing you might want to do is add tests that checks when tabs are switched the aria-selected gets re-set correctly, the roles are present, etc.
Updated•8 years ago
|
Priority: P3 → P2
Assignee | ||
Comment 5•8 years ago
|
||
Marco, do you have a good test example which has similar steps checking aria-* stuff?
Flags: needinfo?(mzehe)
Comment 6•8 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #5)
> Marco, do you have a good test example which has similar steps checking
> aria-* stuff?
Yes, you can look at the tests for the markup viewer, introduced in bug 1242694. There it's TreeView semantics and some other bits, but it shows that the tests test for aria attributes like role or aria-selected etc.
Flags: needinfo?(mzehe)
Comment 7•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
https://reviewboard.mozilla.org/r/65440/#review62938
this looks good with 1 question about keyboard: was there a specific reason to use focus on anchor inside a tab rather than a tab itself? Perhaps it makes sense to tabindex -1 all the anchors inside and handle tabindex on tabs themselves (just a suggestion)?
Attachment #8772701 -
Flags: review?(yzenevich) → review+
Updated•8 years ago
|
Attachment #8772701 -
Flags: a11y-review+
Comment 8•8 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #7)
> this looks good with 1 question about keyboard: was there a specific reason
> to use focus on anchor inside a tab rather than a tab itself? Perhaps it
> makes sense to tabindex -1 all the anchors inside and handle tabindex on
> tabs themselves (just a suggestion)?
Or alternatively, mark the actual li elements with role "presentation" instead and make the focusable anchors inside the tabs with role="tab", as I showed in my blog post about this topic: https://www.marcozehe.de/2013/02/02/advanced-aria-tip-1-tabs-in-web-apps/
Assignee | ||
Comment 9•8 years ago
|
||
Cool! I'll apply li element with role "presentation". And it seems that creating a new test case for verifying accessibility isn't so easy to me so that it would take some time to figure out how to write test.
Assignee | ||
Comment 10•8 years ago
|
||
I guess I was wrong to spent time in writing mochitest (Is it equivalent to unit test?) for the tabs react component.
I encountered failure with `SyntaxError: invalid property id` when I was trying to create fake tabs by invoking `yield setState()`. I attached my WIP test case please point out the right way. Maybe I was wrong at the beginning, and perhaps a browser testing would be more intuitive to write accessibility testing.
Flags: needinfo?(yzenevich)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65440/diff/1-2/
Assignee | ||
Comment 12•8 years ago
|
||
WIP test case:
https://reviewboard.mozilla.org/r/65440/diff/2#2
Comment 13•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
https://reviewboard.mozilla.org/r/65440/#review63610
::: devtools/client/shared/components/test/mochitest/test_tabs_accessibility.html:27
(Diff revision 2)
> +
> + const tabbar = ReactDOM.render(Tabbar(), window.document.body);
> +
> + yield setState(tabbar, {
> + tabs: [
> + {id: "0", title: "tab-0", {}},
There's syntax error here and below, last item in the object {} results in parsing error.
I would suggest to install eslint or other similar tool so you would easily catch similar errors.
You can also run the test with --jsdebugger flag to be able to debug it (and can put debugger; statement inside the js part of your test).
::: devtools/client/shared/components/test/mochitest/test_tabs_accessibility.html:35
(Diff revision 2)
> + {id: "3", title: "tab-3", {}}
> + ],
> + activeTab: 1
> + });
> +
> + is(document.querySelector("li[aria-selected=true]"), "tab-1", "Default aria-selected=true is tab-1");
This will always yield false because you're comparing a DOMNode to a string.
Attachment #8772701 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65440/diff/2-3/
Attachment #8772701 -
Flags: review?(yzenevich)
Assignee | ||
Comment 16•8 years ago
|
||
Yura, thank you for these comments and it was really helpful! However, I'm still stuck in react mochitest today.
My current WIP patch always throw:
Error: Minified exception occurred; use the non-minified dev environment for the full error message and additional helpful warnings.
and it doesn't provide any useful messages to me so that I can't figure out what the root cause is it.
WIP: https://reviewboard.mozilla.org/r/65440/diff/3#2
I noticed a weird thing at line 31. set `activeTab: 1` here will cause above error but it would disappear if set to `activeTab: 4`.
I wonder how do you guys debug with react mochitest and how can I deal with such strange error message?
thanks!
Flags: needinfo?(yzenevich)
Comment 17•8 years ago
|
||
Hi Ricky, I think you need to use a InspectorTabPanel component when you are passing a panel field when setting a new state for the tabbar (similar to how toolsidebar.js does it when tabs are added). I haven't seen the error you mentioned when running locally and instead I got:
INFO TEST-UNEXPECTED-FAIL | devtools/client/shared/components/test/mochitest/test_tabs_accessibility.html | Got an error: Invariant Violation: Objects are not valid as a React child (found: object with keys {props}). If you meant to render a collection of children, use an array instead or wrap the object using createFragment(object) from the React add-ons. Check the render method of `Tabs`.
I generally debug these tests with --jsdebugger flag when running a test and putting some debugger; statements in the test. Perhaps there are other more efficient ways, Jan would you know?
Flags: needinfo?(yzenevich) → needinfo?(odvarko)
Comment 18•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Taking this off for now.
Attachment #8772701 -
Flags: review?(yzenevich)
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65440/diff/3-4/
Attachment #8772701 -
Flags: review?(yzenevich)
Assignee | ||
Comment 20•8 years ago
|
||
Thanks for your helpful information again!
I ended up finishing mochitest after importing InspectorTabPanel. However, stepping into debugger with --jsdebugger still not enough for me to figure out root cause and got same error message too.
Patch has been updated and please take a look. thanks!
Comment 21•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
https://reviewboard.mozilla.org/r/65440/#review64602
I tried your test and it seems to pass. Since I'm not a devtools peer you would need to request and actual review from someone on the Devtools team. They will have some feedback about the tests, I'm sure but please mark me again for a11y-review in further iterations so I coudl make sure that we add the a11y stuff correctly. Thanks
::: devtools/client/shared/components/tabs/tabs.js:206
(Diff revision 4)
> DOM.li({
> ref: ref,
> key: index,
> - className: classes},
> + id: "tab-" + index,
> + className: classes,
> + role: "presentation",
At this point we lost the tab semantics again.
What Marco was suggesting is that we put role presentation on the li (which you did now) *and* move tab semantics (along with aria-contorls and aria-selected) to a child anchor element.
So what you would need to do is to move the role, aria-contorls and aria-selected to around line 214.
Also in tests I would also test that role and aria-controls attributes are also set correcty (in addition to aria-selected).
Attachment #8772701 -
Flags: review?(yzenevich)
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65440/diff/4-5/
Attachment #8772701 -
Flags: review?(yzenevich)
Attachment #8772701 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 23•8 years ago
|
||
Hi Nick!
Because there is a new test case introduced into this bug, I'd like to set you as a reviewer and to hear your feedback from it.
It's a small test case for checking a11y stuff.
Comment 24•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
https://reviewboard.mozilla.org/r/65440/#review64802
::: devtools/client/shared/components/tabs/tabs.js:210
(Diff revisions 4 - 5)
> - "aria-selected": isTabSelected
> },
> DOM.a({
> href: "#",
> tabIndex: this.state.tabActive === index ? 0 : -1,
> - onClick: this.onClickTab.bind(this, index)},
> + role: "presentation",
So this should stay in the li.
And the role for an anchor should be "tab"
Attachment #8772701 -
Flags: review?(yzenevich)
Comment 25•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Hey! I'm not really familiar with the inspector, so I'm forwarding the review.
Attachment #8772701 -
Flags: review?(nfitzgerald) → review?(bgrinstead)
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65440/diff/5-6/
Attachment #8772701 -
Flags: review?(bgrinstead) → review?(yzenevich)
Comment 27•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Sorry , I can't actually sign off on the review, since I'm not dev tools peer. As for me this now looks good from a11y standpoint so it's an a11y-review+ from me.
Attachment #8772701 -
Flags: review?(yzenevich) → review?(bgrinstead)
Assignee | ||
Comment 28•8 years ago
|
||
OK, I'll remove r?you from commit.
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65440/diff/6-7/
Attachment #8772701 -
Flags: review?(bgrinstead) → review?(yzenevich)
Assignee | ||
Updated•8 years ago
|
Attachment #8772701 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•8 years ago
|
Attachment #8772701 -
Flags: review?(yzenevich)
Comment 30•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
https://reviewboard.mozilla.org/r/65440/#review65466
Nice, works for me
::: devtools/client/shared/components/tabs/tabs.js:206
(Diff revision 7)
> DOM.li({
> ref: ref,
> key: index,
> - className: classes},
> + id: "tab-" + index,
> + className: classes,
> + role: "presentation"
Nit: include trailing comma after last property
::: devtools/client/shared/components/tabs/tabs.js:214
(Diff revision 7)
> href: "#",
> tabIndex: this.state.tabActive === index ? 0 : -1,
> - onClick: this.onClickTab.bind(this, index)},
> + "aria-controls": "panel-" + index,
> + "aria-selected": isTabSelected,
> + role: "tab",
> + onClick: this.onClickTab.bind(this, index)
Nit: include trailing comma after last property
::: devtools/client/shared/components/tabs/tabs.js:266
(Diff revision 7)
> key: index,
> + id: "panel-" + index,
> style: style,
> - className: "tab-panel-box"},
> + className: "tab-panel-box",
> + role: "tabpanel",
> + "aria-labelledby": "tab-" + index
Nit: include trailing comma after last property
Attachment #8772701 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65440/diff/7-8/
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(odvarko)
Assignee | ||
Comment 32•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 33•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5e776b32b7c2
support ARIA for devtools inspector tabs. r=bgrinstead
Keywords: checkin-needed
Comment 34•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•8 years ago
|
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Updated•8 years ago
|
QA Contact: adalucin → cristian.comorasu
Comment 35•8 years ago
|
||
I could not find any issue trying to reproduce this bug. The Toolsidebar seems to support ARIA.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•