Don't cancel every pending composition send when one key finalizes - #6090
Open
joonhoekim wants to merge 1 commit into
Open
Don't cancel every pending composition send when one key finalizes#6090joonhoekim wants to merge 1 commit into
joonhoekim wants to merge 1 commit into
Conversation
`_finalizeComposition` defers each finished composition to a `setTimeout(…, 0)` and guards every pending send with one shared boolean, `_isSendingComposition`. That is only sound while at most one send can be outstanding, and nothing enforces it: under load, input events arrive in bursts, so a second composition can start *and* finish before the first one's timer gets a turn. When the next non-composition key then takes the synchronous path, clearing the boolean to cancel "the" pending send cancels every other one too. Whatever text they were carrying is dropped with no error and no partial glyph — typing `알겠습니다.` quickly yields `알겠습다.`, with `니` never reaching the handler. The slice was never wrong; the callback that would have computed it never ran. Replace the boolean with a FIFO queue and have the synchronous path drain it in order instead of cancelling it, so each send is only ever skipped by whoever actually ran it. A send watermark (`_sentUpTo`) records how far into the textarea has been forwarded, which makes draining safe: no range can go out twice regardless of which path reaches it first, so the drain does not need to guess whether the synchronous slice would have covered it. The deferred send's end boundary can no longer branch on `_isComposing`. When a send is flushed by the drain rather than by its own timer, `_finalizeComposition` has already cleared that flag, and the old else branch would read on into the newer composition's preedit. It now asks what the check actually meant — did a newer composition start past my start offset? — which does not depend on timing. The synchronous path also widens its end to the current caret. Its `_compositionPosition.end` is only advanced from `compositionupdate`'s own 0 ms timer, so a key that interrupts a composition before that timer runs would slice `[start, start)` and emit nothing; typing `가나` then space used to leave just the space. Both cases are covered by new unit tests, which fail on master with `expected '' to equal …`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6089.
_finalizeCompositiondefers each finished composition to asetTimeout(…, 0)and guardsevery pending send with one shared boolean,
_isSendingComposition. That is sound only whileat most one send can be outstanding, and nothing enforces it — under load input events arrive
in bursts, so a second composition can start and finish before the first one's timer gets a
turn. The next non-composition key then takes the synchronous path, and clearing the boolean
to cancel "the" pending send cancels every other one too.
Typing
알겠습니다.quickly gives알겠습다.—니never reaches the handler, with no errorand no partial glyph. The slice was never wrong; the callback that would have computed it
never ran. Full trace and analysis in #6089.
What changed
of cancelling it, so a send is only ever skipped by whoever actually ran it.
keydown's gatenow reads
_pendingSends.length > 0, which stays correct while several are queued — aboolean cleared by the first callback would have claimed nothing was in flight.
_sentUpTo) makes draining safe. Every send emits[max(start, _sentUpTo), end)and pushes it forward, so no range can go out twice no matterwhich path reaches it first. Without it, draining the queued composition and then emitting the
synchronous slice would duplicate. It is lowered again on
compositionstartin case_syncTextArearewrote the value underneath and the offsets no longer line up._isComposing. When a send isflushed by the drain rather than by its own timer,
_finalizeCompositionhas already clearedthat flag, and the old
elsebranch would read on into the newer composition's preedit. Itnow asks what the check actually meant — did a newer composition start past my start offset?
_compositionPosition.endisonly advanced from
compositionupdate's own 0 ms timer, so a key interrupting a compositionbefore that timer runs sliced
[start, start)and emitted nothing; typing가나then spaceused to leave just the space.
One small behaviour change worth calling out: the synchronous path now skips
triggerDataEventwhen the slice is empty, matching what the deferred path already did.Tests
Two new cases in
CompositionHelper.test.ts, both failing on master:With the change,
npm run test-unitis 2409 passing (2407 before, plus these two) andnpm run lintis clean. The existing composition tests — Korean carry-over, Japaneseconversion, non-composition characters typed straight after a commit, and trailing-text
preservation — all still pass unchanged.
What I could not test
Only Chromium on Linux with fcitx5 (Korean). The Windows TSF and macOS paths exercised by
#6049 and #5887 are untouched by this change in principle, but I have no way to run them.