search

nolimits4web / swiper

Most modern mobile touch slider with hardware accelerated transitions
https://swiperjs.com
MIT License
39.91k stars 9.75k forks source link

Improve Accessibility Features #3149

Closed fregiese closed 2 years ago

fregiese commented 5 years ago

This is a (multiple allowed):

Expected Behavior

The slider should implement some of the features and html structure of the w3c slider example: https://www.w3.org/TR/wai-aria-practices/examples/carousel/carousel-1/carousel-1.html The w3c example also has a list of reasons why the features and html structure they used is accessible. This would make SwiperJS a lot more accessible. Some of the accessibility enhancements can be set with options, but this as the default would help users who don't know how to make it more accessible. It would also be a good reason to use this library instead of other slider libraries, because SwiperJS would be a good option for everyone who wants to apply wcag 2.1 standards. https://www.w3.org/TR/WCAG21/

Some of the features could only be implemented in the accessibility module, for users who don't want to use them.

Actual Behavior

SwiperJS is only partially accessible.

Features to implement:

Related Issues

The w3c example doesn't use clones, but this issue is also important to improve accessibility: https://github.com/nolimits4web/swiper/issues/2929 Maybe the improved keyboard accessiblity would also fix issues like this: https://github.com/nolimits4web/swiper/issues/2945

philwolstenholme commented 5 years ago

Next and previous should be buttons as they don't take the user to a new page. They also don't need role="button" as the button element has an implied role.

I'm using SiteImprove (an automated a11y review tool) and it's actually penalising me at the moment for the role="button" added by Swiper.

A WAI-ARIA attribute that has the exact same features as the HTML element it has been applied to has been used. The WAI-ARIA attribute is redundant since is doesn't provide the user with any additional information.

For landmarks it has previously been a recommendation to use HTML5 and WAI-ARIA landmark roles together (e.g. WAI-ARIA role="navigation" on HTML5 'nav' elements) to maximize support, but with the widespread adoption of HTML5 this is no longer needed.

The WAI-ARIA attribute can be removed without any impact for end users. The result will be cleaner, easier to maintain code.

I disagree with the above slightly because of this IE11 bug but I think for all modern browsers the role isn't required.

jasonlav commented 4 years ago

The navigation next and previous buttons have click and keypress events. This causes navigation with a keyboard (pressing enter) to press the button twice.

ghost commented 4 years ago

Having tabindex="-1" on duplicated slides would be very helpful when using loop.

masi commented 4 years ago

Actually having tabindex="-1" on all focusable elements in invisible slides would be helpful. Currently I use custom code to manage the tabindex myself. I had assumed that a carousel that claims to be accessible would deal with plain links in slides.

For those who are wondering why this is an issue: if you tab through the slides you'll reach at one point a hidden slide. Then they will scroll into view. This breaks the layout and the state managment of Swiper, ie the wrong slides are "active".

masi commented 4 years ago

BTW aria-roledescription="carousel" is a plain text so it must be translatable if implemented. But aria-roledescription is not welcomed by all a11y advocates.

stefanfrede commented 4 years ago

Speaking of tabindex="-1", it would be nice if disabled navigation buttons would not be focusable.

masi commented 4 years ago

Sadly SwiperJS is yet another library that has a well-meant a11y-code, but no real interest or knowledge to make it actually work without barriers.

xcarlos commented 4 years ago

Actually having tabindex="-1" on all focusable elements in invisible slides would be helpful. Currently I use custom code to manage the tabindex myself. I had assumed that a carousel that claims to be accessible would deal with plain links in slides.

For those who are wondering why this is an issue: if you tab through the slides you'll reach at one point a hidden slide. Then they will scroll into view. This breaks the layout and the state managment of Swiper, ie the wrong slides are "active".

Hi masi, would you mind sharing your solution? I also have links in slides and when I tab trough them it breaks the layout. I'm trying to use their API to slideTo the current focused slide but it's not working.

xcarlos commented 4 years ago

Actually having tabindex="-1" on all focusable elements in invisible slides would be helpful. Currently I use custom code to manage the tabindex myself. I had assumed that a carousel that claims to be accessible would deal with plain links in slides. For those who are wondering why this is an issue: if you tab through the slides you'll reach at one point a hidden slide. Then they will scroll into view. This breaks the layout and the state managment of Swiper, ie the wrong slides are "active".

Hi masi, would you mind sharing your solution? I also have links in slides and when I tab trough them it breaks the layout. I'm trying to use their API to slideTo the current focused slide but it's not working.

Update: I was able to fix this by changing the scrollLeft property of the swiper container to 0

AAZANOVICH commented 4 years ago

Actually having tabindex="-1" on all focusable elements in invisible slides would be helpful. Currently I use custom code to manage the tabindex myself. I had assumed that a carousel that claims to be accessible would deal with plain links in slides.

For those who are wondering why this is an issue: if you tab through the slides you'll reach at one point a hidden slide. Then they will scroll into view. This breaks the layout and the state managment of Swiper, ie the wrong slides are "active".

Came to the same solution with tabindex="-1" for invisible slides, which logically make sense as well, since if element is not visible it should not be focusable.

Also, in IE 11 aria-* attributes are missed on navigation buttons.

philwolstenholme commented 4 years ago

Here's what I use to add tabindex="-1" to inactive slides:

First I use a polyfill for the inert HTML property:

<script src="https://polyfill.io/v3/polyfill.min.js?features=Element.prototype.inert"></script>

Then I have this function:

const makeAllButCurrentSlideInert = function makeAllButCurrentSlideInert() {
  const currentSlideEl = this.slides[this.realIndex];

  this.slides.each((index, slide) => {
    if (slide !== currentSlideEl) {
      slide.setAttribute("inert", "");
    } else {
      slide.removeAttribute("inert");
    }
  });
};

And I call it every time the carousel updates or is initiated:

const swiperOptions = {
  // …
  on: {
    init() {
      makeAllButCurrentSlideInert.call(this);
    },
    slideChange() {
      makeAllButCurrentSlideInert.call(this);
    },
    slideChangeTransitionEnd() {
      const currentSlideEl = this.slides[this.realIndex];
      currentSlideEl.setAttribute("tabindex", "-1");
      currentSlideEl.focus();
    },
  },
};

The slideChangeTransitionEnd function is a little bonus to set focus to the active slide whenever a transition takes place.

AAZANOVICH commented 4 years ago

The slideChangeTransitionEnd function is a little bonus to set focus to the active slide whenever a transition takes place.

I also used transitionEnd event, but also listened for keyboard navigation ('keydown'), since I was able to reproduce that issue when using keyboard navigation (arrows) and then moving focus quickly with tab/shift+tab

jenemde commented 4 years ago

According to the start post by @fregiese in this issue, the implemented list of new features is part of release 6.3.0. Please check also the documentation with the new parameters for accessibility in the documentation (https://swiperjs.com/api/#a11y)

philwolstenholme commented 4 years ago

3834 is the relevant PR for anyone who would like to take a look at the changes

vltansky commented 3 years ago

@philwolstenholme @fregiese are there things left to do? or the issue can be closed?

philwolstenholme commented 3 years ago

I think there are a few extra tasks:

fregiese commented 3 years ago

I updated the inital comment to show the current status

soniktrooth commented 3 years ago

I just noticed that Swiper is adding aria-role-description instead of aria-roledescription. Note the former isn't valid.

jenemde commented 3 years ago

Thanks. This has been commented here: https://github.com/nolimits4web/swiper/issues/4371 and fixed here: https://github.com/nolimits4web/swiper/commit/1b73c3b82dfd0b789650474bc5e13fd200115b28 in 6.5.1.

yordis commented 3 years ago

Be aware of https://github.com/akiran/react-slick/issues/1535

rafael-leal-mccormack commented 3 years ago

Just a quick question which also relates to accessibility, why dont the properties nextSlideMessage and prevSlideMessage also support replacing {{index}} in the string you pass in? I want it to say what the current slide number is when navigating or +1/-1 index depending on if its next or prev (ex. "Next slide, current slide is {{index}}").

If youre using a screen reader and press enter on the navigation buttons, it replaces the text in the swiper-notification element with the same text from the properties which does not cause the screen reader to say anything and gives users with screen readers no indication of a slide change. Am I doing something wrong or is there more than can be done for those accessibility properties?

yuheiy commented 3 years ago

I send related PR: https://github.com/nolimits4web/swiper/pull/4884

michaelbourne commented 3 years ago

Going through this now, when using slidesPerView and the slides have focusable elements, the results of tabbing to the next group are less than desirable. What would be good is if the slider were to simply toggle the next page, or assign the target slide on keypress as the active slide.

I got it working half way with the keypress event, but this seems to not trigger on a shift+tab key press. 

const swiperOptions = {
  // …
  on: {
    keyPress: function (swiper, code) {
      if ( 9 == code ) { 
        var focusEl = document.activeElement.closest('.swiper-slide');
        if ( null != focusEl && !focusEl.classList.contains('swiper-slide-active') ) {
          var slideIndex = Array.prototype.indexOf.call(focusEl.parentNode.children, focusEl);
          swiper.slideTo(slideIndex);
        }
      }
    },
  },
};

Would be great to get it working the opposite direction, right? Why isn't keyPress fired on shift+tab?

Anyways, plan B, let's use an event listener.

const slideFocus = function slideFocus(e) {
    if (e.key === 'Tab') {
        var focusEl = document.activeElement.closest('.swiper-slide');
        if (null != focusEl && !focusEl.classList.contains('swiper-slide-active')) {
            var slideIndex = Array.prototype.indexOf.call(focusEl.parentNode.children, focusEl);
            swiper.slideTo(slideIndex);
        }
    }
};

const swiperOptions = {
    // …
    on: {
        init: function (swiper) {
            swiper.el.addEventListener('keydown', slideFocus);
        }
    }
};

There we go, that seems to work. I can tab back and forth between items, I can tab in and out of my slider, and it all seems to work.

masi commented 3 years ago

The trick is to make sure all slides that are visible have tabindex="-1" on all focusable elements. Once they are visible restore the former value.

It is a bit more difficult than one expect in handlers because of the issues with the index in loop mode. There is no realIndex for the previous element. If you show more than one slide it becomes even more difficult as Swiper does not tell you how many and which elements are or have been visible.

arringtonm commented 3 years ago

Requesting addition of aria-current="true" to active swiper pagination bullet

shiivan commented 3 years ago

I know that the React/Vue/Other framework ports aren't really fully supported, but it would be helpful to make a note in the docs that most (if not all) of this a11y work isn't reflected in the DOM elements created by those ports. e.g. https://github.com/nolimits4web/swiper/blob/26340c4280790c509de499714c60009e35949c3d/src/react/swiper.js#L23

Gr0m commented 2 years ago

Hey guys, thanks for the update, but you forgot to mention in the docs, that "watchSlidesProgress" is now required when you have slidesPerView > 1. Otherwise any focus on slides other than the first one will immediately slideTo() this slide.

johnozbay commented 2 years ago

@Gr0m Thanks for catching this! 🙏🏻 Saved us hours of head-scratching. Linked to two other issues which are likely related to this as well!

sneko commented 1 year ago

Quick questions:

  1. why current slide doesn't have aria-current="true"? (I only saw this for the pagination bullets)
  2. why "we" kept "div" instead of using <ul> for the slider container and <li> for slides?
  3. the slider container is not <section> as checked in the TODO list of this thread, is there something special to do? It should have by default role="region" then I guess?
  4. the slider container has no "role" (whereas slides have role="group"), any reason for this? (maybe not necessary at the end if swiper was using <ul> + <li>?

(brand new to a11y, just curious :) )

masi commented 1 year ago

Quick questions:

1. why current slide doesn't have `aria-current="true"`? (I only saw this for the pagination bullets)

Because it won't tell the user something knew. If only one slide is visible the one will of course be the current one.

In the pagination things are different. A screen reader will read out all bullets and then it makes sense to say which one is the current one.

2. why "we" kept "div" instead of using `<ul>` for the slider container and `<li>` for slides?

IMHO it makes no sense. A list is announced with the number of items it contains. When only one slide is shown you will always stay on a lsit with a single item that changes with each next-slide action. It does not get much better if more than one slide is shown.

Sidenote: I found an example by W3C/WAI that does not agree with me on that. But my codes without LI has passed professionaly a11y audits. So using LI is at least not a must-have.

3. the slider container is not `<section>` as checked in the TODO list of this thread, is there something special to do? It should have by default `role="region"` then I guess?

I don't know why it is checked. At least for the VanillaJS version the other element is in the developers control. You can make it a

or add a role="region". But note that you need an accessible name as well to create a section/region.

The main point IMHO in using section/role is to hadd a hint that the user is about to enter a carousel widget.

But the same as for 2. applies.

4. the slider container has no "role" (whereas slides have `role="group"`), any reason for this? (maybe not necessary at the end if `swiper` was using `<ul> + <li>`?

Yes, with the group role you emphasise that the content within belongs together. This is nice with a DIV though without an acessible name it is pointless. With LI as you point out the grouping is already there.

For the slider containser see my anser to 3.

masi commented 1 year ago

Is there a follow-up ticket for these to issues?

My customers love slides with interactive elements and tabbing into out-of-view slides is highly undiserably. Only FF does not support the inert attribute. Using it by default would improve the experience for many users without much extra code. I assume that you have to enable visbility tracking to make it work, but that's ok.

Disabled elements should not only be not focusable, but really disabled. There is an attribute/state for it. If it were used there wouldn't even be the need for a CSS classed to fake a disabled state.

cferdinandi commented 11 months ago

Is there a follow-up ticket for these to issues?

  • Prevent tabbing to focusable items within out-of-view slides
  • Make sure disabled controls/buttons are not focusable

My customers love slides with interactive elements and tabbing into out-of-view slides is highly undiserably. Only FF does not support the inert attribute. Using it by default would improve the experience for many users without much extra code. I assume that you have to enable visbility tracking to make it work, but that's ok.

Disabled elements should not only be not focusable, but really disabled. There is an attribute/state for it. If it were used there wouldn't even be the need for a CSS classed to fake a disabled state.

Seconding this. Focusable elements in out-of-view slides should NOT be able to be focused on, nor should they toggle a slide transition.

That this happens seems to have been deliberate?

lud-hu commented 5 months ago

I found a fairly easy solution for hiding the non-visible slides for screen readers:

new Swiper(ele, {
      // This option adds the `swiper-slide-visible` class to the slides once they are visible
      watchSlidesProgress: true,
});
.swiper-slide {
  visibility: visible;
  transition: visibility 1s linear;

  &:not(.swiper-slide-visible) {
    // Hide non-visible slide from screen reader
    visibility: hidden;
    // Add transition to avoid flickering
    transition: visibility 1s linear;
  }
}