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

Fix Violations of and Reenable jsx-a11y/no-noninteractive-tabindex #56384

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Feb 7, 2024

Specifically, for all but one violation I fixed it by removing the nonfunctional tabindex attribute from the non-interactive element. In that last case, the element had a click handler so I added a role attribute to make it explicitly interactive.

Testing story

I have not manually tested these changes; I checked few of them and confirmed this change is functionally a no-op, but wasn't sure how to find all of them for testing. In particular, the one element for which I added a role rather than removing a tabindex remains untested.

I feel reasonably confident this change is safe despite the lack of testing, but am very open to disagreement. Let me know what you think!

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Specifically, for all but one violation I fixed it by removing the nonfunctional `tabindex` attribute from the non-interactive element. In that last case, the element had a click handler so I added a `role` attribute to make it explicitly interactive.
@Hamms
Copy link
Contributor Author

Hamms commented Feb 7, 2024

This PR is a continuation of #55558, which was mis-closed by the Git LFS migration (#55759).

Previous Comments:

Previous Reviews:

@Hamms Hamms requested review from bencodeorg, bethanyaconnor and a team February 7, 2024 01:36
@Hamms Hamms marked this pull request as ready for review February 7, 2024 01:37
Copy link
Contributor

@bencodeorg bencodeorg left a comment

Choose a reason for hiding this comment

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

LGTM! I looked at the DanceAiModalHeader component and confirmed no regression from this change (ie, can still tab nav to that component).

@Hamms
Copy link
Contributor Author

Hamms commented Feb 8, 2024

Great, thank you!

@Hamms Hamms merged commit 865d79e into staging Feb 13, 2024
2 checks passed
@Hamms Hamms deleted the fix-jsx-a11y/no-noninteractive-tabindex branch February 13, 2024 23:15
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.

None yet

2 participants