feat(html): port noAriaHiddenOnFocusable a11y rule to HTML#9496
feat(html): port noAriaHiddenOnFocusable a11y rule to HTML#9496ematipico merged 6 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: 309f915 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a new HTML accessibility lint rule Possibly related PRs
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}
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_aria_hidden_on_focusable.rs`:
- Around line 155-164: The check treating all inputs as natively focusable is
causing false positives for <input type="hidden">; update the conditional that
currently uses name_matches("input") (in the focusability check inside
no_aria_hidden_on_focusable.rs) to exclude inputs whose type attribute equals
"hidden" (case-insensitive). Implement the check inline or add a helper like
is_hidden_input(node) that reads the "type" attribute and returns true for
"hidden", and only treat the element as focusable when name_matches("input") &&
!is_hidden_input(node).
- Around line 171-180: has_contenteditable_true currently returns false when the
contenteditable attribute exists without a value or is an empty string; change
the logic to treat an attribute present with no value or an empty value as
truthy per the HTML spec: call element.find_attribute_by_name("contenteditable")
and match on the Option, returning false only when the attribute is None, but
when Some(attr) return true if attr.value() is None OR (attr.value() is Some(v)
and v.trim().eq_ignore_ascii_case("false") is false); update
has_contenteditable_true (and reference AnyHtmlElement/find_attribute_by_name
and attr.value()) accordingly so missing-value and empty-string cases are
detected as true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ff8e9ec-a6c9-43ea-b217-637a1aecbdcb
📒 Files selected for processing (3)
crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rscrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html
aviraldua93
force-pushed
the
feat/html-no-aria-hidden-on-focusable
branch
from
a91f417 to
b57ecff
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs (2)
171-180:⚠️ Potential issue | 🟠 MajorTreat empty/missing
contenteditablevalue as truthy.At Line 175-179,
has_contenteditable_truemisses<div contenteditable ...>and<div contenteditable="" ...>, which should both be treated as editable in HTML. This creates false negatives.Proposed patch
fn has_contenteditable_true(element: &AnyHtmlElement) -> bool { element .find_attribute_by_name("contenteditable") - .is_some_and(|attr| { - attr.value().is_some_and(|value| { - let trimmed = value.trim(); - !trimmed.is_empty() && !trimmed.eq_ignore_ascii_case("false") - }) - }) + .is_some_and(|attr| match attr.value() { + None => true, + Some(value) => { + let trimmed = value.trim(); + trimmed.is_empty() || !trimmed.eq_ignore_ascii_case("false") + } + }) }Per HTML spec, what state does contenteditable have when present without a value, or with an empty string value?🤖 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_aria_hidden_on_focusable.rs` around lines 171 - 180, The function has_contenteditable_true incorrectly treats elements like <div contenteditable> and <div contenteditable=""> as non-editable; update has_contenteditable_true to treat presence of the contenteditable attribute with no value or an empty string as truthy per the HTML spec by returning true when the attribute exists and its value is either missing or empty, and only return false when the attribute value is explicitly "false" (case-insensitive); locate has_contenteditable_true and adjust the attribute value check to consider attr.value() being None or Some("") as true while still treating "false" as false.
155-164:⚠️ Potential issue | 🟠 MajorExclude hidden inputs from native-focusable detection.
Line 157 currently treats all
<input>elements as focusable. That includestype="hidden", which should not trigger this rule, so this can raise false positives. Also add a fixture for<input type="hidden" aria-hidden="true">to lock this down.Proposed patch
- if name_matches("button") - || name_matches("input") - || name_matches("select") + if name_matches("button") + || name_matches("select") || name_matches("textarea") || name_matches("details") || name_matches("summary") { return true; } + + if name_matches("input") { + let is_hidden_input = element + .find_attribute_by_name("type") + .and_then(|attr| attr.value()) + .is_some_and(|value| value.trim().eq_ignore_ascii_case("hidden")); + return !is_hidden_input; + }In the HTML Standard, is <input type="hidden"> part of sequential keyboard focus navigation (tab order)?🤖 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_aria_hidden_on_focusable.rs` around lines 155 - 164, The current native-focusable check treats all inputs as focusable (the name_matches("input") branch) which incorrectly includes <input type="hidden">; update the logic so the input case explicitly excludes inputs where the type attribute is "hidden" (e.g., check the element's "type" attribute or add an is_hidden_input helper and use it in the early-return condition), and add a fixture/test for <input type="hidden" aria-hidden="true"> to verify it no longer triggers the rule; ensure you update the same function containing the name_matches("input") check so hidden inputs are not considered focusable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs`:
- Around line 171-180: The function has_contenteditable_true incorrectly treats
elements like <div contenteditable> and <div contenteditable=""> as
non-editable; update has_contenteditable_true to treat presence of the
contenteditable attribute with no value or an empty string as truthy per the
HTML spec by returning true when the attribute exists and its value is either
missing or empty, and only return false when the attribute value is explicitly
"false" (case-insensitive); locate has_contenteditable_true and adjust the
attribute value check to consider attr.value() being None or Some("") as true
while still treating "false" as false.
- Around line 155-164: The current native-focusable check treats all inputs as
focusable (the name_matches("input") branch) which incorrectly includes <input
type="hidden">; update the logic so the input case explicitly excludes inputs
where the type attribute is "hidden" (e.g., check the element's "type" attribute
or add an is_hidden_input helper and use it in the early-return condition), and
add a fixture/test for <input type="hidden" aria-hidden="true"> to verify it no
longer triggers the rule; ensure you update the same function containing the
name_matches("input") check so hidden inputs are not considered focusable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08de291f-12a3-42f6-85da-cbf9ffc788ea
⛔ Files ignored due to path filters (2)
crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/port-no-aria-hidden-on-focusable-html.mdcrates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rscrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html
✅ Files skipped from review due to trivial changes (1)
- .changeset/port-no-aria-hidden-on-focusable-html.md
aviraldua93
force-pushed
the
feat/html-no-aria-hidden-on-focusable
branch
from
b57ecff to
31fb6dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.html`:
- Around line 9-10: The test snapshot for the noAriaHiddenOnFocusable spec is
out of sync because two new invalid inputs were added (contenteditable elements
with aria-hidden); regenerate and commit the updated snapshot for that fixture:
run the snapshot-update command used by this repo (e.g. the insta snapshot
update flow or the project’s test update command) for the
tests/specs/a11y/noAriaHiddenOnFocusable suite, verify the new diagnostics for
the two added lines are present, and commit the updated invalid.html.snap so the
fixture and expected output match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d1b4aee-6ec5-4d0a-b339-dda8ee5672bc
⛔ Files ignored due to path filters (2)
crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
.changeset/port-no-aria-hidden-on-focusable-html.mdcrates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rscrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.htmlpr-body.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/port-no-aria-hidden-on-focusable-html.md
- crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs
| <div contenteditable aria-hidden="true">editable bare attr</div> | ||
| <div contenteditable="" aria-hidden="true">editable empty string</div> |
There was a problem hiding this comment.
Update the spec snapshot for the new contenteditable cases.
Lines 9–10 add two new invalid inputs, but the existing invalid.html.snap (shown in context) doesn’t include their diagnostics yet. Please regenerate/update the snapshot so the fixture and expected output stay in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.html`
around lines 9 - 10, The test snapshot for the noAriaHiddenOnFocusable spec is
out of sync because two new invalid inputs were added (contenteditable elements
with aria-hidden); regenerate and commit the updated snapshot for that fixture:
run the snapshot-update command used by this repo (e.g. the insta snapshot
update flow or the project’s test update command) for the
tests/specs/a11y/noAriaHiddenOnFocusable suite, verify the new diagnostics for
the two added lines are present, and commit the updated invalid.html.snap so the
fixture and expected output match.
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Thank you, there are a lot of missing tests:
- case sensitivity
- components
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| # Port `noAriaHiddenOnFocusable` a11y lint rule to HTML |
There was a problem hiding this comment.
Refer to our contribution guide on how to write a changeset
There was a problem hiding this comment.
Fixed — changeset now follows the contribution guide format with a description, HTML example snippet, and bumped to minor.
| if let Some(tabindex_attr) = element.find_attribute_by_name("tabindex") { | ||
| if let Some(tabindex_value) = get_tabindex_value(&tabindex_attr) { |
There was a problem hiding this comment.
Use let chain. You didn't use clippy, did you ?
There was a problem hiding this comment.
Fixed — tabindex checking now uses a let-chain (lines 77-79). Ran clippy locally, zero warnings.
| value.trim().parse::<i32>().ok() | ||
| } | ||
|
|
||
| /// Checks if an element is natively focusable or has contenteditable. |
There was a problem hiding this comment.
docstring should explain the business logic of a function. Here, you want to explain when an element is focusable
There was a problem hiding this comment.
Fixed — is_focusable_element docstring now explains exactly when an element is focusable: interactive elements, elements with href, input (except type=hidden), and editing hosts (contenteditable). Returns Some(true)/Some(false)/None semantics documented.
| } | ||
|
|
||
| /// Checks if an element is natively focusable or has contenteditable. | ||
| fn is_focusable_element(element: &AnyHtmlElement, is_html: bool) -> bool { |
There was a problem hiding this comment.
Change the return type to Option<bool> so that you can use the try operator a reduce code branching
There was a problem hiding this comment.
Fixed — is_focusable_element now returns Option and the caller uses ? directly (line 89).
| if is_html { | ||
| element_name.eq_ignore_ascii_case(name) | ||
| } else { | ||
| element_name.text() == name |
There was a problem hiding this comment.
Investigated this. element.name() returns TokenText which is already constructed from token_text_trimmed() internally (see element_ext.rs:17-28). The .text() call on TokenText returns trivia-free text — TokenText doesn't have a text_trimmed() method (only SyntaxToken does). This matches the pattern used by use_button_type.rs, use_alt_text.rs, use_anchor_content.rs, and use_media_caption.rs. The diagnostic range already uses text_trimmed_range() (line 103). Happy to change if you'd prefer a different approach.
Merging this PR will not alter performance
Comparing 
Footnotes
|
aviraldua93
force-pushed
the
feat/html-no-aria-hidden-on-focusable
branch
from
31fb6dc to
1ffe6b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html (1)
13-13: Consider adding an invalidcontenteditablevalue test case.Per the HTML spec, invalid values like
contenteditable="banana"fall back to the Inherit state (not an editing host). Adding a case like<div contenteditable="banana" aria-hidden="true">would document this edge case behaviour.Suggested addition
<div contenteditable="false" aria-hidden="true">not editable</div> +<div contenteditable="banana" aria-hidden="true">invalid value inherits</div> <BUTTON aria-hidden="true" tabindex="-1"></BUTTON>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html` at line 13, Add a test case showing the invalid contenteditable value behavior by including an element like <div contenteditable="banana" aria-hidden="true">not editable</div> in the valid.html tests for noAriaHiddenOnFocusable; this documents that invalid values fall back to Inherit (not an editing host) so aria-hidden on that element is acceptable.
🤖 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_aria_hidden_on_focusable.rs`:
- Around line 165-173: The natively-focusable check (the block using
name_matches for "button", "select", "textarea", "details", "summary") should
also verify the element is not disabled before returning Some(true); update that
logic (e.g., call element.find_attribute_by_name("disabled").is_none()) so
disabled form controls are treated as unfocusable, and add corresponding tests
(e.g., fixtures with <button disabled aria-hidden="true">) to the valid cases to
cover this behavior for both the Rust rule and the JS equivalent.
---
Nitpick comments:
In
`@crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html`:
- Line 13: Add a test case showing the invalid contenteditable value behavior by
including an element like <div contenteditable="banana" aria-hidden="true">not
editable</div> in the valid.html tests for noAriaHiddenOnFocusable; this
documents that invalid values fall back to Inherit (not an editing host) so
aria-hidden on that element is acceptable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6126c960-eb87-415d-b914-53e65ab076e4
📒 Files selected for processing (4)
.changeset/port-no-aria-hidden-on-focusable-html.mdcrates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rscrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html
✅ Files skipped from review due to trivial changes (1)
- .changeset/port-no-aria-hidden-on-focusable-html.md
| // These elements are always natively focusable | ||
| if name_matches("button") | ||
| || name_matches("select") | ||
| || name_matches("textarea") | ||
| || name_matches("details") | ||
| || name_matches("summary") | ||
| { | ||
| return Some(true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the JSX version handles disabled elements
rg -n -A 10 "disabled" crates/biome_js_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# First, let's find the correct paths for both HTML and JS rules
fd -t f "no_aria_hidden_on_focusable" crates/Repository: biomejs/biome
Length of output: 257
🏁 Script executed:
# Let's also check the structure of the analyzers
ls -la crates/ | grep analyzeRepository: biomejs/biome
Length of output: 536
🏁 Script executed:
# Search for how disabled is handled in the HTML rule
rg -n "disabled" crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rsRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Read the HTML rule implementation
wc -l crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rsRepository: biomejs/biome
Length of output: 131
🏁 Script executed:
# Read the JS rule implementation
wc -l crates/biome_js_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rsRepository: biomejs/biome
Length of output: 129
🏁 Script executed:
# Check test specs for HTML rule
fd -t f "no_aria_hidden_on_focusable" crates/biome_html_analyze/tests/Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Read the HTML rule implementation
cat -n crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs | head -100Repository: biomejs/biome
Length of output: 4364
🏁 Script executed:
# Read the JS rule implementation
cat -n crates/biome_js_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rsRepository: biomejs/biome
Length of output: 6722
🏁 Script executed:
# Find test specs with broader search
find crates/biome_html_analyze/tests -type d | grep -i "no_aria\|focusable\|a11y"Repository: biomejs/biome
Length of output: 1914
🏁 Script executed:
# Read the rest of HTML rule to see the is_focusable_element function and lines 165-173
cat -n crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs | tail -n +100Repository: biomejs/biome
Length of output: 5374
🏁 Script executed:
# Check HTML test specs
ls -la crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/Repository: biomejs/biome
Length of output: 289
🏁 Script executed:
# Read HTML test spec files
cat crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.htmlRepository: biomejs/biome
Length of output: 799
🏁 Script executed:
cat crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.htmlRepository: biomejs/biome
Length of output: 738
🏁 Script executed:
# Check if there's a JS test spec for comparison
ls -la crates/biome_js_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/ 2>/dev/null || echo "No JS test spec found"Repository: biomejs/biome
Length of output: 424
🏁 Script executed:
# Read JS test specs
cat crates/biome_js_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.jsxRepository: biomejs/biome
Length of output: 367
🏁 Script executed:
cat crates/biome_js_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.jsxRepository: biomejs/biome
Length of output: 284
🏁 Script executed:
# Search for "disabled" in both test directories
rg -n "disabled" crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/
rg -n "disabled" crates/biome_js_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/Repository: biomejs/biome
Length of output: 39
Both rules should check the disabled attribute for natively focusable elements.
Per the HTML spec, disabled form controls (<button disabled>, <input disabled>, etc.) are not focusable and therefore shouldn't trigger this lint when paired with aria-hidden="true". Currently, neither the HTML nor the JS rule checks the disabled attribute—the HTML rule at lines 165–173 returns Some(true) for buttons/inputs without verifying they're enabled. Test coverage for disabled elements is also missing in both implementations.
Recommend adding a check: element.find_attribute_by_name("disabled").is_none() before marking these elements as focusable, and adding test cases like <button disabled aria-hidden="true"> to the valid fixtures.
🤖 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_aria_hidden_on_focusable.rs`
around lines 165 - 173, The natively-focusable check (the block using
name_matches for "button", "select", "textarea", "details", "summary") should
also verify the element is not disabled before returning Some(true); update that
logic (e.g., call element.find_attribute_by_name("disabled").is_none()) so
disabled form controls are treated as unfocusable, and add corresponding tests
(e.g., fixtures with <button disabled aria-hidden="true">) to the valid cases to
cover this behavior for both the Rust rule and the JS equivalent.
There was a problem hiding this comment.
@coderabbitai Create an issue (task issue type) as reminder to add this check to both rules
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo biomejs/biome \
--title "feat(a11y): add disabled attribute check to noAriaHiddenOnFocusable rule (HTML and JS)" \
--body "## Task
Add a check for the \`disabled\` attribute in the \`noAriaHiddenOnFocusable\` lint rule for both the HTML and JS (JSX) implementations.
## Background
Per the HTML spec, disabled form controls (\`<button disabled>\`, \`<input disabled>\`, \`<select disabled>\`, \`<textarea disabled>\`) are not focusable. Currently, both rules do not account for this and will incorrectly flag \`aria-hidden=\"true\"\` on disabled elements as a violation.
## Changes needed
### HTML rule
File: \`crates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs\`
In the \`is_focusable_element\` function, update the natively-focusable check to also verify the element is not disabled before returning \`Some(true)\`. For example:
- Add \`element.find_attribute_by_name(\"disabled\").is_none()\` before marking form controls (button, input, select, textarea) as focusable.
### JS rule
File: \`crates/biome_js_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs\`
Apply the equivalent fix for JSX elements.
### Test fixtures
Add valid test cases for both HTML and JSX, such as:
- \`<button disabled aria-hidden=\"true\"></button>\`
- \`<input disabled aria-hidden=\"true\" />\`
- \`<select disabled aria-hidden=\"true\"></select>\`
- \`<textarea disabled aria-hidden=\"true\"></textarea>\`
## References
- PR: https://github.com/biomejs/biome/pull/9496
- Review comment: https://github.com/biomejs/biome/pull/9496#discussion_r2985561761
- Requested by: `@ematipico`" \
--assignee ematipico
Length of output: 44
@ematipico, done! I've created the tracking issue here: #9712 — assigned to you, with full context linking back to this PR and review comment.
🧠 Learnings used
Learnt from: ematipico
Repo: biomejs/biome PR: 9369
File: crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/vue/valid.vue:20-22
Timestamp: 2026-03-08T08:18:33.839Z
Learning: In `crates/biome_html_analyze/src/lint/a11y/use_anchor_content.rs` (biomejs/biome), custom components (PascalCase elements) inside `<a>` tags are intentionally treated as potentially accessible content to avoid false positives. The linter cannot statically determine what a custom component renders, so it must not emit a diagnostic for anchors whose only children are custom components. The `valid` fixture files for non-Astro frameworks (e.g., vue/valid.vue, svelte/valid.svelte) correctly include cases like `<a><Image alt="description" /></a>` and `<a><CustomIcon /></a>` as valid to document this behavior.
Learnt from: ematipico
Repo: biomejs/biome PR: 8537
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:167-210
Timestamp: 2025-12-22T09:26:56.943Z
Learning: When defining lint rules (declare_lint_rule!), only specify fix_kind if the rule implements an action(...) function. Rules that only emit diagnostics without a code fix should omit fix_kind. This applies to all Rust lint rule definitions under crates/.../src/lint (e.g., crates/biome_js_analyze/src/lint/...).
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-03-21T17:22:03.949Z
Learning: Applies to crates/biome_analyze/**/lint/**/*.rs : Use the 'issue_number' field in the 'declare_lint_rule!' macro to mark incomplete rules as work in progress, which adds a footnote link to the GitHub issue
Learnt from: dyc3
Repo: biomejs/biome PR: 8639
File: crates/biome_js_analyze/src/lint/nursery/no_excessive_lines_per_file.rs:101-108
Timestamp: 2025-12-31T15:35:41.261Z
Learning: In crates/biome_analyze/**/*analyze/src/lint/nursery/**/*.rs, the `issue_number` field in `declare_lint_rule!` macro is optional and the vast majority of nursery rules do not need it. Do not recommend adding `issue_number` unless there's a specific reason.
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-03-21T17:22:03.949Z
Learning: Applies to crates/biome_analyze/**/lint/**/*.rs : Rules with 'recommended: true' and specific domains are only enabled when users enable those domains, while recommended rules without domains are always enabled
Learnt from: dyc3
Repo: biomejs/biome PR: 9617
File: crates/biome_html_analyze/src/lint/a11y/use_aria_activedescendant_with_tabindex.rs:58-65
Timestamp: 2026-03-25T13:49:08.724Z
Learning: In Biome’s `biome_html_analyze` HTML element-name checks inside lint rules, the casing of matches must depend on the input file type:
- For `.html` files, compare element names case-insensitively.
- For `.astro`, `.vue`, and `.svelte` files, compare case-sensitively.
Reason: in template languages (Astro/Vue/Svelte), PascalCase typically denotes custom components, while lowercase names denote native HTML elements. Case-sensitive matching is required to avoid misclassifying components as native elements (and vice versa).
Learnt from: ematipico
Repo: biomejs/biome PR: 9416
File: crates/biome_service/src/file_handlers/html.rs:850-868
Timestamp: 2026-03-09T15:54:24.948Z
Learning: In `crates/biome_service/src/file_handlers/html.rs` (biomejs/biome), Svelte `on:` event directives (e.g., `on:click={...}`) are old Svelte 3/4 syntax and are intentionally not supported by Biome. Svelte 5 runes mode uses regular attributes for event handlers instead. Do not flag missing handling of Svelte `on:` directives as an issue.
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-03-21T17:22:03.949Z
Learning: Applies to crates/biome_analyze/**/lint/**/*.rs : Add a 'deprecated' field to 'declare_lint_rule!' macro when deprecating a rule to communicate the reason for deprecation (e.g., suggesting an alternative rule)
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-03-21T17:22:03.949Z
Learning: Applies to crates/biome_analyze/**/lint/**/*.rs : Set rule severity to 'error' for rules in 'correctness', 'security', and 'a11y' groups; use 'warn' or 'info' for other groups based on rule type
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-03-21T17:22:03.949Z
Learning: Applies to crates/biome_analyze/**/lint/**/*.rs : Add 'fix_kind: FixKind::Safe' or 'fix_kind: FixKind::Unsafe' to the 'declare_lint_rule!' macro when implementing the 'action' function
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2026-03-21T17:22:03.949Z
Learning: Applies to crates/biome_analyze/**/lint/**/*.rs : Document rule options in a '## Options' section with h3 headers for each option, describing what it does, its default value, and providing code examples
Learnt from: dyc3
Repo: biomejs/biome PR: 8901
File: crates/biome_analyze_macros/src/group_macro.rs:9-9
Timestamp: 2026-01-29T22:10:30.888Z
Learning: Do not import xtask inside biome crates. In the biomeps repository, ensure that biome crates (e.g., crates/biome_*/...) do not have imports like use xtask_glue::... or other xtask-related crates. The correct dependency direction is for xtask to depend on and import biome crates, not the reverse, since xtask contains heavier utilities (e.g., git2) that can inflate build times when included in biome crates. When reviewing, verify dependency declarations (Cargo.toml) and any use statements to confirm xtask crates are not pulled into biome crates; prefer having xtask depend on biome crates and export needed functionality through stable interfaces.
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Still comments not addressed. Tests with Vue/astro/svelte with components too missing
aviraldua93
force-pushed
the
feat/html-no-aria-hidden-on-focusable
branch
from
1ffe6b4 to
405e77c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html (1)
1-17: Good test coverage for valid (non-diagnostic) cases.The fixture covers key scenarios: aria-hidden on generic elements, negative tabindex,
type="hidden"inputs,contenteditable="false", anchors without href, uppercase tags, and custom components.Consider adding a test for disabled form controls once that check is implemented:
<button disabled aria-hidden="true">disabled button</button> <input disabled aria-hidden="true" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html` around lines 1 - 17, Add coverage for disabled form controls by extending the valid.html fixture to include a disabled button and a disabled input that both have aria-hidden="true" so the test asserts no diagnostics for disabled controls; locate the existing examples with BUTTON/BUTTON, <input> and <button> entries (e.g., the BUTTON aria-hidden cases and input type="hidden" case) and add corresponding disabled cases (a disabled button and a disabled input) adjacent to them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.html`:
- Around line 1-17: Add coverage for disabled form controls by extending the
valid.html fixture to include a disabled button and a disabled input that both
have aria-hidden="true" so the test asserts no diagnostics for disabled
controls; locate the existing examples with BUTTON/BUTTON, <input> and <button>
entries (e.g., the BUTTON aria-hidden cases and input type="hidden" case) and
add corresponding disabled cases (a disabled button and a disabled input)
adjacent to them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf809a62-d336-446d-909d-8947fcdd569e
📒 Files selected for processing (10)
.changeset/port-no-aria-hidden-on-focusable-html.mdcrates/biome_html_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rscrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/astro/invalid.astrocrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/astro/valid.astrocrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/svelte/invalid.sveltecrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/svelte/valid.sveltecrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/valid.htmlcrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/vue/valid.vue
✅ Files skipped from review due to trivial changes (7)
- .changeset/port-no-aria-hidden-on-focusable-html.md
- crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/vue/valid.vue
- crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/astro/valid.astro
- crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/vue/invalid.vue
- crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/svelte/valid.svelte
- crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/astro/invalid.astro
- crates/biome_html_analyze/tests/specs/a11y/noAriaHiddenOnFocusable/svelte/invalid.svelte
aviraldua93
force-pushed
the
feat/html-no-aria-hidden-on-focusable
branch
from
405e77c to
49252ba
Compare
|
Thanks for the patience @ematipico — all feedback addressed in the latest push:
Replied to each inline comment individually as well. |
|
I think you are using some kind of LLMs to generate code and/or making replies. We require disclosure of any AI/LLMs assistance in the PR description |
aviraldua93
force-pushed
the
feat/html-no-aria-hidden-on-focusable
branch
from
49252ba to
31d4e19
Compare
Port the noAriaHiddenOnFocusable lint rule from JSX to HTML. The rule enforces that aria-hidden=true is not set on focusable elements. Addresses all review feedback: - Use let-chains for tabindex checking - Return Option<bool> from is_focusable_element with ? operator - Business-logic doc comments on all helper functions - Exclude input type=hidden from focusability (HTML spec) - Tighten contenteditable to HTML spec enumerated states: only true/empty/plaintext-only are editing hosts - Case sensitivity tests (BUTTON, INPUT uppercase) - Component tests (MyButton - not flagged) - Framework test coverage: Vue, Svelte, Astro (valid + invalid) - Changeset bumped to minor with example snippet - All snapshots generated via cargo insta - Clippy clean, all 8 tests pass Part of biomejs#8155. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
aviraldua93
force-pushed
the
feat/html-no-aria-hidden-on-focusable
branch
from
31d4e19 to
c183dca
Compare
github-actions
bot
added
the
L-JavaScript
3 participants
Summary
Ports the
noAriaHiddenOnFocusablelint rule from JSX to HTML, as part of the umbrella issue #8155.Enforces that
aria-hidden="true"is not set on focusable elements. A focusable element witharia-hiddencan be reached by keyboard but is invisible to screen readers, causing confusion.What counts as focusable:
tabindex(e.g.,tabindex="0")<button>,<input>,<select>,<textarea>,<details>,<summary>,<a href="...">,<area href="...">contenteditable="true",contenteditable="", orcontenteditable="plaintext-only"(editing hosts per HTML spec §6.8.1)<input>except<input type="hidden">which is not focusableExemptions:
aria-hidden="true"on non-focusable elements (div, span, etc.) is validtabindex="-1"witharia-hidden="true"is valid (intentionally removed from tab order)Fix: Provides an unsafe fix that removes the
aria-hiddenattribute.Closes part of #8155.
Test Plan
valid.html/invalid.html: native focusable elements, tabindex handling,<input type="hidden">,<a>without href,<area>without href,contenteditablestates, custom components (<MyButton>), uppercase tags (<BUTTON>,<INPUT>)vue/valid.vue/vue/invalid.vue: Vue framework coverage with custom componentssvelte/valid.svelte/svelte/invalid.svelte: Svelte framework coverage with custom componentsastro/valid.astro/astro/invalid.astro: Astro framework coverage with custom componentscargo insta, clippy clean (zero warnings)AI Disclosure
This PR was developed with assistance from GitHub Copilot (Claude). The implementation, review comment responses, and test cases were reviewed and verified by the author, including running
clippyandcargo testlocally.