Skip to content

fix(input): route IME composition events to the hidden textarea#11

Merged
diegosouzapw merged 1 commit into
mainfrom
fix/port-pr-120b-ime-composition-routing
May 23, 2026
Merged

fix(input): route IME composition events to the hidden textarea#11
diegosouzapw merged 1 commit into
mainfrom
fix/port-pr-120b-ime-composition-routing

Conversation

@diegosouzapw
Copy link
Copy Markdown
Owner

Summary

Ports the IME-routing subset of upstream PR #120 with two deliberate deviations.

Root cause being fixed: composition events fire on the focused element. ghostty-web focuses a hidden textarea for keyboard input, but composition listeners were attached to the container element — every CJK input event was missed.

What changed

  • Composition listeners now attach to the textarea (with symmetric detach on dispose)
  • New state machine for the "terminating key" of an IME composition (space, period). The key is queued during composition and replayed after compositionend so composed text appears before the terminator
  • contenteditable="true" removed from the parent container (was causing IME text to be inserted into the container as text nodes, bypassing the textarea)
  • tabindex="-1" on parent; parent mousedown/focus now redirect to textarea
  • Terminal.focus() targets textarea instead of container

Deviations from upstream coder#120

  • 🚫 Composition-preview overlay NOT ported. Upstream added a div with hardcoded Korean text "조합중:" and inline #ffcc00 styling. Native browsers already render IME feedback, and the overlay was untranslated + theme-hostile.
  • 🚫 Selection wide-char fix NOT here. Already shipped separately as PR fix(selection): skip wide-character continuation cells when copying #9 (#120a).

Regression risk (extensions)

Removing contenteditable was originally added by #78 to stop Vimium-style extensions from intercepting keys. The textarea is itself a real input element so most extensions should leave it alone — but this needs verification in a real browser with extensions installed. If the regression appears, the fallback is to keep contenteditable only while no composition is active.

Attribution

Thanks to @hongsw for the original implementation.

Test plan

  • bun run fmt && bun run lint && bun run typecheck
  • bun test — 331 tests pass (no new tests; existing keyboard suite unchanged), 0 fail
  • bun run build:lib
  • bun run build:wasm — WASM rebuilt locally with Zig 0.15.2
  • Manual smoke test pending in bun run dev:
    • Type Korean / Chinese / Japanese via OS IME, confirm characters appear
    • Confirm the terminating key (space, period) appears AFTER composed text
    • With Vimium installed, confirm key bindings do not hijack the terminal
    • PR fix(selection): skip wide-character continuation cells when copying #9 already covers the copy/paste no-stray-spaces case for wide chars

Risk

Medium. Removes contenteditable (may resurrect coder#78's extension regression — needs browser verification). Big-picture: IME is broken today for CJK users; this is a real functional win at the cost of one regression risk that we can quickly verify.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves IME support for CJK languages by redirecting focus to a hidden textarea and refining composition event handling. Key changes include moving event listeners to the textarea, removing contenteditable from the container to prevent direct text insertion, and implementing a queuing mechanism for keys that terminate composition. Feedback identifies that the compositionJustEnded flag is not correctly updated, special keys are not mapped to terminal escape sequences, and de-duplication logic is missing in the new processPendingKeyAfterComposition method.

Comment thread lib/input-handler.ts
Comment on lines +733 to 740
private processPendingKeyAfterComposition(): void {
if (this.pendingKeyAfterComposition) {
const key = this.pendingKeyAfterComposition;
this.pendingKeyAfterComposition = null;
// Output the key that ended composition
this.onDataCallback(key);
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

There are three issues in this method:

  1. The compositionJustEnded flag is never set to true, which makes the logic in handleKeyDown (line 390) that blocks duplicate events ineffective.
  2. Storing and sending event.key directly is incorrect for special keys like Enter, Tab, or Backspace. In a terminal context, these must be mapped to their respective escape sequences (e.g., \r, \t, \x7F).
  3. This method should call recordKeyDownData to ensure proper de-duplication against subsequent beforeinput events, maintaining consistency with the rest of the input handling logic.
  private processPendingKeyAfterComposition(): void {
    if (this.pendingKeyAfterComposition) {
      const key = this.pendingKeyAfterComposition;
      this.pendingKeyAfterComposition = null;
      this.compositionJustEnded = true;

      // Map common terminating keys to their terminal sequences.
      // For printable characters (like Space or Period), the key is the data.
      let data = key;
      if (key === 'Enter') data = '\r';
      else if (key === 'Tab') data = '\t';
      else if (key === 'Backspace') data = '\x7F';
      else if (key === 'Escape') data = '\x1B';

      // Output the key that ended composition
      this.onDataCallback(data);
      this.recordKeyDownData(data);
    }
  }

IME composition events (compositionstart / compositionupdate /
compositionend) fire on the focused element. ghostty-web focuses a
hidden textarea for keyboard input, but composition listeners were
attached to the container element — so every Korean / Chinese / Japanese
input event was missed.

This commit:

- Moves composition listeners from `container` to `inputElement`
  (textarea) when the input element is available. Detach is also
  retargeted to the same element so disposal is symmetric.
- Adds a state machine to handle the "terminating key" of an IME
  composition (space, period, etc.). The key is queued during
  composition and replayed after compositionend so the composed text
  appears before the terminator.
- Removes `contenteditable="true"` from the parent container. Having
  contenteditable on the container caused IME text to be inserted as
  text nodes in the container, bypassing the textarea entirely. The
  textarea is itself a real input element, so most browser extensions
  (Vimium, etc.) leave it alone — this should not regress the
  motivation behind coder#78, but needs verification in real browsers.
- Sets `tabindex="-1"` on the parent so it is no longer click/tab
  focusable. Redirects parent mousedown and focus events to the
  textarea so any focus eventually lands on the input element.
- Updates `Terminal.focus()` to target the textarea instead of the
  container, with the same delayed-focus backup behaviour.

Differences from upstream PR coder#120 (deliberate):

- The composition-preview overlay (a div with hardcoded Korean text
  "조합중:" and `#ffcc00` on dark background) is intentionally NOT
  ported. Native browsers already render IME composition feedback,
  and the upstream overlay was both untranslated and theme-hostile.
- The selection-manager wide-char fix from that PR was already
  shipped separately as #120a.

Co-authored-by: Seungwoo Hong <ai.baryon.ai@gmail.com>
Inspired-by: coder#120
@diegosouzapw diegosouzapw force-pushed the fix/port-pr-120b-ime-composition-routing branch from fe081ad to 5162a1b Compare May 23, 2026 16:57
@sonarqubecloud
Copy link
Copy Markdown

@diegosouzapw diegosouzapw merged commit ebcc6eb into main May 23, 2026
2 checks passed
@diegosouzapw diegosouzapw deleted the fix/port-pr-120b-ime-composition-routing branch May 23, 2026 17:00
diegosouzapw added a commit that referenced this pull request May 23, 2026
…routing

PR #11 routes focus to the hidden textarea inside the container rather than
to the container element itself. Accept either as valid.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant