Fix IME composition being cancelled on the first character in an empty leaf (Android) - #6096
Conversation
…y leaf An empty leaf renders as a zero-width string (a <br> on Android) rather than a text node. When an IME composes the first character into one, the browser creates the text node itself, so every code path that replaces the leaf's DOM mid-composition cancels the composition and leaves the first character behind uncomposed (typing 안녕 gives ㅇ안녕). Four paths did that: the flush scheduled after every insertCompositionText (ianstormtaylor#5901), the FLUSH_DELAY timer, RestoreDOM undoing the browser-created text node, and TextString rewriting textContent. Fixes ianstormtaylor#5883 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: df2fc34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dacd796d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,4 +1,5 @@ | |||
| import { DebouncedFunc } from 'lodash' | |||
| import { flushSync } from 'react-dom' | |||
There was a problem hiding this comment.
Register flushSync for the UMD CommonJS transform
The production UMD build bundles react-dom, whose CommonJS entry point does not expose statically discoverable named exports; accordingly, config/rollup/rollup.config.js manually registers the exports used by Slate, but currently lists only findDOMNode. Adding this named import without registering flushSync causes the production build:rollup step used by prerelease to report that flushSync is not exported by react-dom, blocking release builds. Add flushSync to that namedExports['react-dom'] entry or access it through the already-supported default import.
Useful? React with 👍 / 👎.
The UMD build resolves react-dom through its CommonJS entry, whose named exports rollup cannot discover statically, so a named import breaks `NODE_ENV=production yarn build:rollup`. Matches how with-react.ts already reaches unstable_batchedUpdates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note for anyone reading later: the Codex review thread on this PR is now collapsed as outdated, because the fix for it rewrote the exact line the comment was anchored to. Summarising it here so it is not lost. Codex flagged (P1): It was right. Fixed in cad63b6 by using the default import ( After the fix: |
|
Confirmed on a physical Android device, in a real chat app rather than a test page. A user hitting this bug ran an instrumented build on their own phone (Korean 2-bulsik keyboard) and typed 안녕 into an empty composer. Trace, times in ms: Result: The important part is what is absent. Between the first character at 54891 and Without this change the same sequence swaps |
The deferral had one exit: compositionend. Slate already notes that event is unreliable, so a stuck composition left the typed text outside the value and re-armed a timer every 200ms. A composition that has lost focus or gone quiet for 5s no longer holds flushes back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Two follow-ups since the device confirmation. Closed a hole in the deferral. The deferral had exactly one exit — A composition now stops holding flushes back once it has lost focus, or once it has been idle for 5 s. The idle window is deliberately generous so that pausing mid-word in a long Japanese or pinyin composition is not cut short. Coverage beyond Korean. The PR claims this fixes #5883, which was reported in Japanese, so verifying only Korean was not enough. On the same Pixel/Android 14/Gboard emulator, into an empty leaf:
The Japanese case is the decisive one: dakuten modifies the character already being composed, so a composition broken at the first character would leave
|
Replacing scheduleAction with a pending selection made the caret lag: the value moved on before the selection did, so after committing a syllable and pressing space the caret sat a character behind for ~200ms before snapping forward. The flush gate already stops that scheduled flush from landing mid-composition, so ianstormtaylor#5901's handling can stay exactly as it was. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Dropped one hunk: replacing #5901's A tester on a physical device reported that after committing a syllable and pressing space, the caret sat a character behind for a moment before jumping forward. Sampling
That is #5901's own bug coming back, which makes sense: it fixed the caret by scheduling the selection as an action, and I had turned it into a pending selection that only lands on the next flush. The swap was also redundant. Its purpose was to stop So |
…rals Deferring flushes kept the first character alive, but cancellation paths were left inconsistent: backspacing the only composing character stranded it in the DOM and could close the Android keyboard, because the deferred text was in neither the value nor anywhere slate could delete it from. Root cause of that whole class: on Android an empty leaf rendered no text node at all, so the IME composed into a node it had to create and that no re-render could preserve. Three changes make the state coherent: - ZeroWidthString renders the same zero-width space other platforms get, so composition lives in a React-owned text node and the browser only ever edits characterData. The wrapper keeps its data-slate-zero-width attributes, so selection mapping is unchanged. - An empty insertCompositionText (the IME discarding its composition) drops the deferred diffs instead of applying them: the value is already in the desired state, and applying-then-re-deleting churned the DOM mid-composition. - After a cancelled composition ends, one forced render lets RestoreDOM put the leaf DOM back for the next composition, and the placeholder is re-shown only once the composition is over. The RestoreDOM childList guard is no longer needed - it existed for the browser-created text node - and is reverted. Verified on a Gboard emulator (Android 14): first character composes, backspacing the last composing character deletes it with the keyboard staying open (3/3), and typing again right after a cancel composes cleanly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Found and fixed a regression in this PR during device testing, and the fix simplified the whole approach (df2fc34). The regression: backspacing away the only composing character (type The underlying problem was one level deeper than this PR originally went. On Android an empty leaf renders no text node at all ( Three changes replace it:
With composition in a stable node, the Re-verified on the Gboard emulator (Android 14, Gboard 12.4.05): first character composes ( |
Fixes #5883.
On Android, composing the first character into an empty leaf cancels the composition, leaving that character behind on its own. In Korean, typing
안녕producesㅇ안녕orㅇㅏㄴ녕; in Japanese,haproduceshあ(the report in #5883); pinyin and Gboard English autocorrect break the same way. It affects the start of the editor and the start of any empty block, so on a chat-style app every message a Korean user types starts with a stray jamo.Why it happens
An empty leaf does not render a text node — it renders
ZeroWidthString, which on Android is a<span data-slate-zero-width><br/></span>. When the IME composes the first character there, the browser creates the text node, and the composition lives inside a node React does not know about. Anything that replaces that node cancels the composition, and the IME then starts a fresh session, orphaning what it had composed so far.Four separate paths do exactly that:
insertCompositionText. Fix Android cursor jumping to word start after autocorrect #5901 added ascheduleActionthat only re-selects, butscheduleActionsetsactionTimeoutId = setTimeout(flush), so a flush lands on the next task — about 10 ms after the first keystroke. Flushing applies the pending diff, the leaf stops being empty, and the re-render swapsZeroWidthStringforTextString, removing the composing node. This is what made the breakage deterministic rather than timing-dependent.FLUSH_DELAYtimer. Same re-render, 200 ms after a keystroke, so it fires for anyone typing at a relaxed pace. This is the pre-Fix Android cursor jumping to word start after autocorrect #5901 version of the bug.RestoreDOM. The browser-created text node is an untracked childList mutation, so any mid-composition re-render (thecompositionstartstate update, a placeholder resize) reverts it away.restoreDOMalready skipscharacterDatamutations "because this interrupts the composition" — childList mutations interrupt it just as much when the composition lives in a node that was just added.TextString's layout effect. It force-rewritestextContentwhenever the DOM differs from the value. During composition the DOM is supposed to run ahead of the value — that is what the pending-diff design is for — so this replaces the composing text node on any render that happens mid-composition.The fix
android-input-manager: replace Fix Android cursor jumping to word start after autocorrect #5901'sscheduleActionwithEDITOR_TO_PENDING_SELECTION. The caret still lands where Fix Android cursor jumping to word start after autocorrect #5901 intended (applied on the next flush) without forcing a flush onto the next task.android-input-manager: inflush, defer whileIS_COMPOSINGand the pending diffs target a leaf that is still empty in the value. That is the only case where applying a diff changes DOM structure; composing into a leaf that already has text only updatestextContent, so it is untouched.android-input-manager: apply those deferred diffs synchronously (flushSync) inhandleCompositionEnd. IMEs that compose a syllable at a time start the next composition in the same tick, so an async render would replace a node the new composition has already started in.restore-dom-manager: skip reverting childList mutations that contain the node the IME is composing in — the same rule already applied tocharacterDatamutations.string.tsx: skip thetextContentrewrite while the IME composes inside that span (Android only).Points 1–4 are inside Android-only code paths; point 5 is explicitly
IS_ANDROID-gated. Nothing changes for other platforms.How this differs from #5921
#5921 addressed the same issue but @12joan found it fixed only the non-empty-editor case, still failed
composes correctly at the start of the editor, and newly failedupdates the Slate value during compositionbecause the value stopped updating on everycompositionupdate.The deferral here is narrowed to pending diffs whose target leaf is empty in the value. In
updates the Slate value during compositionthe caret sits afterType here:, so the leaf is not empty, no deferral happens, and the value still updates on everycompositionupdate. I reproduced that test's shape (caret after existing text, value read mid-composition) and the value contains the composed text before composition ends.I could not run 12joan/slate-android-tests directly (it drives Appium/BrowserStack), so I reproduced each of the four cases by hand against the manual suite instead. Happy to re-verify if you can run the automated suite against this branch.
Verification
Emulator, real Gboard — Pixel API 34 AVD, Android 14, Gboard 12.4.05.482060964 (the same Gboard build @12joan reported), Chrome 113, typing on the actual on-screen keyboard at human pace into
/examples/android-tests:Empty— type 안녕 (Korean 2-bulsik)ㅇㅏㄴ녕, Gboard's suggestion strip desynced toㅏㄴ녕안녕, suggestions안녕 / 안뇽 / 안녕하세요Autocorrect— typeCant, space, theniCan't iCan't i(caret stays after the corrected word, #5901 still fixed)Chromium composition pipeline via CDP (
Input.imeSetComposition/Input.insertTextunder an Android UA), against/examples/plaintextemptied first, driving Gboard's exact event stream:안녕, 350 ms/keyㅇ안녕안녕안녕, 120 ms/keyㅇ안녕안녕hellohhellohello안녕안녕yarn test:jestpasses (96/96),lint:eslintandlint:prettierare clean.lint:typescriptandtest:mochafail identically onmainin my environment (pre-existing, unrelated to this change), with no errors in the files touched here.Also included
A manual
IME first charactercase inandroid-tests, as @12joan asked for on #5921, covering both the start of the editor and the start of an empty block, with the expected-vs-broken output spelled out.Downstream
Found while fixing this in Cinny, a Matrix client, where it makes every Korean message start with a stray jamo: cinnyapp/cinny#3066 carries these same changes as a
patch-packagepatch until this lands.🤖 Generated with Claude Code