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
| ref={ref} | ||
| disabled={disabled} | ||
| hitSlop={shouldExpand ? expansion : hitSlop} | ||
| onLayout={shouldMeasure ? handleLayout : onLayout} |
There was a problem hiding this comment.
check out this comment
| onLayout={shouldMeasure ? handleLayout : onLayout} | |
| onLayout={handleLayout} |
| ref={ref} | ||
| disabled={disabled} | ||
| hitSlop={shouldExpand ? expansion : hitSlop} | ||
| onLayout={shouldMeasure ? handleLayout : onLayout} |
There was a problem hiding this comment.
check out this comment
| onLayout={shouldMeasure ? handleLayout : onLayout} | |
| onLayout={handleLayout} |
| 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,
]}
| style={[styles.touchable, contentStyle]} | ||
| style={[ | ||
| styles.touchable, | ||
| { borderRadius }, |
There was a problem hiding this comment.
check out this comment
| { borderRadius }, | |
| shapeStyles, |
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.