From efabfe490117d85ceabf869acca24f41673516b5 Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Thu, 21 Sep 2023 09:40:17 +0200 Subject: [PATCH 1/7] Rework breaking logic and add tests to fix minPresenceAhead The old minPresenceAhead had some major issues, among others because vertical margins were completely ignored in the presence calculations. --- packages/layout/src/node/getNodesHeight.js | 22 --- packages/layout/src/node/shouldBreak.js | 44 ++--- .../layout/tests/node/shouldBreak.test.js | 160 ++++++++++++++++++ 3 files changed, 175 insertions(+), 51 deletions(-) delete mode 100644 packages/layout/src/node/getNodesHeight.js create mode 100644 packages/layout/tests/node/shouldBreak.test.js diff --git a/packages/layout/src/node/getNodesHeight.js b/packages/layout/src/node/getNodesHeight.js deleted file mode 100644 index 42c26880f..000000000 --- a/packages/layout/src/node/getNodesHeight.js +++ /dev/null @@ -1,22 +0,0 @@ -/** - * Get many nodes height - * - * @param {Array} nodes - * @return {number} nodes height - */ -const getNodesHeight = nodes => { - let max = 0; - let min = Infinity; - - if (!nodes || nodes.length === 0) return 0; - - for (let i = 0; i < nodes.length; i += 1) { - const node = nodes[i]; - min = Math.min(min, node.box.top); - max = Math.max(max, node.box.top + node.box.height); - } - - return max - min; -}; - -export default getNodesHeight; diff --git a/packages/layout/src/node/shouldBreak.js b/packages/layout/src/node/shouldBreak.js index f667deb72..ac9aec529 100644 --- a/packages/layout/src/node/shouldBreak.js +++ b/packages/layout/src/node/shouldBreak.js @@ -1,46 +1,32 @@ /* eslint-disable no-continue */ import getWrap from './getWrap'; -import getNodesHeight from './getNodesHeight'; const getBreak = node => node.props?.break || false; -const getMinPresenceAhead = node => node.props?.minPresenceAhead; - -const defaultPresenceAhead = element => height => - Math.min(element.box.height, height); - -const getPresenceAhead = (elements, height) => { - let result = 0; - - for (let i = 0; i < elements.length; i += 1) { - const element = elements[i]; - - if (!element.box) continue; - - const isElementInside = height > element.box.top; - const presenceAhead = - element.props.presenceAhead || defaultPresenceAhead(element); - - if (element && isElementInside) { - result += presenceAhead(height - element.box.top); - } - } - - return result; -}; +const getMinPresenceAhead = node => node.props?.minPresenceAhead || 0; const shouldBreak = (child, futureElements, height) => { - const minPresenceAhead = getMinPresenceAhead(child); - const presenceAhead = getPresenceAhead(futureElements, height); - const futureHeight = getNodesHeight(futureElements); const shouldSplit = height < child.box.top + child.box.height; const shouldWrap = getWrap(child); + const minPresenceAhead = getMinPresenceAhead(child); + const afterMinPresenceAhead = + child.box.top + + child.box.height + + child.box.marginBottom + + minPresenceAhead; + const endOfLastFutureElement = Math.max( + ...futureElements.map(node => node.box.top + node.box.height), + ); + const afterPresence = Math.min(afterMinPresenceAhead, endOfLastFutureElement); + // If the child is already at the top of the page, breaking won't improve presence + // (as long as react-pdf does not support breaking into differently sized containers) + const breakingImprovesPresence = child.box.top > child.box.marginTop; return ( getBreak(child) || (!shouldWrap && shouldSplit) || - (minPresenceAhead < futureHeight && presenceAhead < minPresenceAhead) + (afterPresence > height && breakingImprovesPresence) ); }; diff --git a/packages/layout/tests/node/shouldBreak.test.js b/packages/layout/tests/node/shouldBreak.test.js new file mode 100644 index 000000000..adbf77327 --- /dev/null +++ b/packages/layout/tests/node/shouldBreak.test.js @@ -0,0 +1,160 @@ +import * as P from '@react-pdf/primitives'; +import shouldBreak from '../../src/node/shouldBreak'; + +describe('node shouldBreak', () => { + test('should not break when the child has enough space on the page', () => { + const result = shouldBreak( + { + box: { top: 50, height: 400 }, + }, + [], + 1000, + ); + + expect(result).toEqual(false); + }); + + test('should break when the child has enough space on the page', () => { + const result = shouldBreak( + { + box: { top: 50, height: 400 }, + props: { break: true }, + }, + [], + 1000, + ); + + expect(result).toEqual(true); + }); + + test('should not break when the child can be wrapped', () => { + const result = shouldBreak( + { + box: { top: 50, height: 1400 }, + props: { wrap: true }, + }, + [], + 1000, + ); + + expect(result).toEqual(false); + }); + + test('should break when the child is an unwrappable node', () => { + const result = shouldBreak( + { + type: P.Image, + box: { top: 50, height: 1400 }, + props: { wrap: true }, + }, + [], + 1000, + ); + + expect(result).toEqual(true); + }); + + test('should break when the child has wrapping disabled', () => { + const result = shouldBreak( + { + box: { top: 50, height: 1400 }, + props: { wrap: false }, + }, + [], + 1000, + ); + + expect(result).toEqual(true); + }); + + test('should break when minPresenceAhead is large enough and there are overflowing siblings after the child', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 400 }, + }, + [{ box: { top: 900, height: 200, marginTop: 0, marginBottom: 0 } }], + 1000, + ); + + expect(result).toEqual(true); + }); + + test('should break when minPresenceAhead is large enough and there are overflowing siblings due to margins after the child', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 400 }, + }, + [{ box: { top: 1100, height: 0, marginTop: 200, marginBottom: 0 } }], + 1000, + ); + + expect(result).toEqual(true); + }); + + test('should not break when minPresenceAhead is not past the page end', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 100 }, + }, + [{ box: { top: 900, height: 200, marginTop: 0, marginBottom: 0 } }], + 1000, + ); + + expect(result).toEqual(false); + }); + + test('should not break when the siblings after the child do not overflow past the page end', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 400 }, + }, + [{ box: { top: 900, height: 100, marginTop: 0, marginBottom: 0 } }], + 1000, + ); + + expect(result).toEqual(false); + }); + + test('should not break when the siblings after the child do not overflow past the page end, with margins', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 400 }, + }, + [{ box: { top: 1000, height: 0, marginTop: 100, marginBottom: 0 } }], + 1000, + ); + + expect(result).toEqual(false); + }); + + test("should not break when only the last sibling's bottom margin overflows past the page end", () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 400 }, + }, + [{ box: { top: 900, height: 100, marginTop: 0, marginBottom: 100 } }], + 1000, + ); + + expect(result).toEqual(false); + }); + + test('should not break due to minPresenceAhead when breaking does not improve presence, to avoid infinite loops', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 500, marginBottom: 0 }, + props: { minPresenceAhead: 400 }, + }, + [{ box: { top: 900, height: 200, marginTop: 0, marginBottom: 0 } }], + 1000, + ); + + expect(result).toEqual(false); + }); +}); From 07e717af9cf834c0905097fbaaa4a96a6e44cb8d Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Thu, 21 Sep 2023 09:44:59 +0200 Subject: [PATCH 2/7] Add changeset --- .changeset/mighty-birds-eat.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/mighty-birds-eat.md diff --git a/.changeset/mighty-birds-eat.md b/.changeset/mighty-birds-eat.md new file mode 100644 index 000000000..98d06eb72 --- /dev/null +++ b/.changeset/mighty-birds-eat.md @@ -0,0 +1,5 @@ +--- +'@react-pdf/layout': minor +--- + +Rework minPresenceAhead detection and add tests From ce8c567ff5a795e878304820f1650dff41c9da86 Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Thu, 21 Sep 2023 12:57:22 +0200 Subject: [PATCH 3/7] Add regression tests --- .../layout/tests/node/shouldBreak.test.js | 214 ++++++++++++++++++ 1 file changed, 214 insertions(+) diff --git a/packages/layout/tests/node/shouldBreak.test.js b/packages/layout/tests/node/shouldBreak.test.js index adbf77327..e6579ebea 100644 --- a/packages/layout/tests/node/shouldBreak.test.js +++ b/packages/layout/tests/node/shouldBreak.test.js @@ -157,4 +157,218 @@ describe('node shouldBreak', () => { expect(result).toEqual(false); }); + + test('should work with trivial minimal reproduction example', () => { + const result = shouldBreak( + { + type: 'VIEW', + box: { + top: 30, + height: 0, + marginTop: 0, + marginBottom: 0, + }, + props: { minPresenceAhead: 100 }, + }, + [ + { + type: 'VIEW', + box: { + top: 30, + height: 70, + marginTop: 0, + marginBottom: 0, + }, + }, + { + type: 'VIEW', + box: { + top: 130, + height: 0, + marginTop: 30, + marginBottom: 0, + }, + }, + ], + 811.89, + ); + + expect(result).toEqual(false); + }); + + test('should work with minimal infinite loop reproduction example', () => { + const result = shouldBreak( + { + type: 'VIEW', + box: { + top: 30, + height: 0, + marginTop: 0, + marginBottom: 0, + }, + props: { minPresenceAhead: 100 }, + }, + [ + { + type: 'VIEW', + box: { + top: 30, + height: 71, + marginTop: 0, + marginBottom: 0, + }, + }, + { + type: 'VIEW', + box: { + top: 131, + height: 0, + marginTop: 30, + marginBottom: 0, + }, + }, + ], + 811.89, + ); + + expect(result).toEqual(false); + }); + + test('should work with reproduction from #2303', () => { + const result = shouldBreak( + { + type: 'TEXT', + box: { + paddingTop: 0, + paddingRight: 0, + paddingBottom: 0, + paddingLeft: 0, + marginTop: 12, + marginRight: 12, + marginBottom: 12, + marginLeft: 12, + borderTopWidth: 0, + borderRightWidth: 0, + borderBottomWidth: 0, + borderLeftWidth: 0, + top: 541.7999877929688, + right: 12, + bottom: 12, + left: 72, + width: 451.280029296875, + height: 250.8800048828125, + }, + style: { + marginTop: 12, + marginRight: 12, + marginBottom: 12, + marginLeft: 12, + fontSize: 14, + textAlign: 'justify', + fontFamily: 'Times-Roman', + }, + props: { + minPresenceAhead: 4, + }, + children: [ + { + type: 'TEXT_INSTANCE', + value: + 'En un lugar de la Mancha, de cuyo nombre no quiero acordarme, no ha mucho tiempo que vivía un hidalgo de los de lanza en astillero, adarga antigua, rocín flaco y galgo corredor. Una olla de algo más vaca que carnero, salpicón las más noches, duelos y quebrantos los sábados, lentejas los viernes, algún palomino de añadidura los domingos, consumían las tres partes de su hacienda. El resto della concluían sayo de velarte, calzas de velludo para las fiestas con sus pantuflos de lo mismo, los días de entre semana se honraba con su vellori de lo más fino. Tenía en su casa una ama que pasaba de los cuarenta, y una sobrina que no llegaba a los veinte, y un mozo de campo y plaza, que así ensillaba el rocín como tomaba la podadera. Frisaba la edad de nuestro hidalgo con los cincuenta años, era de complexión recia, seco de carnes, enjuto de rostro; gran madrugador y amigo de la caza. Quieren decir que tenía el sobrenombre de Quijada o Quesada (que en esto hay alguna diferencia en los autores que deste caso escriben), aunque por conjeturas verosímiles se deja entender que se llama Quijana; pero esto importa poco a nuestro cuento; basta que en la narración dél no se salga un punto de la verdad', + style: { + fontFamily: 'Times-Roman', + fontSize: 14, + textAlign: 'justify', + }, + }, + ], + }, + [ + { + type: 'TEXT', + box: { + paddingTop: 0, + paddingRight: 0, + paddingBottom: 0, + paddingLeft: 0, + marginTop: 550, + marginRight: 0, + marginBottom: 0, + marginLeft: 0, + borderTopWidth: 0, + borderRightWidth: 0, + borderBottomWidth: 0, + borderLeftWidth: 0, + top: 1354.679931640625, + right: 0, + bottom: 0, + left: 60, + width: 475.280029296875, + height: 19.799999237060547, + }, + style: { + marginTop: 550, + }, + props: {}, + children: [ + { + type: 'TEXT_INSTANCE', + value: 'Orphans example. Try changing prop value', + style: {}, + }, + ], + }, + { + type: 'TEXT', + box: { + paddingTop: 0, + paddingRight: 0, + paddingBottom: 0, + paddingLeft: 0, + marginTop: 12, + marginRight: 12, + marginBottom: 12, + marginLeft: 12, + borderTopWidth: 0, + borderRightWidth: 0, + borderBottomWidth: 0, + borderLeftWidth: 0, + top: 1386.47998046875, + right: 12, + bottom: 12, + left: 72, + width: 451.280029296875, + height: 250.8800048828125, + }, + style: { + marginTop: 12, + marginRight: 12, + marginBottom: 12, + marginLeft: 12, + fontSize: 14, + textAlign: 'justify', + fontFamily: 'Times-Roman', + }, + props: { + orphans: 4, + }, + children: [ + { + type: 'TEXT_INSTANCE', + value: + 'En un lugar de la Mancha, de cuyo nombre no quiero acordarme, no ha mucho tiempo que vivía un hidalgo de los de lanza en astillero, adarga antigua, rocín flaco y galgo corredor. Una olla de algo más vaca que carnero, salpicón las más noches, duelos y quebrantos los sábados, lentejas los viernes, algún palomino de añadidura los domingos, consumían las tres partes de su hacienda. El resto della concluían sayo de velarte, calzas de velludo para las fiestas con sus pantuflos de lo mismo, los días de entre semana se honraba con su vellori de lo más fino. Tenía en su casa una ama que pasaba de los cuarenta, y una sobrina que no llegaba a los veinte, y un mozo de campo y plaza, que así ensillaba el rocín como tomaba la podadera. Frisaba la edad de nuestro hidalgo con los cincuenta años, era de complexión recia, seco de carnes, enjuto de rostro; gran madrugador y amigo de la caza. Quieren decir que tenía el sobrenombre de Quijada o Quesada (que en esto hay alguna diferencia en los autores que deste caso escriben), aunque por conjeturas verosímiles se deja entender que se llama Quijana; pero esto importa poco a nuestro cuento; basta que en la narración dél no se salga un punto de la verdad', + style: { + fontFamily: 'Times-Roman', + fontSize: 14, + textAlign: 'justify', + }, + }, + ], + }, + ], + 781.89, + ); + + expect(result).toEqual(false); + }); }); From 4db080507c600a009e0d25bc673bda82e1b00ccc Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Fri, 22 Sep 2023 12:17:45 +0200 Subject: [PATCH 4/7] Ignore fixed elements when calculating page breaks --- packages/layout/src/node/shouldBreak.js | 7 ++++- .../layout/tests/node/shouldBreak.test.js | 31 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/layout/src/node/shouldBreak.js b/packages/layout/src/node/shouldBreak.js index ac9aec529..975d00ab1 100644 --- a/packages/layout/src/node/shouldBreak.js +++ b/packages/layout/src/node/shouldBreak.js @@ -7,6 +7,8 @@ const getBreak = node => node.props?.break || false; const getMinPresenceAhead = node => node.props?.minPresenceAhead || 0; const shouldBreak = (child, futureElements, height) => { + if (child.props?.fixed) return false; + const shouldSplit = height < child.box.top + child.box.height; const shouldWrap = getWrap(child); const minPresenceAhead = getMinPresenceAhead(child); @@ -15,8 +17,11 @@ const shouldBreak = (child, futureElements, height) => { child.box.height + child.box.marginBottom + minPresenceAhead; + const nonFixedFutureElements = futureElements.filter( + node => !node.props?.fixed, + ); const endOfLastFutureElement = Math.max( - ...futureElements.map(node => node.box.top + node.box.height), + ...nonFixedFutureElements.map(node => node.box.top + node.box.height), ); const afterPresence = Math.min(afterMinPresenceAhead, endOfLastFutureElement); // If the child is already at the top of the page, breaking won't improve presence diff --git a/packages/layout/tests/node/shouldBreak.test.js b/packages/layout/tests/node/shouldBreak.test.js index e6579ebea..d34de2da5 100644 --- a/packages/layout/tests/node/shouldBreak.test.js +++ b/packages/layout/tests/node/shouldBreak.test.js @@ -158,6 +158,37 @@ describe('node shouldBreak', () => { expect(result).toEqual(false); }); + test('should never break fixed child', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 400, fixed: true }, + }, + [{ box: { top: 900, height: 200, marginTop: 0, marginBottom: 0 } }], + 1000, + ); + + expect(result).toEqual(false); + }); + + test('should ignore fixed elements after child', () => { + const result = shouldBreak( + { + box: { top: 500, height: 400, marginTop: 0, marginBottom: 0 }, + props: { minPresenceAhead: 400, fixed: true }, + }, + [ + { + box: { top: 900, height: 200, marginTop: 0, marginBottom: 0 }, + props: { fixed: true }, + }, + ], + 1000, + ); + + expect(result).toEqual(false); + }); + test('should work with trivial minimal reproduction example', () => { const result = shouldBreak( { From f8d281f075a6993b74d80d38208738373b27c5a5 Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Fri, 22 Sep 2023 13:34:07 +0200 Subject: [PATCH 5/7] Do not break before a wrapping element --- packages/layout/src/node/shouldBreak.js | 2 +- .../layout/tests/node/shouldBreak.test.js | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/layout/src/node/shouldBreak.js b/packages/layout/src/node/shouldBreak.js index 975d00ab1..2a437a3f3 100644 --- a/packages/layout/src/node/shouldBreak.js +++ b/packages/layout/src/node/shouldBreak.js @@ -31,7 +31,7 @@ const shouldBreak = (child, futureElements, height) => { return ( getBreak(child) || (!shouldWrap && shouldSplit) || - (afterPresence > height && breakingImprovesPresence) + (!shouldSplit && afterPresence > height && breakingImprovesPresence) ); }; diff --git a/packages/layout/tests/node/shouldBreak.test.js b/packages/layout/tests/node/shouldBreak.test.js index d34de2da5..26cfbb7b0 100644 --- a/packages/layout/tests/node/shouldBreak.test.js +++ b/packages/layout/tests/node/shouldBreak.test.js @@ -402,4 +402,32 @@ describe('node shouldBreak', () => { expect(result).toEqual(false); }); + + test('should not break when the child can wrap', () => { + const result = shouldBreak( + { + type: 'TEXT', + box: { + top: 425.23779296875, + height: 419.439453125, + marginTop: 12, + marginBottom: 12, + }, + }, + [ + { + type: 'TEXT', + box: { + top: 868.67724609375, + height: 247.8505859375, + marginTop: 12, + marginBottom: 12, + }, + }, + ], + 776.89, + ); + + expect(result).toEqual(false); + }); }); From 4bddd931d197f345810c0d3aa31ea9e9e0e7dcf2 Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Fri, 22 Sep 2023 13:36:05 +0200 Subject: [PATCH 6/7] Rename variable to make it easier to understand --- packages/layout/src/node/shouldBreak.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/layout/src/node/shouldBreak.js b/packages/layout/src/node/shouldBreak.js index 2a437a3f3..5e01cc64a 100644 --- a/packages/layout/src/node/shouldBreak.js +++ b/packages/layout/src/node/shouldBreak.js @@ -10,7 +10,7 @@ const shouldBreak = (child, futureElements, height) => { if (child.props?.fixed) return false; const shouldSplit = height < child.box.top + child.box.height; - const shouldWrap = getWrap(child); + const canWrap = getWrap(child); const minPresenceAhead = getMinPresenceAhead(child); const afterMinPresenceAhead = child.box.top + @@ -30,7 +30,7 @@ const shouldBreak = (child, futureElements, height) => { return ( getBreak(child) || - (!shouldWrap && shouldSplit) || + (shouldSplit && !canWrap) || (!shouldSplit && afterPresence > height && breakingImprovesPresence) ); }; From ff692343cde9341506d8fd99957c2c2385bce81e Mon Sep 17 00:00:00 2001 From: Carlo Beltrame Date: Fri, 22 Sep 2023 13:46:51 +0200 Subject: [PATCH 7/7] Refactor for better readability --- packages/layout/src/node/shouldBreak.js | 38 ++++++++++++++++--------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/packages/layout/src/node/shouldBreak.js b/packages/layout/src/node/shouldBreak.js index 5e01cc64a..bc213314f 100644 --- a/packages/layout/src/node/shouldBreak.js +++ b/packages/layout/src/node/shouldBreak.js @@ -6,32 +6,42 @@ const getBreak = node => node.props?.break || false; const getMinPresenceAhead = node => node.props?.minPresenceAhead || 0; -const shouldBreak = (child, futureElements, height) => { - if (child.props?.fixed) return false; +const getFurthestEnd = elements => + Math.max(...elements.map(node => node.box.top + node.box.height)); - const shouldSplit = height < child.box.top + child.box.height; - const canWrap = getWrap(child); - const minPresenceAhead = getMinPresenceAhead(child); - const afterMinPresenceAhead = +const getEndOfMinPresenceAhead = child => { + return ( child.box.top + child.box.height + child.box.marginBottom + - minPresenceAhead; - const nonFixedFutureElements = futureElements.filter( - node => !node.props?.fixed, + getMinPresenceAhead(child) ); - const endOfLastFutureElement = Math.max( - ...nonFixedFutureElements.map(node => node.box.top + node.box.height), +}; + +const getEndOfPresence = (child, futureElements) => { + const afterMinPresenceAhead = getEndOfMinPresenceAhead(child); + const endOfFurthestFutureElement = getFurthestEnd( + futureElements.filter(node => !node.props?.fixed), ); - const afterPresence = Math.min(afterMinPresenceAhead, endOfLastFutureElement); - // If the child is already at the top of the page, breaking won't improve presence + return Math.min(afterMinPresenceAhead, endOfFurthestFutureElement); +}; + +const shouldBreak = (child, futureElements, height) => { + if (child.props?.fixed) return false; + + const shouldSplit = height < child.box.top + child.box.height; + const canWrap = getWrap(child); + + // Calculate the y coordinate where the desired presence of the child ends + const endOfPresence = getEndOfPresence(child, futureElements); + // If the child is already at the top of the page, breaking won't improve its presence // (as long as react-pdf does not support breaking into differently sized containers) const breakingImprovesPresence = child.box.top > child.box.marginTop; return ( getBreak(child) || (shouldSplit && !canWrap) || - (!shouldSplit && afterPresence > height && breakingImprovesPresence) + (!shouldSplit && endOfPresence > height && breakingImprovesPresence) ); };