fix(ime): preserve Korean IME last syllable across commits (#8919)#9713
fix(ime): preserve Korean IME last syllable across commits (#8919)#9713ohah wants to merge 2 commits intowarpdotdev:masterfrom
Conversation
Korean (and other CJK) IMEs on macOS can both commit a previous syllable AND start a new composition within a single keyDown — e.g. typing 'ㅏ' after '간' produces commit '가' plus new marked '나', and pressing Enter/Arrow on '안녕하세요' commits the trailing '요'. Two bugs in host_view.m caused the last syllable to be lost in these flows: 1. The `!handled` guard around the textToInsert dispatch dropped the committed text whenever the key was handled by the input field (Arrow/Enter etc.), so the last Korean syllable was discarded. Fixed by also dispatching the commit when `wasComposing` was true on entry — the text represents the user's already-typed input, not text produced by the keybinding. 2. By the time we dispatch the commit, setMarkedText: may have already placed the next composition as a selection in the input field's buffer. Inserting the committed text on top of that selection would overwrite it (losing the next character). Worked around by temporarily clearing the marked text, dispatching the commit, and re-applying the marked text afterwards. Fixes warpdotdev#8919
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @ohah on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adjusts macOS IME handling so committed text from an active composition can still be dispatched after handled key events, and attempts to preserve newly-started marked text across that commit.
Concerns
- The new restoration path treats any remaining
markedTextas newly-started marked text, butinsertText:deliberately leaves the previous marked text in place whileinterpretingKeyEventsis true. Commit-only flows such as Enter/dead-key completion can therefore re-emit stale marked text instead of ending composition.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // text would then overwrite that selection (losing the next | ||
| // character). Workaround: temporarily clear the marked text, | ||
| // dispatch the commit, then re-apply the marked text. | ||
| BOOL hasNewMarked = [self hasMarkedText]; |
There was a problem hiding this comment.
hasNewMarked is also true for the already-committed composition because insertText: intentionally skips unmarkText while interpretingKeyEvents is true, so commit-only flows will clear and then re-emit stale marked text instead of ending IME state; distinguish newly-started marked text from stale markedText before taking this branch.
…tore unmarkText after commit-only flows Address @oz-for-oss feedback on warpdotdev#9713: the previous version of this fix treated any non-empty markedText after interpretKeyEvents: as a freshly-started composition, but insertText: deliberately skips unmarkText while interpretingKeyEvents is true. So a commit-only flow (e.g. dead-key + char) leaves the previous markedText in place, and the fix would re-emit it as if it were new — and, worse, by skipping unmarkText on that branch the host_view's IME state stayed "active", breaking the next keystroke (e.g. Backspace did nothing because the input field was still in composing state). Two changes: 1. Track whether setMarkedText: was actually called during the current interpretKeyEvents: via a new BOOL `setMarkedTextDuringInterpret`, and only take the clear-commit-restore path when it is true. This distinguishes "IME just started a new composition mid-keyDown" from "IME committed but left stale markedText behind". 2. On the commit-only branch (hasNewMarked == false), explicitly call `unmarkText` so the stale markedText object and the IME-active flag are cleared — matching the original code's invariant that committing ends the composition unless a new one was started.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @vorporeal. Comment Powered by Oz |
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR updates the macOS host view IME flow so committed Korean/CJK composition text is still dispatched when the terminating key is handled, and preserves newly marked text that starts during the same key event.
Concerns
- No blocking correctness concerns found in the changed lines.
- Security pass: no security-sensitive data flow, permission, injection, or trust-boundary changes were introduced by this patch.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
1 participant
Description
Fixes a long-standing Korean (and broader CJK) IME bug on macOS where the last composing character could be silently dropped — both when terminating composition with Enter/Arrow keys and during ongoing composition that bridges syllables.
Linked Issue
Fixes #8919.
triaged(implicitlyready-to-implementper CONTRIBUTING.md).Root cause
Two bugs in
crates/warpui/src/platform/mac/objc/host_view.m'skeyDownImpl:interacted with the way Korean (and other CJK) IMEs commit pre-edit text:!handledguard discarded committed text on Arrow/Enter.When the user pressed Enter/Arrow to terminate composition, the macOS Korean IME committed the trailing syllable via
insertText:, but the existing guardif ([textToInsert length] > 0 && !handled)then discarded it because the input field had handled the key. So the very last syllable (e.g. the '요' in '안녕하세요') was lost.Korean IMEs can both commit a previous syllable AND start a new composition within the same keyDown — typing 'ㅏ' after '간' produces commit '가' + new marked '나'. By the time we dispatched the commit,
setMarkedText:had already placed the new marked text as a selection in the input field's buffer, andinsert_internal(called byuser_insert) overwrote that selection with the committed text — losing the new syllable.Fix
In
keyDownImpl::(!handled || wasComposing)so a key that was both consumed by the input field AND committed previously-composed text still dispatches the committed text (it represents the user's already-typed input, not text produced by the keybinding).setMarkedText:time into a new instance variablelastMarkedSelectedRangeso we can re-emit the same range afterwards.Testing
Manual repro on macOS with
cargo run --features ime_marked_text(release/dogfood builds enable this flag automatically — seecrates/warp_features/src/lib.rs):안녕하세요and press Enter → before:안녕하세is sent,요lost. After: full안녕하세요is sent.안녕하세요and press → / ↓ → before: trailing요dropped. After: preserved.가나다slowly and watch composition during typing → before: the last syllable being composed (e.g.다) wasn't shown until the next syllable bumped it. After: each composing syllable is visible as marked text in real time.No automated tests added —
host_view.mis Objective-C and not directly unit-testable; themarked_text_tests.rssuite exercises the editor model layer below this fix and continues to pass. Happy to add an integration test if reviewers want one.Agent Mode
now.mov
after.mov