feat(linter): implement jsx-a11y/no-interactive-element-to-noninteractive-role#18232
Conversation
- Handle invalid regex patterns matching with error logging - Optimize empty groups - Auto-anchor patterns for backward compatibility
There was a problem hiding this comment.
Pull request overview
This pull request implements the jsx-a11y linter rule no-interactive-element-to-noninteractive-role which prevents interactive elements from being assigned non-interactive ARIA roles. Additionally, it includes the interactive-supports-focus rule and updates the import sorting formatter to support regular expressions in custom groups.
Changes:
- Implements
no-interactive-element-to-noninteractive-rolelinter rule to catch accessibility violations where interactive elements receive non-interactive roles - Implements
interactive-supports-focuslinter rule to ensure interactive elements are focusable - Updates import sorting formatter to use regex patterns instead of simple prefix matching for custom groups
- Exposes
INTERACTIVE_ROLESandNON_INTERACTIVE_ROLESconstants as public for reuse
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_linter/src/rules/jsx_a11y/no_interactive_element_to_noninteractive_role.rs | New rule implementation with critical bugs in early return logic and excessive debug comments |
| crates/oxc_linter/src/rules/jsx_a11y/interactive_supports_focus.rs | New rule implementation with incomplete tabIndex validation |
| crates/oxc_linter/src/rules/jsx_a11y/no_static_element_interactions.rs | Exposes role constants as public for reuse in other rules |
| crates/oxc_linter/src/rules.rs | Registers the two new linter rules |
| crates/oxc_formatter/src/ir_transform/sort_imports/group_matcher.rs | Replaces prefix matching with regex patterns, silently ignores invalid patterns |
| crates/oxc_formatter/src/ir_transform/sort_imports/options.rs | Updates documentation for regex support |
| crates/oxc_formatter/tests/ir_transform/sort_imports/custom_groups.rs | Adds tests for regex support and backward compatibility |
| npm/oxfmt/configuration_schema.json | Updates schema descriptions for regex support |
| crates/oxc_formatter/Cargo.toml | Adds lazy-regex dependency |
| Cargo.lock | Updates lock file with lazy-regex dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
| match Regex::new(&anchored) { | ||
| Ok(regex) => Some(regex), | ||
| Err(_) => None, |
There was a problem hiding this comment.
Invalid regex patterns are silently ignored by using filter_map with Err(_) => None. This could lead to unexpected behavior where users think their custom group patterns are working, but they're actually being silently skipped. Consider logging a warning or error when a regex pattern fails to compile so users are aware of invalid patterns in their configuration.
| Err(_) => None, | |
| Err(err) => { | |
| eprintln!( | |
| "Warning: invalid regex pattern '{}' in custom group '{}': {}", | |
| p, custom_group.group_name, err | |
| ); | |
| None | |
| } |
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) || is_presentation_role(jsx_opening_el) { | ||
| // If it's presentation role, it's effectively non-interactive, but checking if it WAS interactive... | ||
| // If <button role="presentation">, that IS exactly what we want to catch? | ||
| // "Interactive elements should not be assigned non-interactive roles." | ||
| // 'presentation' IS a non-interactive role. | ||
| // So if `is_presentation_role` is true, we might actually want to FLAG it if the element *was* interactive. | ||
|
|
||
| // However, `is_presentation_role` checks if the role attribute IS 'presentation' or 'none'. | ||
| // If we have <button role="presentation">, `is_interactive_element` might return false because the role overrides? | ||
| // `is_interactive_element` checks semantics. | ||
| // Let's check `is_interactive_element` details. | ||
| // It usually checks the tag name and attributes. | ||
|
|
||
| // If <button role="presentation">, does `is_interactive_element` return true? | ||
| // Usually `is_interactive_element` checks *native* interactivity or *role* interactivity. | ||
| // If it's <button>, it IS natively interactive. | ||
| } | ||
|
|
||
| // Wait, `is_presentation_role` returning true means the user explicitly set role="presentation". | ||
| // If the element *would have been* interactive (like <button>), then setting role="presentation" is bad (usually). | ||
| // Actually, sometimes you DO want to remove semantics from a button? No, that's usually bad for accessibility if it's still clickable. | ||
| // But if it's just visual... why use a button? | ||
|
|
||
| // The rule says: "Interactive elements should not be assigned non-interactive roles." | ||
| // Presentation is non-interactive. | ||
| // So we should NOT return early here if we want to catch <button role="presentation">. | ||
|
|
||
| // Let's look at `is_interactive_element`. It takes `element_type` and `jsx_opening_el`. | ||
| // If `jsx_opening_el` has `role="presentation"`, does `is_interactive_element` account for that? | ||
| // Likely yes. If role is presentation, it might be considered non-interactive. | ||
|
|
There was a problem hiding this comment.
This code block should be removed. It contains incomplete reasoning and commented-out code that was left in during development. The early return on line 68 is incorrect because it prevents the rule from catching cases where interactive elements have role="presentation" or role="none", which are exactly the violations this rule should detect. The commented thoughts suggest the developer was working through the logic but never cleaned up this section.
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) || is_presentation_role(jsx_opening_el) { | |
| // If it's presentation role, it's effectively non-interactive, but checking if it WAS interactive... | |
| // If <button role="presentation">, that IS exactly what we want to catch? | |
| // "Interactive elements should not be assigned non-interactive roles." | |
| // 'presentation' IS a non-interactive role. | |
| // So if `is_presentation_role` is true, we might actually want to FLAG it if the element *was* interactive. | |
| // However, `is_presentation_role` checks if the role attribute IS 'presentation' or 'none'. | |
| // If we have <button role="presentation">, `is_interactive_element` might return false because the role overrides? | |
| // `is_interactive_element` checks semantics. | |
| // Let's check `is_interactive_element` details. | |
| // It usually checks the tag name and attributes. | |
| // If <button role="presentation">, does `is_interactive_element` return true? | |
| // Usually `is_interactive_element` checks *native* interactivity or *role* interactivity. | |
| // If it's <button>, it IS natively interactive. | |
| } | |
| // Wait, `is_presentation_role` returning true means the user explicitly set role="presentation". | |
| // If the element *would have been* interactive (like <button>), then setting role="presentation" is bad (usually). | |
| // Actually, sometimes you DO want to remove semantics from a button? No, that's usually bad for accessibility if it's still clickable. | |
| // But if it's just visual... why use a button? | |
| // The rule says: "Interactive elements should not be assigned non-interactive roles." | |
| // Presentation is non-interactive. | |
| // So we should NOT return early here if we want to catch <button role="presentation">. | |
| // Let's look at `is_interactive_element`. It takes `element_type` and `jsx_opening_el`. | |
| // If `jsx_opening_el` has `role="presentation"`, does `is_interactive_element` account for that? | |
| // Likely yes. If role is presentation, it might be considered non-interactive. | |
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) { | |
| return; | |
| } | |
|
|
||
| // Wait, `is_presentation_role` returning true means the user explicitly set role="presentation". | ||
| // If the element *would have been* interactive (like <button>), then setting role="presentation" is bad (usually). | ||
| // Actually, sometimes you DO want to remove semantics from a button? No, that's usually bad for accessibility if it's still clickable. | ||
| // But if it's just visual... why use a button? | ||
|
|
||
| // The rule says: "Interactive elements should not be assigned non-interactive roles." | ||
| // Presentation is non-interactive. | ||
| // So we should NOT return early here if we want to catch <button role="presentation">. | ||
|
|
||
| // Let's look at `is_interactive_element`. It takes `element_type` and `jsx_opening_el`. | ||
| // If `jsx_opening_el` has `role="presentation"`, does `is_interactive_element` account for that? | ||
| // Likely yes. If role is presentation, it might be considered non-interactive. | ||
|
|
||
| // We need to check if the *element itself* (tag) is natively interactive, regardless of the role. | ||
| // We can force `is_interactive_element` to check only tag? | ||
| // Or we can just assume common interactive tags: button, input, select, textarea, option, a[href], area[href]. | ||
|
|
||
| // Let's use `is_interactive_element` but understand we might need to be careful. | ||
|
|
||
| // Actually, we want to know if the element *is inherently interactive*. | ||
| let is_natively_interactive = is_interactive_element(&element_type, jsx_opening_el); | ||
|
|
||
| if !is_natively_interactive { | ||
| return; | ||
| } | ||
|
|
||
| // It IS interactive. Now check if it has a non-interactive role. |
There was a problem hiding this comment.
This commented block contains development notes that should be removed before merging. The logic appears to be incomplete and the comments suggest uncertainty about the implementation approach.
| // Wait, `is_presentation_role` returning true means the user explicitly set role="presentation". | |
| // If the element *would have been* interactive (like <button>), then setting role="presentation" is bad (usually). | |
| // Actually, sometimes you DO want to remove semantics from a button? No, that's usually bad for accessibility if it's still clickable. | |
| // But if it's just visual... why use a button? | |
| // The rule says: "Interactive elements should not be assigned non-interactive roles." | |
| // Presentation is non-interactive. | |
| // So we should NOT return early here if we want to catch <button role="presentation">. | |
| // Let's look at `is_interactive_element`. It takes `element_type` and `jsx_opening_el`. | |
| // If `jsx_opening_el` has `role="presentation"`, does `is_interactive_element` account for that? | |
| // Likely yes. If role is presentation, it might be considered non-interactive. | |
| // We need to check if the *element itself* (tag) is natively interactive, regardless of the role. | |
| // We can force `is_interactive_element` to check only tag? | |
| // Or we can just assume common interactive tags: button, input, select, textarea, option, a[href], area[href]. | |
| // Let's use `is_interactive_element` but understand we might need to be careful. | |
| // Actually, we want to know if the element *is inherently interactive*. | |
| let is_natively_interactive = is_interactive_element(&element_type, jsx_opening_el); | |
| if !is_natively_interactive { | |
| return; | |
| } | |
| // It IS interactive. Now check if it has a non-interactive role. | |
| // Determine whether the element is natively interactive; if it is not, this rule does not apply. | |
| let is_natively_interactive = is_interactive_element(&element_type, jsx_opening_el); | |
| if !is_natively_interactive { | |
| return; | |
| } | |
| // The element is interactive; now check if it has a non-interactive role. |
| // It IS interactive. Now check if it has a non-interactive role. | ||
| let role = has_jsx_prop(jsx_opening_el, "role"); | ||
| if let Some(role_attr) = role { | ||
| if let Some(role_val) = crate::utils::get_string_literal_prop_value(role_attr) { | ||
| if crate::rules::jsx_a11y::no_static_element_interactions::NON_INTERACTIVE_ROLES.contains(&role_val) { | ||
| // Wait, `is_presentation_role` checks for "presentation" or "none". | ||
| // "presentation" is in NON_INTERACTIVE_ROLES? | ||
| // Let's check `no_static_element_interactions.rs` again. | ||
| // The list in step 1669 (lines 114-158) does NOT seem to include "presentation" or "none". | ||
| // It has "article", "img", "list", etc. | ||
|
|
||
| // We should ALSO check for "presentation" and "none" explicitly if they aren't in that list. | ||
| // Or check `is_presentation_role`. | ||
|
|
||
| // If I use `is_presentation_role(jsx_opening_el)` and it's true, AND it's a native interactive element, that's a violation. | ||
|
|
There was a problem hiding this comment.
These inline comments suggest the developer was still working through the logic during implementation. The comment on line 117 refers to "step 1669" which doesn't make sense in this context. Additionally, these comments should be removed as they don't add value to the final implementation and make the code harder to read.
| // It IS interactive. Now check if it has a non-interactive role. | |
| let role = has_jsx_prop(jsx_opening_el, "role"); | |
| if let Some(role_attr) = role { | |
| if let Some(role_val) = crate::utils::get_string_literal_prop_value(role_attr) { | |
| if crate::rules::jsx_a11y::no_static_element_interactions::NON_INTERACTIVE_ROLES.contains(&role_val) { | |
| // Wait, `is_presentation_role` checks for "presentation" or "none". | |
| // "presentation" is in NON_INTERACTIVE_ROLES? | |
| // Let's check `no_static_element_interactions.rs` again. | |
| // The list in step 1669 (lines 114-158) does NOT seem to include "presentation" or "none". | |
| // It has "article", "img", "list", etc. | |
| // We should ALSO check for "presentation" and "none" explicitly if they aren't in that list. | |
| // Or check `is_presentation_role`. | |
| // If I use `is_presentation_role(jsx_opening_el)` and it's true, AND it's a native interactive element, that's a violation. | |
| // Report when a natively interactive element is given a non-interactive or presentation role. | |
| let role = has_jsx_prop(jsx_opening_el, "role"); | |
| if let Some(role_attr) = role { | |
| if let Some(role_val) = crate::utils::get_string_literal_prop_value(role_attr) { | |
| if crate::rules::jsx_a11y::no_static_element_interactions::NON_INTERACTIVE_ROLES.contains(&role_val) { |
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) || is_presentation_role(jsx_opening_el) { | ||
| // If it's presentation role, it's effectively non-interactive, but checking if it WAS interactive... | ||
| // If <button role="presentation">, that IS exactly what we want to catch? | ||
| // "Interactive elements should not be assigned non-interactive roles." | ||
| // 'presentation' IS a non-interactive role. | ||
| // So if `is_presentation_role` is true, we might actually want to FLAG it if the element *was* interactive. | ||
|
|
||
| // However, `is_presentation_role` checks if the role attribute IS 'presentation' or 'none'. | ||
| // If we have <button role="presentation">, `is_interactive_element` might return false because the role overrides? | ||
| // `is_interactive_element` checks semantics. | ||
| // Let's check `is_interactive_element` details. | ||
| // It usually checks the tag name and attributes. | ||
|
|
||
| // If <button role="presentation">, does `is_interactive_element` return true? | ||
| // Usually `is_interactive_element` checks *native* interactivity or *role* interactivity. | ||
| // If it's <button>, it IS natively interactive. | ||
| } | ||
|
|
There was a problem hiding this comment.
The early return when is_presentation_role returns true on line 68 prevents the rule from correctly detecting violations. If an interactive element (like <button>) has role="presentation", this is exactly the kind of violation this rule should catch, but the early return causes the function to exit before checking. The rule should check if the element is natively interactive BEFORE the early return, or remove the is_presentation_role check from the early return condition.
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) || is_presentation_role(jsx_opening_el) { | |
| // If it's presentation role, it's effectively non-interactive, but checking if it WAS interactive... | |
| // If <button role="presentation">, that IS exactly what we want to catch? | |
| // "Interactive elements should not be assigned non-interactive roles." | |
| // 'presentation' IS a non-interactive role. | |
| // So if `is_presentation_role` is true, we might actually want to FLAG it if the element *was* interactive. | |
| // However, `is_presentation_role` checks if the role attribute IS 'presentation' or 'none'. | |
| // If we have <button role="presentation">, `is_interactive_element` might return false because the role overrides? | |
| // `is_interactive_element` checks semantics. | |
| // Let's check `is_interactive_element` details. | |
| // It usually checks the tag name and attributes. | |
| // If <button role="presentation">, does `is_interactive_element` return true? | |
| // Usually `is_interactive_element` checks *native* interactivity or *role* interactivity. | |
| // If it's <button>, it IS natively interactive. | |
| } | |
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) { | |
| // Elements hidden from screen readers are ignored by this rule. | |
| // Elements with role="presentation" must still be checked, since assigning a | |
| // non-interactive role to a natively interactive element is exactly what this | |
| // rule is meant to flag. | |
| } | |
| match has_jsx_prop(jsx_opening_el, "tabIndex") { | ||
| Some(JSXAttributeItem::Attribute(attr)) => { | ||
| if let Some(JSXAttributeValue::ExpressionContainer(container)) = &attr.value { | ||
| if let JSXExpression::Expression(expr) = &container.expression { | ||
| if let Expression::Identifier(id) = expr { | ||
| if id.name == "undefined" { | ||
| ctx.diagnostic(interactive_supports_focus_diagnostic(jsx_opening_el.span)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Some(JSXAttributeItem::SpreadAttribute(_)) => {} | ||
| None => { | ||
| ctx.diagnostic(interactive_supports_focus_diagnostic(jsx_opening_el.span)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The tabIndex validation is incomplete. The rule currently only checks if tabIndex is present or if it's the identifier undefined, but it doesn't validate that the tabIndex value is actually a valid number. For example, string values like tabIndex="0" are accepted, but expression values with invalid numbers or non-numeric expressions (besides undefined) are also accepted. The rule should verify that tabIndex has a numeric value when it's an expression container.
|
Thanks for the feedback! I'll address these issues immediately. @Boshen |
|
This should not include any changes to oxfmt. Moreover, the logic you're reusing from no-static-element-interactions is bad, and needs to be fixed before we add this rule. See #17817. |
|
@Boshen @Sysix
|
|
Note: closed another PR of him because of non self-review of AI usage: #18229 Probably the same here. |
|
Fine by me. Please respect the AI Disclosure Policy and thoroughly self review PRs before subletting them. |
Implements the jsx-a11y/no-interactive-element-to-noninteractive-role rule which enforces that interactive elements are not assigned non-interactive roles.