From dc1b5b14b90c54a8f369cfbf0402981138bb7b35 Mon Sep 17 00:00:00 2001 From: Caio Pizzol <97641911+caio-pizzol@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:23:02 +0000 Subject: [PATCH 1/2] fix(super-converter): tolerate empty style properties on table styles (#3861) Keeps tables in documents whose `word/styles.xml` carries empty property containers, such as a self-closing `` on Table Grid. Word opens these files; SuperDoc was dropping the tables. Issue #3861 reports LibreOffice and some Word templates as the source. xml-js omits the `elements` key rather than emitting an empty one, so ``, `` and `` all arrive with no children, and the style reader assumed that key was there. The importer catches per node, so the throw was swallowed and only the table vanished, which is why an affected document looked blank rather than raising an error. - Reading through shared accessors closes five unguarded spots, not just the reported `w:rPr`. A `w:basedOn` pointing at a childless style throws first, before execution reaches the `w:rPr`, and with an identical message, so fixing only the reported line could look like no fix at all. - Conditional formatting entries with no `w:type` are skipped rather than keyed under `undefined`. ECMA-376 marks that attribute required. - Empty `w:tblPr`, `w:tcPr` and `w:trPr` were already safe. The issue lists them as broken, so tests record that they are not. - Fixtures parse XML strings deliberately. Hand-authored nodes using `elements: []` do not reproduce the parser shape and pass against this bug. Byte-identical across v1.43.2, v1.44.2, v1.45.2 and main. Fixes superdoc/docx-editor#3861 Co-authored-by: Caio Pizzol Source-PR: https://github.com/superdoc/docx-editor/pull/3864 Closes superdoc/docx-editor#3864 Ported-From-Source-Repo: superdoc/orbit Ported-From-Source-Commit: 0f2d0c8330880789de1df54e7e9345eacd395ccd Ported-Public-Prefix: superdoc/public --- .../super-converter/docx-helpers/index.js | 1 + .../docx-helpers/xml-node-access.js | 54 ++++++ ...ocxImporter.empty-style-properties.test.js | 84 +++++++++ ...-translator.empty-style-properties.test.js | 175 ++++++++++++++++++ .../v3/handlers/w/tbl/tbl-translator.js | 54 +++--- 5 files changed, 341 insertions(+), 27 deletions(-) create mode 100644 packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/xml-node-access.js create mode 100644 packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js create mode 100644 packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.empty-style-properties.test.js diff --git a/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/index.js b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/index.js index d244450c9f..2584875325 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/index.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/index.js @@ -2,3 +2,4 @@ export * from './docx-helpers.js'; export * from './docx-constants.js'; export * from './get-default-style-definition.js'; +export * from './xml-node-access.js'; diff --git a/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/xml-node-access.js b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/xml-node-access.js new file mode 100644 index 0000000000..e07481f9fd --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/xml-node-access.js @@ -0,0 +1,54 @@ +// @ts-check + +/** + * Safe reads over xml-js element trees. + * + * AIDEV-NOTE: xml-js omits `elements` and `attributes` entirely rather than + * emitting empty ones, so ``, `` and `` all + * parse to `{ type: 'element', name: 'w:rPr' }` with no `elements` key. Every + * child of `CT_Style` is optional in ECMA-376, so empty property containers are + * schema-valid, but readers that assume `node.elements` exists throw on them + * (issue #3861). Reach into parsed OOXML through these helpers rather than + * dereferencing `.elements` or `.attributes` directly. + */ + +/** + * A node in a parsed xml-js tree. + * @typedef {object} XmlNode + * @property {string} [type] Node kind, e.g. `element` or `text`. + * @property {string} [name] Qualified element name, e.g. `w:rPr`. + * @property {Record} [attributes] Absent when the element has no attributes. + * @property {XmlNode[]} [elements] Absent when the element has no children. + * @property {string} [text] Character data, for text nodes. + */ + +/** + * Child elements of a parsed OOXML node, or an empty array when it has none. + * @param {XmlNode | null | undefined} node + * @returns {XmlNode[]} + */ +export const childElements = (node) => (Array.isArray(node?.elements) ? node.elements : []); + +/** + * First child element with the given qualified name. + * @param {XmlNode | null | undefined} node + * @param {string} name Qualified element name, e.g. `w:rPr`. + * @returns {XmlNode | undefined} + */ +export const findChild = (node, name) => childElements(node).find((el) => el?.name === name); + +/** + * Every child element with the given qualified name. + * @param {XmlNode | null | undefined} node + * @param {string} name Qualified element name, e.g. `w:tblStylePr`. + * @returns {XmlNode[]} + */ +export const findChildren = (node, name) => childElements(node).filter((el) => el?.name === name); + +/** + * Attribute value, or undefined when the element carries no attributes. + * @param {XmlNode | null | undefined} node + * @param {string} name Qualified attribute name, e.g. `w:val`. + * @returns {string | undefined} + */ +export const attrValue = (node, name) => node?.attributes?.[name]; diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js new file mode 100644 index 0000000000..4f6baca966 --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js @@ -0,0 +1,84 @@ +// @ts-check +import { describe, expect, it } from 'vitest'; +import * as xmljs from 'xml-js'; +import { createDocumentJson } from './docxImporter.js'; + +/** + * End-to-end import cover for empty OOXML property containers (issue #3861). + * + * AIDEV-NOTE: Fixtures are parsed from XML strings because xml-js omits the + * `elements` key on empty elements. Object literals using `elements: []` do not + * reproduce the failing shape. + */ + +const WORDPROCESSING_NS = 'http://schemas.openxmlformats.org/wordprocessingml/2006/main'; +const RELATIONSHIPS_NS = 'http://schemas.openxmlformats.org/officeDocument/2006/relationships'; + +const parse = (xml) => xmljs.xml2js(xml, { compact: false }); + +const DOCUMENT_XML = ` + + + before + + + + cell + + after + + +`; + +/** Styles.xml where Table Grid inherits from a bare base and carries an empty w:rPr. */ +const STYLES_XML = ` + + + + + + + + +`; + +const buildDocx = () => ({ + 'word/document.xml': parse(DOCUMENT_XML), + 'word/styles.xml': parse(STYLES_XML), + 'word/_rels/document.xml.rels': parse( + ``, + ), +}); + +const buildConverter = () => ({ + headerIds: {}, + headers: {}, + footers: {}, + docHiglightColors: new Set(), + trackedChangesOptions: {}, + convertedXml: {}, +}); + +const textOf = (node) => { + if (!node) return ''; + if (node.type === 'text') return node.text ?? ''; + return (node.content ?? []).map(textOf).join(''); +}; + +describe('DOCX import with empty style property containers', () => { + it('keeps a table whose style has an empty w:rPr and a bare basedOn target', () => { + const exceptions = []; + const editor = { + emit: (name, payload) => { + if (name === 'exception') exceptions.push(payload.error); + }, + options: {}, + }; + + const result = createDocumentJson(buildDocx(), buildConverter(), editor); + + const body = (result?.pmDoc?.content ?? []).map((node) => `${node.type}:${textOf(node)}`); + expect(body).toEqual(['paragraph:before', 'table:cell', 'paragraph:after']); + expect(exceptions).toEqual([]); + }); +}); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.empty-style-properties.test.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.empty-style-properties.test.js new file mode 100644 index 0000000000..416becc526 --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.empty-style-properties.test.js @@ -0,0 +1,175 @@ +// @ts-check +import { describe, expect, it } from 'vitest'; +import * as xmljs from 'xml-js'; +import { _getReferencedTableStyles } from './tbl-translator.js'; + +/** + * Empty property containers in `word/styles.xml` used to abort table import (issue #3861). + * + * AIDEV-NOTE: These fixtures parse real XML strings on purpose. xml-js omits the + * `elements` key for empty elements, and hand-authored nodes using `elements: []` + * do NOT reproduce that shape, so object-literal fixtures pass against the bug. + * Keep building these from XML. + */ + +const WORDPROCESSING_NS = 'http://schemas.openxmlformats.org/wordprocessingml/2006/main'; + +/** + * Build a `docx` bundle whose Table Grid style carries the given child XML. + * @param {string} tableGridChildren Raw XML placed inside ``. + * @param {string} [extraStyles] Raw XML for sibling `` elements. + */ +const docxWithTableGrid = (tableGridChildren, extraStyles = '') => ({ + 'word/styles.xml': xmljs.xml2js( + ` + + + ${extraStyles} + + + ${tableGridChildren} + + `, + { compact: false }, + ), +}); + +const resolve = (docx, styleId = 'TableGrid') => _getReferencedTableStyles(styleId, { docx }); + +describe('_getReferencedTableStyles with empty style properties', () => { + describe('empty run properties (the reported case)', () => { + it.each([ + ['self-closing', ''], + ['self-closing with space', ''], + ['paired', ''], + ['whitespace only', '\n '], + ])('treats a %s w:rPr as a no-op', (_label, rPr) => { + const styles = resolve(docxWithTableGrid(rPr)); + + expect(styles).not.toBeNull(); + expect(styles?.fonts).toBeUndefined(); + expect(styles?.fontSize).toBeUndefined(); + }); + + it('still reads populated run properties', () => { + const styles = resolve( + docxWithTableGrid(''), + ); + + expect(styles?.fonts).toEqual({ ascii: 'Arial', hAnsi: 'Arial', cs: 'Arial' }); + expect(styles?.fontSize).toBe('12pt'); + }); + + it('treats a w:rFonts without attributes as a no-op', () => { + const styles = resolve(docxWithTableGrid('')); + + expect(styles?.fonts).toBeUndefined(); + }); + }); + + describe('empty paragraph properties', () => { + it.each([ + ['self-closing', ''], + ['paired', ''], + ])('treats a %s w:pPr as a no-op', (_label, pPr) => { + const styles = resolve(docxWithTableGrid(pPr)); + + expect(styles?.justification).toBeUndefined(); + }); + + it('still reads justification from a populated w:pPr', () => { + const styles = resolve(docxWithTableGrid('')); + + expect(styles?.justification).toBe('center'); + }); + }); + + describe('inherited styles', () => { + it('tolerates a w:basedOn target that has no children', () => { + const docx = docxWithTableGrid( + '', + '', + ); + + expect(() => resolve(docx)).not.toThrow(); + }); + + it('tolerates a w:basedOn target that does not exist', () => { + expect(() => resolve(docxWithTableGrid(''))).not.toThrow(); + }); + + it('still inherits table properties from a populated base style', () => { + const docx = docxWithTableGrid( + '', + ` + + `, + ); + + expect(resolve(docx)?.borders?.top).toMatchObject({ val: 'single' }); + }); + }); + + describe('conditional formatting', () => { + it('skips a w:tblStylePr that has no w:type to key it by', () => { + const styles = resolve(docxWithTableGrid('')); + + expect(styles).not.toBeNull(); + expect(Object.keys(styles ?? {})).not.toContain('undefined'); + }); + + it('still reads a typed w:tblStylePr', () => { + const styles = resolve(docxWithTableGrid('')); + + expect(styles?.firstRow).toBeDefined(); + }); + + it('treats empty conditional property containers as no-ops', () => { + const styles = resolve( + docxWithTableGrid(''), + ); + + expect(styles?.firstRow).toBeDefined(); + }); + }); + + describe('malformed style records', () => { + it('tolerates a matched style element with no children', () => { + const docx = { + 'word/styles.xml': xmljs.xml2js( + ` + + `, + { compact: false }, + ), + }; + + expect(() => resolve(docx)).not.toThrow(); + }); + + it('ignores style elements that carry no attributes', () => { + const docx = { + 'word/styles.xml': xmljs.xml2js( + ` + + + `, + { compact: false }, + ), + }; + + expect(resolve(docx)?.name).toBeDefined(); + }); + + it('returns null when styles.xml is absent', () => { + expect(resolve({})).toBeNull(); + }); + + it('treats empty w:tblPr, w:tcPr and w:trPr as no-ops', () => { + const styles = resolve(docxWithTableGrid('')); + + expect(styles?.borders).toBeUndefined(); + expect(styles?.cellMargins).toBeUndefined(); + }); + }); +}); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.js b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.js index 2399460cab..70963785c3 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tbl/tbl-translator.js @@ -1,5 +1,11 @@ // @ts-check import { translator as tblStylePrTranslator } from '@converter/v3/handlers/w/tblStylePr'; +import { + attrValue, + childElements, + findChild, + findChildren, +} from '@core/super-converter/docx-helpers/xml-node-access.js'; import { preProcessVerticalMergeCells } from '@core/super-converter/export-helpers/pre-process-vertical-merge-cells.js'; import { eighthPointsToPixels, halfPointToPoints, twipsToPixels } from '@core/super-converter/helpers.js'; import { buildFallbackGridForTable } from '@core/super-converter/helpers/tableFallbackHelpers.js'; @@ -370,44 +376,40 @@ export function _getReferencedTableStyles(tableStyleReference, params) { // Find the style tag in styles.xml const { docx } = params; - const styles = docx['word/styles.xml']; - const { elements } = styles.elements[0]; - const styleElements = elements.filter((el) => el.name === 'w:style'); - const styleTag = styleElements.find((el) => el.attributes['w:styleId'] === tableStyleReference); + const styles = docx?.['word/styles.xml']; + const styleElements = findChildren(childElements(styles)[0], 'w:style'); + const styleTag = styleElements.find((el) => attrValue(el, 'w:styleId') === tableStyleReference); if (!styleTag) return null; - stylesToReturn.name = styleTag.elements.find((el) => el.name === 'w:name'); + stylesToReturn.name = findChild(styleTag, 'w:name'); // Find style it is based on, if any, to inherit table properties from - const basedOn = styleTag.elements.find((el) => el.name === 'w:basedOn'); + const basedOn = findChild(styleTag, 'w:basedOn'); let baseTblPr; if (basedOn?.attributes) { - const baseStyles = styleElements.find((el) => el.attributes['w:styleId'] === basedOn.attributes['w:val']); - baseTblPr = baseStyles ? baseStyles.elements.find((el) => el.name === 'w:tblPr') : {}; + const baseStyles = styleElements.find((el) => attrValue(el, 'w:styleId') === attrValue(basedOn, 'w:val')); + baseTblPr = baseStyles ? findChild(baseStyles, 'w:tblPr') : {}; } // Find paragraph properties to get justification - const pPr = styleTag.elements.find((el) => el.name === 'w:pPr'); - if (pPr) { - const justification = pPr.elements.find((el) => el.name === 'w:jc'); - if (justification?.attributes) stylesToReturn.justification = justification.attributes['w:val']; - } + const justification = findChild(findChild(styleTag, 'w:pPr'), 'w:jc'); + if (justification?.attributes) stylesToReturn.justification = attrValue(justification, 'w:val'); // Find run properties to get fonts and font size - const rPr = styleTag?.elements.find((el) => el.name === 'w:rPr'); + const rPr = findChild(styleTag, 'w:rPr'); if (rPr) { - const fonts = rPr.elements.find((el) => el.name === 'w:rFonts'); - if (fonts) { + const fonts = findChild(rPr, 'w:rFonts'); + if (fonts?.attributes) { const { 'w:ascii': ascii, 'w:hAnsi': hAnsi, 'w:cs': cs } = fonts.attributes; stylesToReturn.fonts = { ascii, hAnsi, cs }; } - const fontSize = rPr.elements.find((el) => el.name === 'w:sz'); - if (fontSize?.attributes) stylesToReturn.fontSize = halfPointToPoints(fontSize.attributes['w:val']) + 'pt'; + const fontSize = findChild(rPr, 'w:sz'); + if (fontSize?.attributes) stylesToReturn.fontSize = halfPointToPoints(attrValue(fontSize, 'w:val')) + 'pt'; } // Find table properties to get borders and cell margins - const tblPr = styleTag.elements.find((el) => el.name === 'w:tblPr'); + const tblPr = findChild(styleTag, 'w:tblPr'); if (tblPr && tblPr.elements) { // Merge base + current for encoding only; do not mutate styles.xml (would duplicate w:tblCellMar etc. per table using this style) const mergedTblPr = @@ -437,14 +439,12 @@ export function _getReferencedTableStyles(tableStyleReference, params) { } } - const tblStylePr = styleTag.elements.filter((el) => el.name === 'w:tblStylePr'); - let styleProps = {}; - if (tblStylePr) { - styleProps = tblStylePr.reduce((acc, el) => { - acc[el.attributes['w:type']] = tblStylePrTranslator.encode({ ...params, nodes: [el] }); - return acc; - }, {}); - } + // Conditional formatting is keyed by w:type; entries without one cannot be addressed by the cascade. + const styleProps = findChildren(styleTag, 'w:tblStylePr').reduce((acc, el) => { + const conditionalType = attrValue(el, 'w:type'); + if (conditionalType) acc[conditionalType] = tblStylePrTranslator.encode({ ...params, nodes: [el] }); + return acc; + }, {}); return { ...stylesToReturn, From 2a9df40bf3251508cccb63c677c9463d9314f2a0 Mon Sep 17 00:00:00 2001 From: Caio Pizzol <97641911+caio-pizzol@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:23:25 +0000 Subject: [PATCH 2/2] fix(super-converter): keep the document when a style record is unreadable (#3861) Stops one malformed `w:style` from discarding an entire imported document. Same root cause as #3861, much worse outcome, so it is split out for separate review. The style catalogue is built outside the importer per-node recovery boundary. A throw there aborts `createDocumentJson`, `getSchema` returns null, and `createDocument` falls back to an empty document, losing the whole body instead of one node. The `exception` event does fire and hosts can subscribe via `onException`, so it is not strictly silent, but the default is a blank editable page with no error state. - Scope: the fragment in #3861 does not reach this path. It needs an empty `w:outlineLvl`, an empty `w:tab`, a repeated `w:styleId` whose second record is empty, or a `w:style` with no attributes. Whether the reporting customer hit this is unknown without their file. - Guarding the crash was not enough. An outline level with a missing or non-numeric `w:val` still parsed to NaN, and a tab stop missing `w:val` or `w:pos` was still emitted half-formed. Both attributes are required in ECMA-376, so both records are now dropped, and the tests assert parsed values rather than only that import survived. - Style records with no `w:styleId` are skipped, since nothing can reference them, and the latent-style loop that collected into an unread list is gone. Not included: the `getSchema` fallback of returning null and mounting an empty document. Turning a parse failure into a blank page is arguably wrong, but it changes an initialization contract and deserves its own decision. Co-authored-by: Caio Pizzol Source-PR: https://github.com/superdoc/docx-editor/pull/3865 Closes superdoc/docx-editor#3865 Ported-From-Source-Repo: superdoc/orbit Ported-From-Source-Commit: 3e95f6364d02575d3ea78019d3fa2a930617dcc4 Ported-Public-Prefix: superdoc/public --- ...-definition.empty-style-properties.test.js | 130 ++++++++++++++++++ .../get-default-style-definition.js | 115 +++++++++------- ...ocxImporter.empty-style-properties.test.js | 96 ++++++++++--- .../v2/importer/docxImporter.js | 37 ++--- 4 files changed, 279 insertions(+), 99 deletions(-) create mode 100644 packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.empty-style-properties.test.js diff --git a/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.empty-style-properties.test.js b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.empty-style-properties.test.js new file mode 100644 index 0000000000..68928a63c3 --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.empty-style-properties.test.js @@ -0,0 +1,130 @@ +// @ts-check +import { describe, expect, it } from 'vitest'; +import * as xmljs from 'xml-js'; +import { getDefaultStyleDefinition } from './get-default-style-definition.js'; + +/** + * Incomplete leaf properties in `word/styles.xml` (issue #3861). + * + * AIDEV-NOTE: Asserts the parsed values, not just that parsing survived. An + * earlier pass returned NaN for outlineLevel and half-formed tab stops, both of + * which a survival-only test would have missed. Fixtures parse real XML because + * xml-js omits the `elements` key on empty elements. + */ + +const WORDPROCESSING_NS = 'http://schemas.openxmlformats.org/wordprocessingml/2006/main'; + +/** @param {string} styleChildren Raw XML placed inside ``. */ +const parseStyle = (styleChildren) => + getDefaultStyleDefinition('Target', { + 'word/styles.xml': xmljs.xml2js( + ` + + ${styleChildren} + `, + { compact: false }, + ), + }); + +describe('getDefaultStyleDefinition with incomplete properties', () => { + describe('w:outlineLvl', () => { + it('reads a valid level', () => { + expect(parseStyle('').attrs.outlineLevel).toBe(2); + }); + + it('reads a negative level', () => { + expect(parseStyle('').attrs.outlineLevel).toBe(-1); + }); + + it('tolerates surrounding whitespace', () => { + expect(parseStyle('').attrs.outlineLevel).toBe(4); + }); + + // w:val is ST_DecimalNumber (xsd:int). parseInt reads a numeric prefix, so partly + // numeric values used to import as a different, valid-looking level. + it.each([ + ['no w:val', ''], + ['a non-numeric w:val', ''], + ['an empty w:val', ''], + ['a numeric prefix', ''], + ['a decimal', ''], + ['exponent notation', ''], + ['hex notation', ''], + ])('reports null for %s', (_label, outlineLvl) => { + expect(parseStyle(`${outlineLvl}`).attrs.outlineLevel).toBeNull(); + }); + }); + + describe('w:tab', () => { + it('reads a complete tab stop', () => { + expect(parseStyle('').styles.tabStops).toEqual([ + { val: 'start', pos: 48, leader: undefined }, + ]); + }); + + it('reads a negative position', () => { + expect(parseStyle('').styles.tabStops).toEqual([ + { val: 'start', pos: -48, leader: undefined }, + ]); + }); + + // w:val and w:pos are both required on CT_TabStop. An empty w:pos is the sharp + // case: it converts to a plausible-looking 0 rather than an obvious NaN. + it.each([ + ['no attributes', ''], + ['only w:val', ''], + ['only w:pos', ''], + ['an empty w:pos', ''], + ['a whitespace-only w:pos', ''], + ['a non-numeric w:pos', ''], + ])('drops a tab stop with %s', (_label, tab) => { + expect(parseStyle(`${tab}`).styles.tabStops).toBeNull(); + }); + + // AIDEV-NOTE: w:pos is ST_SignedTwipsMeasure, a union of xsd:integer and + // ST_UniversalMeasure, so "1.5in" is legal OOXML that twipsToPixels cannot yet + // convert. It is dropped rather than emitted with an undefined position. Supporting + // universal measures is a separate gap, not a licence to validate w:pos as an integer. + it('drops a universal-measure position it cannot convert', () => { + expect( + parseStyle('').styles.tabStops, + ).toBeNull(); + }); + + it('keeps complete stops alongside incomplete ones', () => { + const styles = parseStyle( + '', + ).styles; + + expect(styles.tabStops).toEqual([{ val: 'end', pos: 96, leader: undefined }]); + }); + }); + + describe('duplicate w:styleId records', () => { + const duplicated = ` + + + + + + `; + + const parsed = () => + getDefaultStyleDefinition('Target', { + 'word/styles.xml': xmljs.xml2js( + `${duplicated}`, + { compact: false }, + ), + }); + + it('takes identity properties from whichever record declares them', () => { + expect(parsed().attrs).toMatchObject({ name: 'FromSecond', basedOn: 'BaseFromSecond', qFormat: true }); + }); + + // Documents the current boundary: only the identity children above look past + // the first record. Paragraph properties are not merged across duplicates. + it('still reads paragraph properties from the first record only', () => { + expect(parsed().styles.textAlign).toBeUndefined(); + }); + }); +}); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.js b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.js index ea20bc5e90..625015f9f7 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/docx-helpers/get-default-style-definition.js @@ -1,6 +1,20 @@ import { parseMarks } from '@converter/v2/importer/index.js'; import { twipsToLines, twipsToPixels } from '@converter/helpers.js'; import { kebabCase } from '@superdoc/common'; +import { attrValue, childElements, findChild, findChildren } from './xml-node-access.js'; + +/** + * First child with the given name across every record sharing a styleId. + * + * Applies only to the identity children read below (w:name, w:basedOn, + * w:qFormat). Paragraph and run properties still come from the first matching + * record alone; this does not merge duplicate style records. + * + * @param {import('./xml-node-access.js').XmlNode[]} records Style elements sharing one w:styleId. + * @param {string} name Qualified child name, e.g. `w:basedOn`. + * @returns {import('./xml-node-access.js').XmlNode | undefined} + */ +const findInAnyRecord = (records, name) => records.map((record) => findChild(record, name)).find(Boolean); /** * Gets the default style definition. @@ -15,32 +29,24 @@ export const getDefaultStyleDefinition = (defaultStyleId, docx) => { const styles = docx['word/styles.xml']; if (!styles) return result; - const { elements } = styles.elements[0]; - const elementsWithId = elements.filter((el) => { - const { attributes } = el; - return attributes && attributes['w:styleId'] === defaultStyleId; - }); + const elementsWithId = childElements(childElements(styles)[0]).filter( + (el) => attrValue(el, 'w:styleId') === defaultStyleId, + ); const firstMatch = elementsWithId[0]; if (!firstMatch) return result; if (!firstMatch.elements) return result; - const qFormat = elementsWithId.find((el) => { - const qFormat = el.elements.find((innerEl) => innerEl.name === 'w:qFormat'); - return qFormat; - }); - - const name = elementsWithId - .find((el) => el.elements.some((inner) => inner.name === 'w:name')) - ?.elements.find((inner) => inner.name === 'w:name')?.attributes['w:val']; + const qFormat = findInAnyRecord(elementsWithId, 'w:qFormat'); + const name = attrValue(findInAnyRecord(elementsWithId, 'w:name'), 'w:val'); // pPr - const pPr = firstMatch.elements.find((el) => el.name === 'w:pPr'); - const spacing = pPr?.elements?.find((el) => el.name === 'w:spacing'); - const justify = pPr?.elements?.find((el) => el.name === 'w:jc'); - const indent = pPr?.elements?.find((el) => el.name === 'w:ind'); - const tabs = pPr?.elements?.find((el) => el.name === 'w:tabs'); + const pPr = findChild(firstMatch, 'w:pPr'); + const spacing = findChild(pPr, 'w:spacing'); + const justify = findChild(pPr, 'w:jc'); + const indent = findChild(pPr, 'w:ind'); + const tabs = findChild(pPr, 'w:tabs'); let lineSpaceBefore, lineSpaceAfter, line; if (spacing?.attributes) { @@ -57,56 +63,63 @@ export const getDefaultStyleDefinition = (defaultStyleId, docx) => { firstLine = twipsToPixels(indent.attributes['w:firstLine']); } - let tabStops = []; - if (tabs) { - tabStops = (tabs.elements || []) - .filter((el) => el.name === 'w:tab') - .map((tab) => { - let val = tab.attributes['w:val']; - if (val == 'left') { - val = 'start'; - } else if (val == 'right') { - val = 'end'; - } - return { - val, - pos: twipsToPixels(tab.attributes['w:pos']), - leader: tab.attributes['w:leader'], - }; - }); - } - - const keepNext = pPr?.elements?.find((el) => el.name === 'w:keepNext'); - const keepLines = pPr?.elements?.find((el) => el.name === 'w:keepLines'); - - const outlineLevel = pPr?.elements?.find((el) => el.name === 'w:outlineLvl'); - const outlineLvlValue = outlineLevel?.attributes['w:val']; - - const pageBreakBefore = pPr?.elements?.find((el) => el.name === 'w:pageBreakBefore'); + // ECMA-376 marks w:val and w:pos required on w:tab (CT_TabStop). w:pos is + // ST_SignedTwipsMeasure, a union of xsd:integer and ST_UniversalMeasure (ยง17.18.81), + // so it is judged by whether it converts to a finite position rather than by matching + // the raw string, which would reject legal values like "1.5in". A record that cannot + // place a stop is dropped rather than emitted half-formed: an empty w:pos otherwise + // converts to a plausible-looking 0. + const tabStops = findChildren(tabs, 'w:tab') + .map((tab) => { + let val = attrValue(tab, 'w:val'); + if (val == 'left') { + val = 'start'; + } else if (val == 'right') { + val = 'end'; + } + const rawPos = attrValue(tab, 'w:pos')?.trim(); + return { + val, + pos: rawPos ? twipsToPixels(rawPos) : undefined, + leader: attrValue(tab, 'w:leader'), + }; + }) + .filter((stop) => stop.val != null && Number.isFinite(stop.pos)); + + const keepNext = findChild(pPr, 'w:keepNext'); + const keepLines = findChild(pPr, 'w:keepLines'); + + // w:val is required on w:outlineLvl and is ST_DecimalNumber (xsd:int). parseInt reads + // a numeric prefix, so "2abc" and "3.9" would import as levels 2 and 3 and silently + // reshape headings and the TOC. The whole value must be an integer or there is no + // level to report. + const outlineLevel = findChild(pPr, 'w:outlineLvl'); + const outlineLvlRaw = attrValue(outlineLevel, 'w:val')?.trim(); + const outlineLvlValue = /^[+-]?\d+$/.test(outlineLvlRaw ?? '') ? Number(outlineLvlRaw) : NaN; + + const pageBreakBefore = findChild(pPr, 'w:pageBreakBefore'); let pageBreakBeforeVal = 0; if (pageBreakBefore) { if (!pageBreakBefore.attributes?.['w:val']) pageBreakBeforeVal = 1; else pageBreakBeforeVal = Number(pageBreakBefore?.attributes?.['w:val']); } - const pageBreakAfter = pPr?.elements?.find((el) => el.name === 'w:pageBreakAfter'); + const pageBreakAfter = findChild(pPr, 'w:pageBreakAfter'); let pageBreakAfterVal; if (pageBreakAfter) { if (!pageBreakAfter.attributes?.['w:val']) pageBreakAfterVal = 1; else pageBreakAfterVal = Number(pageBreakAfter?.attributes?.['w:val']); } - const basedOn = elementsWithId - .find((el) => el.elements.some((inner) => inner.name === 'w:basedOn')) - ?.elements.find((inner) => inner.name === 'w:basedOn')?.attributes['w:val']; + const basedOn = attrValue(findInAnyRecord(elementsWithId, 'w:basedOn'), 'w:val'); - const linkToCharacterStyle = firstMatch.elements.find((el) => el.name === 'w:link')?.attributes?.['w:val'] ?? null; + const linkToCharacterStyle = attrValue(findChild(firstMatch, 'w:link'), 'w:val') ?? null; const parsedAttrs = { name, qFormat: qFormat ? true : false, keepNext: keepNext ? true : false, keepLines: keepLines ? true : false, - outlineLevel: outlineLevel ? parseInt(outlineLvlValue) : null, + outlineLevel: Number.isInteger(outlineLvlValue) ? outlineLvlValue : null, pageBreakBefore: pageBreakBeforeVal ? true : false, pageBreakAfter: pageBreakAfterVal ? true : false, basedOn: basedOn ?? null, @@ -115,7 +128,7 @@ export const getDefaultStyleDefinition = (defaultStyleId, docx) => { }; // rPr - const rPr = firstMatch.elements.find((el) => el.name === 'w:rPr'); + const rPr = findChild(firstMatch, 'w:rPr'); const parsedMarks = parseMarks(rPr, [], docx) || []; const parsedStyles = { spacing: { lineSpaceAfter, lineSpaceBefore, line }, diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js index 4f6baca966..e7652342e0 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.empty-style-properties.test.js @@ -30,21 +30,28 @@ const DOCUMENT_XML = ` `; -/** Styles.xml where Table Grid inherits from a bare base and carries an empty w:rPr. */ -const STYLES_XML = ` - - +/** Table Grid inherits from a bare base and carries an empty w:rPr. */ +const TABLE_STYLES = ` - -`; + `; -const buildDocx = () => ({ +/** + * @param {string} [extraStyles] Raw XML for additional `` elements. + */ +const buildDocx = (extraStyles = '') => ({ 'word/document.xml': parse(DOCUMENT_XML), - 'word/styles.xml': parse(STYLES_XML), + 'word/styles.xml': parse( + ` + + + ${TABLE_STYLES} + ${extraStyles} + `, + ), 'word/_rels/document.xml.rels': parse( ``, ), @@ -65,20 +72,69 @@ const textOf = (node) => { return (node.content ?? []).map(textOf).join(''); }; +/** + * Import a document whose body is always `before` / table / `after`. + * @param {string} [extraStyles] Raw XML for additional `` elements. + */ +const importDocument = (extraStyles) => { + const exceptions = []; + const editor = { + emit: (name, payload) => { + if (name === 'exception') exceptions.push(payload.error); + }, + options: {}, + }; + + const result = createDocumentJson(buildDocx(extraStyles), buildConverter(), editor); + + return { + body: (result?.pmDoc?.content ?? []).map((node) => `${node.type}:${textOf(node)}`), + styleIds: (result?.linkedStyles ?? []).map((style) => style.id), + exceptions, + }; +}; + +const FULL_BODY = ['paragraph:before', 'table:cell', 'paragraph:after']; + describe('DOCX import with empty style property containers', () => { it('keeps a table whose style has an empty w:rPr and a bare basedOn target', () => { - const exceptions = []; - const editor = { - emit: (name, payload) => { - if (name === 'exception') exceptions.push(payload.error); - }, - options: {}, - }; - - const result = createDocumentJson(buildDocx(), buildConverter(), editor); - - const body = (result?.pmDoc?.content ?? []).map((node) => `${node.type}:${textOf(node)}`); - expect(body).toEqual(['paragraph:before', 'table:cell', 'paragraph:after']); + const { body, exceptions } = importDocument(); + + expect(body).toEqual(FULL_BODY); expect(exceptions).toEqual([]); }); + + // AIDEV-NOTE: The style catalogue is built outside the importer's per-node recovery + // boundary, so one unreadable w:style used to abort createDocumentJson entirely. + // getSchema then returned null and createDocument fell back to an empty document, + // losing the whole body rather than one node. These cases must stay import-level. + describe('incomplete style records do not discard the document', () => { + it.each([ + [ + 'w:outlineLvl without w:val', + '', + ], + [ + 'w:tab without attributes', + '', + ], + [ + 'a duplicate w:styleId whose second record is empty', + '', + ], + ['a w:style without attributes', ''], + ])('imports the full body when styles.xml contains %s', (_label, extraStyles) => { + const { body, exceptions } = importDocument(extraStyles); + + expect(body).toEqual(FULL_BODY); + expect(exceptions).toEqual([]); + }); + + it('omits unusable style records from the catalogue but keeps the rest', () => { + const { styleIds } = importDocument(''); + + expect(styleIds).toContain('TableGrid'); + expect(styleIds).not.toContain(undefined); + }); + }); }); diff --git a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js index 9cbb01d27d..237255020e 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/v2/importer/docxImporter.js @@ -23,7 +23,7 @@ import { pictNodeHandlerEntity } from './pictNodeImporter.js'; import { importCommentData } from './documentCommentsImporter.js'; import { buildTrackedChangeIdMap, buildTrackedChangeIdMapsByPart } from './trackedChangeIdMapper.js'; import { importFootnoteData, importEndnoteData } from './documentFootnotesImporter.js'; -import { getDefaultStyleDefinition } from '@converter/docx-helpers/index.js'; +import { attrValue, getDefaultStyleDefinition } from '@converter/docx-helpers/index.js'; import { pruneIgnoredNodes } from './ignoredNodes.js'; import { tabNodeEntityHandler } from './tabImporter.js'; import { noBreakHyphenNodeEntityHandler } from './noBreakHyphenImporter.js'; @@ -725,34 +725,15 @@ function getStyleDefinitions(docx) { if (!styles) return []; const elements = styles.elements?.[0]?.elements ?? []; - const styleDefinitions = elements.filter((el) => el.name === 'w:style'); - - // Track latent style exceptions - const latentStyles = elements.find((el) => el.name === 'w:latentStyles'); - const matchedLatentStyles = []; - (latentStyles?.elements ?? []).forEach((el) => { - const { attributes } = el; - const match = styleDefinitions.find((style) => style.attributes['w:styleId'] === attributes['w:name']); - if (match) matchedLatentStyles.push(el); - }); - - // Parse all styles - const allParsedStyles = []; - styleDefinitions.forEach((style) => { - const id = style.attributes['w:styleId']; - const parsedStyle = getDefaultStyleDefinition(id, docx); - - const importedStyle = { - id: style.attributes['w:styleId'], - type: style.attributes['w:type'], - definition: parsedStyle, - attributes: {}, - }; + // A w:style without w:styleId cannot be referenced by the document, so it is not a usable definition. + const styleDefinitions = elements.filter((el) => el.name === 'w:style' && attrValue(el, 'w:styleId')); - allParsedStyles.push(importedStyle); - }); - - return allParsedStyles; + return styleDefinitions.map((style) => ({ + id: attrValue(style, 'w:styleId'), + type: attrValue(style, 'w:type'), + definition: getDefaultStyleDefinition(attrValue(style, 'w:styleId'), docx), + attributes: {}, + })); } export function translateStyleDefinitions(docx) {