feat(linter): add noNonInteractiveTabIndex for HTML#9306
Merged
ematipico merged 8 commits intobiomejs:nextfrom Apr 17, 2026
Merged
feat(linter): add noNonInteractiveTabIndex for HTML#9306ematipico merged 8 commits intobiomejs:nextfrom
ematipico merged 8 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: caf9531 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
A-Linter
viraxslot
commented
Mar 3, 2026
| } | ||
| } | ||
|
|
||
| fn is_not_interactive_element(node: &AnyHtmlElement) -> Option<bool> { |
Contributor
Author
There was a problem hiding this comment.
I'm using a simplified is_not_interactive_element check here. I found the full implementation in biome_aria::roles::AriaRoles::is_not_interactive_element that handles more edge cases, but I wasn't sure how to make it work with HTML AST.
Could you please suggest how to expose/reuse that logic for HTML rules? Or is the simplified approach acceptable for now?
Contributor
There was a problem hiding this comment.
implement the Element and Attribute traits that it uses for html elements and attributes. then you can pass those in to the is_not_interactive_element function
Merging this PR will degrade performance by 64.45%
Performance Changes
Comparing 
Footnotes
|
Contributor
WalkthroughAdds a new HTML accessibility lint rule Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs`:
- Around line 154-166: The match arm that classifies elements by interactivity
is wrong for <header> and anchors/areas; change the "header" arm to return
Some(true) (non-interactive), and replace the combined "a" | "area" arm with
separate handling for "a" and "area" that inspects the node via
node.find_attribute_by_name("href") (using the existing
initializer()/value().ok()/string_value() chain) and only treats the element as
interactive (Some(false)) when a non-empty href is present, otherwise treat it
as non-interactive (Some(true)); keep existing logic for other elements and the
input/type:hidden check using the same node.find_attribute_by_name flow.
- Around line 89-99: The current early-return `?` chain when reading a `role`
attribute (in the block using role_attribute,
role_attr.initializer()?.value().ok()?.string_value()?) causes the lint to exit
on malformed `role` values; change this to use option-preserving combinators
(e.g., replace the `?` chain with and_then/map calls) so you get an
Option<String> from role_attr.initializer()/value()/string_value() instead of
returning, then call AriaRole::from_roles on that Option and only return None
when an aria_role is present and aria_role.is_interactive(); otherwise allow the
lint to continue reporting. Ensure you update the block around role_attribute,
role_attr, AriaRole::from_roles, and aria_role.is_interactive(), and run `just
gen-rules` before the PR.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_html_analyze/tests/specs/a11y/noNoninteractiveTabindex/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noNoninteractiveTabindex/valid.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/html-no-noninteractive-tab-index.mdcrates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rscrates/biome_html_analyze/tests/specs/a11y/noNoninteractiveTabindex/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noNoninteractiveTabindex/valid.html
Comment on lines
+89
to
+99
| let role_attribute = node.find_attribute_by_name("role"); | ||
| if let Some(role_attr) = role_attribute { | ||
| let role_value = role_attr.initializer()?.value().ok()?.string_value()?; | ||
| let role = AriaRole::from_roles(role_value.trim()); | ||
|
|
||
| if let Some(aria_role) = role { | ||
| if aria_role.is_interactive() { | ||
| return None; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs | sed -n '75,110p'Repository: biomejs/biome
Length of output: 1387
🏁 Script executed:
rg "fn from_roles" --type rs -A 2Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
fd "Cargo.toml" | head -1 | xargs grep -A 10 "^\[package\]" | grep -E "(rust-version|edition)"Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
rg "fn from_roles" --type rust -A 3Repository: biomejs/biome
Length of output: 363
🏁 Script executed:
rg "struct AriaRole|enum AriaRole" --type rust -A 5Repository: biomejs/biome
Length of output: 1835
🏁 Script executed:
find . -name "Cargo.toml" -path "*/biome_html_analyze/*" | head -1 | xargs cat | grep -E "rust-version|edition"Repository: biomejs/biome
Length of output: 84
🏁 Script executed:
find . -name "Cargo.toml" -path "./Cargo.toml" | xargs cat | grep -A 5 "^\[workspace\]"Repository: biomejs/biome
Length of output: 300
🏁 Script executed:
grep -A 20 "^\[workspace\]" Cargo.toml | grep -E "edition|rust-version"Repository: biomejs/biome
Length of output: 76
🏁 Script executed:
rg "is_some_and" --type rust | head -5Repository: biomejs/biome
Length of output: 598
Avoid exiting early on malformed role attributes.
At line 91, the ? chain exits the function when a role attribute exists but lacks a usable value, suppressing the violation report. Since malformed roles aren't interactive roles, the lint should still fire. Use and_then to keep role as an Option, then only skip when it's both present and interactive.
Suggested fix
- let role_attribute = node.find_attribute_by_name("role");
- if let Some(role_attr) = role_attribute {
- let role_value = role_attr.initializer()?.value().ok()?.string_value()?;
- let role = AriaRole::from_roles(role_value.trim());
-
- if let Some(aria_role) = role {
- if aria_role.is_interactive() {
- return None;
- }
- }
- }
+ if let Some(role_attr) = node.find_attribute_by_name("role") {
+ let role = role_attr
+ .initializer()
+ .and_then(|init| init.value().ok())
+ .and_then(|value| value.string_value())
+ .and_then(|value| AriaRole::from_roles(value.trim()));
+
+ if role.is_some_and(|aria_role| aria_role.is_interactive()) {
+ return None;
+ }
+ }Remember to run just gen-rules before opening the PR.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let role_attribute = node.find_attribute_by_name("role"); | |
| if let Some(role_attr) = role_attribute { | |
| let role_value = role_attr.initializer()?.value().ok()?.string_value()?; | |
| let role = AriaRole::from_roles(role_value.trim()); | |
| if let Some(aria_role) = role { | |
| if aria_role.is_interactive() { | |
| return None; | |
| } | |
| } | |
| } | |
| if let Some(role_attr) = node.find_attribute_by_name("role") { | |
| let role = role_attr | |
| .initializer() | |
| .and_then(|init| init.value().ok()) | |
| .and_then(|value| value.string_value()) | |
| .and_then(|value| AriaRole::from_roles(value.trim())); | |
| if role.is_some_and(|aria_role| aria_role.is_interactive()) { | |
| return None; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs` around
lines 89 - 99, The current early-return `?` chain when reading a `role`
attribute (in the block using role_attribute,
role_attr.initializer()?.value().ok()?.string_value()?) causes the lint to exit
on malformed `role` values; change this to use option-preserving combinators
(e.g., replace the `?` chain with and_then/map calls) so you get an
Option<String> from role_attr.initializer()/value()/string_value() instead of
returning, then call AriaRole::from_roles on that Option and only return None
when an aria_role is present and aria_role.is_interactive(); otherwise allow the
lint to continue reporting. Ensure you update the block around role_attribute,
role_attr, AriaRole::from_roles, and aria_role.is_interactive(), and run `just
gen-rules` before the PR.
Comment on lines
+154
to
+166
| match element_name { | ||
| "header" => Some(false), | ||
| "a" | "area" | "button" | "select" | "textarea" => Some(false), | ||
| "svg" => Some(true), | ||
| "body" | "br" | "details" | "dir" | "frame" | "iframe" | "label" | "mark" | "marquee" | ||
| | "menu" | "meter" | "optgroup" | "pre" | "progress" | "ruby" => Some(true), | ||
| "input" => Some( | ||
| node.find_attribute_by_name("type") | ||
| .and_then(|attr| attr.initializer()) | ||
| .and_then(|init| init.value().ok()) | ||
| .and_then(|val| val.string_value()) | ||
| .is_some_and(|value| value == "hidden"), | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of the function containing these lines
cat -n crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs | head -200 | tail -100Repository: biomejs/biome
Length of output: 4255
🏁 Script executed:
# Let's also see the function signature and any documentation
cat -n crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs | head -180Repository: biomejs/biome
Length of output: 6825
🏁 Script executed:
# Check the full file to understand the rule intent
wc -l crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rsRepository: biomejs/biome
Length of output: 130
Fix element interactivity classification for correct rule coverage.
Line 155 marks <header> as interactive—incorrect, since it's a semantic container like <article> (shown invalid in rule docs). Line 156 treats all <a> and <area> elements as interactive regardless of href presence, missing non-interactive variants. Update line 155 to Some(true) and split line 156 to check href attribute for anchors/areas:
Suggested fix
match element_name {
- "header" => Some(false),
- "a" | "area" | "button" | "select" | "textarea" => Some(false),
+ "header" => Some(true),
+ "a" | "area" => Some(node.find_attribute_by_name("href").is_none()),
+ "button" | "select" | "textarea" => Some(false),
"svg" => Some(true),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| match element_name { | |
| "header" => Some(false), | |
| "a" | "area" | "button" | "select" | "textarea" => Some(false), | |
| "svg" => Some(true), | |
| "body" | "br" | "details" | "dir" | "frame" | "iframe" | "label" | "mark" | "marquee" | |
| | "menu" | "meter" | "optgroup" | "pre" | "progress" | "ruby" => Some(true), | |
| "input" => Some( | |
| node.find_attribute_by_name("type") | |
| .and_then(|attr| attr.initializer()) | |
| .and_then(|init| init.value().ok()) | |
| .and_then(|val| val.string_value()) | |
| .is_some_and(|value| value == "hidden"), | |
| ), | |
| match element_name { | |
| "header" => Some(true), | |
| "a" | "area" => Some(node.find_attribute_by_name("href").is_none()), | |
| "button" | "select" | "textarea" => Some(false), | |
| "svg" => Some(true), | |
| "body" | "br" | "details" | "dir" | "frame" | "iframe" | "label" | "mark" | "marquee" | |
| | "menu" | "meter" | "optgroup" | "pre" | "progress" | "ruby" => Some(true), | |
| "input" => Some( | |
| node.find_attribute_by_name("type") | |
| .and_then(|attr| attr.initializer()) | |
| .and_then(|init| init.value().ok()) | |
| .and_then(|val| val.string_value()) | |
| .is_some_and(|value| value == "hidden"), | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs` around
lines 154 - 166, The match arm that classifies elements by interactivity is
wrong for <header> and anchors/areas; change the "header" arm to return
Some(true) (non-interactive), and replace the combined "a" | "area" arm with
separate handling for "a" and "area" that inspects the node via
node.find_attribute_by_name("href") (using the existing
initializer()/value().ok()/string_value() chain) and only treats the element as
interactive (Some(false)) when a non-empty href is present, otherwise treat it
as non-interactive (Some(true)); keep existing logic for other elements and the
input/type:hidden check using the same node.find_attribute_by_name flow.
…axslot/biome into feat-html-no-noninteractive-tabindex
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs (2)
89-99:⚠️ Potential issue | 🟠 MajorDon’t let malformed
rolevalues bypass the lint.On Line [91], the
?chain exitsrun(...)whenroleexists but cannot be parsed, so invalid non-interactive elements can slip through. A broken role shouldn’t get a free pass.Suggested fix
- let role_attribute = node.find_attribute_by_name("role"); - if let Some(role_attr) = role_attribute { - let role_value = role_attr.initializer()?.value().ok()?.string_value()?; - let role = AriaRole::from_roles(role_value.trim()); - - if let Some(aria_role) = role - && aria_role.is_interactive() - { - return None; - } - } + if let Some(role_attr) = node.find_attribute_by_name("role") { + let role = role_attr + .initializer() + .and_then(|init| init.value().ok()) + .and_then(|value| value.string_value()) + .and_then(|value| AriaRole::from_roles(value.trim())); + + if role.is_some_and(|aria_role| aria_role.is_interactive()) { + return None; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs` around lines 89 - 99, The current code in run(...) uses the ? operator on role parsing (role_attr.initializer()?.value().ok()?.string_value()?) which causes the function to exit early on malformed role values and thus skip the lint; change these ? chains to explicit Option/Result handling so parsing failures do not short-circuit the function. Locate the role_attribute / role_attr handling and replace the chained ? calls with a safe extraction (e.g., initializer().and_then(...).and_then(...) or nested if let / match) to get role_value only when all steps succeed, then call AriaRole::from_roles(role_value.trim()) and only return None if Some(aria_role) exists and aria_role.is_interactive(); malformed or unparseable roles should be treated as non-interactive (i.e., do not return early).
154-166:⚠️ Potential issue | 🟠 MajorInteractivity mapping is incorrect for
<header>,<a>, and<area>.Line [155] currently treats
<header>as interactive, and Line [156] treats all anchors/areas as interactive regardless ofhref. That under-reports violations.Suggested fix
- "header" => Some(false), - "a" | "area" | "button" | "select" | "textarea" => Some(false), + "header" => Some(true), + "a" | "area" => Some( + node.find_attribute_by_name("href") + .and_then(|attr| attr.initializer()) + .and_then(|init| init.value().ok()) + .and_then(|val| val.string_value()) + .is_none_or(|value| value.trim().is_empty()), + ), + "button" | "select" | "textarea" => Some(false),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs` around lines 154 - 166, The current element interactivity mapping misclassifies elements: change the mapping so "header" is treated as non-interactive and make "a" and "area" interactive only when they have an actual href attribute; replace the current simple match arm ("a" | "area" | "button" | ...) with logic that checks node.find_attribute_by_name("href").and_then(|attr| attr.initializer()).and_then(|init| init.value().ok()).and_then(|v| v.string_value()).is_some() for "a" and "area" (use that predicate to return the interactive boolean), while keeping "button", "select", "textarea" handled as before and ensure the "header" arm returns the non-interactive value used elsewhere in this match (adjust the boolean to match the function's interactive/non-interactive convention).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs`:
- Around line 81-87: The code currently returns early for missing or unparseable
tabindex values and treats any non-string/malformed value as "negative"; update
the tabindex handling so that you only skip (return None) when the attribute has
a valid string that parses to an integer and that integer is negative.
Concretely, in the tabindex handling around tabindex_attribute.initializer(),
.value(), and .string_value() and in the similar block at lines ~140-147, do not
use the ?-early-return for missing initializer/value/string; instead check
presence and get the string, attempt to parse it to an integer (e.g., via
parse::<i32>()), and only call is_negative_tabindex (or replace with a simple
parsed_int < 0 check) when parsing succeeds—if parsing fails or parts are
missing, treat it as non-negative so the rule continues to lint rather than
skipping.
---
Duplicate comments:
In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs`:
- Around line 89-99: The current code in run(...) uses the ? operator on role
parsing (role_attr.initializer()?.value().ok()?.string_value()?) which causes
the function to exit early on malformed role values and thus skip the lint;
change these ? chains to explicit Option/Result handling so parsing failures do
not short-circuit the function. Locate the role_attribute / role_attr handling
and replace the chained ? calls with a safe extraction (e.g.,
initializer().and_then(...).and_then(...) or nested if let / match) to get
role_value only when all steps succeed, then call
AriaRole::from_roles(role_value.trim()) and only return None if Some(aria_role)
exists and aria_role.is_interactive(); malformed or unparseable roles should be
treated as non-interactive (i.e., do not return early).
- Around line 154-166: The current element interactivity mapping misclassifies
elements: change the mapping so "header" is treated as non-interactive and make
"a" and "area" interactive only when they have an actual href attribute; replace
the current simple match arm ("a" | "area" | "button" | ...) with logic that
checks node.find_attribute_by_name("href").and_then(|attr|
attr.initializer()).and_then(|init| init.value().ok()).and_then(|v|
v.string_value()).is_some() for "a" and "area" (use that predicate to return the
interactive boolean), while keeping "button", "select", "textarea" handled as
before and ensure the "header" arm returns the non-interactive value used
elsewhere in this match (adjust the boolean to match the function's
interactive/non-interactive convention).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (1)
crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs
Comment on lines
+81
to
+87
| let initializer = tabindex_attribute.initializer()?; | ||
| let value = initializer.value().ok()?; | ||
| let string_value = value.string_value()?; | ||
|
|
||
| if is_negative_tabindex(&string_value) { | ||
| return None; | ||
| } |
Contributor
There was a problem hiding this comment.
Only skip when tabindex is a valid negative integer.
Right now, malformed/unparseable tabindex values are treated as “negative” (Line [146]) and skipped; plus missing initialisers are skipped via ? on Line [81]-[83]. Both create false negatives.
Suggested fix
- let initializer = tabindex_attribute.initializer()?;
- let value = initializer.value().ok()?;
- let string_value = value.string_value()?;
-
- if is_negative_tabindex(&string_value) {
+ let is_negative = tabindex_attribute
+ .initializer()
+ .and_then(|init| init.value().ok())
+ .and_then(|value| value.string_value())
+ .is_some_and(|value| is_negative_tabindex(&value));
+
+ if is_negative {
return None;
}-/// Verifies if number string is an integer less than 0.
-/// Non-integer numbers are considered valid.
+/// Returns `true` only for valid integers strictly less than 0.
fn is_negative_tabindex(number_like_string: &str) -> bool {
- let number_string_result = number_like_string.trim().parse::<i32>();
-
- match number_string_result {
- Ok(number) => number < 0,
- Err(_) => true,
- }
+ matches!(number_like_string.trim().parse::<i64>(), Ok(number) if number < 0)
}Also applies to: 140-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs` around
lines 81 - 87, The code currently returns early for missing or unparseable
tabindex values and treats any non-string/malformed value as "negative"; update
the tabindex handling so that you only skip (return None) when the attribute has
a valid string that parses to an integer and that integer is negative.
Concretely, in the tabindex handling around tabindex_attribute.initializer(),
.value(), and .string_value() and in the similar block at lines ~140-147, do not
use the ?-early-return for missing initializer/value/string; instead check
presence and get the string, attempt to parse it to an integer (e.g., via
parse::<i32>()), and only call is_negative_tabindex (or replace with a simple
parsed_int < 0 check) when parsing succeeds—if parsing fails or parts are
missing, treat it as non-negative so the rule continues to lint rather than
skipping.
dyc3
reviewed
Mar 22, 2026
|
|
||
| Added the [`noNoninteractiveTabindex`](https://biomejs.dev/linter/rules/no-noninteractive-tabindex/) lint rule for HTML. This rule enforces that `tabindex` is not used on non-interactive elements, as it can cause usability issues for keyboard users. | ||
|
|
||
| `<div tabindex="0">Invalid: non-interactive element</div>` |
| } | ||
| } | ||
|
|
||
| fn is_not_interactive_element(node: &AnyHtmlElement) -> Option<bool> { |
Contributor
There was a problem hiding this comment.
implement the Element and Attribute traits that it uses for html elements and attributes. then you can pass those in to the is_not_interactive_element function
Member
|
@viraxslot, do you plan to finish this PR? |
Contributor
Author
|
@ematipico I'm not sure I'll have enough time in the nearest future, sorry. You can take over it if needed. |
Netail
reviewed
Apr 17, 2026
Comment on lines
+151
to
+166
| let element_name_token = node.name()?; | ||
| let element_name = element_name_token.text(); | ||
|
|
||
| match element_name { | ||
| "header" => Some(false), | ||
| "a" | "area" | "button" | "select" | "textarea" => Some(false), | ||
| "svg" => Some(true), | ||
| "body" | "br" | "details" | "dir" | "frame" | "iframe" | "label" | "mark" | "marquee" | ||
| | "menu" | "meter" | "optgroup" | "pre" | "progress" | "ruby" => Some(true), | ||
| "input" => Some( | ||
| node.find_attribute_by_name("type") | ||
| .and_then(|attr| attr.initializer()) | ||
| .and_then(|init| init.value().ok()) | ||
| .and_then(|val| val.string_value()) | ||
| .is_some_and(|value| value == "hidden"), | ||
| ), |
Member
There was a problem hiding this comment.
Tags in HTML are case insensitive, while in case sensitive in Astro, Svelte & Vue. Let's make use is_html_tag() from utils.rs :)
Netail
reviewed
Apr 17, 2026
| } | ||
|
|
||
| impl Rule for NoNoninteractiveTabindex { | ||
| type Query = Ast<AnyHtmlElement>; |
Member
There was a problem hiding this comment.
Might be good to query on AnyHtmlTagElement instead, limits the scope a bit (OpeningElement & SelfClosingElement)
Member
|
I'll take over the rule |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/biome_html_analyze/src/services/aria.rs (1)
10-19: Nit:Arc<AriaRoles>around a zero-sized type is overkill.
AriaRolesis a ZST (Debug + Default); you could hold it by value and deriveClone/DefaultonAriaServices, skipping the allocation and the service-bag indirection. Perfectly fine as-is though — it mirrors the JS analyzer's pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/services/aria.rs` around lines 10 - 19, AriaRoles is a zero-sized type so wrapping it in Arc is unnecessary; change AriaServices to store AriaRoles by value (replace pub(crate) roles: Arc<AriaRoles> with pub(crate) roles: AriaRoles), update the aria_roles(&self) -> &AriaRoles method to return a reference to the inner value, derive Clone and Default (or implement) for AriaServices so callers can clone without allocation, and remove any Arc imports/usages related to AriaRoles.crates/biome_html_syntax/Cargo.toml (1)
17-22: Tiny ordering nit.Grouping
biome_arianext to the otherbiome_*entries (top of the list) would keep the manifest tidy, but nothing functional here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_syntax/Cargo.toml` around lines 17 - 22, Move the biome_aria dependency line so it's grouped with the other biome_* entries at the top of the dependency list; specifically, locate the existing biome_rowan, biome_string_case (and other biome_* lines) and place the biome_aria = { workspace = true } line adjacent to them to keep the manifest consistently ordered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_html_syntax/src/attr_ext.rs`:
- Around line 24-31: The attribute name accessor returns token.token_text(),
which preserves leading/trailing trivia and causes mismatches; update
AnyHtmlAttribute::name (the match arm for AnyHtmlAttribute::HtmlAttribute that
calls attr.name().ok()?.value_token().ok().map(|token| token.token_text())) to
use the trimmed form of the token text (e.g., token.token_text_trimmed() or the
equivalent text_trimmed helper used elsewhere) so returned TokenText has trivia
removed and matches Element::find_attribute_by_name comparisons.
- Around line 13-20: The trait impl for fn value(&self) in AnyHtmlAttribute uses
as_static_value(), which only extracts HtmlString cases and misses single-text
expressions; change the extraction to use the inherent string_value() path used
by AnyHtmlAttribute::value() (i.e., replace the
attr.initializer()?.value().ok()?.as_static_value() call with the equivalent
call that uses string_value() so both HtmlString and
HtmlAttributeSingleTextExpression are returned), updating the match arm inside
value(&self) for AnyHtmlAttribute::HtmlAttribute(attr) accordingly.
---
Nitpick comments:
In `@crates/biome_html_analyze/src/services/aria.rs`:
- Around line 10-19: AriaRoles is a zero-sized type so wrapping it in Arc is
unnecessary; change AriaServices to store AriaRoles by value (replace pub(crate)
roles: Arc<AriaRoles> with pub(crate) roles: AriaRoles), update the
aria_roles(&self) -> &AriaRoles method to return a reference to the inner value,
derive Clone and Default (or implement) for AriaServices so callers can clone
without allocation, and remove any Arc imports/usages related to AriaRoles.
In `@crates/biome_html_syntax/Cargo.toml`:
- Around line 17-22: Move the biome_aria dependency line so it's grouped with
the other biome_* entries at the top of the dependency list; specifically,
locate the existing biome_rowan, biome_string_case (and other biome_* lines) and
place the biome_aria = { workspace = true } line adjacent to them to keep the
manifest consistently ordered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ac81b4c-de4e-4319-9ec1-4fd482989424
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**
📒 Files selected for processing (10)
.changeset/html-no-noninteractive-tab-index.mdcrates/biome_html_analyze/Cargo.tomlcrates/biome_html_analyze/src/lib.rscrates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rscrates/biome_html_analyze/src/lint/a11y/no_svg_without_title.rscrates/biome_html_analyze/src/services/aria.rscrates/biome_html_analyze/src/services/mod.rscrates/biome_html_syntax/Cargo.tomlcrates/biome_html_syntax/src/attr_ext.rscrates/biome_html_syntax/src/element_ext.rs
✅ Files skipped from review due to trivial changes (3)
- crates/biome_html_analyze/src/services/mod.rs
- crates/biome_html_analyze/Cargo.toml
- .changeset/html-no-noninteractive-tab-index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_html_analyze/src/lint/a11y/no_noninteractive_tabindex.rs
Netail
approved these changes
Apr 17, 2026
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the
noNoninteractiveTabindexa11y rule for HTML files, porting the existing JSX/TSX rule to the HTML analyzer.This rule enforces that
tabindexis not used on non-interactive elements, as it can cause usability issues for keyboard users. The rule allowstabindexon interactive elements (like buttons and links) and permits negativetabindexvalues for managing focus programmatically.Part of #8155
Test Plan
tests/specs/a11y/noNoninteractiveTabindex/invalid.htmlcontains 3 test cases that should trigger the rule:<div tabindex="0">- non-interactive element with positive tabindex<div role="article" tabindex="0">- non-interactive role with positive tabindex<article tabindex="0">- non-interactive semantic element with positive tabindexvalid.htmlcontains 3 valid cases:<div>- no tabindex attribute<button tabindex="0">- interactive element with tabindex<article tabindex="-1">- negative tabindex for programmatic focus managementcargo test -p biome_html_analyze no_noninteractive_tabindexcargo insta reviewDocs
Documentation is included in the rule's rustdoc comments with examples of both invalid and valid cases.