Checkbox accessibility#2624
Conversation
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe checkbox component now styles focus through an absolutely positioned native input, adds Enter-key toggling in the component, and updates the checkbox tests to interact with and assert on the nested input. ChangesCheckbox keyboard and focus handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds accessibility attributes (role, aria-checked, and aria-label) to the internal button of the checkbox component and updates the corresponding tests to use a CSS selector. The reviewer identified that these changes introduce accessibility issues, such as duplicate roles and multiple tab stops, which compromise the accessibility tree and break standard Playwright queries. The feedback recommends hiding the internal button from assistive technologies using tabIndex={-1} and aria-hidden="true", and reverting the test to use the standard getByRole query.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| role="checkbox" | ||
| aria-checked={a11yBoolean(this.checked)} | ||
| aria-label={this.label} |
There was a problem hiding this comment.
Adding role="checkbox", aria-checked, and aria-label to the internal <button> element introduces several accessibility issues:
- Duplicate/Nested Roles: The component now exposes multiple elements with
role="checkbox"(the<Host>element, the native<input type="checkbox">, and the<button>). Nested interactive roles are invalid ARIA and cause screen readers to behave unpredictably. - Multiple Tab Stops: Since both the native
<input>and the<button>are focusable by default, keyboard users will encounter two tab stops for a single checkbox. - Broken Accessibility Queries: This change broke the standard Playwright accessibility query
page.getByRole('checkbox', { name: 'Accept Terms' })in the tests, forcing a fallback to a specific CSS selector. This is a strong indicator that the accessibility tree is compromised.
Recommendation
Instead of making the <button> an interactive checkbox, the native <input type="checkbox"> should be the sole focusable and interactive element for assistive technologies. The visual container should be hidden from the accessibility tree using tabIndex={-1} and aria-hidden="true" so that screen readers and keyboard navigation ignore it entirely.
| role="checkbox" | |
| aria-checked={a11yBoolean(this.checked)} | |
| aria-label={this.label} | |
| tabIndex={-1} | |
| aria-hidden="true" |
| const checkbox = page | ||
| .locator('ix-checkbox') | ||
| .locator('button[role="checkbox"]'); |
There was a problem hiding this comment.
Once the duplicate role="checkbox" is removed from the internal <button> element, we can revert this test to use the standard and more robust getByRole query. Testing via accessible roles and names is highly preferred over implementation-specific CSS selectors as it ensures the component remains accessible to assistive technologies.
const checkbox = page.getByRole('checkbox', { name: 'Accept Terms' });
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/checkbox/checkbox.scss`:
- Around line 42-49: The native checkbox in the checkbox.scss selector for
input[type='checkbox'] is still being painted, which can overlap the custom
button. Update the checkbox styling so the input remains focusable for the
&:focus-visible + button rule in checkbox.scss, but is visually hidden and
aligned to the custom control instead of relying on position: absolute alone.
Keep the change scoped to the checkbox input/button styling so the browser’s
native checkbox UI does not render over the custom box.
In `@packages/core/src/components/checkbox/checkbox.tsx`:
- Around line 104-109: The checkbox component is exposing two accessible
checkbox surfaces because `render()` still maps the host element to
`role="checkbox"` and `aria-checked` while keyboard handling now belongs to the
native input. Update `Checkbox.render()` (and any related host attrs) so the
host no longer presents itself as a checkbox, and keep the accessible name/state
on the native input only; verify any helpers like `handleKeyDown` and the input
wiring still drive the checked state correctly.
In `@packages/core/src/components/checkbox/tests/checkbox.ct.ts`:
- Around line 129-132: The checkbox test currently validates toggling via click
only, so it misses the new keyboard path. Update the existing `checkbox.ct.ts`
coverage to explicitly send an Enter key interaction on `ix-checkbox` and assert
the checked state changes through that path, ensuring the regression exercises
`handleKeyDown()` in `checkbox.tsx` rather than only pointer behavior. Keep the
assertion focused on the keyboard-triggered toggle and use the existing
`checkbox` locator so the test remains tied to the component behavior added in
this PR.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a5698859-23b2-41f5-90fd-1e6c4f748edc
📒 Files selected for processing (3)
packages/core/src/components/checkbox/checkbox.scsspackages/core/src/components/checkbox/checkbox.tsxpackages/core/src/components/checkbox/tests/checkbox.ct.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/checkbox/tests/checkbox.ct.ts`:
- Around line 126-131: The checkbox test is no longer validating accessibility
because it switches from an accessible role/name lookup to a direct DOM query.
Update the test in checkbox.ct.ts to use the existing accessibility-tree locator
for the “Accept Terms” checkbox (the role/name query used elsewhere in the
checkbox tests), then perform the Enter key interaction through that locator and
keep the checked assertion on the same accessible target.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 00ee8f23-9dce-4be5-a59e-68ebd5d83d5d
📒 Files selected for processing (3)
packages/core/src/components/checkbox/checkbox.scsspackages/core/src/components/checkbox/checkbox.tsxpackages/core/src/components/checkbox/tests/checkbox.ct.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/checkbox/checkbox.tsx
|
1 participant
💡 What is the current behavior?
The screen reader does not properly announce checkbox state changes (checked/unchecked) when users navigate to the checkbox using the Tab key and interact with it using the Enter or Space key.
Jira-ticket: IX-4335
🆕 What is the new behavior?
The screen reader correctly announces checkbox state changes (checked/unchecked) when the user navigates to the checkbox using the Tab key and activates it with the Enter or Space key.
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit