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

Review commit: https://reviewboard.mozilla.org/r/65440/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65440/

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

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

Marco, do you have a good test example which has similar steps checking aria-* stuff?

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

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)?

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

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.

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.

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/

WIP test case: https://reviewboard.mozilla.org/r/65440/diff/2#2

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.

Hi Ricky, see my comments above

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/

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!

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?

Comment on attachment 8772701 [details] Bug 1286283 - support ARIA for devtools inspector tabs Taking this off for now.

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/

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

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/

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 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"

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.

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/

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.

OK, I'll remove r?you from commit.

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/

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

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/

push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=672a9069fd56

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/5e776b32b7c2 support ARIA for devtools inspector tabs. r=bgrinstead

I could not find any issue trying to reproduce this bug. The Toolsidebar seems to support ARIA.