From c2cc09e71ab97f752cd07acad7c38443c58cf64f Mon Sep 17 00:00:00 2001 From: PgBiel <9021226+PgBiel@users.noreply.github.com> Date: Mon, 9 Dec 2024 06:55:58 -0300 Subject: [PATCH] Forbid footnote migration in pending floats (#5497) Co-authored-by: Laurenz --- crates/typst-layout/src/flow/compose.rs | 32 +++++++++++++++--- crates/typst-layout/src/flow/distribute.rs | 16 ++++++--- crates/typst-layout/src/pages/collect.rs | 2 +- crates/typst-library/src/model/footnote.rs | 2 +- ...ssue-5435-footnote-migration-in-floats.png | Bin 0 -> 448 bytes tests/suite/layout/flow/footnote.typ | 20 +++++++++++ 6 files changed, 61 insertions(+), 11 deletions(-) create mode 100644 tests/ref/issue-5435-footnote-migration-in-floats.png diff --git a/crates/typst-layout/src/flow/compose.rs b/crates/typst-layout/src/flow/compose.rs index 9650a7bc1..326456752 100644 --- a/crates/typst-layout/src/flow/compose.rs +++ b/crates/typst-layout/src/flow/compose.rs @@ -214,6 +214,13 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { } /// Lay out the inner contents of a column. + /// + /// Pending floats and footnotes are also laid out at this step. For those, + /// however, we forbid footnote migration (moving the frame containing the + /// footnote reference if the corresponding entry doesn't fit), allowing + /// the footnote invariant to be broken, as it would require handling a + /// [`Stop::Finish`] at this point, but that is exclusively handled by the + /// distributor. fn column_contents(&mut self, regions: Regions) -> FlowResult { // Process pending footnotes. for note in std::mem::take(&mut self.work.footnotes) { @@ -222,7 +229,7 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { // Process pending floats. for placed in std::mem::take(&mut self.work.floats) { - self.float(placed, ®ions, false)?; + self.float(placed, ®ions, false, false)?; } distribute(self, regions) @@ -236,13 +243,21 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { /// (depending on `placed.scope`). /// /// When the float does not fit, it is queued into `work.floats`. The - /// value of `clearance` that between the float and flow content is needed - /// --- it is set if there are already distributed items. + /// value of `clearance` indicates that between the float and flow content + /// is needed --- it is set if there are already distributed items. + /// + /// The value of `migratable` determines whether footnotes within the float + /// should be allowed to prompt its migration if they don't fit in order to + /// respect the footnote invariant (entries in the same page as the + /// references), triggering [`Stop::Finish`]. This is usually `true` within + /// the distributor, as it can handle that particular flow event, and + /// `false` elsewhere. pub fn float( &mut self, placed: &'b PlacedChild<'a>, regions: &Regions, clearance: bool, + migratable: bool, ) -> FlowResult<()> { // If the float is already processed, skip it. let loc = placed.location(); @@ -291,7 +306,7 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { } // Handle footnotes in the float. - self.footnotes(regions, &frame, need, false)?; + self.footnotes(regions, &frame, need, false, migratable)?; // Determine the float's vertical alignment. We can unwrap the inner // `Option` because `Custom(None)` is checked for during collection. @@ -326,12 +341,19 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { /// Lays out footnotes in the `frame` if this is the root flow and there are /// any. The value of `breakable` indicates whether the element that /// produced the frame is breakable. If not, the frame is treated as atomic. + /// + /// The value of `migratable` indicates whether footnote migration should be + /// possible (at least for the first footnote found in the frame, as it is + /// forbidden for the second footnote onwards). It is usually `true` within + /// the distributor and `false` elsewhere, as the distributor can handle + /// [`Stop::Finish`] which is returned when migration is requested. pub fn footnotes( &mut self, regions: &Regions, frame: &Frame, flow_need: Abs, breakable: bool, + migratable: bool, ) -> FlowResult<()> { // Footnotes are only supported at the root level. if !self.config.root { @@ -352,7 +374,7 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { let mut relayout = false; let mut regions = *regions; - let mut migratable = !breakable && regions.may_progress(); + let mut migratable = migratable && !breakable && regions.may_progress(); for (y, elem) in notes { // The amount of space used by the in-flow content that contains the diff --git a/crates/typst-layout/src/flow/distribute.rs b/crates/typst-layout/src/flow/distribute.rs index 5b293d352..7a1cf4264 100644 --- a/crates/typst-layout/src/flow/distribute.rs +++ b/crates/typst-layout/src/flow/distribute.rs @@ -240,7 +240,8 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { // Handle fractionally sized blocks. if let Some(fr) = single.fr { - self.composer.footnotes(&self.regions, &frame, Abs::zero(), false)?; + self.composer + .footnotes(&self.regions, &frame, Abs::zero(), false, true)?; self.flush_tags(); self.items.push(Item::Fr(fr, Some(single))); return Ok(()); @@ -323,8 +324,13 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { } // Handle footnotes. - self.composer - .footnotes(&self.regions, &frame, frame.height(), breakable)?; + self.composer.footnotes( + &self.regions, + &frame, + frame.height(), + breakable, + true, + )?; // Push an item for the frame. self.regions.size.y -= frame.height(); @@ -347,11 +353,13 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { placed, &self.regions, self.items.iter().any(|item| matches!(item, Item::Frame(..))), + true, )?; self.regions.size.y -= weak_spacing; } else { let frame = placed.layout(self.composer.engine, self.regions.base())?; - self.composer.footnotes(&self.regions, &frame, Abs::zero(), true)?; + self.composer + .footnotes(&self.regions, &frame, Abs::zero(), true, true)?; self.flush_tags(); self.items.push(Item::Placed(frame, placed)); } diff --git a/crates/typst-layout/src/pages/collect.rs b/crates/typst-layout/src/pages/collect.rs index 1903d6ac5..0bbae9f4c 100644 --- a/crates/typst-layout/src/pages/collect.rs +++ b/crates/typst-layout/src/pages/collect.rs @@ -53,7 +53,7 @@ pub fn collect<'a>( // The initial styles for the next page are ours unless this is a // "boundary" pagebreak. Such a pagebreak is generated at the end of - // the scope of a page set rule to ensure a page boundary. It's + // the scope of a page set rule to ensure a page boundary. Its // styles correspond to the styles _before_ the page set rule, so we // don't want to apply it to a potential empty page. if !pagebreak.boundary(styles) { diff --git a/crates/typst-library/src/model/footnote.rs b/crates/typst-library/src/model/footnote.rs index d9971dd11..ffc78ea05 100644 --- a/crates/typst-library/src/model/footnote.rs +++ b/crates/typst-library/src/model/footnote.rs @@ -194,7 +194,7 @@ cast! { /// before any page content, typically at the very start of the document. #[elem(name = "entry", title = "Footnote Entry", Show, ShowSet)] pub struct FootnoteEntry { - /// The footnote for this entry. It's location can be used to determine + /// The footnote for this entry. Its location can be used to determine /// the footnote counter state. /// /// ```example diff --git a/tests/ref/issue-5435-footnote-migration-in-floats.png b/tests/ref/issue-5435-footnote-migration-in-floats.png new file mode 100644 index 0000000000000000000000000000000000000000..672a5af8354377b57d01f583670e3fe56ccfc6e9 GIT binary patch literal 448 zcmV;x0YCnUP)0{{R382;U20002GP)t-s|Ns90 z007kCU#jxWdE7zro3yoS>YZrkI(XaB_Nje1yWp%!-VZa&&z4_4T~I#>dCU zXlQ8G*49^7S3EpCsi~>7w6xRH(|~}0PEJnZ;^Iq7OH@=;Nl8hptgMldk%fhYm6esg zzP`c1!NkPG>FMe8^z`-h_xt<%i;Ii%^Ye!-V1EDr0KrK_K~#9!?b%hX0x%Rs(eqlK zyW{Te&itpv4EO>LBzsL@CvDn~dj$Xh000000010rOgMP}$M9Jx$>O72ltn1yW$}>B z$s&Y6_&j2a{3!r%;dTe|NW)mXzZELKWehD qgr78>Tf)yTuL(IMZh74RF4i6h93@VhU)O{H0000, C @fn, D @fn, E @fn. --- issue-5256-multiple-footnotes-in-footnote --- // Test whether all footnotes inside another footnote are listed. #footnote[#footnote[A]#footnote[B]#footnote[C]] + +--- issue-5435-footnote-migration-in-floats --- +// Test that a footnote should not prompt migration when in a float that was +// queued to the next page (due to the float being too large), even if the +// footnote does not fit, breaking the footnote invariant. +#set page(height: 50pt) + +#place( + top, + float: true, + { + v(100pt) + footnote[a] + } +) +#place( + top, + float: true, + footnote[b] +)