SD-1708: Footnotes overlap footer and fall off the page#2021
SD-1708: Footnotes overlap footer and fall off the page#2021VladaHarbour wants to merge 2 commits intomainfrom
Conversation
VladaHarbour
requested review from
afn,
caio-pizzol,
chittolinag,
harbournick and
luccas-harbour
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e093b892f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reserves = nextReserves; | ||
| if (reservesStable) break; |
There was a problem hiding this comment.
When the for loop exits because MAX_FOOTNOTE_LAYOUT_PASSES is reached (without hitting reservesStable), reserves is updated to nextReserves but layout still reflects the previous reserve vector. The subsequent plan/injection phase then runs against a layout that may not match reserves, and because reservesDiffer compares against reserves (not the reserve vector actually used to produce layout), this mismatch can slip through and place footnotes in the wrong reserved band. In long/oscillating footnote pagination cases this can reintroduce overlap/truncation near the footer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@VladaHarbour worth double checking and adding a comment if relevant or not
Visual diffs detectedPixel differences were found in visual tests. This is not blocking — reproduce locally with |
| let reserves = plan.reserves; | ||
|
|
||
| // If any reserves, relayout once, then re-assign and inject. | ||
| const MAX_FOOTNOTE_LAYOUT_PASSES = 4; |
There was a problem hiding this comment.
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.warn would help debugging.
| layout = layoutDocument(currentBlocks, currentMeasures, { | ||
| ...options, | ||
| footnoteReservedByPageIndex: reserves, | ||
| headerContentHeights, |
There was a problem hiding this comment.
same layoutDocument(...) call with same options appears 3x here. a small relayout(reserves) helper would clean this up.
| ({ measuresById } = await measureFootnoteBlocks(collectFootnoteIdsByColumn(idsByColumn))); | ||
| plan = computeFootnoteLayoutPlan(layout, idsByColumn, measuresById, reserves, pageColumns); | ||
| const nextReserves = plan.reserves; | ||
| const reservesStable = |
There was a problem hiding this comment.
no test hits the multi-pass path. one where footnotes shift pages between passes would catch regressions.
|
|
||
| // 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) |
There was a problem hiding this comment.
+ 0 on the next line is leftover from replacing + 4 -- just drop it. same for this comment: if padding is 0, drop the mention.
2 participants
Hi team! Here I had to remove default top/bottom padding for table cells because it caused inconsistency between Superdoc and Word layouts(Superdoc tables occupied more space on the page that similar ones in Word). Also it caused issues in footnotes reserves calculations.