Set Rules Episode VI: Return of the Refactor

src/eval/node.rs

@@ -81,7 +81,7 @@ impl Node {
         if let Node::Block(packed) = self {
             packed
         } else {
-            let mut packer = NodePacker::new(true);
+            let mut packer = Packer::new(false);
             packer.walk(self, Styles::new());
             packer.into_block()
         }
@@ -89,7 +89,7 @@ impl Node {
 
     /// Lift to a document node, the root of the layout tree.
     pub fn into_document(self) -> DocumentNode {
-        let mut packer = NodePacker::new(false);
+        let mut packer = Packer::new(true);
         packer.walk(self, Styles::new());
         packer.into_document()
     }
@@ -126,49 +126,37 @@ impl AddAssign for Node {
 }
 
 /// Packs a [`Node`] into a flow or whole document.
-struct NodePacker {
+struct Packer {
-    /// Whether packing should produce a block-level node.
+    /// Whether this packer produces the top-level document.
-    block: bool,
+    top: bool,
     /// The accumulated page nodes.
     pages: Vec<PageNode>,
     /// The accumulated flow children.
-    flow: Vec<FlowChild>,
+    flow: Builder<FlowChild>,
-    /// The common style properties of all items on the current flow.
-    flow_styles: Styles,
-    /// The kind of thing that was last added to the current flow.
-    flow_last: Last<FlowChild>,
     /// The accumulated paragraph children.
-    par: Vec<ParChild>,
+    par: Builder<ParChild>,
-    /// The common style properties of all items in the current paragraph.
-    par_styles: Styles,
-    /// The kind of thing that was last added to the current paragraph.
-    par_last: Last<ParChild>,
 }
 
-impl NodePacker {
+impl Packer {
     /// Start a new node-packing session.
-    fn new(block: bool) -> Self {
+    fn new(top: bool) -> Self {
         Self {
-            block,
+            top,
             pages: vec![],
-            flow: vec![],
+            flow: Builder::default(),
-            flow_styles: Styles::new(),
+            par: Builder::default(),
-            flow_last: Last::None,
-            par: vec![],
-            par_styles: Styles::new(),
-            par_last: Last::None,
         }
     }
 
     /// Finish up and return the resulting flow.
     fn into_block(mut self) -> PackedNode {
-        self.finish_par();
+        self.parbreak(None);
-        FlowNode(self.flow).pack()
+        FlowNode(self.flow.children).pack()
     }
 
     /// Finish up and return the resulting document.
     fn into_document(mut self) -> DocumentNode {
-        self.pagebreak(true);
+        self.pagebreak();
         DocumentNode(self.pages)
     }
 
@@ -176,34 +164,49 @@ impl NodePacker {
     fn walk(&mut self, node: Node, styles: Styles) {
         match node {
             Node::Space => {
-                if self.is_flow_compatible(&styles) && self.is_par_compatible(&styles) {
+                // A text space is "soft", meaning that it can be eaten up by
-                    self.par_last.soft(ParChild::text(' ', styles));
+                // adjacent line breaks or explicit spacings.
-                }
+                self.par.last.soft(ParChild::text(' ', styles));
             }
             Node::Linebreak => {
-                self.par_last.hard();
+                // A line break eats up surrounding text spaces.
+                self.par.last.hard();
                 self.push_inline(ParChild::text('\n', styles));
-                self.par_last.hard();
+                self.par.last.hard();
             }
             Node::Parbreak => {
+                // An explicit paragraph break is styled according to the active
+                // styles (`Some(_)`) whereas paragraph breaks forced by
+                // incompatibility take their styles from the preceding
+                // paragraph.
                 self.parbreak(Some(styles));
             }
             Node::Pagebreak => {
-                self.pagebreak(true);
+                // We must set the flow styles after the page break such that an
-                self.flow_styles = styles;
+                // empty page created by two page breaks in a row has styles at
+                // all.
+                self.pagebreak();
+                self.flow.styles = styles;
             }
             Node::Text(text) => {
                 self.push_inline(ParChild::text(text, styles));
             }
             Node::Spacing(SpecAxis::Horizontal, kind) => {
-                self.par_last.hard();
+                // Just like a line break, explicit horizontal spacing eats up
+                // surrounding text spaces.
+                self.par.last.hard();
                 self.push_inline(ParChild::Spacing(SpacingNode { kind, styles }));
-                self.par_last.hard();
+                self.par.last.hard();
             }
             Node::Spacing(SpecAxis::Vertical, kind) => {
-                self.finish_par();
+                // Explicit vertical spacing ends the current paragraph and then
-                self.flow.push(FlowChild::Spacing(SpacingNode { kind, styles }));
+                // discards the paragraph break.
-                self.flow_last.hard();
+                self.parbreak(None);
+                self.make_flow_compatible(&styles);
+                self.flow
+                    .children
+                    .push(FlowChild::Spacing(SpacingNode { kind, styles }));
+                self.flow.last.hard();
             }
             Node::Inline(inline) => {
                 self.push_inline(ParChild::Node(inline.styled(styles)));
@@ -212,6 +215,8 @@ impl NodePacker {
                 self.push_block(block.styled(styles));
             }
             Node::Sequence(list) => {
+                // For a list of nodes, we apply the list's styles to each node
+                // individually.
                 for (node, mut inner) in list {
                     inner.apply(&styles);
                     self.walk(node, inner);
@@ -222,22 +227,22 @@ impl NodePacker {
 
     /// Insert an inline-level element into the current paragraph.
     fn push_inline(&mut self, child: ParChild) {
-        if let Some(child) = self.par_last.any() {
+        if let Some(child) = self.par.last.any() {
-            self.push_inline_impl(child);
+            self.push_coalescing(child);
         }
 
         // The node must be both compatible with the current page and the
         // current paragraph.
         self.make_flow_compatible(child.styles());
         self.make_par_compatible(child.styles());
-        self.push_inline_impl(child);
+        self.push_coalescing(child);
-        self.par_last = Last::Any;
+        self.par.last.any();
     }
 
     /// Push a paragraph child, coalescing text nodes with compatible styles.
-    fn push_inline_impl(&mut self, child: ParChild) {
+    fn push_coalescing(&mut self, child: ParChild) {
         if let ParChild::Text(right) = &child {
-            if let Some(ParChild::Text(left)) = self.par.last_mut() {
+            if let Some(ParChild::Text(left)) = self.par.children.last_mut() {
                 if left.styles.compatible(&right.styles, TextNode::has_property) {
                     left.text.push_str(&right.text);
                     return;
@@ -245,137 +250,122 @@ impl NodePacker {
             }
         }
 
-        self.par.push(child);
+        self.par.children.push(child);
     }
 
     /// Insert a block-level element into the current flow.
     fn push_block(&mut self, node: PackedNode) {
-        let mut is_placed = false;
+        let placed = node.is::<PlacedNode>();
-        if let Some(placed) = node.downcast::<PlacedNode>() {
-            is_placed = true;
-
-            // This prevents paragraph spacing after the placed node if it
-            // is completely out-of-flow.
-            if placed.out_of_flow() {
-                self.flow_last = Last::None;
-            }
-        }
 
         self.parbreak(None);
         self.make_flow_compatible(&node.styles);
-
+        self.flow.children.extend(self.flow.last.any());
-        if let Some(child) = self.flow_last.any() {
+        self.flow.children.push(FlowChild::Node(node));
-            self.flow.push(child);
-        }
-
-        self.flow.push(FlowChild::Node(node));
         self.parbreak(None);
 
-        // This prevents paragraph spacing between the placed node and
+        // Prevent paragraph spacing between the placed node and the paragraph
-        // the paragraph below it.
+        // below it.
-        if is_placed {
+        if placed {
-            self.flow_last = Last::None;
+            self.flow.last.hard();
         }
     }
 
     /// Advance to the next paragraph.
     fn parbreak(&mut self, break_styles: Option<Styles>) {
-        let styles = break_styles.unwrap_or_else(|| self.par_styles.clone());
+        // Erase any styles that will be inherited anyway.
-        self.finish_par();
+        let Builder { mut children, styles, .. } = mem::take(&mut self.par);
+        for child in &mut children {
+            child.styles_mut().erase(&styles);
+        }
 
-        // Insert paragraph spacing.
+        // For explicit paragraph breaks, `break_styles` is already `Some(_)`.
-        self.flow_last.soft(FlowChild::Parbreak(styles));
+        // For page breaks due to incompatibility, we fall back to the styles
-    }
+        // of the preceding paragraph.
+        let break_styles = break_styles.unwrap_or_else(|| styles.clone());
 
-    fn finish_par(&mut self) {
+        // We don't want empty paragraphs.
-        let mut children = mem::take(&mut self.par);
-        let styles = mem::take(&mut self.par_styles);
-        self.par_last = Last::None;
-
-        // No empty paragraphs.
         if !children.is_empty() {
-            // Erase any styles that will be inherited anyway.
-            for child in &mut children {
-                child.styles_mut().erase(&styles);
-            }
-
-            if let Some(child) = self.flow_last.any() {
-                self.flow.push(child);
-            }
-
             // The paragraph's children are all compatible with the page, so the
             // paragraph is too, meaning we don't need to check or intersect
             // anything here.
-            let node = ParNode(children).pack().styled(styles);
+            let par = ParNode(children).pack().styled(styles);
-            self.flow.push(FlowChild::Node(node));
+            self.flow.children.extend(self.flow.last.any());
+            self.flow.children.push(FlowChild::Node(par));
         }
+
+        // Insert paragraph spacing.
+        self.flow.last.soft(FlowChild::Break(break_styles));
     }
 
     /// Advance to the next page.
-    fn pagebreak(&mut self, keep: bool) {
+    fn pagebreak(&mut self) {
-        if self.block {
+        if self.top {
-            return;
+            self.parbreak(None);
-        }
 
-        self.finish_par();
+            // Take the flow and erase any styles that will be inherited anyway.
-
+            let Builder { mut children, styles, .. } = mem::take(&mut self.flow);
-        let styles = mem::take(&mut self.flow_styles);
-        let mut children = mem::take(&mut self.flow);
-        self.flow_last = Last::None;
-
-        if keep || !children.is_empty() {
-            // Erase any styles that will be inherited anyway.
             for child in &mut children {
                 child.styles_mut().erase(&styles);
             }
 
-            let node = PageNode { node: FlowNode(children).pack(), styles };
+            let flow = FlowNode(children).pack();
-            self.pages.push(node);
+            let page = PageNode { child: flow, styles };
+            self.pages.push(page);
         }
     }
 
     /// Break to a new paragraph if the `styles` contain paragraph styles that
     /// are incompatible with the current paragraph.
     fn make_par_compatible(&mut self, styles: &Styles) {
-        if self.par.is_empty() {
+        if self.par.children.is_empty() {
-            self.par_styles = styles.clone();
+            self.par.styles = styles.clone();
             return;
         }
 
-        if !self.is_par_compatible(styles) {
+        if !self.par.styles.compatible(&styles, ParNode::has_property) {
             self.parbreak(None);
-            self.par_styles = styles.clone();
+            self.par.styles = styles.clone();
             return;
         }
 
-        self.par_styles.intersect(&styles);
+        self.par.styles.intersect(&styles);
     }
 
     /// Break to a new page if the `styles` contain page styles that are
     /// incompatible with the current flow.
     fn make_flow_compatible(&mut self, styles: &Styles) {
-        if self.flow.is_empty() && self.par.is_empty() {
+        if self.flow.children.is_empty() && self.par.children.is_empty() {
-            self.flow_styles = styles.clone();
+            self.flow.styles = styles.clone();
             return;
         }
 
-        if !self.is_flow_compatible(styles) {
+        if self.top && !self.flow.styles.compatible(&styles, PageNode::has_property) {
-            self.pagebreak(false);
+            self.pagebreak();
-            self.flow_styles = styles.clone();
+            self.flow.styles = styles.clone();
             return;
         }
 
-        self.flow_styles.intersect(styles);
+        self.flow.styles.intersect(styles);
     }
+}
 
-    /// Whether the given styles are compatible with the current page.
+/// Container for building a flow or paragraph.
-    fn is_par_compatible(&self, styles: &Styles) -> bool {
+struct Builder<T> {
-        self.par_styles.compatible(&styles, ParNode::has_property)
+    /// The intersection of the style properties of all `children`.
-    }
+    styles: Styles,
+    /// The accumulated flow or paragraph children.
+    children: Vec<T>,
+    /// The kind of thing that was last added.
+    last: Last<T>,
+}
 
-    /// Whether the given styles are compatible with the current flow.
+impl<T> Default for Builder<T> {
-    fn is_flow_compatible(&self, styles: &Styles) -> bool {
+    fn default() -> Self {
-        self.block || self.flow_styles.compatible(&styles, PageNode::has_property)
+        Self {
+            styles: Styles::new(),
+            children: vec![],
+            last: Last::None,
+        }
     }
 }
 
@@ -383,6 +373,7 @@ impl NodePacker {
 enum Last<N> {
     None,
     Any,
+    Hard,
     Soft(N),
 }
 
@@ -401,6 +392,6 @@ impl<N> Last<N> {
     }
 
     fn hard(&mut self) {
-        *self = Self::None;
+        *self = Self::Hard;
     }
 }

src/library/flow.rs

@@ -30,30 +30,30 @@ impl Debug for FlowNode {
 /// A child of a flow node.
 #[derive(Hash)]
 pub enum FlowChild {
+    /// A paragraph/block break.
+    Break(Styles),
     /// Vertical spacing between other children.
     Spacing(SpacingNode),
     /// An arbitrary node.
     Node(PackedNode),
-    /// A paragraph break.
-    Parbreak(Styles),
 }
 
 impl FlowChild {
     /// A reference to the child's styles.
     pub fn styles(&self) -> &Styles {
         match self {
+            Self::Break(styles) => styles,
             Self::Spacing(node) => &node.styles,
             Self::Node(node) => &node.styles,
-            Self::Parbreak(styles) => styles,
         }
     }
 
     /// A mutable reference to the child's styles.
     pub fn styles_mut(&mut self) -> &mut Styles {
         match self {
+            Self::Break(styles) => styles,
             Self::Spacing(node) => &mut node.styles,
             Self::Node(node) => &mut node.styles,
-            Self::Parbreak(styles) => styles,
         }
     }
 }
@@ -61,14 +61,14 @@ impl FlowChild {
 impl Debug for FlowChild {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         match self {
-            Self::Spacing(node) => node.fmt(f),
+            Self::Break(styles) => {
-            Self::Node(node) => node.fmt(f),
-            Self::Parbreak(styles) => {
                 if f.alternate() {
                     styles.fmt(f)?;
                 }
-                write!(f, "Parbreak")
+                write!(f, "Break")
             }
+            Self::Spacing(node) => node.fmt(f),
+            Self::Node(node) => node.fmt(f),
         }
     }
 }
@@ -132,6 +132,13 @@ impl<'a> FlowLayouter<'a> {
     fn layout(mut self, ctx: &mut LayoutContext) -> Vec<Constrained<Rc<Frame>>> {
         for child in self.children {
             match child {
+                FlowChild::Break(styles) => {
+                    let chain = styles.chain(&ctx.styles);
+                    let amount = chain
+                        .get(ParNode::SPACING)
+                        .resolve(chain.get(TextNode::SIZE).abs);
+                    self.layout_absolute(amount.into());
+                }
                 FlowChild::Spacing(node) => match node.kind {
                     SpacingKind::Linear(v) => self.layout_absolute(v),
                     SpacingKind::Fractional(v) => {
@@ -146,13 +153,6 @@ impl<'a> FlowLayouter<'a> {
 
                     self.layout_node(ctx, node);
                 }
-                FlowChild::Parbreak(styles) => {
-                    let chain = styles.chain(&ctx.styles);
-                    let amount = chain
-                        .get(ParNode::SPACING)
-                        .resolve(chain.get(TextNode::SIZE).abs);
-                    self.layout_absolute(amount.into());
-                }
             }
         }
 
@@ -248,7 +248,7 @@ impl<'a> FlowLayouter<'a> {
                     output.push_frame(pos, frame);
                 }
                 FlowItem::Placed(frame) => {
-                    output.push_frame(Point::with_y(offset), frame);
+                    output.push_frame(Point::zero(), frame);
                 }
             }
         }

src/library/page.rs

@@ -62,7 +62,7 @@ pub fn pagebreak(_: &mut EvalContext, _: &mut Args) -> TypResult<Value> {
 #[derive(Hash)]
 pub struct PageNode {
     /// The node producing the content.
-    pub node: PackedNode,
+    pub child: PackedNode,
     /// The page's styles.
     pub styles: Styles,
 }
@@ -116,7 +116,7 @@ impl PageNode {
         };
 
         // Pad the child.
-        let padded = PadNode { child: self.node.clone(), padding }.pack();
+        let padded = PadNode { child: self.child.clone(), padding }.pack();
 
         // Layout the child.
         let expand = size.map(Length::is_finite);
@@ -143,7 +143,7 @@ impl Debug for PageNode {
             self.styles.fmt(f)?;
         }
         f.write_str("Page(")?;
-        self.node.fmt(f)?;
+        self.child.fmt(f)?;
         f.write_str(")")
     }
 }

src/library/placed.rs

@@ -51,13 +51,6 @@ impl Layout for PlacedNode {
         let target = regions.expand.select(regions.current, Size::zero());
         Rc::make_mut(frame).resize(target, Align::LEFT_TOP);
 
-        // Place relative to parent's base origin by offsetting our elements by
-        // the negative cursor position.
-        if out_of_flow {
-            let offset = (regions.current - regions.base).to_point();
-            Rc::make_mut(frame).translate(offset);
-        }
-
         // Set base constraint because our pod size is base and exact
         // constraints if we needed to expand or offset.
         *cts = Constraints::new(regions.expand);

tests/typ/layout/placed.typ

@@ -1,3 +1,6 @@
+// Test the `place` function.
+
+---
 #page("a8")
 #place(bottom + center)[© Typst]
 
@@ -20,3 +23,13 @@ the line breaks still had to be inserted manually.
   #place(center, dx: 7pt, dy: 5pt)[Hello]
   Hello #h(1fr) Hello
 ]
+
+---
+// Test how the placed node interacts with paragraph spacing around it.
+#page("a8", height: 60pt)
+
+First
+
+#place(bottom + right)[Placed]
+
+Second

tests/typ/layout/spacing.typ

@@ -17,6 +17,13 @@ Add #h(10pt) #h(10pt) up
 // Fractional.
 | #h(1fr) | #h(2fr) | #h(1fr) |
 
+---
+// Test that spacing has style properties.
+
+A[#par(align: right)#h(1cm)]B
+[#page(height: 20pt)#v(1cm)]
+B
+
 ---
 // Missing spacing.
 // Error: 11-13 missing argument: spacing