diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/__conformance__/contract-conformance.test.ts b/packages/super-editor/src/editors/v1/document-api-adapters/__conformance__/contract-conformance.test.ts index 225c199d96..867e51eab6 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/__conformance__/contract-conformance.test.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/__conformance__/contract-conformance.test.ts @@ -627,6 +627,7 @@ function makeTextEditor( addMark: vi.fn(), removeMark: vi.fn(), replaceWith: vi.fn(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn(), setMeta: vi.fn(), mapping: { map: (pos: number) => pos }, @@ -1294,6 +1295,7 @@ function makeTableEditor( delete: vi.fn().mockReturnThis(), setNodeMarkup: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { @@ -1638,15 +1640,26 @@ const NON_RECEIPT_MUTATION_OPS: ReadonlySet = new Set([ ] as OperationId[]); /** - * Content-control operations whose handlers always return `true` because they - * build and dispatch their own ProseMirror transaction directly (via - * `editor.view!.dispatch(tr)`) rather than delegating to an editor command whose - * boolean result propagates to the domain-command executor. + * Content-control operations excluded from the structured-failure conformance + * check because they have no synthetic-failure path that + * `makeNoOpSdtEditor` can simulate. * - * Because the handler always returns `true`, the `domain.command` executor marks - * the step effect as `'changed'` and `executeSdtMutation` returns success. - * There is no code path that produces the `NO_OP` structured failure for these - * operations, so they are excluded from the failureCase conformance check. + * The originals (wrap, unwrap, copy, move, insertBefore, insertAfter, group + * wrap/ungroup, repeatingSection insertItem/cloneItem/deleteItem) build and + * dispatch their own PM transaction directly via `editor.view!.dispatch(tr)` + * rather than delegating to an editor command whose boolean result propagates + * back through the executor. The SD-3123 additions (patch, setLockMode, + * setType, setBinding, clearBinding, patchRawProperties, text.setMultiline, + * the date family, the checkbox family, the choiceList family, and + * repeatingSection.setAllowInsertDelete) no longer route through + * `editor.commands.updateStructuredContentById`; the synthetic + * `updateStructuredContentById = vi.fn(() => false)` mock that previously + * drove the failure case has no effect on the AttrStep / inner-range write + * path. + * + * In both groups, the operations can still fail in production (missing target, + * lock violation, schema invalidation in PM dispatch). They just don't have a + * clean synthetic failure mode reachable from the mock editor. */ const CC_DIRECT_DISPATCH_OPS: ReadonlySet = new Set([ 'contentControls.wrap', @@ -1661,6 +1674,28 @@ const CC_DIRECT_DISPATCH_OPS: ReadonlySet = new Set([ 'contentControls.repeatingSection.insertItemAfter', 'contentControls.repeatingSection.cloneItem', 'contentControls.repeatingSection.deleteItem', + // SD-3123: synthetic noop-mock failure (updateStructuredContentById=false) + // no longer applies — these now write via tr.setNodeAttribute (metadata) + // or tr.replaceWith on the SDT inner range (content). + 'contentControls.patch', + 'contentControls.patchRawProperties', + 'contentControls.setLockMode', + 'contentControls.setType', + 'contentControls.setBinding', + 'contentControls.clearBinding', + 'contentControls.text.setMultiline', + 'contentControls.date.setValue', + 'contentControls.date.clearValue', + 'contentControls.date.setDisplayFormat', + 'contentControls.date.setDisplayLocale', + 'contentControls.date.setStorageFormat', + 'contentControls.date.setCalendar', + 'contentControls.checkbox.setState', + 'contentControls.checkbox.toggle', + 'contentControls.checkbox.setSymbolPair', + 'contentControls.choiceList.setItems', + 'contentControls.choiceList.setSelected', + 'contentControls.repeatingSection.setAllowInsertDelete', ] as OperationId[]); const HAS_STRUCTURED_FAILURE_RESULT = (operationId: OperationId): boolean => @@ -2367,6 +2402,7 @@ function makeTocEditor(commandOverrides: Record = {}): Editor { insert: vi.fn().mockReturnThis(), setNodeMarkup: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, docChanged: true, @@ -2439,6 +2475,7 @@ function makeImageEditor(): Editor { insert: vi.fn().mockReturnThis(), setNodeMarkup: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, docChanged: true, @@ -2513,6 +2550,7 @@ function makeMultiBlockImageEditor(): Editor { insert: vi.fn().mockReturnThis(), setNodeMarkup: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, docChanged: true, @@ -2675,6 +2713,7 @@ function makeSdtEditor(overrideAttrs: Record = {}, textContent addMark: vi.fn().mockReturnThis(), removeMark: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, @@ -2769,6 +2808,7 @@ function makeSdtEditorWithRepeatingSectionItems(): Editor { addMark: vi.fn().mockReturnThis(), removeMark: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, @@ -2888,6 +2928,7 @@ function makeCaptionImageEditor( insert: vi.fn().mockReturnThis(), delete: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), setNodeMarkup: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, @@ -2953,6 +2994,7 @@ function makeRefEditor( addMark: vi.fn().mockReturnThis(), removeMark: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), setNodeMarkup: vi.fn().mockReturnThis(), @@ -11984,6 +12026,7 @@ describe('document-api adapter conformance', () => { insert: vi.fn().mockReturnThis(), setNodeMarkup: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, docChanged: true, diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/content-controls/sdt-properties-write.ts b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/content-controls/sdt-properties-write.ts index 16160b568f..c581dd547f 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/helpers/content-controls/sdt-properties-write.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/helpers/content-controls/sdt-properties-write.ts @@ -11,7 +11,7 @@ import type { Editor } from '../../../core/Editor.js'; import type { ContentControlTarget } from '@superdoc/document-api'; -import { resolveSdtByTarget } from './target-resolution.js'; +import { resolveSdtByTarget, SDT_NODE_NAMES } from './target-resolution.js'; // --------------------------------------------------------------------------- // XML element helpers for sdtPr.elements @@ -68,13 +68,64 @@ function removeSdtPrChild(sdtPr: SdtPrElement, childName: string): SdtPrElement // --------------------------------------------------------------------------- /** - * Apply an attribute patch to an SDT node via updateStructuredContentById. - * Returns true if the command executed successfully. + * PM node-type names that carry a content-control SDT identity. Imported from + * `target-resolution.ts` so the search here resolves the same nodes the + * upstream `resolveSdtByTarget` would resolve. `documentSection` and + * `documentPartObject` are intentionally not in `SDT_NODE_NAMES` — they have + * their own write paths and could otherwise collide on `id` and cause this + * loop to find and mutate the wrong node. + */ +const SDT_NODE_TYPES = new Set(SDT_NODE_NAMES); + +/** + * Apply an attribute patch to an SDT node. + * + * Uses `tr.setNodeAttribute` per key, which emits PM AttrSteps. AttrSteps + * have no `from`/`to` range and are explicitly skipped by the structured- + * content lock plugin's `filterTransaction`, so this path can mutate + * metadata (id, tag, alias, lockMode, controlType, sdtPr, appearance, ...) + * on `sdtLocked` and `sdtContentLocked` controls without tripping the + * wrapper-damage check. + * + * The previous implementation delegated to `editor.commands.updateStructuredContentById`, + * which dispatches `tr.replaceWith(pos, pos + node.nodeSize, ...)`. That + * step's range covered the entire SDT, which the lock plugin read as + * wrapper damage and silently filtered for locked controls — producing + * false-success mutations. + * + * Returns true if a matching SDT was found and the transaction dispatched, + * false if no SDT matched the given id or the editor cannot dispatch. */ export function applyAttrsUpdate(editor: Editor, nodeId: string, attrsPatch: Record): boolean { - const updateCmd = editor.commands?.updateStructuredContentById; - if (typeof updateCmd !== 'function') return false; - return Boolean(updateCmd(nodeId, { attrs: attrsPatch })); + if (!editor?.state) return false; + + let foundPos: number | null = null; + editor.state.doc.descendants((node, pos) => { + if (foundPos !== null) return false; + if (SDT_NODE_TYPES.has(node.type.name) && String(node.attrs.id) === String(nodeId)) { + foundPos = pos; + return false; + } + return true; + }); + + if (foundPos === null) return false; + + const tr = editor.state.tr; + for (const [key, value] of Object.entries(attrsPatch)) { + tr.setNodeAttribute(foundPos, key, value); + } + + if (tr.steps.length === 0) return true; + + if (editor.view?.dispatch) { + editor.view.dispatch(tr); + } else if (typeof editor.dispatch === 'function') { + editor.dispatch(tr); + } else { + return false; + } + return true; } // --------------------------------------------------------------------------- diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.test.ts b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.test.ts index 4a570dbe66..a9fadb6833 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.test.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.test.ts @@ -192,6 +192,7 @@ function makeSdtEditor(overrideAttrs: Record = {}, sdtChildren? addMark: vi.fn().mockReturnThis(), removeMark: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, @@ -289,6 +290,7 @@ function makeInlineSdtEditor(overrideAttrs: Record = {}, sdtChi addMark: vi.fn().mockReturnThis(), removeMark: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, @@ -401,6 +403,7 @@ function makeSdtEditorWithBlockRange(): Editor { addMark: vi.fn().mockReturnThis(), removeMark: vi.fn().mockReturnThis(), replaceWith: vi.fn().mockReturnThis(), + setNodeAttribute: vi.fn().mockReturnThis(), insert: vi.fn().mockReturnThis(), setMeta: vi.fn().mockReturnThis(), mapping: { map: (pos: number) => pos }, @@ -531,7 +534,7 @@ describe('contentControls text clearing', () => { expect((editor.state.tr as any).replaceWith).toHaveBeenCalledTimes(1); }); - it('text.clearValue clears inline SDTs without routing through updateStructuredContentById', () => { + it('text.clearValue clears inline SDTs by deleting the inner range, not rewriting the wrapper', () => { const editor = makeInlineSdtEditor(); const adapter = createContentControlsAdapter(editor); @@ -539,7 +542,11 @@ describe('contentControls text clearing', () => { expect(result.success).toBe(true); expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); - expect((editor.state.tr as any).replaceWith).toHaveBeenCalledTimes(1); + // Empty inline clears via tr.delete on the inner content range so the SDT + // wrapper stays intact — the lock plugin's filterTransaction won't read it + // as wrapper damage on sdtLocked controls. + expect((editor.state.tr as any).delete).toHaveBeenCalledTimes(1); + expect((editor.state.tr as any).replaceWith).not.toHaveBeenCalled(); }); it('clearContent clears block SDTs that only contain non-text block content', () => { @@ -596,7 +603,7 @@ describe('contentControls plain-text replacement no-op detection', () => { expect((editor.state.tr as any).replaceWith).toHaveBeenCalledTimes(1); }); - it('replaceContent rewrites inline SDTs when matching text still carries run formatting', () => { + it('replaceContent rewrites inline SDTs by replacing the inner content range, not the wrapper', () => { const editor = makeInlineSdtEditor({}, [createRunNode('Inline SDT content', { runProperties: { bold: true } })]); const adapter = createContentControlsAdapter(editor); @@ -606,12 +613,11 @@ describe('contentControls plain-text replacement no-op detection', () => { ); expect(result.success).toBe(true); - expect(editor.commands!.updateStructuredContentById).toHaveBeenCalledWith('sdt-inline-1', { - text: 'Inline SDT content', - }); + expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); + expect((editor.state.tr as any).replaceWith).toHaveBeenCalledTimes(1); }); - it('text.setValue rewrites inline text controls when matching text still carries run formatting', () => { + it('text.setValue rewrites inline text controls by replacing the inner content range, not the wrapper', () => { const editor = makeInlineSdtEditor({}, [createRunNode('Inline SDT content', { runProperties: { bold: true } })]); const adapter = createContentControlsAdapter(editor); @@ -621,23 +627,29 @@ describe('contentControls plain-text replacement no-op detection', () => { ); expect(result.success).toBe(true); - expect(editor.commands!.updateStructuredContentById).toHaveBeenCalledWith('sdt-inline-1', { - text: 'Inline SDT content', - }); + expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); + expect((editor.state.tr as any).replaceWith).toHaveBeenCalledTimes(1); }); }); describe('contentControls.setType OOXML element transitions', () => { - it('calls updateStructuredContentById to persist controlType and type attrs', () => { + it('persists controlType and type attrs via tr.setNodeAttribute (AttrSteps, lock-safe)', () => { const editor = makeSdtEditor({ controlType: 'text', type: 'text' }); const adapter = createContentControlsAdapter(editor); const result = adapter.setType({ target: SDT_TARGET, controlType: 'date' }, { changeMode: 'direct' }); expect(result.success).toBe(true); - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - // Should be called at least once for the type element transitions + once for attrs - expect(updateCmd).toHaveBeenCalled(); + // After SD-3123, metadata writes flow through tr.setNodeAttribute (AttrStep, no + // from/to) instead of editor.commands.updateStructuredContentById (which dispatched + // a full-wrapper replaceWith that the lock plugin filtered on sdtLocked controls). + const setAttr = (editor.state.tr as any).setNodeAttribute as ReturnType; + expect(setAttr).toHaveBeenCalled(); + const attrKeys = setAttr.mock.calls.map((c: any[]) => c[1]); + expect(attrKeys).toContain('controlType'); + expect(attrKeys).toContain('type'); + const ctCall = setAttr.mock.calls.find((c: any[]) => c[1] === 'controlType'); + expect(ctCall?.[2]).toBe('date'); }); it('returns NO_OP when target already has the requested type', () => { @@ -653,7 +665,6 @@ describe('contentControls.setType OOXML element transitions', () => { }); it('removes old type element and adds new type element in sdtPr', () => { - // Start with a 'text' control that has a w:text element in sdtPr const sdtPr = { name: 'w:sdtPr', elements: [{ name: 'w:text', type: 'element' }], @@ -663,21 +674,36 @@ describe('contentControls.setType OOXML element transitions', () => { adapter.setType({ target: SDT_TARGET, controlType: 'date' }, { changeMode: 'direct' }); - // updateStructuredContentById is called 3 times: - // 1) remove old w:text element from sdtPr - // 2) add new w:date element to sdtPr - // 3) update controlType/type attrs - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - expect(updateCmd.mock.calls.length).toBeGreaterThanOrEqual(2); - - // Verify the final attrs update includes the new type - const lastCall = updateCmd.mock.calls[updateCmd.mock.calls.length - 1]; - expect(lastCall[1].attrs.controlType).toBe('date'); - expect(lastCall[1].attrs.type).toBe('date'); + // setType issues at least two AttrStep writes against sdtPr (via tr.setNodeAttribute): + // one removes the old w:text, another adds the new w:date. (The mock doesn't fold + // intermediate AttrSteps back into the node's attrs the way a real PM transaction + // would, so the two writes appear independently rather than composed.) + const setAttr = (editor.state.tr as any).setNodeAttribute as ReturnType; + const sdtPrCalls = setAttr.mock.calls.filter((c: any[]) => c[1] === 'sdtPr'); + expect(sdtPrCalls.length).toBeGreaterThanOrEqual(2); + + type SdtPrWrite = { elements?: Array<{ name: string }> }; + const sdtPrWrites = sdtPrCalls.map((c: any[]) => c[2] as SdtPrWrite); + expect(sdtPrWrites.some((s) => !s.elements?.some((el) => el.name === 'w:text'))).toBe(true); + expect(sdtPrWrites.some((s) => s.elements?.some((el) => el.name === 'w:date'))).toBe(true); + + const ctCall = setAttr.mock.calls.find((c: any[]) => c[1] === 'controlType'); + expect(ctCall?.[2]).toBe('date'); }); }); describe('contentControls.patchRawProperties element normalization', () => { + // Helper: read the final sdtPr written by tr.setNodeAttribute(...,'sdtPr',...). + // patchRawProperties / setType now flow through applyAttrsUpdate's AttrStep path, + // so the sdtPr lands on the editor's transaction instead of the legacy command. + const lastWrittenSdtPr = ( + editor: Editor, + ): { elements?: Array<{ name: string; attributes?: Record }> } => { + const setAttr = (editor.state.tr as any).setNodeAttribute as ReturnType; + const sdtPrCalls = setAttr.mock.calls.filter((c: any[]) => c[1] === 'sdtPr'); + return sdtPrCalls[sdtPrCalls.length - 1]?.[2]; + }; + it('normalizes set op elements to include name and type', () => { const editor = makeSdtEditor(); const adapter = createContentControlsAdapter(editor); @@ -696,17 +722,16 @@ describe('contentControls.patchRawProperties element normalization', () => { { changeMode: 'direct' }, ); - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - expect(updateCmd).toHaveBeenCalled(); - - // The sdtPr written back should contain the normalized element - const call = updateCmd.mock.calls[updateCmd.mock.calls.length - 1]; - const writtenSdtPr = call[1].attrs.sdtPr; - const customEl = writtenSdtPr.elements.find((el: any) => el.name === 'w:custom'); + const writtenSdtPr = lastWrittenSdtPr(editor); + const customEl = writtenSdtPr.elements?.find((el) => el.name === 'w:custom') as { + name: string; + type?: string; + attributes?: Record; + }; expect(customEl).toBeDefined(); expect(customEl.name).toBe('w:custom'); expect(customEl.type).toBe('element'); - expect(customEl.attributes['w:val']).toBe('hello'); + expect(customEl.attributes?.['w:val']).toBe('hello'); }); it('set op forces name to match patch.name even if element has a different name', () => { @@ -727,13 +752,9 @@ describe('contentControls.patchRawProperties element normalization', () => { { changeMode: 'direct' }, ); - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - const call = updateCmd.mock.calls[updateCmd.mock.calls.length - 1]; - const writtenSdtPr = call[1].attrs.sdtPr; - const el = writtenSdtPr.elements.find((e: any) => e.name === 'w:correct'); - expect(el).toBeDefined(); - // No element with the wrong name should exist - expect(writtenSdtPr.elements.find((e: any) => e.name === 'w:wrong')).toBeUndefined(); + const writtenSdtPr = lastWrittenSdtPr(editor); + expect(writtenSdtPr.elements?.find((e) => e.name === 'w:correct')).toBeDefined(); + expect(writtenSdtPr.elements?.find((e) => e.name === 'w:wrong')).toBeUndefined(); }); it('setAttr modifies attributes on an existing sdtPr element', () => { @@ -752,11 +773,9 @@ describe('contentControls.patchRawProperties element normalization', () => { { changeMode: 'direct' }, ); - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - const call = updateCmd.mock.calls[updateCmd.mock.calls.length - 1]; - const writtenSdtPr = call[1].attrs.sdtPr; - const el = writtenSdtPr.elements.find((e: any) => e.name === 'w:custom'); - expect(el.attributes['w:val']).toBe('new'); + const writtenSdtPr = lastWrittenSdtPr(editor); + const el = writtenSdtPr.elements?.find((e) => e.name === 'w:custom'); + expect(el?.attributes?.['w:val']).toBe('new'); }); it('removeAttr removes an attribute from an existing sdtPr element', () => { @@ -775,12 +794,10 @@ describe('contentControls.patchRawProperties element normalization', () => { { changeMode: 'direct' }, ); - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - const call = updateCmd.mock.calls[updateCmd.mock.calls.length - 1]; - const writtenSdtPr = call[1].attrs.sdtPr; - const el = writtenSdtPr.elements.find((e: any) => e.name === 'w:custom'); - expect(el.attributes['w:val']).toBeUndefined(); - expect(el.attributes['w:other']).toBe('y'); + const writtenSdtPr = lastWrittenSdtPr(editor); + const el = writtenSdtPr.elements?.find((e) => e.name === 'w:custom'); + expect(el?.attributes?.['w:val']).toBeUndefined(); + expect(el?.attributes?.['w:other']).toBe('y'); }); it('remove op deletes an element from sdtPr.elements', () => { @@ -802,11 +819,135 @@ describe('contentControls.patchRawProperties element normalization', () => { { changeMode: 'direct' }, ); - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - const call = updateCmd.mock.calls[updateCmd.mock.calls.length - 1]; - const writtenSdtPr = call[1].attrs.sdtPr; - expect(writtenSdtPr.elements.find((e: any) => e.name === 'w:custom')).toBeUndefined(); - expect(writtenSdtPr.elements.find((e: any) => e.name === 'w:other')).toBeDefined(); + const writtenSdtPr = lastWrittenSdtPr(editor); + expect(writtenSdtPr.elements?.find((e) => e.name === 'w:custom')).toBeUndefined(); + expect(writtenSdtPr.elements?.find((e) => e.name === 'w:other')).toBeDefined(); + }); +}); + +// SD-3123 regression: per ECMA-376, sdtLocked protects the wrapper from removal +// but content stays editable. Before this fix, operations the API allowed on +// sdtLocked (text.setValue, replaceContent, appendContent, prependContent, and +// setLockMode itself) still flowed through editor.commands.updateStructuredContentById +// which dispatched tr.replaceWith spanning [pos, pos+nodeSize] — the lock plugin +// read that as wrapper damage and silently filtered the transaction, producing +// false-success no-ops. After the fix: +// - metadata writes emit AttrSteps via tr.setNodeAttribute (no from/to, +// explicitly skipped by the lock plugin's filterTransaction) +// - text-content writes emit a ReplaceStep on [pos+1, pos+nodeSize-1] which +// the lock plugin classifies as wouldModifyContent (allowed on sdtLocked). +// +// API-level guards (assertNotSdtLocked on patch, setType, setBinding, etc.) +// are intentionally out of scope here — relaxing those is a separate behavior +// decision tracked as a follow-up. +describe('SD-3123: sdtLocked controls mutate through AttrSteps and content-range writes', () => { + // Helpers to read the resolved SDT's position out of the mock doc so the + // tests assert on the actual inner range rather than just "replaceWith was + // called". A full-wrapper replaceWith(pos, pos+nodeSize, ...) — the + // pre-fix shape — would otherwise pass the call-count assertion. + const findSdtPos = ( + editor: Editor, + nodeName: 'structuredContent' | 'structuredContentBlock', + ): { pos: number; nodeSize: number } => { + let found: { pos: number; nodeSize: number } | null = null; + (editor.state.doc as any).descendants((node: any, pos: number) => { + if (node.type.name === nodeName) { + found = { pos, nodeSize: node.nodeSize }; + return false; + } + return true; + }); + if (!found) throw new Error(`No ${nodeName} found in mock doc`); + return found; + }; + + const assertInnerRange = (from: number, to: number, sdtPos: number, sdtNodeSize: number): void => { + expect(from).toBe(sdtPos + 1); + expect(to).toBe(sdtPos + sdtNodeSize - 1); + // Sanity: this must not equal the full wrapper bounds. + expect(from).not.toBe(sdtPos); + expect(to).not.toBe(sdtPos + sdtNodeSize); + }; + + it('setLockMode on an sdtLocked control updates lockMode via tr.setNodeAttribute', () => { + const editor = makeSdtEditor({ lockMode: 'sdtLocked' }); + const adapter = createContentControlsAdapter(editor); + + const result = adapter.setLockMode({ target: SDT_TARGET, lockMode: 'unlocked' }, { changeMode: 'direct' }); + + expect(result.success).toBe(true); + const setAttr = (editor.state.tr as any).setNodeAttribute as ReturnType; + const lockCall = setAttr.mock.calls.find((c: any[]) => c[1] === 'lockMode'); + expect(lockCall?.[2]).toBe('unlocked'); + expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); + }); + + it('text.setValue on an sdtLocked inline control replaces the inner range (pos+1, pos+nodeSize-1)', () => { + const editor = makeInlineSdtEditor({ lockMode: 'sdtLocked' }); + const adapter = createContentControlsAdapter(editor); + const { pos, nodeSize } = findSdtPos(editor, 'structuredContent'); + + const result = adapter.text.setValue({ target: INLINE_SDT_TARGET, value: 'updated' }, { changeMode: 'direct' }); + + expect(result.success).toBe(true); + const replaceWith = (editor.state.tr as any).replaceWith as ReturnType; + expect(replaceWith).toHaveBeenCalledTimes(1); + const [from, to] = replaceWith.mock.calls[0]; + assertInnerRange(from, to, pos, nodeSize); + expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); + }); + + it('text.clearValue on an sdtLocked inline control deletes the inner range (pos+1, pos+nodeSize-1)', () => { + const editor = makeInlineSdtEditor({ lockMode: 'sdtLocked' }); + const adapter = createContentControlsAdapter(editor); + const { pos, nodeSize } = findSdtPos(editor, 'structuredContent'); + + const result = adapter.text.clearValue({ target: INLINE_SDT_TARGET }, { changeMode: 'direct' }); + + expect(result.success).toBe(true); + const deleteFn = (editor.state.tr as any).delete as ReturnType; + expect(deleteFn).toHaveBeenCalledTimes(1); + const [from, to] = deleteFn.mock.calls[0]; + assertInnerRange(from, to, pos, nodeSize); + expect((editor.state.tr as any).replaceWith).not.toHaveBeenCalled(); + expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); + }); + + it('replaceContent on an sdtLocked block control replaces the inner range (pos+1, pos+nodeSize-1)', () => { + const editor = makeSdtEditor({ lockMode: 'sdtLocked' }); + const adapter = createContentControlsAdapter(editor); + const { pos, nodeSize } = findSdtPos(editor, 'structuredContentBlock'); + + const result = adapter.replaceContent( + { target: SDT_TARGET, content: 'updated block content' }, + { changeMode: 'direct' }, + ); + + expect(result.success).toBe(true); + const replaceWith = (editor.state.tr as any).replaceWith as ReturnType; + expect(replaceWith).toHaveBeenCalledTimes(1); + const [from, to] = replaceWith.mock.calls[0]; + assertInnerRange(from, to, pos, nodeSize); + expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); + }); + + it('clearContent on an sdtLocked block control replaces the inner range (pos+1, pos+nodeSize-1)', () => { + // clearContent flows through the same replaceSdtTextContent path with an + // empty body; on block controls the inner range is replaced with an empty + // paragraph. Without the SD-3123 inner-range fix this would dispatch a + // full-wrapper replaceWith and the lock plugin would filter it. + const editor = makeSdtEditor({ lockMode: 'sdtLocked' }); + const adapter = createContentControlsAdapter(editor); + const { pos, nodeSize } = findSdtPos(editor, 'structuredContentBlock'); + + const result = adapter.clearContent({ target: SDT_TARGET }, { changeMode: 'direct' }); + + expect(result.success).toBe(true); + const replaceWith = (editor.state.tr as any).replaceWith as ReturnType; + expect(replaceWith).toHaveBeenCalledTimes(1); + const [from, to] = replaceWith.mock.calls[0]; + assertInnerRange(from, to, pos, nodeSize); + expect(editor.commands!.updateStructuredContentById).not.toHaveBeenCalled(); }); }); @@ -1158,16 +1299,19 @@ describe('contentControls.setType default sdtPr seeding', () => { const result = adapter.setType({ target: SDT_TARGET, controlType: 'checkbox' }, { changeMode: 'direct' }); expect(result.success).toBe(true); - const updateCmd = editor.commands!.updateStructuredContentById as ReturnType; - const checkboxWrite = updateCmd.mock.calls.find((call) => - Boolean(call[1]?.attrs?.sdtPr?.elements?.find((el: { name: string }) => el.name === 'w14:checkbox')), + // After SD-3123, sdtPr writes flow via tr.setNodeAttribute. Find the sdtPr write + // that introduced the w14:checkbox element. + const setAttr = (editor.state.tr as any).setNodeAttribute as ReturnType; + const sdtPrCalls = setAttr.mock.calls.filter((c: any[]) => c[1] === 'sdtPr'); + const checkboxWrite = sdtPrCalls.find((c: any[]) => + Boolean(c[2]?.elements?.find((el: { name: string }) => el.name === 'w14:checkbox')), ); expect(checkboxWrite).toBeDefined(); - const checkbox = checkboxWrite?.[1]?.attrs?.sdtPr?.elements?.find( - (el: { name: string; elements?: Array<{ name: string }> }) => el.name === 'w14:checkbox', + const checkbox = (checkboxWrite?.[2]?.elements as Array<{ name: string; elements?: Array<{ name: string }> }>).find( + (el) => el.name === 'w14:checkbox', ); - expect(checkbox?.elements?.some((el: { name: string }) => el.name === 'w14:checked')).toBe(true); - expect(checkbox?.elements?.some((el: { name: string }) => el.name === 'w14:checkedState')).toBe(true); - expect(checkbox?.elements?.some((el: { name: string }) => el.name === 'w14:uncheckedState')).toBe(true); + expect(checkbox?.elements?.some((el) => el.name === 'w14:checked')).toBe(true); + expect(checkbox?.elements?.some((el) => el.name === 'w14:checkedState')).toBe(true); + expect(checkbox?.elements?.some((el) => el.name === 'w14:uncheckedState')).toBe(true); }); }); diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts index 311344ae24..82ab9e11ff 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/content-controls-wrappers.ts @@ -346,28 +346,40 @@ function alreadyMatchesPlainTextReplacement( return isCanonicalPlainTextParagraph(getOnlyChild(sdt.node), expectedText); } +/** + * Replace the text content of an SDT, operating on the inner content range + * (pos+1 to pos+nodeSize-1) instead of rewriting the whole wrapper. This + * keeps the wrapper node intact so the structured-content lock plugin's + * filterTransaction doesn't read the change as wrapper damage on `sdtLocked` + * controls, which would silently no-op (spec: sdtLocked protects the wrapper + * but the content is editable). + * + * Callers that need to enforce content-lock semantics (`contentLocked` and + * `sdtContentLocked`) must call `assertNotContentLocked` before reaching + * here; this helper trusts that guard and emits a content-range step. + */ function replaceSdtTextContent(editor: Editor, target: ContentControlTarget, text: string): boolean { const resolved = resolveSdtByTarget(editor.state.doc, target); + const { tr } = editor.state; + const innerFrom = resolved.pos + 1; + const innerTo = resolved.pos + resolved.node.nodeSize - 1; if (resolved.kind === 'inline') { - const updateCmd = editor.commands?.updateStructuredContentById; if (text.length > 0) { - return Boolean(updateCmd?.(target.nodeId, { text })); + const textNode = editor.schema.text(text); + tr.replaceWith(innerFrom, innerTo, textNode); + } else { + tr.delete(innerFrom, innerTo); } - - const updatedNode = resolved.node.type.create({ ...resolved.node.attrs }, null, resolved.node.marks); - const { tr } = editor.state; - tr.replaceWith(resolved.pos, resolved.pos + resolved.node.nodeSize, updatedNode); dispatchTransaction(editor, tr); return true; } const paragraph = buildEmptyBlockContent(editor, resolved.node); + if (!paragraph) return false; const paragraphText = text.length > 0 ? buildTextWithTabs(editor.schema, text, undefined) : null; - const updatedParagraph = paragraph?.type.create(paragraph.attrs ?? null, paragraphText, paragraph.marks) ?? null; - const updatedNode = resolved.node.type.create({ ...resolved.node.attrs }, updatedParagraph, resolved.node.marks); - const { tr } = editor.state; - tr.replaceWith(resolved.pos, resolved.pos + resolved.node.nodeSize, updatedNode); + const updatedParagraph = paragraph.type.create(paragraph.attrs ?? null, paragraphText, paragraph.marks); + tr.replaceWith(innerFrom, innerTo, updatedParagraph); dispatchTransaction(editor, tr); return true; } diff --git a/tests/behavior/tests/sdt/document-api-locked-mutations.spec.ts b/tests/behavior/tests/sdt/document-api-locked-mutations.spec.ts new file mode 100644 index 0000000000..7e7948441f --- /dev/null +++ b/tests/behavior/tests/sdt/document-api-locked-mutations.spec.ts @@ -0,0 +1,235 @@ +/** + * Real-editor regression for SD-3123: programmatic content edits on sdtLocked + * SDTs must flow through (the wrapper is protected, but the content is + * editable per OOXML spec). Before SD-3123, all paths dispatched a + * full-wrapper `tr.replaceWith(pos, pos + nodeSize, ...)` which the + * structured-content lock plugin's `filterTransaction` read as wrapper damage + * and silently filtered — producing false-success no-ops. Mock-based unit + * tests prove the new transaction shapes (AttrSteps for metadata, inner-range + * ReplaceSteps for content), but they do not run the real lock plugin. + * These tests close that gap. + * + * Operates through `editor.doc.contentControls.*` (the customer surface), + * not `editor.commands.*`. Reads results back through `getContent` and the + * painter-rendered DOM rather than PM internals. + */ + +import { test, expect } from '../../fixtures/superdoc.js'; + +const BLOCK_SDT = '.superdoc-structured-content-block'; +const INLINE_SDT = '.superdoc-structured-content-inline'; + +type LockMode = 'unlocked' | 'sdtLocked' | 'contentLocked' | 'sdtContentLocked'; +type ContentControlTarget = { kind: 'inline' | 'block'; nodeType: 'sdt'; nodeId: string }; +type MutationResult = + | { success: true; contentControl: ContentControlTarget } + | { success: false; failure: { code: string; message?: string } }; +type GetContentResult = { content: string; format: 'text' | 'html' }; + +interface CreateOpts { + kind: 'inline' | 'block'; + controlType?: 'text' | 'richText'; + alias: string; + content?: string; +} + +async function createControl(superdoc: any, opts: CreateOpts): Promise { + const result = await superdoc.page.evaluate((o: CreateOpts) => { + const r = (window as any).editor.doc.create.contentControl( + { kind: o.kind, controlType: o.controlType, alias: o.alias, content: o.content }, + { changeMode: 'direct' }, + ); + if (!r.success) throw new Error(`create failed: ${r.failure?.code} ${r.failure?.message}`); + return r.contentControl; + }, opts); + return result as ContentControlTarget; +} + +async function setLock(superdoc: any, target: ContentControlTarget, lockMode: LockMode): Promise { + return superdoc.page.evaluate( + ({ target, lockMode }: { target: ContentControlTarget; lockMode: LockMode }) => + (window as any).editor.doc.contentControls.setLockMode({ target, lockMode }, { changeMode: 'direct' }), + { target, lockMode }, + ); +} + +async function textSetValue(superdoc: any, target: ContentControlTarget, value: string): Promise { + return superdoc.page.evaluate( + ({ target, value }: { target: ContentControlTarget; value: string }) => + (window as any).editor.doc.contentControls.text.setValue({ target, value }, { changeMode: 'direct' }), + { target, value }, + ); +} + +async function replaceContent(superdoc: any, target: ContentControlTarget, content: string): Promise { + return superdoc.page.evaluate( + ({ target, content }: { target: ContentControlTarget; content: string }) => + (window as any).editor.doc.contentControls.replaceContent( + { target, content, format: 'text' }, + { changeMode: 'direct' }, + ), + { target, content }, + ); +} + +async function clearContent(superdoc: any, target: ContentControlTarget): Promise { + return superdoc.page.evaluate( + ({ target }: { target: ContentControlTarget }) => + (window as any).editor.doc.contentControls.clearContent({ target }, { changeMode: 'direct' }), + { target }, + ); +} + +async function getContent(superdoc: any, target: ContentControlTarget): Promise { + return superdoc.page.evaluate( + ({ target }: { target: ContentControlTarget }) => (window as any).editor.doc.contentControls.getContent({ target }), + { target }, + ); +} + +// =========================================================================== +// sdtLocked: wrapper protected, content editable +// =========================================================================== + +test.describe('SD-3123: Document API mutations on sdtLocked content controls', () => { + test('text.setValue updates an sdtLocked inline plain-text control', async ({ superdoc }) => { + const target = await createControl(superdoc, { + kind: 'inline', + controlType: 'text', + alias: 'Locked plain-text', + content: 'initial', + }); + await superdoc.waitForStable(); + + const lockResult = await setLock(superdoc, target, 'sdtLocked'); + expect(lockResult.success).toBe(true); + await superdoc.waitForStable(); + + const before = await getContent(superdoc, target); + expect(before.content).toBe('initial'); + + const setResult = await textSetValue(superdoc, target, 'updated'); + expect(setResult.success).toBe(true); + await superdoc.waitForStable(); + + const after = await getContent(superdoc, target); + expect(after.content).toBe('updated'); + + // Wrapper still in the painter DOM — the lock didn't get bypassed in a way + // that drops the SDT. + await superdoc.assertElementExists(INLINE_SDT); + }); + + test('replaceContent updates an sdtLocked block rich-text control', async ({ superdoc }) => { + const target = await createControl(superdoc, { + kind: 'block', + controlType: 'richText', + alias: 'Locked rich-text block', + content: 'initial block body', + }); + await superdoc.waitForStable(); + + const lockResult = await setLock(superdoc, target, 'sdtLocked'); + expect(lockResult.success).toBe(true); + await superdoc.waitForStable(); + + const setResult = await replaceContent(superdoc, target, 'updated block body'); + expect(setResult.success).toBe(true); + await superdoc.waitForStable(); + + const after = await getContent(superdoc, target); + expect(after.content).toContain('updated block body'); + + await superdoc.assertElementExists(BLOCK_SDT); + }); + + test('clearContent empties an sdtLocked block control without removing the wrapper', async ({ superdoc }) => { + const target = await createControl(superdoc, { + kind: 'block', + controlType: 'richText', + alias: 'Locked clear target', + content: 'will be cleared', + }); + await superdoc.waitForStable(); + + const lockResult = await setLock(superdoc, target, 'sdtLocked'); + expect(lockResult.success).toBe(true); + await superdoc.waitForStable(); + + const clearResult = await clearContent(superdoc, target); + expect(clearResult.success).toBe(true); + await superdoc.waitForStable(); + + const after = await getContent(superdoc, target); + expect(after.content.trim()).toBe(''); + await superdoc.assertElementExists(BLOCK_SDT); + }); + + test('round-trip: sdtLocked → unlocked → text.setValue succeeds (lock-state attr writes via AttrStep)', async ({ + superdoc, + }) => { + const target = await createControl(superdoc, { + kind: 'inline', + controlType: 'text', + alias: 'Round-trip', + content: 'initial', + }); + await superdoc.waitForStable(); + + expect((await setLock(superdoc, target, 'sdtLocked')).success).toBe(true); + await superdoc.waitForStable(); + + expect((await setLock(superdoc, target, 'unlocked')).success).toBe(true); + await superdoc.waitForStable(); + + expect((await textSetValue(superdoc, target, 'after unlock')).success).toBe(true); + await superdoc.waitForStable(); + + expect((await getContent(superdoc, target)).content).toBe('after unlock'); + }); +}); + +// =========================================================================== +// contentLocked: content protected. API guard fires before reaching engine. +// =========================================================================== + +test.describe('SD-3123: contentLocked still rejects content mutation via API guard', () => { + test('text.setValue on a contentLocked control returns LOCK_VIOLATION and leaves content unchanged', async ({ + superdoc, + }) => { + const target = await createControl(superdoc, { + kind: 'inline', + controlType: 'text', + alias: 'Content-locked', + content: 'protected', + }); + await superdoc.waitForStable(); + + expect((await setLock(superdoc, target, 'contentLocked')).success).toBe(true); + await superdoc.waitForStable(); + + // The wrapper's `assertNotContentLocked` guard throws inside the adapter; + // page.evaluate surfaces it as a rejected promise. Catch and inspect. + const rejection = await superdoc.page.evaluate( + ({ target }: { target: ContentControlTarget }) => { + try { + (window as any).editor.doc.contentControls.text.setValue( + { target, value: 'changed' }, + { changeMode: 'direct' }, + ); + return { threw: false }; + } catch (err) { + const e = err as { code?: string; message?: string }; + return { threw: true, code: e.code, message: e.message }; + } + }, + { target }, + ); + + expect(rejection.threw).toBe(true); + expect(rejection.code).toBe('LOCK_VIOLATION'); + + // Content should remain unchanged. + expect((await getContent(superdoc, target)).content).toBe('protected'); + }); +});