fix: expand interactive targets to the 48dp minimum - #5080
Conversation
| // A caller hitSlop wins, so there is nothing to measure for. `null` counts as | ||
| // supplied, it means "no slop". | ||
| const shouldMeasure = hitSlop === undefined; | ||
|
|
||
| // Gates whether the measurement is applied, not whether it happens. RN emits | ||
| // onLayout on mount and on layout change, so a touchable that mounts disabled | ||
| // gets no event once it is enabled and would stay small. | ||
| const shouldExpand = shouldMeasure && !disabled; |
There was a problem hiding this comment.
could we keep measuring while custom hitSlop is set?
for example:
- component mounts at
32×32withhitSlop={6}. - caller removes it with
hitSlop={undefined}. - component should now calculate
{ top: 8, right: 8, bottom: 8, left: 8 }. - currently it cannot, because
const shouldMeasure = hitSlop === undefined;(line 145) disabled measurement whilehitSlop={6}.
addingonLayoutafterward doesn't trigger new event unless the layout changes (RN docs](https://reactnative.dev/docs/view.html#onlayout))
so could what about removing shouldMeasure:
| // A caller hitSlop wins, so there is nothing to measure for. `null` counts as | |
| // supplied, it means "no slop". | |
| const shouldMeasure = hitSlop === undefined; | |
| // Gates whether the measurement is applied, not whether it happens. RN emits | |
| // onLayout on mount and on layout change, so a touchable that mounts disabled | |
| // gets no event once it is enabled and would stay small. | |
| const shouldExpand = shouldMeasure && !disabled; | |
| // Gates whether the measurement is applied, not whether it happens. RN emits | |
| // onLayout on mount and on layout change, so a touchable that mounts disabled | |
| // gets no event once it is enabled and would stay small. | |
| const shouldExpand = hitSlop === undefined && !disabled; |
and using handleLayout only in both Pressable's:
- onLayout={shouldMeasure ? handleLayout : onLayout}
+ onLayout={handleLayout}
I guess, this should keep latest measurement ready while still letting the caller-provided hitSlop win
There was a problem hiding this comment.
ah yeah, you are right and it does not even fall back to the caller value too. ends up with no slop at all. Tried with hitSlop={2}, droped it, and hitSlop is undefined 🤦
web is right, getTouchTargetStyle runs every render, so only native was wrong
Done: shouldExpand = hitSlop === undefined && !disabled, plus handleLayout on both Pressables, so this covers your other comments as well. Added a test for good measure. Thansk for that!
Cost is one extra measure and render on mount for touchables that do pass hitSlop, plus a render on every resize for a value that is never used. Nothing in Paper sets hitSlop internally any more, so it is only callers who ask for it. Fine by me :)
| style={[ | ||
| StyleSheet.absoluteFill, | ||
| { backgroundColor, opacity: backgroundOpacity }, | ||
| { backgroundColor, opacity: backgroundOpacity, borderRadius }, |
There was a problem hiding this comment.
what about preserving custom corner styles like borderTopLeftRadius, borderTopRightRadius etc when moving clipping from Surface to TouchableRipple?
before this PR Surface’s overflow: 'hidden' clipped the ripple with a square top-left corner.
now only borderRadius is forwarded. so could we extract every border*Radius property & apply them to both new clipping layers?
so what about replacing lines 150 - 160 with smth like that:
const flattenedStyle = (StyleSheet.flatten(style) || {}) as ViewStyle;
const {
borderWidth = mode === 'outlined' && !selected ? 1 : 0,
} = flattenedStyle;
const [, radiusStyles] = splitStyles(
flattenedStyle,
(key) => key.startsWith('border') && key.endsWith('Radius')
);
const {
borderRadius = buttonSize / 2,
...cornerRadiusStyles
} = radiusStyles;
const shapeStyles = {
borderRadius,
...cornerRadiusStyles,
};
const borderStyles = {
borderWidth,
borderColor,
...shapeStyles,
};
and then applying shapeStyles here:
| { backgroundColor, opacity: backgroundOpacity, borderRadius }, | |
| { backgroundColor, opacity: backgroundOpacity, ...shapeStyles }, |
and in TouchableRipple :
<TouchableRipple
borderless
centered
onPress={onPress}
aria-label={ariaLabel}
style={[
styles.touchable,
shapeStyles,
// The Surface used to clip the ripple, so the touchable does it now.
// Native only: its own overflow does not clip its hitSlop, but on web
// it would clip the touch target, where the container already clips.
Platform.OS !== 'web' && styles.clipToShape,
contentStyle,
]}
There was a problem hiding this comment.
Yeppers, this is a regression from the PR. looks like borderTopLeftRadius: 0 squares the container off and leaves the overlay and the ripple round
so i took splitStyles, which is what Surface, Card, CardCover and Button already do, but skipped the destructure since it spreads borderRadius back in:
const shapeStyles = { borderRadius: buttonSize / 2, ...borderRadiusStyles };
same result, same as Card. Applied to the overlay and the touchable, so the other comment is covered too. mends web as well, ripple container copies corner radii (radiuses :D) off the touchable style.
Test added
|
Heads-up from #5097 (Button MD3): Button had its own version of this - a computed Worth noting that the Button isn't covered here either: the Note: both PRs touch |
satya164
left a comment
There was a problem hiding this comment.
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.
| /** | ||
| * 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 | ||
| * material-web it is 24x24 with no expansion. This column is wider than that and | ||
| * gets no vertical expansion, so the strips above and below belong to the body | ||
| * and a near miss activates the chip rather than deleting it. | ||
| * @see https://github.com/material-components/material-web/blob/main/chips/internal/_trailing-icon.scss | ||
| */ | ||
| const CLOSE_AFFORDANCE_WIDTH = 34; | ||
|
|
||
| /** | ||
| * Floor for the clamp below. The glyph is 18dp and sits 8dp from the right, so | ||
| * under this it hangs over the chip body, and part of the visible icon would | ||
| * activate the chip instead of removing it. | ||
| */ | ||
| const CLOSE_AFFORDANCE_MIN_WIDTH = 26; | ||
|
|
There was a problem hiding this comment.
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
Motivation
Touch targets matched the drawn box:
Checkbox40x40,RadioButton32x36,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.
Two mechanisms, since one does not cover both platforms:
TouchableRipplemeasures itself withonLayoutand setshitSloparia-hiddenabsolutely positioned child atmax(48px, 100%)react-native-web removed
hitSlopin 0.13.0, so web needs its own. An absolutelypositioned child is what material-web uses. Pseudo elements are not an option, a
::beforeis clipped like any other child.Measuring and applying are separate. A component that mounts
disabledgets no layoutevent once it is enabled, since RN does not replay one, so gating the measurement on
interactivity would leave it small permanently.
A caller
hitSlopwins on both platforms.A parent with
overflow: 'hidden'clips the expanded target.IconButtonhas beenshipping a
hitSlopthat never applied for this reason. So on web the ripple is nowclipped by its own container instead of by the touchable. Output is pixel identical.
That change pulls in:
IconButton'sSurfacedropsoverflow: 'hidden', and the radius moves to theoverlay and the touchable so they clip themselves. Its own
hitSlopis removed.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
Closes #5079
Touches the same file as #5071, which splits interactive from control. No conflict,
but whichever lands second needs a look.
Test plan
Lint, typecheck and tests pass.
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. The touchablecannot clip without clipping the target. Nothing in Paper depends on it, checked
across 569 touchables on 15 screens. Prop doc updated.
Mount cost is up 1.6% on device, 165ms to 167.8ms for 300 touchables, three runs each
way, dev build. Not measurable in jest.
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.