-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Add HOF to ignore IME keydowns #59081
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I think it makes sense to create a HOF because if we have an issue with the IME in the future, we'll probably write the same code.
By the way, I was not able to reproduce the problem reported in #59080 in my environment. It might be a good idea to first identify the environment in which the issue reported in #59080 occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a neat abstraction, thanks 👍
I prefer leaving a full review to someone who uses an input method editor more frequently, but this looks good code-wise!
One thing I was curious about is whether we can add some unit tests for the new HOF?
@t-hamano I've determined the cause of #59080 to be a certain Chrome extension. I don't know what changed in the Escape handling in Command Palette from WP 6.4 to 6.5, and any other Escape handling is still working, such as in the Modal component. Anyway, I've disabled the extension in my wp-admins for now.
@tyxla Not really, at least in an easy or reliable way. There is no official support for IME composition in Puppeteer nor Playwright yet. It seems like you can now simulate composition with some new Chrome APIs and whatnot, but not sure how meaningful it will be for this specific use case. Maybe useful to test basic IME compatibility in more complex applications like the editor writing flow. At this stage, I think we'd have a bigger impact on QA if we focused on developer awareness, for example adding IME info to the keyboard testing guides for a11y, or adding some kind of linter rule for |
Great to hear that! If so, is it possible to delete this part as well? gutenberg/packages/commands/src/components/command-menu.js Lines 255 to 266 in 96cb434
|
I'd like that to be a separate PR, since it warrants some extra investigation/testing, and it'll be easier to revert if something goes wrong 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like that to be a separate PR, since it warrants some extra investigation/testing, and it'll be easier to revert if something goes wrong 👍
I see, then I think it's okay to ship this PR. I have already confirmed that all affected components continue to work correctly 👍
I only left feedback in one place.
@@ -371,6 +361,7 @@ function UnforwardedModal( | |||
* ```jsx | |||
* import { Button, Modal } from '@wordpress/components'; | |||
* import { useState } from '@wordpress/element'; | |||
import { withIgnoreIMEEvents } from '../utils/with-ignore-ime-events'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, VS Code sometimes gets confused and auto-imports stuff in the JSDoc code snippet 😅 Fixed!
# Conflicts: # packages/components/CHANGELOG.md
Flaky tests detected in 6fde8f6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7960681611
|
What?
Extracts the IME-handing logic we added in #46827 etc. into a higher-order function for easier reuse.
Affected components
Why?
For code DRYness and clarity, as well as in preparation for fixing #58188 which will likely require another one of these.
This kind of HOF doesn't seem necessary outside of the components package yet, but we might want to consider moving it to
@wordpress/compose
if we start to see more use cases. Currently the only usage is in the Command Palette — although I'm not exactly sure if it's still needed, since there's already a handler in the Modal.In any case the Escape handling in there is broken right now (#59080).Testing Instructions for Keyboard
Smoke test the affected components to make sure that they're ignoring IME events as before.
For example, open a
Modal
in the Storybook and use a CJK IME to type in the text field inside the dialog. Esc keypresses during a composition should not close the modal.