Skip to content

Commit 6400a1f

Browse files
authored
fix(track-changes): allow linked style changes in suggesting mode (SD-2182) (#2373)
* fix(track-changes): allow linked style changes in suggesting mode (SD-2182) Heading/paragraph style changes were silently blocked in suggesting mode. tr.setNodeMarkup() produces a ReplaceAroundStep which was being dropped by the track changes handler. This detects node markup changes (same structure, different attrs) and allows them through. Also removes the empty selection guard from toggleLinkedStyle so keyboard shortcuts work. Note: style changes are applied directly without tracked change marks. Tracked change decoration for paragraph styles is a follow-up. * fix(track-changes): tighten setNodeMarkup heuristic and fix toggleLinkedStyle cursor handling - Add step.insert === 1 check to exclude lift() false positives - Add map.appendMap() for consistency with other step handlers - Fix nodeAt() null-return by always falling back to findParentNodeClosestToPos - Update stale test to reflect new cursor selection behavior * test(track-changes): add tests for linked style changes in suggesting mode (SD-2182) - Unit: toggleLinkedStyle toggle-off with cursor selection - Unit: isNodeMarkupChange passthrough, lift blocking, map.appendMap - Behavior: heading style apply/toggle in suggesting mode * refactor(track-changes): improve comment accuracy and deduplicate test helpers - Update replaceAroundStep.js comment to note that the isNodeMarkupChange heuristic matches both setNodeMarkup and setBlockType (indistinguishable at the step level), with reference to SD-2191 follow-up - Extract findFirstParagraphRange() into testUtils.js, replacing 4 duplicated paragraph-finding loops in replaceAroundStep.test.js - Extract getFirstParagraphStyleId() helper in behavior test, replacing 3 duplicated page.evaluate blocks * fix(test): update toggleHeading test for cursor selection behavior toggleHeading uses toggleLinkedStyle, which now supports cursor selections. Update the test to expect true (style applied) instead of false (rejected).
1 parent 1b1e712 commit 6400a1f

7 files changed

Lines changed: 279 additions & 23 deletions

File tree

packages/super-editor/src/extensions/heading/heading.test.js

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,12 @@ describe('Heading Extension', () => {
5656
});
5757

5858
describe('toggleHeading', () => {
59-
it('should return false for an empty selection', () => {
59+
it('should apply heading with a cursor (empty) selection', () => {
6060
tr.setSelection(TextSelection.create(tr.doc, 1)); // Cursor selection
6161
const result = editor.commands.toggleHeading({ level: 1 });
6262

63-
expect(result).toBe(false);
64-
const styleId = editor.state.doc.content.content[0].attrs.paragraphProperties?.styleId ?? null;
65-
expect(styleId).toBeNull();
63+
expect(result).toBe(true);
64+
expect(editor.state.doc.content.content[0].attrs.paragraphProperties?.styleId).toBe('Heading1');
6665
});
6766

6867
it('should toggle heading on for a paragraph', () => {

packages/super-editor/src/extensions/linked-styles/linked-styles.js

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,17 +61,14 @@ export const LinkedStyles = Extension.create({
6161
* @example
6262
* const style = editor.helpers.linkedStyles.getStyleById('Heading1');
6363
* editor.commands.toggleLinkedStyle(style)
64-
* @note If selection is empty, returns false
64+
* @note Works with both cursor position and text selection
6565
* @note Removes style if already applied, applies it if not
6666
*/
6767
toggleLinkedStyle: (style) => (params) => {
6868
const { tr } = params;
69-
if (tr.selection.empty) {
70-
return false;
71-
}
7269
let node = tr.doc.nodeAt(tr.selection.$from.pos);
7370

74-
if (node && node.type.name !== 'paragraph') {
71+
if (!node || node.type.name !== 'paragraph') {
7572
node = findParentNodeClosestToPos(tr.selection.$from, (n) => {
7673
return n.type.name === 'paragraph';
7774
})?.node;

packages/super-editor/src/extensions/linked-styles/linked-styles.test.js

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -135,13 +135,29 @@ describe('LinkedStyles Extension', () => {
135135
});
136136

137137
describe('toggleLinkedStyle', () => {
138-
it('should return false for an empty selection', () => {
138+
it('should apply style with a cursor (empty) selection', () => {
139139
setParagraphCursor(editor.view, 0); // Cursor selection at first paragraph
140140
const result = editor.commands.toggleLinkedStyle(headingStyle, 'paragraph');
141141

142-
expect(result).toBe(false);
142+
expect(result).toBe(true);
143143
const firstParagraph = findParagraphInfo(editor.state.doc, 0);
144-
expect(getParagraphProps(firstParagraph.node).styleId).toBeUndefined();
144+
expect(getParagraphProps(firstParagraph.node).styleId).toBe('Heading1');
145+
});
146+
147+
it('should toggle off style with a cursor (empty) selection', () => {
148+
// Apply style first
149+
setParagraphCursor(editor.view, 0);
150+
editor.commands.setLinkedStyle(headingStyle);
151+
let firstParagraph = findParagraphInfo(editor.state.doc, 0);
152+
expect(getParagraphProps(firstParagraph.node).styleId).toBe('Heading1');
153+
154+
// Toggle off with cursor
155+
setParagraphCursor(editor.view, 0);
156+
const result = editor.commands.toggleLinkedStyle(headingStyle, 'paragraph');
157+
158+
expect(result).toBe(true);
159+
firstParagraph = findParagraphInfo(editor.state.doc, 0);
160+
expect(getParagraphProps(firstParagraph.node).styleId).toBe(null);
145161
});
146162

147163
it('should apply style when no style is currently set', () => {

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,23 @@ export const replaceAroundStep = ({
9494
return;
9595
}
9696

97+
// Detect node-markup-change steps (setNodeMarkup and setBlockType both
98+
// produce this same ReplaceAroundStep shape — they can't be distinguished
99+
// at the step level). Used here to let paragraph style changes through in
100+
// suggesting mode (e.g. Normal → Heading1 via setNodeMarkup).
101+
// step.insert === 1 excludes lift() operations (insert === 0).
102+
// Note: setBlockType is not triggered via UI in suggesting mode, but if
103+
// it were, it would also bypass tracking. SD-2191 will add proper tracked
104+
// change marks for these operations.
105+
const isNodeMarkupChange =
106+
step.structure && step.insert === 1 && step.gapFrom === step.from + 1 && step.gapTo === step.to - 1;
107+
108+
if (isNodeMarkupChange) {
109+
newTr.step(step);
110+
map.appendMap(step.getMap());
111+
return;
112+
}
113+
97114
const inputType = tr.getMeta('inputType');
98115
const isBackspace = inputType === 'deleteContentBackward';
99116

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js

Lines changed: 128 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { replaceAroundStep } from './replaceAroundStep.js';
77
import { TrackDeleteMarkName, TrackInsertMarkName } from '../constants.js';
88
import { TrackChangesBasePluginKey } from '../plugins/trackChangesBasePlugin.js';
99
import { initTestEditor } from '@tests/helpers/helpers.js';
10-
import { findTextPos } from './testUtils.js';
10+
import { findTextPos, findFirstParagraphRange } from './testUtils.js';
1111

1212
describe('replaceAroundStep handler', () => {
1313
let editor;
@@ -53,16 +53,7 @@ describe('replaceAroundStep handler', () => {
5353
// We find the first paragraph and create a step that would "unwrap" it
5454
// by replacing the paragraph's opening and closing tokens while preserving
5555
// the content between them.
56-
let paraStart = null;
57-
let paraEnd = null;
58-
doc.forEach((node, offset) => {
59-
if (paraStart === null && node.type.name === 'paragraph') {
60-
paraStart = offset;
61-
paraEnd = offset + node.nodeSize;
62-
}
63-
});
64-
65-
if (paraStart === null) throw new Error('No paragraph found');
56+
const { paraStart, paraEnd } = findFirstParagraphRange(doc);
6657

6758
// Build a transaction with a ReplaceAroundStep.
6859
// The step unwraps the paragraph: replaces the paragraph node but keeps its inline content.
@@ -148,6 +139,132 @@ describe('replaceAroundStep handler', () => {
148139
return newTr;
149140
};
150141

142+
describe('isNodeMarkupChange detection', () => {
143+
it('allows setNodeMarkup-style steps through (structure=true, insert=1, gap=±1)', () => {
144+
const doc = schema.nodes.doc.create(
145+
{},
146+
schema.nodes.paragraph.create(
147+
{ paragraphProperties: { styleId: 'Normal' } },
148+
schema.nodes.run.create({}, [schema.text('Hello')]),
149+
),
150+
);
151+
const state = createState(doc);
152+
153+
const { paraStart, paraEnd } = findFirstParagraphRange(state.doc);
154+
155+
const newParagraph = schema.nodes.paragraph.create({ paragraphProperties: { styleId: 'Heading1' } });
156+
const step = new ReplaceAroundStep(
157+
paraStart,
158+
paraEnd,
159+
paraStart + 1,
160+
paraEnd - 1,
161+
new Slice(Fragment.from(newParagraph), 0, 0),
162+
1,
163+
true,
164+
);
165+
166+
const tr = state.tr;
167+
tr.setMeta('inputType', 'insertParagraph'); // non-backspace — would normally be blocked
168+
const newTr = state.tr;
169+
const map = new Mapping();
170+
171+
replaceAroundStep({
172+
state,
173+
tr,
174+
step,
175+
newTr,
176+
map,
177+
doc: state.doc,
178+
user,
179+
date,
180+
originalStep: step,
181+
originalStepIndex: 0,
182+
});
183+
184+
// The step should be applied directly (not blocked)
185+
expect(newTr.steps.length).toBe(1);
186+
expect(newTr.steps[0]).toBe(step);
187+
});
188+
189+
it('blocks lift-style steps (structure=true, insert=0, gap=±1)', () => {
190+
const doc = schema.nodes.doc.create(
191+
{},
192+
schema.nodes.paragraph.create({}, schema.nodes.run.create({}, [schema.text('Hello')])),
193+
);
194+
const state = createState(doc);
195+
196+
const { paraStart, paraEnd } = findFirstParagraphRange(state.doc);
197+
198+
// lift-style step: insert=0, structure=true, gap=±1
199+
const step = new ReplaceAroundStep(paraStart, paraEnd, paraStart + 1, paraEnd - 1, Slice.empty, 0, true);
200+
201+
const tr = state.tr;
202+
tr.setMeta('inputType', 'insertParagraph');
203+
const newTr = state.tr;
204+
const map = new Mapping();
205+
206+
replaceAroundStep({
207+
state,
208+
tr,
209+
step,
210+
newTr,
211+
map,
212+
doc: state.doc,
213+
user,
214+
date,
215+
originalStep: step,
216+
originalStepIndex: 0,
217+
});
218+
219+
// Should be blocked — not a node markup change
220+
expect(newTr.steps.length).toBe(0);
221+
});
222+
223+
it('appends step mapping after applying node markup change', () => {
224+
const doc = schema.nodes.doc.create(
225+
{},
226+
schema.nodes.paragraph.create(
227+
{ paragraphProperties: { styleId: 'Normal' } },
228+
schema.nodes.run.create({}, [schema.text('Hello')]),
229+
),
230+
);
231+
const state = createState(doc);
232+
233+
const { paraStart, paraEnd } = findFirstParagraphRange(state.doc);
234+
235+
const newParagraph = schema.nodes.paragraph.create({ paragraphProperties: { styleId: 'Heading1' } });
236+
const step = new ReplaceAroundStep(
237+
paraStart,
238+
paraEnd,
239+
paraStart + 1,
240+
paraEnd - 1,
241+
new Slice(Fragment.from(newParagraph), 0, 0),
242+
1,
243+
true,
244+
);
245+
246+
const tr = state.tr;
247+
const newTr = state.tr;
248+
const map = new Mapping();
249+
250+
replaceAroundStep({
251+
state,
252+
tr,
253+
step,
254+
newTr,
255+
map,
256+
doc: state.doc,
257+
user,
258+
date,
259+
originalStep: step,
260+
originalStepIndex: 0,
261+
});
262+
263+
// map should have been updated
264+
expect(map.maps.length).toBe(1);
265+
});
266+
});
267+
151268
describe('non-backspace blocking', () => {
152269
it('blocks non-backspace ReplaceAroundStep (no steps added to newTr)', () => {
153270
const doc = schema.nodes.doc.create(

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/testUtils.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,21 @@ export function findTextPos(docNode, exactText) {
1717
});
1818
return found;
1919
}
20+
21+
/**
22+
* Find the start and end positions of the first paragraph node in a document.
23+
* @param {import('prosemirror-model').Node} doc - Document node to search
24+
* @returns {{ paraStart: number, paraEnd: number }}
25+
*/
26+
export function findFirstParagraphRange(doc) {
27+
let paraStart = null;
28+
let paraEnd = null;
29+
doc.forEach((node, offset) => {
30+
if (paraStart === null && node.type.name === 'paragraph') {
31+
paraStart = offset;
32+
paraEnd = offset + node.nodeSize;
33+
}
34+
});
35+
if (paraStart === null) throw new Error('No paragraph found');
36+
return { paraStart, paraEnd };
37+
}
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
import { test, expect } from '../../fixtures/superdoc.js';
2+
import type { Page } from '@playwright/test';
3+
4+
async function getFirstParagraphStyleId(page: Page): Promise<string | null> {
5+
return page.evaluate(() => {
6+
const editor = (window as any).editor;
7+
let result: string | null = null;
8+
editor.state.doc.descendants((node: any) => {
9+
if (node.type.name === 'paragraph') {
10+
result = node.attrs?.paragraphProperties?.styleId ?? null;
11+
return false;
12+
}
13+
return true;
14+
});
15+
return result;
16+
});
17+
}
18+
19+
test.use({ config: { toolbar: 'full', comments: 'on', trackChanges: true } });
20+
21+
test.describe('SD-2182 heading style changes in suggesting mode', () => {
22+
test('applying heading style via setStyleById works in suggesting mode', async ({ superdoc }) => {
23+
// Type text in editing mode
24+
await superdoc.type('Hello world');
25+
await superdoc.waitForStable();
26+
27+
// Switch to suggesting mode
28+
await superdoc.setDocumentMode('suggesting');
29+
await superdoc.waitForStable();
30+
31+
// Select all and apply Heading1 style
32+
await superdoc.selectAll();
33+
await superdoc.page.evaluate(() => {
34+
(window as any).editor.commands.setStyleById('Heading1');
35+
});
36+
await superdoc.waitForStable();
37+
38+
const styleId = await getFirstParagraphStyleId(superdoc.page);
39+
expect(styleId).toBe('Heading1');
40+
});
41+
42+
test('toggling heading style with cursor works in suggesting mode', async ({ superdoc }) => {
43+
// Type text in editing mode
44+
await superdoc.type('Hello world');
45+
await superdoc.waitForStable();
46+
47+
// Switch to suggesting mode (cursor is at end of text — empty selection)
48+
await superdoc.setDocumentMode('suggesting');
49+
await superdoc.waitForStable();
50+
51+
// Apply Heading1 via toggleLinkedStyle with cursor (no selection)
52+
const result = await superdoc.page.evaluate(() => {
53+
const editor = (window as any).editor;
54+
const style = editor.helpers.linkedStyles.getStyleById('Heading1');
55+
return editor.commands.toggleLinkedStyle(style);
56+
});
57+
await superdoc.waitForStable();
58+
59+
expect(result).toBe(true);
60+
61+
const styleId = await getFirstParagraphStyleId(superdoc.page);
62+
expect(styleId).toBe('Heading1');
63+
});
64+
65+
test('toggling heading style off with cursor works in suggesting mode', async ({ superdoc }) => {
66+
// Type text and apply heading in editing mode
67+
await superdoc.type('Hello world');
68+
await superdoc.waitForStable();
69+
await superdoc.selectAll();
70+
await superdoc.page.evaluate(() => {
71+
(window as any).editor.commands.setStyleById('Heading1');
72+
});
73+
await superdoc.waitForStable();
74+
75+
// Switch to suggesting mode
76+
await superdoc.setDocumentMode('suggesting');
77+
await superdoc.waitForStable();
78+
79+
// Toggle off Heading1 with cursor (no selection)
80+
const result = await superdoc.page.evaluate(() => {
81+
const editor = (window as any).editor;
82+
const style = editor.helpers.linkedStyles.getStyleById('Heading1');
83+
return editor.commands.toggleLinkedStyle(style);
84+
});
85+
await superdoc.waitForStable();
86+
87+
expect(result).toBe(true);
88+
89+
const styleId = await getFirstParagraphStyleId(superdoc.page);
90+
expect(styleId).toBeNull();
91+
});
92+
});

0 commit comments

Comments
 (0)