feat/fix residual md3 deviations in switch - #5102
Open
likevy wants to merge 15 commits into
Open
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.
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`.
likevy
force-pushed
the
feat/fix-residual-md3-deviations-in-switch
branch
from
September 4, 2026 10:56
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
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.
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
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
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.
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.