fix(loom): fix margin collapsing, image weight, and PDF save errors

.claude/skills/craft-language/SKILL.md

@@ -64,8 +64,8 @@ offending attribute and what the tag actually accepts.
 | `x`, `y` | float | Meaningful only when `position="absolute"` (or implied — see below). |
 | `width`, `height` | float | Only has effect on node types that expose a setter for it (Rectangle, Canvas, Image, Text `width`=wrap width, Blank `height`). See `references/gotchas.md` #7 for which tags silently ignore it. |
 | `padding` | float | Inset between a container's box and its children. No-op on leaf nodes. |
-| `margin` | float | Shorthand for all 4 edges — space a node asks its parent's stack to reserve around it. Adjacent siblings' touching margins are combined via **max()**, not summed (CSS-style collapsing). |
-| `margin_top`/`right`/`bottom`/`left` | float | Per-edge override; combines with `margin` (per-edge wins where set). |
+| `margin` | float | Shorthand for all 4 edges — space a node asks its parent's stack to reserve around it. Adjacent siblings' touching margins are combined via true CSS-style collapsing: `max(positive requests) + min(negative requests)` — so a lone negative margin still pulls siblings together instead of being dropped. |
+| `margin_top`/`right`/`bottom`/`left` | float | Per-edge override; combines with `margin` (per-edge wins where set). The two edges *along* the stack's own axis (top/bottom in a vertical `<Layout>`, left/right in a horizontal one) participate in the collapsing above. The two **cross-axis** edges (left/right in a vertical Layout, top/bottom in a horizontal one) have no sibling to collapse against — each is honored as a plain per-child offset within that child's own slot, and the container grows to still fit it. |
 | `position` | enum: `block`, `absolute` | `block` (default) = normal flow, advances the parent's cursor. `absolute` = positioned at `x`/`y`, does not advance the cursor. **If `x` and/or `y` is given and `position` is omitted, absolute is implied.** |
 | `z_index` | int | Default `0`. Sibling-scoped paint order only — does **not** affect layout/flow, only which sibling paints on top. Higher paints later (on top). Stable-sorted, so equal `z_index` siblings keep declaration order. Scope is one container's direct children only (not recursive into grandchildren, not cross-container). |
 | `visible` | bool (`true`/`false`) | If `false`, the whole element and its subtree are dropped at build time — not just hidden, genuinely not built. Not `${...}`-templatable. |

.claude/skills/craft-language/references/nodes.md

@@ -345,6 +345,11 @@ an explicit `height="..."` — a VStack's height is otherwise derived bottom-up
 ambient "page height" budget the way HStack always has a page width to divide), so per-child
 `weight` alone on a heightless vertical layout is silently dropped.
 
+A weighted child is stretched to fill its resolved column/row share — **except an `<Image>` that
+declares its own `width`** (see below), which keeps that declared width undistorted; everything
+else (a `<Rectangle>` used as a column background, plain text, etc.) still fills the slot, which is
+the point of `weight` for those.
+
 ```xml
 <Layout orientation="horizontal" spacing="8">
   <Text weight="1">Narrow</Text>
@@ -370,6 +375,11 @@ ambient "page height" budget the way HStack always has a page width to divide),
 `src` and `data` together throws. Image format derives from `src`'s extension first letter:
 `p`/`P` → PNG, `j`/`J` → JPEG, else raw.
 
+A declared `width` survives even inside a weighted `<Layout orientation="horizontal">` column —
+the image is not force-stretched to the column's resolved width the way other weighted children
+are (that used to distort it, widening only its width and leaving height untouched). Omit `width`
+if you *do* want the image to fill its weighted slot.
+
 ```xml
 <Image src="assets/logo.png" width="50" height="50" />
 <Image src="${logo_path}" />

docraft/include/docraft/loom/nodes/docraft_loom_image.h

@@ -43,6 +43,8 @@ namespace docraft::loom::nodes {
         float height() const;
         void set_height(float height);
 
+        bool keeps_own_size_in_weighted_slot() const override;
+
     private:
         std::string path_;
         ImageFormat format_ = ImageFormat::kPng;

docraft/include/docraft/loom/nodes/docraft_loom_node.h

@@ -278,6 +278,20 @@ namespace docraft::loom::nodes {
         void set_margin(float margin);
         void set_margin(float top, float right, float bottom, float left);
 
+        /**
+         * @brief Whether this node's own declared size should survive being placed
+         * into a weighted stacking layout's (HStack/VStack) resolved slot, instead
+         * of being stretched to fill it. Default false -- filling the slot is the
+         * whole point of weight() for most node types (a Rectangle used as a
+         * column background, plain text, a nested container). DocraftLoomImage
+         * overrides this when it has an explicit width(): stretching an image
+         * along only one axis (its own height() is left untouched) distorts it.
+         * A plain virtual here lets the layout processor ask any child without a
+         * dynamic_cast/type-check chain -- a future node type with the same
+         * concern just overrides this, no processor edits required.
+         */
+        virtual bool keeps_own_size_in_weighted_slot() const { return false; }
+
     private:
         std::vector<std::shared_ptr<DocraftLoomNode>> children_;
         LayoutBox layout_box_ = {};

docraft/src/docraft/backend/pdf/docraft_haru_output_backend.cc

@@ -18,6 +18,8 @@
 
 #include <hpdf.h>
 
+#include <fmt/format.h>
+
 #include "docraft/exception/docraft_exceptions.h"
 
 namespace docraft::backend::pdf {
@@ -30,7 +32,12 @@ namespace docraft::backend::pdf {
         if (!pdf) {
             throw docraft::exception::BackendStateException("Haru document is not initialized");
         }
-        HPDF_SaveToFile(pdf, path.c_str());
+        const HPDF_STATUS status = HPDF_SaveToFile(pdf, path.c_str());
+        if (status != HPDF_OK) {
+            HPDF_ResetError(pdf);
+            throw docraft::exception::RenderingFailedException(
+                fmt::format("Failed to save PDF to output file '{}' (HPDF error_no={:#x})", path, status));
+        }
     }
 
     std::string DocraftHaruOutputBackend::file_extension() const {

docraft/src/docraft/loom/nodes/docraft_loom_image.cc

@@ -84,4 +84,9 @@ namespace docraft::loom::nodes {
     {
         requested_height_ = height;
     }
+
+    bool DocraftLoomImage::keeps_own_size_in_weighted_slot() const
+    {
+        return width() > 0.0F;
+    }
 } // docraft

docraft/src/docraft/loom/nodes/docraft_loom_layout_container.cc

@@ -22,8 +22,19 @@ namespace docraft::loom::nodes {
 
     float DocraftLoomLayoutContainer::resolve_child_gap(float container_spacing, float margin_a, float margin_b)
     {
-        const float requested_margin = std::max(margin_a, margin_b);
-        return requested_margin > 0.0F ? requested_margin : container_spacing;
+        // Neither neighbor asked for a margin on this touching edge -- the container's
+        // own spacing (which may itself be negative) is what fills the gap.
+        if (margin_a == 0.0F && margin_b == 0.0F)
+        {
+            return container_spacing;
+        }
+        // CSS-style margin collapsing: the combined gap is the larger of the two
+        // positive requests plus the smaller (most negative) of the two negative
+        // requests -- so a lone negative margin still pulls its neighbors together
+        // instead of being discarded the way a plain max() would drop it.
+        const float positive = std::max({margin_a, margin_b, 0.0F});
+        const float negative = std::min({margin_a, margin_b, 0.0F});
+        return positive + negative;
     }
 
     float DocraftLoomLayoutContainer::resolve_outer_margin(const DocraftLoomNode& node, bool leading)

docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc

@@ -281,8 +281,11 @@ namespace docraft::loom::pipeline {
         for (int i = 0; i < n; ++i)
         {
             inherited_width_ = relay_width;
-            cursor_.set_position(start_x, current_y);
             auto child = node->edit_child(i);
+            // Cross-axis margin (left/right): no sibling shares this axis to collapse
+            // against, so it's a plain per-child offset within the column, mirroring
+            // how HStack honors margin_top/bottom below.
+            cursor_.set_position(start_x + child->margin().left, current_y);
             child->accept(*this);
             float advance = child->layout_box().measured_size.height;
             if (!resolved_heights.empty())
@@ -386,14 +389,26 @@ namespace docraft::loom::pipeline {
             {
                 inherited_width_ = resolved_widths[static_cast<std::size_t>(i)];
             }
-            cursor_.set_position(current_x, start_y);
             auto child = node->edit_child(i);
+            // Cross-axis margin (top/bottom): no sibling shares this axis to collapse
+            // against, so it's a plain per-child offset within the row, mirroring how
+            // VStack honors margin_left/right above.
+            cursor_.set_position(current_x, start_y + child->margin().top);
             child->accept(*this);
             float advance = child->layout_box().measured_size.width;
             if (!resolved_widths.empty())
             {
                 advance = resolved_widths[static_cast<std::size_t>(i)];
-                edit_frame(child->edit_layout_box()).size.width = advance;
+                // A child that opts out (e.g. an Image with its own declared width --
+                // see DocraftLoomNode::keeps_own_size_in_weighted_slot()) keeps its own
+                // width instead of being stretched to the resolved slot, which would
+                // distort it (only this axis gets overwritten, height is left alone).
+                // Every other child type still fills the slot, the whole point of
+                // weights() for them.
+                if (!child->keeps_own_size_in_weighted_slot())
+                {
+                    edit_frame(child->edit_layout_box()).size.width = advance;
+                }
             }
             current_x += advance;
             if (i < n - 1)

docraft/src/docraft/loom/pipeline/docraft_loom_measure_processor.cc

@@ -223,7 +223,10 @@ namespace docraft::loom::pipeline {
             if (i < n - 1) {
                 total_height += gaps[static_cast<std::size_t>(i)];
             }
-            max_width = std::max(max_width, sz.width);
+            // Cross-axis margins (left/right) have no sibling on this axis to collapse
+            // against, unlike top/bottom -- they're a plain per-child inset the column
+            // must widen to still fit, mirroring how HStack treats top/bottom below.
+            max_width = std::max(max_width, sz.width + child->margin().left + child->margin().right);
         }
         total_height += node->resolve_outer_margin(*node, /*leading=*/false);
         auto &ms = node->edit_layout_box().measured_size;
@@ -297,7 +300,10 @@ namespace docraft::loom::pipeline {
             if (i < n - 1) {
                 total_width += gaps[static_cast<std::size_t>(i)];
             }
-            max_height = std::max(max_height, sz.height);
+            // Cross-axis margins (top/bottom) have no sibling on this axis to collapse
+            // against, unlike left/right -- they're a plain per-child inset the row
+            // must grow to still fit, mirroring how VStack treats left/right above.
+            max_height = std::max(max_height, sz.height + child->margin().top + child->margin().bottom);
         }
         auto &ms = node->edit_layout_box().measured_size;
         ms.width = total_width + leading_margin + trailing_margin + (n > 0 ? (2.0F * padding) : 0.0F);

docraft/test/docraft/backend/docraft_haru_backend_test.cc

@@ -193,6 +193,20 @@ TEST_F(DocraftHaruBackendTest, SavesPdfToFile) {
     std::filesystem::remove(output_path);
 }
 
+// Bug #97: HPDF_SaveToFile's HPDF_STATUS was discarded, so a failed write (e.g.
+// the output path is unwritable) still logged success and returned normally --
+// docraft_tool exited 0 with the old file on disk untouched. save_to_file() must
+// now surface that failure instead of swallowing it.
+TEST_F(DocraftHaruBackendTest, ThrowsWhenOutputPathCannotBeWritten) {
+    ASSERT_NE(backend().output_backend(), nullptr);
+    // A path inside a directory that doesn't exist -- libharu's own file open
+    // fails the same way it does for a locked/inaccessible file (HPDF_FILE_OPEN_ERROR).
+    const std::string unwritable_path =
+        "/__docraft_nonexistent_directory__/out.pdf";
+    EXPECT_THROW(backend().output_backend()->save_to_file(unwritable_path),
+                 docraft::exception::RenderingFailedException);
+}
+
 TEST_F(DocraftHaruBackendTest, SavesPdfWithMetadataInfo) {
     DocraftDocumentMetadata metadata;
     metadata.set_title("Docraft Metadata Title");

docraft/test/docraft/loom/nodes/docraft_loom_stack_nodes_test.cc

@@ -666,4 +666,107 @@ namespace docraft::test {
         EXPECT_FLOAT_EQ(backend.draw_line_calls()[1].x2 - backend.draw_line_calls()[1].x1, 30.0F);
         EXPECT_FLOAT_EQ(backend.draw_line_calls()[1].y2 - backend.draw_line_calls()[1].y1, 0.0F);
     }
+
+    // ── Bug #99: cross-axis margins ──────────────────────────────────────────────
+    // margin_top/bottom on an HStack child, and margin_left/right on a VStack
+    // child, have no sibling on that axis to collapse against -- they used to be
+    // silently dropped instead of being honored as a per-child inset.
+
+    TEST_F(DocraftLoomStackNodesTest, HStack_HonorsChildMarginTopAsCrossAxisOffset)
+    {
+        EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(50.0F));
+        EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F));
+
+        auto hstack = std::make_shared<loom::nodes::DocraftLoomHStack>();
+        hstack->set_padding(0.0F);
+        auto plain = make_text("a");
+        auto offset = make_text("b");
+        offset->set_margin(10.0F, 0.0F, 0.0F, 0.0F); // top only
+        hstack->add_child(plain);
+        hstack->add_child(offset);
+
+        hstack->accept(*measure_);
+        hstack->accept(*layout_);
+
+        const float plain_y = plain->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.y;
+        const float offset_y = offset->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.y;
+        EXPECT_FLOAT_EQ(offset_y - plain_y, 10.0F);
+        // The row must grow to still fit the offset child instead of clipping it.
+        EXPECT_FLOAT_EQ(hstack->layout_box().measured_size.height, 20.0F); // 10 (text) + 10 (margin_top)
+    }
+
+    TEST_F(DocraftLoomStackNodesTest, VStack_HonorsChildMarginLeftAsCrossAxisOffset)
+    {
+        EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(50.0F));
+        EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F));
+
+        auto vstack = std::make_shared<loom::nodes::DocraftLoomVStack>();
+        vstack->set_padding(0.0F);
+        auto plain = make_text("a");
+        auto offset = make_text("b");
+        offset->set_margin(0.0F, 0.0F, 0.0F, 40.0F); // left only
+        vstack->add_child(plain);
+        vstack->add_child(offset);
+
+        vstack->accept(*measure_);
+        vstack->accept(*layout_);
+
+        const float plain_x = plain->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.x;
+        const float offset_x = offset->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.x;
+        EXPECT_FLOAT_EQ(offset_x - plain_x, 40.0F);
+        // The column must widen to still fit the offset child's full extent.
+        EXPECT_FLOAT_EQ(vstack->layout_box().measured_size.width, 90.0F); // 50 (text) + 40 (margin_left)
+    }
+
+    // ── Bug #100: negative margins between siblings ────────────────────────────
+    // resolve_child_gap() used to drop any margin <= 0, dropping the request in
+    // favor of the container's own spacing -- so a negative margin_top on a
+    // second-or-later child had no effect. CSS-style collapsing (max of positives
+    // plus min of negatives) must let it pull siblings together, exactly like a
+    // negative spacing() already does.
+
+    TEST_F(DocraftLoomStackNodesTest, VStack_NegativeMarginBetweenSiblingsPullsThemTogether)
+    {
+        EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(50.0F));
+        EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F));
+
+        auto vstack = std::make_shared<loom::nodes::DocraftLoomVStack>();
+        vstack->set_padding(0.0F);
+        vstack->set_spacing(0.0F);
+        auto first = make_text("a");
+        auto second = make_text("b");
+        second->set_margin(-5.0F, 0.0F, 0.0F, 0.0F); // margin_top="-5" on the second (non-first) child
+        vstack->add_child(first);
+        vstack->add_child(second);
+
+        vstack->accept(*measure_);
+        vstack->accept(*layout_);
+
+        const float first_y = first->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.y;
+        const float second_y = second->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.y;
+        // 10 (first's natural height) - 5 (negative margin pulls it up), not the old
+        // silently-dropped-to-spacing(0) result of 10.
+        EXPECT_FLOAT_EQ(second_y - first_y, 5.0F);
+    }
+
+    TEST_F(DocraftLoomStackNodesTest, VStack_NoMarginOnEitherSideFallsBackToContainerSpacing)
+    {
+        EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(50.0F));
+        EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F));
+
+        auto vstack = std::make_shared<loom::nodes::DocraftLoomVStack>();
+        vstack->set_padding(0.0F);
+        vstack->set_spacing(6.0F);
+        auto first = make_text("a");
+        auto second = make_text("b");
+        vstack->add_child(first);
+        vstack->add_child(second);
+
+        vstack->accept(*measure_);
+        vstack->accept(*layout_);
+
+        const float first_y = first->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.y;
+        const float second_y = second->layout_box().frame(docraft::test::utils::LayoutBoxTestAccess::make_layout_proof()).position.y;
+        EXPECT_FLOAT_EQ(second_y - first_y, 16.0F); // 10 (natural height) + 6 (spacing)
+    }
 } // namespace docraft::test