ciampo / macro-carousel

Carousel as a Vanilla Web Component.
https://ciampo.github.io/macro-carousel/demo/
MIT License
77 stars 6 forks source link

hotfix(a11y): fix for aria-hidden not applying to slides properly #46

Open pimdewit opened 5 years ago

pimdewit commented 5 years ago

https://github.com/ciampo/macro-carousel/issues/43 Turns out that inert-polyfill (used by the demos) has a side-effect if you apply it by calling setAttribute('inert', true). It takes a frame before aria-hidden is applied. You can omit this by setting inert the "native" way; element.inert = flag.

This is not a bug on our end, but it looks like this fix would not hurt

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 65


Files with Coverage Reduction New Missed Lines %
dist/macro-carousel-test.js 27 90.11%
<!-- Total: 27 -->
Totals Coverage Status
Change from base Build 62: -0.8%
Covered Lines: 534
Relevant Lines: 573

💛 - Coveralls
pimdewit commented 4 years ago

I've updated the PR - we set the tabindex in _onSlidesSlotChange to -1 to ensure we can programmatically focus the slides. If inert=true, the element will not be focusable regardless of the tabindex value. It will also set the semantic properties under the hood, including aria-hidden (and this is reflected through an attribute when using the wicg-inert polyfill).

So basically I have removed the aria-hidden and tabindex logic, since inert takes care of that. What do you think @ciampo?

ciampo commented 4 years ago

What happens now though is:

This logic conflicts with the slides having tabindex="-1" set in the _onSlidesSlotChange function — we should probably remove this bit of code and handle this accessibility aspect all through the inert property?

Do these changes affect at all the bit where the newly selected slide is programmatically focused?