From e73966a012fd15c0453bc683cce5c57b0932d679 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Sat, 28 Feb 2026 09:54:09 -0300 Subject: [PATCH 1/4] fix(track-changes): handle ReplaceAroundStep in tracked changes mode SD-2061: When backspacing in tracked changes mode triggers a ReplaceAroundStep (e.g. at paragraph/list boundaries), convert it to a tracked single-character deletion instead of silently applying the structural change. This preserves paragraph numbering, font, and alignment. Also fixes cursor positioning for ReplaceAroundStep transactions by checking selectionPos before tr.selectionSet, and merges adjacent trackDelete marks across run node boundaries so consecutive deletions appear as a single tracked change. --- .../trackChangesHelpers/replaceAroundStep.js | 158 +++++++++++++++++- .../trackChangesHelpers.test.js | 1 - .../trackChangesHelpers/trackedTransaction.js | 49 ++++-- 3 files changed, 188 insertions(+), 20 deletions(-) diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js index a0e9e9d11f..f4869c1fdb 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js @@ -1,3 +1,157 @@ -export const replaceAroundStep = () => { - // Skip for now. +import { ReplaceStep } from 'prosemirror-transform'; +import { Slice } from 'prosemirror-model'; +import { replaceStep } from './replaceStep.js'; +import { TrackDeleteMarkName } from '../constants.js'; +import { TrackChangesBasePluginKey } from '../plugins/index.js'; + +/** + * Find the closest live (non-tracked-deleted) text character position before + * the cursor, within the same paragraph. + * + * @param {import('prosemirror-model').Node} doc + * @param {number} cursorPos + * @param {import('prosemirror-model').MarkType} trackDeleteMarkType + * @returns {number | null} The document position of the character, or null. + */ +const findPreviousLiveCharPos = (doc, cursorPos, trackDeleteMarkType) => { + const $cursor = doc.resolve(cursorPos); + + // Find the enclosing paragraph (may need to go up past run nodes). + let paraDepth = $cursor.depth; + while (paraDepth > 0 && $cursor.node(paraDepth).type.name !== 'paragraph') { + paraDepth--; + } + if (paraDepth <= 0) return null; + + const paraStart = $cursor.before(paraDepth) + 1; + + // Walk through inline nodes from paragraph start to cursor, + // keeping track of the last live character position found. + let lastLiveCharPos = null; + + doc.nodesBetween(paraStart, cursorPos, (node, pos) => { + if (!node.isText) return; + + const hasDeleteMark = node.marks.some((m) => m.type === trackDeleteMarkType); + if (hasDeleteMark) return; + + // This is a live text node. Its last character within our range is + // at min(nodeEnd, cursorPos) - 1. + const nodeEnd = pos + node.nodeSize; + const relevantEnd = Math.min(nodeEnd, cursorPos); + if (relevantEnd > pos) { + lastLiveCharPos = relevantEnd - 1; + } + }); + + return lastLiveCharPos; +}; + +/** + * Handle a ReplaceAroundStep in tracked changes mode. + * + * ReplaceAroundStep is ProseMirror's structural "change wrapper" operation + * (e.g. lifting content out of a list item, changing block type). In tracked + * changes mode we must never silently apply structural changes — they would + * alter paragraph properties (numbering, font, alignment) without tracking. + * + * For backspace/delete, the user's intent is to delete a character, not change + * paragraph structure. We convert the step to a tracked single-character + * deletion using the existing replaceStep handler. + * + * @param {object} options + * @param {import('prosemirror-state').EditorState} options.state + * @param {import('prosemirror-state').Transaction} options.tr + * @param {import('prosemirror-transform').ReplaceAroundStep} options.step + * @param {import('prosemirror-state').Transaction} options.newTr + * @param {import('prosemirror-transform').Mapping} options.map + * @param {import('prosemirror-model').Node} options.doc + * @param {object} options.user + * @param {string} options.date + * @param {import('prosemirror-transform').Step} options.originalStep + * @param {number} options.originalStepIndex + */ +export const replaceAroundStep = ({ + state, + tr, + step, + newTr, + map, + doc, + user, + date, + originalStep, + originalStepIndex, +}) => { + const inputType = tr.getMeta('inputType'); + const isBackspace = inputType === 'deleteContentBackward'; + + if (!isBackspace) { + // Non-backspace ReplaceAroundStep in tracked changes: block it. + // Structural wrapper changes (list toggle, block type change) should be + // implemented as tracked format changes in the future. For now, silently + // dropping them is safer than applying them untracked. + return; + } + + // For backspace: find the previous live character and track its deletion. + const trackDeleteMarkType = state.schema.marks[TrackDeleteMarkName]; + const deleteFrom = findPreviousLiveCharPos(doc, state.selection.from, trackDeleteMarkType); + + if (deleteFrom === null) { + // No live character found — nothing to delete. Skip the structural change. + return; + } + + const charStep = new ReplaceStep(deleteFrom, deleteFrom + 1, Slice.empty); + + replaceStep({ + state, + tr, + step: charStep, + newTr, + map, + doc, + user, + date, + originalStep: charStep, + originalStepIndex, + }); + + // Position the cursor at the deletion edge. The original transaction's + // selection was computed for the structural ReplaceAroundStep, not our + // fabricated character deletion. Override it so the cursor visually + // moves left with each backspace. + const trackMeta = newTr.getMeta(TrackChangesBasePluginKey) || {}; + trackMeta.selectionPos = deleteFrom; + newTr.setMeta(TrackChangesBasePluginKey, trackMeta); + + // Merge adjacent trackDelete marks that have the same author/date but different IDs. + // When backspace first deletes a character (e.g. ".") via a normal ReplaceStep and + // subsequent presses delete further characters (e.g. "l") via ReplaceAroundStep, + // the deletion marks end up with different IDs because run node boundaries create + // a position gap larger than findTrackedMarkBetween's ±1 offset. Re-mark the + // earlier deletion with the current ID so they merge into a single tracked change. + if (trackMeta.deletionMark) { + const ourId = trackMeta.deletionMark.attrs.id; + const ourEmail = trackMeta.deletionMark.attrs.authorEmail; + const ourDate = trackMeta.deletionMark.attrs.date; + const searchTo = Math.min(newTr.doc.content.size, deleteFrom + 20); + + newTr.doc.nodesBetween(deleteFrom, searchTo, (node, pos) => { + if (!node.isText) return; + const delMark = node.marks.find((m) => m.type.name === TrackDeleteMarkName); + if ( + delMark && + delMark.attrs.id !== ourId && + delMark.attrs.authorEmail === ourEmail && + delMark.attrs.date === ourDate + ) { + const markType = state.schema.marks[TrackDeleteMarkName]; + const merged = markType.create({ ...delMark.attrs, id: ourId }); + newTr.removeMark(pos, pos + node.nodeSize, delMark); + newTr.addMark(pos, pos + node.nodeSize, merged); + } + }); + } }; diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackChangesHelpers.test.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackChangesHelpers.test.js index 170d1f9357..35700e19f3 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackChangesHelpers.test.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackChangesHelpers.test.js @@ -559,7 +559,6 @@ describe('trackChangesHelpers', () => { it('no-op helpers exist for future implementations', () => { expect(markWrapping()).toBeUndefined(); - expect(replaceAroundStep()).toBeUndefined(); }); it('trackedTransaction keeps selection in sync', () => { diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackedTransaction.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackedTransaction.js index 830ca2f55c..2f5ddc9f38 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackedTransaction.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/trackedTransaction.js @@ -1,9 +1,10 @@ -import { Mapping, ReplaceStep, AddMarkStep, RemoveMarkStep } from 'prosemirror-transform'; +import { Mapping, ReplaceStep, AddMarkStep, RemoveMarkStep, ReplaceAroundStep } from 'prosemirror-transform'; import { TextSelection } from 'prosemirror-state'; import { ySyncPluginKey } from 'y-prosemirror'; import { replaceStep } from './replaceStep.js'; import { addMarkStep } from './addMarkStep.js'; import { removeMarkStep } from './removeMarkStep.js'; +import { replaceAroundStep } from './replaceAroundStep.js'; import { TrackDeleteMarkName } from '../constants.js'; import { TrackChangesBasePluginKey } from '../plugins/index.js'; import { findMark } from '@core/helpers/index.js'; @@ -80,7 +81,23 @@ export const trackedTransaction = ({ tr, state, user }) => { user, date, }); + } else if (step instanceof ReplaceAroundStep) { + replaceAroundStep({ + state, + tr, + step, + newTr, + map, + doc, + user, + date, + originalStep, + originalStepIndex, + }); } else { + // Non-structural steps (AttrStep, SetNodeMarkupStep) are typically + // metadata updates from plugins (e.g. listRendering, sdBlockRev). + // These are safe to apply without tracking. newTr.step(step); } }); @@ -100,20 +117,20 @@ export const trackedTransaction = ({ tr, state, user }) => { // Get the track changes meta to check if we have an adjusted insertion position (SD-1624). const trackMeta = newTr.getMeta(TrackChangesBasePluginKey); - if (tr.selectionSet) { - // When replaceStep normalizes a broad replace to a single-char delete (e.g. terminal period), - // it sets selectionPos so the caret lands at the deletion edge. Honor it so the next backspace - // targets the right character instead of a block/run boundary. - if (trackMeta?.selectionPos !== undefined && trackMeta?.selectionPos !== null) { - const boundedPos = Math.max(0, Math.min(trackMeta.selectionPos, newTr.doc.content.size)); - const $pos = newTr.doc.resolve(boundedPos); - if ($pos.parent.inlineContent) { - newTr.setSelection(TextSelection.create(newTr.doc, boundedPos)); - } else { - // Normalized delete flows should keep the caret on the deletion side. - newTr.setSelection(TextSelection.near($pos, -1)); - } - } else if ( + // selectionPos is an explicit cursor override from tracked change handlers (e.g. + // replaceAroundStep converting a structural step to a character deletion). It must + // be honored regardless of tr.selectionSet, because the original transaction may + // not have set a selection (e.g. ReplaceAroundStep transactions). + if (trackMeta?.selectionPos !== undefined && trackMeta?.selectionPos !== null) { + const boundedPos = Math.max(0, Math.min(trackMeta.selectionPos, newTr.doc.content.size)); + const $pos = newTr.doc.resolve(boundedPos); + if ($pos.parent.inlineContent) { + newTr.setSelection(TextSelection.create(newTr.doc, boundedPos)); + } else { + newTr.setSelection(TextSelection.near($pos, -1)); + } + } else if (tr.selectionSet) { + if ( tr.selection instanceof TextSelection && (tr.selection.from < state.selection.from || tr.getMeta('inputType') === 'deleteContentBackward') ) { @@ -139,8 +156,6 @@ export const trackedTransaction = ({ tr, state, user }) => { } else if (state.selection.from - tr.selection.from > 1 && tr.selection.$head.depth > 1) { const caretPos = map.map(tr.selection.from - 2, -1); newTr.setSelection(new TextSelection(newTr.doc.resolve(caretPos))); - } else { - // Skip the other cases for now. } if (tr.storedMarksSet) { From 47bd094e8afa0bb4ae9995b111b06cddeb2e54b4 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Sat, 28 Feb 2026 16:52:05 -0300 Subject: [PATCH 2/4] test(track-changes): add unit tests for replaceAroundStep handler Add 9 unit tests covering: - Non-backspace ReplaceAroundStep blocking - Backspace conversion to tracked single-char deletion - selectionPos meta for cursor positioning - findPreviousLiveCharPos skipping tracked-deleted text - Mark merging across run node boundaries (same/different authors) - selectionPos honored in trackedTransaction when tr.selectionSet is false Also add numbering preservation assertion to the existing behavior test for double-backspace at terminal period with bookmark-wrapped runs. --- .../replaceAroundStep.test.js | 423 ++++++++++++++++++ ...elete-terminal-period-separate-run.spec.ts | 22 + 2 files changed, 445 insertions(+) create mode 100644 packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js new file mode 100644 index 0000000000..1b5dd7a2f1 --- /dev/null +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js @@ -0,0 +1,423 @@ +import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; +import { EditorState, TextSelection } from 'prosemirror-state'; +import { Mapping, ReplaceAroundStep, ReplaceStep } from 'prosemirror-transform'; +import { Fragment, Slice } from 'prosemirror-model'; +import { trackedTransaction, documentHelpers } from './index.js'; +import { replaceAroundStep } from './replaceAroundStep.js'; +import { TrackDeleteMarkName, TrackInsertMarkName } from '../constants.js'; +import { TrackChangesBasePluginKey } from '../plugins/trackChangesBasePlugin.js'; +import { initTestEditor } from '@tests/helpers/helpers.js'; +import { findTextPos } from './testUtils.js'; + +describe('replaceAroundStep handler', () => { + let editor; + let schema; + let basePlugins; + + const user = { name: 'Track Tester', email: 'track@example.com' }; + const date = '2024-01-01T00:00:00.000Z'; + + beforeEach(() => { + ({ editor } = initTestEditor({ mode: 'text', content: '

' })); + schema = editor.schema; + basePlugins = editor.state.plugins; + }); + + afterEach(() => { + vi.restoreAllMocks(); + editor?.destroy(); + editor = null; + }); + + const createState = (doc) => + EditorState.create({ + schema, + doc, + plugins: basePlugins, + }); + + /** + * Create a ReplaceAroundStep that simulates ProseMirror's structural backspace. + * A ReplaceAroundStep replaces from..gapFrom and gapTo..to with a slice, + * preserving the content between gapFrom..gapTo (the "gap"). + * + * For backspace at a list/paragraph boundary, ProseMirror creates a step that + * lifts content out of its wrapper. We simulate this by wrapping the text + * in a listItem-like structure and creating a step that unwraps it. + */ + const createBackspaceReplaceAroundTr = (state, doc) => { + // Create a ReplaceAroundStep that covers the paragraph node. + // This simulates what ProseMirror generates when backspacing at a + // paragraph boundary to lift content out of its wrapper. + // + // We find the first paragraph and create a step that would "unwrap" it + // by replacing the paragraph's opening and closing tokens while preserving + // the content between them. + let paraStart = null; + let paraEnd = null; + doc.forEach((node, offset) => { + if (paraStart === null && node.type.name === 'paragraph') { + paraStart = offset; + paraEnd = offset + node.nodeSize; + } + }); + + if (paraStart === null) throw new Error('No paragraph found'); + + // Build a transaction with a ReplaceAroundStep. + // The step unwraps the paragraph: replaces the paragraph node but keeps its inline content. + // from=paraStart, to=paraEnd, gapFrom=paraStart+1, gapTo=paraEnd-1 + // This means: replace [paraStart..paraStart+1] (open tag) and [paraEnd-1..paraEnd] (close tag) + // with a new paragraph wrapper, keeping the content (the "gap") intact. + const tr = state.tr; + const step = new ReplaceAroundStep( + paraStart, + paraEnd, + paraStart + 1, + paraEnd - 1, + new Slice(Fragment.from(schema.nodes.paragraph.create()), 0, 0), + 1, + true, + ); + + if (!tr.maybeStep(step).failed) { + return tr; + } + + // Fallback: just create a simple structural step to test the handler + return null; + }; + + /** + * Helper to invoke replaceAroundStep handler directly. + * Sets up tr with a real step applied so tr.docs[0] is populated. + */ + const invokeHandler = ({ state, inputType = 'deleteContentBackward' }) => { + const doc = state.doc; + const trackDeleteMarkType = state.schema.marks[TrackDeleteMarkName]; + + // We need to supply a tr that has tr.docs populated (i.e., a step was applied to it). + // Apply a dummy ReplaceStep to the tr to populate tr.docs[0]. + const tr = state.tr; + + // Find cursor position and the char we'd delete + const cursorPos = state.selection.from; + let lastLiveCharPos = null; + + // Walk the paragraph to find the live char (same logic as findPreviousLiveCharPos) + const $cursor = doc.resolve(cursorPos); + let paraDepth = $cursor.depth; + while (paraDepth > 0 && $cursor.node(paraDepth).type.name !== 'paragraph') { + paraDepth--; + } + if (paraDepth > 0) { + const paraStart = $cursor.before(paraDepth) + 1; + doc.nodesBetween(paraStart, cursorPos, (node, pos) => { + if (!node.isText) return; + const hasDeleteMark = node.marks.some((m) => m.type === trackDeleteMarkType); + if (hasDeleteMark) return; + const nodeEnd = pos + node.nodeSize; + const relevantEnd = Math.min(nodeEnd, cursorPos); + if (relevantEnd > pos) lastLiveCharPos = relevantEnd - 1; + }); + } + + // Apply a ReplaceStep to tr so tr.docs gets populated + if (lastLiveCharPos !== null) { + tr.step(new ReplaceStep(lastLiveCharPos, lastLiveCharPos + 1, Slice.empty)); + } + tr.setMeta('inputType', inputType); + + const newTr = state.tr; + const map = new Mapping(); + const fakeStep = {}; + + replaceAroundStep({ + state, + tr, + step: fakeStep, + newTr, + map, + doc, + user, + date, + originalStep: fakeStep, + originalStepIndex: 0, + }); + + return newTr; + }; + + describe('non-backspace blocking', () => { + it('blocks non-backspace ReplaceAroundStep (no steps added to newTr)', () => { + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, schema.nodes.run.create({}, [schema.text('Hello')])), + ); + const state = createState(doc); + const cursorPos = findTextPos(state.doc, 'Hello') + 5; + const stateWithSelection = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state: stateWithSelection, inputType: 'insertParagraph' }); + + // No steps should be added — the structural change is blocked + expect(newTr.steps.length).toBe(0); + }); + + it('blocks ReplaceAroundStep without inputType meta', () => { + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, schema.nodes.run.create({}, [schema.text('Hello')])), + ); + const state = createState(doc); + + const tr = state.tr; + // No inputType meta set + const newTr = state.tr; + const map = new Mapping(); + + replaceAroundStep({ + state, + tr, + step: {}, + newTr, + map, + doc: state.doc, + user, + date, + originalStep: {}, + originalStepIndex: 0, + }); + + expect(newTr.steps.length).toBe(0); + }); + }); + + describe('backspace character deletion', () => { + it('converts backspace ReplaceAroundStep to tracked single-char deletion', () => { + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, schema.nodes.run.create({}, [schema.text('Hello')])), + ); + let state = createState(doc); + + // Position cursor after 'o' (end of "Hello") + const cursorPos = findTextPos(state.doc, 'Hello') + 5; + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state }); + + // Should have added steps (the tracked deletion) + expect(newTr.steps.length).toBeGreaterThan(0); + + // The last character 'o' should be marked as deleted + let hasDeleteMark = false; + newTr.doc.descendants((node) => { + if (node.isText && node.marks.some((m) => m.type.name === TrackDeleteMarkName)) { + hasDeleteMark = true; + } + }); + expect(hasDeleteMark).toBe(true); + }); + + it('sets selectionPos on track meta for cursor positioning', () => { + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, schema.nodes.run.create({}, [schema.text('Hello')])), + ); + let state = createState(doc); + + const cursorPos = findTextPos(state.doc, 'Hello') + 5; + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state }); + const trackMeta = newTr.getMeta(TrackChangesBasePluginKey); + + expect(trackMeta).toBeDefined(); + expect(trackMeta.selectionPos).toBeDefined(); + // selectionPos should be the position of the deleted character (before cursor) + expect(trackMeta.selectionPos).toBeLessThan(cursorPos); + }); + + it('returns early when no live character exists before cursor', () => { + // All content is tracked-deleted — nothing to delete + const deleteMark = schema.marks[TrackDeleteMarkName].create({ + id: 'del-existing', + author: user.name, + authorEmail: user.email, + date, + }); + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, schema.nodes.run.create({}, [schema.text('Deleted', [deleteMark])])), + ); + let state = createState(doc); + + const cursorPos = findTextPos(state.doc, 'Deleted') + 7; + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state }); + + // No steps — nothing to delete + expect(newTr.steps.length).toBe(0); + }); + }); + + describe('findPreviousLiveCharPos (tested through handler)', () => { + it('skips tracked-deleted text and deletes the last live character', () => { + // "Hel" (live) + "lo" (deleted by another author) — cursor at end + // Should delete 'l' (the last live char), not 'o' + const deleteMark = schema.marks[TrackDeleteMarkName].create({ + id: 'del-existing', + author: 'Other Author', + authorEmail: 'other@example.com', + date: '2023-01-01T00:00:00.000Z', + }); + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create( + {}, + schema.nodes.run.create({}, [schema.text('Hel'), schema.text('lo', [deleteMark])]), + ), + ); + let state = createState(doc); + + // Cursor after 'lo' (end of all text) + const hPos = findTextPos(state.doc, 'Hel'); + const cursorPos = hPos + 5; // After "Hello" = Hel + lo + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state }); + + // Should have tracked deletion + expect(newTr.steps.length).toBeGreaterThan(0); + + // 'l' from "Hel" should be the newly deleted character + let newlyDeletedText = ''; + newTr.doc.descendants((node) => { + if (!node.isText) return; + const mark = node.marks.find((m) => m.type.name === TrackDeleteMarkName); + if (mark && mark.attrs.id !== 'del-existing') { + newlyDeletedText += node.text; + } + }); + expect(newlyDeletedText).toBe('l'); + }); + }); + + describe('mark merging', () => { + it('merges adjacent trackDelete marks with same author/date but different IDs', () => { + // Simulate the scenario: "." was deleted previously (id A), now "l" is deleted (id B) + // They are in separate run nodes with a gap between them. + // After handler runs, both should have the same ID. + const existingDeleteMark = schema.marks[TrackDeleteMarkName].create({ + id: 'existing-period-id', + author: user.name, + authorEmail: user.email, + date, + }); + + // "Material" (live) in one run, "." (deleted, id=existing) in another run + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, [ + schema.nodes.run.create({}, [schema.text('Material')]), + schema.nodes.run.create({}, [schema.text('.', [existingDeleteMark])]), + ]), + ); + let state = createState(doc); + + // Cursor after 'l' in "Material" (position to delete 'l') + const textPos = findTextPos(state.doc, 'Material'); + const cursorPos = textPos + 8; // After "Material" + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state }); + const trackMeta = newTr.getMeta(TrackChangesBasePluginKey); + + // The handler should have created a deletion mark + expect(trackMeta?.deletionMark).toBeDefined(); + + // Check that the existing "." deletion was re-marked with the new ID + const newId = trackMeta.deletionMark.attrs.id; + let periodMarkId = null; + newTr.doc.descendants((node) => { + if (!node.isText || node.text !== '.') return; + const delMark = node.marks.find((m) => m.type.name === TrackDeleteMarkName); + if (delMark) periodMarkId = delMark.attrs.id; + }); + + // Both the 'l' deletion and the '.' deletion should now share the same ID + expect(periodMarkId).toBe(newId); + }); + + it('does not merge marks from different authors', () => { + const otherAuthorMark = schema.marks[TrackDeleteMarkName].create({ + id: 'other-author-del', + author: 'Other Author', + authorEmail: 'other@example.com', + date, + }); + + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, [ + schema.nodes.run.create({}, [schema.text('Material')]), + schema.nodes.run.create({}, [schema.text('.', [otherAuthorMark])]), + ]), + ); + let state = createState(doc); + + const textPos = findTextPos(state.doc, 'Material'); + const cursorPos = textPos + 8; + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state }); + + // The "." should keep the other author's ID (not merged) + let periodMarkId = null; + newTr.doc.descendants((node) => { + if (!node.isText || node.text !== '.') return; + const delMark = node.marks.find((m) => m.type.name === TrackDeleteMarkName); + if (delMark) periodMarkId = delMark.attrs.id; + }); + + expect(periodMarkId).toBe('other-author-del'); + }); + }); + + describe('selectionPos in trackedTransaction', () => { + it('selectionPos is honored even when tr.selectionSet is false', () => { + // This tests the critical fix in trackedTransaction.js: + // ReplaceAroundStep transactions may not set selection on the original tr, + // but our handler sets selectionPos on the meta. trackedTransaction must + // check selectionPos BEFORE checking tr.selectionSet. + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, schema.nodes.run.create({}, [schema.text('Hello')])), + ); + let state = createState(doc); + + const cursorPos = findTextPos(state.doc, 'Hello') + 5; + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + // Create a transaction with a ReplaceStep but do NOT set selection on tr, + // simulating the ReplaceAroundStep behavior where tr.selectionSet is false. + const deletePos = cursorPos - 1; + const tr = state.tr; + tr.step(new ReplaceStep(deletePos, cursorPos, Slice.empty)); + tr.setMeta('inputType', 'deleteContentBackward'); + // Critically: do NOT call tr.setSelection — this is what happens with ReplaceAroundStep + + // Set selectionPos meta to simulate what replaceAroundStep handler does + tr.setMeta(TrackChangesBasePluginKey, { selectionPos: deletePos }); + + // Now run through trackedTransaction + const tracked = trackedTransaction({ tr, state, user }); + const finalState = state.apply(tracked); + + // The selection should be at or near deletePos, not at the original cursor position + expect(finalState.selection.from).toBeLessThanOrEqual(cursorPos); + }); + }); +}); diff --git a/tests/behavior/tests/comments/delete-terminal-period-separate-run.spec.ts b/tests/behavior/tests/comments/delete-terminal-period-separate-run.spec.ts index c458ae9f02..f32b214111 100644 --- a/tests/behavior/tests/comments/delete-terminal-period-separate-run.spec.ts +++ b/tests/behavior/tests/comments/delete-terminal-period-separate-run.spec.ts @@ -102,4 +102,26 @@ test('two backspaces track period and l for bookmark-wrapped runs', async ({ sup expect(newDeletedCombined).toBe('l.'); expect(snapshot.bookmarkStartCount).toBe(before.bookmarkStartCount); expect(snapshot.bookmarkEndCount).toBe(before.bookmarkEndCount); + + // SD-2061: Verify numbered list marker is preserved after backspace. + // Before the fix, ReplaceAroundStep was applied untracked, which removed + // paragraph properties (numbering, font, alignment). + const markerAfter = await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let marker: string | null = null; + + doc.descendants((node: any) => { + if (node.type?.name !== 'paragraph') return; + const normalized = String(node.textContent ?? '') + .replace(/\s+/g, ' ') + .trim(); + if (!normalized.includes('any and all such Confidential Material')) return; + marker = String(node.attrs?.listRendering?.markerText ?? '').trim(); + return false; + }); + + return marker; + }); + expect(markerAfter).toBe('1.'); }); From c9575c6c57b2d11b5e0bda358870dd48e6b57113 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Sat, 28 Feb 2026 16:59:35 -0300 Subject: [PATCH 3/4] fix(track-changes): only merge contiguous trackDelete marks Stop the merge loop at the first live (non-deleted) text node so non-adjacent deletions within the search window are not incorrectly collapsed into a single tracked change. --- .../trackChangesHelpers/replaceAroundStep.js | 13 ++--- .../replaceAroundStep.test.js | 51 +++++++++++++++++++ 2 files changed, 58 insertions(+), 6 deletions(-) diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js index f4869c1fdb..5f11e1fa3f 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js @@ -138,15 +138,16 @@ export const replaceAroundStep = ({ const ourDate = trackMeta.deletionMark.attrs.date; const searchTo = Math.min(newTr.doc.content.size, deleteFrom + 20); + let contiguous = true; newTr.doc.nodesBetween(deleteFrom, searchTo, (node, pos) => { + if (!contiguous) return false; if (!node.isText) return; const delMark = node.marks.find((m) => m.type.name === TrackDeleteMarkName); - if ( - delMark && - delMark.attrs.id !== ourId && - delMark.attrs.authorEmail === ourEmail && - delMark.attrs.date === ourDate - ) { + if (!delMark) { + contiguous = false; // Live text — stop, deletions are no longer contiguous. + return; + } + if (delMark.attrs.id !== ourId && delMark.attrs.authorEmail === ourEmail && delMark.attrs.date === ourDate) { const markType = state.schema.marks[TrackDeleteMarkName]; const merged = markType.create({ ...delMark.attrs, id: ourId }); newTr.removeMark(pos, pos + node.nodeSize, delMark); diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js index 1b5dd7a2f1..5764beddea 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js @@ -384,6 +384,57 @@ describe('replaceAroundStep handler', () => { expect(periodMarkId).toBe('other-author-del'); }); + + it('does not merge non-contiguous marks separated by live text', () => { + // "Material" (live) + "." (deleted, same author) + "end" (live) + "!" (deleted, same author) + // Only "." should be merged with the new deletion; "!" should keep its own ID + // because "end" (live text) breaks contiguity. + const deleteMark1 = schema.marks[TrackDeleteMarkName].create({ + id: 'period-del', + author: user.name, + authorEmail: user.email, + date, + }); + const deleteMark2 = schema.marks[TrackDeleteMarkName].create({ + id: 'excl-del', + author: user.name, + authorEmail: user.email, + date, + }); + + const doc = schema.nodes.doc.create( + {}, + schema.nodes.paragraph.create({}, [ + schema.nodes.run.create({}, [schema.text('Material')]), + schema.nodes.run.create({}, [schema.text('.', [deleteMark1])]), + schema.nodes.run.create({}, [schema.text('end')]), + schema.nodes.run.create({}, [schema.text('!', [deleteMark2])]), + ]), + ); + let state = createState(doc); + + const textPos = findTextPos(state.doc, 'Material'); + const cursorPos = textPos + 8; + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, cursorPos))); + + const newTr = invokeHandler({ state }); + const trackMeta = newTr.getMeta(TrackChangesBasePluginKey); + const newId = trackMeta?.deletionMark?.attrs?.id; + + // "." should be merged (same ID as new deletion) + let periodMarkId = null; + let exclMarkId = null; + newTr.doc.descendants((node) => { + if (!node.isText) return; + const delMark = node.marks.find((m) => m.type.name === TrackDeleteMarkName); + if (!delMark) return; + if (node.text === '.') periodMarkId = delMark.attrs.id; + if (node.text === '!') exclMarkId = delMark.attrs.id; + }); + + expect(periodMarkId).toBe(newId); // Adjacent — merged + expect(exclMarkId).toBe('excl-del'); // Non-contiguous — NOT merged + }); }); describe('selectionPos in trackedTransaction', () => { From ec969e74d7ea7340f17847bf01b8ff9f3f4f3515 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Sun, 1 Mar 2026 07:22:36 -0300 Subject: [PATCH 4/4] fix(tests): reduce webkit flakiness in comment highlight assertions Increase default assertCommentHighlightExists timeout from 15s to 20s to match what most callers already pass explicitly. Consolidate 4 sequential post-italic assertions into a single composite poll to avoid multiplicative timeouts in webkit's slower rendering pipeline. --- tests/behavior/fixtures/superdoc.ts | 2 +- .../decoration-survives-mark-change.spec.ts | 35 +++++++++++++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/tests/behavior/fixtures/superdoc.ts b/tests/behavior/fixtures/superdoc.ts index 381019d341..64296db09e 100644 --- a/tests/behavior/fixtures/superdoc.ts +++ b/tests/behavior/fixtures/superdoc.ts @@ -648,7 +648,7 @@ function createFixture(page: Page, editor: Locator, modKey: string) { async assertCommentHighlightExists(opts?: { text?: string; commentId?: string; timeoutMs?: number }) { const expectedText = opts?.text; const expectedCommentId = opts?.commentId; - const timeoutMs = opts?.timeoutMs ?? 15_000; + const timeoutMs = opts?.timeoutMs ?? 20_000; await expect .poll( () => diff --git a/tests/behavior/tests/formatting/decoration-survives-mark-change.spec.ts b/tests/behavior/tests/formatting/decoration-survives-mark-change.spec.ts index 4c17c44a2b..ccc8fb2001 100644 --- a/tests/behavior/tests/formatting/decoration-survives-mark-change.spec.ts +++ b/tests/behavior/tests/formatting/decoration-survives-mark-change.spec.ts @@ -74,13 +74,34 @@ test.describe('comment highlight survives mark changes', () => { await superdoc.waitForStable(); // Comment highlight must still exist — after the run split the highlight may - // span multiple elements, so check by commentId rather than full text - await superdoc.assertCommentHighlightExists({ commentId }); - - // Each part of the range should still carry the highlight - await superdoc.assertCommentHighlightExists({ text: 'quick' }); - await superdoc.assertCommentHighlightExists({ text: 'brown' }); - await superdoc.assertCommentHighlightExists({ text: 'fox' }); + // span multiple elements, so check by commentId AND all text segments in a + // single poll to avoid multiplicative timeouts in slower engines (webkit). + await expect + .poll( + () => + superdoc.page.evaluate( + ({ cId }) => { + const normalize = (v: string) => v.replace(/\s+/g, ' ').trim(); + const highlights = Array.from(document.querySelectorAll('.superdoc-comment-highlight')); + if (highlights.length === 0) return 'no highlights'; + const hasId = highlights.some((el) => + (el.getAttribute('data-comment-ids') ?? '') + .split(/[\s,]+/) + .filter(Boolean) + .includes(cId), + ); + if (!hasId) return 'missing commentId'; + const texts = highlights.map((el) => normalize(el.textContent ?? '')); + for (const word of ['quick', 'brown', 'fox']) { + if (!texts.some((t) => t.includes(word))) return `missing "${word}"`; + } + return 'ok'; + }, + { cId: commentId }, + ), + { timeout: 25_000 }, + ) + .toBe('ok'); // Italic applied to "brown" await superdoc.assertTextHasMarks('brown', ['italic']);