-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: Updated ButtonBase and ListItems to have gesture detection on Android cp-7.53.0 #18104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bcf7ec6
0ee868e
a30c9fe
5ca1135
c6fe1f6
0a8420c
a0cad6c
8375b9a
5b4aa75
03e0af3
0601796
1c79404
1704f35
581a757
b065a47
2abf75d
e02d75e
12189a6
c741d5e
c500a79
253625a
cecc715
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,12 @@ | |
|
|
||
| // Third party dependencies. | ||
| import React from 'react'; | ||
| import { TouchableOpacity } from 'react-native'; | ||
| import { | ||
| TouchableOpacity as RNTouchableOpacity, | ||
| TouchableOpacityProps, | ||
| Platform, | ||
| GestureResponderEvent, | ||
| } from 'react-native'; | ||
|
|
||
| // External dependencies. | ||
| import Text from '../../../../Texts/Text'; | ||
|
|
@@ -19,6 +24,61 @@ import { | |
| DEFAULT_BUTTONBASE_ICON_SIZE, | ||
| DEFAULT_BUTTONBASE_LABEL_TEXTVARIANT, | ||
| } from './ButtonBase.constants'; | ||
| import { Gesture, GestureDetector } from 'react-native-gesture-handler'; | ||
|
|
||
| const TouchableOpacity = ({ | ||
| onPress, | ||
| disabled, | ||
| children, | ||
| ...props | ||
| }: TouchableOpacityProps & { children?: React.ReactNode }) => { | ||
| // Handle both 'disabled' and 'isDisabled' props for compatibility | ||
| const isDisabled = disabled || (props as { isDisabled?: boolean }).isDisabled; | ||
| const tap = Gesture.Tap() | ||
| .runOnJS(true) | ||
| .onEnd((gestureEvent) => { | ||
| if (onPress && !isDisabled) { | ||
| // Create a proper GestureResponderEvent-like object from gesture event | ||
| const syntheticEvent = { | ||
| nativeEvent: { | ||
| locationX: gestureEvent.x || 0, | ||
| locationY: gestureEvent.y || 0, | ||
| pageX: gestureEvent.absoluteX || 0, | ||
| pageY: gestureEvent.absoluteY || 0, | ||
| timestamp: Date.now(), | ||
| }, | ||
| persist: () => { | ||
| /* no-op for synthetic event */ | ||
| }, | ||
| preventDefault: () => { | ||
| /* no-op for synthetic event */ | ||
| }, | ||
| stopPropagation: () => { | ||
| /* no-op for synthetic event */ | ||
| }, | ||
| } as GestureResponderEvent; | ||
| onPress(syntheticEvent); | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| // Preserve onPress for accessibility (screen readers, keyboard navigation) | ||
| // but ensure it respects disabled state | ||
| const accessibleOnPress = isDisabled ? undefined : onPress; | ||
|
|
||
| return ( | ||
| <GestureDetector gesture={tap}> | ||
| <RNTouchableOpacity | ||
| disabled={isDisabled} | ||
| onPress={accessibleOnPress} // Preserve for accessibility | ||
| {...props} | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| // Ensure disabled prop is available to tests | ||
| {...(process.env.NODE_ENV === 'test' && { disabled: isDisabled })} | ||
| > | ||
|
brianacnguyen marked this conversation as resolved.
|
||
| {children} | ||
| </RNTouchableOpacity> | ||
| </GestureDetector> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Custom Wrapper Causes Visual, Functional, Accessibility IssuesThe custom
cursor[bot] marked this conversation as resolved.
brianacnguyen marked this conversation as resolved.
|
||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Android Button Double Tap IssueOn Android, Additional Locations (3)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Double Tap Issue in Custom TouchableOpacity WrapperOn Android in production environments, a single tap on |
||
| }; | ||
|
brianacnguyen marked this conversation as resolved.
brianacnguyen marked this conversation as resolved.
brianacnguyen marked this conversation as resolved.
|
||
|
|
||
| const ButtonBase = ({ | ||
| label, | ||
|
|
@@ -40,11 +100,31 @@ const ButtonBase = ({ | |
| isDisabled, | ||
| }); | ||
|
|
||
| // Disable gesture wrapper in test environments to prevent test interference | ||
| const isE2ETest = | ||
| process.env.IS_TEST === 'true' || | ||
| process.env.METAMASK_ENVIRONMENT === 'e2e'; | ||
| const isUnitTest = process.env.NODE_ENV === 'test'; | ||
| const TouchableComponent = | ||
| Platform.OS === 'android' && !isE2ETest && !isUnitTest | ||
| ? TouchableOpacity | ||
| : RNTouchableOpacity; | ||
|
|
||
| // Handle disabled state properly in all environments | ||
| // For custom TouchableOpacity (Android), pass original onPress and let it handle disabled state internally | ||
| // For standard TouchableOpacity, apply conditional logic to prevent disabled interaction | ||
| const conditionalOnPress = | ||
| TouchableComponent === TouchableOpacity | ||
| ? onPress | ||
| : isDisabled | ||
| ? undefined | ||
| : onPress; | ||
|
|
||
| return ( | ||
| <TouchableOpacity | ||
| <TouchableComponent | ||
| disabled={isDisabled} | ||
| activeOpacity={1} | ||
| onPress={onPress} | ||
| onPress={conditionalOnPress} | ||
|
cursor[bot] marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Android Button Double Tap IssueOn Android, the |
||
| style={styles.base} | ||
| accessibilityRole="button" | ||
| accessible | ||
|
|
@@ -77,7 +157,7 @@ const ButtonBase = ({ | |
| style={styles.endIcon} | ||
| /> | ||
| )} | ||
| </TouchableOpacity> | ||
| </TouchableComponent> | ||
| ); | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this onPress being called twice?
as onPress on the RNTouchableOpacity will be called as well?
https://github.com/MetaMask/metamask-mobile/pull/18104/files#diff-d8ff46ee74cd57f89c7ac7deb9144c994a7f9db5145060058638387669a72b5aR72