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

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

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

Conversation

n-chardon
Copy link
Contributor

fixes #938

@@ -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?

@sangikhan29
Copy link

@ljharb, do you know why the tests here are failing for this simple update to a md file? I was working on a draft and found similar tests failing. I ran npm run jest on the master branch itself locally and found the same thing.

@ljharb
Copy link
Member

ljharb commented Jun 21, 2023

Yes, see #937; you can ignore it for now.

@ljharb ljharb marked this pull request as draft August 11, 2023 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

role-supports-aria-props documentation page displays a non working sample of code
4 participants