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.

Sign up for GitHub

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

Update role-supports-aria-props.md: fix code sample #939

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Update role-supports-aria-props.md: fix code sample #939

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/rules/role-supports-aria-props.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ This rule takes no arguments.
### Succeed
```jsx
<!-- Good: the radiogroup role does support the aria-required property -->
<ul role="radiogroup" aria-required aria-labelledby="foo">
<ul role="radiogroup" aria-required="true" aria-labelledby="foo">
Copy link
Member

Choose a reason for hiding this comment

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

what needs fixing here exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the aria-required attribute must have a "true" value to work properly with assistive technology. Without the value, the attribute does not work to inform screenreaders of the "required" state of the group.

Without ="true" in devtools :
image

With aria-required="true" :
image

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. It seems like we don't have any rule or option that can catch this currently - @jessebeach, does this seem like a good addition? Perhaps an option to aria-props or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ljharb and @n-chardon, honestly, it strikes me as something that requires its own rule, something like no-empty-boolean-attributes. The attributes that such a rule would apply to are:

aria-atomic
aria-busy
aria-disabled
aria-modal
aria-multiline
aria-multi selectable
aria-readonly
aria-required

And, confusingly, there is a tri-state version (true/false/undefined) of these "boolean" attributes that allow "undefined", which indicates that the attribute is not applicable. Such a state shouldn't exist, but it exists in the spec, so it needs to be accounted for here:

aria-expanded
aria-grabbed
aria-hidden
aria-selected

A good solution is pulling the properties map from https://github.com/A11yance/aria-query/blob/main/src/ariaPropsMap.js and refactoring it into a util. A better solution is referring to aria-query to get the information about the allowed props and their value types rather than having this information hard-coded in this lint rule plugin.

@n-chardon would you like to write up this rule?

<li tabIndex="-1" role="radio" aria-checked="false">Rainbow Trout</li>
<li tabIndex="-1" role="radio" aria-checked="false">Brook Trout</li>
<li tabIndex="0" role="radio" aria-checked="true">Lake Trout</li>
Expand Down