-
Notifications
You must be signed in to change notification settings - Fork 960
fix(terminal): forward Ctrl+<letter> to PTY under non-Latin IME #4917
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -76,6 +76,29 @@ export function createTerminalKeyEventHandler( | |||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Non-Latin IME (e.g. Korean 2-set) maps physical letter keys to | ||||||||||
| // non-ASCII glyphs: pressing O while Korean is active yields | ||||||||||
| // event.key="ㅐ" instead of "o". xterm derives the Ctrl+<letter> | ||||||||||
| // control byte from event.key, so it silently drops Ctrl+O, Ctrl+K, | ||||||||||
| // and every other Ctrl+<letter> chord typed while such an IME is | ||||||||||
| // active. Compute the byte from the physical key position | ||||||||||
| // (event.code) instead — that value is always layout-agnostic. | ||||||||||
| if ( | ||||||||||
| event.type === "keydown" && | ||||||||||
| event.ctrlKey && | ||||||||||
| !event.metaKey && | ||||||||||
| !event.altKey && | ||||||||||
| !event.shiftKey | ||||||||||
| ) { | ||||||||||
| const codeMatch = event.code.match(/^Key([A-Z])$/); | ||||||||||
| if (codeMatch && event.key.charCodeAt(0) > 127) { | ||||||||||
| event.preventDefault(); | ||||||||||
| const charCode = codeMatch[1].charCodeAt(0) - 64; // A→1 … Z→26 | ||||||||||
| terminal.input(String.fromCharCode(charCode), false); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/terminal-key-event-handler.ts
Line: 97
Comment:
**`wasUserInput: false` is inconsistent with the rest of the handler**
The `translateLineEditChord` path directly above (line 54) calls `terminal.input(translation, true)`. When `wasUserInput` is `true`, xterm scrolls the viewport to the bottom before delivering the bytes — the expected behavior for a real keypress. Passing `false` here means a user who has scrolled up and then presses Ctrl+K (or any Ctrl+letter) while Korean IME is active won't have the viewport snapped to the current prompt, so the shell's response appears off-screen. Since this block only fires on a genuine `keydown`, `true` is the correct value.
```suggestion
terminal.input(String.fromCharCode(charCode), true);
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Mark this forwarded control byte as user input so xterm applies normal keypress behavior (including scroll-to-bottom and user-input side effects) consistently with the other keyboard translation path. Prompt for AI agents
Suggested change
|
||||||||||
| return false; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return true; | ||||||||||
| }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
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.
isComposingis trueIf an IME composition is in progress (e.g. the user is mid-way through composing a Korean syllable) and presses Ctrl+letter,
isComposingmay betrue. xterm's own handler skips events whereisComposingis true, so it would naturally ignore that event. The new block doesn't guard for this state, meaning it would still callterminal.input()andpreventDefault()— potentially swallowing an event that should have been ignored and interrupting IME composition unexpectedly. Adding&& !event.isComposingto the outer condition would match xterm's own behavior and avoid this edge case.Prompt To Fix With AI