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

[core] fix(AsyncControllableInput): handle compound composition events #5165

Merged
merged 3 commits into from
Mar 16, 2022
Merged

[core] fix(AsyncControllableInput): handle compound composition events #5165

merged 3 commits into from
Mar 16, 2022

Conversation

Athenodoros
Copy link
Contributor

There's an issue with the AsyncControllableInput, where Korean input can lead to incorrect values being stored.

The Bug

The cause is the way that Korean handles keyboard events: letters in the Korean language are combined into symbols, which are bubbled up as CompositionEvents. For instance, when typing the characters -> -> -> , they would successively render as , , , 디가 - note that with the last input the value splits out into two characters, which manifests as one CompositionEvent () ending and then a new one () immediately starting. I put together some logging to show the flow (times in seconds, logging code here):

Screen Shot 2022-03-14 at 11 49 44 AM

Currently, this gives incorrect results with AsyncControllableInput: because the value prop can be updated on a delay (eg. example from the docs), when the first composition ends the stored value () is blown away by the old state (), which results in the incorrect final result 닥가 (note the extra in the first character).

The solution

As far as I can tell, there's no way in the onCompositionEnd callback to determine whether another CompositionEvent is about to start. If that's the case, the best solution I can think of is to manually wait to unlock the component for edits after some time delay - I've chosen 10ms, because in my experimentation it was always <=1ms before the next event started.

Checklist

  • Includes tests
  • Update documentation

Is there documentation I should add?

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @Athenodoros! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@adidahiya adidahiya changed the title Handle Compound Composition Events [core] fix(AsyncControllableInput): handle compound composition events Mar 15, 2022
@adidahiya
Copy link
Contributor

docs preview: https://52033-71939872-gh.circle-artifacts.com/0/packages/docs-app/dist/index.html#core/components/text-inputs.input-group

first input group in that example uses asyncControl={true}. could you verify the bug is fixed with korean characters there @Athenodoros?

@Athenodoros
Copy link
Contributor Author

Yep, looks good to me - thanks @adidahiya!

@adidahiya adidahiya merged commit b0a6143 into palantir:develop Mar 16, 2022
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.

3 participants