Fix #18272 TextInput.setNativeProps({text: ''}) to work by magicien · Pull Request #18278 · react/react-native

magicien · 2018-03-08T17:09:55Z

Motivation

Fix#18272. Calling textInputRef.setNativeProps({text: ''}) or textInputRef.clear() should clear the text input.

Test Plan

All tests of yarn run test are passed
Test with the sample app.
- TextInput.clear() and TextInput.setNativeProps({ text: '***' }) worked
- When clear() or setNativeProps() called, onChange/onChangeText wasn't called
  - Same behavior as react 0.53.0
- When non-string values are given to setNativeProps({text: ___}), its behavior is the same as react 0.53.0.
  - Value Type Result
    null same as empty string ''
    undefined nothing changes
    number throw error
    function throw error
    object throw error
- When clear() or setNativeProps() called, attributed text keeps the attributes
- When value prop is set, the text can't be changed

react 0.54.0 (with the bug)

clear() doesn't work from the second time
setNativeProps({text '***'}) doesn't work from the second time
Even when value prop is set, you can change the text

with bugfix

clear() works every time
setNativeProps({text '****'}) works every time

Attributed text with bugfix

The text keeps the attributes (font family, size, color, text align)

`value` prop with bugfix

If value prop is set, the text should not be changed

Release Notes

[IOS] [BUGFIX] [TextInput] - Fix TextInput.clear() and TextInput.setNativeProps({text: ''}) to work

- Add text VIEW_PROPERTY to RCTBaseTextInputViewManager.m - Add text accessor to RCTBaseTextInputView.m

benadamstyles · 2018-03-13T10:56:07Z

What is the status of this? The regression fixed by this PR is holding us back from publishing our app so I'm keen to help out in any way I can!

magicien · 2018-03-13T11:14:27Z

Thank you! I was testing my changes.
I'm going to update this PR and remove "WIP" soon!

magicien · 2018-03-13T14:30:40Z

I added Test Plan and removed "WIP" from the title.
How can I solve the CI error? It seems to have nothing to do with this change.

Reapply attributes of NSAttributedString on the setText method

magicien · 2018-03-13T19:20:42Z

I added an attributed text test. (Thank you @reyalpsirc!)
And it seems that CI errors are fixed.

Apply react#18278 to fix TextInput.clear() bug

shergin

I am afraid, this is not right approach.
text property must be managed by ShadowView, not UIView instance.

magicien · 2018-03-15T05:22:56Z

@shergin Thank you for your review!
I looked into the code again, and I guess the problem is that the value of ShadowView is not sync with user input.
If input events are handled correctly, text property doesn't need to be added to UIView instance.
Let me think how to fix it.

- Remove text property from RCTBaseTextInputView - Add text property accessor to RCTBaseTextInputShadowView

magicien · 2018-03-19T17:39:47Z

I removed text property from RCTBaseTextInputView and added text accessor to RCTBaseTextInputShadowView.
When text native property (or value prop of TextInput component) is explicitly set, text should be compared with the current local text (_localAttributedText) to check if it needs to update.

This change will also fix #18389 (TextInput value not binding on IOS).

ngandhy · 2018-08-31T21:06:30Z

@hramos - See: #18278 (comment) . The requested changes by @shergin 15 days ago was made by original author 12 days ago. Can we get this regression fix merged now? If @shergin is still MIA on the other project this may sit around indefinitely

Thanks!

facebook-github-bot

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

facebook-github-bot

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

rowinbot · 2018-09-10T04:04:09Z

Any news on this?

hramos · 2018-09-10T22:49:30Z

@rowinbot it's imported. Once the reviewer accepts the changes, it will land internally and eventually sync out to github.

react-native-bot · 2018-09-11T00:55:03Z

@magicien merged commit 2307ea6 into facebook:master.

Once this commit is added to a release, you will see the corresponding version tag below the description at 2307ea6. If the commit has a single master tag, it is not yet part of a release.

hramos · 2018-09-11T01:38:55Z

I've turned off these types of notifications from the bot for now.

We check the thread for an existing comment from the bot before adding a new comment. I'll need to look into why this failed on this PR.

Summary: Fix#18272. Calling textInputRef.setNativeProps({text: ''}) or textInputRef.clear() should clear the text input. - All tests of `yarn run test` are passed - Test with [the sample app](https://github.com/magicien/react-native-textinput-clear). - TextInput.clear() and TextInput.setNativeProps({ text: '***' }) worked - When clear() or setNativeProps() called, onChange/onChangeText wasn't called - Same behavior as react 0.53.0 - When non-string values are given to `setNativeProps({text: ___})`, its behavior is the same as react 0.53.0. - Value Type | Result ---------- | ------------ null | same as empty string '' undefined | nothing changes number | throw error function | throw error object | throw error - When clear() or setNativeProps() called, attributed text keeps the attributes - When `value` prop is set, the text can't be changed - `clear()` doesn't work from the second time - `setNativeProps({text '***'})` doesn't work from the second time - Even when `value` prop is set, you can change the text ![ScreenShot_0.54.0](https://raw.githubusercontent.com/magicien/react-native-textinput-clear/master/screenshot/0.54.0_test.gif) - `clear()` works every time - `setNativeProps({text '****'})` works every time ![ScreenShot_Clear_1](https://raw.githubusercontent.com/magicien/react-native-textinput-clear/master/screenshot/clear_test_1.gif) ![ScreenShot_Clear_2](https://raw.githubusercontent.com/magicien/react-native-textinput-clear/master/screenshot/clear_test_2.gif) - The text keeps the attributes (font family, size, color, text align) ![ScreenShot_Slider](https://raw.githubusercontent.com/magicien/react-native-textinput-clear/master/screenshot/attributed_text_test.gif) - If `value` prop is set, the text should not be changed ![ScreenShot_Value](https://raw.githubusercontent.com/magicien/react-native-textinput-clear/master/screenshot/value_test.gif) [IOS] [BUGFIX] [TextInput] - Fix TextInput.clear() and TextInput.setNativeProps({text: ''}) to work Pull Request resolved: #18278 Reviewed By: shergin Differential Revision: D9692561 Pulled By: hramos fbshipit-source-id: b7ce8f6740fdf666e71d6a85743331ca4805edcb

Add text property to RCTBaseTextInputView
3d8e3c7
- Add text VIEW_PROPERTY to RCTBaseTextInputViewManager.m - Add text accessor to RCTBaseTextInputView.m

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 Mar 8, 2018

magicien mentioned this pull request Mar 8, 2018
[0.54] TextInput.setNativeProps({text: ''}) no longer works #18272
Closed

magicien changed the title ~~[WIP][IOS][BUGFIX][TextInput] Fix #18272 TextInput.setNativeProps({text: ''}) to work~~[IOS][BUGFIX][TextInput] Fix #18272 TextInput.setNativeProps({text: ''}) to workMar 13, 2018

magicien changed the title ~~[IOS][BUGFIX][TextInput] Fix #18272 TextInput.setNativeProps({text: ''}) to work~~[iOS][TextInput] Fix #18272 TextInput.setNativeProps({text: ''}) to workMar 13, 2018

reyalpsircand others added 2 commits March 13, 2018 17:58

Reapply attributes of NSAttributedString on the setText method
01440a8

Merge pull request#1from reyalpsirc/patch-2
d7fac1e
Reapply attributes of NSAttributedString on the setText method

hramos added the Platform: iOS iOS applications. label Mar 13, 2018

magicien added a commit to CHANOMA/react-native that referenced this pull request Mar 14, 2018
Applyreact#18278to fix TextInput.clear() bug
ab370f3

This was referenced Mar 14, 2018
Apply #18278 to fix TextInput.clear() bug #18363
Closed
Apply #18278 to fix TextInput.clear() bug CHANOMA/react-native#1
Merged

atsuhiro added a commit to CHANOMA/react-native that referenced this pull request Mar 14, 2018
Merge pull request#1from CHANOMA/feature/fix_textinput_clear
fc91d0e
Apply react#18278 to fix TextInput.clear() bug

shergin self-requested a review March 14, 2018 19:16

shergin suggested changes Mar 14, 2018
View reviewed changes

maraujop mentioned this pull request Mar 15, 2018
TextInput value not binding on IOS #18389
Closed

react-native-bot added Ran Commands One of our bots successfully processed a command. Component: TextInput Related to the TextInput component. Bug Fix 🐛 Platform: iOS iOS applications. labels Mar 16, 2018

Revise: change "text" property handling
1dcb876
- Remove text property from RCTBaseTextInputView - Add text property accessor to RCTBaseTextInputShadowView

magicien mentioned this pull request Mar 20, 2018
Controlled TextInput broken for Chinese (and other languages) in v0.54 on iOS #18403
Closed

magicien added a commit to magicien/react-native that referenced this pull request Mar 20, 2018
Inlucdereact#18278
f13805a

chrisnojima mentioned this pull request Aug 20, 2018
Keybase fixes off 0.57.rc.0 keybase/react-native#7
Closed
1 task

chrisnojima added a commit to keybase/react-native that referenced this pull request Aug 23, 2018
fix setNativeProps:react#18278
14f1582

chrisnojima added a commit to keybase/react-native that referenced this pull request Aug 29, 2018
fix setNativeProps:react#18278
1ec1d9c

This comment has been minimized.
Sign in to view

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 6, 2018

facebook-github-bot approved these changes Sep 6, 2018
View reviewed changes

facebook-github-bot reviewed Sep 7, 2018
View reviewed changes

bhrott mentioned this pull request Sep 8, 2018
iOS allows too many characters bhrott/react-native-masked-text#68
Closed

chrisnojima added a commit to keybase/react-native that referenced this pull request Sep 10, 2018
fix setNativeProps:react#18278
550d771

facebook-github-bot closed this in 2307ea6Sep 11, 2018

react locked as resolved and limited conversation to collaborators Sep 11, 2018

react-native-bot added Merged This PR has been merged. labels Sep 11, 2018

Value Type	Result
null	same as empty string ''
undefined	nothing changes
number	throw error
function	throw error
object	throw error

Uh oh!

Conversation

magicien commented Mar 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Test Plan

react 0.54.0 (with the bug)

with bugfix

Attributed text with bugfix

value prop with bugfix

Release Notes

Uh oh!

benadamstyles commented Mar 13, 2018

Uh oh!

magicien commented Mar 13, 2018

Uh oh!

magicien commented Mar 13, 2018

Uh oh!

magicien commented Mar 13, 2018

Uh oh!

shergin left a comment

Choose a reason for hiding this comment

Uh oh!

magicien commented Mar 15, 2018

Uh oh!

magicien commented Mar 19, 2018

Uh oh!

This comment has been minimized.

ngandhy commented Aug 31, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

rowinbot commented Sep 10, 2018

Uh oh!

hramos commented Sep 10, 2018

Uh oh!

react-native-bot commented Sep 11, 2018

Uh oh!

hramos commented Sep 11, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

magicien commented Mar 8, 2018•
edited
Loading

`value` prop with bugfix

ngandhy commented Aug 31, 2018•
edited
Loading

hramos commented Sep 11, 2018•
edited
Loading