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

Implement acessibility features #4

Closed
DouglasdeMoura opened this issue May 4, 2016 · 23 comments

Comments

@DouglasdeMoura
Copy link

commented May 4, 2016

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented May 4, 2016

Will check this, thanks

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented May 4, 2016

Tiny-slider is still in progress right now, please give more ideas if possible.
Thanks

@epigeyre

This comment has been minimized.

Copy link

commented May 10, 2016

This series of articles offer a nice sum up of accessibility best practices for carousels:

I thought this might help make your script even better!

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented May 10, 2016

Great, thanks
On Tue, May 10, 2016 at 6:11 AM Etienne Pigeyre notifications@github.com
wrote:

This series of articles offer a nice sum up of accessibility best
practices for carousels:

I thought this might help your script even better!


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#4 (comment)

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented Jun 7, 2016

Hey,
Accessibility features added. Could you check?
Only for touch screen I couldn't figure out how to support. Any idea?

@epigeyre

This comment has been minimized.

Copy link

commented Jun 8, 2016

Thanks, I will check as soon as I can and get back to you!

@epigeyre

This comment has been minimized.

Copy link

commented May 17, 2017

Hello,

Sorry it's been so long! So I had time to check and here is my feeback:

  • aria-hidden -> OK.
  • aria-controls -> On the carousel pagination, if the number of items is set to more than one, the aria-controls value doesn't reflect this setting (it's selecting the next one instead of the n + 2 if we set items value to 2).
  • aria-selected -> OK.
  • Content order -> From an accessibility stand point, it makes more sense to have navigation and pagination above carousel.
  • Limiting tab selection -> tabindex="-1" should be added to every inactive slide.
  • Navigation keyboard control -> Add tabindex="0" to the Carousel navigation container and tabindex="-1" on both carousel navigation buttons.
  • aria-live -> In my opinion it depends too much on the purpose of the carousel to be implemented in a global solution so don't bother.

For touch screens I'm sorry but my Javascript skills are very limited, I can't help you...

Hope this helps!

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented May 20, 2017

Thanks @epigeyre for your feedback.
I will check these when I have time.

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented May 26, 2017

  • Content order -> From an accessibility stand point, it makes more sense to have navigation and pagination above carousel.

Will update in next main version, since this will cause a DOM change.

  • Limiting tab selection -> tabindex="-1" should be added to every inactive slide.
  • Navigation keyboard control -> Add tabindex="0" to the Carousel navigation container and tabindex="-1" on both carousel navigation buttons.

Done

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented May 26, 2017

  • aria-controls -> On the carousel pagination, if the number of items is set to more than one, the aria-controls value doesn't reflect this setting (it's selecting the next one instead of the n + 2 if we set items value to 2).

Will work on this maybe this weekend.
Done

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented Jul 31, 2017

  • Content order -> From an accessibility stand point, it makes more sense to have navigation and pagination above carousel.

This will be done from v2.0

@ganlanyuan ganlanyuan closed this Jul 31, 2017

@Eric-Draven

This comment has been minimized.

Copy link

commented Apr 17, 2018

How to place navigation and pagination under carousel?

@epigeyre

This comment has been minimized.

Copy link

commented Apr 19, 2018

@Eric-Draven The best way is probably to use position: relative on your slider's wrapper and position: absolute on your navigation and pagination elements so that you can place them wherever you want.

@Eric-Draven

This comment has been minimized.

Copy link

commented Apr 23, 2018

Thank you, I know this, but it would be nice to be able to change through the options.

@OD-fraja

This comment has been minimized.

Copy link

commented May 31, 2018

@ganlanyuan @epigeyre Regarding tabindex="0" on the controls container what was the reason behind it? Is that really needed? We use a company to audit our site and they logged this as an issue that tabindex="0" causes and extra tab which is not needed.

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented May 31, 2018

@OD-fraja So that it can be focused (instead of prev, next button been focused) and user can control the slider using arrow keys.

@OD-fraja

This comment has been minimized.

Copy link

commented May 31, 2018

@ganlanyuan Aha I see, makes sense. I did not know that you could use arrow keys to control the slider. That makes sense now. Thanks for your prompt reply.

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented Jun 1, 2018

@OD-fraja You're welcome

@missmatsuko

This comment has been minimized.

Copy link

commented Jul 12, 2018

Limiting tab selection -> tabindex="-1" should be added to every inactive slide.
Navigation keyboard control -> Add tabindex="0" to the Carousel navigation container and tabindex="-1" on both carousel navigation buttons.

Done

I'm just checking out the sliders on the demo page. I can see that the tabindex is set as described above, but I still can't seem to tab past a carousel without going through each slide. Is some JS that is programmatically focusing the inactive slides on tab?

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented Jul 14, 2018

@missmatsuko Hey, it's because of the <a> tag in each slide.

@missmatsuko

This comment has been minimized.

Copy link

commented Jul 15, 2018

@ganlanyuan Hmm, I see. I think the content inside the inactive slides really shouldn't be focusable, even if they have links in them.

@ganlanyuan

This comment has been minimized.

Copy link
Owner

commented Jul 16, 2018

You're right.
But it's too much for the slider to check all the possible focusable child elements of inactive slides and make them non-focusable.
I will search for ways which just need to work on the slides themselves.

@nickblass

This comment has been minimized.

Copy link

commented Sep 7, 2018

The problem we're having with this, is that we are lazy loading the images so if we tab through and the images haven't loaded (which a user may do), then it shows an invalid image which is not ideal. I have went ahead and targeted our links and disabled the tab index just so this does not occur. However, I think it would provide a better UX if the tab index was only enabled for elements within the visible slide?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.