HTML ToolSidebar should support ARIA

VERIFIED FIXED in Firefox 51

Status

P1
enhancement
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: Honza, Assigned: rickychien)

Tracking

unspecified
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [reserve-html][good taipei bug])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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

2 years ago
Whiteboard: [devtools-html]
(Reporter)

Updated

2 years ago
Depends on: 1259819
Severity: normal → enhancement

Updated

2 years ago
Blocks: 1263741
Flags: qe-verify?
Whiteboard: [devtools-html] → [devtools-html] [triage]

Updated

2 years ago
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [reserve-html]
(Reporter)

Updated

2 years ago
Whiteboard: [reserve-html] → [devtools-html] [triage][good taipei bug]
(Reporter)

Updated

2 years ago
Whiteboard: [devtools-html] [triage][good taipei bug] → [reserve-html][good taipei bug]

Updated

2 years ago
Duplicate of this bug: 1284509
(Assignee)

Updated

2 years ago
Assignee: nobody → rchien

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs

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

2 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

2 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

2 years ago
Priority: P3 → P2
(Assignee)

Comment 5

2 years ago
Marco, do you have a good test example which has similar steps checking aria-* stuff?
Flags: needinfo?(mzehe)

Comment 6

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

2 years ago
Attachment #8772701 - Flags: a11y-review+

Comment 8

2 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

2 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.
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)
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/
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+
Hi Ricky, see my comments above
Flags: needinfo?(yzenevich)
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)
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)
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 on attachment 8772701 [details]
Bug 1286283 - support ARIA for devtools inspector tabs

Taking this off for now.
Attachment #8772701 - Flags: review?(yzenevich)
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)
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).
Attachment #8772701 - Flags: review?(yzenevich)
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)
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"
Attachment #8772701 - Flags: review?(yzenevich)
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)
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 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)
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/
Attachment #8772701 - Flags: review?(bgrinstead) → review?(yzenevich)
(Assignee)

Updated

2 years ago
Attachment #8772701 - Flags: review?(bgrinstead)
(Assignee)

Updated

2 years ago
Attachment #8772701 - Flags: review?(yzenevich)
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+
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

2 years ago
Flags: needinfo?(odvarko)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 33

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e776b32b7c2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

2 years ago
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1

Updated

2 years ago
QA Contact: adalucin → cristian.comorasu
I could not find any issue trying to reproduce this bug. The Toolsidebar seems to support ARIA.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.