express-labs / pure-react-carousel

A highly impartial suite of React components that can be assembled by the consumer to create a carousel with almost no limits on DOM structure or CSS styles. If you're tired of fighting some other developer's CSS and DOM structure, this carousel is for you.
https://express-labs.github.io/pure-react-carousel/
MIT License
1.68k stars 161 forks source link

Aria role listbox not suitable for Carousels #112

Open bluSCALE4 opened 5 years ago

bluSCALE4 commented 5 years ago

The argument has been made here:

https://github.com/twbs/bootstrap/issues/22061

mrbinky3000 commented 5 years ago

There is a discussion going on in the PR. Unlike bootstrap's carousel, our carousel captures keyboard events so you can use the left and right arrows to navigate. When you assign event handlers like onkeydown and onclick to a non-interactive HTML element, like a div, then Aria requires that you assign the element a role. At least that's my understanding of WCAG.

listbox seemed like an option because you can tab to each visible slide in the slider. However, the audio that is read by a screen reader is a little misleading. It speaks of "options" to select. There's no options.

If anyone has an idea of which role we should use in this situation, please speak up.

tim-steele commented 5 years ago

@bluSCALE4 please see the response on PR #113 - I think we don't need to remove the listbox, but I think we could discuss if tablist would be a better option?

bluSCALE4 commented 5 years ago

@tim-steele I agree, tablist looks like a much better option for dots. But there's still the issue that a listbox semantically represents a <select />. It makes sense to use role="listbox" is the carousel is a "scrolling" selection of products, sizes or models but not advertisement or any of the samples provided. Here's what I was reviewing:

http://www.webaxe.org/carousels-and-aria-tabs/

  • Each carousel content container has a role of tabpanel.
  • Each control, typical designed as dots, has a role of tab.
  • The container of the controls/tabs has a role of tablist.
  • Add aria-labelledby to the tabpanels which point to the id of the associated control/tab.
  • To each control/tab, add aria-controls (which points to the id of the associated tabpanel) and aria-selected (boolean) attributes.

This is assuming that we're willing to draw a logical connection between a carousel and tabs. It does have similarities though a carousel includes additional functionality.

bluSCALE4 commented 5 years ago

How you guys looked at role: grid? Seems to be the catch all of ARIA widget roles. https://www.w3.org/TR/wai-aria-1.1/#grid

edit Though W3C claims grid doesn't necessarily need to follow a table format, NVDA announces 0 rows and 0 columns even if you have aria-roledescription in place. Other grid examples don't make this announcement but they do include rows and columns.

bluSCALE4 commented 5 years ago

Into the rabbits hole we go... found this recent conversation at aria-practices. There's a ongoing conversation that I'll be adding to momentarily.

https://github.com/w3c/aria-practices/issues/43

There is another example that lurked in the code that I recreated.

https://w65wynpw25.codesandbox.io/

What both these examples have avoided is adding actions listeners to the slides themselves so they successfully avoid dealing with role. On the plus side, they have lots of clever solutions not yet in place.

tim-steele commented 5 years ago

@bluSCALE4 it does look like they are including the spec. I am reviewing their carousel recommendation here https://www.w3.org/TR/wai-aria-practices-1.1/#carousel. I don't think it is final yet.

RTG-MColeman commented 5 years ago

It is also important to note that many developers will over use a slider, not as a pure carousel but as a means of selection (ex: a product that has 10 different colors) in this case your list box maybe useful. Also if it is required that a selection be made then maybe even role="radiogroup" to help convey that this is part of a form selection.

There is also possibilities of expanding the useful meaning of the buttons for screen readers (ex: next button should read "Next 3 of 10" or however many slides as visible VS remaining) Same for previous first and last."First 1 of 10" / "Last 10 of 10"

Also usage of tab seems to be throwing the slider off. In your demo of "Simple Carousel With React Redux" attempt to only use the tab key to move through the slides. The proper order should be Slide 1,2,3 then Next arrow. However as you can see the tab order continues to push the slider all the way through the slides before ever getting to the next button. Even then when the user reaches the next button they do not know they are at the end of the ordered list because the next button is still a valid button. Usage of aria-hidden=true/false and tabindex=0/-1 will keep the hidden slides out the tab order.

Good luck!

hcientist commented 3 years ago

As this doesn't currently have a clear solution, might it make sense for now to default torole="listbox" (remaining consistent with the current implementation, but to permit passing in props to override it (e.g. if their use case matches different guidance)? Currently it's not possible to override. Similarly, perhaps the aria-roledescription could be passed through to the element?

tim-steele commented 3 years ago

@hcientist would you be open to making a Pull Request for making it possible to override? @kylane mentions in Issue #315 the ability to override aria props, so it looks like it something that has support.

TwisterMc commented 2 years ago

Where have we landed on this one? Removing role="listbox" seems to be a good start or giving us a way to overwrite it.

misterbracket commented 2 years ago

We found a very basic workaround to solve these issues for our purposes.

Without going into too much detail on how the roles work, for us, it was enough to make this work with role="listbox". This change did it for us.

            {/* Adding role={undefined} and trayProps={{role: 'listbox'}} fixes an issue 
                with accessibility. Basically, the role attribute was on the wrong element.
                The role 'listbox' needs need wrap direct decedents with the role 'option'.
                We hope that this gets addressed by the library soon. */}
            <Slider
                aria-orientation={carouselState.orientation}
                aria-labelledby={carouselId}
                classNameTrayWrap="carousel-tray-wrapper"
                role={undefined}
                trayProps={{role: 'listbox'}}
            >
            ...
            </Slider>