Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tabs accessibility #1446

Open
wjmao88 opened this issue Mar 26, 2019 · 3 comments
Open

Tabs accessibility #1446

wjmao88 opened this issue Mar 26, 2019 · 3 comments

Comments

@wjmao88
Copy link

wjmao88 commented Mar 26, 2019

  • components: TabContent, TabPane, Nav
  • reactstrap version 7.1.0
  • react version ^16
  • bootstrap version ^4

What is happening?

Tab panels created using existing components doesn't follow w3c recommendations.

What should be happening?

It should follow https://www.w3.org/TR/wai-aria-practices/#tabpanel
To summarize:

  • The nav buttons and tab panels should have tab related roles, e.g. tablist, tab, tabpanel, and have aria attributes to indicate relationships.
  • The tab buttons should support special keyboard interactions, similar to what Dropdown currently has.

Steps to reproduce issue

  1. goto https://reactstrap.github.io/components/tabs/
  2. compare to example in https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-2/tabs.html
@hoslersk
Copy link

hoslersk commented May 24, 2019

Long time fan, first time caller.

I want to figure out what makes the most sense, since I’ve never contributed here.

The Nav component has more than one use case (standard nav or tabs container), which complicates the enforced use of WCAG compliant attributes. We’ve all encountered navs that look like tabs and vice versa, which makes the current level of flexibility a major plus. That also means we can’t simply use the tabs prop as an indicator for when to add tab specific attributes.

To satisfy this issue it might make sense to make a new Tabs component that is a strict use case with built-in WCAG compliance. This would probably look more like the Carousel example in terms of how and what can be used as a child component. Of course, this would be a pretty big change.

Thankfully, the components are all flexible enough that the developer can (and should?) add and manage the necessary WCAG compliant attributes and behavior. I also took a look at the thread for #1013, which seems to align with this way of thinking. Perhaps this is just a matter of adding documentation and/or showing an example of a WCAG compliant implementation.

Thoughts?

TL;DR:

This might just be a matter of adding documentation and/or examples as discussed on #1013. This project is great because of its flexibility. I would argue that the developer should be in control of WCAG compliant attributes and behavior in this case.

A possible alternative would be a new Tabs component that is much more rigid and can only be used for true tabs (in terms of behavior). This would obviously require a lot of new code and tests.

@jansensan
Copy link

jansensan commented Dec 18, 2019

Many points mentioned by @hoslersk are important, e.g.:

  • "The Nav component has more than one use case (standard nav or tabs container)"
  • "[The] developer can (and should?) add and manage the necessary WCAG compliant attributes and behavior."

There are a few things that should be handled by reactstrap where possible.

Ensure role="tabpanel" is set by default to <TabPane>

The easiest I believe would be to ensure that <TabPane> automatically comes with role="tabpanel", as it is explicitly the intent of this component.

If a developer adds any other role, e.g.

<TabPane role="button">
    <div>content goes here</div>
</TabPane>

then the library should override its internal role and set whatever the developer applied, even if it is erroneous.

As for aria-labelledby="someDOMElementId" (which is necessary to identify which tab labels the panel) this would also be the responsibility of the developer to add to the <TabPane /> at instantiation.

Some references

Do we need <TabButton>?

The idea of using <NavLink>, mentioned in this discussion and in the example, bothers me.

The main reason is that <NavLink> renders an anchor (<a>), which is ill suited for most uses of tabs. Regarding accessibility, it has been documented many times that anchors and buttons are not freely interchangeable:

  • <a> creates a hyperlink to web pages;
  • <button> acts on the current page.

With this in mind, inviting developers to use an anchor onto which we add a click handler is making matters worse.

There is the case where the tabs actually lead to separate URLs. In this case, using anchors would make sense.

Ideally, a <TabButton> with some defaults set (i.e. role="tab", aria-selected="true/false") which would require the panel's id so it can set aria-controls={props.panelId} (or something like this) would resolve part of that issue.

There could also be a <TabList> component (as it is not a nav, unless it is composed of tabs that lead to separate URLs), which would have its default role set as role="tablist".

Having these components would likely create something like this, which reduces some boilerplate and ensures that semantically correct DOM elements are created:

<div>

  <TabList aria-label="Sample Tabs">
    <TabButton id="tab1" selected="true" controls="panel1" onClick={() => { toggle('1'); }}>
      First Tab
    </TabButton>
    <TabButton id="tab2" selected="false" controls="panel2" onClick={() => { toggle('2'); }}>
      Second Tab
    </TabButton>
    <TabButton id="tab3" selected="false" controls="panel3" onClick={() => { toggle('3'); }}>
      Third Tab
    </TabButton>
  </TabList>
  
  <TabContent activeTab={activeTab}>
    <TabPane tabId="1" id="panel1" tabindex="-1" labelledBy="tab1">
      <p>Content for the first panel</p>
    </TabPane>
    <TabPane tabId="1" id="panel2" tabindex="-1" labelledBy="tab2">
      <p>Content for the first panel</p>
    </TabPane>
    <TabPane tabId="1" id="panel3" tabindex="-1" labelledBy="tab3">
      <p>Content for the first panel</p>
    </TabPane>
  </TabContent>

</div>

<TabPane> should probably have tabindex="-1" set internally (and overridable) to reduce boilerplate and ensure that it is reachable via focus by JavaScript.

As @hoslersk mentioned, this means creating new components, which may be larger than the scope mentioned in @wjmao88's comments.

Or maybe they are exactly what is needed? I lean towards dedicated components. Since this library has a lot of semantically charged components, it stands to reason that a component (e.g. <NavLink>) should not try to overload it's meaning or function by doing a job that would be better served by another component.

Some references

Tabs documentation should not suggest anchors with onClick

While this point is discussed (expand current components functionalites vs. create new dedicated ones), I believe it would be important to update the tabs documentation to mention in which case should anchors or buttons be used, and have explicit examples to avoid spreading bad habits.

Those where many words :) Are you still reading? What do people think of these points?

@watinha
Copy link
Contributor

watinha commented Jul 19, 2020

Hey guys,

I have a few doubts in regards to this issue. I would also consider implementing new components for affording tab and tablist roles for the elements. However, this still leaves the issue description part in regards to keyboard interaction.

In order to implement standard ARIA keyboard interaction for this component, I think we would need a container element which aggregates the tab list, tab and tabpanel elements. And this component should manage all the interaction for the Tab Widget, with clicks, keyboard interaction and focus management.

I think this behavior would dramatically change the behavior of the current Tab Widget, which lets up to developers to implement the Widget behavior. Even so, do you guys think this would be still cool for the project? If you guys agree to this, I can try to implement that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants