-
Notifications
You must be signed in to change notification settings - Fork 69
SD-1708: Footnotes overlap footer and fall off the page #2021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -739,7 +739,7 @@ | |
| // Dirty region computation | ||
| const dirtyStart = performance.now(); | ||
| const dirty = computeDirtyRegions(previousBlocks, nextBlocks); | ||
| const dirtyTime = performance.now() - dirtyStart; | ||
|
|
||
| if (dirty.deletedBlockIds.length > 0) { | ||
| measureCache.invalidate(dirty.deletedBlockIds); | ||
|
|
@@ -1668,32 +1668,32 @@ | |
| 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
| 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 = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no test hits the multi-pass path. one where footnotes shift pages between passes would catch regressions. |
||
| 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; | ||
|
Comment on lines
+1693
to
+1694
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @VladaHarbour worth double checking and adding a comment if relevant or not |
||
| } | ||
|
|
||
| // 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), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| 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); | ||
| }); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to module level -- other layout limits in the pipeline are defined at the top of the file. also, if this loop maxes out without stabilizing, a
console.warnwould help debugging.