Skip to content

fix: don't expose handler-less TouchableRipple as a disabled control - #5071

Open
lukemorawski wants to merge 1 commit into
callstack:mainfrom
lukemorawski:fix/touchable-ripple-non-interactive-semantics
Open

fix: don't expose handler-less TouchableRipple as a disabled control#5071
lukemorawski wants to merge 1 commit into
callstack:mainfrom
lukemorawski:fix/touchable-ripple-non-interactive-semantics

Conversation

@lukemorawski

Copy link
Copy Markdown

Motivation

TouchableRipple had const disabled = disabledProp || !hasPassedTouchHandler, so anything without a press handler got announced as a disabled control. Split into two flags:

const isControl = hasPassedTouchHandler || Boolean(disabledProp);
const isInteractive = hasPassedTouchHandler && !disabledProp;

Non-controls now render a plain Animated.View instead of a Pressable.

Worth knowing why it drops the Pressable instead of just passing the caller's disabled through: disabled also gates touch-responder claiming (Pressability.js:448, onStartShouldSetResponder: () => !disabled). Fixing only the announcement turns claiming on, so handler-less ripples start swallowing taps. TextInput wraps its field and icons in <Pressable onPress={focusInput}> (TextInput.tsx:352), so tapping a decorative icon stopped focusing the field. Tests stayed green through that.

Two things kept on purpose, both break stuff if dropped:

  • accessible={rest.accessible !== false}, or a checked CheckboxItem with no handler loses its role and state.
  • focusable={rest.focusable ?? false}, or on web the icon lands in the tab order, since RNW turns role="button" into a real <button>.

Related issue

Closes #5070

Supersedes #5011, which fixes the same symptom in Chip.tsx only by injecting an empty onPress. That swaps a false "disabled" for a false "enabled" and leaves the other 21 consumers broken.

Test plan

Lint, typecheck and tests pass: 55 suites, 742 tests, 169 snapshots.

The suite can't catch this though. It renders an element tree, never a DOM or a native view, and jest never loads TouchableRipple.tsx at all. So it's checked on device, example app, TextInput screen with "Leading icon" on. Leading magnifier has no handler, trailing clear button does.

announced disabled tap reaches field clear button still works
iOS 18.3 enabled: false -> true yes yes
Android 15 enabled="false" -> "true" yes, 3/3 runs yes
web <button disabled> -> gone yes yes

Web also keeps tabindex="-1" on the decorative icon.

Snapshots are noisy but mechanical: 80 elements drop the Pressable handlers and the always-undefined a11y scaffolding. Checked every style key/value pair, nothing net-added. Some aria-* show up raw now because View.js does that folding at runtime and the jest preset mocks View.

Two extras, worth a look

TextInputIcon was faking disabled by nulling its own handler, which only worked because of this bug. Now passes disabled through.

IconButton.getIconColor returned on disabled before reading customIconColor, so once TextInputIcon forwarded disabled a disabled field's icon lost its custom and error colour. Reordered so an explicit colour wins, opacity still shows disabled. No snapshot churn, so nothing else passes both, but it is a public component behaviour change and the bit I'd most want checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TouchableRipple exposes non-interactive surfaces as disabled controls

1 participant