feat(html/a11y): add noRedundantRoles rule for HTML#9276
feat(html/a11y): add noRedundantRoles rule for HTML#9276IxxyDev wants to merge 1 commit intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 183ad77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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
WalkthroughThis PR introduces a new HTML 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/some-trams-cut.md:
- Around line 1-3: The changeset currently marks the package update as a patch;
change the entry for "@biomejs/biome": patch to use a minor release instead
(i.e. "@biomejs/biome": minor) so the new HTML support is recorded as a
feature-level changeset; update the YAML header value accordingly in the
.changeset file.
In `@crates/biome_html_analyze/src/lint/a11y/no_redundant_roles.rs`:
- Around line 89-90: Update the diagnostic message in no_redundant_roles.rs so
the user-facing sentence reads "implied by its semantics" instead of "implied by
its semantic"; locate the message string (the literal containing "Using the role
attribute '{role_attribute}' on the '{element_name}' element is redundant,
because it is implied by its semantic.") and replace "semantic" with "semantics"
in that string so the diagnostic copy is grammatically correct.
- Around line 69-71: The fixer currently reads role_attribute_value and calls
AriaRole::from_roles on the whole string but then removes the entire role
attribute if the first parsed role is redundant; instead split the
role_attribute_value by whitespace into tokens, map/parse each token via
AriaRole::from_roles (or the appropriate single-token parse), determine which
tokens are redundant for the element (reuse the existing redundancy check
logic), and then: if all tokens are redundant remove the role attribute,
otherwise reconstruct the role attribute value from only the non-redundant
tokens and update the attribute (preserving remaining fallback roles). Update
the logic around role_attribute.initializer()?value().value().string_value()?
and AriaRole::from_roles usage to operate on each token rather than assuming a
single role.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalidHtmlAamRoleGeneric.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/valid.html.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (8)
.changeset/some-trams-cut.mdcrates/biome_html_analyze/Cargo.tomlcrates/biome_html_analyze/src/lint/a11y/no_redundant_roles.rscrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalidHtmlAamRoleGeneric.htmlcrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/valid.htmlcrates/biome_html_syntax/Cargo.tomlcrates/biome_html_syntax/src/element_ext.rs
| --- | ||
| "@biomejs/biome": patch | ||
| --- |
There was a problem hiding this comment.
Use minor for this feature changeset.
Given this introduces new HTML support, patch looks under-scoped here.
Suggested fix
---
-"@biomejs/biome": patch
+"@biomejs/biome": minor
---As per coding guidelines: “Use change types in changesets: patch for bug fixes and non-breaking changes (targets main branch), minor for new features and non-breaking additions (targets next branch), major for breaking changes (targets next branch)”.
📝 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.
| --- | |
| "@biomejs/biome": patch | |
| --- | |
| --- | |
| "@biomejs/biome": minor | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/some-trams-cut.md around lines 1 - 3, The changeset currently
marks the package update as a patch; change the entry for "@biomejs/biome":
patch to use a minor release instead (i.e. "@biomejs/biome": minor) so the new
HTML support is recorded as a feature-level changeset; update the YAML header
value accordingly in the .changeset file.
| let role_attribute_value = role_attribute.initializer()?.value().ok()?.string_value()?; | ||
| let explicit_role = AriaRole::from_roles(role_attribute_value.trim())?; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate AriaRole::from_roles implementation =="
rg -nP 'fn\s+from_roles\s*\(' --type rust -C3
echo
echo "== Inspect no_redundant_roles usage of parsed role + fixer =="
rg -nP 'from_roles|get_implicit_role|remove_node' crates/biome_html_analyze/src/lint/a11y/no_redundant_roles.rs -C4
echo
echo "== Check fixture coverage for multi-token or case-variant role values =="
rg -nP 'role="[^"]+\s+[^"]+"|role="[A-Z][^"]*"' crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles -C2Repository: biomejs/biome
Length of output: 1834
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== List fixture files for noRedundantRoles =="
find crates/biome_html_analyze/tests/specs/a11y -name "*noRedundantRoles*" -type f | head -20
echo
echo "== Search fixture content broadly for role attributes =="
find crates/biome_html_analyze/tests/specs/a11y -name "*oRedundantRoles*" -type f -exec cat {} \; | grep -i 'role=' | head -20
echo
echo "== Check AriaRole documentation and parse method =="
rg -nP 'pub fn from_roles|pub fn parse|impl.*Parse' crates/biome_aria_metadata/src/lib.rs -C6Repository: biomejs/biome
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find test files broadly =="
find crates/biome_html_analyze/tests -type f -name "*.html" -o -name "*.json" | grep -i "redundant\|role" | head -20
echo
echo "== Check the full no_redundant_roles.rs file structure =="
wc -l crates/biome_html_analyze/src/lint/a11y/no_redundant_roles.rs
head -n 120 crates/biome_html_analyze/src/lint/a11y/no_redundant_roles.rs
echo
echo "== Check AriaRole full implementation =="
wc -l crates/biome_aria_metadata/src/lib.rs
rg -nP 'pub fn from_roles|pub fn|impl.*FromStr' crates/biome_aria_metadata/src/lib.rs -A5 -B2 | head -80Repository: biomejs/biome
Length of output: 7446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check noRedundantRoles fixtures content =="
cat crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.html
echo ""
echo "---"
echo ""
cat crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/valid.html
echo ""
echo "== Search all fixture files for space-separated role values =="
rg 'role="[^"]*\s+[^"]*"' crates/biome_html_analyze/tests/specs/a11y/ -t htmlRepository: biomejs/biome
Length of output: 1718
Verify multi-token role handling before shipping.
The fixer removes the entire role attribute (line 96) but only checks the first parsed role (line 70). If a multi-token role like role="button presentation" appears on a <button>, the code detects button as redundant and discards the entire attribute—losing the presentation fallback. The ARIA spec allows space-separated fallback roles, yet test fixtures for noRedundantRoles don't cover this case (though useValidAriaRole fixtures show multi-token roles are used elsewhere in the codebase).
Consider handling token lists properly: either parse all tokens and verify none are redundant, or preserve non-redundant fallback roles.
🤖 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_redundant_roles.rs` around lines
69 - 71, The fixer currently reads role_attribute_value and calls
AriaRole::from_roles on the whole string but then removes the entire role
attribute if the first parsed role is redundant; instead split the
role_attribute_value by whitespace into tokens, map/parse each token via
AriaRole::from_roles (or the appropriate single-token parse), determine which
tokens are redundant for the element (reuse the existing redundancy check
logic), and then: if all tokens are redundant remove the role attribute,
otherwise reconstruct the role attribute value from only the non-redundant
tokens and update the attribute (preserving remaining fallback roles). Update
the logic around role_attribute.initializer()?value().value().string_value()?
and AriaRole::from_roles usage to operate on each token rather than assuming a
single role.
| "Using the role attribute '"{role_attribute}"' on the '"{element_name}"' element is redundant, because it is implied by its semantic." | ||
| }, |
There was a problem hiding this comment.
Polish the diagnostic copy.
Line 89 says “implied by its semantic”; “implied by its semantics” reads cleaner in user-facing output.
Suggested fix
- "Using the role attribute '"{role_attribute}"' on the '"{element_name}"' element is redundant, because it is implied by its semantic."
+ "Using the role attribute '"{role_attribute}"' on the '"{element_name}"' element is redundant, because it is implied by its semantics."📝 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.
| "Using the role attribute '"{role_attribute}"' on the '"{element_name}"' element is redundant, because it is implied by its semantic." | |
| }, | |
| "Using the role attribute '"{role_attribute}"' on the '"{element_name}"' element is redundant, because it is implied by its semantics." | |
| }, |
🤖 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_redundant_roles.rs` around lines
89 - 90, Update the diagnostic message in no_redundant_roles.rs so the
user-facing sentence reads "implied by its semantics" instead of "implied by its
semantic"; locate the message string (the literal containing "Using the role
attribute '{role_attribute}' on the '{element_name}' element is redundant,
because it is implied by its semantic.") and replace "semantic" with "semantics"
in that string so the diagnostic copy is grammatically correct.
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
- This needs to be based on
next - needs to have a
minorchangeset
| version: "next", | ||
| name: "noRedundantRoles", | ||
| language: "html", | ||
| sources: &[RuleSource::EslintJsxA11y("no-redundant-roles").same()], |
There was a problem hiding this comment.
is there a rule like this in https://html-eslint.org/ ? if so, add it as a rule source.
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
...and it needs to restore the original PR template |
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
The lint rule needs more work and harness:
- it doesn't check for HTML elements VS components
- there aren't tests for Vue/svelte/astro
- components will hit this rule, which is a bug
3 participants
Summary
noRedundantRoleslint rule.biome_aria::Elementandbiome_aria::Attributetraits for HTML elements, enabling ARIA role detection.roleattributes on HTML elements (e.g.,<button role="button">) and provides an unsafe fix to remove them.Test plan
cargo test -p biome_html_analyze -- no_redundant_roles— 3 tests pass (invalid, valid, HTML-AAM generic role)cargo formatpassescargo lintpassescargo run -p xtask_codegen -- analyzerruns without errors