mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-07 16:09:43 +02:00
fix(terminal): forward the orphaned input event instead of replaying a guessed key
The previous shape guessed the character from `event.key` on keydown, re-emitted it, and then tried to suppress a late canonical copy with a 250 ms character-keyed dedupe. Review found three defects in that, all reproducible: the dedupe matched on the character alone with nothing scoping a candidate to the keydown that created it, so the same character typed twice inside the window had its second, real byte swallowed; anything whose committed text differed from `event.key` (Enter, IME punctuation) was delivered twice, because the dedupe could never match it; and the trigger ignored `key === 'Unidentified'`, which is what a soft keyboard reports, so it may never have fired where it was needed. The input event already carries the committed text in `ev.data` — exactly what xterm itself would have forwarded — so nothing has to be guessed. The controller now only decides WHETHER to forward, by asking whether xterm produced canonical data since the keydown that began the keystroke. No character-keyed matching survives, so the first two defects are structurally impossible rather than defended against, and nothing reads `key`/`keyCode`, so the third cannot recur. Three details are load-bearing and each has a test that fails without it: - The "did xterm speak?" snapshot is taken at KEYDOWN, not at the input event. `_keyPress` emits and sets `_keyPressHandled` before `input` fires, so a snapshot read at input time already contains that emission, reads it as silence, and delivers the character twice. - Our `input` listener is registered with `capture: true`. The target is visited twice in the event path, so a capture listener calling `stopPropagation()` stops later BUBBLE listeners on that same target; xterm's `cancel()` runs exactly in the branch where it handled the input, so on bubble we would never observe handled events, and whether we observed them at all would hang off `options.cancelEvents`. Measured in jsdom and headless chromium; the table is in the module header. - Enter is deliberately no longer special-cased. That mapping is what made the committed text differ from the re-emitted value in the first place. The scope is also narrower than the old name suggests, and the browser test now proves it rather than assuming it. For a keydown that reports keyCode 229 xterm ALREADY self-rescues, via `CompositionHelper._handleAnyTextareaChanges()` diffing the helper textarea on a 0 ms timer. A test asserting "we recovered it" there passes while xterm does all the work, so the browser tests assert WHO delivered the byte: zero canonical emissions for the genuinely orphaned case, exactly one delivery for the case xterm rescues itself. Also addresses review notes: the module gains an `@fileoverview` with `@dependency`/`@loadorder` and an entry in the load-order list and module inventory, and the wiring test moves out of the Ctrl+C smart-copy file into its own. The keydown hook deliberately still runs for every key event rather than moving behind the 229 gate: gating it would reinstate exactly the blindness described above, and it is now a single counter assignment.
This commit is contained in:
Aamer Akhter
@@ -1,3 +1,15 @@
|
||||
/**
|
||||
* Orphaned-input recovery for xterm's helper textarea (PR #388 / COD-27).
|
||||
*
|
||||
* xterm's CoreBrowserTerminal._inputEvent only forwards an `insertText` input
|
||||
* event when `(!ev.composed || !this._keyDownSeen)`. Chrome-on-Android's soft
|
||||
* keyboard produces `composed: true` input events preceded by a keydown, so
|
||||
* that guard is false and the committed character is silently dropped.
|
||||
*
|
||||
* The controller under test forwards the event's own `data` when — and only
|
||||
* when — xterm produced no canonical data for that keystroke. These tests
|
||||
* drive it with synthetic events and an injected timer; no browser is needed.
|
||||
*/
|
||||
import { readFileSync } from 'node:fs';
|
||||
import vm from 'node:vm';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
@@ -6,74 +18,66 @@ type Listener = (event: Record<string, unknown>) => void;
|
||||
|
||||
function makeTextarea() {
|
||||
const listeners = new Map<string, Set<Listener>>();
|
||||
const registrations: Array<{ type: string; capture: unknown }> = [];
|
||||
return {
|
||||
addEventListener(type: string, listener: Listener) {
|
||||
addEventListener(type: string, listener: Listener, capture?: unknown) {
|
||||
const bucket = listeners.get(type) ?? new Set<Listener>();
|
||||
bucket.add(listener);
|
||||
listeners.set(type, bucket);
|
||||
registrations.push({ type, capture });
|
||||
},
|
||||
removeEventListener(type: string, listener: Listener) {
|
||||
listeners.get(type)?.delete(listener);
|
||||
},
|
||||
fire(type: string, event: Record<string, unknown> = {}) {
|
||||
for (const listener of listeners.get(type) ?? []) listener({ type, ...event });
|
||||
for (const listener of [...(listeners.get(type) ?? [])]) listener({ type, ...event });
|
||||
},
|
||||
listenerCount() {
|
||||
return [...listeners.values()].reduce((total, bucket) => total + bucket.size, 0);
|
||||
},
|
||||
registrations() {
|
||||
return [...registrations];
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function key(overrides: Record<string, unknown> = {}) {
|
||||
return {
|
||||
type: 'keydown',
|
||||
key: 'x',
|
||||
keyCode: 229,
|
||||
isComposing: false,
|
||||
ctrlKey: false,
|
||||
altKey: false,
|
||||
metaKey: false,
|
||||
getModifierState: () => false,
|
||||
...overrides,
|
||||
};
|
||||
/** A committed-text `input` event of the shape Chrome-on-Android delivers. */
|
||||
function inputEvent(data: string, overrides: Record<string, unknown> = {}) {
|
||||
return { data, inputType: 'insertText', isComposing: false, ...overrides };
|
||||
}
|
||||
|
||||
function harness({ emitThrows = false } = {}) {
|
||||
function harness({ screenReader = false } = {}) {
|
||||
const source = readFileSync(new URL('../src/web/public/terminal-keycode229-recovery.js', import.meta.url), 'utf8');
|
||||
const exposed: Record<string, any> = {};
|
||||
vm.runInNewContext(source, { window: exposed, globalThis: exposed }, { filename: 'terminal-keycode229-recovery.js' });
|
||||
|
||||
const textarea = makeTextarea();
|
||||
const emitted: string[] = [];
|
||||
const microtasks: Array<() => void> = [];
|
||||
const timers = new Map<number, () => void>();
|
||||
let timerId = 0;
|
||||
let now = 1_000;
|
||||
|
||||
const controller = exposed.CodemanKeyCode229Recovery.create({
|
||||
textarea,
|
||||
emitRecovered: (data: string) => {
|
||||
if (emitThrows) throw new Error('recovery callback failed');
|
||||
emitted.push(data);
|
||||
},
|
||||
queueMicrotask: (callback: () => void) => microtasks.push(callback),
|
||||
emitRecovered: (data: string) => emitted.push(data),
|
||||
isScreenReaderMode: () => screenReader,
|
||||
setTimer: (callback: () => void) => {
|
||||
const id = ++timerId;
|
||||
timers.set(id, callback);
|
||||
return id;
|
||||
},
|
||||
clearTimer: (id: number) => timers.delete(id),
|
||||
now: () => now,
|
||||
});
|
||||
|
||||
return {
|
||||
controller,
|
||||
emitted,
|
||||
textarea,
|
||||
advance(ms: number) {
|
||||
now += ms;
|
||||
/** A keydown that carries NO usable key identity, exactly like GBoard's. */
|
||||
keydown(overrides: Record<string, unknown> = {}) {
|
||||
controller.handleKeyEvent({ type: 'keydown', key: 'Unidentified', keyCode: 229, ...overrides });
|
||||
},
|
||||
flushMicrotasks() {
|
||||
while (microtasks.length) microtasks.shift()!();
|
||||
input(data: string, overrides: Record<string, unknown> = {}) {
|
||||
textarea.fire('input', inputEvent(data, overrides));
|
||||
},
|
||||
flushTimers() {
|
||||
for (const [id, callback] of [...timers]) {
|
||||
@@ -85,181 +89,190 @@ function harness({ emitThrows = false } = {}) {
|
||||
};
|
||||
}
|
||||
|
||||
describe('keyCode 229 terminal input recovery', () => {
|
||||
it('recovers an explicit printable key and Enter after xterm gets the first opportunity', () => {
|
||||
describe('orphaned terminal input recovery', () => {
|
||||
it('forwards the committed text when xterm stayed silent', () => {
|
||||
const h = harness();
|
||||
|
||||
h.controller.handleKeyEvent(key());
|
||||
expect(h.emitted).toEqual([]);
|
||||
h.flushMicrotasks();
|
||||
h.keydown();
|
||||
h.input('x');
|
||||
expect(h.emitted).toEqual([]);
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
|
||||
h.controller.handleKeyEvent(key({ key: 'Enter' }));
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x', '\r']);
|
||||
});
|
||||
|
||||
it('lets matching canonical terminal data win before fallback', () => {
|
||||
it('forwards nothing when xterm emitted canonical data after the keydown', () => {
|
||||
// The "xterm handled it" case is decided by the COUNTER, never by assuming
|
||||
// the input event does not reach us. On capture it always does (xterm's
|
||||
// cancel() only stops later BUBBLE listeners), so this test dispatches the
|
||||
// real input event AND has xterm emit canonical data for that keystroke.
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key());
|
||||
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
h.flushMicrotasks();
|
||||
h.keydown();
|
||||
h.input('x');
|
||||
h.controller.notifyCanonicalData();
|
||||
h.flushTimers();
|
||||
|
||||
expect(h.emitted).toEqual([]);
|
||||
});
|
||||
|
||||
it('cancels fallback when the helper textarea receives browser input or composition', () => {
|
||||
const input = harness();
|
||||
input.controller.handleKeyEvent(key());
|
||||
input.textarea.fire('input', { data: 'x' });
|
||||
input.flushMicrotasks();
|
||||
input.flushTimers();
|
||||
expect(input.emitted).toEqual([]);
|
||||
|
||||
const composition = harness();
|
||||
composition.controller.handleKeyEvent(key());
|
||||
composition.textarea.fire('compositionstart');
|
||||
composition.flushMicrotasks();
|
||||
composition.flushTimers();
|
||||
expect(composition.emitted).toEqual([]);
|
||||
});
|
||||
|
||||
it('suppresses one delayed matching canonical value from the recovered key token', () => {
|
||||
it('registers the input listener in the CAPTURE phase', () => {
|
||||
// Measured in jsdom and headless chromium: a capture-phase listener on the
|
||||
// TARGET calling stopPropagation() (which is what xterm's cancel() does in
|
||||
// the branch where it handled the input) stops later BUBBLE listeners on
|
||||
// that same target, because the target is visited twice in the event path.
|
||||
//
|
||||
// capture-then-BUBBLE, stopPropagation: ours NEVER fires
|
||||
// capture-then-CAPTURE, stopPropagation: ours still fires
|
||||
//
|
||||
// So this must not be "tidied" to bubble: on bubble we would silently stop
|
||||
// seeing exactly the events xterm handled, and whether we saw them at all
|
||||
// would depend on xterm's `options.cancelEvents`, which Codeman never sets.
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key());
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
|
||||
h.textarea.fire('beforeinput', { data: 'x' });
|
||||
h.flushMicrotasks();
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(true);
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
const input = h.textarea.registrations().filter((entry) => entry.type === 'input');
|
||||
expect(input).toHaveLength(1);
|
||||
expect(input[0].capture).toBe(true);
|
||||
for (const entry of h.textarea.registrations()) expect(entry.capture).toBe(true);
|
||||
});
|
||||
|
||||
it('does not suppress an unattributed same byte after recovery', () => {
|
||||
it('forwards nothing on the keypress path, where canonical data precedes the input event', () => {
|
||||
// xterm's _keyPress calls triggerDataEvent() and sets _keyPressHandled
|
||||
// BEFORE the input event fires. The "did xterm speak?" snapshot therefore
|
||||
// has to be taken at keydown; taken at input time it would already include
|
||||
// this emission and the character would be delivered twice.
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key());
|
||||
h.flushMicrotasks();
|
||||
h.keydown();
|
||||
h.controller.notifyCanonicalData();
|
||||
h.input('x');
|
||||
h.flushTimers();
|
||||
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
});
|
||||
|
||||
it('does not let an immediate ordinary same-character key cancel pending recovery', () => {
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key());
|
||||
h.controller.handleKeyEvent(key({ keyCode: 88 }));
|
||||
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
});
|
||||
|
||||
it('resolves overlapping eligible candidates independently of the latest keydown', () => {
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key({ key: 'x' }));
|
||||
h.controller.handleKeyEvent(key({ key: 'y' }));
|
||||
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['y']);
|
||||
});
|
||||
|
||||
it('suppresses a claimed recovery even after a newer ordinary keydown', () => {
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key({ key: 'x' }));
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
h.textarea.fire('beforeinput', { data: 'x' });
|
||||
|
||||
h.controller.handleKeyEvent(key({ key: 'y', keyCode: 89 }));
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(true);
|
||||
});
|
||||
|
||||
it('fails open when the recovery callback throws', () => {
|
||||
const h = harness({ emitThrows: true });
|
||||
h.controller.handleKeyEvent(key());
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
h.textarea.fire('beforeinput', { data: 'x' });
|
||||
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
expect(h.emitted).toEqual([]);
|
||||
});
|
||||
|
||||
it('keeps rapid repeated 229 keys and an ordinary same-character key distinct', () => {
|
||||
it('does not let a stale candidate swallow a later identical keystroke (defect 1)', () => {
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key());
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
|
||||
h.controller.handleKeyEvent(key());
|
||||
h.textarea.fire('input', { data: 'x' });
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
h.flushMicrotasks();
|
||||
// First keystroke: orphaned, recovered.
|
||||
h.keydown();
|
||||
h.input('x');
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
|
||||
h.controller.handleKeyEvent(key({ keyCode: 88 }));
|
||||
h.textarea.fire('input', { data: 'x' });
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
});
|
||||
|
||||
it('never recovers an ordinary keydown that xterm already handles', () => {
|
||||
const h = harness();
|
||||
h.controller.handleKeyEvent(key({ keyCode: 88 }));
|
||||
h.flushMicrotasks();
|
||||
// Second identical keystroke, handled by xterm itself.
|
||||
h.keydown();
|
||||
h.input('x');
|
||||
h.controller.notifyCanonicalData();
|
||||
h.flushTimers();
|
||||
|
||||
expect(h.emitted).toEqual([]);
|
||||
// Exactly one recovery total, and the second keystroke's canonical byte was
|
||||
// never claimed or suppressed by the first one.
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
});
|
||||
|
||||
it('does not recover real composition, unidentified keys, modifiers, or keyup', () => {
|
||||
it('forwards committed text that no keydown key could describe, exactly once (defect 2)', () => {
|
||||
const h = harness();
|
||||
for (const event of [
|
||||
key({ isComposing: true }),
|
||||
key({ key: 'Process' }),
|
||||
key({ key: 'Unidentified' }),
|
||||
key({ key: 'Dead' }),
|
||||
key({ ctrlKey: true }),
|
||||
key({ altKey: true }),
|
||||
key({ metaKey: true }),
|
||||
key({ getModifierState: (name: string) => name === 'AltGraph' }),
|
||||
key({ type: 'keyup' }),
|
||||
key({ key: 'ArrowLeft' }),
|
||||
]) {
|
||||
h.controller.handleKeyEvent(event);
|
||||
h.keydown({ key: 'Enter' });
|
||||
h.input('a longer commit');
|
||||
h.flushTimers();
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['a longer commit']);
|
||||
});
|
||||
|
||||
it('recovers a GBoard keydown and never reads key or keyCode (defect 3)', () => {
|
||||
const h = harness();
|
||||
const reads: string[] = [];
|
||||
h.controller.handleKeyEvent({
|
||||
type: 'keydown',
|
||||
get key() {
|
||||
reads.push('key');
|
||||
return 'Unidentified';
|
||||
},
|
||||
get keyCode() {
|
||||
reads.push('keyCode');
|
||||
return 229;
|
||||
},
|
||||
get which() {
|
||||
reads.push('which');
|
||||
return 229;
|
||||
},
|
||||
});
|
||||
h.input('x');
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
expect(reads).toEqual([]);
|
||||
});
|
||||
|
||||
it('ignores input events that are not committed text', () => {
|
||||
const h = harness();
|
||||
for (const inputType of ['insertCompositionText', 'deleteContentBackward', 'insertLineBreak', 'insertFromPaste']) {
|
||||
h.keydown();
|
||||
h.input('x', { inputType });
|
||||
}
|
||||
h.flushMicrotasks();
|
||||
h.keydown();
|
||||
h.input('');
|
||||
h.keydown();
|
||||
h.textarea.fire('input', { data: null, inputType: 'insertText' });
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual([]);
|
||||
});
|
||||
|
||||
it('expires deduplication and destroys listeners and scheduled work', () => {
|
||||
it('ignores composition and cancels pending candidates on compositionstart', () => {
|
||||
const composing = harness();
|
||||
composing.keydown();
|
||||
composing.input('x', { isComposing: true });
|
||||
composing.flushTimers();
|
||||
expect(composing.emitted).toEqual([]);
|
||||
|
||||
const lifecycle = harness();
|
||||
lifecycle.textarea.fire('compositionstart');
|
||||
lifecycle.keydown();
|
||||
lifecycle.input('中');
|
||||
lifecycle.flushTimers();
|
||||
expect(lifecycle.emitted).toEqual([]);
|
||||
|
||||
// compositionstart arriving after a candidate is queued must cancel it.
|
||||
const cancelled = harness();
|
||||
cancelled.keydown();
|
||||
cancelled.input('x');
|
||||
cancelled.textarea.fire('compositionstart');
|
||||
cancelled.flushTimers();
|
||||
expect(cancelled.emitted).toEqual([]);
|
||||
|
||||
// compositionend releases the gate again.
|
||||
cancelled.textarea.fire('compositionend');
|
||||
cancelled.keydown();
|
||||
cancelled.input('y');
|
||||
cancelled.flushTimers();
|
||||
expect(cancelled.emitted).toEqual(['y']);
|
||||
});
|
||||
|
||||
it('stays out of the way in screen reader mode', () => {
|
||||
const h = harness({ screenReader: true });
|
||||
h.keydown();
|
||||
h.input('x');
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual([]);
|
||||
});
|
||||
|
||||
it('recovers an input event that arrives with no preceding keydown', () => {
|
||||
const h = harness();
|
||||
h.input('x');
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
});
|
||||
|
||||
it('clears timers and listeners on destroy', () => {
|
||||
const h = harness();
|
||||
expect(h.textarea.listenerCount()).toBeGreaterThan(0);
|
||||
h.controller.handleKeyEvent(key());
|
||||
h.flushMicrotasks();
|
||||
h.flushTimers();
|
||||
h.advance(500);
|
||||
h.textarea.fire('input', { data: 'x' });
|
||||
expect(h.controller.consumeTerminalData('x')).toBe(false);
|
||||
|
||||
h.controller.handleKeyEvent(key({ key: 'y' }));
|
||||
h.flushMicrotasks();
|
||||
h.keydown();
|
||||
h.input('x');
|
||||
expect(h.pendingTimers()).toBe(1);
|
||||
|
||||
h.controller.destroy();
|
||||
expect(h.pendingTimers()).toBe(0);
|
||||
expect(h.textarea.listenerCount()).toBe(0);
|
||||
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual(['x']);
|
||||
expect(h.emitted).toEqual([]);
|
||||
|
||||
// Nothing fires after destroy, even if a stray event is delivered.
|
||||
h.textarea.fire('input', inputEvent('y'));
|
||||
h.flushTimers();
|
||||
expect(h.emitted).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user