fix(converter): preserve theme rFonts on DOCX round-trip (SD-2894) (#3225)

* fix(converter): preserve theme rFonts on DOCX round-trip (SD-2894)

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.

* fix(super-editor): respect explicit font changes on themed runs (SD-2894 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.

* fix(super-editor): narrow user-override gate to primary fontFamily slot (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).

* test(super-editor): mutation matrix for SD-2894 theme rFonts round-trip

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.

* test(style-engine): pin 4-slot theme/concrete dedup; document plugin 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.

Author: tupizz

packages/layout-engine/style-engine/src/cascade.test.ts

@@ -152,6 +152,109 @@ describe('cascade - combineRunProperties', () => {
       bold: true,
     });
   });
+
+  // SD-2894: each `<w:rFonts>` script slot has both a concrete form (`w:ascii`,
+  // `w:hAnsi`, `w:eastAsia`, `w:cs`) and a theme form (`w:asciiTheme`,
+  // `w:hAnsiTheme`, `w:eastAsiaTheme`, `w:cstheme` — note `cstheme` lowercase).
+  // When a higher-priority source supplies one form for a slot, the cascade must
+  // drop the other form from lower-priority sources, or Word resolves the concrete
+  // name as an override and defeats per-script theme resolution.
+  //
+  // The original SD-2894 bug was that only the (ascii, asciiTheme) pair was
+  // dropping correctly; hAnsi/eastAsia/cs leaked through. These tests pin the
+  // full 4-slot dedup so the bug cannot regress for any single slot.
+  describe('SD-2894 four-slot theme/concrete dedup', () => {
+    it('drops concrete `ascii` from lower when higher supplies `asciiTheme`', () => {
+      const result = combineRunProperties([
+        { fontFamily: { ascii: 'Calibri' } },
+        { fontFamily: { asciiTheme: 'majorBidi' } },
+      ]);
+      expect(result.fontFamily).toEqual({ asciiTheme: 'majorBidi' });
+    });
+
+    it('drops concrete `hAnsi` from lower when higher supplies `hAnsiTheme`', () => {
+      const result = combineRunProperties([
+        { fontFamily: { hAnsi: 'Calibri' } },
+        { fontFamily: { hAnsiTheme: 'majorHAnsi' } },
+      ]);
+      expect(result.fontFamily).toEqual({ hAnsiTheme: 'majorHAnsi' });
+    });
+
+    it('drops concrete `eastAsia` from lower when higher supplies `eastAsiaTheme`', () => {
+      const result = combineRunProperties([
+        { fontFamily: { eastAsia: 'SimSun' } },
+        { fontFamily: { eastAsiaTheme: 'majorEastAsia' } },
+      ]);
+      expect(result.fontFamily).toEqual({ eastAsiaTheme: 'majorEastAsia' });
+    });
+
+    it('drops concrete `cs` from lower when higher supplies `cstheme` (lowercase)', () => {
+      const result = combineRunProperties([
+        { fontFamily: { cs: 'Arial' } },
+        { fontFamily: { cstheme: 'majorBidi' } },
+      ]);
+      expect(result.fontFamily).toEqual({ cstheme: 'majorBidi' });
+    });
+
+    it('drops theme `asciiTheme` from lower when higher supplies concrete `ascii`', () => {
+      const result = combineRunProperties([
+        { fontFamily: { asciiTheme: 'majorBidi' } },
+        { fontFamily: { ascii: 'Arial' } },
+      ]);
+      expect(result.fontFamily).toEqual({ ascii: 'Arial' });
+    });
+
+    it('drops theme `hAnsiTheme` from lower when higher supplies concrete `hAnsi`', () => {
+      const result = combineRunProperties([
+        { fontFamily: { hAnsiTheme: 'majorHAnsi' } },
+        { fontFamily: { hAnsi: 'Arial' } },
+      ]);
+      expect(result.fontFamily).toEqual({ hAnsi: 'Arial' });
+    });
+
+    it('drops theme `eastAsiaTheme` from lower when higher supplies concrete `eastAsia`', () => {
+      const result = combineRunProperties([
+        { fontFamily: { eastAsiaTheme: 'majorEastAsia' } },
+        { fontFamily: { eastAsia: 'Arial' } },
+      ]);
+      expect(result.fontFamily).toEqual({ eastAsia: 'Arial' });
+    });
+
+    it('drops theme `cstheme` from lower when higher supplies concrete `cs`', () => {
+      const result = combineRunProperties([
+        { fontFamily: { cstheme: 'majorBidi' } },
+        { fontFamily: { cs: 'Arial' } },
+      ]);
+      expect(result.fontFamily).toEqual({ cs: 'Arial' });
+    });
+
+    it('Athenaintelligence customer shape: all 4 concretes from defaults dropped by inline themes', () => {
+      // Mirrors the customer fixture: docDefaults supply concrete fonts (Arial),
+      // inline rPr supplies theme refs on ascii/hAnsi/cs (no eastAsiaTheme). The
+      // cascade must keep only the theme refs on those three slots; eastAsia
+      // concrete from defaults is independent.
+      const result = combineRunProperties([
+        { fontFamily: { ascii: 'Arial', hAnsi: 'Arial', cs: 'Arial', eastAsia: 'Arial' } },
+        { fontFamily: { asciiTheme: 'majorBidi', hAnsiTheme: 'majorBidi', cstheme: 'majorBidi' } },
+      ]);
+      expect(result.fontFamily).toEqual({
+        asciiTheme: 'majorBidi',
+        hAnsiTheme: 'majorBidi',
+        cstheme: 'majorBidi',
+        eastAsia: 'Arial',
+      });
+    });
+
+    it('exports FONT_SLOT_THEME_PAIRS so callers (super-editor plugin) can stay in sync', async () => {
+      const { FONT_SLOT_THEME_PAIRS } = await import('./cascade.js');
+      expect(FONT_SLOT_THEME_PAIRS).toEqual([
+        ['ascii', 'asciiTheme'],
+        ['hAnsi', 'hAnsiTheme'],
+        ['eastAsia', 'eastAsiaTheme'],
+        ['cs', 'cstheme'],
+      ]);
+    });
+  });
 });
 
 describe('cascade - combineIndentProperties', () => {

packages/layout-engine/style-engine/src/cascade.ts

@@ -104,20 +104,51 @@ function isObject(item: unknown): item is PropertyObject {
  * @param propertiesArray - Ordered list of run property objects.
  * @returns Combined run property object.
  */
+// SD-2894: `<w:rFonts>` exposes four independent script slots (ascii / hAnsi / eastAsia / cs).
+// Each slot can be filled either by a concrete font name (`w:ascii="Calibri"`) or a theme
+// reference (`w:asciiTheme="majorBidi"`). When a higher-priority style supplies one form for a
+// slot, the other form from lower-priority sources is no longer applicable and must be
+// dropped — otherwise the merged run carries both, and Word resolves the concrete name as an
+// override that defeats per-script theme resolution (Latin body text mapped to `majorBidi`
+// then renders as Calibri Light instead of Times New Roman for Hebrew/Arab).
+//
+// Pair `<slot>` with `<slot>Theme`, except for the cs slot whose theme attribute is the
+// lowercase `cstheme` (OOXML inconsistency, not a typo).
+// Exported so super-editor's calculateInlineRunPropertiesPlugin can consume the same
+// list instead of duplicating it (SD-2894 follow-up).
+export const FONT_SLOT_THEME_PAIRS: Array<[keyof RunFontFamilyProperties, keyof RunFontFamilyProperties]> = [
+  ['ascii', 'asciiTheme'],
+  ['hAnsi', 'hAnsiTheme'],
+  ['eastAsia', 'eastAsiaTheme'],
+  ['cs', 'cstheme'],
+];
+
+function dropConflictingFontSlots(
+  target: RunFontFamilyProperties,
+  source: RunFontFamilyProperties,
+): RunFontFamilyProperties {
+  const result = { ...target };
+  for (const [concreteKey, themeKey] of FONT_SLOT_THEME_PAIRS) {
+    if (source[themeKey] != null) {
+      delete result[concreteKey];
+      delete result[themeKey];
+    } else if (source[concreteKey] != null) {
+      delete result[themeKey];
+    }
+  }
+  return result;
+}
+
 export function combineRunProperties(propertiesArray: RunProperties[]): RunProperties {
   return combineProperties(propertiesArray, {
     fullOverrideProps: ['color'],
     specialHandling: {
       fontFamily: (target: Record<string, unknown>, source: Record<string, unknown>): unknown => {
         const fontFamilySource = { ...(source.fontFamily as object) } as RunFontFamilyProperties;
-        const fontFamilyTarget = { ...(target.fontFamily as object) } as RunFontFamilyProperties;
-        if (fontFamilySource.asciiTheme != null) {
-          delete fontFamilyTarget.ascii;
-          delete fontFamilyTarget.asciiTheme;
-        }
-        if (fontFamilySource.ascii != null) {
-          delete fontFamilyTarget.asciiTheme;
-        }
+        const fontFamilyTarget = dropConflictingFontSlots(
+          (target.fontFamily as RunFontFamilyProperties) ?? {},
+          fontFamilySource,
+        );
         return { ...(fontFamilyTarget as object), ...(fontFamilySource as object) };
       },
     },

packages/layout-engine/style-engine/src/ooxml/index.ts

@@ -5,7 +5,12 @@
  * This module is format-aware (docx), but translator-agnostic.
  */
 
-import { combineIndentProperties, combineProperties, combineRunProperties } from '../cascade.js';
+import {
+  combineIndentProperties,
+  combineProperties,
+  combineRunProperties,
+  FONT_SLOT_THEME_PAIRS,
+} from '../cascade.js';
 import type { PropertyObject } from '../cascade.js';
 import type { ParagraphConditionalFormatting, ParagraphProperties, ParagraphTabStop, RunProperties } from './types.ts';
 import type { NumberingProperties } from './numbering-types.ts';
@@ -18,7 +23,7 @@ import type {
   TableCellProperties,
 } from './styles-types.ts';
 
-export { combineIndentProperties, combineProperties, combineRunProperties };
+export { combineIndentProperties, combineProperties, combineRunProperties, FONT_SLOT_THEME_PAIRS };
 export type { PropertyObject };
 export type * from './types.ts';
 export type * from './numbering-types.ts';

packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js

@@ -2,6 +2,7 @@ import { Plugin, TextSelection } from 'prosemirror-state';
 import { Fragment } from 'prosemirror-model';
 import { TableMap } from 'prosemirror-tables';
 import { decodeRPrFromMarks, encodeMarksFromRPr, resolveRunProperties } from '@converter/styles.js';
+import { FONT_SLOT_THEME_PAIRS } from '@superdoc/style-engine/ooxml';
 import {
   calculateResolvedParagraphProperties,
   getResolvedParagraphProperties,
@@ -28,6 +29,85 @@ const RUN_PROPERTIES_DERIVED_FROM_MARKS = new Set([
 
 export const TRANSIENT_HYPERLINK_STYLE_IDS = new Set(['Hyperlink', 'FollowedHyperlink']);
 
+/**
+ * Merge mark-derived concrete fontFamily slots with theme references preserved on the
+ * existing run. For each slot where the existing fontFamily has a theme reference, keep
+ * the theme and drop the concrete value from marks; otherwise take the mark value.
+ *
+ * Only safe to call when the caller has already verified that marks are a re-derivation
+ * of `existing` rather than a user override — otherwise this silently reverts user font
+ * changes. See `marksMatchExistingFontFamily` for the gate.
+ *
+ * SD-2894: when the plugin overrides a run's fontFamily from mark-derived values, the
+ * mark only carries the resolved CSS font name (`ascii: "Calibri Light"`) and forgets the
+ * theme reference (`asciiTheme: "majorBidi"`) that came from the imported `<w:rFonts>`.
+ * Without this merge we'd export concrete font names that defeat Word's per-script theme
+ * resolution.
+ *
+ * AIDEV-NOTE: The gate + merge pair operate at the fontFamily-key level (whole rPr
+ * fontFamily object), not at the per-slot level. A user action that mutates only a
+ * per-script mark attr (`eastAsiaFontFamily` or `csFontFamily`) without changing the
+ * primary `fontFamily` will pass the gate, and this merge will silently restore the
+ * stale theme on that slot. We accept this trade-off because (a) the toolbar's
+ * setFontFamily writes all four slots to the same family, so a real user override
+ * always changes the primary; (b) no current API or paste path produces per-script-only
+ * mark changes against an imported theme run; (c) a per-slot model adds intricate
+ * encoder probes that risk regressing the SD-2894 customer fixture again (see commit
+ * bec7e63ed for the prior per-script regression we just fixed). If a real customer
+ * reproduction surfaces — e.g. a paste-from-Word flow with per-script fonts being
+ * silently reverted — file a follow-up ticket with that fixture and rework this helper
+ * into a per-slot decision then. Codex flagged this on the PR-3225 review.
+ *
+ * @param {Record<string, unknown>|null|undefined} fromMarks
+ * @param {Record<string, unknown>|null|undefined} existing
+ * @returns {Record<string, unknown>}
+ */
+function mergeFontFamilyPreservingThemeRefs(fromMarks, existing) {
+  const merged = { ...(fromMarks || {}) };
+  if (!existing || typeof existing !== 'object') return merged;
+  for (const [concreteKey, themeKey] of FONT_SLOT_THEME_PAIRS) {
+    if (existing[themeKey] != null) {
+      merged[themeKey] = existing[themeKey];
+      delete merged[concreteKey];
+    }
+  }
+  return merged;
+}
+
+/**
+ * True when the mark-derived fontFamily value encodes to the same primary ASCII font as
+ * the run's existing (imported) fontFamily — i.e., marks are a faithful re-derivation,
+ * not a user override.
+ *
+ * The encoder resolves theme references to their CSS font name. So:
+ *   - import case: existing = { asciiTheme: 'majorBidi' }, marks = { ascii: 'Calibri Light' }
+ *     → both encode to fontFamily: 'Calibri Light' → match → preserve theme.
+ *   - user-edit case: existing = { asciiTheme: 'majorBidi' }, marks = { ascii: 'Arial' }
+ *     → existing encodes to 'Calibri Light', marks encode to 'Arial' → mismatch → user
+ *       has overridden, drop the theme and respect the new value.
+ *
+ * Compare ONLY the primary `fontFamily` attr, not the full mark attrs object. Per-script
+ * extras (`csFontFamily`, `eastAsiaFontFamily`) on the mark are derived only when the
+ * input has a *concrete* per-script slot. Existing run-props can carry mixed shapes
+ * (e.g. `{ asciiTheme, hAnsiTheme, cstheme, eastAsia: 'Arial' }`) that encode without
+ * `csFontFamily` while the mark re-decode adds a concrete `cs` and therefore does
+ * include it. A full-attrs JSON compare would falsely flag this as user override and
+ * drop the theme — the regression caught on the SD-2894 customer fixture round-trip.
+ *
+ * @param {{ attrs?: Record<string, unknown> } | null | undefined} markFromMarks
+ * @param {Record<string, unknown>|null|undefined} existingFontFamily
+ * @param {(props: Record<string, unknown>, docx: Record<string, unknown>) => Array<{ attrs?: Record<string, unknown> }>} encode
+ * @param {Record<string, unknown>} docx
+ * @returns {boolean}
+ */
+function marksMatchExistingFontFamily(markFromMarks, existingFontFamily, encode, docx) {
+  if (!existingFontFamily || typeof existingFontFamily !== 'object') return false;
+  if (!markFromMarks?.attrs) return false;
+  const markFromExisting = encode({ fontFamily: existingFontFamily }, docx)?.[0];
+  if (!markFromExisting?.attrs) return false;
+  return markFromMarks.attrs.fontFamily === markFromExisting.attrs.fontFamily;
+}
+
 const RUN_PROPERTY_PRESERVE_META_KEY = 'sdPreserveRunPropertiesKeys';
 const COMPANION_INLINE_KEYS = {
   fontSizeCs: 'fontSize',
@@ -536,10 +616,23 @@ function getInlineRunProperties(
     const valueFromStyles = runPropertiesFromStyles[key];
     if (JSON.stringify(valueFromMarks) !== JSON.stringify(valueFromStyles)) {
       if (key === 'fontFamily') {
-        const markFromStyles = encodeMarksFromRPr({ [key]: valueFromStyles }, editor.converter?.convertedXml ?? {})[0];
-        const markFromMarks = encodeMarksFromRPr({ [key]: valueFromMarks }, editor.converter?.convertedXml ?? {})[0];
+        const docx = editor.converter?.convertedXml ?? {};
+        const markFromStyles = encodeMarksFromRPr({ [key]: valueFromStyles }, docx)[0];
+        const markFromMarks = encodeMarksFromRPr({ [key]: valueFromMarks }, docx)[0];
         if (JSON.stringify(markFromMarks?.attrs) !== JSON.stringify(markFromStyles?.attrs)) {
-          inlineRunProperties[key] = valueFromMarks;
+          // SD-2894 follow-up (PR #3225 review): only preserve theme refs from `existing`
+          // when the marks are a faithful re-derivation of the imported value. If marks
+          // diverge from existing (user picked a new font), respect the user's choice
+          // and drop the stale theme reference.
+          const existingFontFamily = existingRunProperties?.[key];
+          inlineRunProperties[key] = marksMatchExistingFontFamily(
+            markFromMarks,
+            existingFontFamily,
+            encodeMarksFromRPr,
+            docx,
+          )
+            ? mergeFontFamilyPreservingThemeRefs(valueFromMarks, existingFontFamily)
+            : valueFromMarks;
         }
       } else {
         inlineRunProperties[key] = valueFromMarks;