diff --git a/src/components/IconButton/IconButton.tsx b/src/components/IconButton/IconButton.tsx index 270c9289ac..4086f9fd67 100644 --- a/src/components/IconButton/IconButton.tsx +++ b/src/components/IconButton/IconButton.tsx @@ -202,7 +202,10 @@ const IconButton = ({ testID={testID} {...rest} > - + {loading ? ( ) : ( diff --git a/src/components/TextInput/TextInputIcon.tsx b/src/components/TextInput/TextInputIcon.tsx index 4fcd7b6e18..eeb7854b10 100644 --- a/src/components/TextInput/TextInputIcon.tsx +++ b/src/components/TextInput/TextInputIcon.tsx @@ -7,6 +7,7 @@ import { styles } from './styles'; import { getIconColor } from './utils'; import { useInternalTheme } from '../../core/theming'; import type { $Omit } from '../../types'; +import hasTouchHandler from '../../utils/hasTouchHandler'; import IconButton from '../IconButton/IconButton'; export type TextInputAccessoryProps = { @@ -78,7 +79,16 @@ const TextInputIcon = ({ isDisabled: disabled, }); - const onPressHandler = disabled ? undefined : onPress; + // A decorative icon is not a control, so `disabled` would only announce it as + // a disabled button. Must match TouchableRipple's predicate, or an icon with + // only `onLongPress` stays pressable on a disabled field. + const { onLongPress, onPressIn, onPressOut } = rest; + const isInteractive = hasTouchHandler({ + onPress, + onLongPress, + onPressIn, + onPressOut, + }); return ( @@ -87,8 +97,17 @@ const TextInputIcon = ({ icon={icon} iconColor={color} size={iconSize} - style={[styles.icon, style]} - onPress={onPressHandler} + style={[ + styles.icon, + style, + // TextInput already dims the whole accessory when the field is + // disabled. Forwarding `disabled` makes IconButton dim the icon too, + // and the two multiply, so let IconButton own it and cancel the outer + // one. A decorative icon keeps it, nothing else dims it. + isInteractive && disabled ? styles.notDimmed : null, + ]} + disabled={isInteractive ? disabled : undefined} + onPress={onPress} /> ); diff --git a/src/components/TextInput/styles.ts b/src/components/TextInput/styles.ts index 0a65662a5a..fc80723c6e 100644 --- a/src/components/TextInput/styles.ts +++ b/src/components/TextInput/styles.ts @@ -73,6 +73,9 @@ export const styles = StyleSheet.create({ icon: { margin: 0, }, + notDimmed: { + opacity: 1, + }, }); export const filledStyles = StyleSheet.create({ diff --git a/src/components/TouchableRipple/TouchableRipple.native.tsx b/src/components/TouchableRipple/TouchableRipple.native.tsx index 513355afa5..68f556fc7e 100644 --- a/src/components/TouchableRipple/TouchableRipple.native.tsx +++ b/src/components/TouchableRipple/TouchableRipple.native.tsx @@ -1,5 +1,5 @@ import * as React from 'react'; -import { Platform, StyleSheet, View } from 'react-native'; +import { Animated, Platform, StyleSheet, View } from 'react-native'; import type { PressableAndroidRippleConfig, StyleProp, @@ -15,7 +15,9 @@ import { SettingsContext } from '../../core/settings'; import type { Settings } from '../../core/settings'; import { useInternalTheme } from '../../core/theming'; import type { ThemeProp } from '../../types'; -import hasTouchHandler from '../../utils/hasTouchHandler'; +import hasTouchHandler, { + ACTIVATABLE_ROLES, +} from '../../utils/hasTouchHandler'; const ANDROID_VERSION_LOLLIPOP = 21; const ANDROID_VERSION_PIE = 28; @@ -61,7 +63,18 @@ const TouchableRipple = ({ onPressOut, }); - const disabled = disabledProp || !hasPassedTouchHandler; + // With no touch handler and no explicit disabled this is not a control, so it + // renders as a plain View. A Pressable is wrong here either way: keep the old + // disabled flag and it gets announced as a disabled control, drop the flag and + // it starts claiming the touch, swallowing taps meant for whatever wraps it. + const isControl = hasPassedTouchHandler || Boolean(disabledProp); + const isInteractive = hasPassedTouchHandler && !disabledProp; + + // `role` wins over `accessibilityRole` on every platform, so resolve it the + // same way instead of testing both. + const effectiveRole: string | undefined = rest.role ?? rest.accessibilityRole; + const claimsActivatable = + effectiveRole !== undefined && ACTIVATABLE_ROLES.includes(effectiveRole); const { calculatedRippleColor, calculatedUnderlayColor } = getTouchableRippleColors({ @@ -78,21 +91,46 @@ const TouchableRipple = ({ const useForeground = Platform.OS === 'android' && Platform.Version >= ANDROID_VERSION_PIE; + const containerStyle = TouchableRipple.supported + ? [useForeground && styles.overflowHidden, style] + : [borderless && styles.overflowHidden, style]; + + if (!isControl) { + return ( + , which is tabbable by default. Nothing to activate here. + focusable={rest.focusable ?? false} + style={containerStyle} + > + {React.Children.only(children)} + + ); + } + if (TouchableRipple.supported) { - const androidRipple = rippleEffectEnabled - ? (background ?? { - color: calculatedRippleColor, - borderless, - foreground: useForeground, - }) - : undefined; + const androidRipple = + rippleEffectEnabled && isInteractive + ? (background ?? { + color: calculatedRippleColor, + borderless, + foreground: useForeground, + }) + : undefined; return ( {React.Children.only(children)} @@ -104,12 +142,12 @@ const TouchableRipple = ({ {({ pressed }) => ( <> - {pressed && rippleEffectEnabled && ( + {pressed && rippleEffectEnabled && isInteractive && ( void; /** @@ -129,6 +132,26 @@ const TouchableRipple = ({ const { onPress, onLongPress, onPressIn, onPressOut } = rest; + const hasPassedTouchHandler = hasTouchHandler({ + onPress, + onLongPress, + onPressIn, + onPressOut, + }); + + // With no touch handler and no explicit disabled this is not a control, so it + // renders as a plain View. A Pressable is wrong here either way: keep the old + // disabled flag and it gets announced as a disabled control, drop the flag and + // it starts claiming the touch, swallowing taps meant for whatever wraps it. + const isControl = hasPassedTouchHandler || Boolean(disabledProp); + const isInteractive = hasPassedTouchHandler && !disabledProp; + + // `role` wins over `accessibilityRole` on every platform, so resolve it the + // same way instead of testing both. + const effectiveRole: string | undefined = rest.role ?? rest.accessibilityRole; + const claimsActivatable = + effectiveRole !== undefined && ACTIVATABLE_ROLES.includes(effectiveRole); + const handlePressIn = React.useCallback( (e: any) => { onPressIn?.(e); @@ -264,14 +287,34 @@ const TouchableRipple = ({ [onPressOut, rippleEffectEnabled] ); - const hasPassedTouchHandler = hasTouchHandler({ - onPress, - onLongPress, - onPressIn, - onPressOut, - }); - - const disabled = disabledProp || !hasPassedTouchHandler; + if (!isControl) { + const state = { pressed: false, hovered: false, focused: false }; + + return ( + , which is tabbable by default. Nothing to activate here. + focusable={rest.focusable ?? false} + style={[ + styles.touchable, + borderless && styles.borderless, + styles.disabled, + typeof style === 'function' ? style(state) : style, + ]} + > + {React.Children.only( + typeof children === 'function' ? children(state) : children + )} + + ); + } return ( [ styles.touchable, borderless && styles.borderless, // focused state is not ready yet: https://github.com/necolas/react-native-web/issues/1849 // state.focused && { backgroundColor: ___ }, - state.hovered && { backgroundColor: hoverColor }, - disabled && styles.disabled, + state.hovered && isInteractive && { backgroundColor: hoverColor }, + !isInteractive && styles.disabled, typeof style === 'function' ? style(state) : style, ]} > diff --git a/src/components/__tests__/Appbar/__snapshots__/Appbar.test.tsx.snap b/src/components/__tests__/Appbar/__snapshots__/Appbar.test.tsx.snap index a5d9d95766..2d60c6aeb7 100644 --- a/src/components/__tests__/Appbar/__snapshots__/Appbar.test.tsx.snap +++ b/src/components/__tests__/Appbar/__snapshots__/Appbar.test.tsx.snap @@ -122,28 +122,11 @@ exports[`Appbar does not pass any additional props to Searchbar 1`] = ` testID="search-bar-icon-container" > @@ -185,6 +152,7 @@ exports[`Appbar does not pass any additional props to Searchbar 1`] = ` "opacity": 1, } } + testID="search-bar-icon-icon-opacity" > { expect(tree).toMatchSnapshot(); }); -it('renders disabled button if there is no touch handler passed', async () => { - await render(); +it('does not mark a button without a touch handler as disabled', async () => { + await render(); + + expect(screen.getByTestId('plain-button')).not.toBeDisabled(); +}); + +it('renders disabled button when the disabled prop is passed', async () => { + await render( + + ); expect(screen.getByTestId('disabled-button')).toBeDisabled(); }); diff --git a/src/components/__tests__/Checkbox/__snapshots__/Checkbox.test.tsx.snap b/src/components/__tests__/Checkbox/__snapshots__/Checkbox.test.tsx.snap index 54f2e4f7a4..208b23644d 100644 --- a/src/components/__tests__/Checkbox/__snapshots__/Checkbox.test.tsx.snap +++ b/src/components/__tests__/Checkbox/__snapshots__/Checkbox.test.tsx.snap @@ -2,55 +2,25 @@ exports[`renders Checkbox with custom testID 1`] = ` @@ -188,55 +158,25 @@ exports[`renders Checkbox with custom testID 1`] = ` exports[`renders checked Checkbox with color 1`] = ` { expect(tree).toMatchSnapshot(); }); -it('renders disabled chip if there is no touch handler passed', async () => { - await render(Disabled chip); +it('does not mark a chip without a touch handler as disabled', async () => { + await render(Plain chip); + + expect(screen.getByTestId('plain-chip')).not.toBeDisabled(); +}); + +it('renders disabled chip when the disabled prop is passed', async () => { + await render( + {}} testID="disabled-chip"> + Disabled chip + + ); expect(screen.getByTestId('disabled-chip')).toBeDisabled(); }); diff --git a/src/components/__tests__/IconButton.test.tsx b/src/components/__tests__/IconButton.test.tsx index b28456c5ce..884f68b6a0 100644 --- a/src/components/__tests__/IconButton.test.tsx +++ b/src/components/__tests__/IconButton.test.tsx @@ -97,6 +97,21 @@ describe('getIconButtonColor - icon color', () => { }); }); + it('should let the disabled color outrank an explicit icon color', () => { + // matches Button's `customTextColor && !disabled` and the documented + // `onSurfaceDisabled` + expect( + getIconButtonColor({ + theme: getTheme(), + disabled: true, + customIconColor: 'purple', + }) + ).toMatchObject({ + iconColor: getTheme().colors.onSurface, + iconOpacity: stateOpacity.disabled, + }); + }); + it('should return correct disabled color, for theme version 3', () => { expect( getIconButtonColor({ diff --git a/src/components/__tests__/MenuItem.test.tsx b/src/components/__tests__/MenuItem.test.tsx index ba66e705a7..6e225276d7 100644 --- a/src/components/__tests__/MenuItem.test.tsx +++ b/src/components/__tests__/MenuItem.test.tsx @@ -54,7 +54,9 @@ describe('Menu Item', () => { }); it('accepts aria-checked prop', async () => { - await render(); + await render( + {}} title="Option 1" /> + ); expect(screen.getByRole('menuitem')).toHaveProp( 'accessibilityState', diff --git a/src/components/__tests__/RadioButton/__snapshots__/RadioButton.test.tsx.snap b/src/components/__tests__/RadioButton/__snapshots__/RadioButton.test.tsx.snap index c20910f20e..b77c6bc6e0 100644 --- a/src/components/__tests__/RadioButton/__snapshots__/RadioButton.test.tsx.snap +++ b/src/components/__tests__/RadioButton/__snapshots__/RadioButton.test.tsx.snap @@ -7,7 +7,7 @@ exports[`RadioButton RadioButton with custom testID renders properly 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -94,7 +94,7 @@ exports[`RadioButton on default platform renders properly 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -180,7 +180,7 @@ exports[`RadioButton on ios platform renders properly 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -266,7 +266,7 @@ exports[`RadioButton when RadioButton is wrapped by RadioButtonContext.Provider { "busy": undefined, "checked": true, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } diff --git a/src/components/__tests__/RadioButton/__snapshots__/RadioButtonGroup.test.tsx.snap b/src/components/__tests__/RadioButton/__snapshots__/RadioButtonGroup.test.tsx.snap index 1ae7f560be..f04cb343f3 100644 --- a/src/components/__tests__/RadioButton/__snapshots__/RadioButtonGroup.test.tsx.snap +++ b/src/components/__tests__/RadioButton/__snapshots__/RadioButtonGroup.test.tsx.snap @@ -10,7 +10,7 @@ exports[`RadioButtonGroup renders properly 1`] = ` { "busy": undefined, "checked": true, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } diff --git a/src/components/__tests__/RadioButton/__snapshots__/RadioButtonItem.test.tsx.snap b/src/components/__tests__/RadioButton/__snapshots__/RadioButtonItem.test.tsx.snap index 5867b1408c..e16cb79ab4 100644 --- a/src/components/__tests__/RadioButton/__snapshots__/RadioButtonItem.test.tsx.snap +++ b/src/components/__tests__/RadioButton/__snapshots__/RadioButtonItem.test.tsx.snap @@ -7,7 +7,7 @@ exports[`can render leading radio button control 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -61,7 +61,7 @@ exports[`can render leading radio button control 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -185,7 +185,7 @@ exports[`can render the Android radio button on different platforms 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -275,7 +275,7 @@ exports[`can render the Android radio button on different platforms 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -337,7 +337,7 @@ exports[`can render the iOS radio button on different platforms 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -427,7 +427,7 @@ exports[`can render the iOS radio button on different platforms 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -515,7 +515,7 @@ exports[`renders unchecked 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } @@ -605,7 +605,7 @@ exports[`renders unchecked 1`] = ` { "busy": undefined, "checked": false, - "disabled": false, + "disabled": undefined, "expanded": undefined, "selected": undefined, } diff --git a/src/components/__tests__/TextInput.test.tsx b/src/components/__tests__/TextInput.test.tsx index b87e482c7f..7e2342edef 100644 --- a/src/components/__tests__/TextInput.test.tsx +++ b/src/components/__tests__/TextInput.test.tsx @@ -240,6 +240,108 @@ it('disables TextInput.Icon when the field is disabled', async () => { expect(buttons[1]).toBeDisabled(); }); +it('does not expose a handler-less TextInput.Icon as a control when the field is disabled', async () => { + await render( + {}} + disabled + startAccessory={(props: TextInputAccessoryProps) => ( + + )} + endAccessory={(props: TextInputAccessoryProps) => ( + {}} + testID="clear" + /> + )} + /> + ); + + const decorative = screen.getByTestId('decorative'); + expect(decorative).toHaveProp('role', 'none'); + expect(decorative).not.toBeDisabled(); + + // control: a real handler still gets disabled + const clear = screen.getByTestId('clear'); + expect(clear).toHaveProp('role', 'button'); + expect(clear).toBeDisabled(); +}); + +it('dims a disabled TextInput.Icon exactly once', async () => { + await render( + {}} + disabled + startAccessory={(props: TextInputAccessoryProps) => ( + + )} + endAccessory={(props: TextInputAccessoryProps) => ( + {}} + testID="clear" + /> + )} + /> + ); + + const disabledOpacity = tokens.md.sys.state.opacity.disabled; + + // TextInput dims the accessory wrapper and IconButton dims a disabled icon. + // Both firing multiplies to 0.14, so exactly one has to apply per icon. + // decorative: IconButton is not disabled, so the wrapper carries the dim + expect(screen.getByTestId('decorative-container-outer-layer')).toHaveStyle({ + opacity: disabledOpacity, + }); + expect(screen.getByTestId('decorative-icon-opacity')).toHaveStyle({ + opacity: 1, + }); + + // interactive: IconButton carries it, so the wrapper is cancelled + expect(screen.getByTestId('clear-container-outer-layer')).toHaveStyle({ + opacity: 1, + }); + expect(screen.getByTestId('clear-icon-opacity')).toHaveStyle({ + opacity: disabledOpacity, + }); +}); + +it('disables a TextInput.Icon that only has onLongPress when the field is disabled', async () => { + const onLongPress = jest.fn<(e: GestureResponderEvent) => void>(); + await render( + {}} + disabled + endAccessory={(props: TextInputAccessoryProps) => ( + + )} + /> + ); + + // onLongPress alone still makes this a control; keying off onPress would + // leave it live + const long = screen.getByTestId('long'); + expect(long).toHaveProp('role', 'button'); + expect(long).toBeDisabled(); + + await userEvent.longPress(long); + expect(onLongPress).not.toHaveBeenCalled(); +}); + it('does not disable TextInput.Icon when the field is read-only (editable false)', async () => { await render( { expect(onPress).not.toHaveBeenCalled(); }); + it('is not exposed as a control when no touch handler is passed', async () => { + await render( + + Not a button + + ); + + const plain = screen.getByTestId('plain'); + + expect(plain).not.toBeDisabled(); + // no Pressable underneath, so nothing claims the touch. A responder here + // would swallow taps meant for whatever wraps this, e.g. TextInput's + // press to focus. + expect(plain).not.toHaveProp('onStartShouldSetResponder'); + expect(plain).toHaveProp('focusable', false); + }); + + it('drops a button role when no touch handler is passed', async () => { + await render( + + Not a button + + ); + + const plain = screen.getByTestId('plain'); + + // react-native-web turns role button into a real