Take padding into consideration when deciding to break on minPresenceAhead#2895
Take padding into consideration when deciding to break on minPresenceAhead#2895cosdon wants to merge 1 commit intodiegomura:masterfrom
Conversation
|
|
We've been experiencing the issue in question and I can confirm this PR solves the problem. We've added a workaround for now to use margin instead of padding but it's a little hacky. Would love to see this one get merged. |
|
We've also been experiencing the same issue when it comes to images that push other nodes outside the view in multiple wrapped pages. Accounting for padding in the breaking logic would fix this issue as we use padding to style our wrapped page breaks. This PR should get more attention |
|
We're also seeing infinite loops and page hanging when the content overflows a single page. I removed the padding values from our PDF and this fixed them. |
…o 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.
When a node is already at the top of the page, it does not help to break it to the next page. We knew that already, but the implementation of this knowledge has been flawed. Apparently, page paddings, page borders, fixed elements and possibly other things can cause the previous position-based detection method to not work correctly. This commit replaces the flawed implementation with a hopefully more solid one: When there are no non-fixed nodes before the current one on the page, we can assume that the current node will not be placed higher up on the page even if we were to break it to the next page. Fixes diegomura#2884 Fixes diegomura#2895 Fixes further parts of diegomura#2974
When a node is already at the top of the page, it does not help to break it to the next page. We knew that already, but the implementation of this knowledge has been flawed. Apparently, page paddings, page borders, fixed elements and possibly other things can cause the previous position-based detection method to not work correctly. This commit replaces the flawed implementation with a hopefully more solid one: When there are no non-fixed nodes before the current one on the page, we can assume that the current node will not be placed higher up on the page even if we were to break it to the next page. Fixes diegomura#2884 Fixes diegomura#2895 Fixes further parts of diegomura#2974
carlobeltrame
left a comment
carlobeltrame
left a comment
There was a problem hiding this comment.
Hey there, this is the author of the current version of the page breaking algorithm speaking. Thanks for all the debugging and the proposed fix! I have had a detailed look, and I found that the same problem can also be caused by other (albeit more rare) causes, e.g. page borders or fixed non-absolutely-positioned elements before the element in question. Therefore I have created a similar but more general fix, taking the idea of your change but generalizing it to a more robust form. I have pushed my fix onto an existing PR of mine which is connected to #2884 as well: 3789d74
Please have a look and test it with your reproduction documents if you can. I have already tested the two reproduction cases from the issue #2884 locally, my fix also seems to solve those.
5 participants
Fixes #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.