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

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

Open
micmania1 opened this issue Feb 12, 2019 · 24 comments
Open

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

micmania1 opened this issue Feb 12, 2019 · 24 comments

Comments

@micmania1
Copy link

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
Copy link

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
Copy link

isGabe commented Apr 3, 2019

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
Copy link

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.
#4 (comment)

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
Copy link

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

@jkphl
Copy link

jkphl commented Aug 20, 2019

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
Copy link

robskrob commented Sep 5, 2019

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
Copy link
Owner

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
Copy link

mollylentz commented Sep 6, 2019

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
Copy link
Owner

ganlanyuan commented Sep 6, 2019

@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?

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".

But sometimes there are more than one visible slides

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.

Check this

@jkphl
Copy link

jkphl commented Sep 6, 2019

@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
Copy link
Owner

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
Copy link

jkphl commented Sep 6, 2019

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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

@jkphl
Copy link

jkphl commented Sep 6, 2019

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
Copy link

kfalencikRealise commented Oct 16, 2019

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
Copy link

masi commented Jan 30, 2020

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
Copy link

Abdull commented Apr 2, 2020

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

@Chi-teck
Copy link

Chi-teck commented Jun 18, 2020

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

@nebulousGirl
Copy link

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
Copy link

odyn commented Apr 25, 2023

Where do you put this code?

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

@MrBlank
Copy link

MrBlank commented Aug 18, 2023

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);
}

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

No branches or pull requests