From f546d430638564c1b7c476ab25a0ed4fcea40fc0 Mon Sep 17 00:00:00 2001 From: William Bell Date: Mon, 6 Jul 2026 20:13:39 -0600 Subject: [PATCH 1/2] fix(layout): don't squash a container's sole child at a page break (#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) --- .changeset/pagination-page-break-overlap.md | 5 + .../layout/src/steps/resolvePagination.ts | 62 ++++- .../tests/steps/resolvePagination.test.ts | 215 ++++++++++++++++++ 3 files changed, 273 insertions(+), 9 deletions(-) create mode 100644 .changeset/pagination-page-break-overlap.md diff --git a/.changeset/pagination-page-break-overlap.md b/.changeset/pagination-page-break-overlap.md new file mode 100644 index 000000000..e2f3b6b32 --- /dev/null +++ b/.changeset/pagination-page-break-overlap.md @@ -0,0 +1,5 @@ +--- +'@react-pdf/layout': patch +--- + +Fix content overlapping itself at a page break. A node that is the sole child of its container and lands near the bottom of a page (e.g. a `wrap={false}` block, or an element taller than the space left) was force-fit into the remaining space instead of moving to the next page, drawing its lines on top of each other. Pagination now bases that decision on whether the page is actually empty above the node, which also avoids an infinite page loop. Closes #3449. diff --git a/packages/layout/src/steps/resolvePagination.ts b/packages/layout/src/steps/resolvePagination.ts index f029d7619..38b78f253 100644 --- a/packages/layout/src/steps/resolvePagination.ts +++ b/packages/layout/src/steps/resolvePagination.ts @@ -55,7 +55,16 @@ const warnUnavailableSpace = (node: SafeNode) => { ); }; -const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => { +const splitNodes = ( + height: number, + contentArea: number, + nodes: SafeNode[], + // Whether the current page has no laid-out (non-fixed) content above these + // nodes yet. Threaded through the recursion so the "keep an un-splittable + // node on the current page" decision reflects the *page*, not just the local + // container. See https://github.com/diegomura/react-pdf/issues/3449 + pageEmpty = true, +) => { const currentChildren: SafeNode[] = []; const nextChildren: SafeNode[] = []; @@ -64,6 +73,11 @@ const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => { const futureNodes = nodes.slice(i + 1); const futureFixedNodes = futureNodes.filter(isFixed); + // The page is empty above this node only when it was empty entering this + // container and nothing non-fixed has been placed before it here. + const emptyAbove = + pageEmpty && !currentChildren.some((node) => !isFixed(node)); + const nodeTop = getTop(child); const nodeHeight = child.box.height; const isOutside = height <= nodeTop; @@ -111,12 +125,24 @@ const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => { } if (shouldSplit) { - const [currentChild, nextChild] = split(child, height, contentArea); + const [currentChild, nextChild] = split( + child, + height, + contentArea, + emptyAbove, + ); // All children are moved to the next page, it doesn't make sense to show the parent on the current page if (child.children.length > 0 && currentChild.children.length === 0) { - // But if the current page is empty then we can just include the parent on the current page - if (currentChildren.length === 0) { + // Keep the (now empty) parent on the current page only when the page is + // empty above it — otherwise there is nothing to gain and we would just + // squash an oversized subtree into the little space left here, drawing + // its lines on top of each other. When the page already has content, we + // move the whole node to the next page instead. Using page emptiness + // (not the local `currentChildren.length === 0`) is what prevents both + // the overlap and an infinite page loop. + // See https://github.com/diegomura/react-pdf/issues/3449 + if (emptyAbove) { currentChildren.push(child, ...futureFixedNodes); nextChildren.push(...futureNodes); } else { @@ -143,18 +169,29 @@ const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => { return [currentChildren, nextChildren]; }; -const splitChildren = (height: number, contentArea: number, node: SafeNode) => { +const splitChildren = ( + height: number, + contentArea: number, + node: SafeNode, + pageEmpty: boolean, +) => { const children = node.children || []; const availableHeight = height - getTop(node); - return splitNodes(availableHeight, contentArea, children); + return splitNodes(availableHeight, contentArea, children, pageEmpty); }; -const splitView = (node: SafeNode, height: number, contentArea: number) => { +const splitView = ( + node: SafeNode, + height: number, + contentArea: number, + pageEmpty: boolean, +) => { const [currentNode, nextNode] = splitNode(node, height); const [currentChilds, nextChildren] = splitChildren( height, contentArea, node, + pageEmpty, ); return [ @@ -163,8 +200,15 @@ const splitView = (node: SafeNode, height: number, contentArea: number) => { ]; }; -const split = (node: SafeNode, height: number, contentArea: number) => - isText(node) ? splitText(node, height) : splitView(node, height, contentArea); +const split = ( + node: SafeNode, + height: number, + contentArea: number, + pageEmpty: boolean, +) => + isText(node) + ? splitText(node, height) + : splitView(node, height, contentArea, pageEmpty); const shouldResolveDynamicNodes = (node: SafeNode) => { const children = node.children || []; diff --git a/packages/layout/tests/steps/resolvePagination.test.ts b/packages/layout/tests/steps/resolvePagination.test.ts index 2466aaf4b..058f0186a 100644 --- a/packages/layout/tests/steps/resolvePagination.test.ts +++ b/packages/layout/tests/steps/resolvePagination.test.ts @@ -4,6 +4,8 @@ import FontStore from '@react-pdf/font'; import { loadYoga } from '../../src/yoga'; import resolvePagination from '../../src/steps/resolvePagination'; import resolveDimensions from '../../src/steps/resolveDimensions'; +import resolveStyles from '../../src/steps/resolveStyles'; +import resolvePagePaddings from '../../src/steps/resolvePagePaddings'; import { SafeDocumentNode } from '../../src/types'; const fontStore = new FontStore(); @@ -459,3 +461,216 @@ describe('pagination step', () => { expect(subChapter3.props!.bookmark).toEqual(bookmarkSubChapter3); }); }); + +/** + * Regression: content overlapping itself at a page break (issue #3449). + * + * A `wrap={false}` subtree that is the ONLY child of its container and lands + * near the bottom of a page used to be force-fit into the little space left + * (because `splitNodes` treated "this container has no current children" as + * "the page is empty") instead of moving to the next page. Yoga then compressed + * every line on top of the next — the reported "overlapping at the page break". + * + * The fix only keeps such a node on the current page when it genuinely cannot + * fit on a page by itself; otherwise it moves to the next page. + */ +describe('pagination overflow regression (#3449)', () => { + const resolveFull = (node: SafeDocumentNode) => + resolvePagination( + resolveDimensions(resolvePagePaddings(resolveStyles(node)), fontStore), + fontStore, + ); + + const text = (value: string, style: any = {}) => ({ + type: 'TEXT', + style, + props: {}, + children: [{ type: 'TEXT_INSTANCE', value }], + }); + const view = (style: any, children: any[], props: any = {}) => ({ + type: 'VIEW', + style, + props, + children, + }); + + // A two-column "itinerary entry": short left column + taller right column, + // kept together with wrap={false}. + const entry = (time: string, title: string, blocks: string[][]) => + view({}, [ + view( + { + flexDirection: 'row', + borderBottomWidth: 1, + paddingTop: 8, + paddingBottom: 8, + }, + [ + view({ width: 130, marginLeft: 20 }, [ + text(time, { fontSize: 11, marginBottom: 5 }), + text('PENDING', { fontSize: 7 }), + ]), + view({ flex: 1, marginRight: 20 }, [ + text(title, { fontSize: 10 }), + text('2 Adults', { fontSize: 8, marginTop: 5 }), + ...blocks.map(([label, value]) => + view({ fontSize: 8, marginTop: 5 }, [ + text(label, { fontWeight: 700 }), + text(value), + ]), + ), + ]), + ], + { wrap: false }, + ), + ]); + + // A "day": a date header + first entry kept together, then the remaining + // entries in a sibling container (so a later entry is the sole child of it). + const day = (name: string, first: any, rest: any[]) => + view({}, [ + view({ minPresenceAhead: 100 }, [ + view({ marginTop: 10, marginBottom: 8 }, [ + text(name, { fontSize: 9, fontWeight: 700 }), + ]), + first, + ]), + view({}, rest), + ]); + + // Count text spans that vertically overlap another span in the same column. + const countOverlaps = (layout: any) => { + let overlaps = 0; + layout.children.forEach((page: any) => { + const spans: { top: number; bottom: number; left: number }[] = []; + const walk = (node: any, top: number, left: number) => { + const t = top + (node.box?.top || 0); + const l = left + (node.box?.left || 0); + if (node.type === 'TEXT' && (node.lines || []).length) { + const h = node.lines.reduce( + (acc: number, line: any) => acc + line.box.height, + 0, + ); + spans.push({ top: t, bottom: t + h, left: l }); + } + (node.children || []).forEach((c: any) => walk(c, t, l)); + }; + walk(page, 0, 0); + + const columns: Record = {}; + spans.forEach((s) => { + const key = Math.round(s.left / 10); + (columns[key] ||= []).push(s); + }); + Object.values(columns).forEach((col) => { + col.sort((a, b) => a.top - b.top); + for (let i = 1; i < col.length; i += 1) { + if (col[i].top < col[i - 1].bottom - 1) overlaps += 1; + } + }); + }); + return overlaps; + }; + + const buildDoc = (yoga: any, pageHeight: number): SafeDocumentNode => + ({ + type: 'DOCUMENT', + yoga, + props: {}, + children: [ + { + type: 'PAGE', + props: {}, + style: { + width: 595.28, + height: pageHeight, + paddingTop: 40, + paddingBottom: 90, + paddingHorizontal: 40, + flexDirection: 'column', + }, + children: [ + view({}, [text('Daily Itinerary', { fontSize: 22 })], { + fixed: true, + }), + view({ flex: 1 }, [ + day( + 'TUESDAY, APR 7', + entry('12:25 AM', 'Juvia', [ + ['Address', '1111 Lincoln Road, Miami Beach, Florida, 33139'], + ['Website', 'http://www.juviamiami.com/'], + ['Phone', '+1 305-763-8272'], + [ + 'Cancellation Policy', + 'A $50.00 cancellation fee will be charged for each person on the reservation, to the credit card on file if not cancelled by 3:00 PM.', + ], + ]), + [ + entry('01:05 AM', 'Miami Beach Golf Club', [ + ['Address', '2301 Alton Road, Miami Beach, Florida, 33140'], + ['Website', 'http://www.miamibeachgolfclub.com/'], + ['Phone', '+1 305-532-3350'], + ]), + ], + ), + ]), + view( + { + position: 'absolute', + bottom: 0, + left: 40, + right: 40, + height: 50, + }, + [text('FOOTER', { fontSize: 8 })], + { fixed: true }, + ), + ], + }, + ], + }) as any; + + // Sweep a range of page heights so the second entry lands at different + // offsets from the page bottom; none of them may overlap. + for (const pageHeight of [520, 560, 600, 640]) { + test(`no text overlaps at the page break (page height ${pageHeight})`, async () => { + const yoga = await loadYoga(); + const layout = resolveFull(buildDoc(yoga, pageHeight)); + expect(countOverlaps(layout)).toBe(0); + }); + } + + // Guard the fix against an infinite page loop: a node that fits the content + // area but never fits the space left under a tall fixed header must NOT be + // moved forever — when the page is empty above it, it stays (and overflows), + // which is what keeps pagination terminating. + test('does not loop when a sole child never fits under a tall fixed header', async () => { + const yoga = await loadYoga(); + const layout = resolveFull({ + type: 'DOCUMENT', + yoga, + props: {}, + children: [ + { + type: 'PAGE', + props: {}, + style: { + width: 200, + height: 200, + padding: 20, + flexDirection: 'column', + }, + children: [ + view({ height: 150 }, [], { fixed: true }), + view({ flex: 1 }, [ + view({}, [view({ height: 100 }, [], { wrap: false })]), + ]), + ], + }, + ], + } as any); + + // Terminates with a small number of pages instead of looping forever. + expect(layout.children.length).toBeLessThan(20); + }); +}); From 38a3154ff6394c890ea87b99b98264c33e695fbb Mon Sep 17 00:00:00 2001 From: William Bell Date: Mon, 6 Jul 2026 20:32:44 -0600 Subject: [PATCH 2/2] docs: add REPL.md with a step-by-step reproduction for #3449 Co-Authored-By: Claude Opus 4.8 (1M context) --- REPL.md | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 REPL.md diff --git a/REPL.md b/REPL.md new file mode 100644 index 000000000..8d6253d75 --- /dev/null +++ b/REPL.md @@ -0,0 +1,140 @@ +# Reproducing #3449 — content overlapping itself at a page break + +This document shows how to reproduce, by hand, the pagination bug fixed in this +branch: a block that lands near the bottom of a page is **squashed into the few +points of space left** — its lines drawn on top of each other — instead of +moving cleanly to the next page. + +- **Affected package:** `@react-pdf/layout` (the pagination step). It is what + `@react-pdf/renderer` uses to break a document into pages, so the bug shows up + through the renderer even though the renderer version number looks unrelated + (e.g. renderer `4.5.1` depends on layout `4.6.1`). +- **Still present on the latest published version** (layout `4.6.1`). + +## Quick reproduction (react-pdf REPL) + +1. Open . +2. Replace the editor contents with the snippet below. +3. Look at the **bottom of page 1**. + +```jsx +const Block = ({ label, value }) => ( + + {label} + {value} + +); + +const Entry = ({ time, title, a, w, p }) => ( + + + + {time} + PENDING + + + {title} + 2 Adults + + + + + + +); + +const Doc = () => ( + + + + Daily Itinerary + + + {/* spacer: pushes the 2nd entry to the bottom of page 1 */} + + + + + TUESDAY, APR 7 + + + + + + + + + + EXCLUSIVE RESORTS + + + +); + +ReactPDF.render(); +``` + +### What you should see (the bug) + +At the bottom of page 1, the **"01:05 AM / Miami Beach Golf Club"** entry is +crammed into the sliver of space left: + +- `Address` is drawn on top of `2301 Alton Road, …` +- `Website` is drawn on top of the URL +- `Phone` is drawn on top of the phone number + +i.e. every field of that entry overlaps the next. With the fix, the entry moves +to page 2 intact and page 1 ends cleanly after the "Juvia" entry. + +### Important: the trigger is position-sensitive + +The overlap only happens when the second entry lands with **just barely not +enough** room — enough to *start* on the page but not enough to fit. The exact +`height` that lands it there depends on font metrics, which differ slightly +between the in-browser REPL and a server render. The verified window is roughly +**`height: 490`–`510`**. If `500` renders cleanly for you, try `490` or `510`. + +## Why it happens (root cause) + +In `packages/layout/src/steps/resolvePagination.ts`, `splitNodes` decides what +to do when a node is split and **all** of its children move to the next page +(leaving an empty shell on the current page): + +```js +if (child.children.length > 0 && currentChild.children.length === 0) { + // "if the current page is empty, keep the parent here" + if (currentChildren.length === 0) { + currentChildren.push(child, ...futureFixedNodes); // kept on the current page + } else { + // move it to the next page + } +} +``` + +The intent of `currentChildren.length === 0` is *"the page is empty, so keeping +this node here (even if it overflows) is better than producing an empty page / +looping forever."* But `currentChildren` is the **local** list for the node's +own container — it is `0` simply because the node is the **first child of its +container**, which is not the same as the page being empty. + +In the snippet above the golf entry is the only child of its (sibling) +container, yet the page already holds the "Juvia" entry. The check misfires, +the whole entry is force-kept in the tiny remaining space, and Yoga compresses +its lines on top of each other. + +## The fix + +Base the keep-vs-move decision on whether the page is actually **empty above +the node**, threaded through the split recursion (`pageEmpty`): + +- Page already has content above → **move** the node to the next page. +- Page genuinely empty and the node cannot fit → keep it (and let it overflow), + which is what preserves termination and prevents an infinite page loop. + +See `packages/layout/src/steps/resolvePagination.ts` and the regression tests in +`packages/layout/tests/steps/resolvePagination.test.ts` (overlap sweep across +page heights + a tall-fixed-header loop guard).