-
Notifications
You must be signed in to change notification settings - Fork 660
feat(rome_js_analyze): noAriaUnsupportedElements #4340
Conversation
feat: implemente rule logic chore: fix doc
docs: website lint rule page
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good! I just added some additional feedback to enhance the code :)
|
||
let element_name = node.name().ok()?.as_jsx_name()?.value_token().ok()?; | ||
let element_name = element_name.text_trimmed(); | ||
let aria_unsuppurted_elements = ["meta", "html", "script", "style"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this array outside the function and make it a const. Doing so, we can optimize its creation using the compiler:
const ARIA_UNSUPPORTED_ELEMENTS: [&str; 4] = ["meta", "html", "script", "style"];
If you apply this change, the next if
will be like this:
if aria_unsuppurted_elements.contains(&&element_name) {}
}) | ||
.collect(); | ||
|
||
if !attributes.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterators have a function called .count()
that can be used instead of is_empty()
. This means that we don't need to allocate a new vector!
We can remove .collect()
and use if attributes.count() > 0
7 │ <script role="script"></script> | ||
8 │ <style aria-labelledby></style> | ||
|
||
i Using roles on elements that do not support them can cause issues with screen readers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hint is a bit misleading here. The example applies an ARIA attribute, not a role
attribute. Do you think we can customise the message based on the attribute used? aria-*
VS role
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely it is better to guide against role
and aria-*
respectively, so I will implement it that way!
|
||
if ARIA_UNSUPPORTED_ELEMENTS.contains(&element_name) { | ||
// Check if the unsupported element has `role` or `aria-*` attribute | ||
let report = node.attributes().iter().find_map(|attribute| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the logic to use find_map
instead of filter_map
. Because I think it is enough that the rule needs to be reported when one of the violations (role or aria-*) are found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
@ematipico Thanks for the first review, could you check the report again? |
Summary
Closes #3937
This
noAriaUnsupportedElements
rule enforces that elements that do not support ARIA roles, states, and properties do not have those attributes.Diagnostics Example
Test Plan
cargo test -p rome_js_analyze -- no_aria_unsupported_elements
test cases: https://github.com/rome/tools/blob/archived-js/internal/compiler/lint/rules/a11y/noAriaUnsupportedElements.test.toml
Changelog
Documentation
[ ] I will create a new PR to update the documentation