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

TextInput component fix maxLength property when entering characters at the beginning. #21798

Closed
wants to merge 1 commit into from
Closed

TextInput component fix maxLength property when entering characters at the beginning. #21798

wants to merge 1 commit into from

Conversation

MadeinFrance
Copy link

@MadeinFrance MadeinFrance commented Oct 15, 2018

Fix TextInput component and the maxLength property when entering characters at the beginning. (#21639).

Improvements: In TextInputTest.js We could place the native cursor at the beginning of the input in the function componentDidMount. Not sure how to do that from the JavaScript.

Summary:

  • In RCTBaseTextInputView.m the ivar _predictedText is not updated correctly when inserting at the beginning.
  • The actual fix is to update the value before the checking the text mismatch from backedTextInputView.

Test Plan:

  • When entering 1 character at the beginning of the TextInput, we expect to have only 1 character inserted. However we experience that the character is inserted and the current value is duplicated.
  • Insert 0 character before the value 123 with: <TextInput maxLength={5} />.
    broken-input

Release Notes:

[IOS] [BUGFIX] [TextInput] - Fix maxLength property when entering characters at the beginning.

Result:

ios-input-fixed

@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 Oct 15, 2018
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.
  • flow found some issues.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.
  • flow found some issues.

throw new Error('TestModule.verifySnapshot not defined.');
}

updateText();

Choose a reason for hiding this comment

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

no-undef: 'updateText' is not defined.

throw new Error('TestModule.verifySnapshot not defined.');
}

updateText();

Choose a reason for hiding this comment

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

Cannot resolve name updateText.

@react-native-bot react-native-bot added Component: TextInput Related to the TextInput component. 🔶Components labels Oct 15, 2018
…acters at the beginning. (#21639)

Summary:
----------
- In RCTBaseTextInputView.m the ivar _predictedText is not updated correctly when inserting at the beginning.
- The actual fix is to update the value before the checking the text mismatch from `backedTextInputView`.

Test Plan:
----------
- When entering 1 character at the beginning of the TextInput, we expect to have only 1 character inserted. However we experience that the character is inserted and the current value is duplicated.
- Insert 1 character before the value "123" with: `<TextInput maxLength={5} defaultValue="123" />`.

Release Notes:
--------------
[IOS] [BUGFIX] [TextInput] - Fix maxLength property when entering characters at the beginning.
@shergin
Copy link
Contributor

shergin commented Oct 15, 2018

@MadeinFrance Thank you!
Could you please describe why exactly your fix fixes things (doing them right)?

@MadeinFrance
Copy link
Author

MadeinFrance commented Oct 16, 2018

Thank you for the feedback, I appreciate that.

Lets use the TestCase I described in the description (<TextInput maxLength={5} /> and try to insert 0 at the beginning in order to have 0123 as value.)


What happen when inserting 0:

  1. RCTBackedTextInputDelegateAdapter.shouldChangeCharactersInRange triggers and unfortunately override the ivar _predictedText to 0.
  2. At the same time, from BackedInputDelegateAdapter, textFieldDidChange delegate is called.

The bug is that the BaseTextInputView (line) will find a mismatch between 0123 and 0 and try to add a replacementText of 123 at NSRange: location=1, length=0.

  1. The error result will be 01123.

if (_predictedText) {
    _predictedText = [_predictedText stringByReplacingCharactersInRange:range withString:text];
}

@MadeinFrance
Copy link
Author

Fixed in a later commit. Closing.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants