Skip to content

fix(super-converter): tolerate empty style properties on table styles (#3861) - #3864

Closed
caio-pizzol wants to merge 1 commit into
mainfrom
fix/empty-ooxml-style-properties
Closed

fix(super-converter): tolerate empty style properties on table styles (#3861)#3864
caio-pizzol wants to merge 1 commit into
mainfrom
fix/empty-ooxml-style-properties

Conversation

@caio-pizzol

@caio-pizzol caio-pizzol commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Keeps tables in documents whose word/styles.xml carries empty property containers, such as a self-closing <w:rPr/> 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 <w:rPr/>, <w:rPr /> and <w:rPr></w:rPr> 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 #3861

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I attempted to verify against ECMA-376 via the ecma-spec MCP tools, but the tool calls were denied permission in this session (tried several times). I'll base the review on the diff itself and standard WordprocessingML knowledge, and flag that spec lookups couldn't be run.


Status: PASS

This is a defensive refactor, not a semantic change to which OOXML gets read. The whole point of the PR is to tolerate empty property containers (<w:rPr/>, <w:pPr/>, bare <w:style>, etc.) — valid OOXML that xml-js parses without an elements/attributes key. It swaps direct .elements/.attributes dereferences for the new xml-node-access helpers but reads exactly the same elements and attributes as before. No new elements or attributes are introduced, so nothing new to check against the schema.

The elements/attributes touched are all standard WordprocessingML and used correctly:

  • w:style / @w:styleId, w:name, w:basedOn / @w:val — style definitions (spec).
  • w:pPrw:jc / @w:val, w:rPrw:rFonts (@w:ascii/@w:hAnsi/@w:cs, all optional) and w:sz / @w:val — the fonts?.attributes guard is correct since an empty w:rFonts legitimately carries no attributes.
  • w:tblPr, w:tblStylePr / @w:type.

One thing worth calling out as correct: the new code skips a w:tblStylePr that has no w:type. Per the schema w:type is required on that element (spec), so a missing one is malformed input — keying the conditional-formatting map on undefined was the old bug, and dropping the entry (with the explanatory comment) is the right defensive handling rather than a spec violation.

No non-existent attributes/elements, no missing-required regressions (the reads that were mandatory before are still read), and no default-value assumptions were changed.

Note: I couldn't run the ecma-spec verification tools (permission denied), so the schema claims above are from domain knowledge rather than a live spec query — if you want the schema graph consulted (e.g. to double-check w:type is required on CT_TblStylePr), re-run with those MCP tools approved.

xml-js omits the `elements` key entirely for empty elements, so a valid
`<w:rPr/>` on a referenced table style parsed to a node with no children and
`_getReferencedTableStyles` threw while dereferencing it. The importer catches
per-node, so the table was dropped and documents built mostly from tables
imported near-empty.

Adds shared xml-node accessors and routes the table style reader through them.
@caio-pizzol
caio-pizzol force-pushed the fix/empty-ooxml-style-properties branch from f259bf3 to 1f5e584 Compare August 4, 2026 18:04
@caio-pizzol
caio-pizzol marked this pull request as ready for review August 4, 2026 18:09
@caio-pizzol
caio-pizzol requested a review from a team as a code owner August 4, 2026 18:09
@caio-pizzol

Copy link
Copy Markdown
Contributor Author

Ran the schema lookups the review could not, against the ECMA-376 transitional XSD graph.

w:type on CT_TblStylePr is confirmed required:

name use type
type required ST_TblStyleOverrideType

So a w:tblStylePr without it is schema-invalid and cannot be addressed by the cascade. Dropping it is correct rather than a spec violation, which matches the review.

Also confirmed while here, since the same reasoning underpins the rest of the diff:

  • Every child of CT_Style is minOccurs=0, so an empty <w:rPr/>, <w:pPr/> or bare <w:style/> is schema-valid input. That is the premise of the whole fix.
  • w:rFonts carries only optional attributes, so the fonts?.attributes guard the review called out is load-bearing for real files, not defensive noise.

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Powered by Qodo

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 5 files

Re-trigger cubic

@superdoc-bot

superdoc-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

This pull request landed on main as dc1b5b1.

@superdoc-bot superdoc-bot Bot closed this in dc1b5b1 Aug 4, 2026
@caio-pizzol
caio-pizzol deleted the fix/empty-ooxml-style-properties branch August 4, 2026 23:34
@caio-pizzol
caio-pizzol restored the fix/empty-ooxml-style-properties branch August 4, 2026 23:35
@superdoc-orbit

superdoc-orbit Bot commented Aug 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in superdoc-cli v0.23.1

The release is available on GitHub release

@superdoc-orbit

superdoc-orbit Bot commented Aug 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in superdoc-sdk v1.22.1

@superdoc-orbit

superdoc-orbit Bot commented Aug 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in @superdoc-dev/mcp v0.18.1

The release is available on GitHub release

@superdoc-orbit

superdoc-orbit Bot commented Aug 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in superdoc v1.46.1

The release is available on GitHub release

@superdoc-orbit

superdoc-orbit Bot commented Aug 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in @superdoc-dev/react v1.17.1

The release is available on GitHub release

@superdoc-orbit

superdoc-orbit Bot commented Aug 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in vscode-ext v2.18.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty <w:rPr /> (and similar) in styles.xml table styles aborts DOCX import

1 participant