Allow filtering characters in onChangeText for TextInput #19087
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
react-native-bot
added
📋Release Notes
Component: TextInput
rplankenhorn
force-pushed
the
filterCharactersFix
branch
from
4a6346e to
bae1f02
Compare
rplankenhorn
requested review from
grabbou,
hramos,
janicduplessis,
Kureev,
mhorowitz and
shergin
as code owners
rplankenhorn
force-pushed
the
filterCharactersFix
branch
from
bae1f02 to
bfe0554
Compare
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@rplankenhorn It doesn't work for me. But this can help: #18456 |
|
Hi @tangkunyin. I already commented on that change and it actually breaks a few things with text input so I don’t think it should be merged. I’ll work on creating a sample app so you can view the issue and my fix. |
|
This patch and #18456 both seem to work for me to get react-native-tag-input working reliably again. With this patch I need to also apply #18627 along with it to fix the faulty backspace emitting issue, where #18456 seems to take care of that issue as well. Edit: I dunno maybe something was cached, need #18627 either way now, current fork using this + hamaron's backspace fix. |
|
@rplankenhorn I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
|
Does this PR achieve the same goals as #18456? Can somebody explain the differences? |
|
Applied this patch to react-native |
|
Applied patch to react-native 0.54.2 and works really well, @rplankenhorn . Please merge this pull request. So will use the same. Thanks ! |
|
I also have the same problem, the version is 0.55.4 |
react-native-bot
added
✅Test Plan
and removed
Missing Test Plan
|
Does 892212b solve this problem? The initial issue was constrained to Unicode emoticons yes? |
|
This is a nice patch, thanks @rplankenhorn! After applying, I observed that I could no longer enter strings into uncontrolled password TextInput fields, so I modified the patch to ignore those situations. |
|
What Ashoat said. Do we still need this patch now that 892212b has landed? |
|
Don't think 892212b fixes issue. Or at least, after replacing the patched RCTBaseTextInputShadowView.m with RCTBaseTextInputShadowView.m from master in my 55.3 project, the issue that this patch solves reappears. Haven't tested on 56. |
The value in the native text input would sometimes be respected over the value passed in. This was specifically an issue for text normalization: for example, if we wanted to only allow digits in a text field and a user enters "123a", the attributedText passed in from JS is "123" (since it gets normalized), _previousAttributedText is "123" (since that was the last value updated from the JS, but baseTextInputView.attributedText is "123a" (since that gets updated by the native event). In that scenario, the text in the input never gets normalized. There's a comment warning about not notifying the view of a text change that isn't a real update, but as far as I can tell, this is a non-issue here because RCTBaseTextInputView does its own checks to see if values have changed. This implementation was inspired by PR facebook#19087, but simplifies by removing `_previousAttributedText` entirely and avoids breaking uncontrolled text inputs.
|
I applied this patch to 0.55.4 and is working perfectly so far! Thanks @rplankenhorn! I owe you coffee. |
|
I have tested this patch on 0.56 and it works nice but the filtered characters are showed for a second. This should be handled as on Android, where undesirable characters are not showed. |
|
+1. Using a patch is literally a patch :p |
The value in the native text input would sometimes be respected over the value passed in. This was specifically an issue for text normalization: for example, if we wanted to only allow digits in a text field and a user enters "123a", the attributedText passed in from JS is "123" (since it gets normalized), _previousAttributedText is "123" (since that was the last value updated from the JS, but baseTextInputView.attributedText is "123a" (since that gets updated by the native event). In that scenario, the text in the input never gets normalized. There's a comment warning about not notifying the view of a text change that isn't a real update, but as far as I can tell, this is a non-issue here because RCTBaseTextInputView does its own checks to see if values have changed. This implementation was inspired by PR facebook#19087, but simplifies by removing `_previousAttributedText` entirely and avoids breaking uncontrolled text inputs.
|
In order to move this PR forward, it would be useful to have someone review if it fixes the issue when this patch is applied on top of the changes we have on master right now. We've landed some fixes to related issues since this PR was opened, and I want to make sure the PR is still valid before proceeding. |
|
For my use case, issue appears to be resolved in 0.57! |
|
@hramos I can confirm that filtering issue already resolved in RN 0.57.0 |
The value in the native text input would sometimes be respected over the value passed in. This was specifically an issue for text normalization: for example, if we wanted to only allow digits in a text field and a user enters "123a", the attributedText passed in from JS is "123" (since it gets normalized), _previousAttributedText is "123" (since that was the last value updated from the JS, but baseTextInputView.attributedText is "123a" (since that gets updated by the native event). In that scenario, the text in the input never gets normalized. There's a comment warning about not notifying the view of a text change that isn't a real update, but as far as I can tell, this is a non-issue here because RCTBaseTextInputView does its own checks to see if values have changed. This implementation was inspired by PR facebook#19087, but simplifies by removing `_previousAttributedText` entirely and avoids breaking uncontrolled text inputs.
The value in the native text input would sometimes be respected over the value passed in. This was specifically an issue for text normalization: for example, if we wanted to only allow digits in a text field and a user enters "123a", the attributedText passed in from JS is "123" (since it gets normalized), _previousAttributedText is "123" (since that was the last value updated from the JS, but baseTextInputView.attributedText is "123a" (since that gets updated by the native event). In that scenario, the text in the input never gets normalized. There's a comment warning about not notifying the view of a text change that isn't a real update, but as far as I can tell, this is a non-issue here because RCTBaseTextInputView does its own checks to see if values have changed. This implementation was inspired by PR facebook#19087, but simplifies by removing `_previousAttributedText` entirely and avoids breaking uncontrolled text inputs.
The value in the native text input would sometimes be respected over the value passed in. This was specifically an issue for text normalization: for example, if we wanted to only allow digits in a text field and a user enters "123a", the attributedText passed in from JS is "123" (since it gets normalized), _previousAttributedText is "123" (since that was the last value updated from the JS, but baseTextInputView.attributedText is "123a" (since that gets updated by the native event). In that scenario, the text in the input never gets normalized. There's a comment warning about not notifying the view of a text change that isn't a real update, but as far as I can tell, this is a non-issue here because RCTBaseTextInputView does its own checks to see if values have changed. This implementation was inspired by PR facebook#19087, but simplifies by removing `_previousAttributedText` entirely and avoids breaking uncontrolled text inputs.
Fixes #18874
Test Plan
Create a sample app that performs validation in the onChangeText callback and modifies the characters. (e.g. filters out emojis).
Release Notes
[IOS] [BUGFIX] [TextInput] - Fixed issue with not being able to filter characters in onChangeText.