feat(rome_js_analyze): rule useAnchorContent
#3370
Conversation
❌ Deploy Preview for docs-rometools failed.
|
crates/rome_js_analyze/src/analyzers/nursery/use_anchor_content.rs
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/tests/specs/nursery/useAnchorContent.jsx
Outdated
Show resolved
Hide resolved
crates/rome_js_analyze/src/analyzers/nursery/use_anchor_content.rs
Outdated
Show resolved
Hide resolved
|
|
||
| // If there's no `aria-hidden` attribute on the a element, | ||
| // proceed to check the accessibility of its child elements | ||
| if !node.is_hidden_from_screen_reader().unwrap_or(false) && node.has_accessible_child()? { |
There was a problem hiding this comment.
The usage of unwrap_or is a bit confusing.. any reason why you didn't use the try operator ? here? If it needs to be used, I would have expected is_hidden_from_screen_reader to use it, and change its return signature to bool
There was a problem hiding this comment.
I found the usage of unwrap_or not intuitive too. I wasn't sure what is the better way to handle the None propagation from the is_hidden_from_screen_reader helper function. I didn’t use the ? operator here because it would propagate None immediately without proceeding to run node.has_accessible_child()?.
So instead of using unwrap_or inside node.is_hidden_from_screen_reader (which I also found a bit unintuitive 🤔), it ended up having a Option<bool> return signature.
I adjusted the signature per your suggestions in 2159260, please let me know if we can improve the code further. Thanks!
There was a problem hiding this comment.
I think it's fine the current implementation. Unfortunately the Option<> can make this confusing when paired with boolean checks.
But I also think it's fine to use the try operator ? even it doesn't feel intuitive. The try operator is needed to bounce any potential errors in our syntax (among other things). That's why the rules are filled with ok()?.
For example here we are bypassing any potential errors inside the code (we have to remember that our parser is recoverable, so it's able to spit an AST that contains unknown nodes, which are errors). While this is fine and it doesn't break any logic, it's a case where we could terminate early instead of moving to the next check.
crates/rome_js_analyze/src/analyzers/nursery/use_anchor_content.rs
Outdated
Show resolved
Hide resolved
| /// | ||
| /// ```jsx,expect_diagnostic | ||
| /// <a><span aria-hidden="true">content</span></a | ||
| /// ``` |
There was a problem hiding this comment.
What's the expected output for an element that only contains whitespace
<a> </a>;
There was a problem hiding this comment.
It should be invalid, but the current implementation doesn't catch this case. I'll fix this issue.
Edit:
Fixed the issue and added more test cases in 3119407
| fn is_anchor(&self) -> Option<bool> { | ||
| Some(match self { | ||
| UseAnchorContentNode::JsxElement(element) => { | ||
| element.opening_element().ok()?.name().ok()?.text() == "a" |
There was a problem hiding this comment.
You have to use text_trimmed because it's possible for JSX elements to contain comments:
<div /*comment */>
There was a problem hiding this comment.
You have to use
text_trimmedbecause it's possible for JSX elements to contain comments:<div /*comment */>
The .text() API internally uses text_trimmed() :)
There was a problem hiding this comment.
@ematipico do you remember the rule where you re-wrote the match of the element name to use the token instead?
I think that would be beneficial to do here too because comparing SyntaxNodeText is more expensive and e.g. not even necessary if name is a JsxMemberName or JsxNamespaceName
There was a problem hiding this comment.
I can't recall unfortunately. I can do this refactor, in a separate PR. This is not the only place where we should check.
Summary
useAnchorContent#3300Test Plan
Added various test cases