Conversation
ad60f24 to
f7b7f18
Compare
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
Motivation
Touch targets used to match the drawn box exactly:
Checkbox40×40,RadioButtonAndroid/RadioButtonIOS~36×36 (20dp glyph + 8dp margin on Android, 24dp glyph + 6dp padding on iOS),IconButton40×40,Chip's close icon 26×18.MD3 instead grows the target around the component without resizing it, and only when the component is interactive - so a 40dp state layer stays 40dp visually but gains slop around it.
Each interactive component now computes its own
hitSlopfrom its own fixed render-time size, via a newgetMinInteractiveSizeHitSlop({ width?, height? })utility. It returns theInsetsneeded to reach the 48dp minimum, orundefinedif the component is already big enough.TouchableRippleitself does no measuring, it honors whateverhitSlopit's given. A caller-suppliedhitSlopoverrides the computed default, and adisabledcomponent gets no slop at all.This normalizes
RadioButtonAndroid/RadioButtonIOSonto the same 40dp state layer asCheckbox/Switch(RadioButtonTokens.stateLayerSize), with glyph size/padding adjusted to stay centered. New per-component token files (Chip/tokens.ts,RadioButton/tokens.ts) hold the MD3 dimensions these computations derive from. The 48dp minimum itself is a localMIN_INTERACTIVE_SIZEconstant ingetMinInteractiveSizeHitSlop.ts- an earlier revision added it as a sharedminInteractiveSizetoken ontheme/tokens/sys/state.ts, but that was reverted per review feedback.Two platforms, because one mechanism doesn't cover both:
Insetspass straight through toPressable'shitSlop, same as a caller-supplied one.react-native-webdroppedhitSlopsupport in 0.13.0, soTouchableRippleinstead renders anaria-hidden, absolutely-positioned sibling sized from the sameInsets, for the browser to hit-test. It renders before the children so it can never cover an interactive child.Switchdoesn't go throughTouchableRipple, so it carries its own equivalentwebTouchTargetview.A parent with
overflow: 'hidden'clips the expanded target - which is whyIconButton'shitSlophad never actually applied. So on web, the ripple now clips itself via its own inset, radius-matched container, rather than relying on the touchable or an ancestor.That change pulls in a few more:
IconButton's container dropsoverflow: 'hidden'; the radius moves to the overlay and touchable so they clip themselves. Its hardcodedhitSlop({10}/{6}depending onTouchableRipple.supported) is replaced by the computed one. Since shape can no longer be read out ofstyle(it may be an animated value on the UI thread, invisible to synchronousStyleSheet.flatten),IconButtonnow also acceptsborderRadius/borderTop*Radius/etc. as plain props, so callers needing a custom shape (e.g. segmented ends) can apply it directly to the self-clipping overlay/touchable.SegmentedButtonItemnow computes its own defaulthitSlopfrom content height (2 * paddingVertical + iconSize, never shorter than the label) - previously it only forwarded a caller-suppliedhitSlop, with no floor of its own.Chip's close button now fills the 34dp column the chip already reserved, instead of just the 26×18 icon.Related issue
Follow-up to #5080 (
fix: expand interactive targets to the 48dp minimum) - same change, rebased onto currentmainand updated per that PR's review feedback (theonLayout-based measurement was replaced with the static per-component computation described above).Closes #5079.
Touches the same files as #5071 (
fix: don't expose handler-less TouchableRipple as a disabled control), still open - no conflict today, but whichever lands second needs a look.Buttonhas the same clipped, non-applyinghitSlopbut is intentionally left unfixed 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. A new
TouchableRippleWeb.test.tsxpins the web target's style/ordering against the touchable's own bounds, a caller-suppliedhitSlop,disabled, and the no-handler case.The suite only renders a tree with no layout or hit testing, so it pins props, not behavior. Verified on device by tapping inside the expected slop and again past it:
Notes
onClosewhere it used to fireonPress. This matches MD3, where the primary action stops where the trailing action begins.borderlessno longer clips content on web. It still clips the ripple, now via its own inset container instead of the touchable itself - the touchable can't clip without also clipping the touch target. Nothing in Paper depended on the old behavior (checked across 569 touchables on 15 screens); prop docs updated.Videos
Visual confirmation that
TouchableRippleitself is unchanged before/after - only thehitSlop/touch target changed, not how the ripple looks or animates. Each pair is the same interaction recorded before and after this PR on the same platform.Web
web_before.mov
web_after.mov
Android
android_before.mov
android_after.mov
iOS
ios_before.mov
ios_after.mov