From 66340bffc58b021277f070de2a50b7d91bdcf539 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:16:32 -0700 Subject: [PATCH] fix(terminal): give the composing-chord deferral an owner and a ceiling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chord held for a live composition waits with `fallbackMs: null`, so nothing but compositionend can end it. `sendTerminalInputAfterComposition` returned void, so nobody could stop it either: the two listeners it puts on the terminal element outlived the pane, and a later composition on that element flushed the stale chord. Return a disposer from the helper and put a sender in front of it that owns every pending chord, so blur and pane teardown drop them the way the Enter path already clears its state. The sender also bounds the wait — generous enough for a conversion candidate window, and it discards rather than sends, because a chord arriving mid-preedit is the corruption the wait exists to prevent. The Enter path needs none of this: its 200 ms fallback always runs, so its listeners cannot outlive it. Pinned so that stays true. Fixes STA-4476 --- ...oard-handlers-ime-composing-chord.test.tsx | 79 ++++++++++++++++++ .../terminal-pane/keyboard-handlers.ts | 14 ++-- .../terminal-ime-deferred-chord.test.ts | 81 +++++++++++++++++++ .../terminal-ime-deferred-chord.ts | 51 ++++++++++++ .../terminal-ime-deferred-newline.test.ts | 40 ++++++++- .../terminal-ime-deferred-newline.ts | 21 ++++- 6 files changed, 275 insertions(+), 11 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.test.ts create mode 100644 src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.ts diff --git a/src/renderer/src/components/terminal-pane/keyboard-handlers-ime-composing-chord.test.tsx b/src/renderer/src/components/terminal-pane/keyboard-handlers-ime-composing-chord.test.tsx index 2200f2d5c9ee..f13a57115b73 100644 --- a/src/renderer/src/components/terminal-pane/keyboard-handlers-ime-composing-chord.test.tsx +++ b/src/renderer/src/components/terminal-pane/keyboard-handlers-ime-composing-chord.test.tsx @@ -14,6 +14,7 @@ import { XTERM_COMPOSITION_SESSION_END_EVENT, XTERM_COMPOSITION_SESSION_START_EVENT } from './terminal-ime-composition-route' +import { TERMINAL_IME_DEFERRED_CHORD_ABANDON_MS } from './terminal-ime-deferred-chord' import { useTerminalKeyboardShortcuts } from './keyboard-handlers' type KeyboardHandlersDeps = Parameters[0] @@ -30,12 +31,34 @@ function keyboardEvent( return event } +/** Live registrations on the terminal element, so a deferral that never disposes is visible. */ +function trackCompositionListeners(element: HTMLElement): () => number { + const live = new Set() + const watched = new Set(['compositionend', XTERM_COMPOSITION_SESSION_END_EVENT]) + const { addEventListener, removeEventListener } = element + element.addEventListener = function (type, listener, options): void { + if (watched.has(type) && listener) { + live.add(listener) + } + addEventListener.call(this, type, listener, options) + } + element.removeEventListener = function (type, listener, options): void { + if (watched.has(type) && listener) { + live.delete(listener) + } + removeEventListener.call(this, type, listener, options) + } + return () => live.size +} + function createHarness(): { deps: KeyboardHandlersDeps terminalElement: HTMLDivElement terminalInput: HTMLTextAreaElement /** Every byte reaching the pty, in arrival order, whichever route it took. */ wire: string[] + /** Registrations added after the composition route's own, i.e. the deferrals still waiting. */ + deferralListenerCount: () => number startComposition: () => void endComposition: (data: string) => void dispose: () => void @@ -77,6 +100,8 @@ function createHarness(): { capturedTransport: transport, getCurrentTransport: () => transport }) + // Installed after the route so only the deferral's own registrations are counted. + const deferralListenerCount = trackCompositionListeners(terminalElement) const deps: KeyboardHandlersDeps = { tabId: 'tab-1', @@ -110,6 +135,7 @@ function createHarness(): { terminalElement, terminalInput, wire, + deferralListenerCount, startComposition: () => { terminalElement.dispatchEvent( new CustomEvent(XTERM_COMPOSITION_SESSION_START_EVENT, { detail: { id: 1 } }) @@ -220,4 +246,57 @@ describe('a cursor chord pressed during a composition', () => { hook.unmount() harness.dispose() }) + + // STA-4476: an indefinite wait has no exit of its own. Without a disposer the listeners outlive + // the pane and a later composition on the same element flushes the stale chord. + it('drops the held chord and its listeners when the pane tears down', () => { + const harness = createHarness() + const hook = renderHook(() => useTerminalKeyboardShortcuts(harness.deps)) + + harness.startComposition() + pressCmdArrowLeft(harness, true) + expect(harness.deferralListenerCount()).toBeGreaterThan(0) + + hook.unmount() + harness.endComposition('다') + vi.runAllTimers() + + expect(harness.wire).toEqual(['다']) + expect(harness.deferralListenerCount()).toBe(0) + harness.dispose() + }) + + it('drops the held chord and its listeners on blur', () => { + const harness = createHarness() + const hook = renderHook(() => useTerminalKeyboardShortcuts(harness.deps)) + + harness.startComposition() + pressCmdArrowLeft(harness, true) + window.dispatchEvent(new Event('blur')) + harness.endComposition('다') + vi.runAllTimers() + + expect(harness.wire).toEqual(['다']) + expect(harness.deferralListenerCount()).toBe(0) + hook.unmount() + harness.dispose() + }) + + // A composition that never commits must not strand the wait forever. The chord is discarded, + // never sent late — a late send is #12871 again. + it('abandons a chord whose composition never ends', () => { + const harness = createHarness() + const hook = renderHook(() => useTerminalKeyboardShortcuts(harness.deps)) + + harness.startComposition() + pressCmdArrowLeft(harness, true) + vi.advanceTimersByTime(TERMINAL_IME_DEFERRED_CHORD_ABANDON_MS + 1) + harness.endComposition('다') + vi.runAllTimers() + + expect(harness.wire).toEqual(['다']) + expect(harness.deferralListenerCount()).toBe(0) + hook.unmount() + harness.dispose() + }) }) diff --git a/src/renderer/src/components/terminal-pane/keyboard-handlers.ts b/src/renderer/src/components/terminal-pane/keyboard-handlers.ts index 0a5fe0ac5b93..3614af3b888f 100644 --- a/src/renderer/src/components/terminal-pane/keyboard-handlers.ts +++ b/src/renderer/src/components/terminal-pane/keyboard-handlers.ts @@ -15,9 +15,9 @@ import { getTerminalImeModifiedEnterKind, isTerminalImeConsumedKey, isTerminalImeEnterKeyUp, - isTerminalImeProcessEnter, - sendTerminalInputAfterComposition + isTerminalImeProcessEnter } from './terminal-ime-deferred-newline' +import { createTerminalImeDeferredChordSender } from './terminal-ime-deferred-chord' import { hasPendingTerminalImeComposition } from './terminal-ime-composition-route' import { requestCapturedTerminalReconfirmation, @@ -290,6 +290,7 @@ export function useTerminalKeyboardShortcuts({ const terminalImeEnterModifierKeydowns = new Set<'shift' | 'ctrl'>() const nativeOnlyShortcutTracker = createTerminalNativeOnlyShortcutTracker() const deferredNewlineSender = createTerminalImeDeferredNewlineSender() + const deferredChordSender = createTerminalImeDeferredChordSender() const modifiedEnterChordOwner = createTerminalImeModifiedEnterChordOwner() const reconcileHeldImeEnterModifiers = ( event: KeyboardEvent, @@ -651,11 +652,10 @@ export function useTerminalKeyboardShortcuts({ // runs after this keydown. Sending now puts a cursor chord ahead of the text it was typed // after — `가나다` then Cmd+Left leaves `다가나` (#12871). Enter is handled above, where a // fallback timer is right because a newline arriving late still arrives; a chord arriving - // mid-preedit is the corruption itself, so this one waits without a deadline. + // mid-preedit is the corruption itself, so this one waits on the composition rather than a + // deadline. The sender owns the wait so blur and teardown can drop it. if (e.isComposing || hasPendingImeComposition) { - sendTerminalInputAfterComposition(pane.terminal.element, sendResolvedInput, { - fallbackMs: null - }) + deferredChordSender.defer(pane.terminal.element, sendResolvedInput) return } sendResolvedInput() @@ -986,6 +986,7 @@ export function useTerminalKeyboardShortcuts({ terminalImeEnterModifierKeydowns.clear() modifiedEnterChordOwner.clear() deferredNewlineSender.clearRedispatchedEnters() + deferredChordSender.cancelPending() observedEnterKeydownTimeStamps.clear() } @@ -1000,6 +1001,7 @@ export function useTerminalKeyboardShortcuts({ optionKittyReleases.clear() modifiedEnterChordOwner.clear() deferredNewlineSender.clearRedispatchedEnters() + deferredChordSender.cancelPending() observedEnterKeydownTimeStamps.clear() window.removeEventListener('keydown', onModifierDown, { capture: true }) window.removeEventListener('keyup', onKeyUp, { capture: true }) diff --git a/src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.test.ts b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.test.ts new file mode 100644 index 000000000000..d54ab6de4556 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.test.ts @@ -0,0 +1,81 @@ +// @vitest-environment happy-dom +// STA-4476: the composing-chord deferral waits on the composition, not a deadline (#12871), so it +// is the sender that has to guarantee an exit — otherwise the wait and its listeners outlive the +// pane and a later composition flushes a stale chord. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createTerminalImeDeferredChordSender, + TERMINAL_IME_DEFERRED_CHORD_ABANDON_MS +} from './terminal-ime-deferred-chord' +import { XTERM_COMPOSITION_SESSION_END_EVENT } from './terminal-ime-composition-route' + +describe('createTerminalImeDeferredChordSender', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + afterEach(() => { + vi.useRealTimers() + }) + + it('sends once the composition commits', () => { + const el = document.createElement('div') + const send = vi.fn() + const sender = createTerminalImeDeferredChordSender() + + sender.defer(el, send) + expect(send).not.toHaveBeenCalled() + + el.dispatchEvent(new Event('compositionend')) + vi.advanceTimersByTime(0) + expect(send).toHaveBeenCalledTimes(1) + }) + + it('cancels every pending chord and detaches its listeners', () => { + const el = document.createElement('div') + const removeEventListener = vi.spyOn(el, 'removeEventListener') + const first = vi.fn() + const second = vi.fn() + const sender = createTerminalImeDeferredChordSender() + + sender.defer(el, first) + sender.defer(el, second) + sender.cancelPending() + + expect(removeEventListener).toHaveBeenCalledTimes(4) + + el.dispatchEvent(new Event('compositionend')) + el.dispatchEvent(new CustomEvent(XTERM_COMPOSITION_SESSION_END_EVENT)) + vi.runAllTimers() + expect(first).not.toHaveBeenCalled() + expect(second).not.toHaveBeenCalled() + }) + + it('abandons a chord whose composition never ends rather than sending it late', () => { + const el = document.createElement('div') + const send = vi.fn() + const sender = createTerminalImeDeferredChordSender() + + sender.defer(el, send) + vi.advanceTimersByTime(TERMINAL_IME_DEFERRED_CHORD_ABANDON_MS - 1) + expect(send).not.toHaveBeenCalled() + + vi.advanceTimersByTime(2) + el.dispatchEvent(new Event('compositionend')) + vi.runAllTimers() + expect(send).not.toHaveBeenCalled() + }) + + it('stops tracking a chord that already sent, so a later cancel is inert', () => { + const el = document.createElement('div') + const send = vi.fn() + const sender = createTerminalImeDeferredChordSender() + + sender.defer(el, send) + el.dispatchEvent(new Event('compositionend')) + vi.advanceTimersByTime(0) + sender.cancelPending() + vi.runAllTimers() + + expect(send).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.ts b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.ts new file mode 100644 index 000000000000..8598a0ec1f8f --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-chord.ts @@ -0,0 +1,51 @@ +import { sendTerminalInputAfterComposition } from './terminal-ime-deferred-newline' + +/** + * Ceiling on the otherwise indefinite wait. Generous enough for a conversion candidate window, + * which can stay open for seconds; a chord that outlives it is discarded rather than sent late, + * because firing mid-preedit is the corruption the wait exists to prevent (#12871). + */ +export const TERMINAL_IME_DEFERRED_CHORD_ABANDON_MS = 10_000 + +export type TerminalImeDeferredChordSender = { + defer: (terminalElement: HTMLElement | null | undefined, send: () => void) => void + cancelPending: () => void +} + +/** + * Owns every chord held for a live composition so blur and pane teardown can drop them. An + * unowned deferral has no exit when compositionend never arrives: its listeners outlive the pane + * and a later composition flushes the stale chord against a rebound terminal. + */ +export function createTerminalImeDeferredChordSender(): TerminalImeDeferredChordSender { + const pendingStops = new Set<() => void>() + + return { + defer: (terminalElement, send) => { + let abandonTimer: number | undefined + let stopComposingWait: (() => void) | null = null + const stopWaiting = (): void => { + window.clearTimeout(abandonTimer) + pendingStops.delete(stopWaiting) + stopComposingWait?.() + } + abandonTimer = window.setTimeout(stopWaiting, TERMINAL_IME_DEFERRED_CHORD_ABANDON_MS) + pendingStops.add(stopWaiting) + stopComposingWait = sendTerminalInputAfterComposition( + terminalElement, + () => { + stopWaiting() + send() + }, + { fallbackMs: null } + ) + }, + cancelPending: () => { + // Each stop removes itself; deleting the visited entry is safe for a Set iterator. + for (const stopWaiting of pendingStops) { + stopWaiting() + } + pendingStops.clear() + } + } +} diff --git a/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.test.ts b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.test.ts index 5b8613c1f7fd..0ac8ce17377c 100644 --- a/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.test.ts @@ -6,7 +6,8 @@ import { createTerminalImeModifiedEnterChordOwner, isTerminalImeEnterKeyUp, isTerminalImeProcessEnter, - sendTerminalInputAfterComposition + sendTerminalInputAfterComposition, + TERMINAL_IME_DEFERRED_NEWLINE_FALLBACK_MS } from './terminal-ime-deferred-newline' import { installTerminalImeCompositionRoute, @@ -53,6 +54,27 @@ describe('sendTerminalInputAfterComposition', () => { expect(send).toHaveBeenCalledTimes(1) }) + // STA-4476: `fallbackMs: null` has no exit of its own, so the disposer is the only thing that + // can detach the listeners when compositionend never arrives. + it('detaches its listeners and never sends once disposed', () => { + const el = document.createElement('div') + const send = vi.fn() + const removeEventListener = vi.spyOn(el, 'removeEventListener') + + const dispose = sendTerminalInputAfterComposition(el, send, { fallbackMs: null }) + dispose() + + expect(removeEventListener).toHaveBeenCalledWith('compositionend', expect.any(Function)) + expect(removeEventListener).toHaveBeenCalledWith( + XTERM_COMPOSITION_SESSION_END_EVENT, + expect.any(Function) + ) + + el.dispatchEvent(new Event('compositionend')) + vi.runAllTimers() + expect(send).not.toHaveBeenCalled() + }) + it('falls back to sending when no compositionend arrives', () => { const el = document.createElement('div') const send = vi.fn() @@ -63,6 +85,22 @@ describe('sendTerminalInputAfterComposition', () => { expect(send).toHaveBeenCalledTimes(1) }) + // STA-4476: why the Enter path needs no disposer of its own — its fallback always runs, so the + // listeners cannot outlive it even when the composition never ends. + it('detaches its listeners on the fallback, not only on compositionend', () => { + const el = document.createElement('div') + const removeEventListener = vi.spyOn(el, 'removeEventListener') + + sendTerminalInputAfterComposition(el, vi.fn()) + vi.advanceTimersByTime(TERMINAL_IME_DEFERRED_NEWLINE_FALLBACK_MS) + + expect(removeEventListener).toHaveBeenCalledWith('compositionend', expect.any(Function)) + expect(removeEventListener).toHaveBeenCalledWith( + XTERM_COMPOSITION_SESSION_END_EVENT, + expect.any(Function) + ) + }) + it('finishes from the captured xterm transaction when deferral starts after compositionend', () => { const el = document.createElement('div') const send = vi.fn() diff --git a/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.ts b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.ts index 6eb2ce0e7e66..b27f25e4742d 100644 --- a/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.ts +++ b/src/renderer/src/components/terminal-pane/terminal-ime-deferred-newline.ts @@ -10,15 +10,19 @@ export const TERMINAL_IME_DEFERRED_NEWLINE_FALLBACK_MS = 200 * right for it; a cursor chord arriving mid-preedit reproduces the corruption the wait exists to * prevent, and a conversion can hold its candidate window open for seconds. Dropping the chord * costs one keypress, firing early costs a mangled line. + * + * Returns a disposer that stops waiting without sending. An indefinite wait has no other exit, so + * a caller that can outlive the composition must hold it — otherwise the listeners stay on the + * terminal element and a later composition flushes the stale send. */ export function sendTerminalInputAfterComposition( terminalElement: HTMLElement | null | undefined, send: () => void, options?: { fallbackMs?: number | null } -): void { +): () => void { if (!terminalElement) { - window.setTimeout(send, 0) - return + const immediateTimer = window.setTimeout(send, 0) + return () => window.clearTimeout(immediateTimer) } const fallbackMs = @@ -27,7 +31,7 @@ export function sendTerminalInputAfterComposition( : (options?.fallbackMs ?? TERMINAL_IME_DEFERRED_NEWLINE_FALLBACK_MS) let done = false - const finish = (): void => { + const stopWaiting = (): void => { if (done) { return } @@ -40,6 +44,13 @@ export function sendTerminalInputAfterComposition( if (fallbackTimer !== undefined) { window.clearTimeout(fallbackTimer) } + } + + const finish = (): void => { + if (done) { + return + } + stopWaiting() // xterm flushes the committed glyph after compositionend. window.setTimeout(send, 0) } @@ -54,6 +65,8 @@ export function sendTerminalInputAfterComposition( terminalElement.addEventListener('compositionend', onCompositionEnd) terminalElement.addEventListener(XTERM_COMPOSITION_SESSION_END_EVENT, onCompositionSessionEnd) const fallbackTimer = fallbackMs === null ? undefined : window.setTimeout(finish, fallbackMs) + + return stopWaiting } export type TerminalImeDeferredNewlineSender = {