fix: expand interactive targets to the 48dp minimum - #5108
Conversation
ad60f24 to
f7b7f18
Compare
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
Copied from my review on #5080. The layout-measurement feedback below has already been addressed in this PR. The remaining inline comments are copied unchanged.
Remove LLM generated comments. Only add comments where the code maybe unclear and it's necessary.
The touchable should not enforce a minimum hitSlop. Adding unnecessary onLayout everywhere has performance overhead. It should only accept hitSlop prop without layout measurement or minimums. The web version only needs to implement hitSlop since React Native Web doesn't support it.
The actual hitSlop should be passed by components where they are needed, e.g. checkbox. It's simpler and doesn't have require layout measurements.
The following original Chip comment is also already addressed here, since the constants now precede the component JSDoc:
These constants are added between the component and its JSDoc, which will break documentation generation for the component.
| * Minimum size of an interactive target. Applied by expanding outside the | ||
| * component's bounds, so it is separate from the 40dp state layer that | ||
| * Checkbox and Switch render. |
There was a problem hiding this comment.
Applied by expanding outside the component's bounds
This is an implementation detail the constant here can't possibly know or control
so it is separate from the 40dp state layer that Checkbox and Switch render
That's unnecessarily specific and the information doesn't belong here.
The comment should only contain link to MD guidelines, not implementation specific notes.
| * Checkbox and Switch render. | ||
| * @see https://m3.material.io/foundations/designing/structure | ||
| */ | ||
| minInteractiveSize: 48, |
There was a problem hiding this comment.
minInteractiveSize is not a state. so it shouldn't be here
| {/* Before the children, not after. It hit-tests, so as the last | ||
| sibling it covers anything interactive inside the touchable and | ||
| takes its presses, e.g. a pressable List.Item with a control in | ||
| `right`. Ahead of them it still covers the area outside the | ||
| touchable, where there is nothing else to hit. | ||
| Nothing that cannot be pressed gets a target, same as native. */} |
There was a problem hiding this comment.
the comment is unnecessary. everything it says is self-evident
| {!disabled && ( | ||
| <View | ||
| aria-hidden | ||
| style={getTouchTargetStyle(hitSlop)} | ||
| testID="touchable-ripple-touch-target" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
This could be simplified:
| {!disabled && ( | |
| <View | |
| aria-hidden | |
| style={getTouchTargetStyle(hitSlop)} | |
| testID="touchable-ripple-touch-target" | |
| /> | |
| )} | |
| {!disabled && hitSlop != null && ( | |
| <View | |
| aria-hidden | |
| style={ | |
| typeof hitSlop === 'number' ? { | |
| position: 'absolute', | |
| top: -hitSlop, | |
| right: -hitSlop, | |
| bottom: -hitSlop, | |
| left: -hitSlop, | |
| } : { | |
| position: 'absolute', | |
| top: -(hitSlop.top ?? 0), | |
| right: -(hitSlop.right ?? 0), | |
| bottom: -(hitSlop.bottom ?? 0), | |
| left: -(hitSlop.left ?? 0), | |
| } | |
| } | |
| /> | |
| )} |
| <View | ||
| aria-hidden | ||
| style={getTouchTargetStyle(hitSlop)} | ||
| testID="touchable-ripple-touch-target" |
| // We don't apply `focusIndicator.outerOffset`, so the ring stays inside the 40dp | ||
| // circle. `TouchableRipple borderless` used to crop anything outside it; on web | ||
| // it no longer does, since the touchable cannot clip without clipping the touch | ||
| // target. Native still clips. Check both when revisiting the offset. |
There was a problem hiding this comment.
what it used to is not relevant as a code comment
| * that, and those ancestors have to stop clipping to reach into the `hitSlop`. | ||
| */ | ||
| const getUnderlayShape = (style: StyleProp<ViewStyle>): ViewStyle => { | ||
| const flat = StyleSheet.flatten(style); |
There was a problem hiding this comment.
StyleSheet.flatten needs to be removed. add explicit border radius props if needed similar to Surface
| : { top: 6, left: 6, bottom: 6, right: 6 } | ||
| } | ||
| testID={testID} | ||
| hitSlop={hitSlop} |
There was a problem hiding this comment.
Codex: We need to account for joined controls when adding this default. ToggleButton.Row renders 42x42 buttons with no gap, and on web the right button's expanded target covers the last 3px inside the left button's visible box. Browser hit testing selects the right button in that strip. We need component-specific slop for joined buttons so pressing inside one option doesn't activate its neighbor.
There was a problem hiding this comment.
While working on ToggleButton.Row, I divided up hitSlop so adjacent buttons don't steal presses from each other on the shared edge, but that protection only applies to the horizontal Row layout.
When ToggleButton.Group is used bare, stacking buttons vertically (as shown in the "Group & enums" example), each button keeps its full, unshared hitSlop. Since there's no gap between them, pressing near the bottom edge of one button can activate the next one down instead. It is the same kind of overlap issue I fixed for Row, just not handled for vertical stacking.
Since Group is public API and documented to work without Row, this is reachable by anyone stacking toggle buttons vertically, not just in the example.
Options I see:
- Add a
ToggleButton.Column— mirrorsRow's existing logic, just for top/bottom instead of left/right - Make
ToggleButtonGrouporientation-aware so it handles this safely by default, without a new component
Which direction would you like for this PR, or should it be handled separately from the 48dp work?
| const buttonSize = size + 2 * PADDING; | ||
| const borderWidth = mode === 'outlined' && !selected ? 1 : 0; | ||
|
|
||
| const shapeStyles = { |
There was a problem hiding this comment.
Codex: We still accept corner radii through style, but the overlay and touchable now only get the radii from these props. With style={{ borderRadius: 0 }}, the disabled contained button's fill changes from square to circular. ToggleButton and ToggleButton.Row also still pass their corners through style, so their press effects get the wrong shape. We need one source for the shape and need to update the existing callers if we're moving the radii to explicit props.
| // slop of its own, only what a caller's own `hitSlop` in `rest` supplies. | ||
| const hitSlop = disabled | ||
| ? undefined | ||
| : getMinInteractiveSizeHitSlop({ width: buttonSize, height: buttonSize }); |
There was a problem hiding this comment.
Codex: We're calculating slop from the outer default size, but the touchable can be smaller. With the default outlined button, the 1px border leaves a 38x38 touchable, so 4px of slop only reaches 46x46. TextInput.Icon also passes a 24x24 style, which gives us a 32x32 target with this calculation. Both sizes were verified in the browser. We need to account for the rendered component dimensions and border when choosing the default slop.
| return typeof hitSlop === 'number' | ||
| ? { | ||
| position: 'absolute', | ||
| top: inset(hitSlop), |
There was a problem hiding this comment.
Codex: These absolute offsets start inside the touchable's border, so we don't expand from its outer bounds. With a 40x40 touchable, borderWidth: 4, and hitSlop={2}, we get a 36x36 target overlay. A point 1px outside the visible button misses in the browser, though it should be within the requested slop. We need to account for the border when positioning the web target.
| disabled={disabled} | ||
| {...accessibilityProps} | ||
| testID={testID} | ||
| hitSlop={rest.hitSlop ?? (disabled ? undefined : CHECKBOX_HIT_SLOP)} |
There was a problem hiding this comment.
Codex: With ??, we replace an explicit hitSlop={null} with the computed default. TouchableRipple treats null as no slop, so we're losing the caller's override here. The same issue exists in both RadioButtons, Chip, and SegmentedButtonItem, while IconButton preserves null. We should only apply the default when hitSlop is undefined.
| * Room the chip reserves on its right for the close button, which fills all of | ||
| * it, so the body stops here and the two divide the chip. | ||
| * | ||
| * MD3 splits the same way and does not give a chip's trailing action 48dp; in |
There was a problem hiding this comment.
Codex: The Material Web reference doesn't support this exception. Its remove button renders a .touch element, and that element has 48px height. The 24x24 dimensions in _trailing-icon.scss are for the ripple and focus ring. Here we leave the close target unexpanded and give the strips above and below it to the body, so a tap near the close icon can activate the chip instead. We need vertical expansion for the close action and a matching division of the two targets.
The component doc comment ended up separated from `const Chip =` by the hitSlop helper constants, so the docs generator could no longer find it.
main removed the hardcoded default testIDs from Chip and IconButton (callstack#5088). Guard Chip's close-icon testID the same way its container already is, and pass explicit testID props in the hitSlop/close-icon tests that relied on the old defaults.
271d56a to
8910723
Compare
3 participants
Motivation
Touch targets matched the drawn box:
Checkbox40x40,RadioButtonAndroid/RadioButtonIOS~36x36 (20dp glyph + 8dp margin on Android, 24dp glyph + 6dp padding on iOS),
IconButton40x40,Chipclose icon 26x18.MD3 grows the target outside the component rather than resizing it, and only when the
component is interactive, so the 40dp state layer stays 40dp and gains slop around it.
Each interactive component now computes its own
hitSlopfrom its own known, fixedrender-time size — a
getMinInteractiveSizeHitSlop({ width?, height? })utility thatreturns the
Insetsneeded to reach the 48dp minimum, orundefinedwhen thecomponent is already big enough.
TouchableRippleitself does no measuring andenforces no minimum: it is a dumb primitive that just honors whatever
hitSlopit isgiven, the same on both platforms. A caller-supplied
hitSlopstill wins over thecomputed default; a
disabledcomponent gets no slop of its own.That normalizes
RadioButtonAndroid/RadioButtonIOSonto a shared 40dp state layer(
RadioButtonTokens.stateLayerSize), matchingCheckbox/Switch, instead of theprevious ~36dp box — glyph size/padding adjusted to stay centered in it.
Chipandnew per-component token files (
Chip/tokens.ts,RadioButton/tokens.ts) hold the MD3spec dimensions each computation is derived from. A
minInteractiveSize: 48token wasadded to
theme/tokens/sys/state.tsas the shared constant.Two platforms, since a single mechanism doesn't cover both:
Insetsobject is passed straight through toPressable'shitSlop, same as any caller-supplied one.react-native-webremovedhitSlopsupport in 0.13.0, soTouchableRipplerenders an
aria-hidden, absolutely positioned sibling sized from that sameInsetsobject for the browser to hit-test instead. It renders before thechildren (not after) so it can't cover an interactive child, e.g. a pressable
List.Itemwith a control inright.Switchdoesn't go throughTouchableRipple(it's a plain
Pressable), so it carries its own equivalentwebTouchTargetview,positioned the same way.
A parent with
overflow: 'hidden'clips the expanded target.IconButtonhas beenshipping a
hitSlopthat never applied for this reason. So on web the ripple nowclips itself via its own inset, radius-matched container, instead of relying on the
touchable or an ancestor to clip it — unconditionally, not
centered ? 'visible' : 'hidden'as before (a ripple that escaped used to be caught by whichever ancestorclipped, and those ancestors have to stop clipping to reach into the target; e.g.
ToggleButtonpassesborderless={false}down toIconButton, which spread it overits own
Surface, previously holding the ripple in). Output is pixel identical.That change pulls in:
IconButton's container dropsoverflow: 'hidden'; the radius moves to the overlayand the touchable so they clip themselves. Its own hardcoded
hitSlop({10}/{6}depending onTouchableRipple.supported) is removed in favor of the computedone. Since shape can no longer be read out of
style(which may be an animatedvalue on the UI thread, invisible to a synchronous
StyleSheet.flatten),IconButtonnow also accepts
borderRadius/borderTop*Radius/etc. as plain props so callersneeding a custom shape (e.g. segmented ends) can still apply it to the
self-clipping overlay/touchable.
SegmentedButtonItemnow computes its own defaulthitSlopfrom its contentheight (
2 * paddingVertical + iconSize, never shorter than the label) — previouslyit only forwarded a caller-supplied
hitSlopwith no floor of its own.square, and only looked right because a parent clipped it.
Chip's close button fills the 34dp column the chip already reserved, rather thanjust the 26x18 icon.
Related issue
Follow-up to #5080 (
fix: expand interactive targets to the 48dp minimum) — samechange, rebased onto current
mainand updated per review feedback there (theonLayout-based measurement was replaced with the static per-component computationdescribed above).
Closes #5079
Touches the same files as #5071 (
fix: don't expose handler-less TouchableRipple as a disabled control), still open. No conflict currently, but whichever lands secondneeds a look.
Buttonhas the same clipped, non-applyinghitSlop, but is intentionally leftunfixed here — per review discussion on #5080, unclipping it is deferred to a
follow-up on #5097 (Button MD3) once this merges. Both PRs touch
__snapshots__/Button.test.tsx.snap; whichever lands second regenerates it.Test plan
Lint, typecheck and tests pass. New
TouchableRippleWeb.test.tsxpins the webtarget's style/ordering against the touchable's own bounds, a caller-supplied
hitSlop,disabled, and the no-handler case.The suite renders an element tree with no layout and no hit testing, so it only pins
props. Checked on device by tapping inside the expected slop and again past it.
Both run Fabric, and it applies from first mount without scrolling.
On Chip, the close button takes the right 34dp and the body the rest, on all three.
Notes
Chipchanges behaviour. The right 34dp firesonClosewhere it firedonPress.That matches MD3, where the primary action stops where the trailing one starts.
borderlessno longer clips content on web. It still clips the ripple, now via itsown inset container rather than the touchable itself — the touchable cannot clip
without clipping the touch target. Nothing in Paper depends on the old behavior,
checked across 569 touchables on 15 screens. Prop doc updated.
Targets can now overlap, which is the MD3 default. On web the later sibling takes the
shared strip. They can reserve space instead if you prefer.
Videos
Visual confirmation that
TouchableRippleitself is identical before and after. Only thehitSlop/touch target changed, not how the ripple looks or animates. Each pair below is the same interaction recorded before and after this PR on different platforms.Web
web_before.mov
web_after.mov
Android
android_before.mov
android_after.mov
iOS
ios_before.mov
ios_after.mov