Repository navigation
fix(button): prevent false ripples during scroll deceleration (#4911) - #5133
Priyanshu0007 wants to merge 1 commit into
Conversation
|
Found potential problems with the pull request:
|
MikitasK
left a comment
MikitasK
left a comment
There was a problem hiding this comment.
good job 👍
the fix looks good: I reproduced the bug & fix, so the direction is right 💯
overall:
- 2 things looks blocking to me:
Build packageCI step & test coverage - 1 coordination point: #5097 rewrites most of
Button.tsx, so worth agreeing on sequencing before you spend more time here - the rest is discussion, not blocking
| /** | ||
| * Delay in ms, from the start of the touch, before the press is engaged and ripple is triggered. | ||
| */ | ||
| unstable_pressDelay?: number; |
There was a problem hiding this comment.
could we check how this lands against #5097 ? that PR is actively rewriting this file & already covers all 3 places this PR edits, so it'll definitely conflict.
since 6.x is moving Button to Expressive, what do you think about rebasing on it?
There was a problem hiding this comment.
[blocker] another thing to flag: Build package CI step will fail for this PR (I reproduced it locally)
API leak guard in scripts/typescript-output-lint.ts sees unstable_pressDelay in generated Button.d.ts & flags it as unexpected RN prop.
Now that Button defines it, could we just remove it from unusedViewProps at scripts/typescript-output-lint.ts ?
| expect(screen.getByTestId('active-button')).toBeEnabled(); | ||
| }); | ||
|
|
||
| it('renders button with unstable_pressDelay and triggers onPress', async () => { |
There was a problem hiding this comment.
[blocker] could we strengthen these?
I tried running this test after reverting changes in Button.tsx. Result - test passed, so seems like nothing here actually guards the delay.
userEvent.press resolves regardless of it. also RNL 14 removed UNSAFE_getByType that's why prop assertion isn't available either.
so wdyt about driving RN's responder state machine with fake timers asserting pressIn is suppressed on RESPONDER_TERMINATED?
| testID, | ||
| }: IconProps) => React.ReactNode; | ||
| rippleEffectEnabled?: boolean; | ||
| ripplePressDelay?: number; |
There was a problem hiding this comment.
could we keep this PR to per-component unstable_pressDelay prop & leave global ripplePressDelay for separate discussion?
#4911 is about a button inside scrolling list, but this setting changes every TouchableRipple-based component app-wide the moment someone sets it on PaperProvider.
it's also not fully global: Card uses its own Pressable with unstable_pressDelay={0} so pressable Card in a list would still misbehave.
for what it's worth, #3806 (same problem) was solved at the call site rather than with library-wide default. the wiring itself looks fine & consistent with rippleEffectEnabled - mainly wanted to keep this PR's surface area small
| onPressIn={hasPassedTouchHandler ? handlePressIn : undefined} | ||
| onPressOut={hasPassedTouchHandler ? handlePressOut : undefined} | ||
| delayLongPress={delayLongPress} | ||
| unstable_pressDelay={unstable_pressDelay} |
There was a problem hiding this comment.
what do you think about fast-tap case?
with a delay set, tap shorter than it still fires onPress but with no visible feedback at all - close to the mirror image of original complaint.
could we document the trade-off on the prop? and/or suggest a smaller value maybe?
| */ | ||
| delayLongPress?: number; | ||
| /** | ||
| * Delay in ms, from the start of the touch, before the press is engaged and ripple is triggered. |
There was a problem hiding this comment.
[nit] I tested it out & seems like press still fires - only feedback is delayed
so maybe we could reword it to smth like "...before the press feedback (ripple) is shown"?
Motivation
When tapping or touching an active Button or TouchableRipple inside a decelerating scroll view (or during quick list fling/scroll cancellation touches), the native button immediately triggers an ink ripple animation. However, when the parent
ScrollViewdetects scrolling motion, it steals the touch gesture (cancelling the press andonPress). The button is never activated, leaving the user with false visual feedback.This PR addresses the issue by:
unstable_pressDelay?: numberonButtonandTouchableRippleprops to forward to React Native's underlyingPressable.ripplePressDelay?: numberconfiguration viaSettingsContext(PaperProvider), allowing apps to configure a global press delay (e.g. 50-100ms) across all paper ripple surfaces without having to pass the prop to every component manually.Related issue
Fixes #4911
Test plan
Automated Tests:
Button.test.tsxverifyingunstable_pressDelaytriggersonPresscorrectly.TouchableRipple.test.tsxverifying bothunstable_pressDelayprop andripplePressDelayfromSettingsContext.yarn test,yarn lint,yarn typecheck).Manual Verification (Android):
<ScrollView>.<PaperProvider settings={{ ripplePressDelay: 80 }}>or addunstable_pressDelay={80}to<Button>.onPresstrigger as expected.