-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[TextInput]: Fixed duplicated letters being added when changing text through onChangeText #27757
Conversation
Does this retain selections when typing in the middle of text? Could you add some videos? |
That is true @JoshuaGross. Append would always put the cursor to the end. Do you have any ideas I could try for this? I debugged this for a while and can't find a solid answer. Maybe a diff check and find the appropriate place to put the cursor? |
@safaiyeh @JoshuaGross I’m not expert at this but here are some suggestions, can we introduce a function that transforms the text at java instead of letting keyboard transform text? Ex. We have autocapitalise option then on changetext we can apply the transformation and forward the updated text to view. Currently user can type in lowercase even if the uppercase is set. Or we create a transformation prop which will update the text at JavaScript and the complete state will be managed at JavaScript. I’m not sure about the performance impact. |
@martianatwork I like those solutions. I will give them a shot! One flaw to having options for on change is custom text transforms. I like the second solution because it can cover both. |
textinput is very tricky. We wrote our own component for rich text input, and just want to add that if this is to be merged, it should be very thoroughly tested. I think some issues will only get resolved once we have sync communication between JS and native (fabric stable). |
Is that component open sourced? @vonovak Ive working on a text input heavy app and the performance kills UX at some point. |
@safaiyeh no, it's not open sourced, and it's highly unlikely that it would be... |
@@ -503,7 +503,8 @@ public void maybeSetText(ReactTextUpdate reactTextUpdate) { | |||
// When we update text, we trigger onChangeText code that will | |||
// try to update state if the wrapper is available. Temporarily disable | |||
// to prevent an infinite loop. | |||
getText().replace(0, length(), spannableStringBuilder); | |||
getText().clear(); | |||
append(spannableStringBuilder); | |||
} |
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.
Pull request safaiyeh#2
Integer currentPosition = getSelectionStart();
getText().clear();
append(spannableStringBuilder);
setSelection(currentPosition);
setSelection
sets the cursor position to the previous one after append operation.
This is some quick testing of the functionality with the react-native master branch. Let me know your feedback, I can publish a separate Pull Request if you are not available otherwise I can futher help you to review your pull request and test the changes.
BEFORE | AFTER |
---|---|
Thanks a lot
Fabrizio Bertoglio
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 have the same issue when using toUpperCase method on the input string. |
How can I apply this as a patch? |
@safaiyeh, @fabriziobertoglio1987 Is this PR still relevant with #29070 open? They seem like they both fix issue #11068. If so, can I close this PR? |
Yup that PR takes care of it. I’ll close this @lunaleaps |
Summary
Fix for #11068.
getText().replace
caused a bug where letters would be duplicated when the text was being set through onChangeText:Debugged and tried a bunch of different ways to solve it explained here: #11068 (comment)
Changelog
[Android] [Fixed] - Fixed duplicated letters being added when changing text through onChangeText
Test Plan
Added this to RNTester:
Verified it works properly and there were no regressions in the other examples.
Note: There is some performance issues when typing for a long time, probably a lag between clearing and appending. Would love to see if someone has suggestions in handling that.
Edit: Seems like the perf is a known issue #20119