From a09d2a662e65bc740506c9b0cdc58d71646c7707 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Wed, 11 Mar 2026 08:03:19 -0300 Subject: [PATCH 1/2] fix(track-changes): cancel tracked format changes when reverted to original (SD-2181) When a format change (e.g., superscript) is applied in track changes mode and then reverted (e.g., back to baseline), the tracked change now cancels out instead of persisting as two separate changes with zero net effect. - Filter reverted entries from the `after` array in the `foundBefore` branch - Add `isTrackFormatNoOp` check to detect and remove no-op format changes - Handle identity attribute values (vertAlign: baseline, position: 0pt) - Compose `normalizeSnapshotAttrs` on `normalizeAttrs` to avoid duplication - Export and reuse `getTypeName` helper across modules --- .../trackChangesHelpers/addMarkStep.js | 21 ++++- .../markSnapshotHelpers.js | 63 ++++++++++++- .../markSnapshotHelpers.test.js | 64 +++++++++++++ .../trackChangesHelpers.test.js | 90 +++++++++++++++++++ 4 files changed, 232 insertions(+), 6 deletions(-) diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js index 2b56b4e2ef..b2f115f08f 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js @@ -2,7 +2,13 @@ import { TrackDeleteMarkName, TrackFormatMarkName } from '../constants.js'; import { v4 as uuidv4 } from 'uuid'; import { TrackChangesBasePluginKey } from '../plugins/trackChangesBasePlugin.js'; import { CommentsPluginKey } from '../../comment/comments-plugin.js'; -import { hasMatchingMark, markSnapshotMatchesStepMark, upsertMarkSnapshotByType } from './markSnapshotHelpers.js'; +import { + hasMatchingMark, + markSnapshotMatchesStepMark, + upsertMarkSnapshotByType, + isTrackFormatNoOp, + getTypeName, +} from './markSnapshotHelpers.js'; import { getLiveInlineMarksInRange } from './getLiveInlineMarksInRange.js'; /** @@ -61,7 +67,9 @@ export const addMarkStep = ({ state, step, newTr, doc, user, date }) => { before = [ ...formatChangeMark.attrs.before.filter((mark) => !markSnapshotMatchesStepMark(mark, step.mark, true)), ]; - after = [...formatChangeMark.attrs.after]; + // The step restores the original mark for this type — remove the + // corresponding "after" entry since the change has been reverted. + after = formatChangeMark.attrs.after.filter((mark) => getTypeName(mark) !== step.mark.type.name); } else { before = [...formatChangeMark.attrs.before]; after = upsertMarkSnapshotByType(formatChangeMark.attrs.after, { @@ -87,6 +95,15 @@ export const addMarkStep = ({ state, step, newTr, doc, user, date }) => { ]; } + // Check if the format change is effectively a no-op (e.g., reverting + // vertAlign to 'baseline' when the original had no vertAlign). + if (isTrackFormatNoOp(before, after)) { + if (formatChangeMark) { + newTr.removeMark(Math.max(step.from, pos), Math.min(step.to, pos + node.nodeSize), formatChangeMark); + } + return; + } + if (after.length || before.length) { const newFormatMark = state.schema.marks[TrackFormatMarkName].create({ id: wid, diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js index 379d9fe142..f1a14be124 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js @@ -4,16 +4,71 @@ const normalizeAttrs = (attrs = {}) => { return Object.fromEntries(Object.entries(attrs).filter(([, value]) => value !== null && value !== undefined)); }; +/** + * Attribute values that are semantically equivalent to "not set" for tracking purposes. + * These represent the default visual state and should not count as a change. + */ +const IDENTITY_ATTR_VALUES = { + vertAlign: 'baseline', + position: '0pt', +}; + +/** + * Mark types where the mark's effect is determined entirely by its attributes. + * An entry with empty normalized attrs means the mark has no visual effect. + * In contrast, structural marks (bold, italic) have their effect from being present. + */ +const ATTRIBUTE_ONLY_MARKS = ['textStyle']; + +/** + * Normalize snapshot attrs for tracked change comparison. + * Strips null/undefined AND identity values that represent the default visual state. + */ +const normalizeSnapshotAttrs = (attrs = {}) => { + const base = normalizeAttrs(attrs); + return Object.fromEntries(Object.entries(base).filter(([key, value]) => IDENTITY_ATTR_VALUES[key] !== value)); +}; + +export const getTypeName = (markLike) => { + return markLike?.type?.name ?? markLike?.type; +}; + +/** + * Check if a tracked format change is effectively a no-op. + * Compares before and after snapshots after normalizing identity attribute values. + * A no-op means the format change has no net visual effect. + */ +export const isTrackFormatNoOp = (before, after) => { + const normalize = (entries) => + entries + .map((s) => ({ + type: getTypeName(s), + attrs: normalizeSnapshotAttrs(s.attrs || {}), + })) + .filter((s) => { + // For attribute-only marks (e.g. textStyle), empty attrs = no visual effect → filter out + if (ATTRIBUTE_ONLY_MARKS.includes(s.type) && Object.keys(s.attrs).length === 0) return false; + return true; + }); + + const normBefore = normalize(before); + const normAfter = normalize(after); + + if (normBefore.length === 0 && normAfter.length === 0) return true; + if (normBefore.length !== normAfter.length) return false; + + return ( + normBefore.every((b) => normAfter.some((a) => a.type === b.type && isEqual(a.attrs, b.attrs))) && + normAfter.every((a) => normBefore.some((b) => b.type === a.type && isEqual(b.attrs, a.attrs))) + ); +}; + export const attrsExactlyMatch = (left = {}, right = {}) => { const normalizedLeft = normalizeAttrs(left); const normalizedRight = normalizeAttrs(right); return isEqual(normalizedLeft, normalizedRight); }; -const getTypeName = (markLike) => { - return markLike?.type?.name ?? markLike?.type; -}; - const marksMatch = (left, right, exact = true) => { if (!left || !right || getTypeName(left) !== getTypeName(right)) { return false; diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.test.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.test.js index 0a206c6c75..5eba583d3b 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.test.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.test.js @@ -7,6 +7,7 @@ import { hasMatchingMark, upsertMarkSnapshotByType, findMarkInRangeBySnapshot, + isTrackFormatNoOp, } from './markSnapshotHelpers.js'; describe('markSnapshotHelpers', () => { @@ -140,6 +141,69 @@ describe('markSnapshotHelpers', () => { expect(match).toBeNull(); }); + describe('isTrackFormatNoOp', () => { + it('returns true when both before and after are empty', () => { + expect(isTrackFormatNoOp([], [])).toBe(true); + }); + + it('returns true when after has only textStyle with vertAlign baseline (identity value)', () => { + // Scenario: text had no textStyle, user added superscript then reverted to baseline + expect(isTrackFormatNoOp([], [{ type: 'textStyle', attrs: { vertAlign: 'baseline' } }])).toBe(true); + }); + + it('returns true when before and after differ only by vertAlign baseline', () => { + // Scenario: text had textStyle with fontSize, user added superscript then reverted + expect( + isTrackFormatNoOp( + [{ type: 'textStyle', attrs: { fontSize: '24pt' } }], + [{ type: 'textStyle', attrs: { fontSize: '24pt', vertAlign: 'baseline' } }], + ), + ).toBe(true); + }); + + it('returns false for a real format change', () => { + expect( + isTrackFormatNoOp( + [{ type: 'textStyle', attrs: { fontSize: '12pt' } }], + [{ type: 'textStyle', attrs: { fontSize: '24pt' } }], + ), + ).toBe(false); + }); + + it('returns false when bold is added (structural mark)', () => { + expect(isTrackFormatNoOp([], [{ type: 'bold', attrs: {} }])).toBe(false); + }); + + it('returns false when bold is removed (structural mark)', () => { + expect(isTrackFormatNoOp([{ type: 'bold', attrs: {} }], [])).toBe(false); + }); + + it('returns true when textStyle with only null attrs is in after', () => { + expect(isTrackFormatNoOp([], [{ type: 'textStyle', attrs: { vertAlign: null, position: null } }])).toBe(true); + }); + + it('returns false when non-identity textStyle change exists alongside baseline revert', () => { + // Bold was also changed — not a no-op + expect( + isTrackFormatNoOp([{ type: 'bold', attrs: {} }], [{ type: 'textStyle', attrs: { vertAlign: 'baseline' } }]), + ).toBe(false); + }); + + it('returns true when position is reverted to 0pt (identity value)', () => { + expect(isTrackFormatNoOp([], [{ type: 'textStyle', attrs: { position: '0pt' } }])).toBe(true); + }); + + it('returns true when vertAlign superscript matches in both before and after', () => { + // Both say the same thing — no net change + expect( + isTrackFormatNoOp( + [{ type: 'textStyle', attrs: { vertAlign: 'superscript' } }], + [{ type: 'textStyle', attrs: { vertAlign: 'superscript' } }], + ), + ).toBe(true); + }); + }); + it('findMarkInRangeBySnapshot falls back to subset attr match for sparse snapshots', () => { const richTextStyle = schema.marks.textStyle.create({ styleId: 'Emphasis', 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 437bffb45a..8ea87ee294 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 @@ -446,6 +446,96 @@ describe('trackChangesHelpers', () => { expect(meta?.formatMark?.attrs?.after).toEqual([{ type: 'textStyle', attrs: changedTextStyle.attrs }]); }); + it('addMarkStep removes trackFormat when reverting to original state (SD-2181)', () => { + // Step 1: Plain text, apply superscript → creates trackFormat + const state = createState(createDocWithText('Hello')); + const superscriptMark = schema.marks.textStyle.create({ vertAlign: 'superscript' }); + const step1 = new AddMarkStep(2, 6, superscriptMark); + const newTr1 = state.tr; + + addMarkStep({ + state, + step: step1, + newTr: newTr1, + doc: state.doc, + user, + date, + }); + + const meta1 = newTr1.getMeta(TrackChangesBasePluginKey); + expect(meta1?.formatMark?.type.name).toBe(TrackFormatMarkName); + + // Step 2: Apply baseline (revert) on the tracked state + const state2 = state.apply(newTr1); + const baselineMark = schema.marks.textStyle.create({ vertAlign: 'baseline' }); + const step2 = new AddMarkStep(2, 6, baselineMark); + const newTr2 = state2.tr; + + addMarkStep({ + state: state2, + step: step2, + newTr: newTr2, + doc: state2.doc, + user, + date, + }); + + // The trackFormat mark should be removed (no-op), no metadata set + const meta2 = newTr2.getMeta(TrackChangesBasePluginKey); + expect(meta2).toBeUndefined(); + + // Verify no trackFormat mark remains in the document + const finalState = state2.apply(newTr2); + let hasTrackFormat = false; + finalState.doc.descendants((node) => { + if (node.marks?.some((m) => m.type.name === TrackFormatMarkName)) { + hasTrackFormat = true; + } + }); + expect(hasTrackFormat).toBe(false); + }); + + it('addMarkStep preserves other tracked types when partially reverting (SD-2181)', () => { + // Step 1: Apply bold on plain text → creates trackFormat with after: [bold] + const state = createState(createDocWithText('Hello')); + const boldMark = schema.marks.bold.create(); + const step1 = new AddMarkStep(2, 6, boldMark); + const newTr1 = state.tr; + + addMarkStep({ + state, + step: step1, + newTr: newTr1, + doc: state.doc, + user, + date, + }); + + const state2 = state.apply(newTr1); + + // Step 2: Also change textStyle color → trackFormat now has after: [bold, textStyle] + const colorMark = schema.marks.textStyle.create({ color: '#FF0000' }); + const step2 = new AddMarkStep(2, 6, colorMark); + const newTr2 = state2.tr; + + addMarkStep({ + state: state2, + step: step2, + newTr: newTr2, + doc: state2.doc, + user, + date, + }); + + const meta2 = newTr2.getMeta(TrackChangesBasePluginKey); + expect(meta2?.formatMark?.attrs?.after).toEqual( + expect.arrayContaining([ + expect.objectContaining({ type: 'bold' }), + expect.objectContaining({ type: 'textStyle' }), + ]), + ); + }); + it('removeMarkStep records previous formatting when mark removed', () => { const bold = schema.marks.bold.create(); const doc = createDocWithText('Styled', [bold]); From e190ec3cb10af15ff4722392bb6a6c98b101fec7 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Wed, 11 Mar 2026 08:35:03 -0300 Subject: [PATCH 2/2] test(track-changes): add behavior tests for format revert cancellation (SD-2181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three Playwright tests verifying that tracked format changes cancel out when reverted: superscript→baseline, color change→revert, bold on→off. --- ...rmat-revert-cancels-tracked-change.spec.ts | 181 ++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 tests/behavior/tests/comments/sd-2181-format-revert-cancels-tracked-change.spec.ts diff --git a/tests/behavior/tests/comments/sd-2181-format-revert-cancels-tracked-change.spec.ts b/tests/behavior/tests/comments/sd-2181-format-revert-cancels-tracked-change.spec.ts new file mode 100644 index 0000000000..9d3ba5726e --- /dev/null +++ b/tests/behavior/tests/comments/sd-2181-format-revert-cancels-tracked-change.spec.ts @@ -0,0 +1,181 @@ +import { test, expect } from '../../fixtures/superdoc.js'; + +test.use({ config: { toolbar: 'full', comments: 'panel', trackChanges: true } }); + +/** + * SD-2181: Tracked format changes should cancel out when reverted to original. + * + * When a format change (e.g., superscript) is applied in track changes mode + * and then reverted (e.g., baseline), the two changes should cancel out + * instead of leaving ghost TrackFormat marks. + */ + +test('superscript then baseline revert cancels tracked format change', async ({ superdoc }) => { + await superdoc.type('Hello world'); + await superdoc.waitForStable(); + + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + + // Select "world" and apply superscript + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let from = 0; + let to = 0; + doc.descendants((node: any, pos: number) => { + if (node.isText && node.text?.includes('world')) { + const offset = node.text.indexOf('world'); + from = pos + offset; + to = from + 'world'.length; + } + }); + editor.commands.setTextSelection({ from, to }); + editor.commands.setMark('textStyle', { vertAlign: 'superscript' }); + }); + await superdoc.waitForStable(); + + // Verify tracked change was created + await superdoc.assertTrackedChangeExists('format'); + + // Revert to baseline on the same selection + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let from = 0; + let to = 0; + doc.descendants((node: any, pos: number) => { + if (node.isText && node.text?.includes('world')) { + const offset = node.text.indexOf('world'); + from = pos + offset; + to = from + 'world'.length; + } + }); + editor.commands.setTextSelection({ from, to }); + editor.commands.setMark('textStyle', { vertAlign: 'baseline' }); + }); + await superdoc.waitForStable(); + + // No tracked format changes should remain — the revert cancels out + await expect(superdoc.page.locator('.track-format-dec')).toHaveCount(0); + + // Text should be unchanged + await superdoc.assertTextContent('Hello world'); +}); + +test('color change then revert cancels tracked format change', async ({ superdoc }) => { + // Type text and set initial color in editing mode + await superdoc.type('Hello world'); + await superdoc.waitForStable(); + + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let from = 0; + let to = 0; + doc.descendants((node: any, pos: number) => { + if (node.isText && node.text?.includes('world')) { + const offset = node.text.indexOf('world'); + from = pos + offset; + to = from + 'world'.length; + } + }); + editor.commands.setTextSelection({ from, to }); + editor.commands.setColor('#112233'); + }); + await superdoc.waitForStable(); + + // Switch to suggesting mode + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + + // Change color + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let from = 0; + let to = 0; + doc.descendants((node: any, pos: number) => { + if (node.isText && node.text?.includes('world')) { + const offset = node.text.indexOf('world'); + from = pos + offset; + to = from + 'world'.length; + } + }); + editor.commands.setTextSelection({ from, to }); + editor.commands.setColor('#FF0000'); + }); + await superdoc.waitForStable(); + + await superdoc.assertTrackedChangeExists('format'); + + // Revert to original color + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let from = 0; + let to = 0; + doc.descendants((node: any, pos: number) => { + if (node.isText && node.text?.includes('world')) { + const offset = node.text.indexOf('world'); + from = pos + offset; + to = from + 'world'.length; + } + }); + editor.commands.setTextSelection({ from, to }); + editor.commands.setColor('#112233'); + }); + await superdoc.waitForStable(); + + // Tracked change should be gone — color is back to original + await expect(superdoc.page.locator('.track-format-dec')).toHaveCount(0); +}); + +test('bold on then off cancels tracked format change on single word', async ({ superdoc }) => { + await superdoc.type('Hello world'); + await superdoc.waitForStable(); + + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + + // Select "world" and toggle bold on + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let from = 0; + let to = 0; + doc.descendants((node: any, pos: number) => { + if (node.isText && node.text?.includes('world')) { + const offset = node.text.indexOf('world'); + from = pos + offset; + to = from + 'world'.length; + } + }); + editor.commands.setTextSelection({ from, to }); + editor.commands.toggleBold(); + }); + await superdoc.waitForStable(); + + await superdoc.assertTrackedChangeExists('format'); + + // Toggle bold off (revert) + await superdoc.page.evaluate(() => { + const editor = (window as any).editor; + const { doc } = editor.state; + let from = 0; + let to = 0; + doc.descendants((node: any, pos: number) => { + if (node.isText && node.text?.includes('world')) { + const offset = node.text.indexOf('world'); + from = pos + offset; + to = from + 'world'.length; + } + }); + editor.commands.setTextSelection({ from, to }); + editor.commands.toggleBold(); + }); + await superdoc.waitForStable(); + + await expect(superdoc.page.locator('.track-format-dec')).toHaveCount(0); + await superdoc.assertTextLacksMarks('world', ['bold']); +});