fix(converter): preserve theme rFonts on DOCX round-trip (SD-2894)#3225
Conversation
The cascade's font-slot dedup only handled the ascii/asciiTheme pair, so a higher-priority style with hAnsiTheme/eastAsiaTheme/cstheme would still leave the lower-priority concrete name in place. Word reads the concrete value as an override that defeats per-script theme resolution — Latin body text mapped to majorBidi rendered as Calibri Light instead of the per-script font (Times New Roman for Hebrew/Arab). Extend the dedup to all four <w:rFonts> slots, and add the same logic to calculateInlineRunPropertiesPlugin so theme refs from the imported run survive the mark-driven recompute that runs on initial load. Verified with a round-trip integration test on the customer fixture: asciiTheme count 65 → 65 (was dropping to ~25 before the fix), no concrete font substitutions on runs that originally carried theme references.
superdoc-bot
Bot
added
review: careful
review: thorough
and removed
review: careful
labels
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey @tupizz!
I had a look at the code and found a few things. I will add them as inline comments.
The main problem I found was flagged by codex:
- [P1] Let explicit font changes replace stale theme refs — packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js:57-59
When text imported with w:*Theme rFonts later has a concrete font applied by the user, fromMarks contains the newly selected font while existing still contains the old theme reference. This branch unconditionally copies the old theme ref and deletes the concrete mark value, so the export keeps the original theme font instead of the user's font change for those runs.
I tested this in the browser and indeed it causes an issue. If you try to load the document below into the editor, then select the text that says "Change this text to Arial" and try to actually change the font, you'll see that nothing happens. This was working correctly in the main branch so the problem is being caused by the changes in this PR.
theme-rfonts-explicit-font-change-repro.docx
Another thing I will ask is that you run visual tests after making these adjustments since this change has the potential to impact the rendering of a lot of documents.
…894 review) Addresses Luccas's PR review feedback on #3225. P1: when text imported with w:*Theme rFonts is later given a concrete font by the user, mergeFontFamilyPreservingThemeRefs unconditionally restored the old theme reference and dropped the user's choice. setFontFamily('Arial') on themed text appeared to do nothing on export. Fix: gate the merge on whether the mark-derived value still encodes to the same OOXML mark as the run's existing fontFamily. If they match, the marks are a faithful re-derivation of the import and we preserve the theme. If they differ, the user has overridden — drop the theme and keep the new concrete value. DRY: FONT_FAMILY_THEME_PAIRS in the plugin duplicated FONT_SLOT_THEME_PAIRS in style-engine cascade.ts. Export the cascade constant through @superdoc/style-engine/ooxml and consume it from the plugin so the four-slot mapping has a single source of truth. Tests: two new AAA cases in calculateInlineRunPropertiesPlugin.test.js lock both directions of the contract — preserve theme when marks re-encode the import, drop theme on user override.
|
Thanks @luccas-harbour — both points addressed in P1 (user override silently reverted): I was unconditionally restoring theme refs from Reproduced your DRY ( Safeguards: Two new AAA tests in
Both fail loudly if either branch flips in the future. Visual tests: queued — will post results in a follow-up comment once they finish. Test results from this commit:
|
…ot (SD-2894) The previous gate (f046df9) compared the full encoded textStyle attrs between markFromMarks and markFromExisting. That over-fired on imported runs whose existing.fontFamily mixed theme refs with a concrete per-script slot (e.g. { asciiTheme, hAnsiTheme, cstheme, eastAsia: 'Arial' }): the mark re-decode produces a full concrete shape including a `cs:` slot, which the encoder turns into `csFontFamily` on markFromMarks but not on markFromExisting, so JSON.stringify(attrs) mismatched and the theme was dropped. Browser round-trip of the SD-2894 customer fixture: 65/65/65 -> 62/62/62 theme attrs, with 3 `w:ascii="Calibri Light"` substitutions. Regression of the original PR. Fix: compare only `markFromMarks.attrs.fontFamily` against `markFromExisting.attrs.fontFamily`. That is the primary user-override signal; per-script extras don't indicate user intent (the toolbar sets all four slots to the same family). Confirmed in browser: - customer fixture round-trip: 65/65/65, zero CL substitutions - Luccas user-override repro: setFontFamily('Arial') -> exported <w:rFonts w:ascii="Arial" w:hAnsi="Arial" w:eastAsia="Arial" w:cs="Arial"/> Adds a third regression test pinning the mixed theme+concrete-per-script case that the suite previously missed (because the integration test uses isHeadless: true which bypasses the plugin entirely).
13 parametrized scenarios pin every shape of existing.fontFamily x mark-decoded shape that the plugin must handle on round-trip: Group A — theme preservation when marks re-encode the imported value: A1: all 4 theme slots A2: 3 theme slots (ascii/hAnsi/cs) — SD-2894 customer fixture shape A3: 3 themes + concrete eastAsia — the regression case the gate now handles A4: ascii-only theme A5: hAnsi-only theme A6: cs-only theme (lowercase `cstheme` OOXML quirk) A7: eastAsia-only theme Group B — user override drops theme refs: B1: all 4 themes + Arial B2: customer shape + Arial (Luccas's case) B3: mixed theme+concrete + Arial Group C — no-theme baselines: C1: pure concrete unchanged C2: pure concrete + different concrete C3: empty existing The universal encoder mock mirrors resolveDocxFontFamily's behaviour: any theme ref wins for the primary slot, falling back to concrete ascii/hAnsi/etc. Per-script extras (eastAsiaFontFamily/csFontFamily) attach only when concrete slot value differs from primary fontFamily — same emission rule as the real encoder in super-converter/styles.js. These scenarios are the basis for catching any future drift in the gate logic or merge helper — both the import-preservation path and the user-override path are locked at every shape we ship.
|
Update: pushed Scenarios coveredGroup A — theme preservation (marks re-encode imported value → keep theme refs)
Group B — user override (marks diverge → drop theme refs)
Group C — no-theme baselines
Encoder mock fidelityThe universal encoder mirrors Results
The matrix will fail loudly if the gate logic or merge helper ever drift. Together with the smaller unit tests already on this PR, both the preservation path AND the user-override path are now locked at every shape we ship. |
…limitation Addresses two codex review findings on PR #3225: (1) Cascade test coverage: the original SD-2894 bug was that combineRunProperties' slot dedup only handled (ascii, asciiTheme) correctly; hAnsi/eastAsia/cs leaked. Existing cascade tests didn't directly exercise the four-slot behaviour. Adds: - one test per slot for the "theme drops concrete" direction - one test per slot for the "concrete drops theme" direction - an Athenaintelligence-customer-shape test - an export assertion pinning FONT_SLOT_THEME_PAIRS so it stays in sync with the super-editor plugin that re-imports it These prevent any single-slot regression of the original SD-2894 fix. (2) Per-script gate limitation: the plugin's gate + merge pair operate at the fontFamily-key level, not per-slot. A user action that mutates only eastAsiaFontFamily or csFontFamily without changing the primary fontFamily will pass the gate. Documented as an AIDEV-NOTE on mergeFontFamilyPreservingThemeRefs. We accept the trade-off because (a) the toolbar's setFontFamily always changes the primary, (b) no current API or paste path produces this shape against a themed import, (c) the prior attempt at increased precision (f046df9) regressed the customer fixture (3 of 65 runs); narrowing to per-slot would re-open that risk without a concrete reproduction. If a real customer scenario surfaces, file a follow-up with the fixture and rework into per-slot decisions then.
luccas-harbour
deleted the
tadeu/sd-2894-preserve-theme-rfonts-on-round-trip
branch
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.93 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.137 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.139 |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.108 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.30.0-next.88 The release is available on GitHub release |
4 participants
Summary
combineRunPropertiesfont-slot dedup to cover all four<w:rFonts>script slots (was onlyascii/asciiTheme), so a higher-priority style withhAnsiTheme/eastAsiaTheme/csthemeno longer leaves the lower-priority concrete name in place.calculateInlineRunPropertiesPluginso theme references survive the mark-driven recompute that runs after import.Why
Customer fixture in SD-2894 had every body run mapped to
<w:rFonts w:asciiTheme="majorBidi" w:hAnsiTheme="majorBidi" w:cstheme="majorBidi"/>. The cascade only knew how to droptarget.asciiwhensource.asciiThemewas present — the other three slots fell through, so the merged run carried{ ascii: "Arial", hAnsi: "Arial", cs: "Arial", asciiTheme: "majorBidi", hAnsiTheme: "majorBidi", cstheme: "majorBidi" }. Word treats the concrete value as an override that defeats per-script theme resolution: Latin body text mapped tomajorBidirendered as Calibri Light instead of the per-script font (Times New Roman for Hebrew/Arab).A second loss happened in
calculateInlineRunPropertiesPluginon initial load — the plugin computes fontFamily from textStyle marks (which only carry concrete CSS names) and overroderunProperties.fontFamilywith concrete-only values, dropping the theme references that the import had stored.Test plan
sd-2894-theme-rfonts-roundtrip.test.js) that round-trips the customer fixture and assertsw:asciiTheme/w:hAnsiTheme/w:csthemecounts don't drop and that no source theme reference becomes a concrete font on export.ooxml-rFonts-rstyle-linked-combos-roundtriptest still passes.pnpm testforsuper-editor(12 708 / 12 721 pass).@superdoc/style-enginecascade unit tests pass.asciiThemecount: 65 → 65 (was 65 → ~25). No Calibri Light substitutions on body text. Per-script resolution intact.Verified diffs on the customer fixture
w:asciiThemeindocument.xmlw:hAnsiThemeindocument.xmlw:csthemeindocument.xmlw:ascii="Calibri Light"runs indocument.xml