Skip to content

fix: dispatch soft keyboard deletions through the command manager - #6010

Merged
nightwing merged 1 commit into
ajaxorg:masterfrom
aron-intframe:fix/5304-soft-keyboard-deletion-command
Oct 21, 2026
Merged

nightwing merged 1 commit into
ajaxorg:masterfrom
aron-intframe:fix/5304-soft-keyboard-deletion-command

Conversation

@aron-intframe

@aron-intframe aron-intframe commented Sep 26, 2026 •

Copy link
Copy Markdown

Issue #, if available: #5304

Description of changes:

Soft keyboard and IME deletions arrive as input/compositionupdate and are applied by applyComposition, which called this.remove() directly; insertions from the same events go out as insertstring. So commands.on("exec", ...) sees typing but never deletion. Replaying the sequence recorded in test: mobile text deletion: insert → 2 exec events, delete → 0, desktop Backspace → 1. This routes that removal through the command manager as backspace/del.

Limits I know of: the event is informational, cancelling it does not cancel the deletion; with multiple cursors forEachSelection's guard skips the command itself, exec still fires and the text is unchanged; composition insertions still bypass commands. No Android device here, so I reproduced with this repo's recorded sequences, not the reporter's Gboard one; both paths reach the same line.

User-facing changes: exec/afterExec now fire for these deletions, with the command names desktop keys use. Text and cursor position unchanged in every case I measured.

Tests: two tests in textinput_test.js replaying recorded deletions. 1498 passing, eslint + typecheck clean. Reverting the one-line change turns both red.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

  • No backwards incompatible changes to Ace's public interface (ace.d.ts unchanged).

Open kitchen-sink @ 96546741a401764548be88e9a759eb2cbbaa1f7b

Deletions that arrive as input/compositionupdate events are applied by
applyComposition, which called editor.remove() directly, so
commands.on("exec") never saw them, while insertions from the same events
are dispatched as the insertstring command.

Route that removal through the command manager, reusing the existing
backspace/del commands depending on which side the range was extended.

Fixes ajaxorg#5304
@codecov

codecov Bot commented Oct 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.92%. Comparing base (62d5ffd) to head (9654674).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6010      +/-   ##
==========================================
- Coverage   93.92%   93.92%   -0.01%     
==========================================
  Files         647      647              
  Lines      140792   140835      +43     
  Branches    14784    14768      -16     
==========================================
+ Hits       132237   132273      +36     
- Misses       8555     8562       +7     
Flag Coverage Δ
unittests 93.92% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nightwing

Copy link
Copy Markdown
Member

Merging, but this may interfere with composition on desktop, in which case we will have to limit this with useragent.isMobile()

@nightwing
nightwing merged commit cf883e3 into ajaxorg:master Oct 21, 2026
4 checks passed
@aron-intframe

Copy link
Copy Markdown
Author

Checked the composition risk instead of guessing. Short version: on every desktop IME path I can replay here, routing the removal through the command manager changes nothing except that the extra event fires, so useragent.isMobile does not look necessary yet. If you add it anyway, two things will bite, one of them typo-level.

What I ran. Cloned master at cf883e3, replayed the IME recordings through textInput with a snapshot after every single event (value, cursor, inComposition, composition marker range, undo-stack depth, undo delta grouping, curOp), then re-ran the identical replay with the line reverted to this.remove() and diffed the two traces.

Desktop Chinese IME, Backspace pressed inside an active composition (a trimmed replay of the test: chinese ime on ie recording, which is the one existing desktop sequence that actually reaches the !text && !r.isEmpty() branch, 4 times, with composition live):

ev10 compositionupdate value="k" cursor=0,1 inComposition=yes marker=0,0-0,1 undoStack=1 undoDeltas=3 curOp=null

Composition stays open, the marker keeps tracking, and the removal lands inside the same undo group. The entire diff between this.remove() and execCommand(...) across every snapshot of every sequence is one line:

-execs=["insertstring","insertstring","insertstring"]
+execs=["insertstring","insertstring","backspace","insertstring"]

I also ran it with enableLiveAutocompletion on, since doLiveAutocomplete is the realistic afterExec consumer that could disturb an IME (it calls completer.detach() on backspace):

afterExecs=["insertstring","insertstring","backspace","insertstring"]
completerActivated=true
textarea="ky\n\n" editorValue="keyword kerosene\nky"

Popup survives, textarea and document stay in sync. I also re-checked the PR body's claim that the event is informational: with a listener calling e.preventDefault() on the dispatched backspace, the character is still gone, because applyComposition then falls through to this.insert(text, true) over the still non-empty selection.

If you do add the guard, two traps.

  1. It is a property, not a function:
exports.isMobile = exports.isIOS || exports.isAndroid;

so useragent.isMobile() throws; it has to be useragent.isMobile.

  1. Reading useragent.isMobile from editor.js is invisible to $setUserAgentForTests, which only reassigns textinput.js's module-local isMobile. I applied the guard that way and both new tests go red:
    1) test: mobile text deletion is dispatched as a command
    2) test: mobile forward deletion is dispatched as a command

What does work locally: carry the flag on the composition descriptor textinput.js already builds (isMobile: isMobile, next to extendLeft/extendRight), branch on composition.isMobile in applyComposition, and add setUserAgentForTests(true, false) to those two tests. That gives 20 passing in textinput_test.js. I have not pushed it, since the traces above say the guard would only drop the event for desktop IME Backspace without fixing anything I can observe. Say the word and I will open it as a follow-up PR.

Not verified: there is no real desktop browser or physical IME on this machine, so all of the above is replay of this repo's recorded sequences, not a live macOS/Windows IME session. If composition does break for someone on a real desktop IME, these traces point at a consumer's exec/afterExec handler rather than at composition state inside applyComposition.

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.

2 participants