diff --git a/coverage/emitted-schema-known-failures.json b/coverage/emitted-schema-known-failures.json index ee3e96a8..7abd5547 100644 --- a/coverage/emitted-schema-known-failures.json +++ b/coverage/emitted-schema-known-failures.json @@ -1,10 +1,4 @@ [ - { - "id": "duplicate-para-mark-ins", - "issue": "#452", - "match": "}ins': This element is not expected.", - "reason": "tracked paragraph insertion emits two paragraph-mark w:ins markers in w:pPr/w:rPr; CT_ParaRPr allows at most one" - }, { "id": "synthetic-table-missing-tblgrid", "issue": "#453", diff --git a/packages/docx-core/src/baselines/atomizer/documentReconstructor-rpr.test.ts b/packages/docx-core/src/baselines/atomizer/documentReconstructor-rpr.test.ts index 48186d92..44b05f2b 100644 --- a/packages/docx-core/src/baselines/atomizer/documentReconstructor-rpr.test.ts +++ b/packages/docx-core/src/baselines/atomizer/documentReconstructor-rpr.test.ts @@ -14,7 +14,7 @@ import { rejectAllChanges, } from './trackChangesAcceptorAst.js'; import { parseDocumentXml } from './xmlToWmlElement.js'; -import { findAllByTagName, findChildByTagName } from '../../primitives/index.js'; +import { childElements, findAllByTagName, findChildByTagName } from '../../primitives/index.js'; import { EMPTY_PARAGRAPH_TAG } from '../../atomizer.js'; import type { ComparisonUnitAtom, OpcPart } from '../../core-types.js'; import { CorrelationStatus } from '../../core-types.js'; @@ -535,6 +535,63 @@ describe('Step 7: Rebuild path emits pPrChange for inserted paragraphs', () => { expect(innerChildren.length).toBe(0); }); }); + + test('whole-paragraph insertion reuses and reorders an existing paragraph-mark w:ins', async ({ given, when, then }: AllureBddContext) => { + let atom: ComparisonUnitAtom; + let result: string; + + await given('an inserted atom whose source pPr already has a paragraph-mark insertion after w:del', () => { + const existingIns = el('w:ins', { + 'w:id': '77', + 'w:author': 'Source Author', + 'w:date': '2020-01-01T00:00:00Z', + }); + const rPr = el('w:rPr', {}, [ + el('w:del', { + 'w:id': '88', + 'w:author': 'Delete Author', + 'w:date': '2020-01-02T00:00:00Z', + }), + existingIns, + ]); + const pPrEl = el('w:pPr', {}, [rPr]); + const textEl = el('w:t', {}, undefined, 'preserved inserted paragraph'); + const run = el('w:r', {}, [textEl]); + const paragraph = el('w:p', {}, [pPrEl, run]); + + atom = { + sha1Hash: 'hash-existing-ins', + correlationStatus: CorrelationStatus.Inserted, + contentElement: textEl, + ancestorElements: [paragraph, run], + ancestorUnids: [], + part: PART, + paragraphIndex: 0, + rPr: null, + }; + }); + + await when('reconstructDocument serializes the paragraph-mark insertion', () => { + result = reconstructDocument([atom], MINIMAL_DOCXML, OPTS); + }); + + await then('the output has one normalized w:ins before w:del in pPr/rPr', () => { + const root = parseDocumentXml(result); + const pPr = findAllByTagName(root, 'w:pPr').find((candidate) => { + const rPr = findChildByTagName(candidate, 'w:rPr'); + return !!rPr && !!findChildByTagName(rPr, 'w:ins'); + }); + expect(pPr).toBeDefined(); + + const rPr = findChildByTagName(pPr!, 'w:rPr')!; + const markers = findAllByTagName(pPr!, 'w:ins'); + expect(markers).toHaveLength(1); + expect(markers[0]!.getAttribute('w:id')).toBe('77'); + expect(markers[0]!.getAttribute('w:author')).toBe('Test'); + expect(markers[0]!.getAttribute('w:date')).toBe('2025-01-01T00:00:00Z'); + expect(childElements(rPr).map((child) => child.tagName)).toEqual(['w:ins', 'w:del']); + }); + }); }); // ============================================================================= diff --git a/packages/docx-core/src/baselines/atomizer/documentReconstructor.ts b/packages/docx-core/src/baselines/atomizer/documentReconstructor.ts index b2b01e39..146ec8f1 100644 --- a/packages/docx-core/src/baselines/atomizer/documentReconstructor.ts +++ b/packages/docx-core/src/baselines/atomizer/documentReconstructor.ts @@ -25,6 +25,10 @@ import { import { serializeToXml, cloneElement } from './xmlToWmlElement.js'; import { EMPTY_PARAGRAPH_TAG, isParagraphLevelLeaf, nearestHyperlinkAncestor } from '../../atomizer.js'; import { enforceConsumerCompatibility } from './consumerCompatibility.js'; +import { + findParagraphMarkRevisionMarker, + placeParagraphMarkRevisionMarker, +} from './inPlaceModifier-wrappers.js'; import { areRunPropertiesEqual } from '../../format-detection.js'; import { debug } from './debug.js'; @@ -751,13 +755,26 @@ function serializePPrWithParaRevisionMarker( } } - // Insert revision marker at start of rPr. - const marker = createEl(markerTag, { - 'w:id': String(id), - 'w:author': author, - 'w:date': dateStr, - }); - rPr.insertBefore(marker, rPr.firstChild); + // Reuse a pre-existing paragraph-mark marker of the same kind cloned from the + // source pPr (issue #452): CT_ParaRPr allows at most one of each tracked-change + // child. Either way the marker is normalized and placed in its schema-correct + // slot ahead of formatting children. + const existingMarker = findParagraphMarkRevisionMarker(effectivePPr, markerTag); + const marker = + existingMarker ?? + createEl(markerTag, { + 'w:id': String(id), + 'w:author': author, + 'w:date': dateStr, + }); + if (existingMarker) { + existingMarker.setAttribute('w:author', author); + existingMarker.setAttribute('w:date', dateStr); + if (!existingMarker.getAttribute('w:id')) { + existingMarker.setAttribute('w:id', String(id)); + } + } + placeParagraphMarkRevisionMarker(rPr, marker, markerTag); // Append pPrChange at end if provided. if (pPrChangeEl) { diff --git a/packages/docx-core/src/baselines/atomizer/inPlaceModifier-wrappers.ts b/packages/docx-core/src/baselines/atomizer/inPlaceModifier-wrappers.ts index 5c81380b..e8e67ddf 100644 --- a/packages/docx-core/src/baselines/atomizer/inPlaceModifier-wrappers.ts +++ b/packages/docx-core/src/baselines/atomizer/inPlaceModifier-wrappers.ts @@ -181,6 +181,8 @@ export function addParagraphMarkRevisionMarker( paragraph.insertBefore(pPr, paragraph.firstChild); } + const existingMarker = findParagraphMarkRevisionMarker(pPr, markerTag); + // Find or create rPr within pPr (paragraph mark properties). let rPr = findChildByTagName(pPr, 'w:rPr'); if (!rPr) { @@ -197,8 +199,20 @@ export function addParagraphMarkRevisionMarker( } } - // Avoid duplicating markers. - if (findChildByTagName(rPr, markerTag)) return; + // Avoid duplicating markers. A legacy/bypass path may already have put the + // paragraph-mark marker in another w:rPr under the same pPr; keep that marker + // and normalize its revision context instead of adding a second CT_ParaRPr child. + if (existingMarker) { + existingMarker.setAttribute('w:author', author); + existingMarker.setAttribute('w:date', dateStr); + if (!existingMarker.getAttribute('w:id')) { + existingMarker.setAttribute('w:id', String(allocateRevisionId(state))); + } + // The bypass path may have left the marker mid-sequence (or in another + // w:rPr); move it to the schema-correct slot in the canonical rPr. + placeParagraphMarkRevisionMarker(rPr, existingMarker, markerTag); + return; + } const id = allocateRevisionId(state); const marker = createEl(markerTag, { @@ -207,8 +221,42 @@ export function addParagraphMarkRevisionMarker( 'w:date': dateStr, }); - // Insert marker at the start of rPr for consistency with Aspose/Word patterns. - rPr.insertBefore(marker, rPr.firstChild); + placeParagraphMarkRevisionMarker(rPr, marker, markerTag); +} + +/** + * Position a paragraph-mark revision marker in its schema-correct rPr slot. + * + * CT_ParaRPr ordering: the tracked-change group (w:ins, w:del, w:moveFrom, + * w:moveTo — in that order) comes before every formatting child (w:rStyle, + * w:rFonts, ...). So w:ins always goes first, and w:del goes right after a + * w:ins sibling when one exists, else first. + */ +export function placeParagraphMarkRevisionMarker( + rPr: Element, + marker: Element, + markerTag: 'w:ins' | 'w:del' +): void { + const insSibling = markerTag === 'w:del' ? findChildByTagName(rPr, 'w:ins') : null; + if (insSibling) { + if (insSibling.nextSibling !== marker) { + insertAfterElement(insSibling, marker); + } + } else if (rPr.firstChild !== marker) { + rPr.insertBefore(marker, rPr.firstChild); + } +} + +export function findParagraphMarkRevisionMarker( + pPr: Element, + markerTag: 'w:ins' | 'w:del' +): Element | null { + for (const child of childElements(pPr)) { + if (child.tagName !== 'w:rPr') continue; + const marker = findChildByTagName(child, markerTag); + if (marker) return marker; + } + return null; } // @lean-segment: field-wrapper-emission diff --git a/packages/docx-core/src/baselines/atomizer/inPlaceModifier.test.ts b/packages/docx-core/src/baselines/atomizer/inPlaceModifier.test.ts index 0a8e5983..dabccfee 100644 --- a/packages/docx-core/src/baselines/atomizer/inPlaceModifier.test.ts +++ b/packages/docx-core/src/baselines/atomizer/inPlaceModifier.test.ts @@ -813,6 +813,75 @@ describe('inPlaceModifier', () => { expect(insMarker).toBeDefined(); }); }); + + test('reuses an existing paragraph-mark insertion and normalizes revision context', async ({ given, when, then }: AllureBddContext) => { + let pPr: Element, p: Element; + let state: ReturnType; + + await given('a paragraph with a pre-existing paragraph-mark w:ins from a bypass path', () => { + // The bypass marker sits AFTER a formatting child — an invalid + // CT_ParaRPr sequence that reuse must repair, not preserve. + pPr = el('w:pPr', {}, [ + el('w:rPr', {}, [ + el('w:b'), + el('w:ins', { + 'w:id': '99', + 'w:author': 'Bypass', + 'w:date': '2020-01-01T00:00:00Z', + }), + ]), + ]); + p = el('w:p', {}, [pPr, el('w:r', {}, [el('w:t', {}, undefined, 'text')])]); + state = createRevisionIdState(); + }); + + await when('wrapParagraphAsInserted applies the comparison revision context', () => { + wrapParagraphAsInserted(p, author, dateStr, state); + }); + + await then('only one paragraph-mark w:ins remains, first in rPr, with the comparison author and date', () => { + const markers = childElements(pPr) + .filter((c) => c.tagName === 'w:rPr') + .flatMap((rPr) => childElements(rPr).filter((c) => c.tagName === 'w:ins')); + expect(markers).toHaveLength(1); + expect(markers[0]!.getAttribute('w:id')).toBe('99'); + expect(markers[0]!.getAttribute('w:author')).toBe(author); + expect(markers[0]!.getAttribute('w:date')).toBe(dateStr); + + // CT_ParaRPr puts the tracked-change group before formatting children, + // so the reused w:ins must be moved to the front of rPr. + const rPr = childElements(pPr).find((c) => c.tagName === 'w:rPr'); + expect(childElements(rPr!).map((c) => c.tagName)).toEqual(['w:ins', 'w:b']); + }); + }); + + test('keeps the paragraph-mark w:del after w:ins per CT_ParaRPr order', async ({ given, when, then }: AllureBddContext) => { + let pPr: Element, p: Element; + let state: ReturnType; + + await given('a paragraph whose mark already carries a w:ins marker', () => { + pPr = el('w:pPr', {}, [ + el('w:rPr', {}, [ + el('w:ins', { + 'w:id': '7', + 'w:author': 'Bypass', + 'w:date': '2020-01-01T00:00:00Z', + }), + ]), + ]); + p = el('w:p', {}, [pPr, el('w:r', {}, [el('w:t', {}, undefined, 'text')])]); + state = createRevisionIdState(); + }); + + await when('wrapParagraphAsDeleted marks the paragraph mark as deleted', () => { + wrapParagraphAsDeleted(p, author, dateStr, state); + }); + + await then('rPr orders the tracked-change group w:ins then w:del', () => { + const rPr = childElements(pPr).find((c) => c.tagName === 'w:rPr'); + expect(childElements(rPr!).map((c) => c.tagName)).toEqual(['w:ins', 'w:del']); + }); + }); }); describe('addParagraphPropertyChange', () => { diff --git a/packages/docx-core/src/integration/canonical-emission-regression.test.ts b/packages/docx-core/src/integration/canonical-emission-regression.test.ts index 5920af91..5e79e208 100644 --- a/packages/docx-core/src/integration/canonical-emission-regression.test.ts +++ b/packages/docx-core/src/integration/canonical-emission-regression.test.ts @@ -556,7 +556,7 @@ describe('Round-trip with comparison', () => { }); }); - test('insert_paragraph round-trip retains the original AI paragraph-mark insertion alongside comparison output', async ({ + test('insert_paragraph round-trip keeps SafeDocX paragraph-mark insertion metadata normalized', async ({ given, when, then, @@ -585,9 +585,10 @@ describe('Round-trip with comparison', () => { }); }); - await then('comparison output keeps the original SafeDocX paragraph-mark metadata and avoids Comparison', () => { + await then('comparison output keeps SafeDocX metadata, avoids Comparison, and replaces the stale fixed date', () => { expectNoComparisonAuthor(comparedDocumentXml); - expect(comparedDocumentXml).toContain(`w:date="${FIXED_DATE}"`); + expect(comparedDocumentXml).not.toContain(`w:date="${FIXED_DATE}"`); + expect(revisionTuples(comparedDocumentXml, AI_AUTHOR).length).toBeGreaterThan(0); expect(comparedDocumentXml).toContain('Inserted paragraph'); }); });