diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index dd366f225de..5d5162fffae 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -616,6 +616,8 @@ jobs: 'tests/e2e/terminal-ime-exact-byte.spec.ts' \ 'tests/e2e/terminal-korean-composing-chord-order.spec.ts' \ 'tests/e2e/terminal-korean-midline-preedit-occlusion.spec.ts' \ + 'tests/e2e/terminal-macos-system-key-remap.spec.ts' \ + 'tests/e2e/terminal-macos-text-substitution.spec.ts' \ 'tests/e2e/terminal-korean-preedit-visibility.spec.ts' | sort -u)" fi TEST_FILES_JSON="$(printf '%s\n' "$TEST_FILES" | jq --raw-input --slurp --compact-output 'split("\n") | map(select(length > 0))')" diff --git a/src/renderer/src/components/terminal-pane/terminal-ime-native-text-forwarder.ts b/src/renderer/src/components/terminal-pane/terminal-ime-native-text-forwarder.ts index 61fa940a9fa..8e1a7b0ac0d 100644 --- a/src/renderer/src/components/terminal-pane/terminal-ime-native-text-forwarder.ts +++ b/src/renderer/src/components/terminal-pane/terminal-ime-native-text-forwarder.ts @@ -31,6 +31,25 @@ type PendingCommit = { press: ClaimedKeyPress /** Non-null when `keyup` preceded `insertText` — an order some macOS IMEs use. */ keyup: ImeReleaseKeyEvent | null + /** + * Set when the text system deleted text it had already committed before + * delivering this keystroke's own — the shape of a text-*editing* convenience + * like macOS automatic period substitution rewriting `" "` into `". "`, or the + * dash, quote and text-replacement rules in the same family. + * + * That is a different thing from an input source mapping one physical key to + * one different character, which is what this forwarder exists to carry. + * Nothing here honours the editing kind: no reference terminal does, because + * none of them is a text view, so the substitution never reaches them and two + * spaces stay two spaces. A browser-based terminal cannot opt out that way — + * `autocorrect` and `spellcheck` on the helper textarea govern the browser's + * own correction, not the system text service — so the rewrite is declined + * here instead and the key sends what it produced. + * + * The discriminator is structural, not a character table: a + * one-key-one-character substitution never deletes first. + */ + rewritten: boolean } /** @@ -265,7 +284,8 @@ export function installTerminalImeNativeTextForwarder(args: { capsLock: event.getModifierState?.('CapsLock') === true, numLock: event.getModifierState?.('NumLock') === true }, - keyup: null + keyup: null, + rewritten: false } return true } @@ -328,6 +348,15 @@ export function installTerminalImeNativeTextForwarder(args: { if (!commit) { return } + if (event.inputType.startsWith('delete')) { + // Why the claim stays pending: this deletion is the text system rewriting + // what it already committed, and the `insertText` carrying the rewrite is + // still inbound. Retiring here sent nothing and left that insert with no + // claim to attach to, so the whole keystroke produced no bytes at all. + commit.rewritten = true + event.stopImmediatePropagation() + return + } pendingCommit = null if (event.inputType !== 'insertText') { // A non-text input takes the press over; it delivered nothing, so only the @@ -336,14 +365,17 @@ export function installTerminalImeNativeTextForwarder(args: { return } if (event.data) { + // A rewrite is declined, so what goes out is what the key produced rather + // than what the text system substituted for it. + const deliveredText = commit.rewritten ? commit.press.key : event.data // Read the mutable flags EXACTLY once, here: kitty state can change // between keydown and commit, and the release must describe the same // negotiation the press was encoded under. const encoding = encodeImeCommitForKitty(commit.press, args.getKittyKeyboardFlags?.() ?? 0, { - committedText: event.data, + committedText: deliveredText, layoutCharacterForCode: getLayoutCharacterForCode }) - args.sendInput(encoding.report ?? event.data) + args.sendInput(encoding.report ?? deliveredText) settleCommit(commit, encoding.release) } else { settleCommit(commit, null) diff --git a/src/renderer/src/components/terminal-pane/terminal-ime-retroactive-replacement-commit.test.ts b/src/renderer/src/components/terminal-pane/terminal-ime-retroactive-replacement-commit.test.ts new file mode 100644 index 00000000000..eff2be9572f --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-ime-retroactive-replacement-commit.test.ts @@ -0,0 +1,240 @@ +// @vitest-environment happy-dom +/** + * A text system that rewrites text the terminal has already sent. + * + * Every substitution the rest of the suite covers is decided before or during the keystroke that + * produces it — `,` commits as `，`, a `DefaultKeyBinding.dict` remap commits a backquote. One + * keydown, one character, no rewrite of anything already on the wire. That is an *input source* + * substitution and carrying it is what this forwarder exists for. + * + * macOS automatic period substitution is a different thing wearing the same clothes. It is a text + * *editing* convenience: type a second space and the system deletes the first one and inserts + * `". "` in its place. It reaches a browser text field — the helper textarea is one, and + * `autocorrect`/`spellcheck` do not govern the system text service — but it reaches no reference + * terminal, because none of them is a text view. Four were checked on hardware and all four + * deliver two spaces. The dash, smart-quote and text-replacement rules are the same family. + * + * So the rewrite is declined, and the keystroke sends what the key itself produced. The bug this + * pins is that it used to send *nothing at all*: the deletion arrives as an input event whose type + * is not `insertText`, which retired the claim without emitting, and the replacing `insertText` + * then found no claim to attach to. Two spaces became one. + * + * The discriminator is structural rather than a character table — a one-key-one-character + * substitution never deletes first — so nothing here reads which characters were involved. + */ import { Terminal } from '@xterm/xterm' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { installTerminalImeNativeTextForwarder } from './terminal-ime-native-text-forwarder' +import { shouldBypassXtermKeyboardEvent } from './xterm-bypass-policy' + +/** What a cooked tty leaves on the line after the erases the terminal sent are applied. */ +function resolveLine(sent: string): string { + const line: string[] = [] + for (const character of sent) { + if (character === '\x7f' || character === '\b') { + line.pop() + continue + } + line.push(character) + } + return line.join('') +} + +function open(kittyKeyboardFlags = 0): { + sent: () => string + textarea: HTMLTextAreaElement + dispose: () => void +} { + const container = document.createElement('div') + document.body.appendChild(container) + const terminal = new Terminal() + terminal.open(container) + const forwarder = installTerminalImeNativeTextForwarder({ + terminalElement: terminal.element, + isComposing: () => false, + sendInput: (data) => terminal.input(data), + getKittyKeyboardFlags: () => kittyKeyboardFlags + }) + terminal.attachCustomKeyEventHandler((event) => { + if (forwarder.claimKeyEvent(event)) { + return false + } + return !shouldBypassXtermKeyboardEvent(event, { + isMac: true, + hasSelection: false, + kittyKeyboardFlags + }) + }) + const emitted: string[] = [] + terminal.onData((data) => emitted.push(data)) + return { + sent: () => emitted.join(''), + textarea: terminal.textarea!, + dispose: () => { + forwarder.dispose() + terminal.dispose() + } + } +} + +function key( + textarea: HTMLTextAreaElement, + type: string, + init: { key: string; code: string; keyCode: number } +): KeyboardEvent { + const event = new KeyboardEvent(type, { + key: init.key, + code: init.code, + bubbles: true, + cancelable: true + }) + Object.defineProperty(event, 'keyCode', { value: init.keyCode }) + Object.defineProperty(event, 'charCode', { value: type === 'keypress' ? init.keyCode : 0 }) + textarea.dispatchEvent(event) + return event +} + +function inputEvent(textarea: HTMLTextAreaElement, inputType: string, data: string | null): void { + const event = new InputEvent('input', { bubbles: true }) + Object.defineProperty(event, 'inputType', { value: inputType }) + Object.defineProperty(event, 'data', { value: data }) + Object.defineProperty(event, 'composed', { value: true }) + textarea.dispatchEvent(event) +} + +/** A printable key the text system commits verbatim, in the order Chromium delivers it. */ +function pressPrintable( + textarea: HTMLTextAreaElement, + character: string, + code: string, + keyCode: number +): void { + const keydown = key(textarea, 'keydown', { key: character, code, keyCode }) + if (!keydown.defaultPrevented) { + key(textarea, 'keypress', { + key: character, + code, + keyCode: character.charCodeAt(0) + }) + } + textarea.value += character + inputEvent(textarea, 'insertText', character) + key(textarea, 'keyup', { key: character, code, keyCode }) +} + +/** + * The substituting keystroke: a keydown the text system swallows, then a keyless rewrite of text + * the terminal has already sent. + */ +function pressReplacing( + textarea: HTMLTextAreaElement, + press: { key: string; code: string; keyCode: number }, + replacement: { deleteCount: number; insertedText: string } +): void { + key(textarea, 'keydown', press) + const retained = Array.from(textarea.value) + retained.splice(retained.length - replacement.deleteCount, replacement.deleteCount) + textarea.value = retained.join('') + inputEvent(textarea, 'deleteContentBackward', null) + textarea.value += replacement.insertedText + inputEvent(textarea, 'insertText', replacement.insertedText) + key(textarea, 'keyup', press) +} + +describe('a text substitution decided after the terminal already sent the text', () => { + beforeEach(() => { + vi.spyOn(HTMLCanvasElement.prototype, 'getContext').mockReturnValue({ + measureText: () => ({ width: 10 }) + } as unknown as CanvasRenderingContext2D) + }) + afterEach(() => { + vi.restoreAllMocks() + document.body.replaceChildren() + }) + + // Guards the two expectations below from passing vacuously: if this harness stopped reaching the + // forwarder's listeners at all, `it.fails` would still be satisfied and report nothing. + it('delivers a non-replacing commit through the same harness', () => { + const pane = open() + try { + pressPrintable(pane.textarea, 'a', 'KeyA', 65) + pressPrintable(pane.textarea, 'b', 'KeyB', 66) + expect(resolveLine(pane.sent())).toBe('ab') + } finally { + pane.dispose() + } + }) + + // The retraction machinery itself, exercised where the textarea still holds the text being + // rewritten. It does not in the two cases below, which is the whole reason they still fail: the + // forwarder blanks the textarea after every commit, so by the time a rewrite arrives the diff + // has no left side to measure against. + // The declining rule itself. Two spaces in, two spaces out, and no period invented — which is + // what every reference terminal does with these keystrokes. + it('declines a rewrite and sends what the key produced', () => { + const pane = open() + try { + pressPrintable(pane.textarea, 'a', 'KeyA', 65) + pressPrintable(pane.textarea, 'b', 'KeyB', 66) + pressPrintable(pane.textarea, ' ', 'Space', 32) + pressReplacing( + pane.textarea, + { key: ' ', code: 'Space', keyCode: 32 }, + { deleteCount: 1, insertedText: '. ' } + ) + expect(resolveLine(pane.sent())).toBe('ab ') + } finally { + pane.dispose() + } + }) + + // The regression that motivated all of this: before the fix the deletion retired the claim and + // the keystroke produced no bytes, so the user lost the space entirely. + it('does not swallow the keystroke the rewrite was attached to', () => { + const pane = open() + try { + pressReplacing( + pane.textarea, + { key: ' ', code: 'Space', keyCode: 32 }, + { deleteCount: 0, insertedText: '. ' } + ) + expect(pane.sent()).not.toBe('') + } finally { + pane.dispose() + } + }) + + // Same family, same rule: `--` must stay `--` rather than becoming an em dash. + it('declines a dash substitution', () => { + const pane = open() + try { + pressPrintable(pane.textarea, '-', 'Minus', 189) + pressReplacing( + pane.textarea, + { key: '-', code: 'Minus', keyCode: 189 }, + { deleteCount: 1, insertedText: '—' } + ) + expect(resolveLine(pane.sent())).toBe('--') + } finally { + pane.dispose() + } + }) + + // Bit 3 asks for every printable key as a CSI-u report. The declining rule has to hold there + // too: the report identity is the physical key, and the substituted character must not ride + // along as associated text. + it('declines a rewrite on a pane that negotiated kitty bit 3', () => { + const pane = open(8) + try { + pressReplacing( + pane.textarea, + { key: ' ', code: 'Space', keyCode: 32 }, + { deleteCount: 1, insertedText: '. ' } + ) + const sent = pane.sent() + expect(sent).not.toBe('') + expect(sent).not.toContain('.') + } finally { + pane.dispose() + } + }) +}) diff --git a/tests/e2e/terminal-ime-cdp-composition.ts b/tests/e2e/terminal-ime-cdp-composition.ts index a151b23aed5..a70283f568c 100644 --- a/tests/e2e/terminal-ime-cdp-composition.ts +++ b/tests/e2e/terminal-ime-cdp-composition.ts @@ -173,3 +173,51 @@ export async function dispatchResumedCompositionUpdate(page: Page, data: string) ) }, data) } + +/** + * Rewrites text the terminal has already committed, the way a text system that decides *after the + * fact* does: the previous characters are removed and replaced, with no key event of their own. + * + * Two producers share this shape. macOS automatic period substitution rewrites a trailing `" "` + * into `". "` on the second space, and a touch-iOS Korean source rewrites the syllable in place on + * every jamo (`ㅎ` -> `하` -> `한`) while firing no composition event at all. In both the deletion + * carries no `keydown`, so nothing that decides on key events can see it coming. + * + * SYNTHESISED rather than driven through CDP, and the reason is structural: `Input.insertText` + * only inserts at the caret and there is no CDP command that produces a keyless + * `deleteContentBackward`. Dispatching a real Backspace would work, but it would also produce a + * Backspace *keydown* the real substitution never sends — which is the whole difficulty. Same + * escape hatch, and same justification, as `dispatchResumedCompositionUpdate` above. + * + * The textarea value is mutated before each event because the deferred textarea diff reads it, so + * a spec that dispatched the events alone would exercise only half the path. + */ +export async function dispatchRetroactiveTextReplacement( + page: Page, + replacement: { deleteCount: number | 'all'; insertedText: string } +): Promise { + await page.evaluate((step: { deleteCount: number | 'all'; insertedText: string }) => { + const textarea = document.querySelector('.xterm-helper-textarea:focus') + if (!textarea) { + throw new Error('xterm helper textarea is not focused') + } + const fire = (type: string, inputType: string, data: string | null): void => { + textarea.dispatchEvent( + new InputEvent(type, { bubbles: true, composed: true, data, inputType }) + ) + } + // 'all' is for a source that replaces its whole in-progress region rather than a fixed count, + // which is what an in-place syllable rewrite does and what keeps this robust to whether the + // keystroke's own character was inserted before the rewrite ran. + const retained = Array.from(textarea.value) + const deleteCount = step.deleteCount === 'all' ? retained.length : step.deleteCount + retained.splice(retained.length - deleteCount, deleteCount) + textarea.value = retained.join('') + fire('beforeinput', 'deleteContentBackward', null) + fire('input', 'deleteContentBackward', null) + textarea.value += step.insertedText + textarea.setSelectionRange(textarea.value.length, textarea.value.length) + fire('beforeinput', 'insertText', step.insertedText) + fire('input', 'insertText', step.insertedText) + }, replacement) +} diff --git a/tests/e2e/terminal-macos-text-substitution.spec.ts b/tests/e2e/terminal-macos-text-substitution.spec.ts new file mode 100644 index 00000000000..00d476dbd8d --- /dev/null +++ b/tests/e2e/terminal-macos-text-substitution.spec.ts @@ -0,0 +1,165 @@ +/** + * Headless end-to-end coverage for macOS system text substitution reaching the terminal. + * + * This restores, as CDP-synthesised specs that run in the normal headless project, coverage two + * `@headful` macOS-only specs used to carry before they were removed as collateral in + * 17cfc968cf5. Those two depended on an accessibility grant, a live system input source and a + * Swift helper, and one was gated behind an env var, so CI had no lane that ran either. + * + * They also covered less than their names suggest. One typed a period and a space under the Latin + * source and a Hangul word plus a space under the Korean source and asserted that **nothing extra** + * appeared — recon for a report that automatic period substitution corrupts terminal input, which + * recorded the report's stated trigger not reproducing. The other explicitly "asserts + * preconditions, not outcomes". Neither was a regression test. + * + * Two things are pinned here, and they are the same guarantee stated twice: + * + * 1. Typing that looks like it might attract a substitution does not attract one. A Hangul commit + * followed by a word-boundary space, and a Latin period followed by a space, each reach the pty + * as exactly what was typed. + * 2. A substitution that really is delivered is **declined**. macOS automatic period substitution + * rewrites a trailing `" "` into `". "` on the second space, and it does reach a browser text + * field — the helper textarea is one. It reaches no reference terminal: four were checked on + * hardware and all four deliver two spaces, because none of them is a text view. So the + * terminal has to decline it explicitly, and the regression guarded is that the substituting + * keystroke used to produce no bytes at all, turning two spaces into one. + * + * Assertions are on the bytes the pty child receives, through the tty line discipline, because + * that is the line the user ends up with. + */ import { expect, test } from './helpers/orca-app' +import { closeTerminalImePaneArena, openTerminalImePaneArena } from './terminal-ime-pane-arena' +import { readTerminalImeBoundaryTrace } from './terminal-ime-boundary-probe' +import { + commitImeText, + dispatchImeProcessKey, + dispatchImeRewrittenPrintableKey, + dispatchPlainEnter, + dispatchRetroactiveTextReplacement, + setImeComposition, + type ImeKeyIdentity +} from './terminal-ime-cdp-composition' +import { + createTerminalImeByteReader, + removeTerminalImeByteReader, + startTerminalImeByteReader, + waitForTerminalImeBytes +} from './terminal-ime-byte-reader' +import { applyImePlatformPolicy, expectImePlatformPolicy } from './terminal-ime-platform-policy' + +const SPACE: ImeKeyIdentity = { key: ' ', code: 'Space', keyCode: 32 } +const PERIOD: ImeKeyIdentity = { key: '.', code: 'Period', keyCode: 190 } + +/** macOS virtual key codes the removed spec typed, in the identities Chromium reports for them. */ +const KEY_A: ImeKeyIdentity = { key: 'a', code: 'KeyA', keyCode: 65 } +const KEY_B: ImeKeyIdentity = { key: 'b', code: 'KeyB', keyCode: 66 } + +test.describe('Terminal macOS text substitution', () => { + test('sends a Hangul commit and its word-boundary space with nothing added', async ({ + orcaPage, + testRepoPath + }, testInfo) => { + // The removed spec's Korean arm: 2-Set d+k composes 아, then Space separates the word. It ran + // with NSAutomaticPeriodSubstitutionEnabled on and recorded `아 ` — one space, no period. The + // guard is that the terminal adds nothing of its own at a word boundary. + await applyImePlatformPolicy(orcaPage, 'mac') + await expectImePlatformPolicy(orcaPage, 'mac') + const arena = await openTerminalImePaneArena(orcaPage) + const reader = createTerminalImeByteReader(testRepoPath, 1) + let completed = false + try { + await startTerminalImeByteReader(orcaPage, arena.ptyId, reader) + for (const frame of ['ㅇ', '아']) { + await dispatchImeProcessKey(arena.session, { key: 'Process', code: 'KeyD' }) + await setImeComposition(arena.session, frame) + } + await commitImeText(arena.session, '아') + await orcaPage.waitForTimeout(60) + await dispatchImeRewrittenPrintableKey(arena.session, SPACE) + await orcaPage.waitForTimeout(60) + await dispatchPlainEnter(arena.session) + + expect((await readTerminalImeBoundaryTrace(orcaPage)).onData.join('')).toBe('아 \r') + expect(await waitForTerminalImeBytes(orcaPage, reader)).toEqual([ + Buffer.from('아 \n').toString('hex') + ]) + completed = true + } finally { + await closeTerminalImePaneArena(arena, testInfo, 'hangul-word-boundary-space', !completed) + removeTerminalImeByteReader(reader) + } + }) + + test('sends a Latin period and space with nothing added', async ({ + orcaPage, + testRepoPath + }, testInfo) => { + // The removed spec's ABC arm. A period immediately before a space is the position automatic + // period substitution acts on, so this is the paired control: with no second space there is + // nothing to substitute and the terminal must send exactly the two characters typed. + await applyImePlatformPolicy(orcaPage, 'mac') + const arena = await openTerminalImePaneArena(orcaPage) + const reader = createTerminalImeByteReader(testRepoPath, 1) + let completed = false + try { + await startTerminalImeByteReader(orcaPage, arena.ptyId, reader) + for (const identity of [PERIOD, SPACE]) { + await dispatchImeRewrittenPrintableKey(arena.session, identity) + await orcaPage.waitForTimeout(60) + } + await dispatchPlainEnter(arena.session) + + expect((await readTerminalImeBoundaryTrace(orcaPage)).onData.join('')).toBe('. \r') + expect(await waitForTerminalImeBytes(orcaPage, reader)).toEqual([ + Buffer.from('. \n').toString('hex') + ]) + completed = true + } finally { + await closeTerminalImePaneArena(arena, testInfo, 'latin-period-space', !completed) + removeTerminalImeByteReader(reader) + } + }) + + test('declines an automatic period substitution and sends both spaces', async ({ + orcaPage, + testRepoPath + }, testInfo) => { + // Declined, not honoured. Four reference terminals were checked on hardware and none of them + // applies this substitution — they are not text views, so it never reaches them and two spaces + // stay two spaces. A browser-based terminal does receive it, so it has to decline explicitly. + // The regression guarded here is that the substituting keystroke used to produce no bytes at + // all, turning two spaces into one. + await applyImePlatformPolicy(orcaPage, 'mac') + const arena = await openTerminalImePaneArena(orcaPage) + const reader = createTerminalImeByteReader(testRepoPath, 1) + let completed = false + try { + await startTerminalImeByteReader(orcaPage, arena.ptyId, reader) + for (const identity of [KEY_A, KEY_B, SPACE]) { + await dispatchImeRewrittenPrintableKey(arena.session, identity) + await orcaPage.waitForTimeout(60) + } + // The second Space produces no character of its own: the text system swallows it and + // rewrites the trailing space into `. `. + await arena.session.send('Input.dispatchKeyEvent', { + type: 'rawKeyDown', + key: ' ', + code: 'Space', + windowsVirtualKeyCode: 32, + nativeVirtualKeyCode: 32, + text: '', + unmodifiedText: '' + }) + await dispatchRetroactiveTextReplacement(orcaPage, { deleteCount: 1, insertedText: '. ' }) + await orcaPage.waitForTimeout(60) + await dispatchPlainEnter(arena.session) + + expect(await waitForTerminalImeBytes(orcaPage, reader)).toEqual([ + Buffer.from('ab \n').toString('hex') + ]) + completed = true + } finally { + await closeTerminalImePaneArena(arena, testInfo, 'automatic-period-substitution', !completed) + removeTerminalImeByteReader(reader) + } + }) +})