Slick accessibility updates #5151
Conversation
bpmcneilly
added
the
Theme: Accessibility
|
|
||
| function accessibilitySetup() { | ||
|
|
||
| function setA11yAttributes() { |
There was a problem hiding this comment.
Why does this need to be embedded within the function? It doesnt' seem to use the closure...
| @@ -178,6 +180,32 @@ const Carousel = { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| function accessibilitySetup() { | |||
There was a problem hiding this comment.
Please add documentation for the new method explaining its purpose
|
|
||
| function setA11yAttributes() { | ||
| //Ensuring offscreen elements do not receive focus | ||
| $(`${selector} [aria-hidden="true"] a[href]`).attr('tabindex', -1); |
There was a problem hiding this comment.
Cache the result of the selector query before applying other queries to save DOM lookups.
| $(`${selector} button.slick-next`).on('click', function () { | ||
| $(`${selector} button.slick-prev`).focus(); | ||
| }); | ||
| $(selector).on('afterChange', setA11yAttributes); |
There was a problem hiding this comment.
Please add an inline comment explaining the afterChange event, preferably pointing to some documentation.
There was a problem hiding this comment.
Wouldn't this bind multiple click events to the next and previous buttons?
| }); | ||
| $(selector).on('afterChange', setA11yAttributes); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
This new method MUST have a corresponding unit test.
|
Please reassign me when you are ready for another review. Thanks! |
|
See also https://accessible360.github.io/accessible-slick/ which is a fork of the slick library with various accessability enhancements baked in and the authors have good info on real world accessibility of carousels in their github repos. |
|
@bpmcneilly do you have any interest to continue working on this now? I assume not since it has been a while. I'd like to offer this issue up for someone to fork your branch and wrap it up. Thanks! |
|
@RayBB yes please! |
This PR provides an approach to increase the accessibility of our slick slider carousels.
The existing implementation of the slick "accessibility" features is definitely lacking. While, in the long run, we may need to switch away from slick, this PR would ensure that we have some of the most needed accessibility in place while we look for other slider options in the future.
Technical
The biggest issue with the "accessibility" features implemented by Slick are that they do not move keyboard focus appropriately for keyboard-only users, and their existing role of
listboxisn't particularly valid for the type of control displayed. An ARIA listbox role is intended to be something similar to an HTMLselectelement.It's impossible to leave the "accessibility" features on & simply write over the existing content because whenever a user updates the slider the library will reimpose its default roles. So, for this PR I simply opted to turn off all the accessibility features & add on from there.
Doing this requires manually adding back in a few things we got "for free" previously, but the only thing of note is setting
tabindex="-1"to the off-screen interactive elements in a slider to prevent them from receiving focus.In terms of new features, this PR adds in:
aria-disabled="true"from being in the tab orderlist&listitemfor the carousel & its contents, along with a programmatic labelThe biggest downside of this PR is that there is additional JS we will need to maintain & we have to manually set up a lot of the elements here. As an example, because the
listrole is being added with the main slider & thelistitemroles get added locations, it's easy to miss content & create a list with no list items. I did this accidentally when I forgot to addrole="listitem"to the categories slider.Testing
aria-hidden="true"roleattribute with a value oflist& that each carousel element has aroleattribute with a value oflistitemScreenshot
Here's a sample animation of the updated focus movement when navigating via the keyboard:
Stakeholders
@mekarpeles