Skip to content

Conversation

@pimdewit
Copy link
Collaborator

#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

@pimdewit pimdewit requested a review from ciampo September 17, 2019 11:18
@pimdewit pimdewit self-assigned this Sep 17, 2019
@coveralls
Copy link

coveralls commented Sep 17, 2019

Pull Request Test Coverage Report for Build 65

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 27 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.8%) to 90.114%

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

💛 - Coveralls

Copy link
Owner

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Using the inert property instead of the attribute seems like a good choice.

Just have a look at the comment I made, and then we can merge the PR

slideEl.setAttribute(ATTRS.STANDARD.TABINDEX, -1);
} else {
slideEl.setAttribute(ATTRS.STANDARD.INERT, '');
slideEl.setAttribute(ATTRS.STANDARD.TABINDEX, '-1');
Copy link
Owner

Choose a reason for hiding this comment

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

Judging at the comment just before the if, it looks like I left a bug in those lines.
I think, if isSlideInView is true, the tabindex attribute should be removed (instead of being set to -1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think its currently working as intended, we want to give tabindex -1 so we can programmatically focus the slide right?

https://mzl.la/2LY933V
Screenshot 2019-10-08 at 18 01 48

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wicg-inert seems to do some magic with tabindex too.. 😕

@pimdewit pimdewit force-pushed the hotfix/aria-hidden branch from 95983ec to 1fea50d Compare October 8, 2019 17:43
@pimdewit
Copy link
Collaborator Author

pimdewit commented Oct 8, 2019

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?

@pimdewit pimdewit requested a review from ciampo October 8, 2019 17:43
@ciampo
Copy link
Owner

ciampo commented Oct 9, 2019

What happens now though is:

  • setting the inert property to true:
    • adds the inert and aria-hidden="true" attributes
    • removes the tabindex attribute
  • setting the inert property to false:
    • removes the inert and aria-hidden attributes
    • adds the tabindex="-1" attribute

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?

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.

3 participants