Skip to content

feat: add rule aria-conditional-attr#4094

Merged
WilcoFiers merged 4 commits intodevelopfrom
aria-conditional-attr-rule
Jul 31, 2023
Merged

feat: add rule aria-conditional-attr#4094
WilcoFiers merged 4 commits intodevelopfrom
aria-conditional-attr-rule

Conversation

@WilcoFiers
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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