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

feat: add rule aria-conditional-attr #4094

Merged
merged 4 commits into from
Jul 31, 2023
Merged

Conversation

WilcoFiers
Copy link
Contributor

Closes: #4068

@WilcoFiers WilcoFiers requested a review from a team as a code owner July 18, 2023 10:46
Comment on lines 7 to 8
"description": "Ensures ARIA attributes are used as described for the element's role",
"help": "ARIA attributes must be used as described by the element's role"
Copy link
Contributor

@straker straker Jul 24, 2023

Choose a reason for hiding this comment

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

I'm not sure this sentence helps clarify how the rule applies or why. Especially for aria-checked which isn't based on the role but on the checked state of the element, a condition which isn't stated in the aria-checked spec. Maybe something like the following?

Ensures AIRA attributes are used correctly in certain scenarios

It isn't more specific, but does clarify that it's conditional to some set of scenarios where it's allowed to be used but with caveats.

Copy link
Contributor Author

@WilcoFiers WilcoFiers Jul 28, 2023

Choose a reason for hiding this comment

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

"certain scenarios" feels very vague. This checks that if the ARIA says the role has certain restrictions on when/how an attribute can be used, those restrictions are followed. How about:

    "description": "Ensures ARIA attributes are used as described in the specification of the element's role",
    "help": "ARIA attributes must be used as specified for the element's role"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm getting tripped up on as described in the specification of the element's role since what we are doing to validate aria-checked is not defined in any specification (instead the spec calls out to not use aria-checked on an input type="checkbox"). But I don't want to keep bikesheding this, so going to approve and you can feel free to merge or update the message.

straker
straker previously approved these changes Jul 28, 2023
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

See comment above about merging

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.

Put aria-conditional-attr (from aria-allowed-attr) into its own rule
2 participants