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: consistently parse tabindex, following HTML 5 spec #4637

Merged

Conversation

ahmedhalac
Copy link
Contributor

@ahmedhalac ahmedhalac commented Nov 11, 2024

Ensures tabindex is correctly parsed as an integer using regex instead of parseInt to comply with HTML standards.

Closes #4632

Ensures tabindex is correctly parsed as an integer using regex instead of parseInt to comply with HTML standards.

Related to dequelabs#4632
@ahmedhalac ahmedhalac requested a review from a team as a code owner November 11, 2024 20:59
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

This is a good start, but I think we can do a little better. Going over the code base, it looks like axe has an awful lot of places where it's parsing tabindex. I think it would be better if we put the code for this in one place. Normally we'd do this in a commons function, but because one of the places is in core, we'll need to do this in core/utils. So how about this:

In lib/core/utils/ create a file parse-tabindex.js with a parseTabindex function that accepts either a string or null. And which returns an integer if the string is a valid tabindex value, or null if not valid or if the value is null. This would need tests in a new test/core/utils/parse-tabindex.js.

This function could then be used in the following files, instead of the current ad-hoc methods:

  • lib/checks/keyboard/focusable-no-name-evaluate.js
  • lib/checks/keyboard/no-focusable-content-evaluate.js
  • lib/checks/keyboard/tabindex-evaluate.js
  • lib/commons/dom/get-tabbable-elements.js
  • lib/commons/dom/inserted-into-focus-order.js
  • lib/commons/dom/is-focusable.js
  • lib/commons/dom/is-in-tab-order.js
  • lib/core/base/context/create-frame-context.js
  • lib/rules/autocomplete-matches.js
  • lib/rules/no-negative-tabindex-matches.js

Add parseTabindex method in lib/core/utils and reused it in multiple places across the codebase

Related to dequelabs#4632
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

This is looking great! The change required a few additional updates, this is looking really good.

lib/core/utils/parse-tabindex.js Outdated Show resolved Hide resolved
lib/core/utils/parse-tabindex.js Outdated Show resolved Hide resolved
test/core/utils/parse-tabindex.js Outdated Show resolved Hide resolved
test/core/utils/parse-tabindex.js Show resolved Hide resolved
lib/rules/no-negative-tabindex-matches.js Outdated Show resolved Hide resolved
lib/commons/dom/get-tabbable-elements.js Outdated Show resolved Hide resolved
lib/checks/keyboard/tabindex-evaluate.js Show resolved Hide resolved
lib/checks/keyboard/no-focusable-content-evaluate.js Outdated Show resolved Hide resolved
lib/checks/keyboard/focusable-no-name-evaluate.js Outdated Show resolved Hide resolved
lib/checks/keyboard/focusable-no-name-evaluate.js Outdated Show resolved Hide resolved
Resolving pull request comments

Related to dequelabs#4637
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

This is looking great. @ahmedhalac thank you very much for the contribution!

For the books: I reviewed this PR for security.

@WilcoFiers WilcoFiers changed the title refactor(#4632): correct tabindex parsing logic fix: consistently parse tabindex, following HTML 5 spec Nov 20, 2024
@WilcoFiers WilcoFiers merged commit 645a850 into dequelabs:develop Nov 25, 2024
22 of 24 checks passed
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.

isFocusable is too strict on tabindex
2 participants