fix: avoid refocusing find input during IME composition#320898
Conversation
Track find input events fired from compositionend so IME-completed input can update search state without immediately restoring focus. Also avoid focus restoration while an IME session is still active.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves IME (Input Method Editor) handling in the find widget by adding composition-aware input events and avoiding focus changes during IME sessions.
Changes:
- Add a typed payload to
FindInput.onInputto indicate whether the change came fromcompositionend. - Update
SimpleFindWidgetto conditionally refocus the find box based on IME/composition state. - Prevent
focusFindBox()from running while an IME session is in progress.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/codeEditor/browser/find/simpleFindWidget.ts | Avoids refocusing the find box during/around IME composition to prevent disruptive focus churn. |
| src/vs/base/browser/ui/findinput/findInput.ts | Enriches input events with composition provenance so callers can treat IME completion differently. |
| this._register(dom.addDisposableListener(this.inputBox.inputElement, 'compositionend', (e: CompositionEvent) => { | ||
| this.imeSessionInProgress = false; | ||
| this._onInput.fire(); | ||
| this._onInput.fire({ fromCompositionEnd: true }); | ||
| })); | ||
|
|
||
| this.onkeydown(this.inputBox.inputElement, (e) => this._onKeyDown.fire(e)); | ||
| this.onkeyup(this.inputBox.inputElement, (e) => this._onKeyUp.fire(e)); | ||
| this.oninput(this.inputBox.inputElement, (e) => this._onInput.fire()); | ||
| this.oninput(this.inputBox.inputElement, (e) => this._onInput.fire({ fromCompositionEnd: false })); |
There was a problem hiding this comment.
The double-fire isn't introduced by this PR — the previous code already fired onInput from both compositionend and the native input event, just untagged. This PR doesn't change how many events consumers receive; it only adds provenance to each one.
Firing only from input (and not from compositionend) would break Chromium: there, the committed text's input event arrives before compositionend, with isComposing: true and the IME session flag still set, so it is deliberately ignored. With the compositionend fire removed, the search would never update at commit time during continuous IME typing. Firing from compositionend has been the intended design since #125856
The dangerous window (an active composition) is already covered twice: an input event arriving after the next composition has started is skipped by the handler's isImeSessionInProgress check, and focusFindBox() has its own guard. What remains is a single focus bounce at a moment when no composition is active — the same thing that already happens on every Latin keystroke, and strictly less than before this PR (previously both events triggered the bounce).
| private readonly _onInput = this._register(new Emitter<IFindInputEvent>()); | ||
| public get onInput(): Event<IFindInputEvent> { return this._onInput.event; } |
There was a problem hiding this comment.
The only in-tree subscriber is SimpleFindWidget, updated in this PR. Parameterless listeners remain assignable to Event, so no call-site breaks either way; a parallel emitter would duplicate the same signal across two events.
| if (!options.checkImeCompletionState || !e.fromCompositionEnd) { | ||
| this.focusFindBox(); | ||
| } |
There was a problem hiding this comment.
The two checks cover different timing windows rather than duplicating logic: the event-provenance check handles the synchronous compositionend-driven update (at that instant the session flag is already false, so live state can't detect it), while the guard in focusFindBox() handles asynchronous callers like find-result events (where provenance is unknowable and live state is the only correct signal). Centralizing both in focusFindBox() would require a "just completed composition" side-channel on FindInput that's only valid during synchronous dispatch — a fragile contract this PR deliberately avoids.
|
@microsoft-github-policy-service agree |
Fixes #320869
What changed?
Updated the shared find input flow so input events fired from compositionend can update search state without immediately restoring focus to the find box (FindInput.onInput now carries { fromCompositionEnd }). Also added a guard so focusFindBox() does not refocus the find input while an IME composition session is still active.
Why?
focusFindBox() blurs and refocuses the find input (bouncing through the next-match button). When this runs for the input fired at compositionend — or when an async find result arrives while the next character is already composing — the focus change aborts the active IME composition. Korean IME fires compositionend at every syllable boundary, so characters were dropped or corrupted while typing. The search value is still processed as before; only the focus churn around composition is skipped.
How I tested this:
Reproduced the original bug and verified the fix manually with ./scripts/code-web.sh: typing Korean (e.g. 안녕하세요) in the markdown preview find box corrupted characters before this change and is clean after. (Tested on web because webview find is not functional in local desktop builds — stock Electron lacks the findInFrame patch, see #222244.)
Verified English typing, Enter navigation, and find history behavior are unchanged.
npm run compile-check-ts-native and ESLint on the changed files pass.