diff --git a/packages/layout-engine/layout-engine/src/index.test.ts b/packages/layout-engine/layout-engine/src/index.test.ts index f6e3f4fe97..17a987c358 100644 --- a/packages/layout-engine/layout-engine/src/index.test.ts +++ b/packages/layout-engine/layout-engine/src/index.test.ts @@ -2458,6 +2458,48 @@ describe('layoutDocument', () => { expect(layout.pages[1].fragments[0].blockId).toBe('p2'); }); + it('skips empty sectPr marker paragraph before continuous section break (SD-2735)', () => { + // IT-945 shape: "This is my first section" + empty sectPr-marker + + // continuous break into 2-col section. The marker is logically a + // section-properties container, not a visible paragraph. Rendering it + // at docDefaults height would cost a row per column on page 1. + const blocks: FlowBlock[] = [ + { kind: 'paragraph', id: 'p1', runs: [{ text: 'First section', fontFamily: 'Arial', fontSize: 12 }] }, + { + kind: 'paragraph', + id: 'p-marker', + runs: [{ text: '', fontFamily: 'Arial', fontSize: 12 }], + attrs: { sectPrMarker: true }, + }, + { + kind: 'sectionBreak', + id: 'sb1', + type: 'continuous', + margins: {}, + pageSize: { w: 612, h: 792 }, + columns: { count: 2, gap: 36 }, + attrs: { source: 'sectPr' }, + }, + { kind: 'paragraph', id: 'p2', runs: [{ text: 'In 2-col section', fontFamily: 'Arial', fontSize: 12 }] }, + ]; + const measures: Measure[] = [ + { kind: 'paragraph', lines: [makeLine(20)], totalHeight: 20 }, + { kind: 'paragraph', lines: [makeLine(16)], totalHeight: 16 }, + { kind: 'sectionBreak' }, + { kind: 'paragraph', lines: [makeLine(20)], totalHeight: 20 }, + ]; + + const layout = layoutDocument(blocks, measures, { + pageSize: { w: 612, h: 792 }, + margins: { top: 72, right: 72, bottom: 72, left: 72 }, + }); + + // Both visible paragraphs land on page 1; the marker does not appear. + expect(layout.pages).toHaveLength(1); + const fragmentIds = layout.pages[0].fragments.map((f) => f.blockId); + expect(fragmentIds).toEqual(['p1', 'p2']); + }); + it('skips empty sectPr marker paragraph before forced section break', () => { const blocks: FlowBlock[] = [ { kind: 'paragraph', id: 'p1', runs: [{ text: 'Content', fontFamily: 'Arial', fontSize: 12 }] }, diff --git a/packages/layout-engine/layout-engine/src/index.ts b/packages/layout-engine/layout-engine/src/index.ts index c6d5e91903..17997dab08 100644 --- a/packages/layout-engine/layout-engine/src/index.ts +++ b/packages/layout-engine/layout-engine/src/index.ts @@ -2170,25 +2170,23 @@ export function layoutDocument(blocks: FlowBlock[], measures: Measure[], options if (isEmpty) { const isSectPrMarker = paraBlock.attrs?.sectPrMarker === true; - // Check if previous block was pageBreak and next block is sectionBreak const prevBlock = index > 0 ? blocks[index - 1] : null; const nextBlock = index < blocks.length - 1 ? blocks[index + 1] : null; - - const nextSectionBreak = nextBlock?.kind === 'sectionBreak' ? (nextBlock as SectionBreakBlock) : null; - const nextBreakType = - nextSectionBreak?.type ?? (nextSectionBreak?.attrs?.source === 'sectPr' ? 'nextPage' : undefined); - const nextBreakForcesPage = - nextSectionBreak && - (nextBreakType === 'nextPage' || - nextBreakType === 'evenPage' || - nextBreakType === 'oddPage' || - nextSectionBreak.attrs?.requirePageBoundary === true); - - if (isSectPrMarker && nextBreakForcesPage) { + const nextIsSectionBreak = nextBlock?.kind === 'sectionBreak'; + + // A section-break marker paragraph (`` with + // no runs) carries only section metadata in Word — it's not a visible + // paragraph. Skip it regardless of break type (nextPage/continuous/ + // evenPage/oddPage) so intro spacing matches Word for all section + // transitions. Per ECMA-376 §17.6.17 the sectPr defines where the + // section ends, not a rendered paragraph. + if (isSectPrMarker && nextIsSectionBreak) { continue; } - if (prevBlock?.kind === 'pageBreak' && nextBlock?.kind === 'sectionBreak') { + // Pre-sectPrMarker path: empty paragraph sandwiched between an + // explicit page break and a section break — same "pure marker" shape. + if (prevBlock?.kind === 'pageBreak' && nextIsSectionBreak) { continue; } } diff --git a/packages/layout-engine/measuring/dom/src/fontMetricsCache.test.ts b/packages/layout-engine/measuring/dom/src/fontMetricsCache.test.ts index d9aec22840..8f146b9f03 100644 --- a/packages/layout-engine/measuring/dom/src/fontMetricsCache.test.ts +++ b/packages/layout-engine/measuring/dom/src/fontMetricsCache.test.ts @@ -359,4 +359,48 @@ describe('fontMetricsCache', () => { expect(metrics.ascent).toBeGreaterThan(metrics.descent); }); }); + + describe('naturalSingleLine calibration (SD-2735)', () => { + // Per ECMA-376 §17.18.48, spacing multipliers are relative to the font's + // "natural single line height". JSDOM's Canvas implementation cannot read + // the actual ascent/descent from Microsoft's Aptos/Calibri font files, so + // we calibrate these fonts by measured-vs-Word ratio. + + it('populates naturalSingleLine for Aptos at 12pt matching Word (~19.5px)', () => { + const metrics = getFontMetrics(ctx, { fontFamily: 'Aptos', fontSize: 16 }, 'browser', defaultFonts); + expect(metrics.naturalSingleLine).toBeDefined(); + // 16px × 1.218 = 19.488 — Word renders Aptos 12pt at ~19.5px + expect(metrics.naturalSingleLine!).toBeCloseTo(19.488, 1); + }); + + it('populates naturalSingleLine for Calibri at 12pt (~19.5px)', () => { + const metrics = getFontMetrics(ctx, { fontFamily: 'Calibri', fontSize: 16 }, 'browser', defaultFonts); + expect(metrics.naturalSingleLine).toBeDefined(); + expect(metrics.naturalSingleLine!).toBeCloseTo(19.504, 1); + }); + + it('falls back to canvas measurement for fonts without calibration', () => { + const metrics = getFontMetrics(ctx, { fontFamily: 'Arial', fontSize: 16 }, 'browser', defaultFonts); + // Arial has no entry in the calibration table; naturalSingleLine may + // still come through from fontBoundingBox* when the canvas provides it. + // Either a canvas-sourced value or undefined is acceptable here. + if (metrics.naturalSingleLine != null) { + expect(metrics.naturalSingleLine).toBeGreaterThan(0); + expect(metrics.naturalSingleLine).toBeLessThan(30); + } + }); + + it('scales naturalSingleLine linearly with font size for Aptos', () => { + const m12 = getFontMetrics(ctx, { fontFamily: 'Aptos', fontSize: 16 }, 'browser', defaultFonts); + const m24 = getFontMetrics(ctx, { fontFamily: 'Aptos', fontSize: 32 }, 'browser', defaultFonts); + // Calibration is a pure multiplier: doubling fontSize doubles naturalSingle. + expect(m24.naturalSingleLine! / m12.naturalSingleLine!).toBeCloseTo(2, 2); + }); + + it('matches Aptos Display and Aptos to the same calibration', () => { + const m1 = getFontMetrics(ctx, { fontFamily: 'Aptos', fontSize: 16 }, 'browser', defaultFonts); + const m2 = getFontMetrics(ctx, { fontFamily: 'Aptos Display', fontSize: 16 }, 'browser', defaultFonts); + expect(m1.naturalSingleLine).toBeCloseTo(m2.naturalSingleLine!, 2); + }); + }); }); diff --git a/packages/layout-engine/measuring/dom/src/fontMetricsCache.ts b/packages/layout-engine/measuring/dom/src/fontMetricsCache.ts index 1c8aa75f60..025ef3090e 100644 --- a/packages/layout-engine/measuring/dom/src/fontMetricsCache.ts +++ b/packages/layout-engine/measuring/dom/src/fontMetricsCache.ts @@ -23,10 +23,49 @@ export type FontInfo = { /** * Measured font metrics from the Canvas API. + * + * `ascent`/`descent` are painted bounds from `actualBoundingBox*` (used for + * glyph-clipping guards). `naturalSingleLine` is the font's intrinsic + * single-line height used by `w:lineRule="auto"` — from the calibration + * table when present, else from `fontBoundingBox*` when the browser exposes + * it. Undefined signals "caller pick a fallback". */ export type FontMetricsResult = { ascent: number; descent: number; + naturalSingleLine?: number; +}; + +/** + * Natural-single-line multipliers for fonts whose Word-rendered metrics + * differ from what the browser Canvas can measure. The browser does not have + * access to Microsoft's internal font metrics unless the font is bundled, + * so Aptos/Calibri/Cambria must be calibrated against Word's actual output. + * + * Keyed by primary family name lowercased. Add entries when corpus evidence + * shows a systematic gap between Canvas measurement and Word. + */ +const FONT_NATURAL_LINE_CALIBRATION: Record = { + aptos: 1.218, + 'aptos display': 1.218, + calibri: 1.219, + 'calibri light': 1.219, + cambria: 1.219, +}; + +const primaryFontFamily = (fontFamily: string): string => { + if (typeof fontFamily !== 'string') return ''; + const first = fontFamily.split(',')[0] ?? ''; + return first + .trim() + .replace(/^['"]|['"]$/g, '') + .toLowerCase(); +}; + +export const getCalibratedNaturalSingleLine = (fontFamily: string, fontSizePx: number): number | undefined => { + const multiplier = FONT_NATURAL_LINE_CALIBRATION[primaryFontFamily(fontFamily)]; + if (multiplier == null || !Number.isFinite(fontSizePx) || fontSizePx <= 0) return undefined; + return fontSizePx * multiplier; }; /** @@ -219,7 +258,19 @@ export function getFontMetrics( descent = fontInfo.fontSize * 0.2; } - const result: FontMetricsResult = { ascent, descent }; + // Prefer explicit Word-calibration; fall back to the browser's + // fontBoundingBox* when it's exposed; otherwise leave undefined. + let naturalSingleLine = getCalibratedNaturalSingleLine(fontInfo.fontFamily, fontInfo.fontSize); + if ( + naturalSingleLine == null && + typeof textMetrics.fontBoundingBoxAscent === 'number' && + typeof textMetrics.fontBoundingBoxDescent === 'number' && + textMetrics.fontBoundingBoxAscent > 0 + ) { + naturalSingleLine = textMetrics.fontBoundingBoxAscent + textMetrics.fontBoundingBoxDescent; + } + + const result: FontMetricsResult = { ascent, descent, naturalSingleLine }; // Cache the result (with size limit) if (fontMetricsCache.size >= MAX_CACHE_SIZE) { diff --git a/packages/layout-engine/measuring/dom/src/index.test.ts b/packages/layout-engine/measuring/dom/src/index.test.ts index a2a0ee80f6..cf3ddcc6ad 100644 --- a/packages/layout-engine/measuring/dom/src/index.test.ts +++ b/packages/layout-engine/measuring/dom/src/index.test.ts @@ -1002,9 +1002,13 @@ describe('measureBlock', () => { }; const measure = expectParagraphMeasure(await measureBlock(block, 400)); - // `lineUnit: "multiplier"` applies directly to fontSize. - // (pm-adapter already bakes the OOXML auto 1.15 factor into the multiplier value.) - expect(measure.lines[0].lineHeight).toBeCloseTo(1.5 * fontSize, 1); + // `lineUnit: "multiplier"` now applies to naturalSingleLine per + // ECMA-376 §17.18.48 auto rule `(line/240) × naturalSingle`. For fonts + // without explicit calibration, naturalSingle falls back to + // WORD_SINGLE_LINE_SPACING_MULTIPLIER × fontSize = 1.15 × 16 = 18.4. + // → 1.5 × 18.4 = 27.6. + const baseNaturalSingle = 1.15 * fontSize; // 18.4 fallback for Arial + expect(measure.lines[0].lineHeight).toBeCloseTo(1.5 * baseNaturalSingle, 1); }); it('applies higher auto multipliers to the baseline line height', async () => { @@ -1025,7 +1029,10 @@ describe('measureBlock', () => { }; const measure = expectParagraphMeasure(await measureBlock(block, 400)); - expect(measure.lines[0].lineHeight).toBeCloseTo(2 * fontSize, 1); + // ECMA-376 §17.18.48 auto rule: multiplier × naturalSingle (not fontSize). + // For uncalibrated Arial, naturalSingle falls back to 1.15 × fontSize. + const baseNaturalSingle = 1.15 * fontSize; // 18.4 + expect(measure.lines[0].lineHeight).toBeCloseTo(2 * baseNaturalSingle, 1); }); it('applies large auto values as multipliers', async () => { @@ -1045,7 +1052,8 @@ describe('measureBlock', () => { }; const measure = expectParagraphMeasure(await measureBlock(block, 400)); - expect(measure.lines[0].lineHeight).toBeCloseTo(42 * 16, 1); + // 42 × naturalSingle (fallback 1.15 × 16 = 18.4) = 772.8. + expect(measure.lines[0].lineHeight).toBeCloseTo(42 * 1.15 * 16, 1); }); it('does not clamp line height for very small fonts', async () => { @@ -1135,6 +1143,74 @@ describe('measureBlock', () => { expect(measure.lines[0].lineHeight).toBeCloseTo(baseLineHeight, 1); }); + it('honours auto lineRule with a sub-baseline multiplier (no silent min-clamp) per ECMA-376 §17.18.48', async () => { + // SD-2735 regression: `auto` explicitly has "no predetermined minimum + // or maximum" per spec. A multiplier of 0.5 should tighten lines, + // not be silently inflated to the baseline 1.15 × fontSize as the + // pre-fix code did (it reused the atLeast max-clamp branch for auto). + const fontSize = 16; + const block: FlowBlock = { + kind: 'paragraph', + id: 'tight-spacing', + runs: [ + { + text: 'Tight auto spacing', + fontFamily: 'Arial', + fontSize, + }, + ], + attrs: { + spacing: { line: 0.5, lineUnit: 'multiplier', lineRule: 'auto' }, + }, + }; + + const measure = expectParagraphMeasure(await measureBlock(block, 400)); + // 0.5 × naturalSingle (= 0.5 × 18.4 = 9.2 for uncalibrated Arial). + // Previously the `auto` path max-clamped this up to 18.4, eating the + // author's explicit half-spacing intent. After the §17.18.48 rule + // fix the multiplier applies cleanly, even if the result is still + // smaller than the font's cap height (caller is responsible for + // glyph-clipping guards elsewhere). + const baseNaturalSingle = 1.15 * fontSize; // 18.4 fallback + expect(measure.lines[0].lineHeight).toBeCloseTo(0.5 * baseNaturalSingle, 1); + }); + + it('applies multiplier × naturalSingle to empty paragraphs in calibrated fonts (SD-2735)', async () => { + // Caio's third PR-review repro (arial-empty-rows.docx): empty cell + // paragraphs go through `calculateEmptyParagraphMetrics`, which does + // not plumb the font's calibrated `naturalSingleLine` through to + // `resolveLineHeight`. For uncalibrated fonts the bug hides because + // naturalSingle ≈ WORD_SINGLE_LINE_SPACING_MULTIPLIER × fontSize + // (18.4 px at 16 px) matches the fallback floor inside resolveLineHeight. + // For Aptos (calibrated to ~19.5 px at 16 px), the gap surfaces: + // empty rows measure 1.15 × fontSize instead of the calibrated value, + // so empty cells render shorter than text-bearing cells in the same + // table. The fix is symmetric with `calculateTypographyMetrics`: + // populate naturalSingle from the calibration table when fontInfo is + // present, and forward it to resolveLineHeight. + const fontSize = 16; // 12 pt + const block: FlowBlock = { + kind: 'paragraph', + id: 'empty-aptos', + runs: [ + { + text: '', + fontFamily: 'Aptos', + fontSize, + }, + ], + attrs: { + spacing: { line: 1.5, lineUnit: 'multiplier', lineRule: 'auto' }, + }, + }; + + const measure = expectParagraphMeasure(await measureBlock(block, 400)); + // Aptos calibrated naturalSingle ≈ 19.488 px (see fontMetricsCache.test.ts). + // 1.5 × 19.488 ≈ 29.2. + const aptosNaturalSingle = 19.488; + expect(measure.lines[0].lineHeight).toBeCloseTo(1.5 * aptosNaturalSingle, 0); + }); + it('ensures line height is never smaller than glyph bounds to prevent clipping', async () => { // This test verifies the clamp: Math.max(fontSize * 1.15, ascent + descent) // For any font, line height must be >= ascent + descent to prevent glyph overlap diff --git a/packages/layout-engine/measuring/dom/src/index.ts b/packages/layout-engine/measuring/dom/src/index.ts index 39e85716cf..87a129c28a 100644 --- a/packages/layout-engine/measuring/dom/src/index.ts +++ b/packages/layout-engine/measuring/dom/src/index.ts @@ -423,6 +423,12 @@ function calculateTypographyMetrics( const resolvedFontSize = normalizeFontSize(fontSize); let ascent: number; let descent: number; + // `naturalSingle` is the font's intrinsic single-line-height — what Word + // uses as the base for `w:lineRule="auto"` rendering. Per-font calibration + // table in fontMetricsCache populates this for fonts where Word's intrinsic + // metrics diverge from the browser's (Aptos, Calibri). Absent calibration, + // we fall back to ascent + descent below. + let naturalSingle: number | undefined; if ( fontInfo && @@ -435,13 +441,14 @@ function calculateTypographyMetrics( const metrics = getFontMetrics(ctx, fontInfo, measurementConfig.mode, measurementConfig.fonts); ascent = roundValue(metrics.ascent); descent = roundValue(metrics.descent); + naturalSingle = metrics.naturalSingleLine; } else { // Fallback approximations for empty paragraphs or missing font info ascent = roundValue(resolvedFontSize * 0.8); descent = roundValue(resolvedFontSize * 0.2); } - const lineHeight = resolveLineHeight(spacing, fontSize, ascent + descent); + const lineHeight = resolveLineHeight(spacing, fontSize, naturalSingle ?? ascent + descent); return { ascent, @@ -502,6 +509,13 @@ function calculateEmptyParagraphMetrics( const resolvedFontSize = normalizeFontSize(fontSize); let ascent: number; let descent: number; + // Symmetric with `calculateTypographyMetrics`: when fontInfo is present, + // forward the calibrated `naturalSingleLine` so `resolveLineHeight` scales + // a multiplier off Word's intrinsic single-line height (Aptos/Calibri etc.) + // instead of the WORD_SINGLE_LINE_SPACING_MULTIPLIER × fontSize fallback. + // Otherwise empty cells render shorter than text-bearing cells under the + // same auto multiplier. + let naturalSingle: number | undefined; if ( fontInfo && @@ -513,14 +527,15 @@ function calculateEmptyParagraphMetrics( const metrics = getFontMetrics(ctx, fontInfo, measurementConfig.mode, measurementConfig.fonts); ascent = roundValue(metrics.ascent); descent = roundValue(metrics.descent); + naturalSingle = metrics.naturalSingleLine; } else { ascent = roundValue(resolvedFontSize * 0.8); descent = roundValue(resolvedFontSize * 0.2); } // Word treats empty paragraphs as a single font-sized line unless line spacing is explicitly set. - const maxLineHeight = Math.max(resolvedFontSize, ascent + descent); - const lineHeight = roundValue(resolveLineHeight(spacing, resolvedFontSize, maxLineHeight)); + const fallbackNaturalSingle = Math.max(resolvedFontSize, ascent + descent); + const lineHeight = roundValue(resolveLineHeight(spacing, resolvedFontSize, naturalSingle ?? fallbackNaturalSingle)); return { ascent, @@ -3543,17 +3558,38 @@ const appendSegment = ( }); }; -const resolveLineHeight = (spacing: ParagraphSpacing | undefined, fontSize: number, maxHeight: number = -1): number => { - let computedHeight = spacing?.line ?? WORD_SINGLE_LINE_SPACING_MULTIPLIER; - if (spacing?.lineUnit === 'multiplier') { - computedHeight = computedHeight * fontSize; - } +/** + * Resolve paragraph line height per ECMA-376 §17.18.48 (ST_LineSpacingRule). + * + * | lineRule | Result | + * |----------|---------------------------------------------------------| + * | auto | `target` — no min, no max | + * | atLeast | `max(target, naturalSingle)` | + * | exact | `target` (clipped by caller if content taller) | + * + * For `lineUnit='multiplier'`, `target = line × naturalSingle` — the adapter + * has already divided `w:line` by 240, so `line` is the dimensionless ratio. + * + * `naturalSingleLine` is floored at `WORD_SINGLE_LINE_SPACING_MULTIPLIER × + * fontSize` because many text fonts measure smaller than Word's Normal-style + * baseline; without the floor, unspaced paragraphs would render tighter than + * users expect. + */ +const resolveLineHeight = ( + spacing: ParagraphSpacing | undefined, + fontSize: number, + naturalSingleLine: number = -1, +): number => { + const fallbackFloor = WORD_SINGLE_LINE_SPACING_MULTIPLIER * fontSize; + const naturalSingle = naturalSingleLine > 0 ? Math.max(naturalSingleLine, fallbackFloor) : fallbackFloor; - const lineRule = spacing?.lineRule ?? 'auto'; - if (['atLeast', 'auto'].includes(lineRule)) { - return Math.max(computedHeight, maxHeight, WORD_SINGLE_LINE_SPACING_MULTIPLIER * fontSize); - } - return computedHeight; + if (!spacing || spacing.line == null) return naturalSingle; + + const target = spacing.lineUnit === 'multiplier' ? spacing.line * naturalSingle : spacing.line; + const rule = spacing.lineRule ?? 'auto'; + + if (rule === 'atLeast') return Math.max(target, naturalSingle); + return target; }; const sanitizePositive = (value: number | undefined): number => diff --git a/packages/layout-engine/pm-adapter/src/attributes/paragraph.ts b/packages/layout-engine/pm-adapter/src/attributes/paragraph.ts index d7dd40ce57..5631b0b1d4 100644 --- a/packages/layout-engine/pm-adapter/src/attributes/paragraph.ts +++ b/packages/layout-engine/pm-adapter/src/attributes/paragraph.ts @@ -14,7 +14,8 @@ import { type ParagraphIndent, type DropCapDescriptor, type DropCapRun, - type ParagraphFrame, ParagraphDirectionContext + type ParagraphFrame, + ParagraphDirectionContext, } from '@superdoc/contracts'; import type { PMNode, ParagraphFont } from '../types.js'; import type { ResolvedRunProperties } from '@superdoc/word-layout'; diff --git a/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.test.ts b/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.test.ts index 0ad57727a7..4e9b804a59 100644 --- a/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.test.ts +++ b/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.test.ts @@ -45,7 +45,12 @@ describe('normalizeParagraphSpacing', () => { it('converts auto line values > 10 from 240ths of a line', () => { const spacing = { line: 360, lineRule: 'auto' as const } as ParagraphSpacing; // 1.5x const result = normalizeParagraphSpacing(spacing, false); - expect(result?.line).toBeCloseTo(1.725, 5); + // Per ECMA-376 §17.18.48: w:line=360 with lineRule=auto means 360/240 = 1.5 + // multiplier of natural single-line height. The pre-fix code multiplied by an + // extra 1.15 so measuring-dom (which used to do mult × fontSize) coincidentally + // matched naturalSingle. Now that measuring-dom does mult × naturalSingle, the + // 1.15 fudge would double-apply. + expect(result?.line).toBeCloseTo(1.5, 5); expect(result?.lineRule).toBe('auto'); }); @@ -83,7 +88,10 @@ describe('normalizeParagraphSpacing', () => { it('returns undefined for empty or invalid inputs', () => { expect(normalizeParagraphSpacing(undefined, false)).toBeUndefined(); expect(normalizeParagraphSpacing(null as never, false)).toBeUndefined(); - expect(normalizeParagraphSpacing({} as ParagraphSpacing, false)).toEqual({ line: 1.15, lineUnit: 'multiplier' }); + // Empty spacing → single-spacing (1.0 × naturalSingle) per spec. The pre-fix + // default of 1.15 was load-bearing only because measuring-dom didn't have the + // naturalSingle concept; with the §17.18.48 model the canonical default is 1.0. + expect(normalizeParagraphSpacing({} as ParagraphSpacing, false)).toEqual({ line: 1.0, lineUnit: 'multiplier' }); }); it('skips non-numeric values but preserves valid ones', () => { diff --git a/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.ts b/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.ts index 426e10ad03..b85d0eb531 100644 --- a/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.ts +++ b/packages/layout-engine/pm-adapter/src/attributes/spacing-indent.ts @@ -9,8 +9,18 @@ import type { ParagraphAttrs, ParagraphSpacing } from '@superdoc/contracts'; import type { ParagraphSpacing as OoxmlParagraphSpacing } from '@superdoc/style-engine/ooxml'; import { twipsToPx, pickNumber } from '../utilities.js'; -const AUTO_SPACING_DEFAULT_MULTIPLIER = 1.15; - +// Per ECMA-376 §17.18.48 (ST_LineSpacingRule), `lineRule="auto"` means: +// `lineHeight = (w:line / 240) × naturalSingleLineHeight`. +// `w:line/240` is a dimensionless ratio of the font's intrinsic single-line +// height; the multiplication against natural-single happens in measuring-dom +// (`resolveLineHeight`). Earlier versions baked an extra 1.15 here so that +// measuring-dom's pre-§17.18.48 formula `mult × fontSize` coincidentally +// produced approximately the right pixels for Word's Normal style. After +// SD-2735 measuring-dom does `mult × naturalSingle` end-to-end; the fudge +// would double-apply, so we keep this normalization at the spec-canonical +// pure ratio. +const SINGLE_LINE_AUTO_DEFAULT = 1.0; +const AUTO_SPACING_BEFORE_AFTER_DEFAULT_MULTIPLIER = 1.15; const AUTO_SPACING_LINE_DEFAULT = 240; // Default OOXML auto line spacing in twips /** @@ -104,14 +114,14 @@ export const normalizeParagraphSpacing = ( if (isList) { before = undefined; } else { - before = (lineRaw ?? AUTO_SPACING_LINE_DEFAULT) * AUTO_SPACING_DEFAULT_MULTIPLIER; + before = (lineRaw ?? AUTO_SPACING_LINE_DEFAULT) * AUTO_SPACING_BEFORE_AFTER_DEFAULT_MULTIPLIER; } } if (afterAutospacing) { if (isList) { after = undefined; } else { - after = (lineRaw ?? AUTO_SPACING_LINE_DEFAULT) * AUTO_SPACING_DEFAULT_MULTIPLIER; + after = (lineRaw ?? AUTO_SPACING_LINE_DEFAULT) * AUTO_SPACING_BEFORE_AFTER_DEFAULT_MULTIPLIER; } } @@ -127,23 +137,28 @@ export const normalizeParagraphSpacing = ( }; /** - * Normalizes line spacing value based on line rule. - * Converts OOXML line spacing values to a multiplier of font size. - * @param value - OOXML line spacing value in twips + * Normalizes the OOXML `w:line` value into the canonical pipeline format + * consumed by `measuring-dom`. Per ECMA-376 §17.18.48 (ST_LineSpacingRule): + * + * - `auto`: `w:line` is in 240ths-of-a-line. Output is the dimensionless + * ratio `value/240`; measuring-dom multiplies it by the font's natural + * single-line height. + * - `atLeast`/`exact`: `w:line` is in twentieths-of-a-point. Output is in + * pixels (twips → px). + * - missing rule: treat like `auto` per OOXML default but emit the pure + * ratio (no fudge). + * + * @param value - OOXML line spacing value * @param lineRule - Line rule ('auto', 'exact', 'atLeast') - * @returns Normalized line spacing value as a multiplier, or undefined */ export const normalizeLineValue = ( value: number | undefined, lineRule: ParagraphSpacing['lineRule'] | undefined, ): { value: number; unit: 'multiplier' | 'px' } => { - if (value == null) return { value: AUTO_SPACING_DEFAULT_MULTIPLIER, unit: 'multiplier' }; + if (value == null) return { value: SINGLE_LINE_AUTO_DEFAULT, unit: 'multiplier' }; if (lineRule == 'exact' || lineRule == 'atLeast') { return { value: twipsToPx(value), unit: 'px' }; } - if (lineRule === 'auto') { - return { value: (value * AUTO_SPACING_DEFAULT_MULTIPLIER) / AUTO_SPACING_LINE_DEFAULT, unit: 'multiplier' }; - } return { value: value / AUTO_SPACING_LINE_DEFAULT, unit: 'multiplier' }; }; diff --git a/packages/layout-engine/pm-adapter/src/converters/table.test.ts b/packages/layout-engine/pm-adapter/src/converters/table.test.ts index 19b1a5496d..a1dbe20e25 100644 --- a/packages/layout-engine/pm-adapter/src/converters/table.test.ts +++ b/packages/layout-engine/pm-adapter/src/converters/table.test.ts @@ -2362,3 +2362,99 @@ describe('tableCellNodeToBlock — SD-2516: documentPartObject children', () => expect((cellBlocks[0] as ParagraphBlock).runs[0].text).toBe('Inner DPO'); }); }); + +// SD-2735 P2: previously every table-cell paragraph in a styled table was +// flattened to spacing.line=1.0 by a TableGrid-only workaround that the +// converter applied to ANY tableStyleId. Custom table styles (e.g. a +// "1.5×-spacing" table style) lost their explicit multiplier. With the +// pm-adapter normalization producing spec-correct ratios end-to-end, the +// style-engine cascade already pulls the table style's pPr.spacing — the +// workaround is redundant and must not flatten cell paragraphs whose source +// has no inline spacing. +describe('parseTableCell — SD-2735 P2: preserves custom table-style line multiplier', () => { + const mockBlockIdGenerator: BlockIdGenerator = vi.fn((kind) => `test-${kind}`); + const mockPositionMap: PositionMap = new Map(); + + it('does not flatten a 1.5× line multiplier produced by a custom table-style cascade', () => { + // mockParagraphConverter simulates the cascade output: a cell paragraph + // whose source had no explicit `` but whose tableStyle pPr + // resolves to `line=360 lineRule=auto` (= 1.5× multiplier post-fix). + const mockParagraphConverter = vi.fn( + (params: { para: PMNode }) => + [ + { + kind: 'paragraph', + id: 'p1', + attrs: { + spacing: { line: 1.5, lineUnit: 'multiplier', lineRule: 'auto' }, + }, + runs: [ + { + text: (params.para.content?.[0] as { text?: string } | undefined)?.text || 'cell', + fontFamily: 'Arial', + fontSize: 12, + }, + ], + }, + ] as ParagraphBlock[], + ); + + const node: PMNode = { + type: 'table', + attrs: { + tableStyleId: 'CustomLineSpacing', + tableProperties: { tableStyleId: 'CustomLineSpacing', tblLook: { noHBand: true, noVBand: true } }, + }, + content: [ + { + type: 'tableRow', + content: [ + { + type: 'tableCell', + // Source paragraph has NO `` — explicit-spacing guard + // does not apply, so the workaround would flatten this to 1.0. + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Cell' }] }], + }, + ], + }, + ], + }; + + // Register the custom style so `resolveExistingTableEffectiveStyleId` + // resolves `tableStyleId` to a non-null value and the cascade gate fires. + const converterContext: ConverterContext = { + ...DEFAULT_CONVERTER_CONTEXT, + translatedLinkedStyles: { + ...DEFAULT_CONVERTER_CONTEXT.translatedLinkedStyles, + styles: { + CustomLineSpacing: { + type: 'table', + paragraphProperties: { spacing: { line: 360, lineRule: 'auto' } }, + tableProperties: {}, + }, + }, + }, + }; + + const result = tableNodeToBlock( + node, + mockBlockIdGenerator, + mockPositionMap, + 'Arial', + 16, + undefined, + undefined, + undefined, + undefined, + mockParagraphConverter, + converterContext, + ) as TableBlock; + + const cell = result.rows[0].cells[0]; + const cellBlocks = cell.blocks ?? (cell.paragraph ? [cell.paragraph] : []); + const para = cellBlocks[0] as ParagraphBlock; + expect(para.attrs?.spacing?.line).toBe(1.5); + expect(para.attrs?.spacing?.lineUnit).toBe('multiplier'); + expect(para.attrs?.spacing?.lineRule).toBe('auto'); + }); +}); diff --git a/packages/layout-engine/pm-adapter/src/converters/table.ts b/packages/layout-engine/pm-adapter/src/converters/table.ts index c4d63bb834..4ac5ec77ae 100644 --- a/packages/layout-engine/pm-adapter/src/converters/table.ts +++ b/packages/layout-engine/pm-adapter/src/converters/table.ts @@ -333,17 +333,6 @@ const parseTableCell = (args: ParseTableCellArgs): TableCell | null => { const paragraphToFlowBlocks = context.converters.paragraphToFlowBlocks; const tableNodeToBlock = context.converters?.tableNodeToBlock; - /** - * Appends converted paragraph blocks to the cell's blocks array. - * - * This helper: - * 1. Applies SDT metadata to paragraph blocks (for structured content inheritance) - * 2. Filters to only include supported block types (paragraph, image, drawing) - * 3. Appends the filtered blocks to the cell's blocks array - * - * @param paragraphBlocks - The converted flow blocks from a paragraph node - * @param sdtMetadata - Optional SDT metadata to apply (from parent structuredContentBlock) - */ const appendParagraphBlocks = ( paragraphBlocks: FlowBlock[], sdtMetadata?: ReturnType, diff --git a/packages/layout-engine/style-engine/src/ooxml/index.test.ts b/packages/layout-engine/style-engine/src/ooxml/index.test.ts index 7cd3505210..52c3a7cad2 100644 --- a/packages/layout-engine/style-engine/src/ooxml/index.test.ts +++ b/packages/layout-engine/style-engine/src/ooxml/index.test.ts @@ -389,6 +389,40 @@ describe('ooxml - resolveParagraphProperties', () => { expect(result.spacing).toEqual({ before: 120, after: 240 }); expect(result.keepNext).toBe(true); }); + + it('lets a table style override docDefaults line spacing for cell paragraphs without inline spacing (SD-2735)', () => { + // Pins the cascade path that pm-adapter's old `applyTableGridSpacingCascade` + // hack was working around: Word's Normal template sets docDefaults + // `` (≈ 1.15 multiplier) and + // TableGrid sets `` (= 1.0 + // multiplier). For a cell paragraph with no inline spacing, the cascade + // must yield TableGrid's 240, not docDefaults' 276 — otherwise table + // rows render ~5 % taller than Word. + const params = buildParams({ + translatedLinkedStyles: { + ...emptyStyles, + docDefaults: { paragraphProperties: { spacing: { line: 276, lineRule: 'auto' } } }, + styles: { + Normal: { default: true, paragraphProperties: {} }, + TableGrid: { + type: 'table', + paragraphProperties: { spacing: { line: 240, lineRule: 'auto' } }, + tableProperties: {}, + }, + }, + }, + }); + const tableInfo = { + tableProperties: { tableStyleId: 'TableGrid', tblLook: { noHBand: true, noVBand: true } }, + rowIndex: 0, + cellIndex: 0, + numRows: 1, + numCells: 1, + }; + const result = resolveParagraphProperties(params, {}, tableInfo); + expect(result.spacing?.line).toBe(240); + expect(result.spacing?.lineRule).toBe('auto'); + }); }); describe('ooxml - resolveCellStyles', () => {