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

Improve Accessibility Features #3149

Closed
12 of 16 tasks
fregiese opened this issue Jul 15, 2019 · 32 comments
Closed
12 of 16 tasks

Improve Accessibility Features #3149

fregiese opened this issue Jul 15, 2019 · 32 comments

Comments

@fregiese
Copy link

fregiese commented Jul 15, 2019

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • Swiper Version: v4.5.0

  • Platform/Target and Browser Versions: EVERY PLATFORM

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:

  • <section> as the outer container, instead of an <div> element
  • outer container has aria-roledescription="carousel"
  • outer container has aria-label="Example Content" (Label can be set like the next/prev slider buttons text)
  • next and previous buttons are <a>-tags
  • next and previous buttons haverole="button"
  • next and previous buttons has aria-controls="swiperID" (this connects with the id of the content)
  • content container has id="swiperID" (this connects with the aria-attribute of the next and previous buttons)
  • content container has aria-live="off" when autoplay is on and aria-live="polite" when autoplay is off
  • content item has role="group"
  • content item has aria-roledescription="slide"
  • content item has the slide number as aria-label. Example:aria-label="1 of 6"
  • Prevent tabbing to focusable items within out-of-view slides
  • Make sure disabled controls/buttons are not focusable

Related Issues

The w3c example doesn't use clones, but this issue is also important to improve accessibility: #2929
Maybe the improved keyboard accessiblity would also fix issues like this: #2945

@philwolstenholme
Copy link

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

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

ghost commented Apr 11, 2020

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

@masi
Copy link

masi commented Apr 28, 2020

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

masi commented May 25, 2020

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

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

@masi
Copy link

masi commented Jun 28, 2020

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

xcarlos commented Jul 9, 2020

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

xcarlos commented Jul 10, 2020

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

AAZANOVICH commented Oct 23, 2020

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

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

AAZANOVICH commented Oct 28, 2020

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

jenemde commented Nov 5, 2020

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)

  • <section> as the outer container, instead of an <div> element
    --> changed in playground/index.html
  • outer container has aria-roledescription="carousel"
    --> aria-roledescription for outer container can be set by a11y option
  • outer container has aria-label="Example Content" (Label can be set like the next/prev slider buttons text)
    --> aria-label for outer container can be set by a11y option
  • next and previous buttons are <a>-tags
    --> next and previous buttons are <button>-tags in playground/index.html
  • next and previous buttons have role="button"
    --> next and previous buttons get role="button" if no <button>-tags are used
  • next and previous buttons has aria-controls="swiperID" (this connects with the id of the content)
    --> next and previous buttons has aria-controls="[id given for swiper]", see next point
  • content container has id="swiperID" (this connects with the aria-attribute of the next and previous buttons)
    --> content container gets random id if it hasn't an id yet. id corresponds to the aria-attribute of the next and previous buttons
  • content container has aria-live="off" when autoplay is on and aria-live="polite" when autoplay is off
    --> done
  • content item has role="group"
    --> role="group" for content item is set
  • content item has aria-roledescription="slide"
    --> aria-roledescription for content item can be set by a11y option
  • content item has the slide number as aria-label. Example:aria-label="1 of 6"
    --> done (to avoid language problems: 1/6, 2/6 ...)

@philwolstenholme
Copy link

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

@vltansky
Copy link
Collaborator

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

@philwolstenholme
Copy link

I think there are a few extra tasks:

@fregiese
Copy link
Author

I updated the inital comment to show the current status

@soniktrooth
Copy link

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

@jenemde
Copy link
Contributor

jenemde commented May 6, 2021

Thanks. This has been commented here:
#4371
and fixed here:
1b73c3b
in 6.5.1.

@yordis
Copy link

yordis commented May 11, 2021

Be aware of akiran/react-slick#1535

@rafael-leal-mccormack
Copy link

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?

@vltansky vltansky added this to the v7 milestone Jul 8, 2021
@nolimits4web nolimits4web added this to To do in v7 Jul 13, 2021
@yuheiy
Copy link
Contributor

yuheiy commented Aug 28, 2021

I send related PR: #4884

@michaelbourne
Copy link

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

masi commented Sep 23, 2021

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

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

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

@shiivan
Copy link

shiivan commented Oct 13, 2021

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.

wrapperTag: WrapperTag = 'div',

@Gr0m
Copy link

Gr0m commented Feb 18, 2022

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

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

sneko commented Jan 20, 2023

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

masi commented Feb 17, 2023

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

masi commented Feb 17, 2023

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.

@cferdinandi
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v7
To do
Development

No branches or pull requests