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
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.
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)
Motivation
Checkboxis one of the MD3 reference components, but a review against the spec found residual deviations: the state-layer tokens were dead code, so every interaction fell back toTouchableRipple's neutral ripple; the touch target was 40dp; the focus ring sat on the state-layer edge; and a standalone checkbox had no accessible-name contract.Things worth mentioning:
Checkbox.kt:ripple(bounded = false, radius = StateLayerSize / 2); material-web_checkbox.scss:md-ripplesized to the state layer).TouchableRipplecan't get there: Android's ripple only accepts a numeric colour, so a dynamic theme'sPlatformColorroles can never tint it; the ripple and iOS highlight fill the whole pressable, now 48dp; and on web its hover overlay doubles up with the layer. So the platform press is turned off (rippleColor: 'transparent', asSwitchandBottomNavigationalready do) and a 40dp ripple is drawn with Reanimated shared values — static colour, onlyopacity/transformanimate, 200 ms grow with a 200 ms minimum hold so a quick tap still reads (ComposeRippleAnimation, material-webMINIMUM_PRESS_MS). An explicitrippleColororunderlayColorhands the press back to the platform;rippleEffectEnabled: falsestill disables it.PlatformColorVisible behaviour changes, both in the migration guide: the checkbox occupies 48dp instead of 40dp, so rows get ~8dp taller; interaction tints follow selection instead of being neutral. A standalone
Checkboxwith no accessible name now warns.Related issue
Checkbox review checklist:
hitSlop); MD3 requires 48dp (the 40dp state layer itself already matches).primary/ unselectedonSurface; pressed: selectedonSurface/ unselectedprimary. The lib applies one colour per selection state across all three interaction states (src/components/Checkbox/tokens.ts:24-25), so hover/focus are correct and pressed is wrong in both directions. Do not simply swap the two constants. Error states already match (errorthroughout).Checkbox.Itemis the labeled wrapper) — document and guarantee the standalone accessible-name path.Test plan
yarn lint,yarn typecheckandyarn testpass — 738 tests, 168 snapshots. Each commit is independently green, so the series bisects cleanly.60 new tests in
Checkbox.test.tsxandutils.test.tsxcover the state-layer colour per interaction (custom colours and the pressed inversion included), the ripple on all threePlatform.OSvalues, the minimum-press hold, the fade-out duration, the scale reset on a second press, reduce motion, therippleColor/underlayColorescape hatches,rippleEffectEnabled: false,PlatformColorroles, the 48×48 target, the 50dp ring geometry and the accessible-name warning. Mutation-tested: removing the inversion, the hold, the scale reset, the fade duration or the custom-colour path each fails its test.Manual, on the Checkbox example screen:
primarywhen unchecked,onSurfacewhen checked. On Android with dynamic colour on it takes the system accent.errorripple. "Custom color (tertiary)" - tertiary ripple, noprimaryhalo.secondaryring with a 2dp gap around the 40dp layer; a mouse click must not light the ring.primary.aria-labelannounces its name and state;Checkbox.Itemannounces once for the row; one without a name logs the warning.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