Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion src/components/IconButton/IconButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ const IconButton = ({
testID={testID}
{...rest}
>
<View style={{ opacity: iconOpacity }}>
<View
testID={`${testID}-icon-opacity`}
style={{ opacity: iconOpacity }}
>
{loading ? (
<ActivityIndicator size={size} color={iconColor} />
) : (
Expand Down
25 changes: 22 additions & 3 deletions src/components/TextInput/TextInputIcon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 (
<View style={styles.iconWrapper}>
Expand All @@ -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}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what do you think about making it non-accessible when it has no onPress?

Suggested change
onPress={onPress}
onPress={onPress}
accessible={Boolean(onPress)}
role={onPress ? 'button' : 'none'}

updated snapshot still has accessible={true} & role="button". RN Web maps that role to a native <button>, focusable={false} only removes it from tab order.
that's why screen readers may still announce actionable button that does nothing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep, RNW does map role="button" to a real , focusable={false} only adds tabindex="-1". Measured before: web , Android Button clickable=true. Same on iOS.
but i fixed in TouchableRipple instead of here. IconButton, Drawer.Item and Drawer.CollapsedItem all set role="button" unconditionally with an optional onPress, so fixing it here would leave two others. AppbarContent already gates its role on onPress.
Only the role is dropped, the element stays. accessible={false} is a noop on web anyway (not in RNW's forwardedProps) and on naitve it kills aria-label, which for an icon-only FAB or IconButton is the only thing it has to announce. dropping the role alone is enough on both platforms
Covers button, imagebutton, link, menuitem and tab. checkbox, radio and switch are kept, they still describe a read-only view.
After fix: web

, Android plain View clickable=false, clear button unchanged on both.

/>
</View>
);
Expand Down
3 changes: 3 additions & 0 deletions src/components/TextInput/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ export const styles = StyleSheet.create({
icon: {
margin: 0,
},
notDimmed: {
opacity: 1,
},
});

export const filledStyles = StyleSheet.create({
Expand Down
68 changes: 53 additions & 15 deletions src/components/TouchableRipple/TouchableRipple.native.tsx
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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;
Expand Down Expand Up @@ -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({
Expand All @@ -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 (
<Animated.View
{...rest}
ref={ref}
// Pressable defaults this to true, so keep it to preserve any role and
// state the caller set, e.g. a read only checked CheckboxItem.
accessible={rest.accessible !== false}
// Drop the claim, keep the element so its label is still announced.
role={claimsActivatable ? 'none' : rest.role}
// A consumer role of button makes react-native-web render a real
// <button>, which is tabbable by default. Nothing to activate here.
focusable={rest.focusable ?? false}
style={containerStyle}
>
{React.Children.only(children)}
</Animated.View>
);
}

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 (
<Pressable
{...rest}
ref={ref}
disabled={disabled}
style={[useForeground && styles.overflowHidden, style]}
disabled={disabledProp}
style={containerStyle}
android_ripple={androidRipple}
>
{React.Children.only(children)}
Expand All @@ -104,12 +142,12 @@ const TouchableRipple = ({
<Pressable
{...rest}
ref={ref}
disabled={disabled}
style={[borderless && styles.overflowHidden, style]}
disabled={disabledProp}
style={containerStyle}
>
{({ pressed }) => (
<>
{pressed && rippleEffectEnabled && (
{pressed && rippleEffectEnabled && isInteractive && (
<View
testID="touchable-ripple-underlay"
style={[
Expand Down
71 changes: 57 additions & 14 deletions src/components/TouchableRipple/TouchableRipple.tsx
Original file line number Diff line number Diff line change
@@ -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 {
ColorValue,
GestureResponderEvent,
Expand All @@ -16,7 +16,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';

export type Props = PressableProps & {
/**
Expand All @@ -37,7 +39,8 @@ export type Props = PressableProps & {
*/
disabled?: boolean;
/**
* Function to execute on press. If not set, will cause the touchable to be disabled.
* Function to execute on press. If not set, the touchable renders as a plain
* container and is not exposed as a control.
*/
onPress?: (e: GestureResponderEvent) => void;
/**
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -264,29 +287,49 @@ 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 (
<Animated.View
{...rest}
ref={ref}
// Pressable defaults this to true, so keep it to preserve any role and
// state the caller set, e.g. a read only checked CheckboxItem.
accessible={rest.accessible !== false}
// Drop the claim, keep the element so its label is still announced.
role={claimsActivatable ? 'none' : rest.role}
// A consumer role of button makes react-native-web render a real
// <button>, 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
)}
</Animated.View>
);
}

return (
<Pressable
{...rest}
ref={ref}
onPressIn={handlePressIn}
onPressOut={handlePressOut}
disabled={disabled}
disabled={disabledProp}
style={(state) => [
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,
]}
>
Expand Down
Loading