fix(page-number): address PAGE field review feedback

Author: luccas-harbour

packages/layout-engine/layout-bridge/src/layoutHeaderFooter.ts

@@ -1,4 +1,11 @@
-import type { FlowBlock, HeaderFooterLayout, Measure, ParagraphBlock, TableBlock } from '@superdoc/contracts';
+import type {
+  FlowBlock,
+  HeaderFooterLayout,
+  ListBlock,
+  Measure,
+  ParagraphBlock,
+  TableBlock,
+} from '@superdoc/contracts';
 import { layoutHeaderFooter, type HeaderFooterConstraints } from '@superdoc/layout-engine';
 import { MeasureCache } from './cache';
 import { resolveHeaderFooterTokens, cloneHeaderFooterBlocks } from './resolveHeaderFooterTokens';
@@ -121,10 +128,31 @@ function paragraphHasPageToken(para: ParagraphBlock): boolean {
   return false;
 }
 
+function paragraphHasVariableWidthRunLocalPageFormat(para: ParagraphBlock): boolean {
+  for (const run of para.runs) {
+    if (
+      'token' in run &&
+      run.token === 'pageNumber' &&
+      'pageNumberFormat' in run &&
+      run.pageNumberFormat &&
+      run.pageNumberFormat !== 'decimal' &&
+      run.pageNumberFormat !== 'numberInDash'
+    ) {
+      return true;
+    }
+  }
+  return false;
+}
+
 function hasPageTokens(blocks: FlowBlock[]): boolean {
   for (const block of blocks) {
     if (block.kind === 'paragraph') {
       if (paragraphHasPageToken(block as ParagraphBlock)) return true;
+    } else if (block.kind === 'list') {
+      const list = block as ListBlock;
+      for (const item of list.items ?? []) {
+        if (paragraphHasPageToken(item.paragraph)) return true;
+      }
     } else if (block.kind === 'table') {
       // SD-1332: PAGE fields can live inside table cells in headers/footers
       // (Word's typical layout). Skipping tables here would take the
@@ -146,6 +174,32 @@ function hasPageTokens(blocks: FlowBlock[]): boolean {
   return false;
 }
 
+function hasVariableWidthRunLocalPageFormat(blocks: FlowBlock[]): boolean {
+  for (const block of blocks) {
+    if (block.kind === 'paragraph') {
+      if (paragraphHasVariableWidthRunLocalPageFormat(block as ParagraphBlock)) return true;
+    } else if (block.kind === 'list') {
+      const list = block as ListBlock;
+      for (const item of list.items ?? []) {
+        if (paragraphHasVariableWidthRunLocalPageFormat(item.paragraph)) return true;
+      }
+    } else if (block.kind === 'table') {
+      const table = block as TableBlock;
+      for (const row of table.rows ?? []) {
+        for (const cell of row.cells ?? []) {
+          const cellBlocks: FlowBlock[] = cell.blocks
+            ? (cell.blocks as FlowBlock[])
+            : cell.paragraph
+              ? [cell.paragraph]
+              : [];
+          if (hasVariableWidthRunLocalPageFormat(cellBlocks)) return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
 export class HeaderFooterLayoutCache {
   private readonly cache = new MeasureCache<Measure>();
 
@@ -266,7 +320,7 @@ export async function layoutHeaderFooterWithCache(
     // Determine which pages to create layouts for
     let pagesToLayout: number[];
 
-    if (!useBucketing) {
+    if (!useBucketing || hasVariableWidthRunLocalPageFormat(blocks)) {
       // Small doc: create layout for every page
       pagesToLayout = Array.from({ length: docTotalPages }, (_, i) => i + 1);
       HeaderFooterCacheLogger.logBucketingDecision(docTotalPages, false);

packages/layout-engine/layout-bridge/test/layoutHeaderFooterBucketing.test.ts

@@ -89,6 +89,20 @@ const makePageTokenBlock = (id: string): FlowBlock => ({
   ],
 });
 
+const makeFormattedPageTokenBlock = (id: string, pageNumberFormat: TextRun['pageNumberFormat']): FlowBlock => ({
+  kind: 'paragraph',
+  id,
+  runs: [
+    {
+      text: '0',
+      token: 'pageNumber',
+      pageNumberFormat,
+      fontFamily: 'Arial',
+      fontSize: 12,
+    } as TextRun,
+  ],
+});
+
 describe('getBucketForPageNumber', () => {
   it('should return d1 for single-digit page numbers (1-9)', () => {
     expect(getBucketForPageNumber(1)).toBe('d1');
@@ -389,6 +403,59 @@ describe('layoutHeaderFooterWithCache - Digit Bucketing (Large Docs)', () => {
     expect(pageNumbers).toContain(500); // d3
     expect(pageNumbers).not.toContain(5000); // d4 not needed
   });
+
+  it('should disable bucketing for variable-width run-local page number formats', async () => {
+    const sections = {
+      default: [makeFormattedPageTokenBlock('header-roman', 'upperRoman')],
+    };
+
+    const pageResolver: PageResolver = (pageNum) => ({
+      displayText: String(pageNum),
+      displayNumber: pageNum,
+      totalPages: 150,
+    });
+
+    const measureBlock = vi.fn(async () => makeMeasure(20));
+    const result = await layoutHeaderFooterWithCache(
+      sections,
+      { width: 400, height: 80 },
+      measureBlock,
+      undefined,
+      undefined,
+      pageResolver,
+    );
+
+    expect(result.default?.layout.pages).toHaveLength(150);
+    expect(measureBlock).toHaveBeenCalledTimes(150);
+  });
+
+  it.each([
+    ['decimal', 'decimal'],
+    ['numberInDash', 'numberInDash'],
+  ] as const)('should keep bucketing for %s run-local page number format', async (_name, pageNumberFormat) => {
+    const sections = {
+      default: [makeFormattedPageTokenBlock(`header-${pageNumberFormat}`, pageNumberFormat)],
+    };
+
+    const pageResolver: PageResolver = (pageNum) => ({
+      displayText: String(pageNum),
+      displayNumber: pageNum,
+      totalPages: 150,
+    });
+
+    const measureBlock = vi.fn(async () => makeMeasure(20));
+    const result = await layoutHeaderFooterWithCache(
+      sections,
+      { width: 400, height: 80 },
+      measureBlock,
+      undefined,
+      undefined,
+      pageResolver,
+    );
+
+    expect(result.default?.layout.pages).toHaveLength(3);
+    expect(measureBlock).toHaveBeenCalledTimes(3);
+  });
 });
 
 describe('layoutHeaderFooterWithCache - Section-Aware Token Resolution', () => {

packages/layout-engine/painters/dom/src/renderer-page-context-patch.test.ts

@@ -0,0 +1,116 @@
+import { describe, expect, it } from 'vitest';
+import type { FlowBlock, Layout, Measure, TextRun } from '@superdoc/contracts';
+import { createTestPainter } from './_test-utils.js';
+
+const pageNumberBlock: FlowBlock = {
+  kind: 'paragraph',
+  id: 'page-number-block',
+  runs: [
+    {
+      text: '0',
+      token: 'pageNumber',
+      pageNumberFormat: 'upperRoman',
+      fontFamily: 'Arial',
+      fontSize: 12,
+    } as TextRun,
+  ],
+};
+
+const pageNumberMeasure: Measure = {
+  kind: 'paragraph',
+  lines: [
+    {
+      fromRun: 0,
+      fromChar: 0,
+      toRun: 0,
+      toChar: 1,
+      width: 10,
+      ascent: 8,
+      descent: 2,
+      lineHeight: 10,
+    },
+  ],
+  totalHeight: 10,
+};
+
+const staticBlock: FlowBlock = {
+  kind: 'paragraph',
+  id: 'static-block',
+  runs: [
+    {
+      text: 'Static',
+      fontFamily: 'Arial',
+      fontSize: 12,
+    },
+  ],
+};
+
+const staticMeasure: Measure = {
+  kind: 'paragraph',
+  lines: [
+    {
+      fromRun: 0,
+      fromChar: 0,
+      toRun: 0,
+      toChar: 6,
+      width: 40,
+      ascent: 8,
+      descent: 2,
+      lineHeight: 10,
+    },
+  ],
+  totalHeight: 10,
+};
+
+function makeLayout(displayNumber: number): Layout {
+  return {
+    pageSize: { w: 400, h: 500 },
+    pages: [
+      {
+        number: 1,
+        displayNumber,
+        fragments: [
+          {
+            kind: 'para',
+            blockId: 'page-number-block',
+            fromLine: 0,
+            toLine: 1,
+            x: 0,
+            y: 0,
+            width: 200,
+          },
+          {
+            kind: 'para',
+            blockId: 'static-block',
+            fromLine: 0,
+            toLine: 1,
+            x: 0,
+            y: 20,
+            width: 200,
+          },
+        ],
+      },
+    ],
+  };
+}
+
+describe('DomPainter page-number context patching', () => {
+  it('rebuilds token fragments when display page number changes during incremental patch', () => {
+    const mount = document.createElement('div');
+    document.body.appendChild(mount);
+
+    const painter = createTestPainter({
+      blocks: [pageNumberBlock, staticBlock],
+      measures: [pageNumberMeasure, staticMeasure],
+    });
+
+    painter.paint(makeLayout(5), mount);
+    expect(mount.textContent).toContain('V');
+    const staticFragment = mount.querySelector('[data-block-id="static-block"]');
+    expect(staticFragment).toBeTruthy();
+
+    painter.paint(makeLayout(8), mount);
+    expect(mount.textContent).toContain('VIII');
+    expect(mount.querySelector('[data-block-id="static-block"]')).toBe(staticFragment);
+  });
+});

packages/layout-engine/painters/dom/src/renderer.ts

@@ -38,6 +38,7 @@ import type {
   ResolvedDrawingItem,
   LayoutSourceIdentity,
   LayoutStoryLocator,
+  ListBlock,
 } from '@superdoc/contracts';
 import {
   LAYOUT_BOUNDARY_SCHEMA,
@@ -258,6 +259,59 @@ type PageDomState = {
   fragments: FragmentDomState[];
 };
 
+function pageContextSignature(context: FragmentRenderContext): string {
+  return [
+    context.pageNumber,
+    context.totalPages,
+    context.pageNumberText ?? '',
+    context.pageNumberDisplayNumber ?? '',
+  ].join('|');
+}
+
+function hasPageContextTokenInBlock(block: FlowBlock | undefined): boolean {
+  if (!block) return false;
+  if (block.kind === 'paragraph') {
+    for (const run of (block as ParagraphBlock).runs) {
+      if ('token' in run && (run.token === 'pageNumber' || run.token === 'totalPageCount')) {
+        return true;
+      }
+    }
+  } else if (block.kind === 'list') {
+    const list = block as ListBlock;
+    for (const item of list.items ?? []) {
+      if (hasPageContextTokenInBlock(item.paragraph)) {
+        return true;
+      }
+    }
+  } else if (block.kind === 'table') {
+    const table = block as TableBlock;
+    for (const row of table.rows ?? []) {
+      for (const cell of row.cells ?? []) {
+        const cellBlocks: FlowBlock[] = cell.blocks
+          ? (cell.blocks as FlowBlock[])
+          : cell.paragraph
+            ? [cell.paragraph]
+            : [];
+        if (cellBlocks.some(hasPageContextTokenInBlock)) {
+          return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
+function needsRebuildForPageContext(
+  currentContext: FragmentRenderContext,
+  nextContext: FragmentRenderContext,
+  resolvedItem: ResolvedPaintItem | undefined,
+): boolean {
+  const block = resolvedItem?.kind === 'fragment' && 'block' in resolvedItem ? resolvedItem.block : undefined;
+  return (
+    pageContextSignature(currentContext) !== pageContextSignature(nextContext) && hasPageContextTokenInBlock(block)
+  );
+}
+
 /**
  * Rendering context passed to fragment renderers containing page metadata.
  * Provides information about the current page position and section for dynamic content like page numbers.
@@ -2296,6 +2350,7 @@ export class DomPainter {
           (current.element.dataset.betweenBorder === 'true') !== (betweenInfo?.showBetweenBorder ?? false) ||
           (current.element.dataset.suppressTopBorder === 'true') !== (betweenInfo?.suppressTopBorder ?? false) ||
           (current.element.dataset.gapBelow ?? '') !== (betweenInfo?.gapBelow ? String(betweenInfo.gapBelow) : '');
+        const pageContextChanged = needsRebuildForPageContext(current.context, contextBase, resolvedItem);
         // Verify the position mapping is reliable: if mapping the old pmStart doesn't produce
         // the expected new pmStart, the mapping is degenerate (e.g. full-document paste) and
         // we must rebuild to get correct span position attributes.
@@ -2311,6 +2366,7 @@ export class DomPainter {
           current.signature !== resolvedSig ||
           sdtBoundaryMismatch ||
           betweenBorderMismatch ||
+          pageContextChanged ||
           mappingUnreliable;
 
         if (needsRebuild) {

packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/page-instruction.js

@@ -1,6 +1,7 @@
 const PAGE_VALUE_FORMAT_SWITCHES = {
   Arabic: 'decimal',
   Roman: 'upperRoman',
+  ROMAN: 'upperRoman',
   roman: 'lowerRoman',
   ALPHABETIC: 'upperLetter',
   alphabetic: 'lowerLetter',

packages/super-editor/src/editors/v1/core/super-converter/field-references/fld-preprocessors/page-preprocessor.test.js

@@ -22,6 +22,7 @@ describe('preProcessPageInstruction', () => {
     ['PAGE', undefined],
     ['PAGE \\* roman', 'lowerRoman'],
     ['PAGE \\* Roman \\* MERGEFORMAT', 'upperRoman'],
+    ['PAGE \\* ROMAN', 'upperRoman'],
     ['page \\* Arabic', 'decimal'],
     ['PAGE \\* Unsupported \\* MERGEFORMAT', undefined],
   ])('preserves PAGE instruction and parses supported value format: %s', (instruction, pageNumberFormat) => {