Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 132 additions & 0 deletions lib/input-handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ interface MockClipboardEvent {
interface MockHTMLElement {
addEventListener: (event: string, handler: (e: any) => void) => void;
removeEventListener: (event: string, handler: (e: any) => void) => void;
childNodes: Node[];
removeChild: (node: Node) => Node;
appendChild: (node: Node) => Node;
}

// Helper to create mock keyboard event
Expand Down Expand Up @@ -76,7 +79,25 @@ function createClipboardEvent(text: string | null): MockClipboardEvent {
stopPropagation: mock(() => {}),
};
}
interface MockCompositionEvent {
type: string;
data: string | null;
preventDefault: () => void;
stopPropagation: () => void;
}

// Helper to create mock composition event
function createCompositionEvent(
type: 'compositionstart' | 'compositionupdate' | 'compositionend',
data: string | null
): MockCompositionEvent {
return {
type,
data,
preventDefault: mock(() => {}),
stopPropagation: mock(() => {}),
};
}
// Helper to create mock container
function createMockContainer(): MockHTMLElement & {
_listeners: Map<string, ((e: any) => void)[]>;
Expand Down Expand Up @@ -107,6 +128,19 @@ function createMockContainer(): MockHTMLElement & {
handler(event);
}
},
// Mock childNodes and removeChild for text node cleanup test
childNodes: [] as Node[],
removeChild(node: Node) {
const index = this.childNodes.indexOf(node);
if (index >= 0) {
this.childNodes.splice(index, 1);
}
return node;
},
appendChild(node: Node) {
this.childNodes.push(node);
return node;
},
};
}

Expand Down Expand Up @@ -269,6 +303,104 @@ describe('InputHandler', () => {
});
});

describe('IME Composition', () => {
test('handles composition sequence', () => {
const handler = new InputHandler(
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable handler.

Suggested change
const handler = new InputHandler(
new InputHandler(

Copilot uses AI. Check for mistakes.
ghostty,
container as any,
(data) => dataReceived.push(data),
() => {
bellCalled = true;
}
);

// Start composition
const startEvent = createCompositionEvent('compositionstart', '');
container.dispatchEvent(startEvent);

// Update composition (typing)
const updateEvent1 = createCompositionEvent('compositionupdate', 'n');
container.dispatchEvent(updateEvent1);

// Keydown events during composition should be ignored
const keyEvent1 = createKeyEvent('KeyN', 'n');
Object.defineProperty(keyEvent1, 'isComposing', { value: true });
simulateKey(container, keyEvent1);

// Update composition (more typing)
const updateEvent2 = createCompositionEvent('compositionupdate', 'ni');
container.dispatchEvent(updateEvent2);

// End composition (commit)
const endEvent = createCompositionEvent('compositionend', '你好');
container.dispatchEvent(endEvent);

// Should only receive the final committed text
expect(dataReceived).toEqual(['你好']);
});

test('ignores keydown during composition', () => {
const handler = new InputHandler(
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable handler.

Copilot uses AI. Check for mistakes.
ghostty,
container as any,
(data) => dataReceived.push(data),
() => {
bellCalled = true;
}
);

// Start composition
container.dispatchEvent(createCompositionEvent('compositionstart', ''));

// Simulate keydown with isComposing=true
const keyEvent = createKeyEvent('KeyA', 'a');
Object.defineProperty(keyEvent, 'isComposing', { value: true });
simulateKey(container, keyEvent);

// Simulate keydown with keyCode 229
const keyEvent229 = createKeyEvent('KeyB', 'b');
Object.defineProperty(keyEvent229, 'keyCode', { value: 229 });
simulateKey(container, keyEvent229);

// Should not receive any data
expect(dataReceived.length).toBe(0);

// End composition
container.dispatchEvent(createCompositionEvent('compositionend', 'a'));
expect(dataReceived).toEqual(['a']);
});

test('cleans up text nodes in container after composition', () => {
const handler = new InputHandler(
ghostty,
container as any,
(data) => dataReceived.push(data),
() => {
bellCalled = true;
}
);

// Simulate browser inserting text node during composition
const textNode = { nodeType: 3, textContent: '你好' } as Node;
container.appendChild(textNode);

// Also add a non-text node (e.g. canvas) to ensure it's not removed
const elementNode = { nodeType: 1, nodeName: 'CANVAS' } as Node;
container.appendChild(elementNode);

expect(container.childNodes.length).toBe(2);

// End composition
const endEvent = createCompositionEvent('compositionend', '你好');
container.dispatchEvent(endEvent);

// Should have removed the text node but kept the element node
expect(container.childNodes.length).toBe(1);
expect(container.childNodes[0]).toBe(elementNode);
expect(dataReceived).toEqual(['你好']);
});
});
Comment on lines +306 to +402
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing dispose removes listeners and marks inactive test should be updated to verify that the new composition event listeners (compositionstart, compositionupdate, compositionend) are also properly removed during disposal. This would ensure complete cleanup of all event listeners.

Copilot uses AI. Check for mistakes.
Comment on lines +306 to +402
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edge case not tested: What happens if composition is interrupted (e.g., user clicks away or presses Escape)? The isComposing flag would remain true if compositionend is never fired. Consider adding a test for this scenario or adding a mechanism to reset the composition state when the container loses focus or when dispose is called.

Copilot uses AI. Check for mistakes.
Comment on lines +306 to +402
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage: The test suite doesn't cover the case where compositionend fires with empty or null data (e.g., when composition is cancelled). The implementation checks if (data && data.length > 0) before calling the callback, but this path isn't tested. Add a test to verify that cancelling composition doesn't send any data to the terminal.

Copilot uses AI. Check for mistakes.

describe('Control Characters', () => {
test('encodes Ctrl+A', () => {
const handler = new InputHandler(
Expand Down
78 changes: 78 additions & 0 deletions lib/input-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@ export class InputHandler {
private keydownListener: ((e: KeyboardEvent) => void) | null = null;
private keypressListener: ((e: KeyboardEvent) => void) | null = null;
private pasteListener: ((e: ClipboardEvent) => void) | null = null;
private compositionStartListener: ((e: CompositionEvent) => void) | null = null;
private compositionUpdateListener: ((e: CompositionEvent) => void) | null = null;
private compositionEndListener: ((e: CompositionEvent) => void) | null = null;
private isComposing = false;
private isDisposed = false;

/**
Expand Down Expand Up @@ -233,6 +237,15 @@ export class InputHandler {

this.pasteListener = this.handlePaste.bind(this);
this.container.addEventListener('paste', this.pasteListener);

this.compositionStartListener = this.handleCompositionStart.bind(this);
this.container.addEventListener('compositionstart', this.compositionStartListener);

this.compositionUpdateListener = this.handleCompositionUpdate.bind(this);
this.container.addEventListener('compositionupdate', this.compositionUpdateListener);

this.compositionEndListener = this.handleCompositionEnd.bind(this);
this.container.addEventListener('compositionend', this.compositionEndListener);
}

/**
Expand Down Expand Up @@ -287,6 +300,12 @@ export class InputHandler {
private handleKeyDown(event: KeyboardEvent): void {
if (this.isDisposed) return;

// Ignore keydown events during composition
// Note: Some browsers send keyCode 229 for all keys during composition
Comment on lines +303 to +304
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The condition checks three separate ways to detect composition: this.isComposing (internal state), event.isComposing (standard property), and event.keyCode === 229 (legacy approach). While this provides good cross-browser compatibility, the comment should clarify that keyCode 229 is a legacy fallback for older browsers, as keyCode is deprecated. Consider if all three checks are still necessary for the target browsers.

Suggested change
// Ignore keydown events during composition
// Note: Some browsers send keyCode 229 for all keys during composition
// Ignore keydown events during composition.
// We check three ways for cross-browser compatibility:
// - this.isComposing: internal state set by composition events
// - event.isComposing: standard property supported by modern browsers
// - event.keyCode === 229: legacy fallback for older browsers (e.g., IE/Edge), as keyCode is deprecated

Copilot uses AI. Check for mistakes.
if (this.isComposing || event.isComposing || event.keyCode === 229) {
return;
}

// Emit onKey event first (before any processing)
if (this.onKeyCallback) {
this.onKeyCallback({ key: event.key, domEvent: event });
Expand Down Expand Up @@ -493,6 +512,50 @@ export class InputHandler {
this.onDataCallback(text);
}

/**
* Handle compositionstart event
*/
private handleCompositionStart(_event: CompositionEvent): void {
if (this.isDisposed) return;
this.isComposing = true;
}

/**
* Handle compositionupdate event
*/
private handleCompositionUpdate(_event: CompositionEvent): void {
if (this.isDisposed) return;
// We could track the current composition string here if we wanted to
// display it in a custom way, but for now we rely on the browser's
// input method editor UI.
}

/**
* Handle compositionend event
*/
private handleCompositionEnd(event: CompositionEvent): void {
if (this.isDisposed) return;
this.isComposing = false;

const data = event.data;
if (data && data.length > 0) {
this.onDataCallback(data);
}

// Cleanup text nodes in container (fix for duplicate text display)
// When the container is contenteditable, the browser might insert text nodes
// upon composition end. We need to remove them to prevent duplicate display.
if (this.container && this.container.childNodes) {
for (let i = this.container.childNodes.length - 1; i >= 0; i--) {
const node = this.container.childNodes[i];
// Node.TEXT_NODE === 3
if (node.nodeType === 3) {
this.container.removeChild(node);
}
}
}
}

/**
* Dispose the InputHandler and remove event listeners
*/
Expand All @@ -514,6 +577,21 @@ export class InputHandler {
this.pasteListener = null;
}

if (this.compositionStartListener) {
this.container.removeEventListener('compositionstart', this.compositionStartListener);
this.compositionStartListener = null;
}

if (this.compositionUpdateListener) {
this.container.removeEventListener('compositionupdate', this.compositionUpdateListener);
this.compositionUpdateListener = null;
}

if (this.compositionEndListener) {
this.container.removeEventListener('compositionend', this.compositionEndListener);
this.compositionEndListener = null;
}

this.isDisposed = true;
}

Expand Down