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

Fix copy / paste menu and simplify controlled text selection on Android #37424

Closed

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented May 13, 2023

Summary:

Currently when using a TextInput with a controlled selection prop the Copy / Paste menu is constantly getting dismissed and is impossible to use. This is because Android dismisses it when certain method that affect the input text are called (https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/Editor.java;l=1667;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176?q=Editor, https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/TextView.java;l=6792;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176). The solution to fix this is to avoid calling those methods when only the selection changes.

I also noticed there are a lot of differences on how selection is handled in old vs new arch and a lot of the selection handling can actually be removed as it is partially the cause of this issue.

This implements 2 mitigations to avoid the issue:

  • Unify selection handling via commands for old arch, like fabric. Selection is currently a prop in the native component, but it is completely ignored in fabric and selection is set using commands. I removed the selection prop from the native component on Android so now it is exclusively handled with commands like it is currently for fabric. This makes it so that when the selection prop changes the native component no longer re-renders which helps mitigate this issue. More specifically for the old arch we no longer handle the selection prop in ReactTextInputShadowNode, which used to invalidate the shadow node and cause the text to be replaced and the copy / paste menu to close.

  • Only set placeholder if the text value changed. Calling EditText.setHint also causes the copy / paste menu to be dismissed. Fabric will call all props handlers when a single prop changed, so if the selection prop changed the placeholder prop handler would be called too. To fix this we can check that the value changed before calling setHint.

Changelog:

[ANDROID] [FIXED] - Fix copy / paste menu and simplify controlled text selection on Android

Test Plan:

Tested on new and old arch in RNTester example.

Before:

Screen.Recording.2023-05-13.at.11.56.49.mov

After:

Screen.Recording.2023-05-13.at.11.52.16.mov

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels May 13, 2023
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,721,256 -2,912
android hermes armeabi-v7a 8,032,133 -3,043
android hermes x86 9,208,750 -4,057
android hermes x86_64 9,062,024 -3,924
android jsc arm64-v8a 9,285,935 -2,977
android jsc armeabi-v7a 8,474,240 -3,105
android jsc x86 9,344,820 -4,105
android jsc x86_64 9,601,695 -3,986

Base commit: 61335a1
Branch: main

Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

So, we always go through viewCommands.setTextAndSelection()? That makes sense to me I think.

A caveat for this change, because of how Meta ships React native to its own apps, we need to stage this so that the JS change can work with a ~1 week old native build. So, we would need to first land a version where the existing JS platform was compatible with a week old native, platform, before then removing prop-setting, etc from JS.

@janicduplessis
Copy link
Contributor Author

Fabric already uses only view commands, the prop is implemented but never used. For old arch it uses both the prop in shadow node as well as view commands. I will think about how we can roll this out and if this would cause issues.

@janicduplessis
Copy link
Contributor Author

@NickGerleman I tested this and selection still works properly with only the JS changes. Even if we don't pass the selection prop it works since Fabric already doesn't use it and Paper uses both the prop and view commands.

@NickGerleman
Copy link
Contributor

Ah, that makes sense 👍

@facebook-github-bot
Copy link
Contributor

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Great work @janicduplessis !

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label May 24, 2023
@facebook-github-bot
Copy link
Contributor

@NickGerleman merged this pull request in d4f6cf1.

@janicduplessis janicduplessis deleted the @janic/selection-fix branch May 25, 2023 00:26
kelset pushed a commit that referenced this pull request Jun 13, 2023
…id (#37424)

Summary:
Currently when using a TextInput with a controlled selection prop the Copy / Paste menu is constantly getting dismissed and is impossible to use. This is because Android dismisses it when certain method that affect the input text are called (https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/Editor.java;l=1667;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176?q=Editor, https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/TextView.java;l=6792;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176). The solution to fix this is to avoid calling those methods when only the selection changes.

I also noticed there are a lot of differences on how selection is handled in old vs new arch and a lot of the selection handling can actually be removed as it is partially the cause of this issue.

This implements 2 mitigations to avoid the issue:

- Unify selection handling via commands for old arch, like fabric. Selection is currently a prop in the native component, but it is completely ignored in fabric and selection is set using commands. I removed the selection prop from the native component on Android so now it is exclusively handled with commands like it is currently for fabric. This makes it so that when the selection prop changes the native component no longer re-renders which helps mitigate this issue. More specifically for the old arch we no longer handle the `selection` prop in `ReactTextInputShadowNode`, which used to invalidate the shadow node and cause the text to be replaced and the copy / paste menu to close.

- Only set placeholder if the text value changed. Calling `EditText.setHint` also causes the copy / paste menu to be dismissed. Fabric will call all props handlers when a single prop changed, so if the `selection` prop changed the `placeholder` prop handler would be called too. To fix this we can check that the value changed before calling `setHint`.

## Changelog:

[ANDROID] [FIXED] - Fix copy / paste menu and simplify controlled text selection on Android

Pull Request resolved: #37424

Test Plan:
Tested on new and old arch in RNTester example.

Before:

https://github.com/facebook/react-native/assets/2677334/a915b62a-dd79-4adb-9d95-2317780431cf

After:

https://github.com/facebook/react-native/assets/2677334/0dd475ed-8981-410c-8908-f00998dcc425

Reviewed By: cortinico

Differential Revision: D45958425

Pulled By: NickGerleman

fbshipit-source-id: 7b90c1270274f6621303efa60b5398b1a49276ca

# Conflicts:
#	packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputShadowNode.java
Kudo pushed a commit to expo/react-native that referenced this pull request Jun 15, 2023
…id (facebook#37424)

Summary:
Currently when using a TextInput with a controlled selection prop the Copy / Paste menu is constantly getting dismissed and is impossible to use. This is because Android dismisses it when certain method that affect the input text are called (https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/Editor.java;l=1667;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176?q=Editor, https://cs.android.com/android/platform/superproject/+/refs/heads/master:frameworks/base/core/java/android/widget/TextView.java;l=6792;drc=7346c436e5a11ce08f6a80dcfeb8ef941ca30176). The solution to fix this is to avoid calling those methods when only the selection changes.

I also noticed there are a lot of differences on how selection is handled in old vs new arch and a lot of the selection handling can actually be removed as it is partially the cause of this issue.

This implements 2 mitigations to avoid the issue:

- Unify selection handling via commands for old arch, like fabric. Selection is currently a prop in the native component, but it is completely ignored in fabric and selection is set using commands. I removed the selection prop from the native component on Android so now it is exclusively handled with commands like it is currently for fabric. This makes it so that when the selection prop changes the native component no longer re-renders which helps mitigate this issue. More specifically for the old arch we no longer handle the `selection` prop in `ReactTextInputShadowNode`, which used to invalidate the shadow node and cause the text to be replaced and the copy / paste menu to close.

- Only set placeholder if the text value changed. Calling `EditText.setHint` also causes the copy / paste menu to be dismissed. Fabric will call all props handlers when a single prop changed, so if the `selection` prop changed the `placeholder` prop handler would be called too. To fix this we can check that the value changed before calling `setHint`.

## Changelog:

[ANDROID] [FIXED] - Fix copy / paste menu and simplify controlled text selection on Android

Pull Request resolved: facebook#37424

Test Plan:
Tested on new and old arch in RNTester example.

Before:

https://github.com/facebook/react-native/assets/2677334/a915b62a-dd79-4adb-9d95-2317780431cf

After:

https://github.com/facebook/react-native/assets/2677334/0dd475ed-8981-410c-8908-f00998dcc425

Reviewed By: cortinico

Differential Revision: D45958425

Pulled By: NickGerleman

fbshipit-source-id: 7b90c1270274f6621303efa60b5398b1a49276ca

# Conflicts:
#	packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputShadowNode.java
(cherry picked from commit dfc64d5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants