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

Fix a broken input for the Korean alphabet in TextInput #32523

Closed
wants to merge 4 commits into from

Conversation

kmsbernard
Copy link
Contributor

@kmsbernard kmsbernard commented Nov 3, 2021

Summary

Fix #32503

Updating the attributed text in TextView/TextField while inputting Korean language will break input mechanism of the Korean alphabet. This results unexpected text input.

This PR supersedes the previous fixes: #19809, #22546

Changelog

[iOS] [Fixed] - Fix a broken input for the Korean alphabet in TextInput

Test Plan

RPReplay_Final1635916183_c.mov

@facebook-github-bot
Copy link
Contributor

Hi @bernard-kms!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign 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 to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

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

@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Nov 3, 2021
@pull-bot
Copy link

pull-bot commented Nov 3, 2021

PR build artifact for c679dab is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@analysis-bot
Copy link

analysis-bot commented Nov 3, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,290,542 -186,577
android hermes armeabi-v7a 7,854,574 +78,057
android hermes x86 8,657,623 -288,371
android hermes x86_64 8,616,752 -273,391
android jsc arm64-v8a 9,791,779 -154
android jsc armeabi-v7a 8,752,609 +74
android jsc x86 9,740,711 -38
android jsc x86_64 10,341,567 -84

Base commit: 8ace78c
Branch: main

@analysis-bot
Copy link

analysis-bot commented Nov 3, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 8ace78c
Branch: main

@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 Nov 3, 2021
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 3, 2021
@@ -145,26 +145,6 @@ - (void)setTextAlignment:(NSTextAlignment)textAlignment
_placeholderView.textAlignment = textAlignment;
}

- (void)setAttributedText:(NSAttributedString *)attributedText
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this is redundant and safe to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Root cause of this issue is updating attributedText via -setAttributedText: of RCTBaseTextInputView.
In this removed block, it attempts to avoid calling the method by comparing two attributed text with bare strings. But it should be handled by -textOf:quals: which already handles some other special cases such as Chinese and Japanese.
I added detection for Korean to safely fallbacks to bare text comparison in the -textOf:quals:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found out that it is required to invalidate placeholder visibility in this method. I added a required commit.

@lunaleaps
Copy link
Contributor

Thanks for your fix! Wondering if this was tested off of main and whether other languages have been tested to verify no regressions?

@lunaleaps lunaleaps requested review from philIip and p-sun November 3, 2021 22:32
@kmsbernard
Copy link
Contributor Author

Thanks for your fix! Wondering if this was tested off of main and whether other languages have been tested to verify no regressions?

I've tested some other languages with RNTester:

Chinese.mov
Japanese.mov
English.mov

@kmsbernard
Copy link
Contributor Author

@philIip @p-sun Is there anything I can do to help move this forward?

@facebook-github-bot
Copy link
Contributor

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

@pull-bot
Copy link

PR build artifact for 2dd1dc0 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@lunaleaps
Copy link
Contributor

Thanks for the ping! I've reached out to @philIip to land

@kmsbernard
Copy link
Contributor Author

@philIip Is there something I can do to move this forward?

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @bernard-kms in 1a83dc3.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 10, 2022
ShikaSD pushed a commit that referenced this pull request Feb 22, 2022
Summary:
<!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? -->

Fix #32503

Updating the attributed text in TextView/TextField while inputting Korean language will break input mechanism of the Korean alphabet. This results unexpected text input.

This PR supersedes the previous fixes: #19809, #22546

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - Fix a broken input for the Korean alphabet in TextInput

Pull Request resolved: #32523

Test Plan: https://user-images.githubusercontent.com/20317121/140013434-1674c391-54d6-4410-b4c1-c633697e639d.mov

Reviewed By: lunaleaps, sammy-SC

Differential Revision: D32470543

Pulled By: philIip

fbshipit-source-id: e7e34bd362fa2ab2ca579103db01ad8d1a891c35
ShikaSD pushed a commit that referenced this pull request Feb 24, 2022
Summary:
<!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? -->

Fix #32503

Updating the attributed text in TextView/TextField while inputting Korean language will break input mechanism of the Korean alphabet. This results unexpected text input.

This PR supersedes the previous fixes: #19809, #22546

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - Fix a broken input for the Korean alphabet in TextInput

Pull Request resolved: #32523

Test Plan: https://user-images.githubusercontent.com/20317121/140013434-1674c391-54d6-4410-b4c1-c633697e639d.mov

Reviewed By: lunaleaps, sammy-SC

Differential Revision: D32470543

Pulled By: philIip

fbshipit-source-id: e7e34bd362fa2ab2ca579103db01ad8d1a891c35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

State update breaks CJK language input mode in single-line TextInput, after iOS 15
6 participants