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

Temporarily uncontrol InputGroup during IME composition #4262

Closed
wants to merge 2 commits into from

Conversation

rynorris
Copy link
Contributor

@rynorris rynorris commented Aug 6, 2020

Read facebook/react#3926 for full context on the issue.
Basically, while composing text using an IME, onChange events still fire for every keystroke.
From my testing, it seems that IF the InputGroup is controlled AND the setState call in the parent component occurs asynchronously, then the IME composition gets interrupted when the props change.

You can reproduce this behaviour by replacing this line: https://github.com/palantir/blueprint/blob/develop/packages/docs-app/src/examples/core-examples/inputGroupExample.tsx#L58
with

private handleFilterChange = handleStringChange(filterValue => window.setTimeout(this.setState({ filterValue }), 0));

to force an asynchronous update.

I'm not sure this is the right fix, but it's certainly simple. The proposal is that we temporarily stop controlling the input component while IME composition is occurring.
We purposefully continue sending onChange events during this time, since this matches what the behaviour would be in the uncontrolled case, and ensures that once we re-take control when composition ends, the parent component's state should be in-sync with whatever was in the input box as a result of composition.

I have tested in Chrome and Safari on MacOS, and both exhibit correct behaviour after this fix, even with the above hack to force asynchronous state changes.

That said, any behaviour like this has the potential to be fragile, so would appreciate any input from the team.

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Temporarily relinquish control of input component during IME composition.

Reviewers should focus on:

How fragile this is likely to be. Are there any edge cases?

Screenshot

No visible changes.

@adidahiya adidahiya self-requested a review August 6, 2020 15:42
@adidahiya
Copy link
Contributor

It sounds reasonable to implement this workaround in a core Blueprint component, if we are reasonably confident that the composition events won't fire in other unexpected situations. I'm trying out a fix which is closer to https://jsfiddle.net/m792qtys/, and isolates the workaround to its own component. Will send a PR

@adidahiya
Copy link
Contributor

adidahiya commented Aug 6, 2020

Also note that the current implementation in this PR causes this stern React warning:

react_devtools_backend.js:2273 Warning: A component is changing a controlled input of type text to be uncontrolled. Input elements should not switch from controlled to uncontrolled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components
    in input (created by Blueprint3.InputGroup)
    in div (created by Blueprint3.InputGroup)
    in Blueprint3.InputGroup (created by InputGroupExample)
    in div (created by Example)
    in div (created by Example)
    in Example (created by InputGroupExample)
    in InputGroupExample
    in Unknown (created by Page)
    in div (created by Page)
    in div (created by Page)
    in Page (created by HotkeysTarget(Documentation))
    in main (created by HotkeysTarget(Documentation))
    in div (created by HotkeysTarget(Documentation))
    in div (created by HotkeysTarget(Documentation))
    in HotkeysTarget(Documentation) (created by BlueprintDocs)
    in BlueprintDocs

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

see above comments, and my alternative #4266

@rynorris
Copy link
Contributor Author

rynorris commented Aug 7, 2020

Closing in favour of #4266

Thanks @adidahiya ! Your implementation looks more robust, and easily generalizable to other components which use <input> elements, so let's go with that. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants