feat(linter): implement jsx-a11y/interactive-supports-focus rule#18229
feat(linter): implement jsx-a11y/interactive-supports-focus rule#18229Ashutosh0x wants to merge 6 commits intooxc-project:mainfrom
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/interactive-supports-focus accessibility rule which enforces that elements with interactive roles must be focusable. However, the PR also includes unrelated changes to the formatter's custom import grouping feature, which should ideally be in a separate PR.
Changes:
- Implements new linter rule
interactive-supports-focusto enforce focusability for interactive elements - Adds regex pattern matching support for custom import groups in the formatter (unrelated change)
- Updates configuration schema descriptions for element name patterns
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_linter/src/rules/jsx_a11y/interactive_supports_focus.rs | New accessibility linter rule implementation with tests |
| crates/oxc_linter/src/rules.rs | Registration of the new linter rule |
| npm/oxfmt/configuration_schema.json | Updated description for elementNamePattern from "prefixes" to "regular expressions" |
| crates/oxc_formatter/src/ir_transform/sort_imports/group_matcher.rs | Implements regex matching for custom import groups |
| crates/oxc_formatter/src/ir_transform/sort_imports/options.rs | Adds documentation for regex patterns |
| crates/oxc_formatter/tests/ir_transform/sort_imports/custom_groups.rs | Adds tests for regex pattern support |
| crates/oxc_formatter/Cargo.toml | Adds lazy-regex dependency |
| Cargo.lock | Updates dependency lock file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let has_interactive_role = if let Some(role_attr) = role { | ||
| if let Some(role_val) = crate::utils::get_string_literal_prop_value(role_attr) { | ||
| // List from no_static_element_interactions.rs | ||
| crate::rules::jsx_a11y::no_static_element_interactions::INTERACTIVE_ROLES.contains(&role_val) |
There was a problem hiding this comment.
The INTERACTIVE_ROLES constant is not publicly exported from the no_static_element_interactions module. This will cause a compilation error. The constant should either be made public in no_static_element_interactions.rs or the interactive roles list should be duplicated here.
There was a problem hiding this comment.
Can you refactor INTERACTIVE_ROLES into utils? A rule depending on another rule is not so great 🤞
| if has_jsx_prop(jsx_opening_el, "tabIndex").is_none() { | ||
| ctx.diagnostic(interactive_supports_focus_diagnostic(jsx_opening_el.span)); |
There was a problem hiding this comment.
The test case tabIndex={undefined} is expected to fail, but JSX expression values (like {undefined}) are different from missing attributes. This check only verifies if the tabIndex prop exists, not whether its value is a valid string literal. Elements with tabIndex={undefined} will have the attribute present and will not trigger a diagnostic, even though the comment suggests it should be treated as "effectively missing if undefined".
| eprintln!( | ||
| "oxc_formatter: invalid regex pattern `{}` in custom group `{}`: {}", | ||
| p, | ||
| custom_group.group_name, | ||
| err | ||
| ); | ||
| None |
There was a problem hiding this comment.
This error message uses eprintln! which outputs to stderr. For a linter/formatter library, this should use a proper logging mechanism or return errors to the caller rather than directly printing to stderr. Users may not want these messages appearing in their build output.
|
|
||
| fn interactive_supports_focus_diagnostic(span: Span) -> OxcDiagnostic { | ||
| OxcDiagnostic::warn("Elements with interactive roles must be focusable.") | ||
| .with_help("Interactive elements must be able to receive focus. Add a valid `tabIndex` attribute.") |
There was a problem hiding this comment.
The term "tabIndex" should use the correct JSX casing in the help message. In React/JSX, the attribute is tabIndex (camelCase), not tabIndex in backticks suggesting it's a string. The help text should clarify that this refers to the JSX prop tabIndex.
| .with_help("Interactive elements must be able to receive focus. Add a valid `tabIndex` attribute.") | |
| .with_help("Interactive elements must be able to receive focus. In JSX, add a valid tabIndex prop.") |
| // Logic adapted from eslint-plugin-jsx-a11y/interactive-supports-focus | ||
|
|
||
| // 1. If it's not a DOM element, return | ||
| if !HTML_TAG.contains(element_type.as_ref()) { | ||
| return; | ||
| } | ||
|
|
||
| // 2. If it's hidden or presentation, return | ||
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) || is_presentation_role(jsx_opening_el) { | ||
| return; | ||
| } | ||
|
|
||
| // 3. Check if it is interactive | ||
| // The rule only cares if the element is interactive. | ||
| // If it IS interactive, it MUST be focusable. | ||
| // Wait, `is_interactive_element` checks if the *semantics* are interactive (e.g. role=button, or <button>). | ||
| // ESLint logic: | ||
| // - Get roles (implicit + explicit) | ||
| // - If explicit role is interactive, check focusable. | ||
| // - If semantic element is interactive, check focusable. | ||
|
|
||
| // OxC's `is_interactive_element` seems to cover native elements + some roles? | ||
| // Let's check `is_interactive_element` implementation again. | ||
| // It checks: button, details, embed, iframe, label, select, textarea, input(!hidden), a(href), audio(controls), video(controls), img(usemap). | ||
| // It does NOT seem to check `role="button"` on a `div`. | ||
| // We need to check roles manually. | ||
|
|
||
| let is_native_interactive = is_interactive_element(&element_type, jsx_opening_el); | ||
|
|
||
| // Note: We need to handle `role="button"` etc. logic similar to ESLint. | ||
| // For now, let's look at `no_static_element_interactions` which specifically handles roles. | ||
| // But `interactive-supports-focus` logic is: | ||
| // "Elements with interactive roles must be focusable." | ||
|
|
||
| // If the element is natively interactive (like <button>), it IS focusable by default (unless disabled, etc, but typically yes). | ||
| // So this rule mostly catches: | ||
| // - Non-interactive elements (div, span) given an interactive role. | ||
| // - Native interactive elements that are somehow made non-focusable (maybe, but less common). |
There was a problem hiding this comment.
These extensive comments appear to be internal thought processes and implementation notes rather than useful code documentation. They should either be removed or condensed into a brief, clear explanation of the logic. Long stream-of-consciousness comments reduce code maintainability.
|
Hi @Boshen! I'd like to work on this rule implementation to help with the jsx-a11y compatibility effort. I've opened this PR with the implementation. Happy to make any changes needed! |
There was a problem hiding this comment.
Please revert the changes in oxc_formatter this PR should be only for the lint rule.
You can extract them into a new PR if you would like to have this code change inside the formatter.
| return; | ||
| } | ||
|
|
||
| if is_hidden_from_screen_reader(ctx, jsx_opening_el) || is_presentation_role(jsx_opening_el) { |
There was a problem hiding this comment.
A check for disabled element is missing:
https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/8f75961d965e47afb88854d324bd32fafde7acfe/src/rules/interactive-supports-focus.js#L83
| let is_native_interactive = is_interactive_element(&element_type, jsx_opening_el); | ||
|
|
||
| // Check if the element has an interactive role. | ||
| let role = has_jsx_prop(jsx_opening_el, "role"); |
There was a problem hiding this comment.
Please refactor this code block into is_interactive_role utils function in react, where is_interactive_element is there too :)
| false | ||
| }; | ||
|
|
||
| if !is_native_interactive && !has_interactive_role { |
There was a problem hiding this comment.
The rule should only report when an interactive property is found (like onClick):
https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/8f75961d965e47afb88854d324bd32fafde7acfe/src/rules/interactive-supports-focus.js#L73
https://github.com/jsx-eslint/jsx-ast-utils/blob/main/src/eventHandlers.js
| fn test() { | ||
| use crate::tester::Tester; | ||
|
|
||
| let pass = vec![ |
There was a problem hiding this comment.
Please use the provided tests upstream to make sure we implement the rule as expected:
https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/__tests__/src/rules/interactive-supports-focus-test.js
There was a problem hiding this comment.
I know that the upstream tests are complex with JS execution code, but the half of "copy paste" tests are still missing.
| let has_interactive_role = if let Some(role_attr) = role { | ||
| if let Some(role_val) = crate::utils::get_string_literal_prop_value(role_attr) { | ||
| // List from no_static_element_interactions.rs | ||
| crate::rules::jsx_a11y::no_static_element_interactions::INTERACTIVE_ROLES.contains(&role_val) |
There was a problem hiding this comment.
Can you refactor INTERACTIVE_ROLES into utils? A rule depending on another rule is not so great 🤞
| }, | ||
| }; | ||
|
|
||
| fn interactive_supports_focus_diagnostic(span: Span) -> OxcDiagnostic { |
There was a problem hiding this comment.
| /// ``` | ||
| InteractiveSupportsFocus, | ||
| jsx_a11y, | ||
| correctness |
There was a problem hiding this comment.
| correctness | |
| correctness, | |
| pending // auto insert tabIndex |
- Refactor jsx-a11y/interactive-supports-focus to align with upstream - Implement jsx-a11y/no-interactive-element-to-noninteractive-role - Centralize accessibility utilities in utils/react.rs - Remove unrelated formatter changes - Fix AST API compatibility
de37c69 to
ef729c7
Compare
PR #18229 Feedback - Summary of ChangesHi @Sysix! I have addressed all the review feedback in the latest commit:
Verified all changes locally with |
Sysix
left a comment
There was a problem hiding this comment.
Implementation of no-interactive-element-to-noninteractive-role: Added this rule and verified it locally.
Looks AI.
Added 2 more comments that detects that AI.
https://oxc.rs/docs/contribute/introduction.html#ai-usage-policy
Low-quality or unreviewed AI content will be closed immediately
| const INTERACTIVE_PROPS: [&str; 6] = [ | ||
| "onClick", | ||
| "onMouseDown", | ||
| "onMouseUp", | ||
| "onKeyPress", | ||
| "onKeyDown", | ||
| "onKeyUp", | ||
| ]; |
There was a problem hiding this comment.
I did provide you with the exact references to the interactive props, they are still missing some.
#18229 (comment)
| fn test() { | ||
| use crate::tester::Tester; | ||
|
|
||
| let pass = vec![ |
There was a problem hiding this comment.
I know that the upstream tests are complex with JS execution code, but the half of "copy paste" tests are still missing.
|
@Boshen @Sysix
|
This PR implements the
jsx-a11y/interactive-supports-focusrule.It enforces that elements with interactive roles must be focusable (i.e., they must have a valid
tabIndexor be natively focusable).References