Skip to content

Commit 86e9323

Browse files
MendyLandaMendyLanda
committed
Merge diegomura#2895: Take padding into consideration when deciding to break on minPresenceAhead
Fixes diegomura#2884 - Changes splitPage to pass the box paddingTop through to shouldBreak to be used when deciding if breaking a node will improve its presence. Without this shouldBreak can return true for a node that's already as high as it can go on the page, causing an infinite loop.
1 parent 7788f91 commit 86e9323

File tree

5 files changed

+124
-11
lines changed

5 files changed

+124
-11
lines changed

‎packages/layout/src/node/shouldBreak.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ const shouldBreak = (
3131
child: SafeNode,
3232
futureElements: SafeNode[],
3333
height: number,
34+
paddingTop: number,
3435
) => {
3536
if ('fixed' in child.props) return false;
3637

@@ -42,7 +43,8 @@ const shouldBreak = (
4243

4344
// If the child is already at the top of the page, breaking won't improve its presence
4445
// (as long as react-pdf does not support breaking into differently sized containers)
45-
const breakingImprovesPresence = child.box.top > child.box.marginTop;
46+
const breakingImprovesPresence =
47+
child.box.top > child.box.marginTop + paddingTop;
4648

4749
return (
4850
getBreak(child) ||

‎packages/layout/src/page/getContentArea.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@ const getContentArea = (page: SafePageNode) => {
55
const height = page.style?.height as number;
66
const { paddingTop, paddingBottom } = getPadding(page as any);
77

8-
return height - (paddingBottom as number) - (paddingTop as number);
8+
return {
9+
contentArea: height - (paddingBottom as number) - (paddingTop as number),
10+
paddingTop: paddingTop as number,
11+
paddingBottom: paddingBottom as number,
12+
};
913
};
1014

1115
export default getContentArea;

‎packages/layout/src/steps/resolvePagination.ts‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,12 @@ const warnUnavailableSpace = (node: SafeNode) => {
5555
);
5656
};
5757

58-
const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => {
58+
const splitNodes = (
59+
height: number,
60+
contentArea: number,
61+
paddingTop: number,
62+
nodes: SafeNode[],
63+
) => {
5964
const currentChildren: SafeNode[] = [];
6065
const nextChildren: SafeNode[] = [];
6166

@@ -67,7 +72,7 @@ const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => {
6772
const nodeTop = getTop(child);
6873
const nodeHeight = child.box.height;
6974
const isOutside = height <= nodeTop;
70-
const shouldBreak = shouldNodeBreak(child, futureNodes, height);
75+
const shouldBreak = shouldNodeBreak(child, futureNodes, height, paddingTop);
7176
const shouldSplit = height + SAFETY_THRESHOLD < nodeTop + nodeHeight;
7277
const canWrap = canNodeWrap(child);
7378
const fitsInsidePage = nodeHeight <= contentArea;
@@ -106,7 +111,12 @@ const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => {
106111
}
107112

108113
if (shouldSplit) {
109-
const [currentChild, nextChild] = split(child, height, contentArea);
114+
const [currentChild, nextChild] = split(
115+
child,
116+
height,
117+
contentArea,
118+
paddingTop,
119+
);
110120

111121
// All children are moved to the next page, it doesn't make sense to show the parent on the current page
112122
if (child.children.length > 0 && currentChild.children.length === 0) {
@@ -138,17 +148,28 @@ const splitNodes = (height: number, contentArea: number, nodes: SafeNode[]) => {
138148
return [currentChildren, nextChildren];
139149
};
140150

141-
const splitChildren = (height: number, contentArea: number, node: SafeNode) => {
151+
const splitChildren = (
152+
height: number,
153+
contentArea: number,
154+
paddingTop: number,
155+
node: SafeNode,
156+
) => {
142157
const children = node.children || [];
143158
const availableHeight = height - getTop(node);
144-
return splitNodes(availableHeight, contentArea, children);
159+
return splitNodes(availableHeight, contentArea, paddingTop, children);
145160
};
146161

147-
const splitView = (node: SafeNode, height: number, contentArea: number) => {
162+
const splitView = (
163+
node: SafeNode,
164+
height: number,
165+
contentArea: number,
166+
paddingTop: number,
167+
) => {
148168
const [currentNode, nextNode] = splitNode(node, height);
149169
const [currentChilds, nextChildren] = splitChildren(
150170
height,
151171
contentArea,
172+
paddingTop,
152173
node,
153174
);
154175

@@ -158,8 +179,15 @@ const splitView = (node: SafeNode, height: number, contentArea: number) => {
158179
];
159180
};
160181

161-
const split = (node: SafeNode, height: number, contentArea: number) =>
162-
isText(node) ? splitText(node, height) : splitView(node, height, contentArea);
182+
const split = (
183+
node: SafeNode,
184+
height: number,
185+
contentArea: number,
186+
paddingTop: number,
187+
) =>
188+
isText(node)
189+
? splitText(node, height)
190+
: splitView(node, height, contentArea, paddingTop);
163191

164192
const shouldResolveDynamicNodes = (node: SafeNode) => {
165193
const children = node.children || [];
@@ -217,13 +245,14 @@ const splitPage = (
217245
yoga: YogaInstance,
218246
): SafePageNode[] => {
219247
const wrapArea = getWrapArea(page);
220-
const contentArea = getContentArea(page);
248+
const { contentArea, paddingTop } = getContentArea(page);
221249
const dynamicPage = resolveDynamicPage({ pageNumber }, page, fontStore, yoga);
222250
const height = page.style.height;
223251

224252
const [currentChilds, nextChilds] = splitNodes(
225253
wrapArea,
226254
contentArea,
255+
paddingTop,
227256
dynamicPage.children,
228257
);
229258

‎packages/layout/tests/node/shouldBreak.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, expect, test } from 'vitest';
22

33
import * as P from '@rpdf/primitives';
44
import shouldBreak from '../../src/node/shouldBreak';
5+
import { SafeNode } from '../../src/types';
56

67
describe('node shouldBreak', () => {
78
test('should not break when the child has enough space on the page', () => {
@@ -15,6 +16,7 @@ describe('node shouldBreak', () => {
1516
},
1617
[],
1718
1000,
19+
0,
1820
);
1921

2022
expect(result).toEqual(false);
@@ -31,6 +33,7 @@ describe('node shouldBreak', () => {
3133
},
3234
[],
3335
1000,
36+
0,
3437
);
3538

3639
expect(result).toEqual(true);
@@ -54,6 +57,7 @@ describe('node shouldBreak', () => {
5457
},
5558
[],
5659
1000,
60+
0,
5761
);
5862

5963
expect(result).toEqual(false);
@@ -77,6 +81,7 @@ describe('node shouldBreak', () => {
7781
},
7882
[],
7983
1000,
84+
0,
8085
);
8186

8287
expect(result).toEqual(true);
@@ -100,6 +105,7 @@ describe('node shouldBreak', () => {
100105
},
101106
[],
102107
1000,
108+
0,
103109
);
104110

105111
expect(result).toEqual(true);
@@ -142,6 +148,7 @@ describe('node shouldBreak', () => {
142148
},
143149
],
144150
1000,
151+
0,
145152
);
146153

147154
expect(result).toEqual(true);
@@ -184,6 +191,7 @@ describe('node shouldBreak', () => {
184191
},
185192
],
186193
1000,
194+
0,
187195
);
188196

189197
expect(result).toEqual(true);
@@ -226,6 +234,7 @@ describe('node shouldBreak', () => {
226234
},
227235
],
228236
1000,
237+
0,
229238
);
230239

231240
expect(result).toEqual(false);
@@ -268,6 +277,7 @@ describe('node shouldBreak', () => {
268277
},
269278
],
270279
1000,
280+
0,
271281
);
272282

273283
expect(result).toEqual(false);
@@ -310,6 +320,7 @@ describe('node shouldBreak', () => {
310320
},
311321
],
312322
1000,
323+
0,
313324
);
314325

315326
expect(result).toEqual(false);
@@ -352,6 +363,7 @@ describe('node shouldBreak', () => {
352363
},
353364
],
354365
1000,
366+
0,
355367
);
356368

357369
expect(result).toEqual(false);
@@ -394,6 +406,26 @@ describe('node shouldBreak', () => {
394406
},
395407
],
396408
1000,
409+
0,
410+
);
411+
412+
expect(result).toEqual(false);
413+
});
414+
415+
test('should consider padding when breaking on minPresenceAhead', () => {
416+
const result = shouldBreak(
417+
{
418+
box: { top: 550, height: 400, marginTop: 500, marginBottom: 0 },
419+
props: { minPresenceAhead: 400 },
420+
} as SafeNode,
421+
[
422+
{
423+
box: { top: 900, height: 200, marginTop: 0, marginBottom: 0 },
424+
props: {},
425+
} as SafeNode,
426+
],
427+
1000,
428+
50,
397429
);
398430

399431
expect(result).toEqual(false);
@@ -436,6 +468,7 @@ describe('node shouldBreak', () => {
436468
},
437469
],
438470
1000,
471+
0,
439472
);
440473

441474
expect(result).toEqual(false);
@@ -478,6 +511,7 @@ describe('node shouldBreak', () => {
478511
},
479512
],
480513
1000,
514+
0,
481515
);
482516

483517
expect(result).toEqual(false);
@@ -536,6 +570,7 @@ describe('node shouldBreak', () => {
536570
},
537571
],
538572
811.89,
573+
0,
539574
);
540575

541576
expect(result).toEqual(false);
@@ -594,6 +629,7 @@ describe('node shouldBreak', () => {
594629
},
595630
],
596631
811.89,
632+
0,
597633
);
598634

599635
expect(result).toEqual(false);
@@ -721,6 +757,7 @@ describe('node shouldBreak', () => {
721757
},
722758
],
723759
781.89,
760+
0,
724761
);
725762

726763
expect(result).toEqual(false);
@@ -763,6 +800,7 @@ describe('node shouldBreak', () => {
763800
},
764801
],
765802
776.89,
803+
0,
766804
);
767805

768806
expect(result).toEqual(false);

‎packages/layout/tests/steps/resolvePagination.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,4 +276,44 @@ describe('pagination step', () => {
276276
// If calcLayout returns then we did not hit an infinite loop
277277
expect(true).toBe(true);
278278
});
279+
280+
test('should take padding into account when splitting pages', async () => {
281+
const yoga = await loadYoga();
282+
283+
const root = {
284+
type: 'DOCUMENT',
285+
yoga,
286+
children: [
287+
{
288+
type: 'PAGE',
289+
box: {},
290+
style: {
291+
paddingTop: 30,
292+
width: 612,
293+
height: 792,
294+
},
295+
props: { wrap: true },
296+
children: [
297+
{
298+
type: 'VIEW',
299+
box: {},
300+
style: { height: 761, marginBottom: 24 },
301+
props: { wrap: true, break: false },
302+
},
303+
{
304+
type: 'VIEW',
305+
box: {},
306+
style: { height: 80 },
307+
props: { wrap: true, break: false },
308+
},
309+
],
310+
},
311+
],
312+
};
313+
314+
calcLayout(root);
315+
316+
// If calcLayout returns then we did not hit an infinite loop
317+
expect(true).toBe(true);
318+
});
279319
});

0 commit comments

Comments
 (0)