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 and simplify controlled text selection on Android #55

Conversation

janicduplessis
Copy link

Upstream PR Link

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. 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

Copy link

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

Nice!

@Julesssss Julesssss merged commit 86d09f2 into Expensify:ExpensifyRC1-0.72.0-alpha.0 Jun 6, 2023
@Julesssss
Copy link

Okay,now I need to figure out the release steps.

If updating the base react-native version of the fork:

@Julesssss
Copy link

Julesssss commented Jun 6, 2023

If responding, please do so here instead!


Note: If you aren't changing the base RN version, then you don't have to branch off a tag. Instead, you can just branch off of whatever branch was last used to release the fork, CP your new changes into that branch, and then release.

Cool, I'm pretty sure this applies.

The last release was v0.72.0-rc.1-alpha.0 so I think I need to branch to v0.72.0-rc.1-alpha.1 and just bump the version like this example.

However, I this doesn't align with the steps, because it makes the CP step redundant... So either I need to branch from a different commit, or I should have changed the PR base, or maybe the steps need to be updated. I have no idea which is the case tho.

Find the branch that we last used to publish

Whats confusing me is that the latest release matches the branch name from above: v0.72.0-rc.1-alpha.0. So maybe this hasn't been made the official process yet?: going forward, I'm proposing that we use the convention that the branch name is just Expensify-$version-rc.$rcVersion

Pls halp @roryabraham

dog-confused

@Julesssss
Copy link

Moving the release discussion to this internal issue

@parasharrajat
Copy link
Member

@Julesssss Did you release a new version yet? What is the plan for updating the new version of this fork in NewDot?

@Julesssss
Copy link

We were discussing killing the fork. So I held on the release for now.

@roryabraham
Copy link

roryabraham commented Jun 17, 2023

Just for transparency, I think we have consensus that we'll no longer be using our react-native fork going forward. We should focus our efforts on upstream PRs instead. In this case, the upstream PR already landed so we will just need to wait for the first RC of 0.73 to use this.

@janicduplessis
Copy link
Author

@roryabraham It was cherry picked into 0.72 so we could use it now

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.

4 participants