fix: review comments

Author: VladaHarbour

packages/layout-engine/pm-adapter/src/attributes/paragraph.test.ts

@@ -10,7 +10,14 @@
  */
 
 import { describe, it, expect } from 'vitest';
-import { deepClone, normalizeFramePr, normalizeDropCap, computeParagraphAttrs, computeRunAttrs } from './paragraph.js';
+import {
+  deepClone,
+  normalizeFramePr,
+  normalizeDropCap,
+  computeParagraphAttrs,
+  computeRunAttrs,
+  hasExplicitParagraphRunProperties,
+} from './paragraph.js';
 import { twipsToPx } from '../utilities.js';
 
 type PMNode = {
@@ -90,6 +97,12 @@ describe('normalizeDropCap', () => {
 });
 
 describe('computeParagraphAttrs', () => {
+  it('treats only raw paragraph runProperties as explicit', () => {
+    expect(hasExplicitParagraphRunProperties({ runProperties: { fontSize: 24 } } as never)).toBe(true);
+    expect(hasExplicitParagraphRunProperties({ styleId: 'Heading1' } as never)).toBe(false);
+    expect(hasExplicitParagraphRunProperties({ runProperties: {} } as never)).toBe(false);
+  });
+
   it('normalizes spacing, indent, alignment, and tabs from paragraphProperties', () => {
     const paragraph: PMNode = {
       type: { name: 'paragraph' },
@@ -160,6 +173,61 @@ describe('computeParagraphAttrs', () => {
     expect(markerRun?.fontFamily).toContain('MarkerFont');
     expect(markerRun?.fontSize).toBe(11);
   });
+
+  it('does not overwrite numbering marker font family with previousParagraphFont', () => {
+    const previousFont = { fontFamily: 'PrevMarkerFont, sans-serif', fontSize: 11 };
+
+    const paragraph: PMNode = {
+      type: { name: 'paragraph' },
+      attrs: {
+        paragraphProperties: {
+          numberingProperties: { numId: 1, ilvl: 0 },
+        },
+        listRendering: {
+          markerText: '1.',
+          justification: 'left',
+          path: [0],
+          numberingType: 'decimal',
+          suffix: 'tab',
+        },
+      },
+    };
+
+    const minimalContext = {
+      translatedNumbering: {
+        definitions: {
+          '1': {
+            numId: 1,
+            abstractNumId: 1,
+          },
+        },
+        abstracts: {
+          '1': {
+            abstractNumId: 1,
+            levels: {
+              '0': {
+                ilvl: 0,
+                runProperties: {
+                  fontFamily: { ascii: 'Symbol' },
+                },
+              },
+            },
+          },
+        },
+      },
+      translatedLinkedStyles: { docDefaults: {}, styles: {} },
+      tableInfo: null,
+    };
+
+    const { paragraphAttrs } = computeParagraphAttrs(paragraph as never, minimalContext as never, previousFont);
+    const markerRun = (
+      paragraphAttrs as { wordLayout?: { marker?: { run?: { fontFamily?: string; fontSize?: number } } } }
+    )?.wordLayout?.marker?.run;
+
+    expect(markerRun?.fontFamily).toContain('Symbol');
+    // Font size still inherits from previous paragraph when the paragraph has no explicit run props.
+    expect(markerRun?.fontSize).toBe(11);
+  });
 });
 
 describe('computeRunAttrs', () => {
@@ -187,29 +255,6 @@ describe('computeRunAttrs', () => {
     expect(result.vanish).toBe(true);
   });
 
-  it('uses previousParagraphFont for fontFamily and fontSize when provided', () => {
-    const runProps = {};
-    const previousFont = { fontFamily: 'PreviousParaFont, sans-serif', fontSize: 14 };
-
-    const result = computeRunAttrs(runProps as never, undefined, previousFont);
-
-    expect(result.fontFamily).toContain('PreviousParaFont');
-    expect(result.fontSize).toBe(14);
-  });
-
-  it('prefers previousParagraphFont over runProps when both are provided', () => {
-    const runProps = {
-      fontFamily: { ascii: 'RunFont' },
-      fontSize: 22,
-    };
-    const previousFont = { fontFamily: 'PreviousFont, sans-serif', fontSize: 10 };
-
-    const result = computeRunAttrs(runProps as never, undefined, previousFont);
-
-    expect(result.fontFamily).toContain('PreviousFont');
-    expect(result.fontSize).toBe(10);
-  });
-
   it('uses runProps font settings when previousParagraphFont is not provided', () => {
     const runProps = {
       fontFamily: { ascii: 'RunFont' },

packages/layout-engine/pm-adapter/src/attributes/paragraph.ts

@@ -119,6 +119,26 @@ export const normalizeNumberingProperties = (
   }
   return value;
 };
+
+export const hasExplicitParagraphRunProperties = (
+  paragraphProperties?: Pick<ParagraphProperties, 'runProperties'> | null,
+): boolean => paragraphProperties?.runProperties != null && Object.keys(paragraphProperties.runProperties).length > 0;
+
+const applyParagraphFontFallback = (
+  runAttrs: ResolvedRunProperties,
+  previousParagraphFont?: Partial<ParagraphFont>,
+): ResolvedRunProperties => {
+  if (!previousParagraphFont) {
+    return runAttrs;
+  }
+
+  return {
+    ...runAttrs,
+    fontFamily: previousParagraphFont.fontFamily ?? runAttrs.fontFamily,
+    fontSize: previousParagraphFont.fontSize ?? runAttrs.fontSize,
+  };
+};
+
 export const normalizeDropCap = (
   framePr: ParagraphFrameProperties | undefined,
   para: PMNode,
@@ -205,7 +225,6 @@ const extractDropCapRunFromParagraph = (para: PMNode, converterContext?: Convert
   const runAttrs = computeRunAttrs(
     resolvedRunProperties,
     converterContext,
-    undefined,
     DEFAULT_DROP_CAP_FONT_SIZE_PX,
     DEFAULT_DROP_CAP_FONT_FAMILY,
   );
@@ -299,18 +318,49 @@ export const computeParagraphAttrs = (
       Boolean(paragraphProperties.numberingProperties),
     );
 
-    const hasExplicitRunProps =
-      resolvedParagraphProperties.runProperties != null &&
-      Object.keys(resolvedParagraphProperties.runProperties).length > 0;
+    const markerRunAttrs = computeRunAttrs(markerRunProperties, converterContext);
+
+    // Only attempt to inherit `previousParagraphFont` when the paragraph doesn't define
+    // explicit runProperties. Otherwise markerRunProperties/resolveRunProperties already
+    // fully defines marker font.
+    let markerFontFallback: Partial<ParagraphFont> | undefined;
+    if (!hasExplicitParagraphRunProperties(paragraphProperties) && previousParagraphFont) {
+      // Detect whether numbering explicitly overrides the marker font family
+      // (e.g. Symbol/Wingdings). If it does, we must NOT overwrite it.
+      const markerRunPropertiesWithoutNumbering = resolveRunProperties(
+        converterContext!,
+        resolvedParagraphProperties.runProperties,
+        resolvedParagraphProperties,
+        converterContext!.tableInfo,
+        false,
+        Boolean(paragraphProperties.numberingProperties),
+      );
+
+      const markerFontFamilyFromNumbering = resolveDocxFontFamily(
+        markerRunProperties.fontFamily as Record<string, unknown>,
+        converterContext!.docx,
+      );
+      const markerFontFamilyWithoutNumbering = resolveDocxFontFamily(
+        markerRunPropertiesWithoutNumbering.fontFamily as Record<string, unknown>,
+        converterContext!.docx,
+      );
+
+      const numberingDefinesMarkerFontFamily =
+        markerFontFamilyFromNumbering != null && markerFontFamilyFromNumbering !== markerFontFamilyWithoutNumbering;
+
+      markerFontFallback = {
+        // When numbering explicitly sets a marker font (Symbol/Wingdings), keep it.
+        fontFamily: numberingDefinesMarkerFontFamily ? undefined : previousParagraphFont.fontFamily,
+        // Preserve existing behavior: if the paragraph has no explicit run props,
+        // marker font size inherits from the previous paragraph.
+        fontSize: previousParagraphFont.fontSize,
+      };
+    }
 
     paragraphAttrs.wordLayout = computeWordParagraphLayout({
       paragraph: paragraphAttrs,
       listRenderingAttrs: normalizedListRendering,
-      markerRun: computeRunAttrs(
-        markerRunProperties,
-        converterContext,
-        hasExplicitRunProps ? undefined : previousParagraphFont,
-      ),
+      markerRun: applyParagraphFontFallback(markerRunAttrs, markerFontFallback),
     });
   }
 
@@ -320,15 +370,12 @@ export const computeParagraphAttrs = (
 export const computeRunAttrs = (
   runProps: RunProperties,
   converterContext?: ConverterContext,
-  previousParagraphFont?: ParagraphFont,
   defaultFontSizePx = 12,
   defaultFontFamily = 'Times New Roman',
 ): ResolvedRunProperties => {
   let fontFamily;
 
-  if (previousParagraphFont && previousParagraphFont.fontFamily) {
-    fontFamily = previousParagraphFont.fontFamily;
-  } else if (converterContext) {
+  if (converterContext) {
     fontFamily =
       resolveDocxFontFamily(runProps.fontFamily as Record<string, unknown>, converterContext.docx) || defaultFontFamily;
   } else {
@@ -337,13 +384,7 @@ export const computeRunAttrs = (
   }
   const vertAlign = runProps.vertAlign as 'superscript' | 'subscript' | 'baseline' | undefined;
   const hasPosition = runProps.position != null && Number.isFinite(runProps.position);
-  let fontSize;
-
-  if (previousParagraphFont && previousParagraphFont.fontSize) {
-    fontSize = previousParagraphFont.fontSize;
-  } else {
-    fontSize = runProps.fontSize ? ptToPx(runProps.fontSize / 2)! : defaultFontSizePx;
-  }
+  let fontSize = runProps.fontSize ? ptToPx(runProps.fontSize / 2)! : defaultFontSizePx;
 
   // Scale font size for superscript/subscript when no custom position override
   if (!hasPosition && (vertAlign === 'superscript' || vertAlign === 'subscript')) {

packages/layout-engine/pm-adapter/src/converters/paragraph.test.ts

@@ -2987,6 +2987,16 @@ describe('paragraph converters', () => {
 
         it('ignores previousParagraphFont when paragraph has explicit run properties', () => {
           const previousFont = { fontFamily: 'PreviousFont', fontSize: 10 };
+          const paraWithExplicitRunProps: PMNode = {
+            ...emptyNumberedPara,
+            attrs: {
+              paragraphProperties: {
+                numberingProperties: { numId: 1, ilvl: 0 },
+                runProperties: { fontFamily: { ascii: 'ExplicitFont' }, fontSize: 24 },
+              },
+            },
+          };
+
           vi.mocked(computeParagraphAttrs).mockReturnValue({
             paragraphAttrs: {},
             resolvedParagraphProperties: {
@@ -2996,7 +3006,7 @@ describe('paragraph converters', () => {
           });
 
           const blocks = baseParagraphToFlowBlocks({
-            para: emptyNumberedPara,
+            para: paraWithExplicitRunProps,
             nextBlockId,
             positions,
             trackedChangesConfig: undefined,
@@ -3015,6 +3025,36 @@ describe('paragraph converters', () => {
           expect(paraBlock.runs[0].fontFamily).toContain('ExplicitFont');
           expect(paraBlock.runs[0].fontSize).not.toBe(10);
         });
+
+        it('uses previousParagraphFont when run properties are only inherited from styles', () => {
+          const previousFont = { fontFamily: 'PreviousFont', fontSize: 10 };
+          vi.mocked(computeParagraphAttrs).mockReturnValue({
+            paragraphAttrs: {},
+            resolvedParagraphProperties: {
+              numberingProperties: { numId: 1, ilvl: 0 },
+              runProperties: { fontFamily: { ascii: 'StyledFont' }, fontSize: 24 },
+            },
+          });
+
+          const blocks = baseParagraphToFlowBlocks({
+            para: emptyNumberedPara,
+            nextBlockId,
+            positions,
+            trackedChangesConfig: undefined,
+            bookmarks: new Map(),
+            hyperlinkConfig: DEFAULT_HYPERLINK_CONFIG,
+            themeColors: undefined,
+            converters: {} as NestedConverters,
+            converterContext: defaultConverterContext,
+            enableComments: true,
+            previousParagraphFont: previousFont,
+          });
+
+          expect(blocks).toHaveLength(1);
+          const paraBlock = blocks[0] as ParagraphBlock;
+          expect(paraBlock.runs[0].fontFamily).toBe(previousFont.fontFamily);
+          expect(paraBlock.runs[0].fontSize).toBe(previousFont.fontSize);
+        });
       });
 
       it('should clone paragraph attrs for each paragraph block', () => {

packages/layout-engine/pm-adapter/src/converters/paragraph.ts

@@ -8,7 +8,15 @@
  */
 
 import type { ParagraphProperties, RunProperties } from '@superdoc/style-engine/ooxml';
-import type { FlowBlock, ParagraphBlock, Run, TextRun, SdtMetadata, DrawingBlock, TrackedChangeMeta } from '@superdoc/contracts';
+import type {
+  FlowBlock,
+  ParagraphBlock,
+  Run,
+  TextRun,
+  SdtMetadata,
+  DrawingBlock,
+  TrackedChangeMeta,
+} from '@superdoc/contracts';
 import type {
   PMNode,
   PMMark,
@@ -25,7 +33,7 @@ import { trackedChangesCompatible, applyMarksToRun, collectTrackedChangeFromMark
 import { applyTrackedChangesModeToRuns } from '../tracked-changes.js';
 import { textNodeToRun } from './inline-converters/text-run.js';
 import { DEFAULT_HYPERLINK_CONFIG, TOKEN_INLINE_TYPES } from '../constants.js';
-import { computeRunAttrs } from '../attributes/paragraph.js';
+import { computeRunAttrs, hasExplicitParagraphRunProperties } from '../attributes/paragraph.js';
 import { resolveRunProperties } from '@superdoc/style-engine/ooxml';
 import { footnoteReferenceToBlock } from './inline-converters/footnote-reference.js';
 import { endnoteReferenceToBlock } from './inline-converters/endnote-reference.js';
@@ -539,11 +547,10 @@ export function paragraphToFlowBlocks({
 
   // Extract font data for list items
   const extracted = extractDefaultFontProperties(converterContext, resolvedParagraphProperties);
-  const hasExplicitRunProps =
-    resolvedParagraphProperties.runProperties != null &&
-    Object.keys(resolvedParagraphProperties.runProperties).length > 0;
   const usePreviousFont =
-    previousParagraphFont != null && resolvedParagraphProperties.numberingProperties != null && !hasExplicitRunProps;
+    previousParagraphFont != null &&
+    resolvedParagraphProperties.numberingProperties != null &&
+    !hasExplicitParagraphRunProperties(paragraphProps);
   const defaultFont =
     usePreviousFont && previousParagraphFont.fontFamily ? previousParagraphFont.fontFamily : extracted.defaultFont;
   const defaultSize =
@@ -1009,6 +1016,12 @@ export function handleParagraphNode(node: PMNode, context: NodeHandlerContext):
     const { entry: cached, nodeJson, nodeRev } = flowBlockCache.get(prefixedStableId, node);
     if (cached) {
       // Cache hit: reuse blocks with position adjustment
+      // Cache hit reuses previously-converted blocks as-is. That means we don't
+      // recompute previousParagraphFont (used for empty list items without
+      // explicit run properties). If the user changes the font on the prior
+      // paragraph (e.g. paragraph A), an empty list item (paragraph B) can keep
+      // the old font until the cache entry is invalidated. Narrow case, but
+      // avoids confusing incremental-edit behavior.
       const delta = pmStart - cached.pmStart;
       const reusedBlocks = shiftCachedBlocks(cached.blocks, delta);
       applyTrackedGhostListAdjustments(node, reusedBlocks, context);

packages/layout-engine/pm-adapter/src/types.ts

@@ -420,6 +420,6 @@ export interface OoxmlBorder {
 export type UnderlineStyle = NonNullable<import('@superdoc/contracts').TextRun['underline']>['style'];
 
 export type ParagraphFont = {
-  fontFamily?: string;
-  fontSize?: number;
+  fontFamily: string;
+  fontSize: number;
 };