diff --git a/packages/super-editor/src/editors/v1/core/helpers/getMarksFromSelection.test.js b/packages/super-editor/src/editors/v1/core/helpers/getMarksFromSelection.test.js index e75201a812..42e3860118 100644 --- a/packages/super-editor/src/editors/v1/core/helpers/getMarksFromSelection.test.js +++ b/packages/super-editor/src/editors/v1/core/helpers/getMarksFromSelection.test.js @@ -451,7 +451,8 @@ describe('getMarksFromSelection', () => { const result = getSelectionFormattingState(cursorState); - expect(result.inlineRunProperties).toEqual({ bold: true, boldCs: true }); + // SD-2912: `boldCs` is no longer auto-propagated from the bold mark. + expect(result.inlineRunProperties).toEqual({ bold: true }); expect(result.inlineMarks.some((mark) => mark.type.name === 'bold')).toBe(true); expect(result.resolvedMarks.some((mark) => mark.type.name === 'bold')).toBe(true); }); diff --git a/packages/super-editor/src/editors/v1/core/helpers/syncParagraphRunProperties.test.js b/packages/super-editor/src/editors/v1/core/helpers/syncParagraphRunProperties.test.js index 5659a6b993..a45633c86f 100644 --- a/packages/super-editor/src/editors/v1/core/helpers/syncParagraphRunProperties.test.js +++ b/packages/super-editor/src/editors/v1/core/helpers/syncParagraphRunProperties.test.js @@ -50,7 +50,8 @@ describe('syncParagraphRunProperties', () => { italic: true, styleId: 'Heading1Char', bold: true, - boldCs: true, + // SD-2912: `boldCs` is no longer auto-propagated from the bold mark — see + // `decodeRPrFromMarks` in super-converter/styles.js. }); }); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/exporter.js b/packages/super-editor/src/editors/v1/core/super-converter/exporter.js index ae6b381fae..3c3aa348c1 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/exporter.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/exporter.js @@ -106,6 +106,44 @@ export const ensureSectionLayoutDefaults = (sectPr, converter) => { return sectPr; }; +/** + * Walk an XML JSON tree in place and normalize every `` element's + * numeric attributes to integer twips. + * + * ECMA-376 §17.6.11 requires `ST_TwipsMeasure` values to be non-negative whole + * numbers when expressed as raw twips, but some authoring pipelines emit + * float-valued twips like `w:top="168.160400390625"` that strict consumers + * reject. SuperDoc preserves those floats verbatim on the paragraph-level + * sectPr passthrough path. This pass is the single normalization point — + * applied once at the export root — so we produce schema-valid output + * regardless of which path the pgMar values reached the tree through + * (body sectPr → `pageMargins` → `inchesToTwips`, or paragraph-level + * passthrough sectPr that preserved source attrs). + * + * Idempotent. Mutates the tree in place; returns nothing. SD-2912. + * + * @param {{ name?: string, attributes?: Record, elements?: Array } | null | undefined} node + * @returns {void} + */ +export const normalizePgMarTwipsInTree = (node) => { + if (!node || typeof node !== 'object') return; + if (node.name === 'w:pgMar' && node.attributes && typeof node.attributes === 'object') { + for (const key of Object.keys(node.attributes)) { + const value = node.attributes[key]; + if (value == null) continue; + const serialized = String(value).trim(); + if (!serialized) continue; + const num = Number(serialized); + if (Number.isFinite(num) && !/^-?\d+$/.test(serialized)) { + node.attributes[key] = String(Math.round(num)); + } + } + } + if (Array.isArray(node.elements)) { + for (const child of node.elements) normalizePgMarTwipsInTree(child); + } +}; + export const isLineBreakOnlyRun = (node) => { if (!node) return false; if (node.type === 'lineBreak' || node.type === 'hardBreak') return true; @@ -402,6 +440,12 @@ function translateDocumentNode(params) { attributes, }; + // SD-2912: normalize every in the final tree to integer twips, + // catching both the body sectPr path (already integer-correct via + // inchesToTwips) and the paragraph-level passthrough sectPr path that + // preserves source attrs verbatim. + normalizePgMarTwipsInTree(node); + return [node, params]; } @@ -568,6 +612,9 @@ function translateMark(mark) { break; case 'highlight': { const highlightValue = attrs.color ?? attrs.highlight ?? null; + if (String(highlightValue).trim().toLowerCase() === 'transparent' && !attrs.ooxmlHighlightClear) { + return {}; + } const translated = wHighlightTranslator.decode({ node: { attrs: { highlight: highlightValue } } }); return translated || {}; } diff --git a/packages/super-editor/src/editors/v1/core/super-converter/exporter.pgmar-normalize.test.js b/packages/super-editor/src/editors/v1/core/super-converter/exporter.pgmar-normalize.test.js new file mode 100644 index 0000000000..5ba2dcbdd1 --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/super-converter/exporter.pgmar-normalize.test.js @@ -0,0 +1,156 @@ +import { describe, it, expect } from 'vitest'; +import { normalizePgMarTwipsInTree, processOutputMarks } from './exporter.js'; + +// SD-2912: attributes must be integer twips per ECMA-376 §17.6.11 +// (ST_TwipsMeasure). Some documents carry float-valued twips like +// `w:top="168.160400390625"` that pass through the import → export pipeline +// verbatim on paragraph-level sectPr passthrough; strict consumers reject the +// result. This helper is the single normalization point: it walks the export +// XML JSON tree and rounds every numeric pgMar attribute to an integer. + +describe('normalizePgMarTwipsInTree', () => { + it('does not throw on undefined input', () => { + expect(() => normalizePgMarTwipsInTree(undefined)).not.toThrow(); + }); + + it('does not throw on null input', () => { + expect(() => normalizePgMarTwipsInTree(null)).not.toThrow(); + }); + + it('leaves a tree without any w:pgMar element unchanged', () => { + const tree = { + name: 'w:document', + elements: [{ name: 'w:body', elements: [{ name: 'w:p', elements: [] }] }], + }; + const before = JSON.stringify(tree); + normalizePgMarTwipsInTree(tree); + expect(JSON.stringify(tree)).toBe(before); + }); + + it('rounds a single float pgMar attribute to an integer twips value', () => { + const tree = { name: 'w:pgMar', attributes: { 'w:top': '168.160400390625' } }; + normalizePgMarTwipsInTree(tree); + expect(tree.attributes['w:top']).toBe('168'); + }); + + it('rounds every numeric pgMar attribute, leaves already-integer values exact', () => { + const tree = { + name: 'w:pgMar', + attributes: { + 'w:top': '168.160400390625', + 'w:bottom': '146.0200023651123', + 'w:left': '352.31998443603516', + 'w:right': '663.9990234375', + 'w:gutter': '0', + 'w:header': '720', + }, + }; + normalizePgMarTwipsInTree(tree); + expect(tree.attributes).toEqual({ + 'w:top': '168', + 'w:bottom': '146', + 'w:left': '352', + 'w:right': '664', + 'w:gutter': '0', + 'w:header': '720', + }); + }); + + it('canonicalizes decimal pgMar tokens even when the numeric value is integral', () => { + const tree = { + name: 'w:pgMar', + attributes: { + 'w:top': '168.0', + 'w:left': '352.000000', + 'w:right': '663.9990234375', + 'w:header': '720', + }, + }; + + normalizePgMarTwipsInTree(tree); + + expect(tree.attributes).toEqual({ + 'w:top': '168', + 'w:left': '352', + 'w:right': '664', + 'w:header': '720', + }); + }); + + it('walks into nested elements and normalizes pgMar attrs at any depth', () => { + const tree = { + name: 'w:document', + elements: [ + { + name: 'w:body', + elements: [ + { + name: 'w:p', + elements: [ + { + name: 'w:pPr', + elements: [ + { + name: 'w:sectPr', + elements: [{ name: 'w:pgMar', attributes: { 'w:top': '146.0200023651123' } }], + }, + ], + }, + ], + }, + { name: 'w:sectPr', elements: [{ name: 'w:pgMar', attributes: { 'w:bottom': '352.31998443603516' } }] }, + ], + }, + ], + }; + normalizePgMarTwipsInTree(tree); + const firstPgMar = tree.elements[0].elements[0].elements[0].elements[0].elements[0]; + const secondPgMar = tree.elements[0].elements[1].elements[0]; + expect(firstPgMar.attributes['w:top']).toBe('146'); + expect(secondPgMar.attributes['w:bottom']).toBe('352'); + }); + + it('is idempotent — re-running on already-normalized values is a no-op', () => { + const tree = { name: 'w:pgMar', attributes: { 'w:top': '168.5' } }; + normalizePgMarTwipsInTree(tree); + const afterFirst = { ...tree.attributes }; + normalizePgMarTwipsInTree(tree); + expect(tree.attributes).toEqual(afterFirst); + expect(tree.attributes['w:top']).toBe('169'); + }); + + it('ignores non-numeric attribute values (defensive against future OOXML extensions)', () => { + const tree = { name: 'w:pgMar', attributes: { 'w:top': '168', 'w:custom': 'auto' } }; + normalizePgMarTwipsInTree(tree); + expect(tree.attributes['w:custom']).toBe('auto'); + }); + + it('does not affect attributes on elements other than w:pgMar', () => { + const tree = { + name: 'w:document', + elements: [ + { name: 'w:pgSz', attributes: { 'w:w': '12240.5', 'w:h': '15840.7' } }, + { name: 'w:pgMar', attributes: { 'w:top': '168.5' } }, + ], + }; + normalizePgMarTwipsInTree(tree); + expect(tree.elements[0].attributes).toEqual({ 'w:w': '12240.5', 'w:h': '15840.7' }); + expect(tree.elements[1].attributes['w:top']).toBe('169'); + }); +}); + +describe('processOutputMarks highlight clear export', () => { + it('does not emit highlight XML for plain transparent highlight marks', () => { + const outputMarks = processOutputMarks([{ type: 'highlight', attrs: { color: 'transparent' } }]); + + expect(outputMarks).toEqual([{}]); + }); + + it('emits highlight none only for imported explicit highlight clears', () => { + const outputMarks = processOutputMarks([ + { type: 'highlight', attrs: { color: 'transparent', ooxmlHighlightClear: true } }, + ]); + + expect(outputMarks).toEqual([{ name: 'w:highlight', attributes: { 'w:val': 'none' } }]); + }); +}); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/styles.js b/packages/super-editor/src/editors/v1/core/super-converter/styles.js index f9f1a61e96..56a9fa88a0 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/styles.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/styles.js @@ -45,7 +45,8 @@ export function encodeMarksFromRPr(runProperties, docx) { const marks = []; const textStyleAttrs = {}; - let highlightColor = null; + /** @type {{ color: string, ooxmlHighlightClear?: boolean } | null} */ + let highlightAttrs = null; let hasHighlightTag = false; Object.keys(runProperties).forEach((key) => { const value = runProperties[key]; @@ -134,7 +135,10 @@ export function encodeMarksFromRPr(runProperties, docx) { const color = getHighLightValue(value); if (color) { hasHighlightTag = true; - highlightColor = color; + highlightAttrs = { color }; + if (color.toLowerCase() === 'transparent' && String(value?.['w:val']).toLowerCase() === 'none') { + highlightAttrs.ooxmlHighlightClear = true; + } } break; case 'shading': { @@ -144,11 +148,11 @@ export function encodeMarksFromRPr(runProperties, docx) { const fill = value['fill']; const shdVal = value['val']; if (fill && String(fill).toLowerCase() !== 'auto') { - highlightColor = `#${String(fill).replace('#', '')}`; + highlightAttrs = { color: `#${String(fill).replace('#', '')}` }; } else if (typeof shdVal === 'string') { const normalized = shdVal.toLowerCase(); if (normalized === 'clear' || normalized === 'nil' || normalized === 'none') { - highlightColor = 'transparent'; + highlightAttrs = { color: 'transparent' }; } } break; @@ -175,8 +179,8 @@ export function encodeMarksFromRPr(runProperties, docx) { marks.push({ type: 'textStyle', attrs: textStyleAttrs }); } - if (highlightColor) { - marks.push({ type: 'highlight', attrs: { color: highlightColor } }); + if (highlightAttrs) { + marks.push({ type: 'highlight', attrs: highlightAttrs }); } return marks; @@ -535,11 +539,14 @@ export function decodeRPrFromMarks(marks) { case 'italic': case 'bold': runProperties[type] = mark.attrs.value !== '0' && mark.attrs.value !== false; - if (type === 'bold') { - runProperties.boldCs = runProperties.bold; - } else if (type === 'italic') { - runProperties.italicCs = runProperties.italic; - } + // SD-2912: do NOT auto-propagate `boldCs` / `italicCs` from the latin + // bold/italic mark. The complex-script companion is an independent OOXML + // property (ECMA-376 §17.3.2). Auto-propagating it injects elements that + // weren't in the source rPr — every run gets a `` regardless of + // whether the original `` contained one. When the source genuinely + // had ``, it round-trips via the run's stored runProperties + // (preserved by the plugin's existing-keys branch — see the matching + // SD-2912 change in `calculateInlineRunPropertiesPlugin.js`). break; case 'underline': { const { underlineType, underlineColor, underlineThemeColor, underlineThemeTint, underlineThemeShade } = @@ -566,12 +573,13 @@ export function decodeRPrFromMarks(marks) { break; } case 'highlight': - if (mark.attrs.color) { - if (mark.attrs.color.toLowerCase() === 'transparent') { + if (!mark.attrs.color) break; + if (mark.attrs.color.toLowerCase() === 'transparent') { + if (mark.attrs.ooxmlHighlightClear) { runProperties.highlight = { 'w:val': 'none' }; - } else { - runProperties.highlight = { 'w:val': mark.attrs.color }; } + } else { + runProperties.highlight = { 'w:val': mark.attrs.color }; } break; case 'link': diff --git a/packages/super-editor/src/editors/v1/core/super-converter/styles.test.js b/packages/super-editor/src/editors/v1/core/super-converter/styles.test.js index b4f246cc2d..187c660d1d 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/styles.test.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/styles.test.js @@ -435,7 +435,10 @@ describe('decodeRPrFromMarks', () => { { type: 'strike', attrs: { value: true } }, ]; const rPr = decodeRPrFromMarks(marks); - expect(rPr).toEqual({ bold: true, boldCs: true, italic: true, italicCs: true, strike: true }); + // SD-2912: `boldCs`/`italicCs` are independent OOXML properties and are no longer + // auto-propagated from the latin bold/italic marks. They round-trip via the run's + // stored runProperties when the source rPr actually contained them. + expect(rPr).toEqual({ bold: true, italic: true, strike: true }); }); it('should decode textStyle marks for color and fontSize', () => { @@ -487,15 +490,48 @@ describe('decodeRPrFromMarks', () => { const rPr = decodeRPrFromMarks(marks); expect(rPr).toEqual({ styleId: 'Hyperlink' }); }); + + // SD-2912: redundant default emissions on round-trip. + // OOXML default for bCs/iCs is OFF and for w:highlight is "none" — emitting them + // explicitly is byte noise that wasn't in the source. Word treats absence and explicit + // off as identical, but the export should not introduce elements that weren't there. + + it('does not propagate boldCs when the bold mark is off (SD-2912)', () => { + const marks = [{ type: 'bold', attrs: { value: false } }]; + const rPr = decodeRPrFromMarks(marks); + expect(rPr.bold).toBe(false); + expect(rPr.boldCs).toBeUndefined(); + }); + + it('does not propagate italicCs when the italic mark is off (SD-2912)', () => { + const marks = [{ type: 'italic', attrs: { value: false } }]; + const rPr = decodeRPrFromMarks(marks); + expect(rPr.italic).toBe(false); + expect(rPr.italicCs).toBeUndefined(); + }); + + it('does not emit highlight when the highlight mark is transparent (SD-2912)', () => { + const marks = [{ type: 'highlight', attrs: { color: 'transparent' } }]; + const rPr = decodeRPrFromMarks(marks); + expect(rPr.highlight).toBeUndefined(); + }); + + it('preserves imported explicit highlight none clears (SD-2912)', () => { + const marks = [{ type: 'highlight', attrs: { color: 'transparent', ooxmlHighlightClear: true } }]; + const rPr = decodeRPrFromMarks(marks); + expect(rPr.highlight).toEqual({ 'w:val': 'none' }); + }); }); describe('marks encoding/decoding round-trip', () => { it('should correctly round-trip basic properties', () => { + // SD-2912: `boldCs`/`italicCs` are not represented as PM marks (they are independent + // OOXML properties tracked on the run's stored runProperties). The encode/decode mark + // round-trip therefore lossily drops them at the mark layer; preservation through a + // full DOCX round-trip happens via the run-properties passthrough path. const initialRPr = { bold: true, - boldCs: true, italic: true, - italicCs: true, strike: true, underline: { 'w:val': 'single', 'w:color': 'auto' }, color: { val: 'FF0000' }, @@ -536,6 +572,18 @@ describe('marks encoding/decoding round-trip', () => { expect(finalRPr2).toEqual({ highlight: { 'w:val': '#FFA500' } }); }); + it('distinguishes explicit highlight none from transparent shading on round-trip', () => { + const explicitClearMarks = encodeMarksFromRPr({ highlight: { 'w:val': 'none' } }, {}); + expect(explicitClearMarks).toEqual([ + { type: 'highlight', attrs: { color: 'transparent', ooxmlHighlightClear: true } }, + ]); + expect(decodeRPrFromMarks(explicitClearMarks)).toEqual({ highlight: { 'w:val': 'none' } }); + + const transparentShadingMarks = encodeMarksFromRPr({ shading: { val: 'clear', fill: 'auto' } }, {}); + expect(transparentShadingMarks).toEqual([{ type: 'highlight', attrs: { color: 'transparent' } }]); + expect(decodeRPrFromMarks(transparentShadingMarks)).toEqual({}); + }); + it('should show asymmetry in textTransform/caps round-trip', () => { const rPrTextTransform = { textTransform: 'uppercase' }; const marks = encodeMarksFromRPr(rPrTextTransform, {}); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.js index 782343e054..4e1638016a 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.js @@ -32,6 +32,32 @@ const hasXmlNodeNamed = (node, targetName) => { return node.elements.some((child) => hasXmlNodeNamed(child, targetName)); }; +const COMPLEX_SCRIPT_CODEPOINT_RANGES = [ + [0x0590, 0x08ff], + [0x0900, 0x109f], + [0x1780, 0x18af], + [0x1cd0, 0x1cff], + [0xa800, 0xa8ff], + [0xaa60, 0xaa7f], +]; + +const textHasComplexScript = (text) => { + for (const char of text) { + const codePoint = char.codePointAt(0); + if (COMPLEX_SCRIPT_CODEPOINT_RANGES.some(([start, end]) => codePoint >= start && codePoint <= end)) { + return true; + } + } + return false; +}; + +const hasComplexScriptText = (node) => { + if (!node || typeof node !== 'object') return false; + if (typeof node.text === 'string' && textHasComplexScript(node.text)) return true; + if (!Array.isArray(node.content)) return false; + return node.content.some((child) => hasComplexScriptText(child)); +}; + const getRunPropertiesNode = (runNode) => { if (!runNode) return null; if (!Array.isArray(runNode.elements)) runNode.elements = []; @@ -348,6 +374,14 @@ const decode = (params, decodedAttrs = {}) => { const runPropertiesToExport = exportKeys.length > 0 ? Object.fromEntries(exportKeys.map((k) => [k, runProperties[k]])) : {}; + if (hasComplexScriptText(runNodeForExport)) { + if ('bold' in runPropertiesToExport && !('boldCs' in runPropertiesToExport)) { + runPropertiesToExport.boldCs = runPropertiesToExport.bold; + } + if ('italic' in runPropertiesToExport && !('italicCs' in runPropertiesToExport)) { + runPropertiesToExport.italicCs = runPropertiesToExport.italic; + } + } // Decode child nodes within the run const exportParams = { diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.test.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.test.js index 67e2bd73e9..486ce989c4 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.test.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.test.js @@ -535,6 +535,41 @@ describe('w:r r-translator decode (export only inline run properties)', () => { expect(names).toContain('w:iCs'); }); + it('exports complex-script companions when newly formatted complex-script text has base bold/italic', () => { + const params = runWithContent({ + runProperties: { bold: true, italic: true }, + runPropertiesInlineKeys: ['bold', 'italic'], + runPropertiesStyleKeys: [], + }); + params.node.content[0].text = 'مرحبا'; + + const result = translator.decode(params); + const rPr = result?.elements?.find((el) => el?.name === 'w:rPr'); + expect(rPr).toBeDefined(); + const names = (rPr.elements ?? []).map((e) => e.name); + expect(names).toContain('w:b'); + expect(names).toContain('w:bCs'); + expect(names).toContain('w:i'); + expect(names).toContain('w:iCs'); + }); + + it('does not synthesize complex-script companions for latin-only text', () => { + const params = runWithContent({ + runProperties: { bold: true, italic: true }, + runPropertiesInlineKeys: ['bold', 'italic'], + runPropertiesStyleKeys: [], + }); + + const result = translator.decode(params); + const rPr = result?.elements?.find((el) => el?.name === 'w:rPr'); + expect(rPr).toBeDefined(); + const names = (rPr.elements ?? []).map((e) => e.name); + expect(names).toContain('w:b'); + expect(names).toContain('w:i'); + expect(names).not.toContain('w:bCs'); + expect(names).not.toContain('w:iCs'); + }); + it('exports w:iCs from legacy runProperties.iCs payloads', () => { const params = runWithContent({ runProperties: { italic: true, iCs: true }, diff --git a/packages/super-editor/src/editors/v1/extensions/highlight/highlight.js b/packages/super-editor/src/editors/v1/extensions/highlight/highlight.js index 77eff9e6d8..6a4d4c1a6a 100644 --- a/packages/super-editor/src/editors/v1/extensions/highlight/highlight.js +++ b/packages/super-editor/src/editors/v1/extensions/highlight/highlight.js @@ -15,6 +15,7 @@ import { cssColorToHex } from '@core/utilities/cssColorToHex.js'; * @typedef {Object} HighlightAttributes * @category Attributes * @property {string} [color] - Background color (CSS color value) + * @property {boolean} [ooxmlHighlightClear] - Imported explicit OOXML highlight clear marker */ /** @@ -47,6 +48,11 @@ export const Highlight = Mark.create({ }; }, }, + ooxmlHighlightClear: { + default: null, + parseDOM: () => null, + renderDOM: () => ({}), + }, }; }, diff --git a/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js b/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js index f33a09ac31..8999052ef9 100644 --- a/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js +++ b/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js @@ -8,12 +8,17 @@ import { } from '@extensions/paragraph/resolvedPropertiesCache.js'; import { collectChangedRangesThroughTransactions } from '@utils/rangeUtils.js'; +// SD-2912: `boldCs` / `italicCs` are NOT marks. The OOXML companions for complex-script +// bold/italic are independent properties (ECMA-376 §17.3.2.1, §17.3.2.16) carried by the +// run's stored runProperties. Listing them here caused the plugin to overwrite their +// preserved value on every appendTransaction, which combined with the auto-propagation in +// `decodeRPrFromMarks` injected a `` / `` element into every run on round- +// trip even when the source rPr had none. Keeping them out of this set lets the existing- +// runProperties branch in `getInlineRunProperties` preserve them verbatim. const RUN_PROPERTIES_DERIVED_FROM_MARKS = new Set([ 'strike', 'italic', - 'italicCs', 'bold', - 'boldCs', 'underline', 'highlight', 'textTransform', @@ -123,6 +128,7 @@ export const calculateInlineRunPropertiesPlugin = (editor) => tableInfo, $pos, editor, + removedKeys, preservedDerivedKeys, preferExistingKeys, ); @@ -240,6 +246,8 @@ export const calculateInlineRunPropertiesPlugin = (editor) => if (!runProperties) runProperties = {}; lostKeys.forEach((k) => { if (removedKeys.has(k)) return; + const baseKey = COMPANION_INLINE_KEYS[k]; + if (baseKey && removedKeys.has(baseKey)) return; if (runNode.attrs?.runProperties?.[k] !== undefined) { runProperties[k] = runNode.attrs.runProperties[k]; } @@ -395,6 +403,7 @@ function segmentRunByInlineProps( tableInfo, $pos, editor, + removedKeys, preservedDerivedKeys, preferExistingKeys, ) { @@ -411,6 +420,7 @@ function segmentRunByInlineProps( tableInfo, $pos, editor, + removedKeys, preservedDerivedKeys, preferExistingKeys, ); @@ -458,6 +468,7 @@ function computeInlineRunProps( tableInfo, $pos, editor, + removedKeys, preservedDerivedKeys, preferExistingKeys, ) { @@ -481,6 +492,7 @@ function computeInlineRunProps( runPropertiesFromStyles, existingRunProperties, editor, + removedKeys, preservedDerivedKeys, preferExistingKeys, ); @@ -503,6 +515,7 @@ function getInlineRunProperties( runPropertiesFromStyles, existingRunProperties, editor, + removedKeys = new Set(), preservedDerivedKeys = new Set(), preferExistingKeys = new Set(), ) { @@ -550,6 +563,8 @@ function getInlineRunProperties( if (existingRunProperties != null) { Object.keys(existingRunProperties).forEach((key) => { if (RUN_PROPERTIES_DERIVED_FROM_MARKS.has(key) && !preservedDerivedKeys.has(key)) return; + const baseKey = COMPANION_INLINE_KEYS[key]; + if (baseKey && removedKeys.has(baseKey)) return; if ( key === 'styleId' && TRANSIENT_HYPERLINK_STYLE_IDS.has(existingRunProperties[key]) && diff --git a/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.test.js b/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.test.js index 5b510c94e4..55e610d164 100644 --- a/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.test.js +++ b/packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.test.js @@ -272,6 +272,54 @@ describe('calculateInlineRunPropertiesPlugin', () => { expect(runNode?.attrs.runProperties).toBeNull(); }); + it('drops complex-script bold companion when the bold mark is removed', () => { + decodeRPrFromMarksMock.mockImplementation(() => ({ bold: false })); + resolveRunPropertiesMock.mockImplementation(() => ({ bold: false })); + + const schema = makeSchema(); + const boldMark = schema.marks.bold.create(); + const doc = paragraphDoc( + schema, + { + runProperties: { bold: true, boldCs: true }, + runPropertiesInlineKeys: ['bold', 'boldCs'], + }, + [boldMark], + ); + const state = createState(schema, doc); + const { from, to } = runTextRange(state.doc, 0, doc.textContent.length); + + const tr = state.tr.removeMark(from, to, schema.marks.bold); + const { state: nextState } = state.applyTransaction(tr); + + const runNode = nextState.doc.nodeAt(runPos(nextState.doc) ?? 0); + expect(runNode?.attrs.runProperties).toBeNull(); + }); + + it('drops complex-script italic companion when the italic mark is removed', () => { + decodeRPrFromMarksMock.mockImplementation(() => ({ italic: false })); + resolveRunPropertiesMock.mockImplementation(() => ({ italic: false })); + + const schema = makeSchema(); + const italicMark = schema.marks.italic.create(); + const doc = paragraphDoc( + schema, + { + runProperties: { italic: true, italicCs: true }, + runPropertiesInlineKeys: ['italic', 'italicCs'], + }, + [italicMark], + ); + const state = createState(schema, doc); + const { from, to } = runTextRange(state.doc, 0, doc.textContent.length); + + const tr = state.tr.removeMark(from, to, schema.marks.italic); + const { state: nextState } = state.applyTransaction(tr); + + const runNode = nextState.doc.nodeAt(runPos(nextState.doc) ?? 0); + expect(runNode?.attrs.runProperties).toBeNull(); + }); + it('uses cached paragraph properties when available', () => { getResolvedParagraphPropertiesMock.mockReturnValue({ cached: true }); calculateResolvedParagraphPropertiesMock.mockImplementation(() => { diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/addMarkStep.js b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/addMarkStep.js index 76893f16dd..c169b4a08b 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/addMarkStep.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/addMarkStep.js @@ -9,6 +9,7 @@ import { upsertMarkSnapshotByType, isTrackFormatNoOp, getTypeName, + createMarkSnapshot, } from './markSnapshotHelpers.js'; import { getLiveInlineMarksInRange } from './getLiveInlineMarksInRange.js'; @@ -78,7 +79,7 @@ export const addMarkStep = ({ state, step, newTr, doc, user, date }) => { before = [...beforeSnapshots]; after = upsertMarkSnapshotByType(afterSnapshots, { type: step.mark.type.name, - attrs: { ...step.mark.attrs }, + attrs: step.mark.attrs, }); } } else { @@ -88,15 +89,10 @@ export const addMarkStep = ({ state, step, newTr, doc, user, date }) => { ![TrackDeleteMarkName, TrackFormatMarkName].includes(mark.type.name), ); before = existingMarkOfSameType - ? [{ type: existingMarkOfSameType.type.name, attrs: { ...existingMarkOfSameType.attrs } }] + ? [createMarkSnapshot(existingMarkOfSameType.type.name, existingMarkOfSameType.attrs)] : []; - after = [ - { - type: step.mark.type.name, - attrs: { ...step.mark.attrs }, - }, - ]; + after = [createMarkSnapshot(step.mark.type.name, step.mark.attrs)]; } // Check if the format change is effectively a no-op (e.g., reverting diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js index 944782e77b..8cb4bcabfe 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/markSnapshotHelpers.js @@ -4,6 +4,21 @@ const normalizeAttrs = (attrs = {}) => { return Object.fromEntries(Object.entries(attrs).filter(([, value]) => value !== null && value !== undefined)); }; +const stripUnsetInternalSnapshotAttrs = (attrs = {}) => { + const nextAttrs = { ...attrs }; + if (nextAttrs.ooxmlHighlightClear === null || nextAttrs.ooxmlHighlightClear === undefined) { + delete nextAttrs.ooxmlHighlightClear; + } + return nextAttrs; +}; + +export const createMarkSnapshot = (type, attrs = {}) => { + return { + type, + attrs: stripUnsetInternalSnapshotAttrs(attrs), + }; +}; + /** * Attribute values that are semantically equivalent to "not set" for tracking purposes. * These represent the default visual state and should not count as a change. @@ -96,11 +111,11 @@ export const upsertMarkSnapshotByType = (snapshots, incoming) => { if (existing) { const merged = { ...existing, - attrs: { ...existing.attrs, ...incoming.attrs }, + attrs: stripUnsetInternalSnapshotAttrs({ ...existing.attrs, ...incoming.attrs }), }; return snapshots.map((mark) => (mark === existing ? merged : mark)); } - return [...snapshots, incoming]; + return [...snapshots, createMarkSnapshot(incoming.type, incoming.attrs)]; }; const markMatchesSnapshot = (mark, snapshot, exact = true) => { diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/removeMarkStep.js b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/removeMarkStep.js index 581d49fd7e..686788cbe0 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/removeMarkStep.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/removeMarkStep.js @@ -2,7 +2,12 @@ import { v4 as uuidv4 } from 'uuid'; import { TrackDeleteMarkName, TrackFormatMarkName, TrackedFormatMarkNames } from '../constants.js'; import { TrackChangesBasePluginKey } from '../plugins/trackChangesBasePlugin.js'; import { CommentsPluginKey } from '../../comment/comments-plugin.js'; -import { hasMatchingMark, markSnapshotMatchesStepMark, upsertMarkSnapshotByType } from './markSnapshotHelpers.js'; +import { + createMarkSnapshot, + hasMatchingMark, + markSnapshotMatchesStepMark, + upsertMarkSnapshotByType, +} from './markSnapshotHelpers.js'; import { getLiveInlineMarksInRange } from './getLiveInlineMarksInRange.js'; /** @@ -74,19 +79,14 @@ export const removeMarkStep = ({ state, step, newTr, doc, user, date }) => { after = [...formatChangeMark.attrs.after]; before = upsertMarkSnapshotByType(formatChangeMark.attrs.before, { type: step.mark.type.name, - attrs: { ...step.mark.attrs }, + attrs: step.mark.attrs, }); } } else { after = []; let existingMark = node.marks.find((mark) => mark.type === step.mark.type); if (existingMark) { - before = [ - { - type: step.mark.type.name, - attrs: { ...existingMark.attrs }, - }, - ]; + before = [createMarkSnapshot(step.mark.type.name, existingMark.attrs)]; } else { before = []; } diff --git a/packages/super-editor/src/editors/v1/extensions/types/mark-attributes.ts b/packages/super-editor/src/editors/v1/extensions/types/mark-attributes.ts index 5a2d93f8c5..ce4b81c25a 100644 --- a/packages/super-editor/src/editors/v1/extensions/types/mark-attributes.ts +++ b/packages/super-editor/src/editors/v1/extensions/types/mark-attributes.ts @@ -118,6 +118,8 @@ export type HighlightColor = string; export interface HighlightAttrs { /** Highlight color */ color?: HighlightColor | null; + /** @internal Imported explicit OOXML highlight clear marker */ + ooxmlHighlightClear?: boolean | null; } // ============================================ diff --git a/packages/super-editor/src/editors/v1/tests/data/sd-2912-pgmar-roundtrip.docx b/packages/super-editor/src/editors/v1/tests/data/sd-2912-pgmar-roundtrip.docx new file mode 100644 index 0000000000..8876d14845 Binary files /dev/null and b/packages/super-editor/src/editors/v1/tests/data/sd-2912-pgmar-roundtrip.docx differ diff --git a/packages/super-editor/src/editors/v1/tests/import-export/sd-2912-pgmar-roundtrip.test.js b/packages/super-editor/src/editors/v1/tests/import-export/sd-2912-pgmar-roundtrip.test.js new file mode 100644 index 0000000000..1a923697d1 --- /dev/null +++ b/packages/super-editor/src/editors/v1/tests/import-export/sd-2912-pgmar-roundtrip.test.js @@ -0,0 +1,130 @@ +import { describe, it, expect } from 'vitest'; +import { dirname, join } from 'path'; +import { fileURLToPath } from 'node:url'; +import { promises as fs } from 'fs'; +import { Editor } from '@core/Editor.js'; +import DocxZipper from '@core/DocxZipper.js'; +import { initTestEditor } from '../helpers/helpers.js'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); + +const countOccurrences = (haystack, needle) => { + let n = 0; + let i = 0; + while ((i = haystack.indexOf(needle, i)) !== -1) { + n++; + i += needle.length; + } + return n; +}; + +/** + * Parse every in the given XML string into a list of attribute maps. + * The fixture's pgMar elements are all self-closing single-line tags, so a regex is + * adequate here and avoids pulling in a full XML parser for the assertion. + */ +const extractPgMarAttrs = (xml) => { + const result = []; + const tagRe = /]*?)\/?>/g; + let m; + while ((m = tagRe.exec(xml)) !== null) { + const attrs = {}; + const attrRe = /([\w:]+)="([^"]*)"/g; + let am; + while ((am = attrRe.exec(m[1])) !== null) { + attrs[am[1]] = am[2]; + } + result.push(attrs); + } + return result; +}; + +async function roundTripCounts(fixtureFileName) { + const docxPath = join(__dirname, '../data', fixtureFileName); + const docxBuffer = await fs.readFile(docxPath); + + const inputZipper = new DocxZipper(); + const inputEntries = await inputZipper.getDocxData(docxBuffer, true); + const inputDocXml = inputEntries.find((e) => e.name === 'word/document.xml').content; + + const [docx, media, mediaFiles, fonts] = await Editor.loadXmlData(docxBuffer, true); + const { editor } = await initTestEditor({ content: docx, media, mediaFiles, fonts, isHeadless: true }); + + const exportedBuffer = await editor.exportDocx({ isFinalDoc: false }); + const exportedZipper = new DocxZipper(); + const exportedEntries = await exportedZipper.getDocxData(exportedBuffer, true); + const exportDocXml = exportedEntries.find((e) => e.name === 'word/document.xml').content; + + return { + input: { + bCs: countOccurrences(inputDocXml, ' { + it('does not add `` elements that were not in the source document.xml', async () => { + const { input, output } = await roundTripCounts('sd-2912-pgmar-roundtrip.docx'); + expect(input.bCs).toBe(0); + expect(output.bCs).toBe(0); + }); + + it('does not add `` elements that were not in the source document.xml', async () => { + const { input, output } = await roundTripCounts('sd-2912-pgmar-roundtrip.docx'); + expect(input.iCs).toBe(0); + expect(output.iCs).toBe(0); + }); + + it('does not add `` elements that were not in the source document.xml', async () => { + const { input, output } = await roundTripCounts('sd-2912-pgmar-roundtrip.docx'); + expect(input.highlight).toBe(0); + expect(output.highlight).toBe(0); + }); + + // SD-2912 customer ask: source has float-valued attributes like + // `w:top="168.160400390625"` that are schema-invalid per ECMA-376 §17.6.11 + // (ST_TwipsMeasure must be a non-negative whole number when expressed as + // raw twips). Strict consumers reject the document. On export we must + // normalize every pgMar twips attribute to an integer regardless of which + // path it reached the export tree through (body sectPr → pageMargins → + // inchesToTwips, or paragraph-level passthrough sectPr). + describe('SD-2912 pgMar integer-twips normalization', () => { + it('every attribute in the exported document.xml is an integer twips value', async () => { + const { output } = await roundTripCounts('sd-2912-pgmar-roundtrip.docx'); + expect(output.pgMar.length).toBeGreaterThan(0); + for (const attrs of output.pgMar) { + for (const [key, value] of Object.entries(attrs)) { + const num = Number(value); + expect(Number.isFinite(num), `pgMar attr ${key}="${value}" is not numeric`).toBe(true); + expect(Number.isInteger(num), `pgMar attr ${key}="${value}" is not an integer`).toBe(true); + } + } + }); + + it('the exported document.xml contains no decimal-valued pgMar attribute', async () => { + const { output } = await roundTripCounts('sd-2912-pgmar-roundtrip.docx'); + // Regex sanity check on raw XML — catches any pgMar attr value with a `.` + // followed by a digit, which is the symptom strict consumers reject. + expect(output.raw).not.toMatch(/]*"\d+\.\d/); + }); + + it('source fixture confirmed to carry decimal-valued pgMar attrs (otherwise the assertion is vacuous)', async () => { + const { input } = await roundTripCounts('sd-2912-pgmar-roundtrip.docx'); + const decimalSourceAttrs = input.pgMar.flatMap((attrs) => + Object.entries(attrs).filter(([, v]) => /\d+\.\d/.test(v)), + ); + expect(decimalSourceAttrs.length).toBeGreaterThan(0); + }); + }); +});