goetsu commented 6 years ago

This is an issue about accessibility problems that are due to slick code structure and behaviors.

.slider :

remove role="toolbar" because it's not relevant here. Sliders are not toolbars see https://www.w3.org/TR/wai-aria-1.1/#toolbar

.slick-prev, .slick-next :

remove role="button" as you already use button element,
when slider on infinite loop mode content inside prev button on first slide need to be "Go to last slide" otherwise user will not know it's looping
when slider on infinite loop mode content inside next button on last slide need to be "Go to first slide" otherwise user will not know it's looping
by default I recommend to use "Previous slide" and "Next slide" as it's more understandable that just "Previous" and "Next"
by default I recommend to move focus on the new active slide (or first new one in case of multiple items) when user activate .slick-prev / .slick-next specially when there is focusable elements inside active slide
move hidden label inside those button inside a <span class="sr-only"> see https://hugogiraudel.com/2016/10/13/css-hide-and-seek/ because font-size="0" can make the content not speakable in some screenreader

.slick-list :

remove aria-live at least when autoplay is on otherwise it will announce the content everytime current slide is changed even is user is elsewhere on the page
when not in autoplay I also recommend to remove it and instead to move focus on the new active slide (or first new one in case of multiple items) when user activate .slick-prev / .slick-next or dots navigation button specially when there is focusable elements inside active slide

.slick-track :

remove role="listbox" because it's not relevant here. Sliders are not listbox see https://www.w3.org/TR/wai-aria-1.1/#listbox You can use role="list" if you want a to provide list semantic but will make only sense if you have multiple item per slide.

.slick-slide :

remove role="option" because it's not relevant here as listbox is not relevant You can use role="listitem" if you want a to provide list semantic but will make only sense if you have multiple item per slide.
replace aria-describedby by aria-labelledby because label inside corresponding dot is more a label than a description
remove this aria-describedby/labelledby when there is no dots navigation

.slick-dots :

remove role="tablist" because it's not relevant here. Dots navigation is not a tablist see https://www.w3.org/TR/wai-aria-1.1/#tablist
on <li> inside it remove aria-hidden (because they need to be readable), role="presentation" (because otherwise you loose <li> semantics, aria-selected (because not focusable), aria-controls (because not focusable)
on button inside <li> remove role="button" (because useless), tabindex="0" (because useless)
on button inside .slick-active add aria-current="true", this dot need also to visually be bigger than others otherwise there is information by color only
by default I recommend to use "Slide x" where x= number as content inside button because it's more understandable for users than 1,2,3, etc
by default I recommend to move focus on the new active slide (or first new one in case of multiple items) when user activate button specially when there is focusable elements inside active slide
move hidden label inside those button inside a <span class="sr-only"> see https://hugogiraudel.com/2016/10/13/css-hide-and-seek/ because font-size="0" can make the content not speakable in some screenreader

optional :

could be great to have an option to add aria-describedby to button inside .slick-dots and to .slick-prev, .slick-next this way it will allow to add an id value of a hx before the carousel. Specially usefull when you have multiple carousel on the same page with different kind of content inside. For example if you have a news carousel then a products carousel and both of the have those title as hx user will know that there is "next slide news" button "next slide products" button and they are not the same
on autoplay mode when focus is inside .slider you need to pause the carousel automatically otherwise user may not have enough time to read it before moving to next slide

ahmadalfy commented 6 years ago

Very interesting, thanks. I would love to take care of this

fleps commented 6 years ago

Hello,

This is a very interest list.

I'm currently facing problems on a project that needs AA validation and is failing because two things from slick slider:

the slick-track "role" attribute that is not necessary and invalid
the <li> inside the slick-dots that has the role presentation, aria-selected and aria-controls.

As my deadline is Jan 31 what's the proper way I can fix this on my own? We are using the mini version of the plugin, should I switch to the normal version and fix this on the JS, or implement a node change after the page loads?

Any suggestion is appreciated, thanks.

TwisterMc commented 6 years ago

I would also love to see these added for better accessibility.

bregosch commented 6 years ago

Hi @kenwheeler, any chance that the accessibility of slick gets improved? Maybe anyone (including myself) could provide you with a pull request?

anybodesign commented 6 years ago

hi, very interesting request!

I would add:

.slick-slide:

be able to remove tabindex when the slide contains a link

ksudac commented 6 years ago

Hello, I would also like to see these accessibility improvements. I'm running into issues/flags with listbox.

Jakobud commented 6 years ago

Why in the world is this here?

https://github.com/kenwheeler/slick/blob/9325b64f3ce8af474e7958e7bf61345d16ffb3c9/slick/slick.js#L1325

Does not make sense at all why Slick would make everything in it's slides not accessible to keyboards...... I just don't get decisions like this.

fleps commented 6 years ago

@Jakobud that specific line is for the cloned slides I think, which is correct. The only slider that should be focusable is the active one, which it is, the keyboard navigation should then jump to the next arrow (if enabled) or to the bullet navigation to navigate to the next slide.

Jakobud commented 6 years ago

Whenever I unslick() the carousel at specific breakpoints, it appears to add tab-index="-1" to all those elements in all my (previous) slides. Can't figure out how to turn this off.

goetsu commented 6 years ago

If you turn that off it mean that you will force keyboard user to go trough all your slides. I suppose that's not the case for your mouse user so why do you want to impose that to keyboard user. Only the current slide need to be keyboard focusable. I keyboard user want to access the others he will have to use the navigation button like mouse user

Jakobud commented 6 years ago

I'm talking about when you unslick the carousel at various breakpoints. When I am at desktop resolution, the carousel unslick's itself, and then sets all the buttons and important things in my image containers to the -1 tabindex, destroying accessibility. Make sense?

kryp71c commented 6 years ago

Hi @goetsu,

Good list. I would add that the entire carousel should be wrapped in a labeled region to provide context for the carousel elements. W3 uses a section element with an aria label in their tutorials and a figure element in their working example: https://www.w3.org/WAI/tutorials/carousels/structure/ Without any context it is impossible to know what the previous and next buttons actually control (as you mention in your optional points).

I would also argue that the autoplay option would need an additional mechanism for suspending autoplay than just hovering over the carousel or focusing on a dot. Hovering is only accessible for mouse users, and for neither keyboard or mouse users is it readily apparent that focusing on a dot or hovering will interrupt the animation (also, why pause on dot focus but not on previous/next buttons?). (perceivable, operable, understandable, and robust)

Wholeheartedly agree with your live area remarks, there are few things more annoying than content being forced on you when you're not expecting it and can't immediately stop it (pet peeve: auto play videos).

Can I ask why using role="tablist" is not a good way to represent a carousel? There are a few accessibility examples of carousels out there that use the tablist concept (this one for example). In terms of functionality it seems to be very similar (every linked item reveals and focuses exactly one container of content).

goetsu commented 6 years ago

@kryp71c for labeled region I agree but I don't think it must be done by the script itself because best solution would be to use a hx not a aria-label on the section

for pause I ask to pause when focus is inside the whole carrousel not only on dots and yes a real pause button is also needed but slick already offer this possibility so not something to add to the script

for role tablist it's an usability decision base on multiple reason :

your carousel don't look like tabs so why would you like to make them work like it
many user still don't know how to operate ARIA tabs specially
most of the time carousel aren't use in an application so it make no sense to use a desktop app pattern.

But if you want to go for it it's fine if you fully respect the aria design pattern (not the case with slick right now)

kryp71c commented 6 years ago

@goetsu did you mean mobile application in your third point? Wouldn't you then just simply "unslick" the slider? Of course, as @Jakobud pointed out, unslicking apparently renders slide items inaccessible at the moment by setting any tabbable content to tabindex=-1.

@goetsu when you say slick already offers a pause button possibility did you mean the provided methods? It would be nice to have an option to add the pause/play button when autoplay is true, instead of having to resort to custom code (unless I'm somehow overlooked that this already exists). You make a similar point when you suggest the default nav buttons should include the word "slide", since you already have the option of creating custom nav markup.

To adhere to the tab ARIA design pattern the reading order would have to be changed (dotnav first then slides). Since the dotnav list semantics are currently removed by applying role=presentation, the list should probably be removed altogether in favor of a div with a role=tablist that wraps all the buttons. Each button should have a role=tab. This W3C example illustrates the concept.

I think the navigation buttons, ultimately, might be problematic for the tablist approach, since they don't exist for tabs. If we have the tablist first, the tabpanels should immediately follow them, but then where can we fit the nav buttons to make them useful for screen reader users? At present the previous/next buttons also seem confusing to use with a keyboard, since you have to tab through the currently active slide to get to the "next" button.

fleps commented 6 years ago

Just to add my 2 cents here.

A project that I'm working on hired a specialized Accessibility Consultant company and they did an extensive QA on the project and sent us a report with all the issues.

Regarding the carousel, they reported many issues to meet the compliance level AA:

Mandatory

Carousels should either have a play/stop accessible button or they should have auto-play OFF. (As I'm particularly not a fan of auto-play sliders because they are bad user experience overall, we convinced our client to turn them off on the project so this was solved easily)
There's not particular "role" defined for carousels, using "toolbar" is not correct neither any other role, so the request was to remove "role" for all elements of the carousel.
The carousel should have a different markup structure. Each carousel item should be a list item inside an un-ordered element (<ul><li>)
The carousel navigation bullets, when enabled, should have a visible focus indicator (outline should not be disabled) and they should not have the aria-hidden=true.
no aria-live regions should be presented on the carousel

Nice to Have

The carousel arrow navigation's use CSS content to se the visual, which is read by screen readers. Change the code to set the visual on a child element with aria-hidden=true.
The bullet navigation buttons only have the numeric number of the carousel set on the hidden text. Update it to a more comprehensive message like "Navigate to Slider X"
The navigation bullets/button element should be presented BEFORE the carousel on the DOM order, so the screen reader users can access the pagination first.
if a slider have an anchor inside it, the slider itself should not have tabindex=0 or adds an unnecessary additional step for keyboard navigation

We fixed all mandatory issues on our own and now our carousels are in compliance, and will work on some from the nice to have list.

Hope it helps.

kryp71c commented 6 years ago

@fleps that sounds great. Did you contribute your fixes back to this project in some way? Is there some place one could see the changes you ended up making. Was the consultant able to review the changes you made?

fleps commented 6 years ago

@kryp71c

The consultant validated the changes, on a page after our PR got merged, but we will have a final review once we finish all other accessibility improvements on the project, which will take a while.

Unfortunately the changes are on a close environment and until the consultant do the final review i'll avoid making any changes / PR's here on the project.

But if you look on the mandatory list the needed changes are pretty straight forward (considering the same scenario of not using auto-play ON). I basically removed some role attributes, removed the aria-hidden where they should no be and any aria-live, changed the slider html wrappers from div to ul > li and adjusted the navigation focus on the CSS.

Hope it helps.

anybodesign commented 6 years ago

Hello, it would be great to add the attribute "disabled" on slick arrows when they are in disabled state. Because otherwise they are still getting the focus on keyboard navigation. For example:

if (_.options.infinite !== true) {
   _.$prevArrow
   .addClass('slick-disabled')
   .attr('aria-disabled', 'true')
   .attr('disabled', 'disabled');
}

aronmoshe-m commented 6 years ago

I use Slick in a lot of my projects and would love to see it updated to meet AA standards. +1 for this issue.

annetters commented 6 years ago

I'm surprised that navigation dots have a role="presentation". Doesn't this cause the dots to be ignored, suggesting that they're purely decorative elements?

fleps commented 6 years ago

@anevaude Considering the amount of open issues and open PR's, it's unlikely that this will ever happen as the project seems abandoned.

Your best bet is to fix the issues by yourself with some tips from this thread.

khurramkim commented 5 years ago

can you please tell me how to remove these roles cause i am targeting these roles through jquery but it didn't change

cagline commented 5 years ago

it’s not navigating to Items of the carousel due to unnecessary tabindex=”-1” this need to be change to tabindex=”0” ..... along with aria-hidden="false"

ahmadalfy commented 5 years ago

There’re currently multiple PRs addressing accessibility concerns and we’re planning to merge them after review. Some are even not tagged yet. We will get this done with the next version

cespelage commented 5 years ago

@ahmadalfy do you have an idea when the next version will be released? Many of the accessibility issues mentioned above are areas we're trying to improve on our site for ADA compliance. Thanks in Advance!

gaambo commented 4 years ago

Hi there all of you, for a project we needed a lot of slick sliders and a11y was very important. Therefore i tried to implement most of the things discussed here. I've forked the repo - you can find it here: Slick a11y fixes.

I've implemented most of the things @goetsu mentioned in the op. but most importantly @fleps list was implemented by me.

It's important to mention we never use infinite mode or autoplay - that's why I haven't tested them in any way. I also had to create some flags to check for some things (eg if the slide change is coming from an arrow or a dot - if it's changed by an arrow it focuses the new slide, if it's changed by an arrow it focuses the selected slide). This is by no means perfect or finished and there's definately things to optimize. Some changed I made (flags,...) may also seem hacky.

Maybe someone finds this useful. I'm not ready yet to create a PR because it's not as thoroughly tested enough. I've created this for a project rather quick.

EDIT: also, i only edit scss & less files (because in our project we import them and build them in our own gulp tasks) - the .css files are not updated.

annetters commented 4 years ago

@gaambo Thank you for all your hard work!

wixaw commented 4 years ago

Thanks @gaambo @ahmadalfy @kenwheeler can you merge this please ?

gaambo commented 4 years ago

Thanks @gaambo @ahmadalfy @kenwheeler can you merge this please ?

like i said, i didn't even create a pull request. the contributing.md states to test in all scenarios/cases and i didn't do that, especially because i never use infinite or autoplay mode. so more extensive tests and code reviews would be needed before even considering it merge-ready.

wixaw commented 4 years ago

Yes I understood, I would like them to test this and integrate what is functional :pray: knowing that they know the details of the product ^^

BNewing commented 4 years ago

I would be happy to help out with testing and code reviews on any of this accessibility work if a PR is raised :)

jamesarosen commented 4 years ago

I don't think this is covered in the list of issues above, but it's related.

If you apply Slick to an ordered- unordered list, you get invalid and inaccessible HTML.

<!-- before -->
<ul id='my-slides'>
  <li>Slide one</li>
  <li>Slide two</li>
  <li>Slide three</li>
</ul>

<!-- after -->
<ul id='my-slides'>
  <div class='slick-list draggable'>
    <div class='slick-track'>
      <div class='slick-slide slick-current slick-active'>
        <li>Slide one</li>
      </div>
      <div class='slick-slide'>
        <li>Slide two</li>
      </div>
      <div class='slick-slide'>
        <li>Slide three</li>
      </div>
    </div>
  </div>
</ul>

A ul can't contain a div and an li can't be the child of a div.

See #926

AkshayKadam006 commented 3 years ago

I am too facing an issue. I am not sure if anyone has covered this yet but if you have a youtube video as one of the slides which is not 1st one. the video has play button and 2-3 more accessible controls. if you keep on tabbing on your page, tab reaches to your video controlls and disrupts the whole sliding sequence. now when you click on dots instead of bringing the intended image in center, it brings some other. please note that "infinite is true" and slides to show is 1. if anyone knows how can we resolve this, please respond.

airplanenoise commented 2 years ago

https://github.com/Accessible360/accessible-slick may help you out.

aronmoshe-m commented 2 years ago

@airplanenoise Thanks, Kaz! I'm using that on other projects. It's helpful! I'd still love to see the accessibility issues address in the main project, however, since this is such a widely used dependency and WCAG compliance is the way forward.

+1 still for making these changes in this repo.

airplanenoise commented 2 years ago

Agreed! I just actually stumbled upon that build today after a salty Usability audit on a recent project. Gonna keep an eye here, we use slick on pretty much every job these days.

On Feb 4, 2022, at 9:15 PM, Aron @.***> wrote:

@airplanenoise Thanks, Kaz! I'm using that on other projects. It's helpful! I'd still love to see the accessibility issues address in the main project, however, since this is such a widely used dependency and WCAG compliance is the way forward.

+1 still for making these changes in this repo.

— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

kenwheeler / slick

Multiple accessibility issues and solutions to fix #3268

This is an issue about accessibility problems that are due to slick code structure and behaviors.