Change the "sticky" attribute to take a direction#6020
Change the "sticky" attribute to take a direction#6020efeyakinci wants to merge 5 commits intotypst:mainfrom
Conversation
PgBiel
left a comment
There was a problem hiding this comment.
Thanks for the proposal. Regarding the feature itself, I can see there being value in being able to take control over "widow prevention", similar to how the existing sticky attribute allows control over "orphan prevention".
However, I have a few concerns; I'll list some below to start, but in general, one important concern is that a single test won't take care of this change with potential for regressions. I feel like we need to clearly study, document and test the behavior of each possible flow child when affected by sticky: "above", and perhaps add more information about edge cases - I mention some below.
| let has_sticky_successor = self | ||
| .composer | ||
| .work | ||
| .children | ||
| .iter() | ||
| .skip(1) | ||
| .find_map(|child| match child { | ||
| Child::Single(single) => Some( | ||
| single.sticky.as_ref().map(|s| s.is_sticky_above()).unwrap_or(false), | ||
| ), | ||
| Child::Multi(multi) => Some( | ||
| multi.sticky.as_ref().map(|s| s.is_sticky_above()).unwrap_or(false), | ||
| ), | ||
| _ => None, | ||
| }) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
I wonder if it could be more elegant to do this in the compositor, attaching additional sticky information to the previous frame (the latest frame's index is kept during collection) after we find each one. I'd want to try to avoid this potentially "unbounded" search on each frame, in principle.
In principle, it'd be doable to add a "followed by sticky" bool to e.g. Single and Multi children. However, a paragraph is split into multiple Line children for example, so we'd have to track those as well...
Which leads me to a separate question: how would this work with lines? Currently, it seems that all frames between two Single / Multi will become sticky with a sticky: "above", which would mean, essentially, all paragraphs? This seems inappropriate, not to mention that the same search would be repeated for each line, wasting precious CPU power.
So we need to decide whether this should pull the entire last paragraph, or just a few lines at the end, for example. This behavior should be specified and tested. And the repeated search should also be tackled if possible.
| stick_to_successor(); | ||
| } | ||
| _ => { | ||
| // Only clear the snapshot if this frame isn't empty |
There was a problem hiding this comment.
This is a bit self-evident, let's keep (or adapt) the original comment instead.
| // Only restore snapshot if there's no spill | ||
| // If a sticky breakable element can be spilled to the following page, | ||
| // it's fine to put some of it on this page, since the next page will still | ||
| // have the next element on the same page as *some* of the spilled content. | ||
| if self.composer.work.spill.is_none() { | ||
| self.restore(snapshot) | ||
| } |
There was a problem hiding this comment.
Can you add a test where this changes the behavior of existing sticky: "below" blocks so we can see what this is actually changing? Want to avoid regressions here.
| /// ) | ||
| /// #block(sticky: "above")[The above table shows that...] | ||
| /// ``` | ||
| #[default(false)] |
There was a problem hiding this comment.
Let's add some compatibility behavior to temporarily accept bools with a warning. (You can leave this for last, after all other reviews have been addressed.)
| }) | ||
| .unwrap_or(false); | ||
|
|
||
| let mut stick_to_successor = || { |
There was a problem hiding this comment.
I'd love some tests showing how a sequence of sticky: "above" which don't fit in a single page behave. There is one already for sticky: "below", not too sure if it was block-sticky-many, but might have been
|
@efeyakinci Do you still plan to work on this? In principle, it would not be exceedingly hard to implement this feature, but it would need some changes from the current implementation. As it is very central code, it would also need to be well-tested. If you'd prefer not to proceed, that's totally fine, too. Then, I'd close the PR, to "unlock the mutex" on the issue, so to speak. Regarding API design, I posted some thoughts in the issue: #5259 (comment) Implementation-wise, my gut feeling is that it would be easier to move the pre-processing of this to the collection phase ( |
|
For now, I'll close this due to inactivity. Thanks still! |
This change closes #5259
It is useful to be able to create blocks that stick to the element above them. The most notable use case is for lists and enums to prevent pagebreaks in documents like
Other use cases include code blocks that follow text, or footnotes such as table footnotes that refer to content above them
Currently, there is no good way to achieve this. This change modifies the sticky parameter to take one of
"below", "above", "both", or none.This change also includes a change to the behavior of sticky when interacting with breakable blocks that spill that becomes more visible with such use cases. For an example like the following:
The behavior is that the middle block will cause a pagebreak so that it can be with the following block in unbroken form. However, this is likely not the least-surprising behavior. For example, consider a table that covers 90% of the height of a page. If a user added such a table where 20% of the vertical space is occupied and made the table sticky to add footnotes after the table, the expected behavior is likely not that the first page is left 90% blank, and the second page contains the table and footnotes in their entirety.
This PR adds a check to cause a pagebreak on a sticky elements that ends the page. In this case, we break if and only if there is no spill. Examining the cases, we see
The second (rare) edge case does allow for a sticky element to be on a different page than its successor, although the behavior with the change is still (I believe) less surprising than the current behavior.
This is non-trivial to fix, as knowing when to break would require calculating the amount of spillage on the final page of spillage, knowing the size of that page, and calculating whether the items will fit. As such, this is not done in this PR.