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

[iOS] Fixes selection of single line text input #24714

Closed
wants to merge 2 commits into from

Conversation

zhongwuzw
Copy link
Contributor

Summary

UITextField don't have delegate to observe the selection changes of users(multiline text input UITextView have it), so we can add a KVO to observe selection changes.

@cpojer @shergin

Changelog

[iOS] [Fixed] - Fixes selection of single line text input

Test Plan

Create a single line textInput, then do some selection, onSelectionChange can be called.

@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 May 6, 2019
@react-native-bot react-native-bot added Platform: iOS iOS applications. Bug labels May 6, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thank you!

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.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @zhongwuzw in fc8008e.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 7, 2019
@JoshuaGross
Copy link
Contributor

Hi @zhongwuzw, I'm reverting this because it's causing the following crash:

Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'An instance 0x7fb3453eda00 of class RCTUITextField was deallocated while key value observers were still registered with it.

I believe this is because we're adding a KVO observer to the UITextField but only have a weak reference to it, so it can be deallocated before the observer, causing this crash. Feel free to resubmit the PR with a fix for this crash as well.

@zhongwuzw
Copy link
Contributor Author

@JoshuaGross Oops, I missing that weak declaration, I'll fix it :).

zhongwuzw added a commit to zhongwuzw/react-native that referenced this pull request May 14, 2019
zhongwuzw added a commit to zhongwuzw/react-native that referenced this pull request May 16, 2019
zhongwuzw added a commit to zhongwuzw/react-native that referenced this pull request May 21, 2019
facebook-github-bot pushed a commit that referenced this pull request May 21, 2019
Summary:
Resubmit #24714.
From the #24714 (comment), seems it would crash sometimes. JoshuaGross Hi :) I added a method to let `UITextField` call the cleanup explicitly. Please check again, thanks!

## Changelog

[iOS] [Fixed] - Fixes selection of single line text input
Pull Request resolved: #24840

Differential Revision: D15415793

Pulled By: JoshuaGross

fbshipit-source-id: 2bb32aa8a1fd8fad116177aac760509649c4a423
@banky
Copy link

banky commented Oct 7, 2019

Hey, @zhongwuzw I was wondering if this change will still be merged since it looks like you made a fix for the crashing problem. The issue from #21515 still exists

@zhongwuzw
Copy link
Contributor Author

@BankyA c38f167, v0.61.2 fixes, please check.

M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
Resubmit facebook#24714.
From the facebook#24714 (comment), seems it would crash sometimes. JoshuaGross Hi :) I added a method to let `UITextField` call the cleanup explicitly. Please check again, thanks!

## Changelog

[iOS] [Fixed] - Fixes selection of single line text input
Pull Request resolved: facebook#24840

Differential Revision: D15415793

Pulled By: JoshuaGross

fbshipit-source-id: 2bb32aa8a1fd8fad116177aac760509649c4a423
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants