fix(biome_html_analyze): consolidate a11y helpers and fix aria-hidden behavior#8837
Conversation
|
github-actions
bot
added
A-Linter
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes
|
rahuld109
force-pushed
the
refactor/consolidate-a11y-helpers
branch
from
0c05609 to
7b5ee6a
Compare
Extracts duplicated a11y helper logic into shared utilities in `a11y.rs`: - `is_aria_hidden_true()`: strict aria-hidden="true" check - `get_truthy_aria_hidden_attribute()`: returns attribute if truthy - `has_non_empty_attribute()`: checks non-empty trimmed attribute value - `has_accessible_name()`: checks aria-label or title attributes Added type-specific variants to avoid cloning in recursive functions: - `html_element_has_truthy_aria_hidden()` - `html_self_closing_element_has_truthy_aria_hidden()` - `html_self_closing_element_has_accessible_name()` - `html_self_closing_element_has_non_empty_attribute()` Updated rules to use shared helpers: - use_alt_text - use_anchor_content - use_html_lang - use_iframe_title - no_svg_without_title - no_redundant_alt
rahuld109
force-pushed
the
refactor/consolidate-a11y-helpers
branch
from
7b5ee6a to
c66ce63
Compare
Extract duplicated attribute-checking patterns into core helpers: - is_truthy_aria_hidden_value() - is_strict_true_value() - has_non_empty_value() All public helpers now delegate to these core functions.
rahuld109
force-pushed
the
refactor/consolidate-a11y-helpers
branch
from
114386e to
02bd109
Compare
WalkthroughThis PR adds a centralized ARIA/HTML accessibility helper module at crates/biome_html_analyze/src/a11y.rs (truthy aria-hidden checks, case-insensitive attribute comparisons, non-empty attribute checks, accessible-name detection), exports AnyHtmlElement and HtmlAttribute within the module scope, and introduces several pub(crate) helper functions used across lints. Multiple a11y lint rules were refactored to call these helpers (no_redundant_alt, no_svg_without_title, use_alt_text, use_anchor_content, use_html_lang, use_iframe_title). It adds comprehensive tests at crates/biome_html_analyze/src/a11y_tests.rs and a HtmlAttribute::value() accessor in crates/biome_html_syntax/src/attr_ext.rs. 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
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: 1
🤖 Fix all issues with AI agents
In `@crates/biome_html_analyze/src/a11y.rs`:
- Around line 92-95: The accessible-name checks currently only test "aria-label"
and "title"; update both has_accessible_name and
html_self_closing_element_has_accessible_name to also consider "aria-labelledby"
as a valid source by adding a has_non_empty_attribute(element,
"aria-labelledby") OR into their conditionals so elements with a non-empty
aria-labelledby are treated as having an accessible name.
…tion Add `get_attribute_string_value()` as the single source of truth for extracting attribute values. All other helpers now delegate to this fundamental function, eliminating the remaining duplication. Also adds public `attribute_value_equals_ignore_case()` helper and updates `is_hidden_from_screen_reader()` and `has_type_image_attribute()` to use the shared helpers.
Include `aria-labelledby` as a valid accessible name source per ARIA specs in both `has_accessible_name()` and `html_self_closing_element_has_accessible_name()`.
There was a problem hiding this comment.
I finished my review at mid way of the code. There's a lot of slop in here. Docs mention "yes" and "1" as truth values... or sometimes the implementation doesn't reflect the docs.
Please make sure to improve the quality of the prompts you give to the AI agent, refer to the real docs such as MDN as source of truth, and please add tests!!!
- Move get_attribute_string_value to HtmlAttribute::value() in attr_ext.rs - Rename is_truthy_aria_hidden_value to is_aria_hidden_value_truthy - Fix aria-hidden documentation per MDN spec - Remove AI-generated verbose language from docs - Add unit tests for a11y helper functions
rahuld109
force-pushed
the
refactor/consolidate-a11y-helpers
branch
from
c9e3464 to
7395933
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_html_analyze/src/a11y.rs`:
- Around line 17-28: The function is_aria_hidden_value_truthy currently treats
missing attributes as truthy; change it so None and empty-string values return
false. Use HtmlAttribute::value() and implement map_or(false, |v| { if
v.trim().is_empty() { false } else { !v.eq_ignore_ascii_case("false") } }) so
only non-empty values that are not "false" (including explicit "true" or
unrecognized strings) are treated as truthy.
Empty strings, whitespace-only values, and missing attribute values are now considered falsy for aria-hidden. Only non-empty values that are not "false" (case-insensitive) are truthy. This aligns with jsx-a11y's stricter interpretation of the ARIA spec.
rahuld109
force-pushed
the
refactor/consolidate-a11y-helpers
branch
from
62684be to
208f65f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_html_analyze/src/a11y.rs`:
- Around line 72-83: Replace the case-sensitive check using
element.has_truthy_attribute("aria-hidden") inside is_hidden_from_screen_reader
with a call that locates the "aria-hidden" attribute via
element.find_attribute_by_name("aria-hidden") and passes that attribute to
is_aria_hidden_value_truthy(&attr) so the function uses the same
case-insensitive, trimmed semantics as other helpers; keep the rest of
is_hidden_from_screen_reader (including the input[type=hidden] check that uses
attribute_value_equals_ignore_case and the name_value_token logic) unchanged.
🧹 Nitpick comments (1)
crates/biome_html_analyze/src/a11y_tests.rs (1)
221-226: Test name is slightly misleading.The test is named
multiple_sources_first_wins, buthas_accessible_nameuses boolean OR logic—it doesn't prioritise one source over another. Consider renaming to something likemultiple_sources_any_validormultiple_sources_returns_true.♻️ Suggested rename
#[test] - fn multiple_sources_first_wins() { + fn multiple_sources_returns_true() { let element = parse_first_element(r#"<div aria-label="Label" aria-labelledby="id" title="T"></div>"#); assert!(has_accessible_name(&element)); }
crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/invalid.html.snap
Show resolved
Hide resolved
crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/vue/invalid.vue.snap
Show resolved
Hide resolved
crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/svelte/invalid.svelte.snap
Show resolved
Hide resolved
crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/astro/invalid.astro.snap
Show resolved
Hide resolved
|
Here's another suggestion, the PR description is outdated.
It's not because you fixed some bugs, and the description doesn't reflect that |
rahuld109
changed the title
…n_from_screen_reader Replace has_truthy_attribute with find_attribute_by_name + is_aria_hidden_value_truthy for consistent case-insensitive, trimmed semantics across all aria-hidden checks.
rahuld109
force-pushed
the
refactor/consolidate-a11y-helpers
branch
from
07505b8 to
4002ece
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_html_analyze/src/a11y.rs`:
- Around line 62-86: The tag-name check in is_hidden_from_screen_reader is
case-sensitive and should be made case-insensitive; update the match arm that
currently does `name.text_trimmed() == "input"` to use
`name.text_trimmed().eq_ignore_ascii_case("input")` so variants like `<INPUT>`
are detected; keep the surrounding logic (the subsequent check of the "type"
attribute via attribute_value_equals_ignore_case and the aria-hidden branch)
unchanged.
- Around line 17-28: The doc comment for is_aria_hidden_value_truthy
inaccurately implies strict ARIA spec validation; update it to state that the
function intentionally uses a permissive check (accepts any non-empty value
except case-insensitive "false", e.g., "yes" or "1" are treated truthy) while
noting the spec defines "true"/"false". Modify the comment to describe this
lenient behavior, explain which inputs are considered falsy (missing, empty,
whitespace-only, or "false"), and keep or add the MDN ref for context; reference
the function name is_aria_hidden_value_truthy so reviewers can locate the
change.
| /// Checks if an `aria-hidden` attribute has a truthy value. | ||
| /// | ||
| /// Per ARIA spec, `aria-hidden` accepts only `"true"` or `"false"` as valid values. | ||
| /// Returns `true` only for non-empty values that are not `"false"` (case-insensitive). | ||
| /// Missing values, empty strings, and whitespace-only values are considered falsy. | ||
| /// | ||
| /// Ref: <https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-hidden> | ||
| fn is_aria_hidden_value_truthy(attribute: &HtmlAttribute) -> bool { | ||
| attribute.value().is_some_and(|value| { | ||
| let trimmed = value.trim(); | ||
| !trimmed.is_empty() && !trimmed.eq_ignore_ascii_case("false") | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ARIA aria-hidden attribute specification valid values
💡 Result:
Per the WAI-ARIA specification, aria-hidden accepts these valid values (tokens):
"true"— element is hidden from the accessibility API"false"— element is exposed to the accessibility API as if rendered"undefined"(default) — user agent determines hidden state based on whether it is rendered (same effect as omitting the attribute) [1][2]
Any other value is invalid. [1]
Sources: WAI-ARIA 1.2 spec [1], MDN reference [2].
🏁 Script executed:
# Check the complete function and surrounding context
cd crates/biome_html_analyze/src && cat -n a11y.rs | head -50Repository: biomejs/biome
Length of output: 2689
🏁 Script executed:
# Search for tests related to aria-hidden to understand intended behaviour
rg "aria-hidden" --type rust -C 3Repository: biomejs/biome
Length of output: 50371
Align docs with the lax validation approach.
The implementation intentionally accepts any non-empty value other than "false" (per the tests accepting "yes" and "1"), but the doc comment claims the ARIA spec accepts only "true"/"false". Whilst the spec does define those two values, the function is more permissive. Update the docs to clarify this intentional leniency rather than implying strict spec compliance:
Suggested doc adjustment
-/// Per ARIA spec, `aria-hidden` accepts only `"true"` or `"false"` as valid values.
-/// Returns `true` only for non-empty values that are not `"false"` (case-insensitive).
+/// `aria-hidden` is defined with `"true"`/`"false"` values per ARIA spec.
+/// This helper is permissive: treats any non-empty value other than `"false"` as truthy.🤖 Prompt for AI Agents
In `@crates/biome_html_analyze/src/a11y.rs` around lines 17 - 28, The doc comment
for is_aria_hidden_value_truthy inaccurately implies strict ARIA spec
validation; update it to state that the function intentionally uses a permissive
check (accepts any non-empty value except case-insensitive "false", e.g., "yes"
or "1" are treated truthy) while noting the spec defines "true"/"false". Modify
the comment to describe this lenient behavior, explain which inputs are
considered falsy (missing, empty, whitespace-only, or "false"), and keep or add
the MDN ref for context; reference the function name is_aria_hidden_value_truthy
so reviewers can locate the change.
| /// Returns `true` if the element is hidden from assistive technologies. | ||
| /// | ||
| /// An element is hidden from screen readers when: | ||
| /// - It has a truthy `aria-hidden` attribute | ||
| /// - It is an `<input type="hidden">` element | ||
| /// | ||
| /// Ref: | ||
| /// - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-hidden | ||
| /// - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/hidden | ||
| /// - https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/v6.10.0/src/util/isHiddenFromScreenReader.js | ||
| /// - <https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-hidden> | ||
| /// - <https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/hidden> | ||
| /// - <https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/v6.10.0/src/util/isHiddenFromScreenReader.js> | ||
| pub(crate) fn is_hidden_from_screen_reader(element: &AnyHtmlTagElement) -> bool { | ||
| let is_aria_hidden = element.has_truthy_attribute("aria-hidden"); | ||
| if is_aria_hidden { | ||
| if element | ||
| .find_attribute_by_name("aria-hidden") | ||
| .is_some_and(|attr| is_aria_hidden_value_truthy(&attr)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| match element.name_value_token().ok() { | ||
| Some(name) if name.text_trimmed() == "input" => element | ||
| .find_attribute_by_name("type") | ||
| .and_then(|attribute| attribute.initializer()?.value().ok()?.string_value()) | ||
| .is_some_and(|value| value.text() == "hidden"), | ||
| .is_some_and(|attr| attribute_value_equals_ignore_case(&attr, "hidden")), | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand what name_value_token() returns and how it's used
# Check the AnyHtmlTagElement implementation and name_value_token method
fd -e rs crates/biome_html_analyze | head -20Repository: biomejs/biome
Length of output: 462
🏁 Script executed:
# Search for name_value_token implementation and usage patterns
rg "name_value_token" --type rs -A 3 -B 1Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Look for other tag name comparisons in the a11y.rs file and related files
rg "== [\"']input[\"']|== [\"'].*[\"']" crates/biome_html_analyze/src/ --type rs -B 2 -A 2Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Check how HTML tag names are handled elsewhere in the codebase - look for case-insensitive patterns
rg "eq_ignore_ascii_case|to_lowercase|to_lowercase_ascii" crates/biome_html_analyze/ --type rs -B 1 -A 1Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
# Find Rust files in biome_html_analyze
fd . crates/biome_html_analyze -e rs | head -20Repository: biomejs/biome
Length of output: 1196
🏁 Script executed:
# Search for name_value_token usage and implementation
rg "name_value_token" crates/biome_html_analyze/ -A 3 -B 1Repository: biomejs/biome
Length of output: 485
🏁 Script executed:
# Look for other tag name equality checks
rg '== "input"' crates/biome_html_analyze/ -B 2 -A 2Repository: biomejs/biome
Length of output: 485
🏁 Script executed:
# Check for case-insensitive comparisons in HTML analysis code
rg "eq_ignore_ascii_case|to_lowercase" crates/biome_html_analyze/ -B 1 -A 1Repository: biomejs/biome
Length of output: 8518
🏁 Script executed:
# Search for how other HTML linters/analyzers handle tag names - look at related implementations
rg 'text_trimmed|name.*==' crates/biome_html_analyze/src/a11y.rs -B 2 -A 2Repository: biomejs/biome
Length of output: 295
Make the tag name check case-insensitive to match codebase patterns.
HTML tag names are case-insensitive. Every other HTML tag check in the codebase uses eq_ignore_ascii_case() (e.g. use_anchor_content.rs uses it for "input" checks), but this function uses case-sensitive comparison. Source code preserves the original case, so <INPUT> won't match.
Suggested fix
- Some(name) if name.text_trimmed() == "input" => element
+ Some(name) if name.text_trimmed().eq_ignore_ascii_case("input") => element🤖 Prompt for AI Agents
In `@crates/biome_html_analyze/src/a11y.rs` around lines 62 - 86, The tag-name
check in is_hidden_from_screen_reader is case-sensitive and should be made
case-insensitive; update the match arm that currently does `name.text_trimmed()
== "input"` to use `name.text_trimmed().eq_ignore_ascii_case("input")` so
variants like `<INPUT>` are detected; keep the surrounding logic (the subsequent
check of the "type" attribute via attribute_value_equals_ignore_case and the
aria-hidden branch) unchanged.
4 participants
Summary
Consolidates duplicated accessibility helper functions into a shared
a11y.rsmodule and fixes incorrectaria-hiddenattribute handling.Bug Fix
Fixed
aria-hiddenattribute behavior to match ARIA spec:aria-hidden(no value) → now correctly treated as visible (was incorrectly hidden)aria-hidden=""or whitespace → now correctly treated as visiblearia-hidden="true"→ hidden (unchanged)aria-hidden="false"→ visible (unchanged)Per ARIA spec,
aria-hiddenis not an HTML boolean attribute - it requires explicit"true"or"false"values. Tested in Chrome DevTools accessibility tree to confirm behavior.Refactoring
Following @Netail's suggestion in #8155 (comment), helpers are added to
biome_html_analyze/src/a11y.rs.New method in
biome_html_syntax:HtmlAttribute::value()- extracts value from attribute initializerCore helpers:
is_aria_hidden_value_truthy(),is_strict_true_value(),has_non_empty_value(),attribute_value_equals_ignore_case()Element-level helpers:
is_hidden_from_screen_reader(),is_aria_hidden_true(),get_truthy_aria_hidden_attribute(),has_non_empty_attribute(),has_accessible_name()Type-specific variants: For performance in recursive code
Test plan