From f5b5328b32c4bb2e4c790abe9f20dafc7d977921 Mon Sep 17 00:00:00 2001 From: VladaHarbour Date: Wed, 11 Feb 2026 17:58:17 +0200 Subject: [PATCH 1/5] fix: change footnotes layout calculation --- .../layout-bridge/src/incrementalLayout.ts | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts index 82433951d4..47bb0030af 100644 --- a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts +++ b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts @@ -1668,32 +1668,32 @@ export async function incrementalLayout( let plan = computeFootnoteLayoutPlan(layout, idsByColumn, measuresById, [], pageColumns); let reserves = plan.reserves; - // If any reserves, relayout once, then re-assign and inject. + const MAX_FOOTNOTE_LAYOUT_PASSES = 4; + + // Relayout with footnote reserves and iterate until reserves and page count stabilize, + // so each page gets the correct reserve (avoids "too much" on one page and "not enough" on another). if (reserves.some((h) => h > 0)) { - layout = layoutDocument(currentBlocks, currentMeasures, { - ...options, - footnoteReservedByPageIndex: reserves, - headerContentHeights, - footerContentHeights, - remeasureParagraph: (block: FlowBlock, maxWidth: number, firstLineIndent?: number) => - remeasureParagraph(block as ParagraphBlock, maxWidth, firstLineIndent), - }); + for (let pass = 0; pass < MAX_FOOTNOTE_LAYOUT_PASSES; pass += 1) { + layout = layoutDocument(currentBlocks, currentMeasures, { + ...options, + footnoteReservedByPageIndex: reserves, + headerContentHeights, + footerContentHeights, + remeasureParagraph: (block: FlowBlock, maxWidth: number, firstLineIndent?: number) => + remeasureParagraph(block as ParagraphBlock, maxWidth, firstLineIndent), + }); + ({ columns: pageColumns, idsByColumn } = resolveFootnoteAssignments(layout)); + ({ measuresById } = await measureFootnoteBlocks(collectFootnoteIdsByColumn(idsByColumn))); + plan = computeFootnoteLayoutPlan(layout, idsByColumn, measuresById, reserves, pageColumns); + const nextReserves = plan.reserves; + const reservesStable = + nextReserves.length === reserves.length && + nextReserves.every((h, i) => (reserves[i] ?? 0) === h) && + reserves.every((h, i) => (nextReserves[i] ?? 0) === h); + reserves = nextReserves; + if (reservesStable) break; + } - // Pass 2: recompute assignment and reserves for the updated pagination. - ({ columns: pageColumns, idsByColumn } = resolveFootnoteAssignments(layout)); - ({ measuresById } = await measureFootnoteBlocks(collectFootnoteIdsByColumn(idsByColumn))); - plan = computeFootnoteLayoutPlan(layout, idsByColumn, measuresById, reserves, pageColumns); - reserves = plan.reserves; - - // Apply final reserves (best-effort second relayout) then inject fragments. - layout = layoutDocument(currentBlocks, currentMeasures, { - ...options, - footnoteReservedByPageIndex: reserves, - headerContentHeights, - footerContentHeights, - remeasureParagraph: (block: FlowBlock, maxWidth: number, firstLineIndent?: number) => - remeasureParagraph(block as ParagraphBlock, maxWidth, firstLineIndent), - }); let { columns: finalPageColumns, idsByColumn: finalIdsByColumn } = resolveFootnoteAssignments(layout); let { blocks: finalBlocks, measuresById: finalMeasuresById } = await measureFootnoteBlocks( collectFootnoteIdsByColumn(finalIdsByColumn), From 7e093b892f75540dfc88dfefd9f6f3f9b0a06726 Mon Sep 17 00:00:00 2001 From: VladaHarbour Date: Thu, 12 Feb 2026 17:41:01 +0200 Subject: [PATCH 2/5] fix: remove default top/bottom table padding --- .../layout-engine/layout-bridge/src/index.ts | 6 ++-- .../layout-engine/src/layout-table.ts | 4 +-- .../measuring/dom/src/index.test.ts | 32 ++++++++----------- .../layout-engine/measuring/dom/src/index.ts | 7 ++-- .../dom/src/table/renderTableCell.test.ts | 6 ++-- .../painters/dom/src/table/renderTableCell.ts | 6 ++-- 6 files changed, 28 insertions(+), 33 deletions(-) diff --git a/packages/layout-engine/layout-bridge/src/index.ts b/packages/layout-engine/layout-bridge/src/index.ts index f9c681381f..8323118ba6 100644 --- a/packages/layout-engine/layout-bridge/src/index.ts +++ b/packages/layout-engine/layout-bridge/src/index.ts @@ -702,9 +702,9 @@ export const hitTestTableFragment = ( const blockEndY = blockStartY + blockHeight; // Calculate position within the cell (accounting for cell padding) - const padding = cell.attrs?.padding ?? { top: 2, left: 4, right: 4, bottom: 2 }; + const padding = cell.attrs?.padding ?? { top: 0, left: 4, right: 4, bottom: 0 }; const cellLocalX = localX - colX - (padding.left ?? 4); - const cellLocalY = localY - rowY - (padding.top ?? 2); + const cellLocalY = localY - rowY - (padding.top ?? 0); const paragraphBlock = cellBlock as ParagraphBlock; const paragraphMeasure = cellBlockMeasure as ParagraphMeasure; @@ -1339,7 +1339,7 @@ type TableRowBlock = TableBlock['rows'][number]; type TableCellBlock = TableRowBlock['cells'][number]; type TableCellMeasure = TableMeasure['rows'][number]['cells'][number]; -const DEFAULT_CELL_PADDING = { top: 2, bottom: 2, left: 4, right: 4 }; +const DEFAULT_CELL_PADDING = { top: 0, bottom: 0, left: 4, right: 4 }; const getCellPaddingFromRow = (cellIdx: number, row?: TableRowBlock) => { const padding = row?.cells?.[cellIdx]?.attrs?.padding ?? {}; diff --git a/packages/layout-engine/layout-engine/src/layout-table.ts b/packages/layout-engine/layout-engine/src/layout-table.ts index b02c5d9fbe..1c5fc27000 100644 --- a/packages/layout-engine/layout-engine/src/layout-table.ts +++ b/packages/layout-engine/layout-engine/src/layout-table.ts @@ -376,8 +376,8 @@ type CellPadding = { top: number; bottom: number; left: number; right: number }; function getCellPadding(cellIdx: number, blockRow?: TableRow): CellPadding { const padding = blockRow?.cells?.[cellIdx]?.attrs?.padding ?? {}; return { - top: padding.top ?? 2, - bottom: padding.bottom ?? 2, + top: padding.top ?? 0, + bottom: padding.bottom ?? 0, left: padding.left ?? 4, right: padding.right ?? 4, }; diff --git a/packages/layout-engine/measuring/dom/src/index.test.ts b/packages/layout-engine/measuring/dom/src/index.test.ts index dd35aa5b11..2be57f989b 100644 --- a/packages/layout-engine/measuring/dom/src/index.test.ts +++ b/packages/layout-engine/measuring/dom/src/index.test.ts @@ -3255,14 +3255,13 @@ describe('measureBlock', () => { expect(cellMeasure.blocks[1].kind).toBe('paragraph'); expect(cellMeasure.blocks[2].kind).toBe('paragraph'); - // Heights should accumulate (3 paragraphs + padding) + // Heights should accumulate (3 paragraphs) const para1Height = cellMeasure.blocks[0].totalHeight; const para2Height = cellMeasure.blocks[1].totalHeight; const para3Height = cellMeasure.blocks[2].totalHeight; const totalContentHeight = para1Height + para2Height + para3Height; - const padding = 4; // Default top (2) + bottom (2) - expect(cellMeasure.height).toBe(totalContentHeight + padding); + expect(cellMeasure.height).toBe(totalContentHeight); }); it('measures cell with empty blocks array', async () => { @@ -3290,10 +3289,7 @@ describe('measureBlock', () => { const cellMeasure = measure.rows[0].cells[0]; expect(cellMeasure.blocks).toHaveLength(0); - - // Height should be just padding - const padding = 4; // Default top (2) + bottom (2) - expect(cellMeasure.height).toBe(padding); + expect(cellMeasure.height).toBe(0); }); it('maintains backward compatibility with legacy paragraph field', async () => { @@ -4579,8 +4575,8 @@ describe('measureBlock', () => { const para0Height = block0Measure.kind === 'paragraph' ? block0Measure.totalHeight : 0; const para1Height = block1Measure.kind === 'paragraph' ? block1Measure.totalHeight : 0; - // Cell height includes: para0Height + 10 + para1Height + 20 + padding (default 2 top + 2 bottom) - const expectedCellHeight = para0Height + 10 + para1Height + 20 + 4; + // Cell height includes: para0Height + 10 + para1Height + 20 + padding (default 0 top + 0 bottom) + const expectedCellHeight = para0Height + 10 + para1Height + 20 + 0; expect(cellMeasure.height).toBe(expectedCellHeight); }); @@ -4637,8 +4633,8 @@ describe('measureBlock', () => { // Only positive spacing should be added // Zero and negative spacing should not be added - // Cell height = para0 + para1 + para2 + 15 (positive spacing) + 4 (padding) - const expectedCellHeight = para0Height + para1Height + para2Height + 15 + 4; + // Cell height = para0 + para1 + para2 + 15 (positive spacing) + const expectedCellHeight = para0Height + para1Height + para2Height + 15; expect(cellMeasure.height).toBe(expectedCellHeight); }); @@ -4677,8 +4673,8 @@ describe('measureBlock', () => { const paraHeight = block0.kind === 'paragraph' ? block0.totalHeight : 0; - // Cell height should just be paragraph height + padding (no spacing.after) - const expectedCellHeight = paraHeight + 4; + // Cell height should just be paragraph height (no spacing.after) + const expectedCellHeight = paraHeight; expect(cellMeasure.height).toBe(expectedCellHeight); }); @@ -4724,7 +4720,7 @@ describe('measureBlock', () => { const paraHeight = paraMeasure.kind === 'paragraph' ? paraMeasure.totalHeight : 0; // Anchored image is out-of-flow: it should not increase cell height. - const expectedCellHeight = paraHeight + 4; // default top+bottom padding + const expectedCellHeight = paraHeight; expect(cellMeasure.height).toBe(expectedCellHeight); }); @@ -4780,8 +4776,8 @@ describe('measureBlock', () => { const para2Height = block2.kind === 'paragraph' ? block2.totalHeight : 0; // Only the valid number should add spacing - // Cell height = para0 + 10 (valid spacing) + para1 + para2 + 4 (padding) - const expectedCellHeight = para0Height + 10 + para1Height + para2Height + 4; + // Cell height = para0 + 10 (valid spacing) + para1 + para2 + const expectedCellHeight = para0Height + 10 + para1Height + para2Height; expect(cellMeasure.height).toBe(expectedCellHeight); }); @@ -4845,8 +4841,8 @@ describe('measureBlock', () => { const imageHeight = block1.kind === 'image' ? block1.height : 0; const para1Height = block2.kind === 'paragraph' ? block2.totalHeight : 0; - // Cell height = para0 + 10 + image + para1 + 5 + 4 (padding) - const expectedCellHeight = para0Height + 10 + imageHeight + para1Height + 5 + 4; + // Cell height = para0 + 10 + image + para1 + 5 + const expectedCellHeight = para0Height + 10 + imageHeight + para1Height + 5; expect(cellMeasure.height).toBe(expectedCellHeight); }); }); diff --git a/packages/layout-engine/measuring/dom/src/index.ts b/packages/layout-engine/measuring/dom/src/index.ts index 20f39c3daf..3010fb0964 100644 --- a/packages/layout-engine/measuring/dom/src/index.ts +++ b/packages/layout-engine/measuring/dom/src/index.ts @@ -2454,7 +2454,6 @@ function resolveTableWidth(attrs: TableBlock['attrs'], maxWidth: number): number async function measureTableBlock(block: TableBlock, constraints: MeasureConstraints): Promise { const maxWidth = typeof constraints === 'number' ? constraints : constraints.maxWidth; - // Resolve percentage or explicit pixel table width const resolvedTableWidth = resolveTableWidth(block.attrs, maxWidth); @@ -2645,9 +2644,9 @@ async function measureTableBlock(block: TableBlock, constraints: MeasureConstrai } // Get cell padding for height calculation - const cellPadding = cell.attrs?.padding ?? { top: 2, left: 4, right: 4, bottom: 2 }; - const paddingTop = cellPadding.top ?? 2; - const paddingBottom = cellPadding.bottom ?? 2; + const cellPadding = cell.attrs?.padding ?? { top: 0, left: 4, right: 4, bottom: 0 }; + const paddingTop = cellPadding.top ?? 0; + const paddingBottom = cellPadding.bottom ?? 0; const paddingLeft = cellPadding.left ?? 4; const paddingRight = cellPadding.right ?? 4; diff --git a/packages/layout-engine/painters/dom/src/table/renderTableCell.test.ts b/packages/layout-engine/painters/dom/src/table/renderTableCell.test.ts index 644c0f0083..0fb32548f8 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableCell.test.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableCell.test.ts @@ -102,11 +102,11 @@ describe('renderTableCell', () => { cell: baseCell, }); - // Default padding is top: 2, left: 4, right: 4, bottom: 2 - expect(cellElement.style.paddingTop).toBe('2px'); + // Default padding is top: 0, left: 4, right: 4, bottom: 0 + expect(cellElement.style.paddingTop).toBe('0px'); expect(cellElement.style.paddingLeft).toBe('4px'); expect(cellElement.style.paddingRight).toBe('4px'); - expect(cellElement.style.paddingBottom).toBe('2px'); + expect(cellElement.style.paddingBottom).toBe('0px'); }); it('content fills cell with 100% width and height', () => { diff --git a/packages/layout-engine/painters/dom/src/table/renderTableCell.ts b/packages/layout-engine/painters/dom/src/table/renderTableCell.ts index c694c4d7cf..a71fe42610 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableCell.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableCell.ts @@ -599,11 +599,11 @@ export const renderTableCell = (deps: TableCellRenderDependencies): TableCellRen } = deps; const attrs = cell?.attrs; - const padding = attrs?.padding || { top: 2, left: 4, right: 4, bottom: 2 }; + const padding = attrs?.padding || { top: 0, left: 4, right: 4, bottom: 0 }; const paddingLeft = padding.left ?? 4; - const paddingTop = padding.top ?? 2; + const paddingTop = padding.top ?? 0; const paddingRight = padding.right ?? 4; - const paddingBottom = padding.bottom ?? 2; + const paddingBottom = padding.bottom ?? 0; const cellEl = doc.createElement('div'); cellEl.style.position = 'absolute'; From 905eed921581e7a0053304cc53616d8d6b728b7e Mon Sep 17 00:00:00 2001 From: VladaHarbour Date: Mon, 16 Feb 2026 21:47:21 +0200 Subject: [PATCH 3/5] fix: review comments --- .../layout-bridge/src/incrementalLayout.ts | 50 +++--- .../test/footnoteMultiPass.test.ts | 148 ++++++++++++++++++ .../measuring/dom/src/index.test.ts | 4 +- .../layout-engine/measuring/dom/src/index.ts | 2 +- 4 files changed, 181 insertions(+), 23 deletions(-) create mode 100644 packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts diff --git a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts index 47bb0030af..94279bb70a 100644 --- a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts +++ b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts @@ -290,6 +290,7 @@ const resolveFootnoteMeasurementWidth = (options: LayoutOptions, blocks?: FlowBl const MIN_FOOTNOTE_BODY_HEIGHT = 1; const DEFAULT_FOOTNOTE_SEPARATOR_SPACING_BEFORE = 12; +const MAX_FOOTNOTE_LAYOUT_PASSES = 4; const computeMaxFootnoteReserve = (layoutForPages: Layout, pageIndex: number, baseReserve = 0): number => { const page = layoutForPages.pages?.[pageIndex]; @@ -1662,26 +1663,28 @@ export async function incrementalLayout( return { columns, idsByColumn }; }; + const relayout = (footnoteReservedByPageIndex: number[]) => + layoutDocument(currentBlocks, currentMeasures, { + ...options, + footnoteReservedByPageIndex, + headerContentHeights, + footerContentHeights, + remeasureParagraph: (block: FlowBlock, maxWidth: number, firstLineIndent?: number) => + remeasureParagraph(block as ParagraphBlock, maxWidth, firstLineIndent), + }); + // Pass 1: assign + reserve from current layout. let { columns: pageColumns, idsByColumn } = resolveFootnoteAssignments(layout); let { measuresById } = await measureFootnoteBlocks(collectFootnoteIdsByColumn(idsByColumn)); let plan = computeFootnoteLayoutPlan(layout, idsByColumn, measuresById, [], pageColumns); let reserves = plan.reserves; - const MAX_FOOTNOTE_LAYOUT_PASSES = 4; - // Relayout with footnote reserves and iterate until reserves and page count stabilize, // so each page gets the correct reserve (avoids "too much" on one page and "not enough" on another). if (reserves.some((h) => h > 0)) { + let reservesStabilized = false; for (let pass = 0; pass < MAX_FOOTNOTE_LAYOUT_PASSES; pass += 1) { - layout = layoutDocument(currentBlocks, currentMeasures, { - ...options, - footnoteReservedByPageIndex: reserves, - headerContentHeights, - footerContentHeights, - remeasureParagraph: (block: FlowBlock, maxWidth: number, firstLineIndent?: number) => - remeasureParagraph(block as ParagraphBlock, maxWidth, firstLineIndent), - }); + layout = relayout(reserves); ({ columns: pageColumns, idsByColumn } = resolveFootnoteAssignments(layout)); ({ measuresById } = await measureFootnoteBlocks(collectFootnoteIdsByColumn(idsByColumn))); plan = computeFootnoteLayoutPlan(layout, idsByColumn, measuresById, reserves, pageColumns); @@ -1690,8 +1693,22 @@ export async function incrementalLayout( nextReserves.length === reserves.length && nextReserves.every((h, i) => (reserves[i] ?? 0) === h) && reserves.every((h, i) => (nextReserves[i] ?? 0) === h); - reserves = nextReserves; - if (reservesStable) break; + if (reservesStable) { + reserves = nextReserves; + reservesStabilized = true; + break; + } + // Only update reserves when we will do another layout pass; otherwise layout + // would be built with the previous reserves while reserves would be nextReserves, + // and the plan/injection phase could place footnotes in the wrong band. + if (pass < MAX_FOOTNOTE_LAYOUT_PASSES - 1) { + reserves = nextReserves; + } + } + if (!reservesStabilized) { + console.warn( + `[incrementalLayout] Footnote reserve loop hit max passes (${MAX_FOOTNOTE_LAYOUT_PASSES}) without stabilizing; layout may have suboptimal footnote placement.`, + ); } let { columns: finalPageColumns, idsByColumn: finalIdsByColumn } = resolveFootnoteAssignments(layout); @@ -1712,14 +1729,7 @@ export async function incrementalLayout( finalReserves.some((h, i) => (reserves[i] ?? 0) !== h) || reserves.some((h, i) => (finalReserves[i] ?? 0) !== h); if (reservesDiffer) { - layout = layoutDocument(currentBlocks, currentMeasures, { - ...options, - footnoteReservedByPageIndex: finalReserves, - headerContentHeights, - footerContentHeights, - remeasureParagraph: (block: FlowBlock, maxWidth: number, firstLineIndent?: number) => - remeasureParagraph(block as ParagraphBlock, maxWidth, firstLineIndent), - }); + layout = relayout(finalReserves); reservesAppliedToLayout = finalReserves; ({ columns: finalPageColumns, idsByColumn: finalIdsByColumn } = resolveFootnoteAssignments(layout)); ({ blocks: finalBlocks, measuresById: finalMeasuresById } = await measureFootnoteBlocks( diff --git a/packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts b/packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts new file mode 100644 index 0000000000..dd3da486f4 --- /dev/null +++ b/packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts @@ -0,0 +1,148 @@ +import { describe, it, expect, vi } from 'vitest'; +import type { FlowBlock, Measure } from '@superdoc/contracts'; +import * as layoutEngine from '@superdoc/layout-engine'; +import { incrementalLayout } from '../src/incrementalLayout'; + +/** + * Builds a paragraph with pmStart/pmEnd so footnote ref position can be resolved to a page. + */ +const makeParagraph = (id: string, text: string, pmStart: number): FlowBlock => ({ + kind: 'paragraph', + id, + runs: [{ text, fontFamily: 'Arial', fontSize: 12, pmStart, pmEnd: pmStart + text.length }], +}); + +const makeMeasure = (lineHeight: number, textLength: number): Measure => ({ + kind: 'paragraph', + lines: [ + { + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: textLength, + width: 200, + ascent: lineHeight * 0.8, + descent: lineHeight * 0.2, + lineHeight, + }, + ], + totalHeight: lineHeight, +}); + +/** Multi-line paragraph measure so footnote reserve height is large enough to shift content. */ +const makeMultiLineMeasure = (lineHeight: number, lineCount: number): Measure => { + const lines = Array.from({ length: lineCount }, (_, i) => ({ + fromRun: 0, + fromChar: i, + toRun: 0, + toChar: i + 1, + width: 200, + ascent: lineHeight * 0.8, + descent: lineHeight * 0.2, + lineHeight, + })); + return { + kind: 'paragraph', + lines, + totalHeight: lineCount * lineHeight, + }; +}; + +/** + * Scenario that forces the footnote reserve loop to run multiple passes: + * - Content height is small (240px). Body has 12 one-line blocks (20px each) so they + * exactly fill page 1 with no reserve. + * - Footnote ref is in the last body block (ref on page 1 in pass 1). + * - Footnote is tall (5 lines) so its reserve is ~80px. When we relayout with that + * reserve on page 1, content height becomes 160px → only 8 lines fit → the ref + * moves to page 2. So pass 2 assigns the footnote to page 2 and reserves there. + * This catches regressions where layout and reserves get out of sync (e.g. wrong + * reserve vector used for layout when max passes hit). + */ +describe('Footnote multi-pass reserve loop', () => { + it('runs multiple layout passes when footnotes shift pages and stabilizes correctly', async () => { + const BODY_LINE_HEIGHT = 20; + const FOOTNOTE_LINE_HEIGHT = 12; + const LINES_ON_PAGE_1_WITHOUT_RESERVE = 12; + const FOOTNOTE_LINES = 5; + + let pos = 0; + const bodyBlocks: FlowBlock[] = []; + for (let i = 0; i < LINES_ON_PAGE_1_WITHOUT_RESERVE; i += 1) { + const text = `Line ${i + 1}.`; + bodyBlocks.push(makeParagraph(`body-${i}`, text, pos)); + pos += text.length + 1; // +1 for implied break + } + // Ref in last body block (so on page 1 when no reserve, then moves to page 2 when we reserve) + const refPos = pos - 2; // inside last paragraph + const footnoteBlock = makeParagraph( + 'footnote-1-0-paragraph', + 'Footnote content that spans multiple lines here.', + 0, + ); + + const measureBlock = vi.fn(async (block: FlowBlock) => { + if (block.id.startsWith('footnote-')) { + return makeMultiLineMeasure(FOOTNOTE_LINE_HEIGHT, FOOTNOTE_LINES); + } + const textLength = block.kind === 'paragraph' ? (block.runs?.[0]?.text?.length ?? 1) : 1; + return makeMeasure(BODY_LINE_HEIGHT, textLength); + }); + + // Content height 240px: 12 * 20 = 240. With ~80px reserve → 160px → 8 lines on page 1. + const contentHeight = 240; + const margins = { top: 72, right: 72, bottom: 72, left: 72 }; + const pageHeight = contentHeight + margins.top + margins.bottom; + + const layoutDocSpy = vi.spyOn(layoutEngine, 'layoutDocument'); + + const result = await incrementalLayout( + [], + null, + bodyBlocks, + { + pageSize: { w: 612, h: pageHeight }, + margins, + footnotes: { + refs: [{ id: '1', pos: refPos }], + blocksById: new Map([['1', [footnoteBlock]]]), + topPadding: 4, + dividerHeight: 2, + }, + }, + measureBlock, + ); + + const footnoteReserveCalls = layoutDocSpy.mock.calls.filter((call) => + (call[2] as { footnoteReservedByPageIndex?: number[] })?.footnoteReservedByPageIndex?.some((h) => h > 0), + ); + layoutDocSpy.mockRestore(); + + expect(footnoteReserveCalls.length).toBeGreaterThanOrEqual(2); + + const { layout } = result; + expect(layout.pages.length).toBeGreaterThanOrEqual(2); + + const page2 = layout.pages[1]; + expect(page2.footnoteReserved).toBeGreaterThan(0); + + const footnoteFragment = layout.pages.flatMap((p) => p.fragments).find((f) => f.blockId === footnoteBlock.id); + expect(footnoteFragment).toBeTruthy(); + const pageOfFootnote = layout.pages.find((p) => p.fragments.some((f) => f.blockId === footnoteBlock.id)); + expect(pageOfFootnote).toBe(page2); + + // Sanity: footnote band does not overlap body (reserve is at bottom; body content ends above it) + const bodyFragmentsOnPage2 = page2.fragments.filter( + (f) => f.blockId !== footnoteBlock.id && !String(f.blockId).startsWith('footnote-separator'), + ); + const footnoteBandTop = + (page2.size?.h ?? pageHeight) - (page2.margins?.bottom ?? margins.bottom) - (page2.footnoteReserved ?? 0); + for (const f of bodyFragmentsOnPage2) { + const fragBottom = + 'y' in f && typeof f.y === 'number' && 'height' in f + ? f.y + (f.height as number) + : ((f as { y?: number }).y ?? 0); + expect(fragBottom).toBeLessThanOrEqual(footnoteBandTop + 1); + } + }); +}); diff --git a/packages/layout-engine/measuring/dom/src/index.test.ts b/packages/layout-engine/measuring/dom/src/index.test.ts index 2be57f989b..1e1ca736aa 100644 --- a/packages/layout-engine/measuring/dom/src/index.test.ts +++ b/packages/layout-engine/measuring/dom/src/index.test.ts @@ -4575,8 +4575,8 @@ describe('measureBlock', () => { const para0Height = block0Measure.kind === 'paragraph' ? block0Measure.totalHeight : 0; const para1Height = block1Measure.kind === 'paragraph' ? block1Measure.totalHeight : 0; - // Cell height includes: para0Height + 10 + para1Height + 20 + padding (default 0 top + 0 bottom) - const expectedCellHeight = para0Height + 10 + para1Height + 20 + 0; + // Cell height includes: para0Height + 10 + para1Height + 20 + const expectedCellHeight = para0Height + 10 + para1Height + 20; expect(cellMeasure.height).toBe(expectedCellHeight); }); diff --git a/packages/layout-engine/measuring/dom/src/index.ts b/packages/layout-engine/measuring/dom/src/index.ts index 3010fb0964..cd56bda2bc 100644 --- a/packages/layout-engine/measuring/dom/src/index.ts +++ b/packages/layout-engine/measuring/dom/src/index.ts @@ -2674,7 +2674,7 @@ async function measureTableBlock(block: TableBlock, constraints: MeasureConstrai * ``` * cell.blocks = [paragraph1, paragraph2, paragraph3] * contentHeight = para1.height + para2.height + para3.height - * totalCellHeight = contentHeight + 2 (top) + 2 (bottom) + * totalCellHeight = contentHeight; * ``` */ const blockMeasures: Measure[] = []; From 8e962fb549c092b5036ca432491befcd53bf4240 Mon Sep 17 00:00:00 2001 From: Nick Bernal Date: Tue, 17 Feb 2026 17:16:36 -0800 Subject: [PATCH 4/5] fix(layout-bridge): detect footnote reserve oscillation early to avoid wasted layout passes --- .../layout-bridge/src/incrementalLayout.ts | 10 ++- .../test/footnoteMultiPass.test.ts | 62 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts index 94279bb70a..b6881d78f4 100644 --- a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts +++ b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts @@ -1683,6 +1683,7 @@ export async function incrementalLayout( // so each page gets the correct reserve (avoids "too much" on one page and "not enough" on another). if (reserves.some((h) => h > 0)) { let reservesStabilized = false; + const seenReserveKeys = new Set([reserves.join(',')]); for (let pass = 0; pass < MAX_FOOTNOTE_LAYOUT_PASSES; pass += 1) { layout = relayout(reserves); ({ columns: pageColumns, idsByColumn } = resolveFootnoteAssignments(layout)); @@ -1698,6 +1699,13 @@ export async function incrementalLayout( reservesStabilized = true; break; } + // Detect oscillation: if we've produced a reserve vector we already tried, + // the loop will never converge. Break early to avoid wasted relayout passes. + const nextKey = nextReserves.join(','); + if (seenReserveKeys.has(nextKey)) { + break; + } + seenReserveKeys.add(nextKey); // Only update reserves when we will do another layout pass; otherwise layout // would be built with the previous reserves while reserves would be nextReserves, // and the plan/injection phase could place footnotes in the wrong band. @@ -1707,7 +1715,7 @@ export async function incrementalLayout( } if (!reservesStabilized) { console.warn( - `[incrementalLayout] Footnote reserve loop hit max passes (${MAX_FOOTNOTE_LAYOUT_PASSES}) without stabilizing; layout may have suboptimal footnote placement.`, + `[incrementalLayout] Footnote reserve loop did not converge (max ${MAX_FOOTNOTE_LAYOUT_PASSES} passes); layout may have suboptimal footnote placement.`, ); } diff --git a/packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts b/packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts index dd3da486f4..aad9def5e3 100644 --- a/packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts +++ b/packages/layout-engine/layout-bridge/test/footnoteMultiPass.test.ts @@ -145,4 +145,66 @@ describe('Footnote multi-pass reserve loop', () => { expect(fragBottom).toBeLessThanOrEqual(footnoteBandTop + 1); } }); + + it('does not exhaust max reserve passes when reserves oscillate between pages', async () => { + const BODY_LINE_HEIGHT = 20; + const FOOTNOTE_LINE_HEIGHT = 12; + const LINES_ON_PAGE_1_WITHOUT_RESERVE = 12; + const FOOTNOTE_LINES = 5; + + let pos = 0; + const bodyBlocks: FlowBlock[] = []; + for (let i = 0; i < LINES_ON_PAGE_1_WITHOUT_RESERVE; i += 1) { + const text = `Line ${i + 1}.`; + bodyBlocks.push(makeParagraph(`body-${i}`, text, pos)); + pos += text.length + 1; + } + + const refPos = pos - 2; + const footnoteBlock = makeParagraph( + 'footnote-1-0-paragraph', + 'Footnote content that spans multiple lines here.', + 0, + ); + + const measureBlock = vi.fn(async (block: FlowBlock) => { + if (block.id.startsWith('footnote-')) { + return makeMultiLineMeasure(FOOTNOTE_LINE_HEIGHT, FOOTNOTE_LINES); + } + const textLength = block.kind === 'paragraph' ? (block.runs?.[0]?.text?.length ?? 1) : 1; + return makeMeasure(BODY_LINE_HEIGHT, textLength); + }); + + const contentHeight = 240; + const margins = { top: 72, right: 72, bottom: 72, left: 72 }; + const pageHeight = contentHeight + margins.top + margins.bottom; + + const layoutDocSpy = vi.spyOn(layoutEngine, 'layoutDocument'); + + await incrementalLayout( + [], + null, + bodyBlocks, + { + pageSize: { w: 612, h: pageHeight }, + margins, + footnotes: { + refs: [{ id: '1', pos: refPos }], + blocksById: new Map([['1', [footnoteBlock]]]), + topPadding: 4, + dividerHeight: 2, + }, + }, + measureBlock, + ); + + const footnoteReserveCalls = layoutDocSpy.mock.calls.filter((call) => + (call[2] as { footnoteReservedByPageIndex?: number[] })?.footnoteReservedByPageIndex?.some((h) => h > 0), + ); + layoutDocSpy.mockRestore(); + + // Current regression: this scenario oscillates A -> B -> A and runs all passes (+ final relayout). + // Desired behavior: detect oscillation and stop early. + expect(footnoteReserveCalls.length).toBeLessThanOrEqual(3); + }); }); From c0b3e20fcb096141f2104bba7fe7ee7659e5c18f Mon Sep 17 00:00:00 2001 From: Nick Bernal Date: Tue, 17 Feb 2026 17:30:39 -0800 Subject: [PATCH 5/5] chore: fix test --- .../layout-engine/src/layout-table.test.ts | 85 ++++++++++++------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/packages/layout-engine/layout-engine/src/layout-table.test.ts b/packages/layout-engine/layout-engine/src/layout-table.test.ts index 9a286b75c2..460fa49305 100644 --- a/packages/layout-engine/layout-engine/src/layout-table.test.ts +++ b/packages/layout-engine/layout-engine/src/layout-table.test.ts @@ -1815,22 +1815,46 @@ describe('layoutTableBlock', () => { expect(fragmentsWithPartialRow.length).toBeGreaterThan(0); }); - it('should maintain minimum line advancement across all cells', () => { - // Test that the minimum line advancement algorithm correctly identifies - // and applies the minimum advancement across all cells + it('should maintain monotonic per-cell advancement across continuation fragments', () => { + // Verify continuation fragments keep per-cell line progress monotonic + // when cells have different line heights. const block = createMockTableBlock(1); - // Create cells with different line heights where the minimum advancement - // will be determined by the cell with the tallest lines - const measure = createMockTableMeasure( - [100, 100, 100], - [120], - [ - [10, 10, 10, 10, 10], // Cell 0: 5 lines of 10px (total 50px) - [20, 20, 20, 20, 20], // Cell 1: 5 lines of 20px (total 100px) - [40, 40, 40], // Cell 2: 3 lines of 40px (total 120px) - ], - ); + // createMockTableMeasure applies line data per row (not per cell), so seed with + // row defaults then override each cell explicitly. + const measure = createMockTableMeasure([100, 100, 100], [120], [[10, 10, 10, 10, 10]]); + if (measure.rows[0].cells[1]) { + measure.rows[0].cells[1].paragraph = { + kind: 'paragraph', + lines: [20, 20, 20, 20, 20].map((lineHeight) => ({ + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: 1, + width: 100, + ascent: lineHeight * 0.75, + descent: lineHeight * 0.25, + lineHeight, + })), + totalHeight: 100, + }; + } + if (measure.rows[0].cells[2]) { + measure.rows[0].cells[2].paragraph = { + kind: 'paragraph', + lines: [40, 40, 40].map((lineHeight) => ({ + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: 1, + width: 100, + ascent: lineHeight * 0.75, + descent: lineHeight * 0.25, + lineHeight, + })), + totalHeight: 120, + }; + } const fragments: TableFragment[] = []; let cursorY = 0; @@ -1861,24 +1885,21 @@ describe('layoutTableBlock', () => { // Should create multiple fragments expect(fragments.length).toBeGreaterThan(1); - // Verify line advancement consistency - for (const fragment of fragments) { - if ('partialRow' in fragment && fragment.partialRow && !fragment.partialRow.isLastPart) { - const { toLineByCell, fromLineByCell } = fragment.partialRow; - const advancements = toLineByCell.map((to, idx) => to - fromLineByCell[idx]); - - // All cells that have remaining lines should advance by the same amount - // (this is the core of the line advancement algorithm) - const positiveAdvancements = advancements.filter((a) => a > 0); - if (positiveAdvancements.length > 0) { - const minAdvancement = Math.min(...positiveAdvancements); - // In the second pass, all cells should be normalized to minAdvancement - // (unless they've already completed) - positiveAdvancements.forEach((adv) => { - expect(adv).toBe(minAdvancement); - }); - } - } + const partialFragments = fragments.filter((f) => 'partialRow' in f && Boolean(f.partialRow)); + expect(partialFragments.length).toBeGreaterThan(1); + + // First fragment should show uneven (independent) advancement. + const firstPartial = partialFragments[0].partialRow!; + const firstAdvancements = firstPartial.toLineByCell.map((to, idx) => to - firstPartial.fromLineByCell[idx]); + expect(new Set(firstAdvancements.filter((a) => a > 0)).size).toBeGreaterThan(1); + + // Continuations must not regress per-cell line indices. + for (let i = 1; i < partialFragments.length; i += 1) { + const prev = partialFragments[i - 1].partialRow!; + const current = partialFragments[i].partialRow!; + current.fromLineByCell.forEach((fromLine, idx) => { + expect(fromLine).toBe(prev.toLineByCell[idx]); + }); } });