ganlanyuan / tiny-slider

Vanilla javascript slider for all purposes.
MIT License
5.25k stars 786 forks source link

Accessibility - nav controls don't have a natural usage #378

Open micmania1 opened 5 years ago

micmania1 commented 5 years ago

Issue description: Nav controls don't provide clear instructions for use and interactive elements can't be tabbed to (ie. prev/next buttons).

For non-sighted users, the container might be good enough to use for controlling the slides, however there's no prompt for the user to tell them that they can use the left and right arrow keys to switch slides. We can make better use of the aria-label here or potentially use aria-description, however I don't think that has full support across screen readers.

For sighted keyboard users, aria-labels won't matter and the user will naturally try to navigate to the buttons. However, there's no way for the user to get there since the tabindex is set to -1.

Tiny-slider version: latest

dovematrix commented 5 years ago

I too would appreciate the tiny-slider updated to increase the accessibility such that slide thumbnail images can be navigated using the tab key on the keyboard. Tab to go to the next thumbnail image to the right and shift tab to go to the next thumbnail image to the left.

isGabe commented 5 years ago

Yeah this is a real problem for keyboard accessibility. We've been using this library on a project, and we're going to have to find something else since this one hijacks tabindex and prevents normal keyboard navigation.

allendotson commented 5 years ago

The implementation that was done here is completely wrong. This recently caused a headache for me, and I needed to modify my local copy to remove all the tabindex changes. https://github.com/ganlanyuan/tiny-slider/issues/4#issuecomment-304251600

The navigation buttons need to have either no tabindex set, or a tabindex of 0. The navigation container should not be tabbable. If the button is currently disabled, then mark it as aria-disabled, but do not change the tabindex to make it inaccessible.

mollylentz commented 5 years ago

We've also recently been dinged by this - please if this could be a priority, that would be great.

jkphl commented 5 years ago

This one's is truly killing accessibility at the moment. I completely agree with @allendotson here: the navigation buttons need to be focusable to be comparably operable for both keyboard and non-keyboard users. The arrow key navigation (which requires the control container to be focusable) is nice but must be explained, otherwise no one will get it and rather be confused because of the seemingly unnecessary tab. I'd recommend a container with a clear instruction referenced by aria-describedby and potentially even made visible when the control container is focused. See https://inclusive-components.design/a-content-slider/ for a working example.

robskrob commented 5 years ago

Ok so the only thing that seems to be working for me so far -- shoutout to @jibinycricket for pointing me in this direction -- is wrapping the carousel slider images in buttons as such on the onInit callback of the settings object which is somewhat obscurely documented here -- I found this documented here:


var sliderSettings = {
  onInit: function(info) {
    Array.from(document.querySelectorAll('.Product__images__thumbnail-wrapper.tns-item.tns-slide-active')).forEach(function(carouselImageContainerElement) {
          let buttonElement = document.createElement('button');
          buttonElement.classList.add('Product__carousel_button');
          buttonElement.tabIndex = 0;

          buttonElement.setAttribute('type', 'button');

          carouselImageContainerElement.appendChild(buttonElement);

          buttonElement.appendChild(carouselImageContainerElement.querySelector('img'))
      })
  }
}
tns(sliderSettings);

Coding cleanliness aside 🙂 this at least lets me tab through the active carousel images after the slider gets created.

Not too sure if it's the best solution 🤷‍♂ but it gets the job done for now. Of course, I will have to add accessibility attributes to the button to be more compliant but I think the above example is a start.

In the same callback, I can overwrite the tabindex=-1 attribute on the prev / next buttons, but I choose to only make the visible carousel images tab-able.

ganlanyuan commented 5 years ago

I think the only thing lack here is the instructions for non-sighted users -- to let them know they could control the slider with arrow, enter, blank keys. To remove all of the tab-indexs for me is not efficient, because users HAVE TO tab through all the controls and dots to go to next section on the page. aria-disabled shouldn't be used here because the content is still accessible, it just not tabbable.

For the thumbnail images, you could use customized nav and put these thumbnails there. The nav items are not necessarily be button.

mollylentz commented 5 years ago

A few other things to address:

  1. There's something wonky happening in the aria-live region where it will say "slide 6 of 5" which is confusing.

  2. The controls have each been marked up with an aria-label like: “carousel page 1”. Users may not be clear why the carousel has different "pages". It may be misleading because you are referring to the individual slides on the same page. A better approach could be saying something like "carousel slide 1".

  3. I've added my own code for this on our site, but it would be nice if this was looked into: if you have links/buttons inside of your slides, the user will be able to tab to all of them (even the slides that are not showing). It would be beneficial to add a tabindex="-1" to all links inside the slides that are not active and set it to tabindex="0" on link/buttons of the currently active slide.

The above is feedback we received from actual manual testing with a screen reader by non-sighted users.

ganlanyuan commented 5 years ago

@mollylentz Thanks for your feedbacks.

1. There's something wonky happening in the aria-live region where it will say "slide 6 of 5" which is confusing.

Could you submit a new issue with the slider settings, so that I can check for details?

  1. The controls have each been marked up with an aria-label like: “carousel page 1”. Users may not be clear why the carousel has different "pages". It may be misleading because you are referring to the individual slides on the same page. A better approach could be saying something like "carousel slide 1". But sometimes there are more than one visible slides

  2. I've added my own code for this on our site, but it would be nice if this was looked into: if you have links/buttons inside of your slides, the user will be able to tab to all of them (even the slides that are not showing). It would be beneficial to add a tabindex="-1" to all links inside the slides that are not active and set it to tabindex="0" on link/buttons of the currently active slide. Check this

jkphl commented 5 years ago

@ganlanyuan I don't agree with that it's enough to explain to non-sighted users that the slider can be operated with the keyboard and make the visible controls inaccessible at the same time. Please bear in mind that not every screenreader user is non-sighted. Please check these:

As mentioned before, if you aren't already familiar with @heydon's https://inclusive-components.design/a-content-slider/, I very much recommend it as a comprehensive outline of various principles.

ganlanyuan commented 5 years ago

I have a question: What would you do if there are a lot of slides (let's say, 40)? Will you still want to display all the dots?

jkphl commented 5 years ago

What would you do if there are a lot of slides (let's say, 40)? Will you still want to display all the dots?

Assuming that I don't misunderstand you question: as so often, it depends. I think this is implementation specific (and possibly a matter of taste as well). However, it should under no circumstances be the library that's making an assumption about whether something is useful for keyboard or screenreader users or not. If there are visible controls (i.e. the dots and / or the prev / next buttons), these controls must be operable with the keyboard / alternative means as well. There should not be a significant difference depending on the input method in use.

The question of displaying 40 dots or not is worth a discussion in its own right. I consider this a matter of design and usability, affecting all users, but nothing limited to accessibility aspects.

ganlanyuan commented 5 years ago

Yes, it's not related to accessibility, but I have to consider this if I'm going to change the behaviors of nav and controls.

ganlanyuan commented 5 years ago

For the screenreader users, they need access to every slides, but for non-screenreader users, they just need a few dots, each dot points to a page with several slides. So there is a conflict. I was struggled on this when I was designing this project and finally I choose the easy way for non-screenreader users.

ganlanyuan commented 5 years ago

So I want to get more opinions on this. And the tab-index issue, probably will be fixed on next big release.

jkphl commented 5 years ago

I'm not sure if the others involved in this issue all have the same problem as we do. As far as I'm concerned, it's rather simple: I need to be able to access the prev / next buttons with the keyboard (I can see them so I expect to be able to operate them with the keyboard as well), which currently isn't possible as you added a tabindex="-1" to all links and buttons. Simply removing the negative tabindex resolved our issue. Our use case is fairly simple, and given our setup I couldn't find any drawbacks so far.

kfalencikRealise commented 4 years ago

This works for me:

onInit() {
    document.querySelector('.tns-controls').setAttribute('tabindex', -1);
    document.querySelector('[data-controls="prev"]').setAttribute('tabindex', 0);
    document.querySelector('[data-controls="next"]').setAttribute('tabindex', 0);
}

Actually even better solution (would work with multiple carousels on the page):

onInit(info) {
    info.controlsContainer.setAttribute('tabindex', -1);
    info.nextButton.setAttribute('tabindex', 0);
    info.prevButton.setAttribute('tabindex', 0);
}   
masi commented 4 years ago

I tried to access the first demo slider with the keyboard. As the keyboard focus is nearly invisible it was an unpleasant experience.

I understand that you tried hard to make keyboard access fun for sighted users, but it was unclear to me when I had to use the tab key and when to use the cursor keys.

I cannot speak for everyone, but for me it came as a surprise that I have to use cursors keys to move from one pagination bullet to the next. I didn't try it with a screen reader, but as there is no ARIA markup to announce a widget I guess it will be hard to understand.

I fear the good intentions make currently not a good experience. But I have play around with NVDA and JAWS to get a real grasp how non-sighted users can use the slider.

Abdull commented 4 years ago

W3C provides recommendations on how to set up an accessible carousel according to WCAG and ARIA principles. They also have a basic example.

Abdull commented 4 years ago

I fixed this in https://github.com/Abdull/tiny-slider/tree/gesis .

Chi-teck commented 4 years ago

I think the current implementation may cause troubles for non-SR users that do not use mouse for navigation.

nebulousGirl commented 2 years ago

The problem here is that using the arrow keys is not the expected behavior for a sighted user. The trade off here would be to allow the prev/next buttons to be tabbable, but not the dots which I can see as more of a visual cue of location and less as an interactive element.

odyn commented 1 year ago

Where do you put this code?

I'm seeing the broad issue is still not resolved for the out-of-the-box plugin?

MrBlank commented 1 year ago

Having the same dilemma. This seems to work for me, even if it's a bit hacky.

Thanks for a great slider script, @micmania1 Michael Strong!

onInit(info) { // tabindex fix
    // next/prev buttons
    info.controlsContainer.setAttribute('tabindex', -1);
    info.nextButton.setAttribute('tabindex', 0);
    info.prevButton.setAttribute('tabindex', 0);

    // Nav buttons (dots)
    var modifyNavItemsTabindex = function(info) {
        for (const [key, value] of Object.entries(info.navItems)) {
            value.setAttribute('tabindex', 0);
        } 
    }
    modifyNavItemsTabindex(info); 
    // Nav buttons change event ('slider' is the name of your slider object var)
    slider.events.on('transitionStart', modifyNavItemsTabindex);
}