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

Tiny Slider #481

Closed
wants to merge 10 commits into from
Closed

Tiny Slider #481

wants to merge 10 commits into from

Conversation

coreymcollins
Copy link
Contributor

@coreymcollins coreymcollins commented Jun 21, 2019

Doesn't necessarily close, but relates to #460

DESCRIPTION

Replaces Slick Carousel with Tiny Slider.

In an effort to provide a more accessible and better maintained carousel option, I'm looking into a few solutions noted here: #478 (comment)

Slick Carousel hasn't been updated in a year or so and has 900+ issues and nearly 200 pull requests that are unresolved. I think it's time to start exploring other options to replace Slick with something that is actively being maintained.

The first test is with Tiny Slider: https://github.com/ganlanyuan/tiny-slider

There are some open issues and PRs on the repo, but it is also maintained more regularly with a decent focus on accessibility in the carousel itself.

In testing, I was able to quickly spin up a carousel using our Carousel ACF block. I messed with the options a bit to test the various options seen here: http://ganlanyuan.github.io/tiny-slider/demo/

Happy to report that I was able to easily create a bunch of different layout options.

I'm able to tab through the carousel which is a bonus, of course. Tiny Slider adds a stop/start button by default so users are always in control of whether or not the carousel is playing. Our play/pause button on background videos is also still present and works fine.

This carousel does suffer from a similar issue to what we've seen with others where, when tabbing through a slide with links, the carousel gets stuck halfway:

I'll look into solutions here. We may be able to adjust the tabindex of slides depending on the existence of the link button. Something to test for sure.

It steps through the elements well with a screen reader as well.

All in all, it seems like a nice solution with a lot of variety. Going to leave this marked in progress as I test this and others as a Slick replacement.

SCREENSHOTS

GIF link if the above doesn't embed: https://www.dropbox.com/s/9k1dwbfxo5wxofg/wd_s-tiny-slider.gif?dl=0

OTHER

STEPS TO VERIFY

  1. Checkout the branch
  2. Add a Carousel block to your page
  3. Test!

@coreymcollins
Copy link
Contributor Author

coreymcollins commented Jul 22, 2019

I'd still like some others to test and see if they have any ideas for solutions for the tabbing issue, but I'm leaning toward this not being a viable solution if we can't get around the tabbing. It effectively breaks the layout of the carousel and hampers accessibility and usage at worst and, at best, is annoying.

It's frustrating because a lot of the other features of the carousel are nice and it's easy to use, but the tab breaking is a real pain.

@coreymcollins
Copy link
Contributor Author

I might actually be onto a solution for this.

The idea is:

  1. Set the play/pause button for video backgrounds to tabindex=-1
  2. Set the anchor tag for the optional link to tabindex=-1
  3. Listen to slide changes
  4. On change, reset the tabindex for the active slide's buttons/links
  5. Also on change, set the inactive slides/previously active slide's button/link tabindex to -1.

In testing with everything staying at tabindex=-1, I'm able to tab through the carousel and reach the various navigation elements without the carousel breaking.

@coreymcollins
Copy link
Contributor Author

coreymcollins commented Jul 23, 2019

The latest commits (2cb8f08, 4789ebb ) add support for adjusting tabindex on slide change.

How it works:

  1. On load, all slides have their anchor tags and buttons set to tabindex=-1
  2. Also on load, any anchor tags and buttons inside of the initial slide are set to tabindex=0 so they can be naturally tabbed to
  3. On slide change, all other slides have their anchor tags and buttons set to tabindex=-1
  4. Also on slide change, the new current slide has its anchor tags and buttons set to tabindex=0 so they are tab-able

In testing on my local, I have found this to work without tabbing me through the entire carousel or breaking the layout:

@coreymcollins
Copy link
Contributor Author

Ran through some screen reader testing today. Slide changes are communicated to the user, and everything is read as expected. The screen reader doesn't get stuck running through every slide – it reads the active slide, then moves onto any remaining carousel elements and onward down the page.

So far I think I'm pretty happy with how Tiny Slider is implemented.

@coreymcollins
Copy link
Contributor Author

I also ran a page with multiple carousels through Koa11y and came back with 0 errors. There were a handful of warnings for WCAG AA 2.0 around some absolutely positioned elements and images coming back as ignored by assistive technology when used as background images, which I think may be fine? As a background image, they're decorative and don't provide any additional information than what is communicated in the slide contents.

@coreymcollins
Copy link
Contributor Author

coreymcollins commented Jul 26, 2019

Marking a few of y'all for review, if/when you have a moment. You all offer unique sets of eyes to hammer away on this stuff, so I'd love to see each of you kick the wheels here a bit if possible.

Thanks!

Copy link
Contributor

@ImBigWill ImBigWill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great overall, I agree that it might be time to move on and this has a lot of great improvements.

I had a weird issue at first, but I of course can't recreate it now and may have been my own fault with the images where one image appeared to be on top of the one below. Tested this with WP 5.2.2 and ACF 5.8.2.

@coreymcollins
Copy link
Contributor Author

I think one of the main goals of today will be to merge in #480, and this PR currently uses the "old style" of ACF blocks built out as meta rather than Gutenberg blocks.

Once #480 is merged in, I'm going to retool this PR to use the updated ACF + Goots block setup.

@Cheffheid
Copy link
Contributor

Looking pretty good so far!

I've messed around on these carousels with a screen reader enabled I have a few comments on this thing.

  • It seems to announce the slide it's on every time the carousel changes automatically. This is just very friggin' annoying and exponentially so if there is more than one carousel on the page. It's just very likely that it will interfere with other things on the page you're trying to "read" at the time. If those announcements can be disabled then that would be awesome (disabling autoplay altogether would also work for me).
  • The way it handles tabindexes on the navigation is a little strange to me. And I've left my comments below in-tact as I discovered things just to try and show the discovery process lol.
    • In the pagination nav, only the button that corresponds with the active slide can be tabbed to - which I assume is because the intent is to convey what slide is currently active. But it's a button that really does nothing (since the slide is already active) and there is probably a better way to convey that information. Furthermore, by setting the tabindex to -1 on the other buttons in that pagination you make it completely useless as a navigation tool when a keyboard is used.
    • The prev/next buttons are both set to a tabindex of -1, and instead the wrapper div has a tabindex of 0. What this seems to do is that it will announce both of the buttons (as "clickable prev button next"), and hitting enter will activate the "prev" button. But only when a screen reader is active, when it's not it will not do anything. Presumably because screen readers tend to "translate" the Enter key to click events.
    • When using just a keyboard, I can now see that it seems that the intent is to use the arrow keys to navigate both the prev/next button and the pagination buttons when you're on them. However, this does not work when a screen reader is enabled (or NVDA anyway, but I presume JAWS will react in a similar manner). Instead, using the arrow keys there will make it go into the button and allow you to spell out the button text. The only alleviating factor here is that a screen reader will allow you to navigate through all of the buttons on the page, regardless of their tabindex. I'm just not 100% confident in making a statement about how likely it is for people to resort to these alternative navigation methods when tabbing clearly isn't working out.

Other than that, though, I like how it "hides" buttons on inactive slides and doesn't even try to go to them, so in that sense it's already doing better than most carousels. 👍

@Cheffheid Cheffheid mentioned this pull request Aug 30, 2019
@Cheffheid
Copy link
Contributor

Also, as noted in #453, Slick is the last remaining bastion that's holding us to Bower, it seems. So maybe use npm to install its replacement so we can finally send Bower away? :)

@coreymcollins
Copy link
Contributor Author

coreymcollins commented Aug 30, 2019

Thanks for running this through some testing! I've got another branch getting ready for a fresh PR now that #480 has been merged in, as I mentioned in Slack, but I'll update here until that's ready.

100% on disabling auto play. I have that turned off in the new branch I'm working in.

In the pagination nav, only the button that corresponds with the active slide can be tabbed to - which I assume is because the intent is to convey what slide is currently active. But it's a button that really does nothing (since the slide is already active) and there is probably a better way to convey that information. Furthermore, by setting the tabindex to -1 on the other buttons in that pagination you make it completely useless as a navigation tool when a keyboard is used.

Kind of similar to the prev/next pagination, you can tab one element and then use the arrow keys to navigate around. This may not be the expected way to do this, as your testing is revealing.

In the gif below, I:

  • Tab to the pagination, which selects the current slide
  • Use the arrow keys to move from button to button
  • Hit the spacebar to execute, which changes the slide

The prev/next buttons are both set to a tabindex of -1, and instead the wrapper div has a tabindex of 0. What this seems to do is that it will announce both of the buttons (as "clickable prev button next"), and hitting enter will activate the "prev" button. But only when a screen reader is active, when it's not it will not do anything. Presumably because screen readers tend to "translate" the Enter key to click events.

If I'm reading and testing this properly, I think this also applies to your next comment:

When using just a keyboard, I can now see that it seems that the intent is to use the arrow keys to navigate both the prev/next button and the pagination buttons when you're on them. However, this does not work when a screen reader is enabled (or NVDA anyway, but I presume JAWS will react in a similar manner). Instead, using the arrow keys there will make it go into the button and allow you to spell out the button text. The only alleviating factor here is that a screen reader will allow you to navigate through all of the buttons on the page, regardless of their tabindex. I'm just not 100% confident in making a statement about how likely it is for people to resort to these alternative navigation methods when tabbing clearly isn't working out.

So, the prev/next thing was confusing for me at first. I expected to be able to tab to each button individually and advance slides one way or the other. Tiny Slider has an opinion about this, which is:

  • You tab to the navigation container
  • The entire section is active/highlighted
  • You use left/right to page back and forth

When I tab to the buttons, hitting enter does nothing for me. The only thing that moves back and forth is left/right when the navigation is active.

In using VoiceOver on a Mac, the prev/next buttons don't actually get read when you tab to them. They're read when the page is being read through, but when you tab onto them it just tells you that you're on a "navigation group" which isn't all that helpful. I don't think I noticed that difference previously.

I'm wondering if it's worth the effort to try and work around the weirdness it imposes or if it's better to keep looking at options. I like it for its simplicity and wealth of options, but the a11y issues you're noting don't look all that great.

Oh, and by default it does the same thing all of the carousels seem to do which is it tries to read every link in every slide, which winds up breaking the carousel visually. I was able to workaround this with the app.setInitialLinkAttributes and app.setLinkStatesOnChange functions, which basically "hides" inactive slides from a screen reader so you don't get lost in a maze of tabs or confused by a bunch of slides being messily read. I'm sure we could port that same functionality over to another carousel if we land on a better solution.

@gregrickaby gregrickaby deleted the branch master October 16, 2020 20:35
@gregrickaby gregrickaby deleted the feature/carousel-tiny-slider branch October 16, 2020 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants