Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: broken, infinitely looping minPresenceAhead calculation #2400

Merged
merged 7 commits into from
Jan 15, 2024

Conversation

carlobeltrame
Copy link
Contributor

@carlobeltrame carlobeltrame commented Sep 21, 2023

Fixes #2303, where sometimes react-pdf could be caught in an infinite loop during rendering!
Fixes #2238

I debugged the existing shouldBreak logic which was introduced for react-pdf v2, and found some major flaws in the logic. At first I tried to just fix the logic, but I noticed it could be done more efficiently and cleanly, so I went ahead and rewrote part of the logic.

Problem 1: When calculating the "presence" of the sibling nodes following the child in question, vertical margin was completely ignored so far. I agree to ignore the bottom margin of the bottom-most node, but all other margins must be taken into account.

minPresenceAhead-margins

In this picture, shouldBreak(child, futureElements, ...) returned false so far because the height alone of futureElements[0] was not enough to exceed the page height. The marginTop o futureElements[0] was completely ignored in the old logic. The new logic correctly returns true in this case, because it searches for the last box end in all futureElements.

Problem 2: The previous logic would ignore the minPresenceAhead value, if the heights of the nodes following the child didn't add up to at least minPresenceAhead.

minPresenceAhead-not-enough-future-elements

In this picture, the two futureElements have a height total of 80, or 90 if one includes the vertical margins except the last bottom margin, which is less than the 100 minPresenceAhead. The old logic would completely ignore the minPresenceAhead, when the futureElements aren't at least this high. The new logic correctly outputs true for shouldBreak(child, futureElements, ...) in this case.

Problem 3: The previous logic would in certain scenarios output true for shouldBreak(child, futureElements, ...) even when the child is already at the top of the page. Breaking the child over to the next page wouldn't improve the situation at all, because the breaking conditions would then be met again on the next page, and on the next, and so on, leading to an infinite loop. The new logic prevents this by detecting whether the child is already at the top of the page.

Problem 4: The previous logic would incorrectly take fixed futureElements into account. The new logic correctly ignores fixed elements, because they do not have any influence on the page flow.

Notes

  • I completely removed the getNodesHeight function and file, because it was only used in shouldBreak, had no tests, was implemented in an overly specific way for just this one use case, and was not necessary anymore after my rewrite.
  • I also removed the option to specify a presenceAhead function as a prop on any element. This was undocumented, I didn't find any internal uses and it did not seem very helpful to me in the current form (at least, the function would require element as an argument to be useful, like defaultPresenceAhead receives it).
  • I added tests for 100% branch coverage (I hope), as well as regression tests for the known REPL case from the original issue minPresenceAhead crashed application #2303 and for a minimal reproduction example which I created while debugging this code.

The old minPresenceAhead had some major issues, among others because
vertical margins were completely ignored in the presence calculations.
@changeset-bot
Copy link

changeset-bot bot commented Sep 21, 2023

🦋 Changeset detected

Latest commit: ff69234

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@react-pdf/layout Minor
@react-pdf/renderer Patch
@react-pdf/examples Patch

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

@carlobeltrame carlobeltrame changed the title Rework minPresenceAhead calculation Rework broken minPresenceAhead calculation Sep 21, 2023
@carlobeltrame carlobeltrame changed the title Rework broken minPresenceAhead calculation Rework broken minPresenceAhead calculation which could cause infinite loop Sep 22, 2023
@carlobeltrame carlobeltrame changed the title Rework broken minPresenceAhead calculation which could cause infinite loop Rework broken, infinitely looping minPresenceAhead calculation Sep 22, 2023
carlobeltrame added a commit to carlobeltrame/ecamp3 that referenced this pull request Sep 29, 2023
Until diegomura/react-pdf#2400 is merged,
react-pdf can sometimes fall into an infinite loop during layouting,
when a min-presence-ahead element has a following sibling with vertical
margins. This caused our Star Wars example camp to be unprintable.
The bug does not happen with vertical padding, so this workaround will
fix the problem for now.
@thedivac
Copy link

Great work! That fixed a bunch of issues I was having with my layout!

@carlobeltrame
Copy link
Contributor Author

Great work! That fixed a bunch of issues I was having with my layout!

Great to hear someone other than me was finally able to test this!
@diegomura would you care to review? These problems have reportedly affected a number of users now.

Copy link
Owner

@diegomura diegomura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

Amazing. Thanks so much!

@diegomura diegomura changed the title Rework broken, infinitely looping minPresenceAhead calculation fix: broken, infinitely looping minPresenceAhead calculation Jan 15, 2024
@diegomura diegomura merged commit 0538bd9 into diegomura:master Jan 15, 2024
1 check passed
@carlobeltrame carlobeltrame deleted the fix-min-presence-ahead branch January 15, 2024 08:32
mskec pushed a commit to mskec/react-pdf that referenced this pull request Feb 26, 2024
…ra#2400)

* 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.

* Add changeset

* Add regression tests

* Ignore fixed elements when calculating page breaks

* Do not break before a wrapping element

* Rename variable to make it easier to understand

* Refactor for better readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minPresenceAhead crashed application fixed in conjunction with minPresenceAhead does not work as expected
3 participants