diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.js b/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.js index c92654773c..2e5bbd197b 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.js @@ -70,6 +70,11 @@ function decode(params) { const childTranslator = node.attrs.isAnchor ? wpAnchorTranslator : wpInlineTranslator; const resultNode = childTranslator.decode(params); + // Guard: only wrap when we have valid drawing content. + if (!resultNode || (resultNode.name !== 'wp:inline' && resultNode.name !== 'wp:anchor')) { + return resultNode; + } + return wrapTextInRun( { name: 'w:drawing', diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.test.js b/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.test.js index b4b9a8b45b..2fda267d9c 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.test.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.test.js @@ -95,9 +95,9 @@ describe('w:drawing translator', () => { }); describe('decode', () => { - it('delegates to wp:anchor when node.attrs.isAnchor is true', () => { + it('wraps valid wp:anchor child in w:drawing', () => { const params = { node: { type: 'element', attrs: { isAnchor: true } } }; - anchorTranslatorMock.decode.mockReturnValue({ decoded: 'anchor' }); + anchorTranslatorMock.decode.mockReturnValue({ name: 'wp:anchor', decoded: 'anchor' }); const result = translator.decode(params); @@ -105,16 +105,21 @@ describe('w:drawing translator', () => { expect(wrapTextInRun).toHaveBeenCalledWith( expect.objectContaining({ name: 'w:drawing', - elements: [{ decoded: 'anchor' }], + elements: [expect.objectContaining({ name: 'wp:anchor' })], }), [], ); - expect(result).toEqual({ wrapped: { name: 'w:drawing', elements: [{ decoded: 'anchor' }] } }); + expect(result).toEqual({ + wrapped: { + name: 'w:drawing', + elements: [expect.objectContaining({ name: 'wp:anchor' })], + }, + }); }); - it('delegates to wp:inline when node.attrs.isAnchor is false', () => { + it('wraps valid wp:inline child in w:drawing', () => { const params = { node: { type: 'element', attrs: { isAnchor: false } } }; - inlineTranslatorMock.decode.mockReturnValue({ decoded: 'inline' }); + inlineTranslatorMock.decode.mockReturnValue({ name: 'wp:inline', decoded: 'inline' }); const result = translator.decode(params); @@ -122,11 +127,50 @@ describe('w:drawing translator', () => { expect(wrapTextInRun).toHaveBeenCalledWith( expect.objectContaining({ name: 'w:drawing', - elements: [{ decoded: 'inline' }], + elements: [expect.objectContaining({ name: 'wp:inline' })], }), [], ); - expect(result).toEqual({ wrapped: { name: 'w:drawing', elements: [{ decoded: 'inline' }] } }); + expect(result).toEqual({ + wrapped: { + name: 'w:drawing', + elements: [expect.objectContaining({ name: 'wp:inline' })], + }, + }); + }); + + it('returns child as-is when anchor child is not a drawing node', () => { + const params = { node: { type: 'element', attrs: { isAnchor: true } } }; + const textRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ text: 'x' }] }] }; + anchorTranslatorMock.decode.mockReturnValue(textRun); + + const result = translator.decode(params); + + expect(anchorTranslatorMock.decode).toHaveBeenCalledWith(params); + expect(wrapTextInRun).not.toHaveBeenCalled(); + expect(result).toEqual(textRun); + }); + + it('returns child as-is when inline child is not a drawing node', () => { + const params = { node: { type: 'element', attrs: { isAnchor: false } } }; + const textRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ text: 'y' }] }] }; + inlineTranslatorMock.decode.mockReturnValue(textRun); + + const result = translator.decode(params); + + expect(inlineTranslatorMock.decode).toHaveBeenCalledWith(params); + expect(wrapTextInRun).not.toHaveBeenCalled(); + expect(result).toEqual(textRun); + }); + + it('returns null when child translator returns null', () => { + const params = { node: { type: 'element', attrs: { isAnchor: false } } }; + inlineTranslatorMock.decode.mockReturnValue(null); + + const result = translator.decode(params); + + expect(wrapTextInRun).not.toHaveBeenCalled(); + expect(result).toBeNull(); }); it('returns null if node is missing', () => { diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.js b/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.js index 0a73accecf..282c053b1e 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.js @@ -68,6 +68,11 @@ export function translateAnchorNode(params) { const nodeElements = translateImageNode(params); + // Guard: bail out if translateImageNode produced a non-drawing result (e.g. text fallback). + if (!nodeElements?.elements?.some((el) => el?.name === 'wp:extent')) { + return nodeElements; + } + const inlineAttrs = { ...(attrs.originalAttributes || {}), ...(nodeElements.attributes || {}), diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.test.js b/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.test.js index a854861166..4648ae2ce9 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.test.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.test.js @@ -15,10 +15,10 @@ describe('translateAnchorNode', () => { beforeEach(() => { vi.clearAllMocks(); - // default mock for translateImageNode + // default mock for translateImageNode — must include wp:extent so the guard passes translateImageNode.mockReturnValue({ attributes: { fakeAttr: 'val' }, - elements: [{ name: 'pic:fake' }], + elements: [{ name: 'wp:extent' }, { name: 'wp:effectExtent' }, { name: 'pic:fake' }], }); // default mock for pixelsToEmu diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/decode-image-node-helpers.js b/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/decode-image-node-helpers.js index fcb83a916a..9fadc9959e 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/decode-image-node-helpers.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/decode-image-node-helpers.js @@ -37,9 +37,9 @@ function buildHlinkClickElement(attrs, hlinkRId) { * - `a:hlinkClick` child when hyperlink is set (Word's canonical placement per §20.4.2.5) * - Decorative extension child when attrs.decorative is true */ -function buildDocPrElement(attrs, imageName, hlinkRId) { +function buildDocPrElement(attrs, imageName, hlinkRId, drawingId) { const docPrAttrs = { - id: attrs.id || 0, + id: drawingId, name: attrs.alt || `Picture ${imageName}`, }; // Emit descr (accessibility description) unless decorative @@ -84,7 +84,7 @@ function buildDocPrElement(attrs, imageName, hlinkRId) { * - `a:hlinkClick` child when hyperlink is set (mirrors wp:docPr for compatibility) * - `a:picLocks/@noChangeAspect` ← dynamic from attrs.lockAspectRatio */ -function buildNvPicPrElement(attrs, imageName, hlinkRId) { +function buildNvPicPrElement(attrs, imageName, hlinkRId, drawingId) { // --- pic:cNvPr children (hyperlink) --- const cNvPrChildren = []; const hlinkEl = buildHlinkClickElement(attrs, hlinkRId); @@ -96,7 +96,7 @@ function buildNvPicPrElement(attrs, imageName, hlinkRId) { { name: 'pic:cNvPr', attributes: { - id: attrs.id || 0, + id: drawingId, name: attrs.alt || `Picture ${imageName}`, }, ...(cNvPrChildren.length ? { elements: cNvPrChildren } : {}), @@ -295,6 +295,9 @@ export const translateImageNode = (params) => { // Resolve hyperlink relationship once; shared by wp:docPr and pic:cNvPr. const hlinkRId = resolveHyperlinkRId(attrs, params); + // Ensure valid positive docPr/cNvPr IDs (OOXML requires id > 0). + const drawingId = attrs.id && Number(attrs.id) > 0 ? attrs.id : Math.max(1, parseInt(generateDocxRandomId(), 16)); + return { attributes: inlineAttrs, elements: [ @@ -309,7 +312,7 @@ export const translateImageNode = (params) => { name: 'wp:effectExtent', attributes: effectExtentAttrs, }, - buildDocPrElement(attrs, imageName, hlinkRId), + buildDocPrElement(attrs, imageName, hlinkRId, drawingId), { name: 'wp:cNvGraphicFramePr', elements: [ @@ -334,7 +337,7 @@ export const translateImageNode = (params) => { name: 'pic:pic', attributes: { 'xmlns:pic': pictureXmlns }, elements: [ - buildNvPicPrElement(attrs, imageName, hlinkRId), + buildNvPicPrElement(attrs, imageName, hlinkRId, drawingId), { name: 'pic:blipFill', elements: [ diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.js b/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.js index 9a78927417..2cc460eab0 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.js @@ -15,8 +15,13 @@ import { carbonCopy } from '@core/utilities/carbonCopy.js'; export function mergeDrawingChildren({ order, generated, original }) { const genQueues = groupByName(generated); const originalsByIndex = groupByIndex(original); + const merged = mergeWithOrder(order, genQueues, originalsByIndex); - return mergeWithOrder(order, genQueues, originalsByIndex); + // Originals may carry invalid IDs (e.g. id="0") that Word rejects. + // Patch them using the valid ID from the generated wp:docPr. + fixZeroDrawingIds(merged, generated); + + return merged; } function groupByIndex(entries = []) { @@ -70,6 +75,7 @@ function mergeWithOrder(order = [], genQueues, originalsByIndex) { return out; } + function groupByName(nodes = []) { const map = new Map(); nodes.forEach((el) => { @@ -80,3 +86,28 @@ function groupByName(nodes = []) { }); return map; } + +/** + * Patch zero/missing IDs on wp:docPr and pic:cNvPr in merged output. + * When the merge prefers an original element, it may carry an invalid id="0" + * that Word rejects. We fix it using the valid ID from the generated wp:docPr. + */ +function fixZeroDrawingIds(merged, generated) { + const genDocPr = generated?.find((el) => el?.name === 'wp:docPr'); + const validId = genDocPr?.attributes?.id; + if (!validId || !(Number(validId) > 0)) return; + + const docPr = merged.find((el) => el?.name === 'wp:docPr'); + if (docPr?.attributes && !(Number(docPr.attributes.id) > 0)) { + docPr.attributes.id = validId; + } + + const graphic = merged.find((el) => el?.name === 'a:graphic'); + const graphicData = graphic?.elements?.find((el) => el?.name === 'a:graphicData'); + const pic = graphicData?.elements?.find((el) => el?.name === 'pic:pic'); + const nvPicPr = pic?.elements?.find((el) => el?.name === 'pic:nvPicPr'); + const cNvPr = nvPicPr?.elements?.find((el) => el?.name === 'pic:cNvPr'); + if (cNvPr?.attributes && !(Number(cNvPr.attributes.id) > 0)) { + cNvPr.attributes.id = validId; + } +} diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.test.js b/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.test.js index e6a928c016..5de67b999f 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.test.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.test.js @@ -204,6 +204,111 @@ describe('mergeDrawingChildren', () => { }); }); + describe('zero drawing ID fix', () => { + it('patches wp:docPr id=0 using the generated ID', () => { + const result = mergeDrawingChildren({ + order: ['wp:extent', 'wp:docPr', 'a:graphic'], + generated: [ + { name: 'wp:extent', attributes: { cx: 100 } }, + { name: 'wp:docPr', attributes: { id: 42, name: 'Picture 1' } }, + { name: 'a:graphic', attributes: {} }, + ], + original: [ + { index: 1, xml: { name: 'wp:docPr', attributes: { id: 0, name: 'Picture 1', descr: 'alt text' } } }, + { index: 2, xml: { name: 'a:graphic', attributes: {} } }, + ], + }); + + const docPr = result.find((el) => el.name === 'wp:docPr'); + expect(docPr.attributes.id).toBe(42); + expect(docPr.attributes.descr).toBe('alt text'); + }); + + it('patches pic:cNvPr id=0 inside original a:graphic subtree', () => { + const result = mergeDrawingChildren({ + order: ['wp:extent', 'wp:docPr', 'a:graphic'], + generated: [ + { name: 'wp:extent', attributes: { cx: 100 } }, + { name: 'wp:docPr', attributes: { id: 7 } }, + { + name: 'a:graphic', + elements: [ + { + name: 'a:graphicData', + elements: [ + { + name: 'pic:pic', + elements: [ + { + name: 'pic:nvPicPr', + elements: [{ name: 'pic:cNvPr', attributes: { id: 7 } }], + }, + ], + }, + ], + }, + ], + }, + ], + original: [ + { index: 1, xml: { name: 'wp:docPr', attributes: { id: 0 } } }, + { + index: 2, + xml: { + name: 'a:graphic', + elements: [ + { + name: 'a:graphicData', + elements: [ + { + name: 'pic:pic', + elements: [ + { + name: 'pic:nvPicPr', + elements: [{ name: 'pic:cNvPr', attributes: { id: 0, name: 'Original' } }], + }, + ], + }, + ], + }, + ], + }, + }, + ], + }); + + const graphic = result.find((el) => el.name === 'a:graphic'); + const cNvPr = graphic.elements[0].elements[0].elements[0].elements[0]; + expect(cNvPr.attributes.id).toBe(7); + expect(cNvPr.attributes.name).toBe('Original'); + }); + + it('does not overwrite valid positive IDs on originals', () => { + const result = mergeDrawingChildren({ + order: ['wp:extent', 'wp:docPr'], + generated: [ + { name: 'wp:extent', attributes: {} }, + { name: 'wp:docPr', attributes: { id: 99 } }, + ], + original: [{ index: 1, xml: { name: 'wp:docPr', attributes: { id: 5 } } }], + }); + + const docPr = result.find((el) => el.name === 'wp:docPr'); + expect(docPr.attributes.id).toBe(5); + }); + + it('handles missing generated wp:docPr gracefully', () => { + const result = mergeDrawingChildren({ + order: ['wp:extent', 'wp:docPr'], + generated: [{ name: 'wp:extent', attributes: {} }], + original: [{ index: 1, xml: { name: 'wp:docPr', attributes: { id: 0 } } }], + }); + + const docPr = result.find((el) => el.name === 'wp:docPr'); + expect(docPr.attributes.id).toBe(0); + }); + }); + describe('deep copy behavior', () => { it('returns deep copies, not references to original objects', () => { const originalXml = { name: 'wp:docPr', attributes: { id: 1 } }; diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/wp/inline/helpers/translate-inline-node.js b/packages/super-editor/src/core/super-converter/v3/handlers/wp/inline/helpers/translate-inline-node.js index d2d92a6b12..1bc399fb07 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/wp/inline/helpers/translate-inline-node.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/wp/inline/helpers/translate-inline-node.js @@ -10,6 +10,11 @@ export function translateInlineNode(params) { const { attrs } = params.node; const nodeElements = translateImageNode(params); + // Guard: bail out if translateImageNode produced a non-drawing result (e.g. text fallback). + if (!nodeElements?.elements?.some((el) => el?.name === 'wp:extent')) { + return nodeElements; + } + const inlineAttrs = { ...(attrs.originalAttributes || {}), ...(nodeElements.attributes || {}), diff --git a/packages/super-editor/src/tests/export/drawing-guards.test.js b/packages/super-editor/src/tests/export/drawing-guards.test.js new file mode 100644 index 0000000000..56256e3c04 --- /dev/null +++ b/packages/super-editor/src/tests/export/drawing-guards.test.js @@ -0,0 +1,126 @@ +import { describe, it, expect } from 'vitest'; +import { translateInlineNode } from '@core/super-converter/v3/handlers/wp/inline/helpers/translate-inline-node.js'; +import { translateAnchorNode } from '@core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.js'; +import { translateImageNode } from '@core/super-converter/v3/handlers/wp/helpers/decode-image-node-helpers.js'; + +/** Helper to build minimal params for translateImageNode / translateInlineNode */ +function makeImageParams(overrides = {}) { + return { + node: { + type: 'image', + attrs: { + src: 'word/media/image1.png', + size: { width: 64, height: 64 }, + ...overrides, + }, + }, + media: {}, + relationships: [], + converter: { convertedXml: {} }, + }; +} + +describe('translateInlineNode guard', () => { + it('returns wp:inline for valid image nodes', () => { + const result = translateInlineNode(makeImageParams()); + + expect(result).toBeTruthy(); + expect(result.name).toBe('wp:inline'); + expect(result.elements.some((el) => el.name === 'wp:extent')).toBe(true); + }); + + it('returns text run when signature fallback produces no drawing', () => { + const params = { + node: { + type: 'fieldAnnotation', + attrs: { + type: 'signature', + src: 'data:,', + displayLabel: 'Signature', + }, + }, + media: {}, + relationships: [], + converter: { convertedXml: {} }, + editor: { extensionService: { extensions: [] } }, + }; + + const result = translateInlineNode(params); + + expect(result).toBeTruthy(); + expect(result.name).toBe('w:r'); + }); +}); + +describe('translateAnchorNode guard', () => { + it('returns text run when signature fallback produces no drawing', () => { + const params = { + node: { + type: 'fieldAnnotation', + attrs: { + type: 'signature', + src: 'data:,', + displayLabel: 'Signature', + isAnchor: true, + }, + }, + media: {}, + relationships: [], + converter: { convertedXml: {} }, + editor: { extensionService: { extensions: [] } }, + }; + + const result = translateAnchorNode(params); + + expect(result).toBeTruthy(); + expect(result.name).toBe('w:r'); + }); +}); + +describe('translateImageNode IDs', () => { + function findCNvPr(inline) { + const graphic = inline.elements.find((el) => el.name === 'a:graphic'); + const graphicData = graphic.elements.find((el) => el.name === 'a:graphicData'); + const pic = graphicData.elements.find((el) => el.name === 'pic:pic'); + const nvPicPr = pic.elements.find((el) => el.name === 'pic:nvPicPr'); + return nvPicPr.elements.find((el) => el.name === 'pic:cNvPr'); + } + + it('generates non-zero IDs for wp:docPr and pic:cNvPr when attrs.id is missing', () => { + const inline = translateImageNode(makeImageParams()); + + const docPr = inline.elements.find((el) => el.name === 'wp:docPr'); + expect(Number(docPr.attributes.id)).toBeGreaterThan(0); + + const cNvPr = findCNvPr(inline); + expect(Number(cNvPr.attributes.id)).toBeGreaterThan(0); + }); + + it('generates non-zero IDs when attrs.id is explicitly 0', () => { + const inline = translateImageNode(makeImageParams({ id: 0 })); + + const docPr = inline.elements.find((el) => el.name === 'wp:docPr'); + expect(Number(docPr.attributes.id)).toBeGreaterThan(0); + + const cNvPr = findCNvPr(inline); + expect(Number(cNvPr.attributes.id)).toBeGreaterThan(0); + }); + + it('uses same ID for both wp:docPr and pic:cNvPr', () => { + const inline = translateImageNode(makeImageParams()); + + const docPr = inline.elements.find((el) => el.name === 'wp:docPr'); + const cNvPr = findCNvPr(inline); + expect(docPr.attributes.id).toBe(cNvPr.attributes.id); + }); + + it('preserves provided positive ID', () => { + const inline = translateImageNode(makeImageParams({ id: 42 })); + + const docPr = inline.elements.find((el) => el.name === 'wp:docPr'); + expect(Number(docPr.attributes.id)).toBe(42); + + const cNvPr = findCNvPr(inline); + expect(Number(cNvPr.attributes.id)).toBe(42); + }); +});