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][TextInput] Avoid using `-[UITextView setAttributedString:]` while user is typing #19809

Closed
wants to merge 1 commit into
base: master
from

Conversation

@mandrigin
Copy link
Contributor

mandrigin commented Jun 19, 2018

Motivation

iOS-specific.
For languages with complex input (such as Japanese or Chinese), a user has to type multiple characters that are then merged into a single one.
If -[UITextView setAttributedString:] is used while the user is still typing, it resets the input and characters are not being treated as typed together.

This PR avoids calling this method if possible, replacing it by just copying the attributes if the string has not been changed. That preserves the state and user can continue to type Korean or Chinese characters.

Fixes #19339

Test Plan

Essentially, the steps to reproduce are described in the issue:

  1. Type some Korean characters in TextInput, such as "하늘" (buttons ,,,,).
  2. Then move the cursor to the beginning of the text, type "파란" (buttons ,,,,) this time.

Behaviour before this fix (broken)
Actual text: ㅍㅏㄹㅏㄴ하늘.
Expected text: 파란하늘.
Characters aren't combined properly.

ezgif com-resize

Behaviour after this fix (correct)
Actual text: 파란하늘.
Expected text: 파란하늘.
Characters are combined, the same behaviour is in vanilla iOS UITextView.

input-with-fix

Related PRs

Release Notes

[IOS] [BUGFIX] [TextView] - Fix Korean/Chinese/Japanese input for multiline TextView on iOS.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 19, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 19, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

mandrigin added a commit to status-im/status-react that referenced this pull request Jun 20, 2018

[#4829] Add a patch that fixes complex input on iOS in RN 0.55.x.
The fix is described here: facebook/react-native#19809

The patch:
1. A copy of patched RCTUITextView.m file
2. A pre-action in the "Build" scheme to patch the RN file. See more on
   pre-actions here: https://burcugeneci.wordpress.com/2016/05/25/scripts-we-love-xcode-builds-life-savers/
3. A build step to fail the build if the file wasn't patched.

Signed-off-by: Igor Mandrigin <i@mandrigin.ru>
@RebelBIrd

This comment has been minimized.

Copy link

RebelBIrd commented Jul 4, 2018

When the TextInput has props "multiline={true}", this PR is effective.Because UITextView has that Method -[UITextView setAttributedString:].
multiline={false}.still has the problem!!

rasom added a commit to status-im/react-native that referenced this pull request Jul 5, 2018

@mandrigin

This comment has been minimized.

Copy link
Contributor Author

mandrigin commented Jul 16, 2018

@RebelBIrd yeah, this only fixes multi-line text views... I'll try to take a look at the UITextView as well to see if that is possible to fix that there...

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jul 23, 2018

@mandrigin I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

Igor Mandrigin
Avoid using `-[UITextView setAttributedString:]` while user is typing…
… text.

For languages with complex input (such as Japanese or Chinese),
where user presses multiple buttons to input one symbol,
it breaks selection and input, so it is impossible to enter characters.

@mandrigin mandrigin force-pushed the mandrigin:bugfix/korean-chinese-japanese-input branch from 2a08ae3 to 81960c6 Jul 25, 2018

@mandrigin

This comment has been minimized.

Copy link
Contributor Author

mandrigin commented Aug 9, 2018

@RebelBIrd can you check if this commit: 892212b fixes your issues? If so, I can probably just close my PR.

yenda added a commit to yenda/react-native that referenced this pull request Aug 13, 2018

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Aug 24, 2018

@mandrigin I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

yenda added a commit to status-im/react-native that referenced this pull request Nov 29, 2018

@cpojer cpojer requested a review from shergin Dec 4, 2018

@shergin

This comment has been minimized.

Copy link
Contributor

shergin commented Dec 4, 2018

@rigdern Adam, what do you think?

@shergin

This comment has been minimized.

Copy link
Contributor

shergin commented Dec 4, 2018

For context, here is the story behind this if statement:

IIRC, originally, this if compared strings as pure strings (as it is before the change), but later we discovered that when only attributes change on JS side, the text doesn't get updated. And we changed that to comparing them as attributed string (text + attributes).

Then we discovered that sometimes/often UITextField's version of an attributed string has additional sets of attributes which made the compare operation to fail. And we changed this back.

My idea was that we have to introduce a special custom comparing function that ignores some attributes, but eventually I haven't found time for this.

This approach is very similar to what I planned to implement, but... simpler!

@mandrigin Thank you for fixing this! Спасибо!

@facebook-github-bot
Copy link

facebook-github-bot left a comment

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

@mandrigin

This comment has been minimized.

Copy link
Contributor Author

mandrigin commented Dec 4, 2018

@shergin Спасибо! Thanks for your explanation! Yeah, text input is very hard... I see that you are trying to avoid re-implementing all the locale support that is implemented in mobile OS-es. That is a very pragmatic and wise solution.

Simplicity was my goal, because I'm just beginning my journey into React Native world, so I don't know the internals so much. Hence, I tried to be as concise and have as small of the affected code area as possible.

My fix has one gotcha. It only fixes multiline text fields, because on iOS I see that single-line text fields are UITextFields, and not UITextViews.
I can try to investigate and make a follow-up PR to fix single line text fields. We don't have this issue ourselves, mostly because we use multi-line text fields everywhere, but I think for the completeness sake I want to fix it.

@react-native-bot

This comment has been minimized.

Copy link
Collaborator

react-native-bot commented Dec 4, 2018

Igor Mandrigin merged commit f77aa4e into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Dec 4, 2018

@shergin

This comment has been minimized.

kelset added a commit that referenced this pull request Dec 12, 2018

Avoid using `-[UITextView setAttributedString:]` while user is typing (
…#19809)

Summary:
iOS-specific.
For languages with complex input (such as Japanese or Chinese), a user has to type multiple characters that are then merged into a single one.
If `-[UITextView setAttributedString:]` is used while the user is still typing, it resets the input and characters are not being treated as typed together.

This PR avoids calling this method if possible, replacing it by just copying the attributes if the string has not been changed. That preserves the state and user can continue to type Korean or Chinese characters.

Fixes #19339

<!--
  Required: Write your motivation here.
  If this PR fixes an issue, type "Fixes #issueNumber" to automatically close the issue when the PR is merged.
-->

<!--
  Required: Write your test plan here. If you changed any code, please provide us with
  clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
-->

Essentially, the steps to reproduce are described in [the issue](#19339):

1. Type some Korean characters in TextInput, such as "하늘" (buttons `ㅎ`,`ㅏ`,`ㄴ`,`ㅡ`,`ㄹ`).
2. Then move the cursor to the beginning of the text, type "파란" (buttons `ㅍ`,`ㅏ`,`ㄹ`,`ㅏ`,`ㄴ`) this time.

**Behaviour before this fix (broken)**
Actual text: `ㅍㅏㄹㅏㄴ하늘`.
Expected text: `파란하늘`.
Characters aren't combined properly.

![ezgif com-resize](https://user-images.githubusercontent.com/466427/41613572-4256dda8-73f6-11e8-99a9-0ab833202b95.gif)

**Behaviour after this fix (correct)**
Actual text: `파란하늘`.
Expected text: `파란하늘`.
Characters are combined, the same behaviour is in vanilla iOS `UITextView`.

![input-with-fix](https://user-images.githubusercontent.com/466427/41613526-1aae2284-73f6-11e8-87f2-c1cef51cd83a.gif)

<!--
  Does this PR require a documentation change?
  Create a PR at https://github.com/facebook/react-native-website and add a link to it here.
-->

<!--
  Required.
  Help reviewers and the release process by writing your own release notes. See below for an example.
-->

[IOS] [BUGFIX] [TextView] - Fix Korean/Chinese/Japanese input for multiline TextView on iOS.

<!--
  **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

    CATEGORY
  [----------]      TYPE
  [ CLI      ] [-------------]    LOCATION
  [ DOCS     ] [ BREAKING    ] [-------------]
  [ GENERAL  ] [ BUGFIX      ] [ {Component} ]
  [ INTERNAL ] [ ENHANCEMENT ] [ {Filename}  ]
  [ IOS      ] [ FEATURE     ] [ {Directory} ]   |-----------|
  [ ANDROID  ] [ MINOR       ] [ {Framework} ] - | {Message} |
  [----------] [-------------] [-------------]   |-----------|

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Pull Request resolved: #19809

Differential Revision: D13326614

Pulled By: shergin

fbshipit-source-id: 6a5cab3f7290f0f623a6f4c29353a573eb321b0b

grabbou added a commit that referenced this pull request Dec 17, 2018

Avoid using `-[UITextView setAttributedString:]` while user is typing (
…#19809)

Summary:
iOS-specific.
For languages with complex input (such as Japanese or Chinese), a user has to type multiple characters that are then merged into a single one.
If `-[UITextView setAttributedString:]` is used while the user is still typing, it resets the input and characters are not being treated as typed together.

This PR avoids calling this method if possible, replacing it by just copying the attributes if the string has not been changed. That preserves the state and user can continue to type Korean or Chinese characters.

Fixes #19339

<!--
  Required: Write your motivation here.
  If this PR fixes an issue, type "Fixes #issueNumber" to automatically close the issue when the PR is merged.
-->

<!--
  Required: Write your test plan here. If you changed any code, please provide us with
  clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
-->

Essentially, the steps to reproduce are described in [the issue](#19339):

1. Type some Korean characters in TextInput, such as "하늘" (buttons `ㅎ`,`ㅏ`,`ㄴ`,`ㅡ`,`ㄹ`).
2. Then move the cursor to the beginning of the text, type "파란" (buttons `ㅍ`,`ㅏ`,`ㄹ`,`ㅏ`,`ㄴ`) this time.

**Behaviour before this fix (broken)**
Actual text: `ㅍㅏㄹㅏㄴ하늘`.
Expected text: `파란하늘`.
Characters aren't combined properly.

![ezgif com-resize](https://user-images.githubusercontent.com/466427/41613572-4256dda8-73f6-11e8-99a9-0ab833202b95.gif)

**Behaviour after this fix (correct)**
Actual text: `파란하늘`.
Expected text: `파란하늘`.
Characters are combined, the same behaviour is in vanilla iOS `UITextView`.

![input-with-fix](https://user-images.githubusercontent.com/466427/41613526-1aae2284-73f6-11e8-87f2-c1cef51cd83a.gif)

<!--
  Does this PR require a documentation change?
  Create a PR at https://github.com/facebook/react-native-website and add a link to it here.
-->

<!--
  Required.
  Help reviewers and the release process by writing your own release notes. See below for an example.
-->

[IOS] [BUGFIX] [TextView] - Fix Korean/Chinese/Japanese input for multiline TextView on iOS.

<!--
  **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

    CATEGORY
  [----------]      TYPE
  [ CLI      ] [-------------]    LOCATION
  [ DOCS     ] [ BREAKING    ] [-------------]
  [ GENERAL  ] [ BUGFIX      ] [ {Component} ]
  [ INTERNAL ] [ ENHANCEMENT ] [ {Filename}  ]
  [ IOS      ] [ FEATURE     ] [ {Directory} ]   |-----------|
  [ ANDROID  ] [ MINOR       ] [ {Framework} ] - | {Message} |
  [----------] [-------------] [-------------]   |-----------|

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Pull Request resolved: #19809

Differential Revision: D13326614

Pulled By: shergin

fbshipit-source-id: 6a5cab3f7290f0f623a6f4c29353a573eb321b0b

JunielKatarn added a commit to JunielKatarn/react-native that referenced this pull request Feb 11, 2019

Avoid using `-[UITextView setAttributedString:]` while user is typing (
…facebook#19809)

Summary:
iOS-specific.
For languages with complex input (such as Japanese or Chinese), a user has to type multiple characters that are then merged into a single one.
If `-[UITextView setAttributedString:]` is used while the user is still typing, it resets the input and characters are not being treated as typed together.

This PR avoids calling this method if possible, replacing it by just copying the attributes if the string has not been changed. That preserves the state and user can continue to type Korean or Chinese characters.

Fixes facebook#19339

<!--
  Required: Write your motivation here.
  If this PR fixes an issue, type "Fixes #issueNumber" to automatically close the issue when the PR is merged.
-->

<!--
  Required: Write your test plan here. If you changed any code, please provide us with
  clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
-->

Essentially, the steps to reproduce are described in [the issue](facebook#19339):

1. Type some Korean characters in TextInput, such as "하늘" (buttons `ㅎ`,`ㅏ`,`ㄴ`,`ㅡ`,`ㄹ`).
2. Then move the cursor to the beginning of the text, type "파란" (buttons `ㅍ`,`ㅏ`,`ㄹ`,`ㅏ`,`ㄴ`) this time.

**Behaviour before this fix (broken)**
Actual text: `ㅍㅏㄹㅏㄴ하늘`.
Expected text: `파란하늘`.
Characters aren't combined properly.

![ezgif com-resize](https://user-images.githubusercontent.com/466427/41613572-4256dda8-73f6-11e8-99a9-0ab833202b95.gif)

**Behaviour after this fix (correct)**
Actual text: `파란하늘`.
Expected text: `파란하늘`.
Characters are combined, the same behaviour is in vanilla iOS `UITextView`.

![input-with-fix](https://user-images.githubusercontent.com/466427/41613526-1aae2284-73f6-11e8-87f2-c1cef51cd83a.gif)

<!--
  Does this PR require a documentation change?
  Create a PR at https://github.com/facebook/react-native-website and add a link to it here.
-->

<!--
  Required.
  Help reviewers and the release process by writing your own release notes. See below for an example.
-->

[IOS] [BUGFIX] [TextView] - Fix Korean/Chinese/Japanese input for multiline TextView on iOS.

<!--
  **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

    CATEGORY
  [----------]      TYPE
  [ CLI      ] [-------------]    LOCATION
  [ DOCS     ] [ BREAKING    ] [-------------]
  [ GENERAL  ] [ BUGFIX      ] [ {Component} ]
  [ INTERNAL ] [ ENHANCEMENT ] [ {Filename}  ]
  [ IOS      ] [ FEATURE     ] [ {Directory} ]   |-----------|
  [ ANDROID  ] [ MINOR       ] [ {Framework} ] - | {Message} |
  [----------] [-------------] [-------------]   |-----------|

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Pull Request resolved: facebook#19809

Differential Revision: D13326614

Pulled By: shergin

fbshipit-source-id: 6a5cab3f7290f0f623a6f4c29353a573eb321b0b
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.