fix: Various problems with Chinese IMEs#320525
Open
arnavnagzirkar wants to merge 2 commits into
Open
Conversation
Contributor
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @anthonykim1Matched files:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes Chinese IME composition-related keydown handling so raw punctuation keys aren’t incorrectly sent to the shell during an active composition session.
Changes:
- Ignore
keydownevents withevent.isComposing === truein the terminal’s custom key handler to avoid triggering xterm.js’s buggy composition path. - Add a browser test to verify composed keydown events are blocked (handler returns
false) and notpreventDefault’ed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/browser/terminalInstance.ts | Adds an early return for event.isComposing in the xterm custom key event handler. |
| src/vs/workbench/contrib/terminal/test/browser/terminalInstance.test.ts | Adds a unit test asserting composed keydown events are ignored by the custom handler. |
Comment on lines
+1141
to
+1144
| // Do not process keys that are part of an IME composition; compositionstart/end events | ||
| // handle sending the composed text to the terminal. Without this, xterm.js will incorrectly | ||
| // process the raw key (eg. "." instead of "。") and potentially send it to the shell. | ||
| // See: https://github.com/microsoft/vscode/issues/275646 |
| test('custom key event handler should return false for keys that are part of an IME composition to prevent raw key from being sent to shell', async () => { | ||
| const instance = await createTerminalInstance(); | ||
| let capturedHandler: ((e: KeyboardEvent) => boolean) | undefined; | ||
| instance.xterm!.raw.attachCustomKeyEventHandler = handler => { capturedHandler = handler; }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When third-party Chinese IMEs (like Sogou on macOS) substitute punctuation (e.g.
.。), they firecompositionstart/compositionupdate/compositionendevents.Root cause
When third-party Chinese IMEs (like Sogou on macOS) substitute punctuation (e.g.
.。), they firecompositionstart/compositionupdate/compositionendevents. During this composition, the browser also fireskeydownevents withevent.isComposing = trueand a non-229 keyCode (e.g. keyCode 190 for.).xterm.js's
_compositionHelper.keydown()method handleskeydownevents:_isComposing = trueandkeyCode = 229(the standard IME Process code): correctly returnsfalse(no-op, let composition events handle it)_isComposing = trueandkeyCode ≠ 229(e.g. 190 for "."): bug - calls_finalizeComposition(false)(which sends an empty/partial composition) and then returnstrue, causing xterm.js to also process and send the raw key (.) to the shellVS Code's
attachCustomKeyEventHandlerdid not checkevent.isComposing, so it always passed everykeydownevent to xterm.js's handler, triggering the buggy code path.Reference: xtermjs/xterm.js#4486 (unfixed upstream as of this writing)
What changed
src/vs/workbench/contrib/terminal/browser/terminalInstance.tsAdded an early-return guard in
attachCustomKeyEventHandler(the VS Code custom key handler registered with xterm.js):Returning
falsefrom this handler tells xterm.js to skip its own_compositionHelper.keydown()call for that event, preventing the raw key from being sent to the shell. The composition events (compositionstart/compositionupdate/compositionend) are handled by separate direct listeners on the textarea in xterm.js - they are not affected by this change, so the composed Chinese characters are still correctly sent when the composition ends.Why this is safe:
isComposing=trueandkeyCode=229were already no-ops in xterm.js; returningfalsefor those has the same effectisComposing=trueand non-229 keyCode are the bug case; returningfalseskips the buggy_finalizeComposition(false)+ key processing pathcompositionend, so itsisComposingisfalse- it is not affected by this checkIssue
Fixes #275646
Issue: #275646
Diffstat
Testing
Ran the relevant tests and linter for the changed files while developing.
Kept the change minimal and focused on this one issue.
AI assistance
I used GitHub Copilot to help write parts of this change. I've reviewed and tested it myself, I understand what it does, and I'll follow up on any review feedback.