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

feat(linter): eslint-plugin-jsx-a11y no-redundant-roles rule #1981

Merged
merged 5 commits into from
Jan 14, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ mod jsx_a11y {
pub mod no_aria_hidden_on_focusable;
pub mod no_autofocus;
pub mod no_distracting_elements;
pub mod no_redundant_roles;
pub mod prefer_tag_over_role;
pub mod role_has_required_aria_props;
pub mod role_support_aria_props;
Expand Down Expand Up @@ -518,6 +519,7 @@ oxc_macros::declare_all_lint_rules! {
jsx_a11y::no_access_key,
jsx_a11y::no_aria_hidden_on_focusable,
jsx_a11y::no_autofocus,
jsx_a11y::no_redundant_roles,
jsx_a11y::prefer_tag_over_role,
jsx_a11y::role_has_required_aria_props,
jsx_a11y::scope,
Expand Down
122 changes: 122 additions & 0 deletions crates/oxc_linter/src/rules/jsx_a11y/no_redundant_roles.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
use crate::{
context::LintContext,
rule::Rule,
utils::{get_element_type, has_jsx_prop_lowercase},
AstNode,
};
use oxc_ast::{
ast::{JSXAttributeItem, JSXAttributeValue},
AstKind,
};
use oxc_diagnostics::{
miette::{self, Diagnostic},
thiserror::{self, Error},
};
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use phf::{phf_map, phf_set};

#[derive(Debug, Error, Diagnostic)]
#[error(
"eslint-plugin-jsx-a11y(no-redundant-roles): The element `{element}` has an implicit role of `{role}`. Defining this explicitly is redundant and should be avoided."
)]
#[diagnostic(
severity(warning),
help("Remove the redundant role `{role}` from the element `{element}`.")
)]
struct NoRedundantRolesDiagnostic {
#[label]
pub span: Span,
pub element: String,
pub role: String,
}

#[derive(Debug, Default, Clone)]
pub struct NoRedundantRoles;

declare_oxc_lint!(
/// ### What it does
/// Enforces that the explicit role property is not the same as implicit/default role property on element.
///
/// ### Why is this bad?
/// Redundant roles can lead to confusion and verbosity in the codebase.
///
/// ### Example
/// ```javascript
/// // Bad
/// <nav role="navigation" />
///
/// // Good
/// <nav />
/// ```
NoRedundantRoles,
correctness
);

static DEFAULT_ROLE_EXCEPTIONS: phf::Map<&'static str, phf::Set<&'static str>> = phf_map! {
"nav" => phf_set!{"navigation"},
camc314 marked this conversation as resolved.
Show resolved Hide resolved
"button" => phf_set!{"button"},
"body" => phf_set!{"document"},
};

impl Rule for NoRedundantRoles {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if let AstKind::JSXOpeningElement(jsx_el) = node.kind() {
if let Some(component) = get_element_type(ctx, jsx_el) {
if let Some(JSXAttributeItem::Attribute(attr)) =
has_jsx_prop_lowercase(jsx_el, "role")
{
if let Some(JSXAttributeValue::StringLiteral(role_values)) = &attr.value {
let roles: Vec<String> = role_values
.value
.split_whitespace()
.map(std::string::ToString::to_string)
.collect();
for role in &roles {
let exceptions = DEFAULT_ROLE_EXCEPTIONS.get(&component);
if exceptions.map_or(false, |set| set.contains(role)) {
ctx.diagnostic(NoRedundantRolesDiagnostic {
span: attr.span,
element: component.clone(),
role: role.to_string(),
});
}
}
}
}
}
}
}
}

#[test]
fn test() {
use crate::rules::NoRedundantRoles;
use crate::tester::Tester;

fn settings() -> serde_json::Value {
serde_json::json!({
"jsx-a11y": {
"components": {
"Button": "button",
}
}
})
}

let pass = vec![
("<div />", None, None, None),
("<button role='main' />", None, None, None),
("<MyComponent role='button' />", None, None, None),
("<button role={`${foo}button`} />", None, None, None),
("<Button role={`${foo}button`} />", None, Some(settings()), None),
];

let fail = vec![
("<button role='button' />", None, None, None),
("<body role='document' />", None, None, None),
("<Button role='button' />", None, Some(settings()), None),
];

Tester::new(NoRedundantRoles::NAME, pass, fail).test_and_snapshot();
}
26 changes: 26 additions & 0 deletions crates/oxc_linter/src/snapshots/no_redundant_roles.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
source: crates/oxc_linter/src/tester.rs
expression: no_redundant_roles
---
⚠ eslint-plugin-jsx-a11y(no-redundant-roles): The element `button` has an implicit role of `button`. Defining this explicitly is redundant and should be avoided.
╭─[no_redundant_roles.tsx:1:1]
1 │ <button role='button' />
· ─────────────
╰────
help: Remove the redundant role `button` from the element `button`.

⚠ eslint-plugin-jsx-a11y(no-redundant-roles): The element `body` has an implicit role of `document`. Defining this explicitly is redundant and should be avoided.
╭─[no_redundant_roles.tsx:1:1]
1 │ <body role='document' />
· ───────────────
╰────
help: Remove the redundant role `document` from the element `body`.

⚠ eslint-plugin-jsx-a11y(no-redundant-roles): The element `button` has an implicit role of `button`. Defining this explicitly is redundant and should be avoided.
╭─[no_redundant_roles.tsx:1:1]
1 │ <Button role='button' />
· ─────────────
╰────
help: Remove the redundant role `button` from the element `button`.