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] Apply the fix for CJK languages on single-line text fields. #22546

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@mandrigin
Copy link
Contributor

mandrigin commented Dec 6, 2018

Follow-up to #19809

This fix generalizes the setAttributedString: fix to single-line text fields.

Fixes #19339

Pull requests that expand test coverage are more likely to get reviewed. Add a test case whenever possible!

Test Plan:

⚠️ Possible areas of regressions: formatting of single-line text fields.

The test app

import React, {Component} from 'react';
import {StyleSheet, Text, TextInput, View} from 'react-native';

type Props = {};
export default class App extends Component<Props> {
    constructor(props) {
        super(props);
        this.state = { text: '', textM: '' };
    }
    render() {
        return (
            <View style={styles.container}>
            <Text style={styles.instructions}>SINGLE_LINE</Text>
            <TextInput
                style={{height: 40, width: 200, borderWidth: 1}}
                onChangeText={(text) => this.setState({text: text})}
                value={this.state.text}
            />
            <Text style={styles.instructions}>MULTILINE</Text>
            <TextInput
                style={{height: 40, width: 200, borderWidth: 1}}
                multiline={true}
                onChangeText={(text) => this.setState({textM: text})}
                value={this.state.textM}
            />
            </View>
        );
    }
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    paddingTop: 100,
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  instructions: {
    textAlign: 'center',
    color: '#333333',
    marginBottom: 5,
    marginTop: 20,
  },
});

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

  • For both single-line and multi-line text fields:
    • Type some Korean characters in TextInput, such as "하늘" (buttons ,,,,).
    • Then move the cursor to the beginning of the text, type "파란" (buttons ,,,,) this time.

Behaviour before this fix
Actual text (Single-line): ㅍㅏㄹㅏㄴ하늘.
Actual text (Multi-line): 파란하늘.
Expected text (both fields): 파란하늘.
Characters aren't combined properly in single-line.

Behaviour after this fix (correct)
Actual text (Single-line): 파란하늘.
Actual text (Multi-line): 파란하늘.
Expected text (both fields): 파란하늘.
Characters are combined correctly.

Changelog:

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

@shergin
Copy link
Contributor

shergin left a comment

Why can't we move all this logic inside -[RCTUITextView setAttributedText] (which might imply adding sanitizing inside the getter. And same thing for UITextField subclass.

There is a huge reason for that desire: This module is the most complex module in the whole RN rendering subsystem. We have to push all logic that might be considered as "improvements of core UIKit components" down to subclasses dedicated for that. The reason is that our RN-specific model of the world implies that our expectations about how UIKit works are correct. That's really challenging to implement the ideal modal itself, so we cannot afford polluting that with UIKit specific fixes. We have to decouple them in subclasses that "fix" UIKit stuff for us.

@shergin

This comment has been minimized.

Copy link
Contributor

shergin commented Dec 13, 2018

@mandrigin I deeply appreciate your effort and I am existed about those fixes. We struggled with them for so long time! But we really need to do it right to make it sustainable.

[_attributesHolder setAttributedString:super.attributedText];
}

- (NSAttributedString *)attributedText {

This comment has been minimized.

@mandrigin

mandrigin Dec 13, 2018

Author Contributor

@shergin it is easier to me to comment here, to show why I did it this way.
UITextField is a very basic UI element. It doesn't expose any API to just change the string attributes without re-setting the attributed string completely (and breaking CJK input).
Luckily, it relies on it's own public (and overridable) property attributedText when rendering.

What I'm doing here is hijacking this property and replacing its value with the value I store locally, in NSMutableAttributedString, which I can replace without noticeable side-effects. So this override has to be here, if we want to continue to use UITextField. It's a hack and it can potentially break with new iOS releases, etc.

To be able to consolidate logic, we can just use UITextView everywhere, it provides all the necessary functionality as public APIs. What is the history between having 2 different implementations for single and multiline text views?

This comment has been minimized.

@shergin

shergin Dec 13, 2018

Contributor

I mean why not consolidate all attribute specific-logic in this class making attributedText and setAttributedText behave as we want?

We have to have two different underlying components because they simply behave differently. There is no way to configure UITextView behave exactly as UITextField.

This comment has been minimized.

@shergin

shergin Dec 13, 2018

Contributor

I mean we have an original problem: When we call setAttributedText, marked fragment gets reset. So, let's add all checks that prevents that into both RCTUITextField and RCTUITextView (iirc, now we have only one of them).

This comment has been minimized.

@mandrigin

mandrigin Dec 13, 2018

Author Contributor

ah, okay, I mistread your initial comment.
So you mean, just duplicate the logic and encapsulate it inside RCTUITextField and RCTUITextView classes separately, so we can call setAttributedString safely on both RCTUITextField and RCTUITextView?

This comment has been minimized.

@mandrigin

mandrigin Dec 17, 2018

Author Contributor

should be fixed now

This comment has been minimized.

@shergin

shergin Jan 8, 2019

Contributor

(Let's continue discussion here.)
Now we are abstracted from anything else, so we can work on it. 😄

I think, it's great. ❤️

Nits:

  • Instead of referring issues and commenting every single line, we have to leave one comment that describe a problem and (most importantly) the change in the behavior of the methods. That's more sustainable and easy to understand for people from the future which don't have a full context.
  • Codestyle. (New line before { and spaces.)

Btw, I don't think it's only fix for CJK problem. It's more wide problem which happens when we are replacing some attributed string with completely new one without paying attention to what attributes were added to the string by active text input. So, if we can preserve those attributes, I bet full replacing will work with CJK. (The current solution is good enough though.)

This comment has been minimized.

@mandrigin

mandrigin Jan 9, 2019

Author Contributor

pushed the fix to address this feedback

This comment has been minimized.

@mandrigin

mandrigin Jan 16, 2019

Author Contributor

@shergin is there anything else I need to fix in this PR?

// Search for `GH-19339` to see all code affected by this fix.

- (void)setAttributedText:(NSAttributedString *)attributedText {
[_attributesHolder setAttributedString:attributedText];

This comment was marked as outdated.

@shergin

shergin Dec 13, 2018

Contributor

I mean why not to fallback to copyAttributesFrom here, if we see that super.attributedText.string === attributedText?

This comment was marked as outdated.

@mandrigin

mandrigin Dec 17, 2018

Author Contributor

should be fixed now

@mandrigin mandrigin force-pushed the mandrigin:fix-cjk-single-line branch from bc35574 to 5592d66 Dec 17, 2018

@mandrigin

This comment has been minimized.

Copy link
Contributor Author

mandrigin commented Dec 17, 2018

@shergin I just simplified my fix to affect only one source file.

@mandrigin mandrigin force-pushed the mandrigin:fix-cjk-single-line branch from 5592d66 to b67f2c7 Jan 9, 2019

@hramos hramos removed the 🔶Components label Jan 24, 2019

@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.

matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019

Apply the fix for CJK languages on single-line text fields. (facebook…
…#22546)

Summary:
Follow-up to facebook#19809

This fix generalizes the `setAttributedString:` fix to single-line text fields.

Fixes facebook#19339

_Pull requests that expand test coverage are more likely to get reviewed. Add a test case whenever possible!_
Pull Request resolved: facebook#22546

Differential Revision: D13948951

Pulled By: shergin

fbshipit-source-id: 67992c02b32f33f6d61fac4554e4f46b973262c1

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

Apply the fix for CJK languages on single-line text fields. (facebook…
…#22546)

Summary:
Follow-up to facebook#19809

This fix generalizes the `setAttributedString:` fix to single-line text fields.

Fixes facebook#19339

_Pull requests that expand test coverage are more likely to get reviewed. Add a test case whenever possible!_
Pull Request resolved: facebook#22546

Differential Revision: D13948951

Pulled By: shergin

fbshipit-source-id: 67992c02b32f33f6d61fac4554e4f46b973262c1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment