fix: double press interaction on android cp-7.54.0#18729
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18729 +/- ##
==========================================
+ Coverage 75.51% 75.67% +0.15%
==========================================
Files 3092 3111 +19
Lines 71646 71951 +305
Branches 12413 12471 +58
==========================================
+ Hits 54106 54446 +340
+ Misses 14093 14056 -37
- Partials 3447 3449 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const conditionalOnPress = isDisabled | ||
| ? undefined | ||
| : (_pressEvent?: GestureResponderEvent) => { | ||
| const now = Date.now(); |
There was a problem hiding this comment.
Wondering instead of date, we could use a mutex logic here, since we want to prevent double tap, if I'm thinking correctly mutex logic would prevent that, instead of relying on timings
There was a problem hiding this comment.
Nevermind that would be for async operations
| .shouldCancelWhenOutside(false) | ||
| .maxDeltaX(20) // Allow some movement while tapping | ||
| .maxDeltaY(20) |
There was a problem hiding this comment.
Would this be the solution for the press on scroll view right?
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…to fix/android-buttons
…to fix/android-buttons
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a fix for double press interactions on Android devices. The issue was that both the gesture handler and accessibility onPress were being triggered simultaneously, causing duplicate function calls.
- Adds coordination logic using timestamps to prevent double firing of onPress events within a 100ms window
- Wraps TouchableOpacity components with custom gesture detection for Android while preserving accessibility
- Implements platform-specific handling where Android uses gesture detection and iOS maintains standard TouchableOpacity behavior
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ListItemSelect.tsx |
Adds coordination system with timestamp-based deduplication and custom TouchableOpacity wrapper |
ListItemMultiSelect.tsx |
Implements double-press prevention with platform-specific checkbox interaction handling |
ButtonBase.tsx |
Adds gesture detection wrapper and coordination logic for button components |
| Test files | Updates snapshots and test descriptions to reflect coordination logic changes |
| Snapshot files | Updates to reflect new onPress function references and gesture wrapper structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
|
|
||
| // Set timestamp BEFORE calling parent function (timestamp-first pattern) | ||
| lastCheckboxGestureTime.current = Date.now(); |
There was a problem hiding this comment.
The variable name lastCheckboxGestureTime is misleading because it's used for both checkbox gestures and main component coordination. Consider renaming to lastPressTime for consistency with other components like ListItemSelect and ButtonBase.
| const timeSinceLastGesture = now - lastCheckboxGestureTime.current; | ||
|
|
||
| if (onPress && timeSinceLastGesture > COORDINATION_WINDOW) { |
There was a problem hiding this comment.
Variable name timeSinceLastGesture is inconsistent with other components that use timeSinceLastPress. This inconsistency makes the codebase harder to maintain.
| const timeSinceLastGesture = now - lastCheckboxGestureTime.current; | |
| if (onPress && timeSinceLastGesture > COORDINATION_WINDOW) { | |
| const timeSinceLastPress = now - lastCheckboxGestureTime.current; | |
| if (onPress && timeSinceLastPress > COORDINATION_WINDOW) { |
There was a problem hiding this comment.
Nice work tackling this! Approving the design system changes to unblock release. I’ll defer to @MetaMask/mobile-platform engineers on the React Native and iOS/Android details of this fix. There also seem to be some Cursor/Copilot comments that could be addressed
Description
Changelog
CHANGELOG entry:
Related issues
Fixes: #18704
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist