[New rule] disallow aria-hidden="true" on focusable elements#888
Conversation
khiga8
changed the title
aria-hidden="true" on focusable elementsaria-hidden="true" on focusable elements
Codecov Report
@@ Coverage Diff @@
## main #888 +/- ##
=======================================
Coverage 99.29% 99.29%
=======================================
Files 101 103 +2
Lines 1555 1571 +16
Branches 511 514 +3
=======================================
+ Hits 1544 1560 +16
Misses 11 11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jessebeach
left a comment
jessebeach
left a comment
There was a problem hiding this comment.
@khiga8 this is a great idea for a rule. You're off to a solid start. I'd love to move your isFocusable function to its own utility. And I have a question about the reason behind traversing child elements.
I think you can get this to a place where we can get it committed. I look forward to the updates!
| const schema = generateObjSchema(); | ||
|
|
||
| /* Returns true when element is interactive and does not have `tabIndex=-1`, or when an element has a tabIndex that isn't -1. */ | ||
| function isFocusable(type, attributes) { |
There was a problem hiding this comment.
Can you move this function to https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/tree/main/src/util and add tests for it? This is a great utility function to have.
It does need some work though, so I encourage you to set up test cases that enumerate all the possible scenarios. And keep in mind that authors will, in practice, use any and every integer for tabindex, including integers less than -1 and integers greater than zero. So your test cases should take these into account as well.
| JSXOpeningElement(node) { | ||
| const { attributes } = node; | ||
| const type = elementType(node); | ||
| const ariaHidden = getPropValue(getProp(attributes, 'aria-hidden')) === true; |
| const type = elementType(node); | ||
| const ariaHidden = getPropValue(getProp(attributes, 'aria-hidden')) === true; | ||
|
|
||
| if (!ariaHidden) return; |
There was a problem hiding this comment.
It feels like this would be a bit more idiomatic written like this, what do you think?
if (isHidden && isFocusable) {
// create the report side effect
}
| message: errorMessage, | ||
| }); | ||
| } | ||
| const { children } = node.parent; |
There was a problem hiding this comment.
Generally, you want to use ESLint's visitor pattern to visit nodes, rather than doing your own tree traversal within a rule down into child nodes. What is purpose of creating reports for focusable children of the opening node?
There was a problem hiding this comment.
Thank you for pointing this out. I'm new to ESLint rule creation so uh.. please excuse my non-idiomatic code 🙇♀️.
My intention had been to cover cases where aria-hidden is set on an element that contains focusable children like:
<div aria-hidden="true">
<a href="/">Some site</a>
</div>Is this something that can be accomplished with the visitor pattern? I'll have to do a bit more digging into ESLint conventions.
There was a problem hiding this comment.
My intention had been to cover cases where aria-hidden is set on an element that contains focusable children like:
Ok, I understand. For this rule, I'm going to recommend that you keep it narrowly focused to just focusable elements and not their ancestors or descendants. The problem of the cascading effects of aria-hidden on the AX tree is a thorny one and it will complicate a rule that is otherwise straightforward and easy to reason about. Plus, these sorts of tree relationships are hard to lint for, because you're only getting a snippet of the total render tree while linting, so that incidence of positive cases is going to be low with this static linting method. The reason I really like your rule is that it can be narrowly focused to focusable elements and there is good literature about how aria-hidden should interact with these.
There was a problem hiding this comment.
please excuse my non-idiomatic code 🙇♀️
No need for apologies :)
Raise error when aria-hidden="true" is set on focusable element. Exceptions are if `tabindex=-1`. Fixes jsx-eslint#881
jessebeach
left a comment
jessebeach
left a comment
There was a problem hiding this comment.
This is amazing @khiga8! Thank you so much for proposing this rule and going through the iterations to develop it. Let's get it landed!
|
@ljharb ready to land |
ljharb
force-pushed
the
kh-disallow-aria-hidden-on-interactive
branch
from
42db006 to
e22aef4
Compare
3 participants
Fixes: #881
This adds a new rule that flags when
aria-hidden="true"is set on focusable elementsor contain child elements that are focusable. People often setaria-hidden="true"to hide an element from AT users, but if the element is focusable, AT users can still access it which can be confusing.I wasn't sure what the contributing process is like so wanted to get a draft PR going to get thoughts. I looked through the list of jsx-a11y rules and I couldn't find a rule for something like this but let me know if I missed it. If maintainers are okay moving forward with this, I will add a doc page. I've ran the lint and added a test.
Related references:
Do not use aria-hidden="true" on focusable elements.