From 7278fa6a132e598f9aaa764b6b34adcdc460ddf3 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Sun, 31 May 2026 01:42:16 +0800 Subject: [PATCH 1/2] fix(editor): stop escapeMarkBoundary swallowing Escape for multi-cursor selections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/plugins/editorPlugins/keymapUtils.test.ts | 36 ++++++++++++++++--- src/plugins/editorPlugins/keymapUtils.ts | 6 ++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/plugins/editorPlugins/keymapUtils.test.ts b/src/plugins/editorPlugins/keymapUtils.test.ts index dc5b9494..ab32d2f9 100644 --- a/src/plugins/editorPlugins/keymapUtils.test.ts +++ b/src/plugins/editorPlugins/keymapUtils.test.ts @@ -175,7 +175,7 @@ describe("escapeMarkBoundary", () => { it("returns false when no mark range and no stored marks", () => { const view = { state: { - selection: { $from: { pos: 1 }, empty: true }, + selection: { $from: { pos: 1 }, empty: true, ranges: [{}] }, storedMarks: null, }, dispatch: vi.fn(), @@ -187,7 +187,7 @@ describe("escapeMarkBoundary", () => { it("returns false when no mark range and stored marks is empty array", () => { const view = { state: { - selection: { $from: { pos: 1 }, empty: true }, + selection: { $from: { pos: 1 }, empty: true, ranges: [{}] }, storedMarks: [], }, dispatch: vi.fn(), @@ -201,7 +201,7 @@ describe("escapeMarkBoundary", () => { const mockTr = { setStoredMarks: vi.fn().mockReturnThis() }; const view = { state: { - selection: { $from: { pos: 1 }, empty: true }, + selection: { $from: { pos: 1 }, empty: true, ranges: [{}] }, storedMarks: [{ type: "bold" }], tr: mockTr, }, @@ -221,7 +221,7 @@ describe("escapeMarkBoundary", () => { const mockTr = { setStoredMarks: vi.fn().mockReturnThis() }; const view = { state: { - selection: { $from: { pos: 5 }, empty: true }, + selection: { $from: { pos: 5 }, empty: true, ranges: [{}] }, storedMarks: null, tr: mockTr, }, @@ -283,7 +283,7 @@ describe("escapeMarkBoundary", () => { const mockTr = { setStoredMarks: vi.fn().mockReturnThis() }; const view = { state: { - selection: { $from: { pos: 1 }, empty: true }, + selection: { $from: { pos: 1 }, empty: true, ranges: [{}] }, storedMarks: null, tr: mockTr, }, @@ -296,6 +296,32 @@ describe("escapeMarkBoundary", () => { expect(dispatchFn).toHaveBeenCalledWith(mockTr); }); + it("does not act on a multi-range (multi-cursor) selection, even with stored marks", () => { + // Regression: a MultiSelection whose primary is a caret reports empty===true, + // so it slips past the `!empty` guard. With a stored mark present, + // escapeMarkBoundary used to return true and swallow Escape — preempting the + // multi-cursor collapse so the secondary selection (e.g. over a list) never + // cleared. It must bail for multi-range selections and let the multi-cursor + // Escape handler reduce them. + const dispatchFn = vi.fn(); + const mockTr = { setStoredMarks: vi.fn().mockReturnThis() }; + const view = { + state: { + selection: { + $from: { pos: 1 }, + empty: true, // primary range is a caret + ranges: [{}, {}], // multi-cursor: more than one range + }, + storedMarks: [{ type: "bold" }], // would otherwise trigger the swallow + tr: mockTr, + }, + dispatch: dispatchFn, + } as never; + + expect(escapeMarkBoundary(view)).toBe(false); + expect(dispatchFn).not.toHaveBeenCalled(); + }); + it("moves cursor to mark end when inside mark range using real state", () => { // eslint-disable-next-line @typescript-eslint/no-require-imports const { Schema } = require("@tiptap/pm/model"); diff --git a/src/plugins/editorPlugins/keymapUtils.ts b/src/plugins/editorPlugins/keymapUtils.ts index 31c7b090..31a9553b 100644 --- a/src/plugins/editorPlugins/keymapUtils.ts +++ b/src/plugins/editorPlugins/keymapUtils.ts @@ -26,6 +26,12 @@ export function escapeMarkBoundary(view: EditorView): boolean { const { $from, empty } = selection; if (!empty) return false; + // Bail for multi-range (multi-cursor) selections. A MultiSelection whose + // primary is a caret reports empty===true, so it reaches here — but operating + // on its primary $from (and clearing stored marks) would return true and + // swallow Escape, preempting the multi-cursor keymap's own Escape handler that + // collapses the secondary ranges. Let that handler run instead. + if (selection.ranges.length > 1) return false; const pos = $from.pos; const anyMarkRange = findAnyMarkRangeAtCursor( From ff2376334d22f65abe4f09ba9f282400b9ad52f2 Mon Sep 17 00:00:00 2001 From: xiaolai Date: Sun, 31 May 2026 02:09:51 +0800 Subject: [PATCH 2/2] test(editor): real-state Escape regression + correct scope to multi-caret (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. --- src/plugins/editorPlugins/keymapUtils.test.ts | 73 +++++++++++++++++-- 1 file changed, 67 insertions(+), 6 deletions(-) diff --git a/src/plugins/editorPlugins/keymapUtils.test.ts b/src/plugins/editorPlugins/keymapUtils.test.ts index ab32d2f9..0c5f4e09 100644 --- a/src/plugins/editorPlugins/keymapUtils.test.ts +++ b/src/plugins/editorPlugins/keymapUtils.test.ts @@ -31,6 +31,8 @@ import { } from "./keymapUtils"; import { canRunActionInMultiSelection } from "@/plugins/toolbarActions/multiSelectionPolicy"; import { findAnyMarkRangeAtCursor } from "@/plugins/syntaxReveal/marks"; +import { MultiSelection } from "@/plugins/multiCursor/MultiSelection"; +import { collapseMultiSelection } from "@/plugins/multiCursor/commands"; import type { Command } from "@tiptap/pm/state"; describe("toProseMirrorKey", () => { @@ -297,12 +299,13 @@ describe("escapeMarkBoundary", () => { }); it("does not act on a multi-range (multi-cursor) selection, even with stored marks", () => { - // Regression: a MultiSelection whose primary is a caret reports empty===true, - // so it slips past the `!empty` guard. With a stored mark present, - // escapeMarkBoundary used to return true and swallow Escape — preempting the - // multi-cursor collapse so the secondary selection (e.g. over a list) never - // cleared. It must bail for multi-range selections and let the multi-cursor - // Escape handler reduce them. + // Regression: a multi-caret MultiSelection where every range is an empty + // caret reports empty===true (Selection.empty is true only when ALL ranges + // are empty), so it slips past the `!empty` guard. With a stored mark + // present, escapeMarkBoundary used to return true and swallow Escape — + // preempting the multi-cursor collapse, so the extra carets were never + // reduced. It must bail for multi-range selections and let the multi-cursor + // Escape handler collapse them. const dispatchFn = vi.fn(); const mockTr = { setStoredMarks: vi.fn().mockReturnThis() }; const view = { @@ -322,6 +325,64 @@ describe("escapeMarkBoundary", () => { expect(dispatchFn).not.toHaveBeenCalled(); }); + it("real multi-caret MultiSelection + stored mark: does not swallow, and the multi-cursor handler then collapses it (#981 chain)", () => { + // Integration proof of the two-handler Escape chain with a REAL + // MultiSelection (not a mock). The swallow only happens when EVERY range is + // an empty caret — 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`. For the all-carets case the + // editor keymap's escapeMarkBoundary must return false (no swallow) even with + // a stored mark at the primary caret, and the multi-cursor keymap's + // collapseMultiSelection must then reduce it to a plain TextSelection. + // eslint-disable-next-line @typescript-eslint/no-require-imports + const { Schema } = require("@tiptap/pm/model"); + // eslint-disable-next-line @typescript-eslint/no-require-imports + const { EditorState, SelectionRange } = require("@tiptap/pm/state"); + + const testSchema = new Schema({ + nodes: { + doc: { content: "paragraph+" }, + paragraph: { content: "text*", group: "block" }, + text: { inline: true }, + }, + marks: { bold: {} }, + }); + const doc = testSchema.node("doc", null, [ + testSchema.node("paragraph", null, [testSchema.text("hello world")]), + ]); + let state = EditorState.create({ doc, schema: testSchema }); + + // Two empty carets (pos 1 and pos 5) — the all-empty multi-caret case. + const primary = new SelectionRange(doc.resolve(1), doc.resolve(1)); + const secondary = new SelectionRange(doc.resolve(5), doc.resolve(5)); + const multiSel = new MultiSelection([primary, secondary], 0); + state = state.apply( + state.tr + .setSelection(multiSel) + .setStoredMarks([testSchema.marks.bold.create()]) + ); + + // Preconditions: the trap shape — all-empty multi-caret + stored mark. + expect(state.selection.empty).toBe(true); + expect(state.selection.ranges.length).toBe(2); + + // 1) The mark-escape helper must NOT handle it (no swallow, marks untouched). + const dispatch = vi.fn(); + expect(escapeMarkBoundary({ state, dispatch } as never)).toBe(false); + expect(dispatch).not.toHaveBeenCalled(); + + // 2) The multi-cursor Escape handler then collapses it to a single, + // non-MultiSelection caret. (Assert against MultiSelection — the + // top-level import — rather than a require()'d TextSelection, which would + // be a different module instance and break instanceof.) + const tr = collapseMultiSelection(state); + expect(tr).not.toBeNull(); + const collapsed = state.apply(tr); + expect(collapsed.selection).not.toBeInstanceOf(MultiSelection); + expect(collapsed.selection.ranges.length).toBe(1); + expect(collapsed.selection.empty).toBe(true); + }); + it("moves cursor to mark end when inside mark range using real state", () => { // eslint-disable-next-line @typescript-eslint/no-require-imports const { Schema } = require("@tiptap/pm/model");