fix(layout): don't squash a container's sole child at a page break (#3449)#3450
Open
billinspiratobell wants to merge 2 commits into
Open
fix(layout): don't squash a container's sole child at a page break (#3449)#3450billinspiratobell wants to merge 2 commits into
billinspiratobell wants to merge 2 commits into
Conversation
…iegomura#3449) When splitting a node whose children all move to the next page, splitNodes kept the (now empty) parent on the current page whenever `currentChildren` was empty. That is a per-container signal, not proof the page is empty, so an element that is the only child of its container but lands on an already- populated page (a `wrap={false}` block, an itinerary row, etc.) was force-fit into the little space left and its lines were drawn on top of each other ("overlapping at the page break"). Base the keep-vs-move decision on whether the page is empty *above* the node, threaded through the split recursion. When the page already has content the node now moves to the next page; it is only kept (and allowed to overflow) when the page is genuinely empty, which also preserves termination and avoids an infinite page loop. Adds regression tests covering the overlap across several page heights and the tall-fixed-header loop guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 38a3154 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
OK this is ready. See my https://react-pdf.org/repl paste in the code above. It also fixes the recursion. test case is there too. |
|
CAN WE GET THIS MERGED? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3449.
Problem
Content overlaps itself at a page break — a block that lands near the bottom of a page is force-fit into the few points of space left, drawing its lines on top of each other, instead of moving to the next page. It shows up with two-column rows /
wrap={false}blocks (headers, footers, itinerary-style entries) near a page boundary, and is present on the latest published@react-pdf/layout(4.6.1).A full step-by-step reproduction (paste-into-REPL) is in
REPL.md.Root cause
In
resolvePagination'ssplitNodes, when a node is split and all of its children move to the next page (leaving an empty shell), the parent is kept on the current page whenevercurrentChildren.length === 0:currentChildren.length === 0is meant to mean "the page is empty, so keep this here (even if it overflows) rather than emit an empty page / loop forever." ButcurrentChildrenis the local list for the node's own container — it is0simply because the node is the first child of its container, which is not the same as the page being empty. So a node that is the sole child of its container but lands on an already-populated page is force-fit into the space left, and Yoga compresses its lines on top of each other.Fix
Decide based on whether the page is actually empty above the node, threaded through the split recursion as
pageEmpty:The change is contained to
splitNodes/split/splitView/splitChildren.Tests
Added to
packages/layout/tests/steps/resolvePagination.test.ts:wrap={false}two-column entry that is the sole child of its container, swept across several page heights. Fails before this change (7/6/5/3 overlapping spans), passes after.All 416 layout tests pass. Also verified end-to-end by rendering a real document that reproduced the overlap: 8 overlapping spans → 0 after the fix (the entry moves cleanly to the next page).
🤖 Generated with Claude Code