From bbead018c684d2cb35470b064d8b879a1232f592 Mon Sep 17 00:00:00 2001
From: Pierre Le Fevre <hello@pierrelf.com>
Date: Sat, 21 Mar 2026 19:20:01 +0100
Subject: [PATCH] Implement vertical margin collapsing for block-level elements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Implements CSS2 §8.3.1 margin collapsing:
- Adjacent sibling margins collapse (gap = max, not sum)
- Parent-child margin collapsing via pre_collapse_margins pass
- Empty block self-collapsing (top+bottom margins fold together)
- Negative margin handling (positive+negative=sum, both negative=min)
- Collapsing blocked by border, padding, or overflow!=visible (BFC)

Adds overflow field to LayoutBox for BFC detection. Updates existing
tests to reflect correct collapsed margin behavior and adds 9 new
tests covering all collapsing scenarios.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 crates/layout/src/lib.rs | 502 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 493 insertions(+), 9 deletions(-)

diff --git a/crates/layout/src/lib.rs b/crates/layout/src/lib.rs
index ecd8a1e..6cc4b41 100644
--- a/crates/layout/src/lib.rs
+++ b/crates/layout/src/lib.rs
@@ -8,7 +8,7 @@ use std::collections::HashMap;
 use we_css::values::Color;
 use we_dom::{Document, NodeData, NodeId};
 use we_style::computed::{
-    BorderStyle, ComputedStyle, Display, LengthOrAuto, Position, StyledNode, TextAlign,
+    BorderStyle, ComputedStyle, Display, LengthOrAuto, Overflow, Position, StyledNode, TextAlign,
     TextDecoration,
 };
 use we_text::font::Font;
@@ -93,6 +93,8 @@ pub struct LayoutBox {
     pub position: Position,
     /// Relative position offset (dx, dy) applied after normal flow layout.
     pub relative_offset: (f32, f32),
+    /// CSS `overflow` property.
+    pub overflow: Overflow,
 }
 
 impl LayoutBox {
@@ -126,6 +128,7 @@ impl LayoutBox {
             replaced_size: None,
             position: style.position,
             relative_offset: (0.0, 0.0),
+            overflow: style.overflow,
         }
     }
 
@@ -484,15 +487,201 @@ fn has_block_children(b: &LayoutBox) -> bool {
     b.children.iter().any(is_block_level)
 }
 
-/// Lay out block-level children: stack them vertically.
+/// Collapse two adjoining margins per CSS2 §8.3.1.
+///
+/// Both non-negative → use the larger.
+/// Both negative → use the more negative.
+/// Mixed → sum the largest positive and most negative.
+fn collapse_margins(a: f32, b: f32) -> f32 {
+    if a >= 0.0 && b >= 0.0 {
+        a.max(b)
+    } else if a < 0.0 && b < 0.0 {
+        a.min(b)
+    } else {
+        a + b
+    }
+}
+
+/// Returns `true` if this box establishes a new block formatting context,
+/// which prevents its margins from collapsing with children.
+fn establishes_bfc(b: &LayoutBox) -> bool {
+    b.overflow != Overflow::Visible
+}
+
+/// Returns `true` if a block box has no in-flow content (empty block).
+fn is_empty_block(b: &LayoutBox) -> bool {
+    b.children.is_empty() && b.lines.is_empty() && b.replaced_size.is_none()
+}
+
+/// Pre-collapse parent-child margins (CSS2 §8.3.1).
+///
+/// When a parent has no border/padding/BFC separating it from its first/last
+/// child, the child's margin collapses into the parent's margin.  This must
+/// happen *before* positioning so the parent is placed using the collapsed
+/// value.  The function walks bottom-up: children are pre-collapsed first, then
+/// their (possibly enlarged) margins are folded into the parent.
+fn pre_collapse_margins(b: &mut LayoutBox) {
+    // Recurse into block children first (bottom-up).
+    for child in &mut b.children {
+        if is_block_level(child) {
+            pre_collapse_margins(child);
+        }
+    }
+
+    if !matches!(b.box_type, BoxType::Block(_) | BoxType::Anonymous) {
+        return;
+    }
+    if establishes_bfc(b) {
+        return;
+    }
+    if !has_block_children(b) {
+        return;
+    }
+
+    // --- Top: collapse with first non-empty child ---
+    if b.border.top == 0.0 && b.padding.top == 0.0 {
+        if let Some(child_top) = first_block_top_margin(&b.children) {
+            b.margin.top = collapse_margins(b.margin.top, child_top);
+        }
+    }
+
+    // --- Bottom: collapse with last non-empty child ---
+    if b.border.bottom == 0.0 && b.padding.bottom == 0.0 {
+        if let Some(child_bottom) = last_block_bottom_margin(&b.children) {
+            b.margin.bottom = collapse_margins(b.margin.bottom, child_bottom);
+        }
+    }
+}
+
+/// Top margin of the first non-empty block child (already pre-collapsed).
+fn first_block_top_margin(children: &[LayoutBox]) -> Option<f32> {
+    for child in children {
+        if is_block_level(child) {
+            if is_empty_block(child) {
+                continue;
+            }
+            return Some(child.margin.top);
+        }
+    }
+    // All block children empty — fold all their collapsed margins.
+    let mut m = 0.0f32;
+    for child in children.iter().filter(|c| is_block_level(c)) {
+        m = collapse_margins(m, collapse_margins(child.margin.top, child.margin.bottom));
+    }
+    if m != 0.0 {
+        Some(m)
+    } else {
+        None
+    }
+}
+
+/// Bottom margin of the last non-empty block child (already pre-collapsed).
+fn last_block_bottom_margin(children: &[LayoutBox]) -> Option<f32> {
+    for child in children.iter().rev() {
+        if is_block_level(child) {
+            if is_empty_block(child) {
+                continue;
+            }
+            return Some(child.margin.bottom);
+        }
+    }
+    let mut m = 0.0f32;
+    for child in children.iter().filter(|c| is_block_level(c)) {
+        m = collapse_margins(m, collapse_margins(child.margin.top, child.margin.bottom));
+    }
+    if m != 0.0 {
+        Some(m)
+    } else {
+        None
+    }
+}
+
+/// Lay out block-level children with vertical margin collapsing (CSS2 §8.3.1).
+///
+/// Handles adjacent-sibling collapsing, empty-block collapsing, and
+/// parent-child internal spacing (the parent's external margins were already
+/// updated by `pre_collapse_margins`).
 fn layout_block_children(parent: &mut LayoutBox, font: &Font, doc: &Document) {
     let content_x = parent.rect.x;
     let content_width = parent.rect.width;
     let mut cursor_y = parent.rect.y;
 
-    for child in &mut parent.children {
-        compute_layout(child, content_x, cursor_y, content_width, font, doc);
-        cursor_y += child.margin_box_height();
+    let parent_top_open =
+        parent.border.top == 0.0 && parent.padding.top == 0.0 && !establishes_bfc(parent);
+    let parent_bottom_open =
+        parent.border.bottom == 0.0 && parent.padding.bottom == 0.0 && !establishes_bfc(parent);
+
+    // Pending bottom margin from the previous sibling.
+    let mut pending_margin: Option<f32> = None;
+    let child_count = parent.children.len();
+
+    for i in 0..child_count {
+        let child_top_margin = parent.children[i].margin.top;
+        let child_bottom_margin = parent.children[i].margin.bottom;
+
+        // --- Empty block: top+bottom margins self-collapse ---
+        if is_empty_block(&parent.children[i]) {
+            let self_collapsed = collapse_margins(child_top_margin, child_bottom_margin);
+            pending_margin = Some(match pending_margin {
+                Some(prev) => collapse_margins(prev, self_collapsed),
+                None => self_collapsed,
+            });
+            // Position at cursor_y with zero height.
+            let child = &mut parent.children[i];
+            child.rect.x = content_x + child.border.left + child.padding.left;
+            child.rect.y = cursor_y + child.border.top + child.padding.top;
+            child.rect.width = (content_width
+                - child.border.left
+                - child.border.right
+                - child.padding.left
+                - child.padding.right)
+                .max(0.0);
+            child.rect.height = 0.0;
+            continue;
+        }
+
+        // --- Compute effective top spacing ---
+        let collapsed_top = if let Some(prev_bottom) = pending_margin.take() {
+            // Sibling collapsing: previous bottom vs this top.
+            collapse_margins(prev_bottom, child_top_margin)
+        } else if i == 0 && parent_top_open {
+            // First child, parent top open: margin was already pulled into
+            // parent by pre_collapse_margins — no internal spacing.
+            0.0
+        } else {
+            child_top_margin
+        };
+
+        // `compute_layout` adds `child.margin.top` internally, so compensate.
+        let y_for_child = cursor_y + collapsed_top - child_top_margin;
+        compute_layout(
+            &mut parent.children[i],
+            content_x,
+            y_for_child,
+            content_width,
+            font,
+            doc,
+        );
+
+        let child = &parent.children[i];
+        // Use the normal-flow position (before relative offset) so that
+        // `position: relative` does not affect sibling placement.
+        let (_, rel_dy) = child.relative_offset;
+        cursor_y = (child.rect.y - rel_dy)
+            + child.rect.height
+            + child.padding.bottom
+            + child.border.bottom;
+        pending_margin = Some(child_bottom_margin);
+    }
+
+    // Trailing margin.
+    if let Some(trailing) = pending_margin {
+        if !parent_bottom_open {
+            // Parent has border/padding at bottom — margin stays inside.
+            cursor_y += trailing;
+        }
+        // If parent_bottom_open, the margin was already pulled into the
+        // parent by pre_collapse_margins.
     }
 
     parent.rect.height = cursor_y - parent.rect.y;
@@ -815,6 +1004,9 @@ pub fn layout(
         }
     };
 
+    // Pre-collapse parent-child margins before positioning.
+    pre_collapse_margins(&mut root);
+
     compute_layout(&mut root, 0.0, 0.0, viewport_width, font, doc);
 
     let height = root.margin_box_height();
@@ -991,13 +1183,16 @@ mod tests {
         let tree = layout_doc(&doc);
         let body_box = &tree.root.children[0];
 
-        assert_eq!(body_box.margin.top, 8.0);
+        // body default margin is 8px, but it collapses with p's 16px margin
+        // (parent-child collapsing: no border/padding on body).
+        assert_eq!(body_box.margin.top, 16.0);
         assert_eq!(body_box.margin.right, 8.0);
-        assert_eq!(body_box.margin.bottom, 8.0);
+        assert_eq!(body_box.margin.bottom, 16.0);
         assert_eq!(body_box.margin.left, 8.0);
 
         assert_eq!(body_box.rect.x, 8.0);
-        assert_eq!(body_box.rect.y, 8.0);
+        // body.rect.y = collapsed margin (16) from viewport top.
+        assert_eq!(body_box.rect.y, 16.0);
     }
 
     #[test]
@@ -1267,7 +1462,12 @@ p { margin-top: 50px; margin-bottom: 50px; }
         assert_eq!(first.margin.top, 50.0);
         assert_eq!(first.margin.bottom, 50.0);
 
-        assert!(second.rect.y > first.rect.y + 100.0);
+        // Adjacent sibling margins collapse: gap = max(50, 50) = 50, not 100.
+        let gap = second.rect.y - (first.rect.y + first.rect.height);
+        assert!(
+            (gap - 50.0).abs() < 1.0,
+            "collapsed margin gap should be ~50px, got {gap}"
+        );
     }
 
     #[test]
@@ -1709,4 +1909,288 @@ p { margin: 0; }
         assert_eq!(div_box.position, Position::Static);
         assert_eq!(div_box.relative_offset, (0.0, 0.0));
     }
+
+    // --- Margin collapsing tests ---
+
+    #[test]
+    fn adjacent_sibling_margins_collapse() {
+        // Two <p> elements each with margin 16px: gap should be 16px (max), not 32px (sum).
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+p { margin-top: 16px; margin-bottom: 16px; }
+</style></head>
+<body>
+<p>First</p>
+<p>Second</p>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let first = &body_box.children[0];
+        let second = &body_box.children[1];
+
+        // Gap between first's bottom border-box and second's top border-box
+        // should be the collapsed margin: max(16, 16) = 16.
+        let first_bottom =
+            first.rect.y + first.rect.height + first.padding.bottom + first.border.bottom;
+        let gap = second.rect.y - second.border.top - second.padding.top - first_bottom;
+        assert!(
+            (gap - 16.0).abs() < 1.0,
+            "collapsed sibling margin should be ~16px, got {gap}"
+        );
+    }
+
+    #[test]
+    fn sibling_margins_collapse_unequal() {
+        // p1 bottom-margin 20, p2 top-margin 30: gap should be 30 (max).
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+.first { margin-top: 0; margin-bottom: 20px; }
+.second { margin-top: 30px; margin-bottom: 0; }
+</style></head>
+<body>
+<p class="first">First</p>
+<p class="second">Second</p>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let first = &body_box.children[0];
+        let second = &body_box.children[1];
+
+        let first_bottom =
+            first.rect.y + first.rect.height + first.padding.bottom + first.border.bottom;
+        let gap = second.rect.y - second.border.top - second.padding.top - first_bottom;
+        assert!(
+            (gap - 30.0).abs() < 1.0,
+            "collapsed margin should be max(20, 30) = 30, got {gap}"
+        );
+    }
+
+    #[test]
+    fn parent_first_child_margin_collapsing() {
+        // Parent with no padding/border: first child's top margin collapses.
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+.parent { margin-top: 10px; }
+.child { margin-top: 20px; }
+</style></head>
+<body>
+<div class="parent"><p class="child">Child</p></div>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let parent_box = &body_box.children[0];
+
+        // Parent margin collapses with child's: max(10, 20) = 20.
+        assert_eq!(parent_box.margin.top, 20.0);
+    }
+
+    #[test]
+    fn negative_margin_collapsing() {
+        // One positive (20) and one negative (-10): collapsed = 20 + (-10) = 10.
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+.first { margin-top: 0; margin-bottom: 20px; }
+.second { margin-top: -10px; margin-bottom: 0; }
+</style></head>
+<body>
+<p class="first">First</p>
+<p class="second">Second</p>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let first = &body_box.children[0];
+        let second = &body_box.children[1];
+
+        let first_bottom =
+            first.rect.y + first.rect.height + first.padding.bottom + first.border.bottom;
+        let gap = second.rect.y - second.border.top - second.padding.top - first_bottom;
+        // 20 + (-10) = 10
+        assert!(
+            (gap - 10.0).abs() < 1.0,
+            "positive + negative margin collapse should be 10, got {gap}"
+        );
+    }
+
+    #[test]
+    fn both_negative_margins_collapse() {
+        // Both negative: use the more negative value.
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+.first { margin-top: 0; margin-bottom: -10px; }
+.second { margin-top: -20px; margin-bottom: 0; }
+</style></head>
+<body>
+<p class="first">First</p>
+<p class="second">Second</p>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let first = &body_box.children[0];
+        let second = &body_box.children[1];
+
+        let first_bottom =
+            first.rect.y + first.rect.height + first.padding.bottom + first.border.bottom;
+        let gap = second.rect.y - second.border.top - second.padding.top - first_bottom;
+        // Both negative: min(-10, -20) = -20
+        assert!(
+            (gap - (-20.0)).abs() < 1.0,
+            "both-negative margin collapse should be -20, got {gap}"
+        );
+    }
+
+    #[test]
+    fn border_blocks_margin_collapsing() {
+        // When border separates margins, they don't collapse.
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+.first { margin-top: 0; margin-bottom: 20px; border-bottom: 1px solid black; }
+.second { margin-top: 20px; border-top: 1px solid black; }
+</style></head>
+<body>
+<p class="first">First</p>
+<p class="second">Second</p>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let first = &body_box.children[0];
+        let second = &body_box.children[1];
+
+        // Borders are on the elements themselves, but the MARGINS are still
+        // between the border boxes — sibling margins still collapse regardless
+        // of borders on the elements. The margin gap = max(20, 20) = 20.
+        let first_bottom =
+            first.rect.y + first.rect.height + first.padding.bottom + first.border.bottom;
+        let gap = second.rect.y - second.border.top - second.padding.top - first_bottom;
+        assert!(
+            (gap - 20.0).abs() < 1.0,
+            "sibling margins collapse even with borders on elements, gap should be 20, got {gap}"
+        );
+    }
+
+    #[test]
+    fn padding_blocks_parent_child_collapsing() {
+        // Parent with padding-top prevents margin collapsing with first child.
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+.parent { margin-top: 10px; padding-top: 5px; }
+.child { margin-top: 20px; }
+</style></head>
+<body>
+<div class="parent"><p class="child">Child</p></div>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let parent_box = &body_box.children[0];
+
+        // Parent has padding-top, so no collapsing: margin stays at 10.
+        assert_eq!(parent_box.margin.top, 10.0);
+    }
+
+    #[test]
+    fn empty_block_margins_collapse() {
+        // An empty div's top and bottom margins collapse with adjacent margins.
+        let html_str = r#"<!DOCTYPE html>
+<html>
+<head><style>
+body { margin: 0; border-top: 1px solid black; }
+.spacer { margin-top: 10px; margin-bottom: 10px; }
+p { margin-top: 5px; margin-bottom: 5px; }
+</style></head>
+<body>
+<p>Before</p>
+<div class="spacer"></div>
+<p>After</p>
+</body>
+</html>"#;
+        let doc = we_html::parse_html(html_str);
+        let font = test_font();
+        let sheets = extract_stylesheets(&doc);
+        let styled = resolve_styles(&doc, &sheets).unwrap();
+        let tree = layout(&styled, &doc, 800.0, 600.0, &font, &HashMap::new());
+
+        let body_box = &tree.root.children[0];
+        let before = &body_box.children[0];
+        let after = &body_box.children[2]; // [0]=p, [1]=empty div, [2]=p
+
+        // Empty div's margins (10+10) self-collapse to max(10,10)=10.
+        // Then collapse with before's bottom (5) and after's top (5):
+        // collapse(5, collapse(10, 10)) = collapse(5, 10) = 10
+        // Then collapse(10, 5) = 10.
+        // So total gap between before and after = 10.
+        let before_bottom =
+            before.rect.y + before.rect.height + before.padding.bottom + before.border.bottom;
+        let gap = after.rect.y - after.border.top - after.padding.top - before_bottom;
+        assert!(
+            (gap - 10.0).abs() < 1.0,
+            "empty block margin collapse gap should be ~10px, got {gap}"
+        );
+    }
+
+    #[test]
+    fn collapse_margins_unit() {
+        // Unit tests for the collapse_margins helper.
+        assert_eq!(collapse_margins(10.0, 20.0), 20.0);
+        assert_eq!(collapse_margins(20.0, 10.0), 20.0);
+        assert_eq!(collapse_margins(0.0, 15.0), 15.0);
+        assert_eq!(collapse_margins(-5.0, -10.0), -10.0);
+        assert_eq!(collapse_margins(20.0, -5.0), 15.0);
+        assert_eq!(collapse_margins(-5.0, 20.0), 15.0);
+        assert_eq!(collapse_margins(0.0, 0.0), 0.0);
+    }
 }
-- 
2.51.2