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

role-suports-aria-props is not flagging prohibited accessible name usage on generic elements #910

Open
khiga8 opened this issue Dec 2, 2022 · 5 comments
Assignees

Comments

@khiga8
Copy link
Contributor

khiga8 commented Dec 2, 2022

I would expect role-suports-aria-props to flag prohibited accessible name usage on generic elements like:

<div aria-label="Some label">
<span aria-label="Some label">
<div aria-labelledby="some-id">
<span aria-labelledby="some-id">

etc.

We see this misuse of accessible name a lot in our projects and we're hoping this is a case we can cover with eslint-plugin-jsx-a11y.

I'd hoped that the latest bump of aria-query in #814 would start flagging these issues, but when I update the tests in tests/src/rules/role-supports-aria-props-test.js on the latest main to include invalid cases like:

    {
      code: '<div aria-label />',
      errors: [errorMessage('aria-label', 'generic', 'div', true)],
    },

it doesn't seem like it's being recognized as invalid.

@jessebeach
Copy link
Collaborator

@khiga8 hello Kate! Directionally, you are on to something here. We don't have a rule that prohibits labels on elements or in combination with roles that do not support a label. I wouldn't do that in this rule, because it is specifically concerned with the combination of ARIA Role and ARIA Prop. In your examples, none of the HTML elements have a role. It's true that the <div> element corresponds to the generic role, so a label should be disallowed in this case. But what I think we need here is a new rule altogether.

@jessebeach
Copy link
Collaborator

@khiga8 would you like to update this issue to reflect that you're asking for a new rule?

@khiga8
Copy link
Contributor Author

khiga8 commented Dec 2, 2022

Hi @jessebeach! I'd noticed in the role-supports-aria-props-test, there are checks for implicit roles and the doc describes the rule as the following:

Enforce that elements with explicit or implicit roles defined contain only aria-* properties supported by that role.

I'm not sure I understand quite why this use case falls outside of this rule. Would you mind elaborating 🙏?

@jessebeach
Copy link
Collaborator

Hi @jessebeach! I'd noticed in the role-supports-aria-props-test, there are checks for implicit roles and the doc describes the rule as the following:

Enforce that elements with explicit or implicit roles defined contain only aria-* properties supported by that role.

I'm not sure I understand quite why this use case falls outside of this rule. Would you mind elaborating 🙏?

You're right! Alright, this is the right place for this sort of restriction and I agree with you now that this rule isn't catching all cases. aria-label is marked as prohibited for the generic role: https://github.com/A11yance/aria-query/blob/main/src/etc/roles/literal/genericRole.js#LL12

The best way to approach this is for me to update aria-query with the allowed and prohibited aria props information, then reference that in this rule. I'll put it on the to-do list!

Thank you for being persistent here!

@smockle
Copy link

smockle commented Dec 6, 2022

I opened a PR to fix this—reviews and feedback welcomed! Flag aria-label and aria-labelledby on non-interactive elements (jsx-eslint/eslint-plugin-jsx-a11y#911)

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 a pull request may close this issue.

3 participants