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

Allow non-string values as context. #17

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

garg3133
Copy link
Contributor

This PR removes the const el = document.querySelector(selector); statement for the following reasons:

  • axe-core's context argument is really powerful and supports the input in a lot of varied formats, but having the above statement limits the first argument of .axeRun() command to only accept string input.
  • We don't need to resolve the DOM nodes from the selector ourselves as that can be handled by Axe-core itself.
  • Having document.querySelector() limits the axe check to only one matching DOM node and that's it. So, if we want to check some elements for accessibility that have the same class name, it will always choose the first matching node only and the other matching nodes will remain untested. Also, to check for multiple elements with unique CSS selectors, we would need to run .axeRun() command multiple times as we cannot pass all the selectors once (which is supported by axe-core by separating multiple CSS selectors by ,).

Since I am unaware, is there a reason why the above-mentioned statement was included in the code in the first place?

Copy link
Owner

@reallymello reallymello left a comment

Choose a reason for hiding this comment

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

Tests still passing omitting the query selector which adds better functionality as you mentioned. Thanks.

@reallymello reallymello merged commit d19e5a1 into reallymello:master Jan 16, 2024
3 checks passed
@garg3133 garg3133 deleted the fix-context branch January 16, 2024 17:31
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.

None yet

2 participants