Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The untyped fallback remains exposed as enabled, and key interaction paths lack regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Aligns Switch with MD3 visuals, accessibility, interaction, and explicit operability requirements.
Changes:
- Adds focus styling, immediate press feedback, and a 48dp touch target.
- Introduces read-only and operability APIs with migration guidance.
- Updates examples, tests, snapshots, colors, and generated documentation.
File summaries
| File | Description |
|---|---|
src/index.tsx |
Exports narrowed switch props. |
src/components/Switch/utils.ts |
Adds focus handle colors. |
src/components/Switch/tokens.ts |
Updates MD3 sizes and colors. |
src/components/Switch/Switch.tsx |
Implements focus, operability, and layout changes. |
src/components/__tests__/Switch.test.tsx |
Tests accessibility and focus behavior. |
src/components/__tests__/__snapshots__/Switch.test.tsx.snap |
Updates switch snapshots. |
example/src/Examples/TextInputExample.tsx |
Marks row-owned switch read-only. |
example/src/Examples/SwitchExample.tsx |
Adds a read-only example. |
example/src/Examples/FABExample.tsx |
Marks row-owned switch read-only. |
example/src/DrawerItems.tsx |
Marks preference switches read-only. |
docs/src/data/componentDocs6x.json |
Regenerates switch API documentation. |
docs/6.x/docs/guides/migration.md |
Documents breaking migration steps. |
docs/6.x/docs/components/Switch/Switch.mdx |
Updates theming and props documentation. |
Review details
- Files reviewed: 13/13 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.
6f690ed to
ae1a012
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The custom-theme example regresses icon coloring, and central timing and read-only guarantees need effective tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The read-only implementation relies on an ARIA attribute unsupported by the switch role.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/components/Switch/Switch.tsx:437
aria-readonlyis not supported forrole="switch"in ARIA, so browsers and assistive technologies may ignore it and expose this non-operable element like a normal switch. ARIA has no standard read-only state for switches; please use valid static/status semantics, oraria-disabledif disabled semantics are acceptable, instead of relying on this attribute.
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| </Pressable> | ||
|
|
||
| <Animated.View | ||
| testID={testID ? `${testID}-state-layer` : undefined} |
There was a problem hiding this comment.
#5099 removed every ${testID}-... from src/components
| <Pressable | ||
| disabled={disabled} | ||
| focusable={isInteractive} | ||
| aria-readonly={isAnnouncedReadOnly} |
There was a problem hiding this comment.
Have you tried the read-only switch with VoiceOver or TalkBack? I'm afraid RN has no aria-readonly so on native it still announces as operable
BREAKING CHANGE: `Switch` now requires `onValueChange`, `readOnly`, or `disabled`, and reserves the 48dp minimum touch target (40dp before). See the 6.x migration guide.
The handler branch of the union only accepted `readOnly={false}`, so a switch
with a stable handler could not become read-only from state, even though the
runtime already gives `readOnly` precedence and `disabled` allowed exactly that
pattern. The branch that requires `disabled` was equally strict.
A handler still guarantees operability whenever read-only is false, and a
switch with neither a handler nor a statically-true `readOnly`/`disabled` is
still rejected.
Interaction handlers are withheld while a switch is disabled, read-only, or missing a handler, so one that was hovered, pressed, or focused at the moment it flipped never received the matching hover-out, press-out, or blur. The shared value stayed at 1 and the switch kept painting that state -- a disabled switch could show a focus ring, and the stale value was still there if it became interactive again.
The read-only test covered focusability and checked state but never the attribute the read-only contract exists for on web, so dropping it left the test green.
The runtime fallback for an untyped `<Switch value />` withheld the interaction
handlers but still emitted `aria-readonly={false}` alongside
`aria-disabled={false}`, so on web it was still exposed as an enabled switch
rather than the state indicator the guard renders.
Read-only is now derived from the rendered state -- non-operable but not
disabled -- which covers both an explicit `readOnly` and the fallback, while
`aria-disabled` keeps carrying the disabled case on its own.
The focus tests only fired a generic focus event on the default native platform, where `isKeyboardFocusEvent` always returns true, so nothing covered the `:focus-visible` gating that keeps a mouse click from lighting the ring. Adds both web paths, driving `currentTarget.matches` directly.
Removing the 100ms delay was a user-visible fix with no test behind it, so the delay could have come back unnoticed. Asserting the size needs a testID on the view that carries it: `-handle` now names the view holding the handle's size and position, and the tinted fill inside it becomes `-handle-fill`.
Moving the selected icon to `onPrimaryContainer` left the tertiary-theme example with a mixed palette: it overrides `primary`, `onPrimary` and `primaryContainer`, and a partial theme falls back to the app theme for omitted roles, so the icon stayed in the default palette. Documents the role change in the migration guide too, since any partial `Switch` theme in consumer code has the same gap.
The read-only tests checked the exposed metadata but never the guarantee it stands for. The props type deliberately permits `readOnly` alongside `onValueChange`, so read-only has to win at runtime -- and nothing would have caught the handler being attached anyway.
The press test flushed all timers before asserting, so restoring the old `withDelay(100, ...)` snap still reached the pressed size and the test passed. It verified the end state, not the immediacy it was written for. Now asserts the handle has already started growing two frames in, well inside the delay the old implementation waited out, and keeps a separate case for the settled sizes on press and release.
Each test that overrode `Platform.OS` or stubbed `console.warn` restored it on the last line, which never runs when an assertion throws. A single failing test left every later test in the file running as web, turning one real failure into a cascade of misleading ones. Matches the teardown FABExtended and Surface already use.
5532668 to
ec53e24
Compare
|
Found potential problems with the pull request:
|
|
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 Switch hit-slop. |
There was a problem hiding this comment.
Review by Claude
The PR description says a read-only switch "is not reported as disabled" and that a screen reader "announces its on/off state, not disabled". After the last commit, iOS and Android expose a read-only switch as disabled, as the migration guide says. Please update the description so it matches. (via Claude, verify if it makes sense)
| * parameter below stays annotated as the flat `Props`, and callers still get | ||
| * the narrowed type through this one. | ||
| */ | ||
| export type OperableProps = SwitchBaseProps & |
There was a problem hiding this comment.
This union rejects props that are valid at runtime, because TypeScript cannot narrow a boolean to the true members. I checked these against SwitchProps with tsc:
// TS2322, but there is always a handler or `disabled` at runtime
<Switch value={v} onValueChange={canEdit ? setV : undefined} disabled={!canEdit} />
// TS2322 for a wrapper that uses Omit, because Omit collapses the union
type MySwitchProps = Omit<SwitchProps, 'theme'>;
const MySwitch = (props: MySwitchProps) => <Switch {...props} />;The wrapper case is common in apps, so this breaks more code than the migration guide shows. It also needs a second props type and a type annotation on the component to keep the docs generator working. The runtime warning already catches the enabled no-op. Can we keep the flat Props as the public type and rely on the warning? That also matches Checkbox and RadioButton, where onPress stays optional. (via Claude, verify if it makes sense)
| return ( | ||
| <View style={[styles.wrapper, style]}> | ||
| <Pressable | ||
| disabled={Platform.OS === 'web' ? isDisabled : !isInteractive} |
There was a problem hiding this comment.
On web a read-only switch still shows the pointer cursor. React Native Web's Pressable adds cursor: pointer whenever disabled is false, and this line keeps disabled false on web for read-only. On the example screen, getComputedStyle returns pointer for the "Read-only" row. The pointer tells mouse users that a click works, but the click does nothing.
Add a style override for that case:
style={[
styles.touchable,
Platform.OS === 'web' ? webNoOutline : undefined,
Platform.OS === 'web' && !isInteractive ? { cursor: 'auto' } : undefined,
]}(via Claude, verify if it makes sense)
| focusable={isInteractive} | ||
| tabIndex={isInteractive ? 0 : -1} |
There was a problem hiding this comment.
focusable and tabIndex set the same thing. On Android, React Native treats tabIndex as an alias of focusable. On web, React Native Web uses tabIndex when it is set and ignores focusable, which it also marks as deprecated. iOS uses neither. Keep only tabIndex. The tests that check toHaveProp('focusable', ...) can check tabIndex instead.
| focusable={isInteractive} | |
| tabIndex={isInteractive ? 0 : -1} | |
| tabIndex={isInteractive ? 0 : -1} |
(via Claude, verify if it makes sense)
|
|
||
| const { props, children } = tree; | ||
| return [ | ||
| { ...StyleSheet.flatten(props.style), ...getAnimatedStyle(tree) }, |
There was a problem hiding this comment.
This adds StyleSheet.flatten, which the project avoids. The tests that use this helper also compare a dump of every style in the tree, or snapshot it. A change to any internal layer then breaks them, and the snapshot does not show which value the test is about. Can these tests assert the specific values they cover? For example, use toHaveStyle (it already handles style arrays) on the switch for the 52x48 size. For the handle color, focus ring opacity and handle size, use Reanimated's getAnimatedStyle on the one element that changes. (via Claude, verify if it makes sense)
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
See the inline comment. (via Claude, verify if it makes sense)
| <TouchableRipple onPress={toggleTheme}> | ||
| <View style={styles.row}> | ||
| <Text>Dark Theme</Text> | ||
| <View pointerEvents="none"> | ||
| <Switch value={isDarkTheme} readOnly /> | ||
| </View> | ||
| </View> | ||
| </TouchableRipple> |
There was a problem hiding this comment.
In this pattern the row is the control, but screen readers do not get the on/off state from it. The drawer rows in the example app use the same structure:
- iOS: the row is one element with no switch trait and no value. The inner switch is merged into it, so its on/off state is lost.
- Android: the row is a clickable element with no checked state, and the inner switch is a separate disabled switch next to it.
- Web: the focusable row has no role and no
aria-checked.
Give the row the switch role and state, and hide the inner switch:
| <TouchableRipple onPress={toggleTheme}> | |
| <View style={styles.row}> | |
| <Text>Dark Theme</Text> | |
| <View pointerEvents="none"> | |
| <Switch value={isDarkTheme} readOnly /> | |
| </View> | |
| </View> | |
| </TouchableRipple> | |
| <TouchableRipple | |
| onPress={toggleTheme} | |
| role="switch" | |
| aria-checked={isDarkTheme} | |
| > | |
| <View style={styles.row}> | |
| <Text>Dark Theme</Text> | |
| <View pointerEvents="none" aria-hidden> | |
| <Switch value={isDarkTheme} readOnly /> | |
| </View> | |
| </View> | |
| </TouchableRipple> |
I tried this on the "Dark Theme" drawer row. iOS then shows one switch "Dark Theme" with a value that follows the toggle. Android shows one checkable switch "Dark Theme" and the inner switch drops out of the accessibility tree. Web shows role="switch" and aria-checked on the row. The drawer, FAB and TextInput example rows can use the same change. On web, Space still does not toggle the row, because React Native Web handles Space only for button roles. Enter works. (via Claude, verify if it makes sense)
Motivation
Switchis one of the MD3 reference components, but a review against the spec found small residual deviations, mostly accessibility gaps.The significant one is breaking: a switch must now declare how it can be operated —
onValueChange,readOnly, ordisabled.readOnlykeeps the enabled appearance and stays in the accessibility tree so its state is announced, and carriesaria-readonlyon web; it is not reported as disabled. Enforced by the props type plus a runtime guard for untyped callers.Two things worth mentioning:
Propsand the operability union sits on the component's declared type.SwitchPropsexports the narrowed type, so the public type matches what the component accepts. Commented in place so it isn't "simplified" later.Related issue
This closes the Switch review checklist:
primary; MD3 spec isonPrimaryContainer.role="switch"with noonValueChange(an enabled no-op); require or infer explicit operability (handler / read-only / disabled).Test plan
yarn lint,yarn typecheckandyarn testpass — 684 tests, 168 snapshots. Each commit is independently green, so the series bisects cleanly.9 new tests in
Switch.test.tsxcover the focus indicator, state layer and handle colour, the 52×48 touch target, and operability. The state-layer and handle-colour assertions fail if the focus fix is reverted (mutation-tested).Manual, on the Switch example screen:
onPrimaryContainer), not purple.readOnly, with the row owning the press.primaryContainerhandle; a mouse click must not light the ring. Read-only is skipped in the tab order and hasaria-readonly="true".Run on Android, iOS and web, plus a screen-reader pass. All behaved as described.