Skip to content

Don't cancel every pending composition send when one key finalizes - #6090

Open
joonhoekim wants to merge 1 commit into
xtermjs:masterfrom
joonhoekim:fix-composition-cancel-all-pending-sends
Open

Don't cancel every pending composition send when one key finalizes#6090
joonhoekim wants to merge 1 commit into
xtermjs:masterfrom
joonhoekim:fix-composition-cancel-all-pending-sends

Conversation

@joonhoekim

Copy link
Copy Markdown

Fixes #6089.

_finalizeComposition defers each finished composition to a setTimeout(…, 0) and guards
every pending send with one shared boolean, _isSendingComposition. That is sound only 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. 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 error
and 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

  • A FIFO queue replaces the shared boolean. The synchronous path drains it in order instead
    of cancelling it, so a send is only ever skipped by whoever actually ran it. keydown's gate
    now reads _pendingSends.length > 0, which stays correct while several are queued — a
    boolean cleared by the first callback would have claimed nothing was in flight.
  • A send watermark (_sentUpTo) makes draining safe. Every send emits
    [max(start, _sentUpTo), end) and pushes it forward, so no range can go out twice no matter
    which path reaches it first. Without it, draining the queued composition and then emitting the
    synchronous slice would duplicate. It is lowered again on compositionstart in case
    _syncTextArea rewrote the value underneath and the offsets no longer line up.
  • The deferred send's end boundary no longer branches 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?
  • The synchronous path widens its end to the current caret. _compositionPosition.end is
    only advanced from compositionupdate's own 0 ms timer, so a key interrupting a composition
    before that timer runs sliced [start, start) and emitted nothing; typing 가나 then space
    used to leave just the space.

One small behaviour change worth calling out: the synchronous path now skips
triggerDataEvent when the slice is empty, matching what the deferred path already did.

Tests

Two new cases in CompositionHelper.test.ts, both failing on master:

1) Should not drop a queued composition when a non-composition key ends a later one
     expected '' to equal '니다'
2) Should send an in-progress composition interrupted by a non-composition key
     expected '' to equal '가'

With the change, npm run test-unit is 2409 passing (2407 before, plus these two) and
npm run lint is clean. The existing composition tests — Korean carry-over, Japanese
conversion, 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.

`_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 …`.
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.

Fast IME typing drops a committed composition: _isSendingComposition cancels every pending send, not just one

1 participant