Skip to content
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

Don't passthru key events that override Pressibilty api's #1731

Merged

Conversation

shwanton
Copy link

@shwanton shwanton commented Feb 11, 2023

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

#1623 added support for "Enter" and "Space" as default keys to trigger onKeyDown on Pressability which will call onPress
If the onKeyDown or onKeyUp was passed as a prop to TouchableWithoutFeedback, this would clobber the Pressability callback and the onKeyDown -> onPress chain would never get triggered.

CleanShot 2023-02-10 at 15 57 39

CleanShot.2023-02-10.at.15.33.42.mp4

Pressibility supports calling the onKeyDown|up if it's passed as a prop.

const {onKeyDown} = this._config;
if (onKeyDown != null) {
onKeyDown(event);
}

Changeling

[macOS] [Fix] - Don't passthru key events that override Pressibilty api's

Test Plan

CleanShot.2023-02-10.at.15.32.51.mp4

@shwanton shwanton marked this pull request as ready for review February 13, 2023 16:59
@shwanton shwanton requested a review from a team as a code owner February 13, 2023 16:59
@Saadnajmi
Copy link
Collaborator

Interesting, I didn't realize PASSTHROUGH_PROPS existed. It kinda feels like we shouldn't pass through any of the props handled by Pressability? Should we remove validKeysUp/Down as well?

@shwanton
Copy link
Author

Interesting, I didn't realize PASSTHROUGH_PROPS existed. It kinda feels like we shouldn't pass through any of the props handled by Pressability? Should we remove validKeysUp/Down as well?

If those callbacks are being handled in Pressability, we should remove them from PASSTHROUGH_PROPS to avoid accidental override.

@Saadnajmi
Copy link
Collaborator

FWIW, I personally feel like all the Touchable* components are soft deprecated and I don't want to support them. Buuuut way too many people still use it so I can't pitch that to React Native Core just yet 😅.

In the meantime, I would recommend that you replace existing uses of Touchable with Pressable for the best experience :)

@shwanton
Copy link
Author

FWIW, I personally feel like all the Touchable* components are soft deprecated and I don't want to support them. Buuuut way too many people still use it so I can't pitch that to React Native Core just yet 😅.

In the meantime, I would recommend that you replace existing uses of Touchable with Pressable for the best experience :)

Yes, completely agree and we are slowly working on replacing them with Touchable in our code base!

@shwanton
Copy link
Author

Interesting, I didn't realize PASSTHROUGH_PROPS existed. It kinda feels like we shouldn't pass through any of the props handled by Pressability? Should we remove validKeysUp/Down as well?

Just looked into this. Since validKeysUp/Down can be overridden with props and don't delegate to the underlying property, we shouldn't remove them from PASSTHROUGH_PROPS.

@microsoft-github-policy-service microsoft-github-policy-service bot merged commit 1a95fde into microsoft:main Feb 13, 2023
@shwanton shwanton deleted the fix-keyevent-callback branch February 13, 2023 22:09
shwanton added a commit to shwanton/react-native-macos that referenced this pull request Mar 10, 2023
Summary:
**Context**

Sync GH Change from RND OSS
[Don't passthru key events that override Pressibilty api's microsoft#1731](microsoft#1731)

**Change**

* Don't passthru key events that override Pressibilty api's

Test Plan:
|before|after|
| https://pxl.cl/2vqGM |https://pxl.cl/2vqFX|

Subscribers: generatedunixname89002005327315

Differential Revision: https://phabricator.intern.facebook.com/D43705649
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants