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

Remove positive tabIndex #182

Merged
merged 2 commits into from
Feb 13, 2022
Merged

Remove positive tabIndex #182

merged 2 commits into from
Feb 13, 2022

Conversation

crshmk
Copy link
Contributor

@crshmk crshmk commented Nov 30, 2021

A positive tabIndex has become a significant accessibility flag. The axe devtools bot flags any tabIndex={1} on your page as a serious error. Even though this element with a tabIndex of 1 isn't really part of the direct user experience, it's best to get rid of any positive indices on your page.

I found that when setting noFocusGuards, things worked fine when developing your storybook examples, but on my own setup this popped the focus out of the lock.

As I understand the issue, the tabIndex value of 1 here handles cases when devs have other elements on their pages with a positive index.

It seems to me there would be two options:

  • a backwards compatible prop (e.g. maxTabIndex={0}) that would override the current positive index
  • replacing the current positive index to 0 and allowing users with other positive indices on their sites to set a prop that would override it to 1

I believe the latter is a cleaner api and allows for default accessibility compliance, but the former offers backwards compatibility for devs who are using positive indices on their pages.

This PR has two commits for each option, respectively.

@crshmk
Copy link
Contributor Author

crshmk commented Nov 30, 2021

Do you mind rerunning the build? Looks like a pipeline error.

@crshmk crshmk mentioned this pull request Nov 30, 2021
@theKashey theKashey self-requested a review December 12, 2021 03:15
@theKashey theKashey self-assigned this Dec 12, 2021
@TrevorRice
Copy link

Hi all!

Is there anything I can do to assist with this improvement?

@theKashey theKashey merged commit 2e461f7 into theKashey:master Feb 13, 2022
@theKashey
Copy link
Owner

usePositiveIndices(command/hook) -> hasPositiveIndices(situation specification)

The rest is unchanged. Thank you for your contribution.

nickspaargaren pushed a commit to nickspaargaren/react-focus-lock that referenced this pull request Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants