fix(a11y): only use baseConcepts in useSemanticElements rule (#9245)#9311
fix(a11y): only use baseConcepts in useSemanticElements rule (#9245)#9311ruidosujeira wants to merge 3 commits intobiomejs:mainfrom
Conversation
…#9245) The rule previously treated baseConcepts and relatedConcepts identically, causing incorrect suggestions like `<output>` for `role="status"`. Now it only flags roles that have direct semantic HTML equivalents (baseConcepts).
🦋 Changeset detectedLatest commit: aeae6e6 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR corrects the Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 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_js_analyze/tests/specs/a11y/useSemanticElements/valid.jsx`:
- Around line 46-69: The test block labelled “relatedConcepts-only” incorrectly
includes role="navigation" which should be tested as a baseConcept (navigation →
<nav>); remove the <div role="navigation"></div> from this valid JSX fragment
(the fragment containing the list of <div role="checkbox"> ... ) so the
navigation case remains covered by the baseConcept tests, or replace it with a
different relatedConcept-only role if you need to keep the same number of items.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/invalid_self_closing.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/valid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/valid_issue_6384.tsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
.changeset/fix-use-semantic-elements-base-concepts.mdcrates/biome_js_analyze/src/lint/a11y/use_semantic_elements.rscrates/biome_js_analyze/tests/specs/a11y/useSemanticElements/invalid.jsxcrates/biome_js_analyze/tests/specs/a11y/useSemanticElements/invalid_self_closing.jsxcrates/biome_js_analyze/tests/specs/a11y/useSemanticElements/valid.jsxcrates/biome_js_analyze/tests/specs/a11y/useSemanticElements/valid_issue_6384.tsx
💤 Files with no reviewable changes (3)
- crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/valid_issue_6384.tsx
- crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/invalid.jsx
- crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/invalid_self_closing.jsx
crates/biome_js_analyze/tests/specs/a11y/useSemanticElements/valid.jsx
Outdated
Show resolved
Hide resolved
Merging this PR will improve performance by 22.34%
Performance Changes
Comparing 
Footnotes
|
| <div role="checkbox" ></div> | ||
| <div role="radio" ></div> | ||
| <div role="heading" ></div> | ||
| <div role="separator" ></div> |
There was a problem hiding this comment.
Ah see, these feel like they should be flagged by the rule.
We should align ourselves with the source rule's behavior: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/src/rules/prefer-tag-over-role.js
There was a problem hiding this comment.
@dyc3 Do you think the best approach would be to use both baseConcepts and relatedConcepts (aligning with the source rule), but add status to the exclusion list, similar to how combobox, listbox, and option are already excluded?
|
I’m currently reviewing the requested changes and comparing the current implementation against the source rule behavior to make sure we preserve the expected cases while fixing the incorrect suggestion. I’m still evaluating the best approach, but I’m actively working on it and will update the PR soon. |
2 participants
Summary
Fixes #9245
The
useSemanticElementsrule treatedbaseConceptsandrelatedConceptsas equivalent when suggesting HTML element replacements. This caused incorrect suggestions — for example, suggesting<output>for<div role="status">, when<output>is only arelatedConceptof the "status" role.The rule now only considers
baseConceptswhen suggesting semantic HTML elements.Test Plan
Added test case for
<div role="status">to verify it no longer suggests<output>. Existing tests pass — roles with validbaseConcepts(e.g.navigation→<nav>) continue to work as expected.