-
Notifications
You must be signed in to change notification settings - Fork 659
feat(rome_js_analyze): noRedundantRoles #4293
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
/// Given a element and attributes, it returns the metadata of the element's implicit role. | ||
/// | ||
/// Check: https://www.w3.org/TR/html-aria/#docconformance | ||
pub fn get_implicit_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.
To create this function, I referred to the page commented.
https://www.w3.org/TR/html-aria/#docconformance
And I added some role definition.
crates/rome_aria/src/roles.rs
Outdated
"b" | "bdi" | "bdo" | "body" | "data" | "div" | "hgroup" | "i" | "q" | "samp" | ||
| "small" | "span" | "u" => &GenericRole as &dyn AriaRoleDefinition, | ||
"header" | "footer" => { | ||
// This plugin does not support checking a descendant of an element. |
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.
nit: I don't think "plugin" is the correct word in this context... did you mean "crate" by any chance?
crates/rome_aria/src/roles.rs
Outdated
&self, | ||
element: &str, | ||
// To generate `attributes`, you can use `rome_js_analyze::aria_services::AriaServices::extract_defined_attributes` | ||
attributes: HashMap<String, Vec<String>>, |
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 could pass a reference
attributes: HashMap<String, Vec<String>>, | |
attributes: &HashMap<String, Vec<String>>, |
Since we are not modifying the map
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.
Thanks, I updated the argument.
declare_rule! { | ||
/// Enforce explicit `role` property is not the same as implicit/default role property on an element. | ||
/// | ||
/// EsLint Equivalent: [no-redundant-roles](https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/main/docs/rules/no-redundant-roles.md) |
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.
This is not exactly an eslint equivalent, because it comes from a plugin. It's not a "core" eslint rule. Maybe we should rephrase it or remove it
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 described the equivalent is eslint-plugin-jsx-a11y at fcae9ed.
nits: And I'd like to update default declare_rule!
comment EsLint
to ESLint
in another PR.
let node = ctx.query(); | ||
let aria_roles = ctx.aria_roles(); | ||
|
||
if let AnyJsxElement::JsxOpeningElement(_) = &node { |
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.
Should we include self closing elements too? I think
<img role="img" />
Should be invalid case and trigger this rule
@@ -0,0 +1,22 @@ | |||
<> | |||
<a href="#" role="link"></a> |
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.
Self closing tags should be included. Here's the list of tests we had previously: https://github.com/rome/tools/blob/archived-js/internal/compiler/lint/rules/a11y/noRedundantRoles.test.toml
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 didn't notice the file! thanks, I'll fix the test case and logic.
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.
This commit has handled self closing tags.
c0526ee
I commented out some test cases, because they (e.g. <td>
,<option>
, <th>
) need to check the ancestors. To handle those, we need to update aria crate.
@@ -38,6 +39,33 @@ impl AriaServices { | |||
pub fn iso_language_list(&self) -> &'static [&'static str] { | |||
languages() | |||
} | |||
|
|||
/// Extracts defined attributes as HashMap (key: attribute name, value: attribute values). | |||
pub fn extract_defined_attributes( |
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.
nit: maybe extract_attributes
is enough? I had to understand what "defined" meant, and the function documentation didn't mention anything.
The documentation should say that only JsxAttribute
are mapped! That's an essential piece of information.
The documentation should also mention some transformation going on, so maybe mentioning some code example that shows the final result can help developers.
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 agree with all.
- renamed to
extract_attributes
- added some detail to documentation
- added test for
extract_attributes
to show how to use (it's kinda long for rustdoc)
let name = name.to_string().to_lowercase(); | ||
// handle name only attribute e.g. `<img aria-hidden alt="photo" />` | ||
let Some(initializer) = attr.initializer() else { | ||
defined_attributes.entry(name).or_insert(vec!["".to_string()]); |
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.
Attributes without a value should have "true"
as value.
}; | ||
let initializer = initializer.value().ok()?; | ||
let text = initializer.inner_text_value().ok()??; | ||
let text = text.to_lowercase(); |
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.
Any particular reason why we transform the string? If that's intended, we should make sure to have some test that validates this code path (all tests have lower case attribute values)
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.
Sorry there is no need to transform the string. I removed it.
let text = text.to_lowercase(); | ||
let role = text.split(' ').collect::<Vec<&str>>(); | ||
let role = role.first()?; |
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.
Could you explain this part of the code? There's no comment or documentation that explain why we are taking the first part of a split string.
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.
In my view, if a role
attribute has multiple values, the first valid role will be used as role. Others are fallbacks.
Platform accessibility APIs traditionally have had a finite set of predefined roles that are expected by assistive technologies on that platform and only one or two roles may be exposed. In contrast, WAI-ARIA allows multiple roles to be specified as an ordered set of space-separated valid role tokens. The additional roles are fallback roles similar to the concept of specifying multiple fonts in case the first choice font type is not supported. https://www.w3.org/TR/2014/REC-wai-aria-implementation-20140320/#mapping_role
I updated the logic to find first valid role with find_map
and added a comment.
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.
Wow, that's a fantastic piece of information! That's great!
I wouldn't mind to add this in a code comment, so we don't lose it!
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.
// If a role attribute has multiple values, the first valid value (specified role) will be used. | ||
// Check: https://www.w3.org/TR/2014/REC-wai-aria-implementation-20140320/#mapping_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.
yes, I added about that here!
sorry for making the changes after receiving approval, but I've implemented a suggestion fix. |
noRedundantRoles: Enforce explicit `role` property is not the same as implicit/default role property on an element. chore: fix equivalent to eslint-plugin-jsx-a11y chore: fix comment plugin to crate
fix: reduce waste mutation in aria_services fix: handle JsxSelfClosingElement fix: handle input type=text test: update snapshot test: fix test_get_implicit_role test: for extract_attributes refactor: extract_attributes test: update snapshot chore: codegen and remove rule chore: fix for check-ready chore: add comment chore: remove note test: fix waste note chore: update website doc
@unvalley That's great! Could you please update the We recently updated the contribution guidelines. I think something like - [noRedundantRoles](https://docs.rome.tools/linter/rules/noRedundantRoles) Should be fine (please double check the link) |
@ematipico Thanks, I updated the CHANGELOG.md! |
Summary
Closes #3939
Test Plan
Added snapshot test cases refer to jsx-a11y/noRedundantRoles and some.
cargo test -p rome_js_analyze -- no_redundant_roles
Documentation
I will create a new PR to update the documentationThis pr has doc