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(NumericInput): enforce numeric chars only with IME #4160

Conversation

AaronJRubin
Copy link
Contributor

@AaronJRubin AaronJRubin commented May 29, 2020

Close loophole where IMEs could be used to inject invalid characters, and handle Japanese full-width number input.

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Reviewers should focus on:

Whether I should be using preventDefault or any other HTML event API things in my handler (doesn't seem necessary, but maybe there are edge cases?).

Screenshot

Before:

buggy-numeric-input

After:

fixed-numeric-input

…into a NumericInput, and accept full-width Japanese numbers (auto-converting them to ASCII)
@AaronJRubin AaronJRubin marked this pull request as ready for review May 29, 2020 08:02
@adidahiya
Copy link
Contributor

adidahiya commented May 29, 2020

docs preview

this change mostly looks good, thanks for the PR @AaronJRubin

I haven't checked out this branch locally, and this is the first time I'm seeing the onCompositionEnd event... I wonder if there's an easy way to provide user feedback that the non-numeric characters they entered via IME are invalid (perhaps with a "danger" intent on the input)? This is what currently happens in that case (at the end, I hit ENTER and the characters go away, potentially surprising the user):

2020-05-29 12 41 40

... as opposed to non-IME text input, where allowNumericCharactersOnly prevents those characters from ever showing up in the input at all (they are rejected upon keystroke).

If it's an easy change to add to this PR then let's do it.


semi-relatedly, I found this open React issue about IME but it seems like we're working around it: facebook/react#3926

@AaronJRubin
Copy link
Contributor Author

Turned out to be quite the easy change indeed!

intent-danger

@adidahiya adidahiya changed the title Close loophole where IMEs could be used to inject invalid characters, and handle Japanese full-width number input [core] fix(NumericInput): enforce numeric chars only with IME Jun 2, 2020
@adidahiya adidahiya merged commit 7bb4548 into palantir:develop Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants