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

InputControl: Ignore IME events when isPressEnterToChange #60090

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

mirka
Copy link
Member

@mirka mirka commented Mar 21, 2024

Follow-up to #59081
Part of #45605

What?

Ignore Enter and Esc keydowns emitted during IME composition when the isPressEnterToChange prop is enabled.

When this prop is enabled:

  • Enter will need to be hit before an onChange event is fired. Enter keys pressed during IME composition are not intended to commit the change in the entire input field, so they should be ignored.
  • Esc will clear the input field when there are "uncommitted" changes (i.e. changes that have not been committed with Enter yet). Esc keys pressed during IME composition are not intended to clear the entire input field, so they should be ignored.

Why?

This was found during an audit of the components package to see if there is any keydown handling logic that doesn't ignore IME events properly. This was the only violation that I found. (There is unfiltered keydown handling logic in other components too, but they are unlikely to involve IME events.)

Testing Instructions

  1. On the "Default" story for InputControl in the Storybook, enable the isPressEnterToChange prop and reload.
  2. Type with your IME in the input field. Enter keypresses during composition should not log an onChange event in the Actions panel.
  3. Reload. Add some text to the input field, but without hitting Enter to commit the changes to the InputControl. Type some more text using your IME and Esc before completing the composition. The existing text in the input field should remain, and only the uncompleted composition should be canceled.

Screenshots or screencast

Before

CleanShot.2024-03-22.at.03.02.34.mp4

After

CleanShot.2024-03-22.at.03.03.36.mp4

@mirka mirka added [Type] Bug An existing feature does not function as intended Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] Components /packages/components labels Mar 21, 2024
@mirka mirka self-assigned this Mar 21, 2024
@mirka mirka requested a review from ajitbohra as a code owner March 21, 2024 18:18
Copy link

github-actions bot commented Mar 21, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: torounit <toro_unit@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@mirka mirka requested review from torounit, t-hamano and a team March 21, 2024 18:20
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

I think it makes sense to add HOF, but strangely, I was unable to reproduce this problem in my environment (OS: Win11, browser: Chrome/Firefox).

In the video below, I add console.log(key) to observe the key down event of the InputControl component in trunk and start Storybook. During IME input, event.key is always "Process", including the Enter key and Esc key. In your environment, even during IME input, does event.key show the actual key?

afd2173bca8eb62b54e51c5877282267.mp4

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Code-wise this is a straightforward change and looks good, but I'll defer to @t-hamano for actually verifying it works as expected.

@t-hamano t-hamano self-requested a review March 22, 2024 09:41
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Interestingly, keyboardEvent.key seems to be different depending on the OS. I would like to introduce two resources.


https://github.com/seawind543/keydown-key

  • With IME, the keyDown.key value of Chrome is different on Mac and Windows
    • Mac: key === Enter
    • Windows: key === Process
  • With IME, the keyDown.key value of Chrome and FireFox are different on Mac
    • FireFox key === Process on both Mac and Windows

As stated above, the values seem to vary depending on the OS and browser.


https://youtrack.jetbrains.com/issue/PRJ-514/IME-isnt-supported-for-Mac

It is shown that the value of event.key differs depending on the OS.


If all of these resources are correct, this issue may only occur with the Chrome browser on Mac.

In any case, the changes in this PR have no effect on Windows, so LGTM 👍

Copy link
Member

@torounit torounit left a comment

Choose a reason for hiding this comment

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

I was able to reproduce this issue on macOS 14 / Chrome, and also confirmed that it does not occur in Firefox.

bofore

before.mov

after

after.mov

@mirka
Copy link
Member Author

mirka commented Mar 22, 2024

As stated above, the values seem to vary depending on the OS and browser.

😭

I'm glad we have both Win/Mac users for coverage though, thank you all for testing!

@mirka mirka merged commit 2ec46cc into trunk Mar 22, 2024
65 checks passed
@mirka mirka deleted the fix/input-control-ime branch March 22, 2024 16:09
@github-actions github-actions bot added this to the Gutenberg 18.1 milestone Mar 22, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
…s#60090)

* InputControl: Ignore IME events when `isPressEnterToChange`

* Add changelog

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: torounit <toro_unit@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants