diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js index 1825db4489..5e1c7bbef1 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.js @@ -6,6 +6,13 @@ import { generateParagraphProperties } from './generate-paragraph-properties.js' * Comment range markers between tracked changes with the same ID are included * inside the merged wrapper, matching Word's OOXML structure. * + * AIDEV-NOTE: Comment markers (w:commentRangeStart/End and w:r→w:commentReference) + * are only absorbed into the wrapper when a same-id merge actually happens. + * If a tracked change has no matching successor, trailing comment markers are + * preserved as siblings so the import side can re-pair the comment range with + * the wrapped text. Otherwise w:commentRangeEnd ends up inside w:del while + * w:commentRangeStart is outside it, breaking the round-trip (SD-2528). + * * See SD-1519 for details on the ECMA-376 spec compliance. * * @param {Array} elements The translated paragraph elements @@ -14,59 +21,55 @@ import { generateParagraphProperties } from './generate-paragraph-properties.js' function mergeConsecutiveTrackedChanges(elements) { if (!Array.isArray(elements) || elements.length === 0) return elements; + const isCommentMarker = (el) => { + if (!el) return false; + if (el.name === 'w:commentRangeStart' || el.name === 'w:commentRangeEnd') return true; + if (el.name === 'w:r' && el.elements?.length === 1 && el.elements[0]?.name === 'w:commentReference') return true; + return false; + }; + const result = []; let i = 0; while (i < elements.length) { const current = elements[i]; - // Check if this is a tracked change wrapper (w:ins or w:del) if (current?.name === 'w:ins' || current?.name === 'w:del') { const tcId = current.attributes?.['w:id']; const tcName = current.name; - // Collect consecutive elements that belong to this tracked change const mergedElements = [...(current.elements || [])]; + const pendingComments = []; + let didMerge = false; let j = i + 1; while (j < elements.length) { const next = elements[j]; - // Include comment markers - they can sit inside tracked changes per ECMA-376 - if (next?.name === 'w:commentRangeStart' || next?.name === 'w:commentRangeEnd') { - mergedElements.push(next); + if (isCommentMarker(next)) { + pendingComments.push(next); j++; continue; } - // Include comment references (w:r containing w:commentReference) - if (next?.name === 'w:r') { - const hasOnlyCommentRef = next.elements?.length === 1 && next.elements[0]?.name === 'w:commentReference'; - if (hasOnlyCommentRef) { - mergedElements.push(next); - j++; - continue; - } - } - - // Merge with next tracked change if same type and ID if (next?.name === tcName && next.attributes?.['w:id'] === tcId) { - mergedElements.push(...(next.elements || [])); + mergedElements.push(...pendingComments, ...(next.elements || [])); + pendingComments.length = 0; + didMerge = true; j++; continue; } - // Stop merging when we hit a different element break; } - // Create the merged wrapper - result.push({ - name: tcName, - attributes: { ...current.attributes }, - elements: mergedElements, - }); - + if (didMerge) { + result.push({ name: tcName, attributes: { ...current.attributes }, elements: mergedElements }); + result.push(...pendingComments); + } else { + result.push(current); + result.push(...pendingComments); + } i = j; } else { result.push(current); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js index 4b40bb4f83..38b68f5526 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/p/helpers/translate-paragraph-node.test.js @@ -82,4 +82,82 @@ describe('translateParagraphNode', () => { }); expect(generateParagraphProperties).toHaveBeenCalledWith(params); }); + + describe('mergeConsecutiveTrackedChanges (via translateParagraphNode)', () => { + const helloRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ type: 'text', text: 'Hell' }] }] }; + const commentRangeStart = { name: 'w:commentRangeStart', attributes: { 'w:id': '0' } }; + const commentRangeEnd = { name: 'w:commentRangeEnd', attributes: { 'w:id': '0' } }; + const commentReferenceRun = { + name: 'w:r', + elements: [{ name: 'w:commentReference', attributes: { 'w:id': '0' } }], + }; + const restRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ type: 'text', text: ' rest' }] }] }; + + const buildDelWrapper = (id, innerText) => ({ + name: 'w:del', + attributes: { 'w:id': id, 'w:author': 'a', 'w:date': 'd' }, + elements: [{ name: 'w:r', elements: [{ name: 'w:delText', elements: [{ type: 'text', text: innerText }] }] }], + }); + + it('keeps trailing comment markers as siblings when no same-id wrapper follows (SD-2528)', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + translateChildNodes.mockReturnValue([ + helloRun, + commentRangeStart, + buildDelWrapper('1', 'o worl'), + commentRangeEnd, + commentReferenceRun, + restRun, + ]); + + const result = translateParagraphNode(params); + const names = result.elements.map((e) => e.name); + + expect(names).toEqual(['w:r', 'w:commentRangeStart', 'w:del', 'w:commentRangeEnd', 'w:r', 'w:r']); + + const delNode = result.elements.find((e) => e.name === 'w:del'); + expect(delNode.elements.some((e) => e.name === 'w:commentRangeEnd')).toBe(false); + expect(delNode.elements.some((e) => e.name === 'w:commentReference')).toBe(false); + // The inner commentReference run check: delNode's children should be the original delText run only + expect(delNode.elements).toHaveLength(1); + }); + + it('merges two consecutive same-id tracked changes and absorbs comment markers between them (SD-1519)', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + translateChildNodes.mockReturnValue([ + buildDelWrapper('5', 'first'), + commentRangeEnd, + commentReferenceRun, + buildDelWrapper('5', 'second'), + ]); + + const result = translateParagraphNode(params); + + expect(result.elements).toHaveLength(1); + const delNode = result.elements[0]; + expect(delNode.name).toBe('w:del'); + const innerNames = delNode.elements.map((e) => e.name); + // first delText run, commentRangeEnd, commentReference run, second delText run + expect(innerNames).toEqual(['w:r', 'w:commentRangeEnd', 'w:r', 'w:r']); + }); + + it('does not merge wrappers with different ids and keeps comment markers between them as siblings', () => { + const params = baseParams(); + generateParagraphProperties.mockReturnValue(null); + translateChildNodes.mockReturnValue([ + buildDelWrapper('1', 'first'), + commentRangeEnd, + buildDelWrapper('2', 'second'), + ]); + + const result = translateParagraphNode(params); + const names = result.elements.map((e) => e.name); + + expect(names).toEqual(['w:del', 'w:commentRangeEnd', 'w:del']); + expect(result.elements[0].elements).toHaveLength(1); + expect(result.elements[2].elements).toHaveLength(1); + }); + }); });