From 9a551cac136dc2d2fc4509c40158c3100983685a Mon Sep 17 00:00:00 2001 From: Steven Obiajulu Date: Thu, 17 Sep 2026 13:07:20 -0500 Subject: [PATCH 1/2] fix(docx-core): clean resolved terminal mark properties Native terminal mark resolution left vacated paragraph-mark property containers while the AST projection removed them. Clean only selected resolved terminal marks, preserve attributes, remaining glyph formatting, foreign history, content-bearing terminal paragraphs, and the required final table-cell paragraph. Twelve independent body/cell and selective-author controls establish the intended bounded cleanup; no content-only paragraph deletion rule is introduced. Fixes: #982 --- .../tagged/terminalMarkCleanupParity.test.ts | 55 +++++++++++++++++++ .../src/primitives/accept_changes.ts | 3 +- .../primitives/paragraph_merge_formatting.ts | 13 +++++ .../src/primitives/reject_changes.ts | 3 +- 4 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts diff --git a/packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts b/packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts new file mode 100644 index 00000000..80e4166c --- /dev/null +++ b/packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts @@ -0,0 +1,55 @@ +import { describe, expect } from 'vitest'; +import { XMLSerializer } from '@xmldom/xmldom'; +import { testAllure } from '../testing/allure-test.js'; +import { parseXml, serializeXml, acceptChanges, rejectChanges } from '@usejunior/docx-core'; +import { acceptAllChanges, rejectAllChanges } from './trackChangesAcceptorAst.js'; +const test = testAllure.epic('Document Comparison').withLabels({ feature: 'Terminal mark cleanup parity' }) + .conformance({ spec: 'ECMA-376', edition: 5, part: 1, section: '17.13.5.20' }); +const W = 'http://schemas.openxmlformats.org/wordprocessingml/2006/main'; +const subtree = (element: Element) => new XMLSerializer().serializeToString(element); +const wrap = (body: string) => `${body}`; +const paragraph = (kind: string, color: boolean, text: boolean) => '' + + `${color ? '' : ''}` + + `${text ? 'Surviving text' : ''}`; +const cell = (p: string) => '' + p + ''; + +describe('resolved terminal paragraph-mark properties', () => { + for (const [name, kind, nativeProject, astProject] of [ + ['Accept', 'del', acceptChanges, acceptAllChanges], ['Reject', 'ins', rejectChanges, rejectAllChanges], + ] as const) { + for (const inCell of [false, true]) { + for (const color of [false, true]) { + test(`${name} terminal ${inCell ? 'cell' : 'body'} mark ${color ? 'retains color' : 'drops empty rPr'}`, () => { + const p = paragraph(kind, color, true); + const input = wrap(inCell ? cell(p) : p); + const doc = parseXml(input); + nativeProject(doc); + expect(serializeXml(doc)).toBe(serializeXml(parseXml(astProject(input)))); + expect(doc.getElementsByTagNameNS(W, 'p').length).toBe(1); + expect(doc.getElementsByTagNameNS(W, 't')[0]!.textContent).toBe('Surviving text'); + expect(doc.getElementsByTagNameNS(W, 'rPr').length).toBe(color ? 1 : 0); + }); + } + } + + test(`${name} preserves the required empty last table-cell paragraph`, () => { + const input = wrap(cell(paragraph(kind, false, false))); + const doc = parseXml(input); + nativeProject(doc); + expect(doc.getElementsByTagNameNS(W, 'p').length).toBe(1); + expect(doc.getElementsByTagNameNS(W, 'rPr').length).toBe(0); + expect(serializeXml(doc)).toBe(serializeXml(parseXml(astProject(input)))); + }); + + test(`${name} resolves one author while preserving a foreign terminal mark and formatting`, () => { + const foreign = paragraph(kind, true, true).replace('w:author="AI"', 'w:author="Human"'); + const input = wrap(cell(paragraph(kind, false, true)) + cell(foreign)); + const doc = parseXml(input); + const foreignBefore = subtree(doc.getElementsByTagNameNS(W, 'p')[1]!); + nativeProject(doc, { filter: e => e.getAttributeNS(W, 'author') === 'AI' }); + expect(subtree(doc.getElementsByTagNameNS(W, 'p')[1]!)).toBe(foreignBefore); + expect(doc.getElementsByTagNameNS(W, 'p')[0]!.getElementsByTagNameNS(W, 'rPr').length).toBe(0); + expect(doc.getElementsByTagNameNS(W, 'p').length).toBe(2); + }); + } +}); diff --git a/packages/docx-core/src/primitives/accept_changes.ts b/packages/docx-core/src/primitives/accept_changes.ts index 63f6ed1e..43cf49e8 100644 --- a/packages/docx-core/src/primitives/accept_changes.ts +++ b/packages/docx-core/src/primitives/accept_changes.ts @@ -20,7 +20,7 @@ */ import { OOXML } from './namespaces.js'; -import { retainLeadingParagraphFormatting, isEmptyParagraphFormattingRun } from './paragraph_merge_formatting.js'; +import { retainLeadingParagraphFormatting, isEmptyParagraphFormattingRun, removeEmptyParagraphMarkProperties } from './paragraph_merge_formatting.js'; const W_NS = OOXML.W_NS; @@ -289,6 +289,7 @@ function resolveParagraphMarkRevision(p: Element): void { const target = findFollowingSiblingParagraph(p); if (!target) { + removeEmptyParagraphMarkProperties(p); if (!paragraphHasContent(p) && canSafelyRemoveEmptyParagraph(p)) { parent.removeChild(p); } diff --git a/packages/docx-core/src/primitives/paragraph_merge_formatting.ts b/packages/docx-core/src/primitives/paragraph_merge_formatting.ts index 68edd113..381585ce 100644 --- a/packages/docx-core/src/primitives/paragraph_merge_formatting.ts +++ b/packages/docx-core/src/primitives/paragraph_merge_formatting.ts @@ -6,6 +6,19 @@ const children = (node: Element): Element[] => Array.from(node.childNodes) const properties = (paragraph: Element): Element | undefined => children(paragraph) .find(child => child.namespaceURI === W && child.localName === 'pPr'); +/** Remove vacated mark-property containers after resolving a terminal mark. + * @internal + * @conformance ECMA-376 edition 5, Part 1 § 17.3.1.26 + * @see https://github.com/UseJunior/safe-docx/issues/982 + */ +export function removeEmptyParagraphMarkProperties(paragraph: Element): void { + const pPr = properties(paragraph); + if (!pPr) return; + const rPr = children(pPr).find(child => child.namespaceURI === W && child.localName === 'rPr'); + if (rPr && children(rPr).length === 0 && !rPr.textContent?.trim() && rPr.attributes.length === 0) pPr.removeChild(rPr); + if (children(pPr).length === 0 && !pPr.textContent?.trim() && pPr.attributes.length === 0) paragraph.removeChild(pPr); +} + /** Formatting-only test; never use this to decide whether to remove a paragraph. */ export function isEmptyParagraphFormattingRun(element: Element): boolean { return element.namespaceURI === W && element.localName === 'r' && children(element).every(child => diff --git a/packages/docx-core/src/primitives/reject_changes.ts b/packages/docx-core/src/primitives/reject_changes.ts index 575ee935..8d94badd 100644 --- a/packages/docx-core/src/primitives/reject_changes.ts +++ b/packages/docx-core/src/primitives/reject_changes.ts @@ -19,7 +19,7 @@ */ import { OOXML } from './namespaces.js'; -import { retainLeadingParagraphFormatting, isEmptyParagraphFormattingRun } from './paragraph_merge_formatting.js'; +import { retainLeadingParagraphFormatting, isEmptyParagraphFormattingRun, removeEmptyParagraphMarkProperties } from './paragraph_merge_formatting.js'; import type { RevisionFilter } from './accept_changes.js'; const W_NS = OOXML.W_NS; @@ -290,6 +290,7 @@ function resolveParagraphMarkRevision(p: Element): void { const target = findFollowingSiblingParagraph(p); if (!target) { + removeEmptyParagraphMarkProperties(p); if (!paragraphHasContent(p) && canSafelyRemoveEmptyParagraph(p)) { parent.removeChild(p); } From 7d0a337309eddeffafd3a6081aef536e010416a9 Mon Sep 17 00:00:00 2001 From: Steven Obiajulu Date: Thu, 17 Sep 2026 13:31:30 -0500 Subject: [PATCH 2/2] fix(docx-core): preserve metadata during selected mark cleanup The initial terminal cleanup treated XML comments and processing instructions as empty and missed mirror revision directions. Capture selected paragraph-mark histories before resolution, then remove only attribute-free containers containing no nodes beyond blank text. Apply the same conservative vacancy rule to AST projection rather than using agreement with its inherited metadata loss as proof. Independent controls now cover all four ins/del directions, comments, processing instructions, and history-only selection while keeping foreign metadata untouched. Ref: #982 --- .../tagged/terminalMarkCleanupParity.test.ts | 33 ++++++++++++++++--- .../src/tagged/trackChangesAcceptorAst.ts | 6 ++-- .../src/primitives/accept_changes.ts | 5 +++ .../primitives/paragraph_merge_formatting.ts | 10 +++--- .../src/primitives/reject_changes.ts | 3 ++ 5 files changed, 46 insertions(+), 11 deletions(-) diff --git a/packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts b/packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts index 80e4166c..4c871f73 100644 --- a/packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts +++ b/packages/docx-compare/src/tagged/terminalMarkCleanupParity.test.ts @@ -3,8 +3,7 @@ import { XMLSerializer } from '@xmldom/xmldom'; import { testAllure } from '../testing/allure-test.js'; import { parseXml, serializeXml, acceptChanges, rejectChanges } from '@usejunior/docx-core'; import { acceptAllChanges, rejectAllChanges } from './trackChangesAcceptorAst.js'; -const test = testAllure.epic('Document Comparison').withLabels({ feature: 'Terminal mark cleanup parity' }) - .conformance({ spec: 'ECMA-376', edition: 5, part: 1, section: '17.13.5.20' }); +const test = testAllure.epic('Document Comparison').withLabels({ feature: 'Terminal mark cleanup parity' }); const W = 'http://schemas.openxmlformats.org/wordprocessingml/2006/main'; const subtree = (element: Element) => new XMLSerializer().serializeToString(element); const wrap = (body: string) => `${body}`; @@ -16,10 +15,12 @@ const cell = (p: string) => ' describe('resolved terminal paragraph-mark properties', () => { for (const [name, kind, nativeProject, astProject] of [ ['Accept', 'del', acceptChanges, acceptAllChanges], ['Reject', 'ins', rejectChanges, rejectAllChanges], + ['Accept inserted', 'ins', acceptChanges, acceptAllChanges], ['Reject deleted', 'del', rejectChanges, rejectAllChanges], ] as const) { + const revisionTest = test.conformance({ spec: 'ECMA-376', edition: 5, part: 1, section: kind === 'del' ? '17.13.5.15' : '17.13.5.20' }); for (const inCell of [false, true]) { for (const color of [false, true]) { - test(`${name} terminal ${inCell ? 'cell' : 'body'} mark ${color ? 'retains color' : 'drops empty rPr'}`, () => { + revisionTest(`${name} terminal ${inCell ? 'cell' : 'body'} mark ${color ? 'retains color' : 'drops empty rPr'}`, () => { const p = paragraph(kind, color, true); const input = wrap(inCell ? cell(p) : p); const doc = parseXml(input); @@ -32,7 +33,7 @@ describe('resolved terminal paragraph-mark properties', () => { } } - test(`${name} preserves the required empty last table-cell paragraph`, () => { + revisionTest(`${name} preserves the required empty last table-cell paragraph`, () => { const input = wrap(cell(paragraph(kind, false, false))); const doc = parseXml(input); nativeProject(doc); @@ -41,7 +42,7 @@ describe('resolved terminal paragraph-mark properties', () => { expect(serializeXml(doc)).toBe(serializeXml(parseXml(astProject(input)))); }); - test(`${name} resolves one author while preserving a foreign terminal mark and formatting`, () => { + revisionTest(`${name} resolves one author while preserving a foreign terminal mark and formatting`, () => { const foreign = paragraph(kind, true, true).replace('w:author="AI"', 'w:author="Human"'); const input = wrap(cell(paragraph(kind, false, true)) + cell(foreign)); const doc = parseXml(input); @@ -52,4 +53,26 @@ describe('resolved terminal paragraph-mark properties', () => { expect(doc.getElementsByTagNameNS(W, 'p').length).toBe(2); }); } + + for (const [name, kind, project, ast] of [['Accept', 'del', acceptChanges, acceptAllChanges], ['Reject', 'ins', rejectChanges, rejectAllChanges]] as const) { + for (const metadata of ['', '']) { + for (const scope of ['pPr', 'rPr']) { + test.conformance({ spec: 'ECMA-376', edition: 5, part: 1, section: kind === 'del' ? '17.13.5.15' : '17.13.5.20' })(`${name} retains ${metadata.startsWith('