-
Notifications
You must be signed in to change notification settings - Fork 2.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
Add isComposing check to EditableText toggleEdit #7021
Conversation
Generate changelog in
|
Fix isComposing checkBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Hey @natsuozawa, thanks for the PR. This looks good to me as far as preserving existing functionality, but I don't have ready access to a Japanese keyboard to test the added functionality here. I'll assume that you have verified these changes locally. Is there any way we can add a test in code as well? |
@@ -357,7 +357,7 @@ export class EditableText extends AbstractPureComponent<EditableTextProps, Edita | |||
} | |||
|
|||
const hasModifierKey = altKey || ctrlKey || metaKey || shiftKey; | |||
if (event.key === "Enter") { | |||
if (event.key === "Enter" && !event.nativeEvent.isComposing) { |
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 think we actually need to do this earlier, before we handle escape. Normally, at least on macOS, pressing escape while composing cancels the current sequence. Both with and without this change, it cancels the entire edit, i.e. everything that was changed will be abandoned, instead of just the current sequence discarded and other changes and editing mode preserved.
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.
Hey you're right that maybe we should not escape when the IME is composing, but in my understanding people don't usually press escape during typing. Personally, I would expect to escape even when composing
Additional change requested by @invliD
Checklist
Changes proposed in this pull request:
EditableText will check that the user is not composing before sending out onConfirm event.
This is necessary because in some IMEs, such as Japanese, users will press Enter to convert text (ie from hiragana to kanji). In these cases, we don't want EditableText to exit from editing mode.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/isComposing
Reviewers should focus on:
Screenshot