Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bcf7ec6
Updated TouchableOpacity for ButtonBase, ListItemSelect, ListItemMult…
brianacnguyen Aug 8, 2025
0ee868e
Updated disabled interaction
brianacnguyen Aug 8, 2025
a30c9fe
Updated snapshots
brianacnguyen Aug 8, 2025
5ca1135
Added additional tests
brianacnguyen Aug 8, 2025
c6fe1f6
Updated buttons to prevent double press executions
brianacnguyen Aug 8, 2025
0a8420c
Fixed buttons for E2E
brianacnguyen Aug 8, 2025
a0cad6c
Fixed lint errors in tests
brianacnguyen Aug 8, 2025
8375b9a
Fixed children issues
brianacnguyen Aug 8, 2025
5b4aa75
Fixed disabled tests
brianacnguyen Aug 8, 2025
03e0af3
Updated snapshots
brianacnguyen Aug 8, 2025
0601796
Updated more test cases
brianacnguyen Aug 8, 2025
1c79404
Merge branch 'main' of https://github.com/MetaMask/metamask-mobile in…
brianacnguyen Aug 11, 2025
1704f35
Updated snapshots
brianacnguyen Aug 11, 2025
581a757
Updated tests for better coverage for buttons
brianacnguyen Aug 11, 2025
b065a47
Updated button tests
brianacnguyen Aug 11, 2025
2abf75d
Missed a test file
brianacnguyen Aug 11, 2025
e02d75e
Merge branch 'main' into fix/buttons
brianacnguyen Aug 14, 2025
12189a6
Merge branch 'main' of https://github.com/MetaMask/metamask-mobile in…
brianacnguyen Aug 14, 2025
c741d5e
Updated pressed and disabled state
brianacnguyen Aug 14, 2025
c500a79
Merge branch 'fix/buttons' of https://github.com/MetaMask/metamask-mo…
brianacnguyen Aug 14, 2025
253625a
Merge branch 'main' into fix/buttons
brianacnguyen Aug 15, 2025
cecc715
Merge branch 'main' into fix/buttons
brianacnguyen Aug 15, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);

Copy link
Copy Markdown
Contributor

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

}
Comment thread
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}
Comment thread
cursor[bot] marked this conversation as resolved.
// Ensure disabled prop is available to tests
{...(process.env.NODE_ENV === 'test' && { disabled: isDisabled })}
>
Comment thread
brianacnguyen marked this conversation as resolved.
{children}
</RNTouchableOpacity>
</GestureDetector>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Custom Wrapper Causes Visual, Functional, Accessibility Issues

The custom TouchableOpacity wrapper, active on Android, introduces three regressions in ButtonBase (and ListItemSelect/ListItemMultiSelect):

  • Visual: It fails to render children, resulting in empty buttons (no label or icons).
  • Functional: The GestureDetector invokes onPress with an empty event, causing crashes when consumers access event properties.
  • Accessibility: Removing onPress from the underlying RNTouchableOpacity breaks native accessibility activation (e.g., TalkBack, keyboard navigation), making buttons non-activatable.
Fix in Cursor Fix in Web

Comment thread
cursor[bot] marked this conversation as resolved.
Comment thread
brianacnguyen marked this conversation as resolved.
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Android Button Double Tap Issue

On Android, onPress handlers in ButtonBase, ListItemSelect, and ListItemMultiSelect are invoked twice for a single tap. This occurs because the custom TouchableOpacity wrapper simultaneously calls onPress via GestureDetector.Tap().onEnd() and passes onPress to the underlying RNTouchableOpacity (intended for accessibility but not gated). This dual wiring causes duplicate actions and is not caught by standard tests.

Additional Locations (3)
Fix in Cursor Fix in Web

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Double Tap Issue in Custom TouchableOpacity Wrapper

On Android in production environments, a single tap on ButtonBase (and other components using the custom TouchableOpacity wrapper) triggers onPress twice. This is due to onPress being called by both the GestureDetector's tap.onEnd handler and the inner RNTouchableOpacity (via accessibleOnPress). This causes duplicate actions, such as double navigation or submissions. Tests do not detect this as the gesture wrapper is disabled in test environments. The same pattern is present in ListItemSelect.tsx and ListItemMultiSelect.tsx.

Fix in Cursor Fix in Web

};
Comment thread
brianacnguyen marked this conversation as resolved.
Comment thread
brianacnguyen marked this conversation as resolved.
Comment thread
brianacnguyen marked this conversation as resolved.

const ButtonBase = ({
label,
Expand All @@ -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}
Comment thread
cursor[bot] marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Android Button Double Tap Issue

On Android, the ButtonBase component (and similarly ListItemSelect, ListItemMultiSelect) invokes onPress twice for a single tap. This occurs because the custom TouchableOpacity wrapper simultaneously triggers onPress from the GestureDetector's Tap gesture onEnd handler and from the onPress prop passed to the inner RNTouchableOpacity (via accessibleOnPress). This leads to double execution of button actions (e.g., navigation, toggles, submissions). This issue is specific to Android and is not observed in test/e2e environments where the gesture wrapper is disabled.

Fix in Cursor Fix in Web

style={styles.base}
accessibilityRole="button"
accessible
Expand Down Expand Up @@ -77,7 +157,7 @@ const ButtonBase = ({
style={styles.endIcon}
/>
)}
</TouchableOpacity>
</TouchableComponent>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ exports[`ButtonBase should render correctly when disabled 1`] = `
accessible={true}
activeOpacity={1}
disabled={true}
onPress={[Function]}
style={
{
"alignItems": "center",
Expand Down
Loading
Loading