From 6a8e29b2e5ad5f0b7d469ad6a307f9cf1c5ad07d Mon Sep 17 00:00:00 2001 From: Laurenz Date: Mon, 14 Oct 2024 16:18:25 +0200 Subject: [PATCH] Fix tag order with `place` and fr block bugs (#5203) --- crates/typst/src/layout/flow/compose.rs | 12 ++- crates/typst/src/layout/flow/distribute.rs | 119 +++++++++++---------- tests/ref/footnote-block-fr.png | Bin 0 -> 833 bytes tests/suite/introspection/query.typ | 17 +++ tests/suite/layout/flow/footnote.typ | 8 ++ 5 files changed, 98 insertions(+), 58 deletions(-) create mode 100644 tests/ref/footnote-block-fr.png diff --git a/crates/typst/src/layout/flow/compose.rs b/crates/typst/src/layout/flow/compose.rs index 3c52af382..4b991f2b2 100644 --- a/crates/typst/src/layout/flow/compose.rs +++ b/crates/typst/src/layout/flow/compose.rs @@ -221,7 +221,7 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { // Process pending floats. for placed in std::mem::take(&mut self.work.floats) { - self.float(placed, ®ions, true)?; + self.float(placed, ®ions, false)?; } distribute(self, regions) @@ -280,7 +280,7 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { }; // We only require clearance if there is other content. - let clearance = if clearance { Abs::zero() } else { placed.clearance }; + let clearance = if clearance { placed.clearance } else { Abs::zero() }; let need = frame.height() + clearance; // If the float doesn't fit, queue it for the next region. @@ -338,7 +338,13 @@ impl<'a, 'b> Composer<'a, 'b, '_, '_> { } // Search for footnotes. - let notes = find_in_frame::(frame); + let mut notes = vec![]; + for tag in &self.work.tags { + let Tag::Start(elem) = tag else { continue }; + let Some(note) = elem.to_packed::() else { continue }; + notes.push((Abs::zero(), note.clone())); + } + find_in_frame_impl::(&mut notes, frame, Abs::zero()); if notes.is_empty() { return Ok(()); } diff --git a/crates/typst/src/layout/flow/distribute.rs b/crates/typst/src/layout/flow/distribute.rs index eeb4e76f2..3fa166266 100644 --- a/crates/typst/src/layout/flow/distribute.rs +++ b/crates/typst/src/layout/flow/distribute.rs @@ -54,6 +54,8 @@ struct DistributionSnapshot<'a, 'b> { /// A laid out item in a distribution. enum Item<'a, 'b> { + /// An introspection tag. + Tag(&'a Tag), /// Absolute spacing and its weakness level. Abs(Abs, u8), /// Fractional spacing or a fractional block. @@ -69,6 +71,7 @@ impl Item<'_, '_> { /// consists solely of such items. fn migratable(&self) -> bool { match self { + Self::Tag(_) => true, Self::Frame(frame, _) => { frame.size().is_zero() && frame.items().all(|(_, item)| { @@ -126,6 +129,15 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { self.composer.work.tags.push(tag); } + /// Generate items for pending tags. + fn flush_tags(&mut self) { + if !self.composer.work.tags.is_empty() { + let tags = &mut self.composer.work.tags; + self.items.extend(tags.iter().copied().map(Item::Tag)); + tags.clear(); + } + } + /// Processes relative spacing. fn rel(&mut self, amount: Rel, weakness: u8) { let amount = amount.relative_to(self.regions.base().y); @@ -157,7 +169,7 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { } return false; } - Item::Abs(..) | Item::Placed(..) => {} + Item::Tag(_) | Item::Abs(..) | Item::Placed(..) => {} Item::Fr(.., None) => return false, Item::Frame(..) | Item::Fr(.., Some(_)) => return true, } @@ -174,7 +186,7 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { self.items.remove(i); break; } - Item::Abs(..) | Item::Placed(..) => {} + Item::Tag(_) | Item::Abs(..) | Item::Placed(..) => {} Item::Frame(..) | Item::Fr(..) => break, } } @@ -185,7 +197,7 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { for item in self.items.iter().rev() { match *item { Item::Abs(amount, 1..) => return amount, - Item::Abs(..) | Item::Placed(..) => {} + Item::Tag(_) | Item::Abs(..) | Item::Placed(..) => {} Item::Frame(..) | Item::Fr(..) => break, } } @@ -219,18 +231,20 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { /// Processes an unbreakable block. fn single(&mut self, single: &'b SingleChild<'a>) -> FlowResult<()> { - // Handle fractionally sized blocks. - if let Some(fr) = single.fr { - self.items.push(Item::Fr(fr, Some(single))); - return Ok(()); - } - // Lay out the block. let frame = single.layout( self.composer.engine, Region::new(self.regions.base(), self.regions.expand), )?; + // Handle fractionally sized blocks. + if let Some(fr) = single.fr { + self.composer.footnotes(&self.regions, &frame, Abs::zero(), false)?; + self.flush_tags(); + self.items.push(Item::Fr(fr, Some(single))); + return Ok(()); + } + // If the block doesn't fit and a followup region may improve things, // finish the region. if !self.regions.size.y.fits(frame.height()) && self.regions.may_progress() { @@ -289,7 +303,7 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { /// Processes an in-flow frame, generated from a line or block. fn frame( &mut self, - mut frame: Frame, + frame: Frame, align: Axes, sticky: bool, breakable: bool, @@ -307,26 +321,13 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { self.sticky = None; } - if !frame.is_empty() { - // Drain tags. - let tags = &mut self.composer.work.tags; - if !tags.is_empty() { - frame.prepend_multiple( - tags.iter().map(|&tag| (Point::zero(), FrameItem::Tag(tag.clone()))), - ); - } - - // Handle footnotes. - self.composer - .footnotes(&self.regions, &frame, frame.height(), breakable)?; - - // Clear the drained tags _after_ the footnotes are handled because - // a [`Stop::Finish`] could otherwise lose them. - self.composer.work.tags.clear(); - } + // Handle footnotes. + self.composer + .footnotes(&self.regions, &frame, frame.height(), breakable)?; // Push an item for the frame. self.regions.size.y -= frame.height(); + self.flush_tags(); self.items.push(Item::Frame(frame, align)); Ok(()) } @@ -341,11 +342,16 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { // ends up at a break due to the float. let weak_spacing = self.weak_spacing(); self.regions.size.y += weak_spacing; - self.composer.float(placed, &self.regions, self.items.is_empty())?; + self.composer.float( + placed, + &self.regions, + self.items.iter().any(|item| matches!(item, Item::Frame(..))), + )?; 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.flush_tags(); self.items.push(Item::Placed(frame, placed)); } Ok(()) @@ -382,17 +388,18 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { init: DistributionSnapshot<'a, 'b>, forced: bool, ) -> FlowResult { - if !forced { - if !self.items.is_empty() && self.items.iter().all(Item::migratable) { - // Restore the initial state of all items are migratable. - self.restore(init); - } else { - // If we ended on a sticky block, but are not yet at the end of - // the flow, restore the saved checkpoint to move the sticky - // suffix to the next region. - if let Some(snapshot) = self.sticky.take() { - self.restore(snapshot) - } + if forced { + // If this is the very end of the flow, flush pending tags. + self.flush_tags(); + } else if !self.items.is_empty() && self.items.iter().all(Item::migratable) { + // Restore the initial state of all items are migratable. + self.restore(init); + } else { + // If we ended on a sticky block, but are not yet at the end of + // the flow, restore the saved checkpoint to move the sticky + // suffix to the next region. + if let Some(snapshot) = self.sticky.take() { + self.restore(snapshot) } } @@ -400,28 +407,34 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { let mut frs = Fr::zero(); let mut used = Size::zero(); + let mut has_fr_child = false; // Determine the amount of used space and the sum of fractionals. for item in &self.items { match item { Item::Abs(v, _) => used.y += *v, - Item::Fr(v, _) => frs += *v, + Item::Fr(v, child) => { + frs += *v; + has_fr_child |= child.is_some(); + } Item::Frame(frame, _) => { used.y += frame.height(); used.x.set_max(frame.width()); } - Item::Placed(..) => {} + Item::Tag(_) | Item::Placed(..) => {} } } // When we have fractional spacing, occupy the remaining space with it. let mut fr_space = Abs::zero(); - let mut fr_frames = vec![]; if frs.get() > 0.0 && region.size.y.is_finite() { fr_space = region.size.y - used.y; used.y = region.size.y; + } - // Lay out fractionally sized blocks. + // Lay out fractionally sized blocks. + let mut fr_frames = vec![]; + if has_fr_child { for item in &self.items { let Item::Fr(v, Some(single)) = item else { continue }; let length = v.share(frs, fr_space); @@ -439,6 +452,7 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { // Determine the region's size. let size = region.expand.select(region.size, used.min(region.size)); + let free = size.y - used.y; let mut output = Frame::soft(size); let mut ruler = FixedAlignment::Start; @@ -448,6 +462,11 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { // Position all items. for item in self.items { match item { + Item::Tag(tag) => { + let y = offset + ruler.position(free); + let pos = Point::with_y(y); + output.push(pos, FrameItem::Tag(tag.clone())); + } Item::Abs(v, _) => { offset += v; } @@ -465,7 +484,7 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { ruler = ruler.max(align.y); let x = align.x.position(size.x - frame.width()); - let y = offset + ruler.position(size.y - used.y); + let y = offset + ruler.position(free); let pos = Point::new(x, y); offset += frame.height(); @@ -475,7 +494,7 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { let x = placed.align_x.position(size.x - frame.width()); let y = match placed.align_y.unwrap_or_default() { Some(align) => align.position(size.y - frame.height()), - _ => offset + ruler.position(size.y - used.y), + _ => offset + ruler.position(free), }; let pos = Point::new(x, y) @@ -486,16 +505,6 @@ impl<'a, 'b> Distributor<'a, 'b, '_, '_, '_> { } } - // If this is the very end of the flow, drain trailing tags. - if forced && !self.composer.work.tags.is_empty() { - let tags = &mut self.composer.work.tags; - let pos = Point::with_y(offset); - output.push_multiple( - tags.iter().map(|&tag| (pos, FrameItem::Tag(tag.clone()))), - ); - tags.clear(); - } - Ok(output) } diff --git a/tests/ref/footnote-block-fr.png b/tests/ref/footnote-block-fr.png new file mode 100644 index 0000000000000000000000000000000000000000..451e4d778dc24c798e4957dc72b378e6eef96b2c GIT binary patch literal 833 zcmV-H1HSx;P)YrDC_I%jg5`*@$qPAX!rN|zre)%`~3Cw_YfQ)>+JA8K0fK` z={q|+P>P}E=H^sXRNvp<-rnBJ%gbqLX~xFJiHV6yN=llWpJuJmbai>v*4l5p;x<}u zG*@drXn&CSi5o13kzt!%p4o1CDSnVp=TriqJ_Vq|Q;!O32r!AN_RRgte~ zu+((P>Se3WRgthobdF4coz>OV!NI{cHa3=)mU6}BOoN#qI6+&UxEn7wKW>0DT4{WI zd~k4Zl$4b9_4Rko@LQd?X|~j0rNL{s*8>Izc+c@5H$ZN^-D9iBetv%1+S=gY;?B;{ zuCTa`kC!k~U`>Xc+}zxC$?LVXwWzAHq^7R2w7jXSv#zeLg@uKRii&SCd3wziOvkVr^K&(F{J`1t$#`z|gn(9qB)CntD#c<=A;LqkK1jEr(} za;mDT?Ck9A?d|sV_EJ((nwpyG>gtn|lb@fT+1c61$jIpE==%El*x1-{adH0s{-~&^ z*VorM-M5Ya00C7=L_t(|+U?d=Q-VPlhT&&F{3OKg?r!Yv?(Xiu!Um*Mf3!0kdEjuy zF6evxfqQm#o}C3DghaSo8-&QrvO@@!IuRl$HQhZc5I~jP)w>M-CdPn!!vnY*A63G2 zaZcMNgpe?HrQh8K3JWA{o)$b^p6zUHvZ7+SMc2ea1ixX?Y<_{sC30Jbj&C?VWnhry zmzK46HuFAl>rk~Ez&p%_`vruT=4Pfw`UHd*XQw9_iwfWxW_jFV%C;WU^4mmmB+KHD8m8aE*%l6(e*(D@3A zF6J+ZU0d~-%-~PjSbur7iU{xT!RHkbK0FW #context query().first() + +--- issue-5117-query-order-place --- +#let t(expected) = context { + let elems = query(selector(metadata).after(here())) + let val = elems.first().value + test(val, expected) +} + +#{ + t("a") + place(metadata("a")) +} + +#{ + t("b") + block(height: 1fr, metadata("b")) +} diff --git a/tests/suite/layout/flow/footnote.typ b/tests/suite/layout/flow/footnote.typ index 4cf49777c..3230c3da0 100644 --- a/tests/suite/layout/flow/footnote.typ +++ b/tests/suite/layout/flow/footnote.typ @@ -114,6 +114,14 @@ Beautiful footnotes. #footnote[Wonderful, aren't they?] A #block(footnote[hello]) +--- footnote-block-fr --- +#set page(height: 110pt) +A +#block(width: 100%, height: 1fr, fill: aqua)[ + B #footnote[I] #footnote[II] +] +C + --- footnote-float-priority --- #set page(height: 100pt)