fix(editor): Escape can't collapse an all-empty multi-caret when a stored mark is present#981
Merged
Merged
Conversation
…or selections A MultiSelection whose primary range is a caret reports empty===true, so it slips past collapseNonEmptySelection's empty-guard and reaches escapeMarkBoundary. That helper only guarded !empty, so for a multi-cursor selection with a mark (or stored marks) at the primary caret it cleared the marks and returned true — swallowing Escape before the multi-cursor keymap's own Escape handler could collapse the secondary ranges. The leftover secondary selection (e.g. over a list) then could not be dismissed with Escape. Intermittent because it depended on mark/stored-mark state at the caret. Bail from escapeMarkBoundary on multi-range selections (mirroring the guard collapseNonEmptySelection already has) so editorKeymap's Escape returns false and the multi-cursor handler reduces the selection. Regression test added.
…aret (audit follow-up) Codex audit-fix flagged the regression test as mock-only. Adding a real EditorState + MultiSelection integration test revealed the original premise was wrong: Selection.empty is true only when ALL ranges are empty, so a multi-cursor selection with any non-empty range has empty===false and escapeMarkBoundary already bails via !empty. The swallow only affects the all-empty multi-caret case. Test and comments corrected to that true scenario; the production guard is unchanged and correct for it.
xiaolai
changed the title
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
1 participant
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.
Scope correction
This started as an attempt to fix "Escape sometimes can't clear a selection over a list," but the real-state regression test (added per a Codex audit) disproved that root cause:
Selection.emptyis true only when all ranges are empty, so a multi-cursor selection with any non-empty (visible) range hasempty === falseandescapeMarkBoundaryalready bails via!empty. Escape already works for the visible-selection case.What this PR actually fixes is a narrower, real bug.
Bug
With multiple empty carets (multi-cursor, no selection) and a stored mark / mark at the primary caret, pressing Escape was swallowed: the editor keymap (priority 1000) runs first,
collapseNonEmptySelectionskips (empty), andescapeMarkBoundary— guarding only!empty— cleared the stored marks and returnedtrue. That preempted the multi-cursor keymap'scollapseMultiSelection, so the extra carets were never reduced.Fix
escapeMarkBoundarynow bails on multi-range selections (selection.ranges.length > 1), mirroring the guardcollapseNonEmptySelectionalready has. The editor keymap's Escape then returns false and the multi-cursor handler collapses the carets.Test
keymapUtils.test.ts:MultiSelectionof two empty carets + a stored mark →escapeMarkBoundaryreturns false (no swallow), thencollapseMultiSelectionreduces it to a single caret. (The existing single-cursor mocks gained a realisticranges: [{}].)Not covered (separate issue)
The originally-reported "visible multi-cursor selection over a list won't clear with one Escape" is a different behavior —
collapseMultiSelectioncollapses to the primary range (not a caret), so a non-empty primary leaves a selection after one Escape. Needs its own investigation/decision.Verification
pnpm test src/plugins/editorPlugins/keymapUtils.test.ts+ multiCursor suites passpnpm check:allgreen