Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The implementation can change an active ripple’s color, double-render customized Android ripples, and break existing style layout semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates Checkbox behavior to align with MD3 interaction, sizing, focus, and accessibility requirements.
Changes:
- Adds MD3 state-layer and custom ripple behavior.
- Expands touch targets and adjusts focus-ring geometry.
- Adds accessibility guidance and comprehensive tests.
File summaries
| File | Description |
|---|---|
src/components/Checkbox/utils.ts |
Resolves interaction colors and opacity. |
src/components/Checkbox/tokens.ts |
Adds sizing and state-layer tokens. |
src/components/Checkbox/Checkbox.tsx |
Implements interaction, focus, sizing, and accessibility behavior. |
src/components/__tests__/Checkbox/utils.test.tsx |
Tests state-layer resolution. |
src/components/__tests__/Checkbox/Checkbox.test.tsx |
Tests interactions, sizing, focus, and accessibility. |
src/components/__tests__/Checkbox/__snapshots__/CheckboxItem.test.tsx.snap |
Updates Checkbox.Item snapshots. |
src/components/__tests__/Checkbox/__snapshots__/Checkbox.test.tsx.snap |
Updates Checkbox snapshots. |
docs/6.x/docs/guides/migration.md |
Documents behavioral and sizing changes. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bfb7a1b to
55f6971
Compare
55f6971 to
8fe46d1
Compare
|
Confirming the direction: we're making the control 48dp rather than adding an invisible tap area, so this approach is the one we want. #5108 will drop its Checkbox hit-slop. |
| }, [isInteractive, pressedSV, rippleHoldSV, rippleAlpha, rippleScale]); | ||
|
|
||
| // `Checkbox.Item` names the row and passes `accessible={false}` here. | ||
| const isInAccessibilityTree = rest.accessible !== false; |
There was a problem hiding this comment.
The new warning fires for checkboxes this repo ships. Opening the Checkbox example screen logs it 18 times in Metro on iOS and Android, because no checkbox in example/src/Examples/CheckboxExample.tsx has an aria-label. example/src/Examples/ListItemExample.tsx also warns for <Checkbox status="checked" /> in List.Item's right, where the checkbox has no press handler and only shows state. The unit tests print it 110 times (108 in Checkbox.test.tsx, 2 in DataTable.test.tsx).
The check also ignores the other ways to hide a view from assistive tech. A checkbox with aria-hidden or importantForAccessibility="no-hide-descendants" still warns.
Please add labels to the example screens and the existing tests, and skip the warning for hidden checkboxes:
| const isInAccessibilityTree = rest.accessible !== false; | |
| const isInAccessibilityTree = | |
| rest.accessible !== false && | |
| rest['aria-hidden'] !== true && | |
| rest.importantForAccessibility !== 'no' && | |
| rest.importantForAccessibility !== 'no-hide-descendants'; |
(via Claude, verify if it makes sense)
| * Margin, padding and transform belong in `style`: here they shift the | ||
| * pressable out from under the focus ring. | ||
| */ | ||
| tapTargetStyle?: StyleProp<ViewStyle>; |
There was a problem hiding this comment.
tapTargetStyle is a new public prop, and its own docs list cases where it breaks the component. Shrinking it clips the 40dp state layer, and margin, padding or transform move the pressable away from the focus ring. It turns the inner pressable into API. MD3 sets the target at 48dp, so a smaller target also works against the goal of this PR.
For a denser layout, a negative margin on style already shrinks the footprint and keeps the 48dp target. Please drop tapTargetStyle, and show that in the "Custom style target" section of the migration guide with a before and after snippet. (via Claude, verify if it makes sense)
| <Animated.View | ||
| pointerEvents="none" | ||
| style={[styles.outline, outlineStyle]} | ||
| testID={testID ? `${testID}-state-layer` : undefined} |
There was a problem hiding this comment.
The -state-layer, -ripple and -focus-ring test IDs exist only so tests can reach internal views. The new tests then assert their exact styles and animated values, copy an internal constant (RIPPLE_START_SCALE in Checkbox.test.tsx), and walk the parent chain to check where the ring sits in the tree. The project test rules ask to avoid IDs on internal wrappers and assertions on internal style values.
These tests fail on any refactor that keeps the same output, for example merging the two layers or moving the ripple to another animation API. Please drop these IDs and keep the tests for public behavior: role and state, press callbacks, disabled handling and the warning. (via Claude, verify if it makes sense)
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
On web, Space does not toggle a focused checkbox. On the Checkbox example screen, Tab to the "unchecked ↔ checked" checkbox and press Space. aria-checked stays false and the browser scrolls the screen instead (the ScrollView moves 43px). Enter toggles it. The WAI-ARIA checkbox pattern uses Space as the toggle key, so keyboard users on web expect it. Checkbox.Item rows have the same problem.
The cause is in react-native-web. Its press responder accepts Space as activation only for role="button" or a <button> element, so role="checkbox" never gets it. This also happens on main, so it is not a regression from this PR. This PR already reworks the press and keyboard focus handling, so it can be the place to handle Space for role="checkbox": prevent the default scroll on key down and toggle on key up. (via Claude, verify if it makes sense)
Doberjohn
left a comment
Doberjohn
left a comment
There was a problem hiding this comment.
@likevy All three of the open requests still stand, and the PR is ready to merge once they're in:
- Accessibility warning: it fires for checkboxes in this repo: the Checkbox example screen, the checkbox in
List.Item'srightin the List example, and about 110 times across the tests. Please add labels to the examples and tests, and skip the warning for hidden checkboxes as in his suggestion (aria-hidden, andimportantForAccessibilityset to"no"or"no-hide-descendants"). tapTargetStyle: please drop it. A prop is easy to add later but can't be removed without a breaking change, and a negative margin onstylealready gives a denser layout while keeping the 48dp target. The "Custom style target" section of the migration guide can show that with a before and after snippet.- Internal test IDs: please drop
-state-layer,-rippleand-focus-ring. 6.0 just stopped deriving internal test IDs fromtestID(#5088, #5099), so these would bring them back. Keep the tests to public behavior: role and state, press callbacks, disabled handling and the warning.
Please also bring the branch up to date with main when you push. I'll approve the CI run as soon as you do.
The state-layer tokens were declared but never read, so hover and focus had no tint of their own. The layer now follows the spec -- `primary` when selected, `onSurface` when not, `error` throughout an error checkbox, `color` and `uncheckedColor` standing in for the role they override on the box -- and fades by opacity alone, so a dynamic theme's `PlatformColor` roles are never interpolated. Press handlers are only attached when something can handle a press, since TouchableRipple keys its own disabled state off that.
MD3 paints the press as a ripple bounded to the 40dp state layer in the inverted role -- `onSurface` when selected, `primary` when not. The platform press could not deliver that: Android's ripple rejects the `PlatformColor` a dynamic theme resolves roles to, iOS and Android only offered the neutral default, and web's hover overlay doubled up with the layer. The checkbox now draws the ripple itself, holds it for a minimum press so a quick tap still reads, and resolves its color once per press so a controlled `status` flip mid-hold cannot recolor it. A caller gets the platform press back only through a prop that platform honors: `rippleColor` anywhere, `background` on Android, `underlayColor` on iOS. `rippleEffectEnabled: false` disables it as everywhere else.
The pressable was sized to the 40dp state layer, 8dp short of the minimum interactive area, with no hitSlop making up the difference. Only the pressable grows; the 40dp layers it centres stay where they were.
The ring sat on the state-layer boundary because the pressable clips overflow to the tap-target shape and would crop the spec's outer offset. Rendered as a sibling of the pressable it takes the spec geometry, which also covers Android P+, where a foreground ripple forces the clip regardless of `borderless`. That wrapper is the component's outer box, so the public `style` moves onto it -- on the pressable it had stopped positioning or transforming the checkbox as a whole.
Moving `style` onto the outer container took away the only way to reach the pressable, which that prop used to document. `tapTargetStyle` reaches it again, named for what it styles rather than `contentStyle`, which elsewhere in the library means a non-interactive content container. Sizing is what it is for. Margin, padding and transform belong in `style`: here they shift the pressable out from under the focus ring.
A standalone checkbox renders no visible label, and `aria-label` reached it only by inheritance, so it appeared in no prop table and nothing flagged one that shipped unnamed. `Checkbox.Item` names the row and is exempt.
A negative margin on `style` already gives a denser layout and keeps the 48dp touch target, so the migration guide shows that instead.
6.0 stopped deriving internal test IDs from `testID`, so the state layer, ripple and focus ring no longer get one. The tests now cover public behavior only: role and state, press callbacks, disabled handling and the accessible name warning. They also name every checkbox, so the warning only prints where a test asserts it.
A standalone checkbox now warns without an accessible name, so the Checkbox and List examples label theirs. The List rows are not pressable, so the checkbox is the only element that carries the checked state and is named after the item it belongs to rather than hidden.
React Native Web presses on Space only for button roles, so Space on a focused checkbox scrolled the page instead of toggling it. Space now toggles a checkbox and a Checkbox.Item row on release, as a native checkbox does. Native platforms and Enter, which React Native Web already handles, are unchanged.
The container had no size of its own, so a parent's default `alignItems: 'stretch'` widened it in a column, which centered the checkbox, and made it as tall as a row. A definite 48dp size keeps the checkbox where it is laid out. A consumer `style` still overrides it.
`importantForAccessibility` hides a view only on Android and `accessibilityElementsHidden` only on iOS, and iOS does not name an element after its `aria-labelledby` target. The warning now counts each of these only on the platform that honors it, so an iOS checkbox that stays exposed or unnamed still warns.
On web, Enter runs press in on key down and press out before the press on release, so it shows the ripple and calls the caller's `onPressIn` and `onPressOut`. Space now does the same, and a blur before the release presses out without pressing. Checkbox.Item rows still only toggle on Space, since the row ripple runs inside TouchableRipple's own press handlers.
Testing Library never dispatches events to a disabled pressable, so the old test passed whether or not the row ignored Space while disabled. The new one disables and re-enables the row between key down and key up, and fails without the guard or the reset.
The resolver shared its name with the theme's `getStateLayer(theme, role, state)` but took different arguments, and `CheckboxStateLayer` repeated the shared `StateLayer` type. It is now `getCheckboxStateLayer`, returns `StateLayer` and resolves its token roles through the shared helper.
The guide covers migrating from 5.x, where the checkbox took 36dp and ignored `style`, so the section now compares against that instead of earlier 6.0 prereleases. It describes the end state of the interaction states, shows the 36dp footprint kept with a negative margin, and adds an accessible name subsection for the new warning.
The usage example is published on the docs page, and without a name it triggers the accessible name warning.
When a pending Space press lost `onPress`, by disabling the checkbox or removing the handler, the press was dropped without pressing out. The caller's `onPressIn` went unmatched and, while the checkbox stayed interactive, the ripple stayed painted. Every way a Space press ends now presses out through one helper, and only a release presses.
Android ignores `accessibilityElementsHidden`, and React Native Web drops it along with `importantForAccessibility`, so a checkbox hidden only by one of them still warns there. The tests fail if a platform check is removed or widened.
The migration guide describes a development warning, but production builds logged it too, including in the browser console on web. It now uses the same `process.env.NODE_ENV` check as the other development-only messages.
8fe46d1 to
c79fe23
Compare
Motivation
Checkboxmissed MD3: presses had a neutral ripple and the target was 40dp. Breaking formain: it takes 48dp andstyletargets a new outer container (see the migration guide).Related issue
Screenshots / Videos
Screen.Recording.2026-09-04.at.16.50.46.mov
Screen.Recording.2026-09-04.at.16.51.11.mov
Screen.Recording.2026-09-04.at.16.51.40.mov
Test plan
Checkbox screen, first checkbox under "Tap to toggle":
primaryunchecked,onSurfacechecked.Checkbox Item screen, web: Space toggles a focused row.