-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 TextInput autocorrect (#7496) #7676
Conversation
TextInput autocorrect was broken by a change to batch event handling in React Native: facebook/react@9f11f8c This fix uses the same approach as facebook@0cd2904 The problem is that TextInput's _onChange handler relied on this.props.value being updated synchronously when calling this.props.onChangeText(text). However, this assumption was broken when React Native event handling started being batched. The fix is to move the code that relies on this.props.value being up-to-date to componentDidUpdate.
By analyzing the blame information on this pull request, we identified @Bhullnatik and @dmmiller to be potential reviewers. |
PR #7651 offers an alternative approach for fixing this. |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to Phabricator to review. |
I'm not able to reproduce the problem that this purports to be fixing. In the TextInput example in UIExplorer, autocorrect seems to be working fine for both single and multi-line fields. Am I missing something? |
Autocorrect definitely isn't working with 0.26.0. When entering text on an iPhone, words are no longer autocorrected, underlined when spelt wrong and the double tap for a full stop doesn't work either. I've applied this as a patch to a custom build of RN and autocorrect functions correctly again. |
@nicklockwood Ah, the bug report was missing a detail. This bug only affects controlled TextInput components and many of the UIExplorer examples are uncontrolled. Here's an example TextInput that would be affected by this bug: <TextInput
autoCorrect={true}
style={{height: 26, width: 100}}
onChangeText={(text) => this.setState({ text })}
value={this.state.text}
/> I updated my PR description to include this detail. |
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
I looked into the bot's concern. It looks like |
Awesome, thanks! |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to Phabricator to review. |
@rigdern. OK, I've managed to repro the issue, and verified that this fixes it, but it's failing to land due to a Flow error (_lastNativeText isn't defined). I'll fix it and try resubmitting. |
26aa27d
Did this get fixed yet? I'm having the same issue on rn 0.27 |
Summary: Autocorrect was broken for controlled TextInput components by a change to batch event handling in React Native: facebook/react@9f11f8c For example, a TextInput like this would be affected by this bug: ```javascript <TextInput autoCorrect={true} style={{height: 26, width: 100}} onChangeText={(text) => this.setState({ text })} value={this.state.text} /> ``` This fix uses the same approach as facebook@0cd2904 The problem is that TextInput's _onChange handler relied on this.props.value being updated synchronously when calling this.props.onChangeText(text). However, this assumption was broken when React Native event handling started being batched. The fix is to move the code that relies on this.props.value being up-to-date to componentDidUpdate. **Test plan (required)** Tested autocorrect now works on iOS in a small app and a large app. Also tested t Closes facebook#7676 Differential Revision: D3346221 Pulled By: nicklockwood fbshipit-source-id: 715df3e8a03aa58cb0a462de4add02289d42782f
Summary: Autocorrect was broken for controlled TextInput components by a change to batch event handling in React Native: facebook/react@9f11f8c For example, a TextInput like this would be affected by this bug: ```javascript <TextInput autoCorrect={true} style={{height: 26, width: 100}} onChangeText={(text) => this.setState({ text })} value={this.state.text} /> ``` This fix uses the same approach as facebook@0cd2904 The problem is that TextInput's _onChange handler relied on this.props.value being updated synchronously when calling this.props.onChangeText(text). However, this assumption was broken when React Native event handling started being batched. The fix is to move the code that relies on this.props.value being up-to-date to componentDidUpdate. **Test plan (required)** Tested autocorrect now works on iOS in a small app and a large app. Also tested t Closes facebook#7676 Differential Revision: D3346221 Pulled By: nicklockwood fbshipit-source-id: 715df3e8a03aa58cb0a462de4add02289d42782f
Autocorrect was broken for controlled TextInput components by a change to batch event handling in React Native:
facebook/react@9f11f8c
For example, a TextInput like this would be affected by this bug:
This fix uses the same approach as
0cd2904
The problem is that TextInput's _onChange handler relied on this.props.value being updated synchronously when calling this.props.onChangeText(text). However, this assumption was broken when React Native event handling started being batched.
The fix is to move the code that relies on this.props.value being up-to-date to componentDidUpdate.
Test plan (required)
Tested autocorrect now works on iOS in a small app and a large app. Also tested that autocorrect works on Android in a small app (I'm not sure whether or not it was broken before this).
@spicyj, thanks for the tip about how to fix this bug. Here's my attempt at fixing it.
Adam Comella
Microsoft Corp