[iOS] Fix TextInput maxLength when insert characters at begin

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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

[zhongwuzw]

Summary

Fixes#21639 , seems we tried to fix this before, please see related PR like D10392176, #18627, but they don't solve it totally.

Changelog

[iOS] [Fixed] - Fix TextInput maxLength when insert characters at begin

Test Plan

From the git log, we faced some issues, like Backspace event, we need to ensure no regression happened, issues like #21639#18627 should be fixed.

[shergin]

@rigdern Adam, what do you think?

[rigdern]

@shergin I'm not familiar with this code. I read over the PR but it's unclear to me what flaw in the code is causing the symptom and why this delta fixes it.

[zhongwuzw]

@shergin@rigdern Thanks for review, my opinion is _predictedText should always equal to the combination of backedTextInputView's original text and replaced text. The original if..else.. check may leads to inconformity between _predictedText and backedTextInputView's text.

[zhongwuzw]

@shergin@cpojer Hi, any opinion?

[ejanzer]

@zhongwuzw Can you share that the issue is you're seeing and how to repro it? Like @rigdern said, it's not clear to me what the issue is and how this fixes it - if you're seeing an inconsistency here, then it seems like predictedText already got out of sync with the backing textinput view. I think this change may make sense regardless, but I just want to take a look and see where things are going wrong.

[zhongwuzw]

@ejanzer Hi, I borrowed the repro from #21639 :

Description

[iOS only]

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.

This behaviour is reproducible when the prop maxLength is used and when length of the current value is above the half of the maxLength prop.

I have created an app from scratch using create-react-native-app, and I have modified App.js to include the following:

typeProps={};exportdefaultclassAppextendsComponent<Props>{constructor(props){super(props);this.state={text: '10000'};}render(){return(<Viewstyle={styles.container}><TextInputstyle={{height: 40,width:120,borderColor: 'gray',borderWidth: 1}}maxLength={10}onChangeText={(text)=>this.setState({text})}value={this.state.text}/></View>);}}

Reproducible Demo

[zhongwuzw]

Based on repro example, when we insert 0 at the begin, the _predictedText should equal to 0 10000, but the old code, it equal to 0, leads to the issue.

[ejanzer]

Oh I see, the problem is if you set an initial value from JS then _predictedText never gets properly initialized, so it's nil when you get here. We could also fix this by explicitly checking if it's nil, but I think this change makes sense - from the code it looks like _predictedText is supposed to be used to detect when the backing textinput updated without textInputShouldChangeTextInRange getting called.

@zhongwuzw would you mind also cleaning up the other places in this file where we set _predictedText, since they're no longer needed?

[ejanzer]

See comment about previousText - also please clean up the other places where we set _predictedText if they're no longer needed:

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L333

and

https://github.com/facebook/react-native/blob/72dd1cf49478f58b428087734d2f5aafaf2ed4c1/Libraries/Text/TextInput/RCTBaseTextInputView.m#L375

[ejanzer]

We should probably move this above here so that previousText is set correctly.

[zhongwuzw]

@ejanzer Hey, I clean up the code, please continue to review it.

[facebook-github-bot]

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

[facebook-github-bot]

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

[react-native-bot]

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

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