-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Focusable Pressables should take focus when they are clicked on
#15327
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "comment": "Pressables should take focus on press", | ||
| "packageName": "react-native-windows", | ||
| "email": "30809111+acoates-ms@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| /** | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| * @format | ||
| */ | ||
|
|
||
| import type * as React from 'react'; | ||
| import {Insets} from '../../../types/public/Insets'; | ||
| import {ColorValue, StyleProp} from '../../StyleSheet/StyleSheet'; | ||
| import {ViewStyle} from '../../StyleSheet/StyleSheetTypes'; | ||
| import { | ||
| GestureResponderEvent, | ||
| MouseEvent, | ||
| NativeSyntheticEvent, | ||
| TargetedEvent, | ||
| } from '../../Types/CoreEventTypes'; | ||
| import {View} from '../View/View'; | ||
| import {AccessibilityProps} from '../View/ViewAccessibility'; | ||
| import {ViewProps} from '../View/ViewPropTypes'; | ||
|
|
||
| export interface PressableStateCallbackType { | ||
| readonly pressed: boolean; | ||
| } | ||
|
|
||
| export interface PressableAndroidRippleConfig { | ||
| color?: null | ColorValue | undefined; | ||
| borderless?: null | boolean | undefined; | ||
| radius?: null | number | undefined; | ||
| foreground?: null | boolean | undefined; | ||
| } | ||
|
|
||
| export interface PressableProps | ||
| extends AccessibilityProps, | ||
| Omit<ViewProps, 'children' | 'style' | 'hitSlop'> { | ||
| /** | ||
| * Called when the hover is activated to provide visual feedback. | ||
| */ | ||
| onHoverIn?: null | ((event: MouseEvent) => void) | undefined; | ||
|
|
||
| /** | ||
| * Called when the hover is deactivated to undo visual feedback. | ||
| */ | ||
| onHoverOut?: null | ((event: MouseEvent) => void) | undefined; | ||
|
|
||
| /** | ||
| * Called when a single tap gesture is detected. | ||
| */ | ||
| onPress?: null | ((event: GestureResponderEvent) => void) | undefined; | ||
|
|
||
| /** | ||
| * Called when a touch is engaged before `onPress`. | ||
| */ | ||
| onPressIn?: null | ((event: GestureResponderEvent) => void) | undefined; | ||
|
|
||
| /** | ||
| * Called when a touch is released before `onPress`. | ||
| */ | ||
| onPressOut?: null | ((event: GestureResponderEvent) => void) | undefined; | ||
|
|
||
| /** | ||
| * Called when a long-tap gesture is detected. | ||
| */ | ||
| onLongPress?: null | ((event: GestureResponderEvent) => void) | undefined; | ||
|
|
||
| /** | ||
| * Called after the element loses focus. | ||
| * @platform macos windows | ||
| */ | ||
| onBlur?: | ||
| | null | ||
| | ((event: NativeSyntheticEvent<TargetedEvent>) => void) | ||
| | undefined; | ||
|
|
||
| /** | ||
| * Called after the element is focused. | ||
| * @platform macos windows | ||
| */ | ||
| onFocus?: | ||
| | null | ||
| | ((event: NativeSyntheticEvent<TargetedEvent>) => void) | ||
| | undefined; | ||
|
|
||
| /** | ||
| * Either children or a render prop that receives a boolean reflecting whether | ||
| * the component is currently pressed. | ||
| */ | ||
| children?: | ||
| | React.ReactNode | ||
| | ((state: PressableStateCallbackType) => React.ReactNode) | ||
| | undefined; | ||
|
|
||
| /** | ||
| * Whether a press gesture can be interrupted by a parent gesture such as a | ||
| * scroll event. Defaults to true. | ||
| */ | ||
| cancelable?: null | boolean | undefined; | ||
|
|
||
| /** | ||
| * Duration to wait after hover in before calling `onHoverIn`. | ||
| * @platform macos windows | ||
| */ | ||
| delayHoverIn?: number | null | undefined; | ||
|
|
||
| /** | ||
| * Duration to wait after hover out before calling `onHoverOut`. | ||
| * @platform macos windows | ||
| */ | ||
| delayHoverOut?: number | null | undefined; | ||
|
|
||
| /** | ||
| * Duration (in milliseconds) from `onPressIn` before `onLongPress` is called. | ||
| */ | ||
| delayLongPress?: null | number | undefined; | ||
|
|
||
| /** | ||
| * Whether the press behavior is disabled. | ||
| */ | ||
| disabled?: null | boolean | undefined; | ||
|
|
||
| //[ Windows | ||
| /** | ||
| * When the pressable is pressed it will take focus | ||
| * Default value: true | ||
| */ | ||
| focusOnPress?: null | boolean | undefined; | ||
| // Windows] | ||
|
|
||
| /** | ||
| * Additional distance outside of this view in which a press is detected. | ||
| */ | ||
| hitSlop?: null | Insets | number | undefined; | ||
|
|
||
| /** | ||
| * Additional distance outside of this view in which a touch is considered a | ||
| * press before `onPressOut` is triggered. | ||
| */ | ||
| pressRetentionOffset?: null | Insets | number | undefined; | ||
|
|
||
| /** | ||
| * If true, doesn't play system sound on touch. | ||
| */ | ||
| android_disableSound?: null | boolean | undefined; | ||
|
|
||
| /** | ||
| * Enables the Android ripple effect and configures its color. | ||
| */ | ||
| android_ripple?: null | PressableAndroidRippleConfig | undefined; | ||
|
|
||
| /** | ||
| * Used only for documentation or testing (e.g. snapshot testing). | ||
| */ | ||
| testOnly_pressed?: null | boolean | undefined; | ||
|
|
||
| /** | ||
| * Either view styles or a function that receives a boolean reflecting whether | ||
| * the component is currently pressed and returns view styles. | ||
| */ | ||
| style?: | ||
| | StyleProp<ViewStyle> | ||
| | ((state: PressableStateCallbackType) => StyleProp<ViewStyle>) | ||
| | undefined; | ||
|
|
||
| /** | ||
| * Duration (in milliseconds) to wait after press down before calling onPressIn. | ||
| */ | ||
| unstable_pressDelay?: number | undefined; | ||
| } | ||
|
|
||
| // TODO use React.AbstractComponent when available | ||
| export const Pressable: React.ForwardRefExoticComponent< | ||
| PressableProps & React.RefAttributes<View> | ||
| >; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,14 @@ type PressableBaseProps = $ReadOnly<{ | |
| */ | ||
| disabled?: ?boolean, | ||
|
|
||
| // [Windows | ||
| /** | ||
| * When the pressable is pressed it will take focus | ||
| * Default value: true | ||
| */ | ||
| focusOnPress?: ?boolean, | ||
| // Windows] | ||
|
|
||
| /** | ||
| * Additional distance outside of this view in which a press is detected. | ||
| */ | ||
|
|
@@ -245,6 +253,7 @@ function Pressable({ | |
| delayLongPress, | ||
| disabled, | ||
| focusable, | ||
| focusOnPress, // Windows | ||
| hitSlop, | ||
| onBlur, | ||
| onFocus, | ||
|
|
@@ -316,6 +325,16 @@ function Pressable({ | |
| hitSlop, | ||
| }; | ||
|
|
||
| const onPressWithFocus = React.useCallback( | ||
| (args: GestureResponderEvent) => { | ||
| if (focusable !== false && focusOnPress !== false) { | ||
PPatBoyd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| viewRef?.current?.focus(); | ||
| } | ||
| onPress?.(args); | ||
| }, | ||
| [focusOnPress, onPress, focusable], | ||
|
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. Do you need to add viewRef here? I can't remember
Comment on lines
+328
to
+335
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. I think it would be better to this in Pressability, where we already override onPress once.
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. ah.. but we don't have the ref there.. |
||
| ); | ||
|
|
||
| const config = useMemo( | ||
| () => ({ | ||
| cancelable, | ||
|
|
@@ -332,7 +351,7 @@ function Pressable({ | |
| onHoverIn, | ||
| onHoverOut, | ||
| onLongPress, | ||
| onPress, | ||
| onPress: onPressWithFocus, | ||
| onPressIn(event: GestureResponderEvent): void { | ||
| if (android_rippleConfig != null) { | ||
| android_rippleConfig.onPressIn(event); | ||
|
|
@@ -378,7 +397,7 @@ function Pressable({ | |
| onHoverIn, | ||
| onHoverOut, | ||
| onLongPress, | ||
| onPress, | ||
| onPressWithFocus, | ||
| onPressIn, | ||
| onPressMove, | ||
| onPressOut, | ||
|
|
||
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.
if the default is true should this prop become
disableFocusOnPress?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.
I prefer to avoid negative props. - But I see an argument for avoiding default true props too.
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 also prefer not having bool in the name, I don't like that we have
enableFocusRinginstead something likefocusRingVisible. I also think it wouldn't be true by default except on windows..