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 autocorrect onChangeText firing (#22691) #23666

Closed
wants to merge 3 commits into from

Conversation

ericlewis
Copy link
Contributor

@ericlewis ericlewis commented Feb 27, 2019

Summary

This fixes an issue where autocorrect changes may cause onChangeText to not fire. It subscribes to the underlying textStorage of the UITextView. It is possible this fixes a few other issues, due to the nature of listening to the underlying textStorage changes.

Changelog

[iOS] [Fixed] - iOS firing onChangeText when choosing autocorrect options

Test Plan

Open RNTester, and interact with textfields.

@ericlewis ericlewis added Platform: iOS iOS applications. Component: TextInput Related to the TextInput component. labels Feb 27, 2019
@ericlewis ericlewis self-assigned this Feb 27, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 27, 2019
@ericlewis
Copy link
Contributor Author

fixes #22691

@ericlewis
Copy link
Contributor Author

Would appreciate some extra eyes on this, it appears to work, though.

@shergin
Copy link
Contributor

shergin commented Feb 28, 2019

cc @rigdern, @mandrigin

That's really cool. I didn't know that the API that we use here exists! That also probably can help us to fix other Korean/Japanese/Chinese issues.

I will test it and hopefully land soon.

didProcessEditing:(__unused NSTextStorageEditActions)editedMask
range:(__unused NSRange)editedRange
changeInLength:(__unused NSInteger)delta {
[_backedTextInputView.textInputDelegate textInputDidChange];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am worried a bit about over-firing. So, e.g. if only text attributes were changed, we don't want to send an event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check if this is the case & fix it if that does occur.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@ericlewis
Copy link
Contributor Author

@shergin I changed this PR- I originally removed code from the UITextField adapter implementation, this was a mistake I believe. So, listening to the NSTextStorage only applies to multiline TextInput components. I feel like it would make sense for us to use UITextView instead of UITextField for all TextInput's due to UITextView having lower level API access.

Copy link
Contributor

@zhongwuzw zhongwuzw left a comment

Choose a reason for hiding this comment

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

Give some my opinions:

  1. - textStorage:didProcessEditing:range:changeInLength: delegate would not be called, we should set textStorage.delegate to adapter.
  2. textStorage is created and managed by UITextView, so is it really safe to change the delegate?
  3. - textStorage:didProcessEditing:range:changeInLength: would be called when text attributes changed, I think it may not what we want.
  4. - textStorage:didProcessEditing:range:changeInLength: would always be called, for example, we set defaultValue props for initial value, it should't be called onChange.
  5. If all things above fixes, singleline textinput still has issues. Because this PR try to fix multiline textinput.

@ericlewis
Copy link
Contributor Author

@zhongwuzw ah yes, I forgot to set the delegate in my cleaned up version. I will fix that. Good point on the remaining issues.

@ericlewis ericlewis closed this Mar 4, 2019
@facebook-github-bot facebook-github-bot added the Contributor A React Native contributor. label Apr 15, 2024
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. Component: TextInput Related to the TextInput component. Contributor A React Native contributor. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants