fix(checkbox): safari keyboard navigation#3328
Merged
rivka-ungar merged 4 commits intomasterfrom Mar 11, 2026
Merged
Conversation
Use visually-hidden pattern (1px+clip) instead of width/height:0, and default tabIndex to 0 so Safari includes the input in keyboard Tab order.
Contributor
Review Summary by QodoFix Checkbox Safari keyboard navigation and focus management
WalkthroughsDescription• Fix Safari keyboard navigation by making wrapper/label the Tab target • Replace hidden input pattern with visually-hidden for proper focus handling • Add Space key handler to toggle checkbox when wrapper receives focus • Update focus styles to work with both wrapper and checkbox focus states Diagramflowchart LR
A["Safari Tab Order Issue"] -->|"Make wrapper Tab target"| B["Set wrapper tabIndex=0"]
A -->|"Remove input from Tab order"| C["Set input tabIndex=-1"]
B -->|"Handle Space key"| D["Add onKeyDown handler"]
D -->|"Toggle checkbox"| E["Call input.click()"]
F["Focus styles"] -->|"Update for wrapper focus"| G["Add wrapper:focus-visible rules"]
F -->|"Update for input focus"| H["Add input:focus-visible rules"]
File Changes1. packages/core/src/components/Checkbox/Checkbox.module.scss
|
Contributor
Code Review by Qodo
1. Label focus lacks semantics
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+221
to
+250
| it("should have tabIndex 0 on the wrapper label by default so Safari includes it in Tab order", () => { | ||
| const { getByTestId } = render(<Checkbox label="Option" data-testid="cb" />); | ||
| const wrapper = getByTestId("cb"); | ||
| expect(wrapper.tabIndex).toBe(0); | ||
| }); | ||
|
|
||
| it("should have tabIndex -1 on the hidden input so it is not a duplicate Tab stop", () => { | ||
| const { getByLabelText } = render(<Checkbox label="Option" />); | ||
| const input = getByLabelText<HTMLInputElement>("Option"); | ||
| expect(input.tabIndex).toBe(-1); | ||
| }); | ||
|
|
||
| it("should respect a custom tabIndex on the wrapper", () => { | ||
| const { getByTestId } = render(<Checkbox label="Option" tabIndex={3} data-testid="cb" />); | ||
| const wrapper = getByTestId("cb"); | ||
| expect(wrapper.tabIndex).toBe(3); | ||
| }); | ||
|
|
||
| it("should have no tabIndex when disabled so it is excluded from Tab order", () => { | ||
| const { getByTestId } = render(<Checkbox label="Option" disabled data-testid="cb" />); | ||
| const wrapper = getByTestId("cb"); | ||
| expect(wrapper.getAttribute("tabindex")).toBeNull(); | ||
| }); | ||
|
|
||
| it("should toggle the checkbox when Space is pressed on the wrapper", () => { | ||
| const onChange = vi.fn(); | ||
| const { getByTestId } = render(<Checkbox label="Option" onChange={onChange} data-testid="cb" />); | ||
| const wrapper = getByTestId("cb"); | ||
| fireEvent.keyDown(wrapper, { key: " " }); | ||
| expect(onChange).toHaveBeenCalledTimes(1); |
Contributor
There was a problem hiding this comment.
1. Ad-hoc data-testid used 📘 Rule violation ✓ Correctness
New Checkbox keyboard navigation tests use a hard-coded data-testid value (cb) instead of the repository’s constants-based test ID pattern. This can reduce consistency and increase maintenance overhead across the test suite.
Agent Prompt
## Issue description
The new Checkbox keyboard navigation tests use a hard-coded `data-testid` value (`cb`) and query it directly, which conflicts with the requirement to use established constants-based test ID patterns.
## Issue Context
The Checkbox component already supports generating its `data-testid` via `getTestId(ComponentDefaultTestId.CHECKBOX, id)` when an `id` is provided and no explicit `data-testid` is passed. Tests should follow that pattern (or use accessible queries where possible).
## Fix Focus Areas
- packages/core/src/components/Checkbox/__tests__/Checkbox.test.tsx[221-250]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+105
to
+114
| // Blur the focused wrapper/label after mouse click so the focus ring doesn't | ||
| // persist after pointer interaction (keyboard focus ring only via :focus-visible). | ||
| const onMouseUpCallback = useCallback((e: React.MouseEvent<HTMLElement>) => { | ||
| const target = e.currentTarget; | ||
| window.requestAnimationFrame(() => { | ||
| window.requestAnimationFrame(() => { | ||
| input.blur(); | ||
| target.blur(); | ||
| }); | ||
| }); | ||
| }, [inputRef]); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
2. Input focus not cleared 🐞 Bug ✓ Correctness
Checkbox.onMouseUpCallback blurs only the wrapper/label, but the component’s visual focus/hover styling is still driven by the hidden input’s focus selectors, so the checkbox can remain in a focused/hovered visual state after pointer interaction. This can leave a persistent focus ring/background until focus moves elsewhere, contradicting the intent of the new blur logic.
Agent Prompt
### Issue description
`onMouseUpCallback` blurs only the wrapper/label, but focus-driven visuals are still controlled by the hidden `<input>` (`.input:focus...`). If the input ends up focused after pointer activation, its focus styles can persist.
### Issue Context
The PR’s intent (per comment) is to remove persistent focus ring after pointer interaction and keep keyboard-only focus indication via `:focus-visible`.
### Fix Focus Areas
- packages/core/src/components/Checkbox/Checkbox.tsx[105-114]
- packages/core/src/components/Checkbox/Checkbox.module.scss[80-96] (verify focus styles align with the updated blur behavior)
### Suggested approach
In `onMouseUpCallback`, after the double `requestAnimationFrame`, check `document.activeElement` and blur `inputRef.current` if it’s focused (and/or blur both wrapper and input safely). Ensure the callback still works for both `separateLabel` and non-separate modes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit e42aa53 |
Comment on lines
+136
to
+144
| const onKeyDownCallback = useCallback( | ||
| (e: React.KeyboardEvent) => { | ||
| if (e.key === " ") { | ||
| e.preventDefault(); | ||
| inputRef.current?.click(); | ||
| } | ||
| }, | ||
| [inputRef] | ||
| ); |
Contributor
There was a problem hiding this comment.
1. Space toggles repeatedly 🐞 Bug ✓ Correctness
Checkbox.tsx triggers a programmatic input click on every Space keydown; holding Space will auto-repeat keydown events and can toggle the checkbox multiple times (multiple onChange calls) for a single long press. The handler is also brittle because it only matches e.key === " ", which can miss Space in some browsers/AT combinations.
Agent Prompt
### Issue description
The Checkbox uses `onKeyDown` to call `inputRef.current?.click()` when Space is pressed. Because `keydown` auto-repeats while Space is held, this can trigger multiple toggles for a single long press. The handler also only matches `e.key === " "`, which can be inconsistent across environments.
### Issue Context
The PR moved the Tab focus target to the wrapper/label and added a Space key handler to preserve keyboard activation.
### Fix Focus Areas
- packages/core/src/components/Checkbox/Checkbox.tsx[136-144]
- packages/core/src/components/Checkbox/Checkbox.tsx[172-180]
- packages/core/src/components/Checkbox/Checkbox.tsx[208-218]
### Implementation notes
- Consider moving activation to `onKeyUp` for Space, or add `if (e.repeat) return;`.
- Prefer `e.code === "Space"` (and/or support legacy `e.key` values) to avoid missing Space in some browsers.
- Add/extend tests to cover `e.repeat: true` or multiple keydown events and ensure only one toggle per press.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 6c5f357 |
Contributor
|
📦 Bundle Size Analysis ✅ No bundle size changes detected. Unchanged Components
📊 Summary:
|
Comment on lines
209
to
217
| // eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions | ||
| <label | ||
| className={cx(styles.wrapper, className)} | ||
| tabIndex={wrapperTabIndex} | ||
| onMouseUp={onMouseUpCallback} | ||
| onKeyDown={onKeyDownCallback} | ||
| data-testid={dataTestId || getTestId(ComponentDefaultTestId.CHECKBOX, id)} | ||
| htmlFor={id} | ||
| onClickCapture={onClickCaptureLabel} |
Contributor
There was a problem hiding this comment.
1. Label focus lacks semantics 🐞 Bug ✓ Correctness
Checkbox moves Tab focus to a <label> (tabIndex=0 by default) and removes the native checkbox input from the Tab order (tabIndex=-1). The focused element has no checkbox semantics (role/aria-checked), so assistive tech won’t announce a checkbox or its checked state for keyboard navigation.
Agent Prompt
### Issue description
Checkbox now places keyboard focus on a `<label>` while the real checkbox `<input>` is removed from the Tab order. The focused element does not expose checkbox semantics (role/state), which breaks assistive-technology announcements for keyboard users.
### Issue Context
- `tabIndex` is moved from the `<input type="checkbox">` to a wrapper/label and Space key is manually handled.
- CSS focus styling is updated to target the wrapper/label.
### Fix Focus Areas
- packages/core/src/components/Checkbox/Checkbox.tsx[136-147]
- packages/core/src/components/Checkbox/Checkbox.tsx[148-189]
- packages/core/src/components/Checkbox/Checkbox.tsx[208-259]
- packages/core/src/components/Checkbox/Checkbox.module.scss[70-114]
- packages/style/src/mixins/_common.scss[9-14]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
166
to
181
| aria-label={finalAriaLabel} | ||
| aria-labelledby={ariaLabelledBy} | ||
| checked={checked} | ||
| tabIndex={tabIndex} | ||
| tabIndex={-1} | ||
| /> | ||
| {/* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */} | ||
| <label | ||
| htmlFor={id} | ||
| className={cx(styles.checkbox, checkboxClassName)} | ||
| data-testid={getTestId(ComponentDefaultTestId.CHECKBOX_CHECKBOX, id)} | ||
| tabIndex={wrapperTabIndex} | ||
| onMouseUp={onMouseUpCallback} | ||
| onKeyDown={onKeyDownCallback} | ||
| onClickCapture={onClickCaptureLabel} | ||
| > | ||
| <Icon |
Contributor
There was a problem hiding this comment.
2. Icon label has no name 🐞 Bug ✓ Correctness
In separateLabel mode, the tabbable element is the icon-only `<label
className={styles.checkbox}>, and its only child is ariaHidden`, so the focused control has no
accessible name. Keyboard/screen-reader users will land on an unnamed focus target.
Agent Prompt
### Issue description
In `separateLabel` mode the focusable element is an icon-only `<label>` whose contents are `ariaHidden`, so it has no accessible name when focused.
### Issue Context
This is introduced by moving tab focus from the hidden input to the label.
### Fix Focus Areas
- packages/core/src/components/Checkbox/Checkbox.tsx[148-203]
- packages/core/src/components/Checkbox/Checkbox.module.scss[96-114]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
talkor
approved these changes
Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
https://monday.monday.com/boards/3532714909/views/113184182/pulses/11477130597
PR Type
Bug fix
Description
Fix Checkbox not focusable via Tab in Safari by using visually-hidden pattern
Move focus ring rendering from input to wrapper label element
Add keyboard Space key handler for wrapper-based checkbox activation
Set wrapper tabIndex to 0 by default, input to -1 to avoid duplicate Tab stops
Diagram Walkthrough
File Walkthrough
Checkbox.module.scss
Update focus styles for Safari keyboard navigationpackages/core/src/components/Checkbox/Checkbox.module.scss
@includefocus-style-css(2px)mixinnon-separate modes
Checkbox.tsx
Implement Safari-compatible keyboard navigation for Checkboxpackages/core/src/components/Checkbox/Checkbox.tsx
iconContainerRefand simplifyonMouseUpCallbackto use eventtarget
onKeyDownCallbackto handle Space key for checkbox activationtabIndexto 0 by default (or custom value), disabled whencomponent is disabled
tabIndexto -1 to prevent duplicate Tab stopsnon-separate label modes
Checkbox.test.tsx
Add keyboard navigation tests for Safari compatibilitypackages/core/src/components/Checkbox/tests/Checkbox.test.tsx