From b165440f5989a1c888155d6561aa0bd6fd7407e9 Mon Sep 17 00:00:00 2001 From: cadons Date: Sun, 16 Aug 2026 18:31:48 +0200 Subject: [PATCH 1/4] feat(loom): warn when explicit table column widths exceed page width Mirrors the existing vertical page-bottom overflow warning (#71): a table whose explicit column widths sum wider than the available width previously overflowed the page margin silently. resolve_table_column_widths already computes available_width and the fixed-column shortfall, so detect the overage there and log it instead of just clamping it away. Co-Authored-By: Claude Sonnet 5 --- .../pipeline/docraft_loom_layout_processor.cc | 9 +++ .../docraft_loom_layout_processor_test.cc | 57 +++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc b/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc index b08be78..49432c6 100644 --- a/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc +++ b/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc @@ -650,6 +650,15 @@ namespace docraft::loom::pipeline { remaining -= resolved[static_cast(c)]; } } + if (remaining < 0.0F) + { + const std::string node_label = + table.name().empty() ? std::string{"A table"} : fmt::format("Table '{}'", table.name()); + LOG_WARNING(fmt::format( + "{} has explicit column widths summing to {:.1f}pt, wider than the {:.1f}pt available " + "on the page -- it overflows past the page margin.", + node_label, available_width - remaining, available_width)); + } remaining = std::max(0.0F, remaining); // 2) Split `remaining` among the flexible columns by weight. total_weight diff --git a/docraft/test/docraft/loom/pipeline/docraft_loom_layout_processor_test.cc b/docraft/test/docraft/loom/pipeline/docraft_loom_layout_processor_test.cc index e7efb82..58e7d76 100644 --- a/docraft/test/docraft/loom/pipeline/docraft_loom_layout_processor_test.cc +++ b/docraft/test/docraft/loom/pipeline/docraft_loom_layout_processor_test.cc @@ -165,6 +165,63 @@ namespace docraft::test { EXPECT_NEAR(w1, w2, 1.0F); } + // Companion to the vertical page-bottom overflow warning (#71): when a table's + // explicit column widths sum wider than the available width, it silently + // overflows past the page margin (bug found while validating #81's flexible-column + // fix). Mirrors DocraftLoomPaginationProcessorTest.OversizedNonTableNodeLogsWarning's + // stdout-capture approach. + TEST(DocraftLoomLayoutProcessorTest, ExplicitColumnWidthsWiderThanAvailableWidthLogsWarning) + { + auto table = std::make_shared(); + table->set_name("wide_table"); + table->set_padding(0.0F); + + std::vector> row; + for (int i = 0; i < 3; ++i) + { + auto cell = std::make_shared(); + cell->set_explicit_width(300.0F); + cell->set_content(std::make_shared("x")); + cell->edit_layout_box().measured_size = {.width = 300.0F, .height = 10.0F}; + row.push_back(cell); + } + table->add_row(row); + + // 900pt of explicit column width against a 200pt page -- must overflow. + loom::pipeline::DocraftLoomLayoutProcessor processor(200.0F); + testing::internal::CaptureStdout(); + table->accept(processor); + const std::string stdout_log = testing::internal::GetCapturedStdout(); + + EXPECT_NE(stdout_log.find("[WARNING]"), std::string::npos); + EXPECT_NE(stdout_log.find("wide_table"), std::string::npos); + } + + TEST(DocraftLoomLayoutProcessorTest, ExplicitColumnWidthsWithinAvailableWidthLogsNoWarning) + { + auto table = std::make_shared(); + table->set_name("narrow_table"); + table->set_padding(0.0F); + + std::vector> row; + for (int i = 0; i < 2; ++i) + { + auto cell = std::make_shared(); + cell->set_explicit_width(50.0F); + cell->set_content(std::make_shared("x")); + cell->edit_layout_box().measured_size = {.width = 50.0F, .height = 10.0F}; + row.push_back(cell); + } + table->add_row(row); + + loom::pipeline::DocraftLoomLayoutProcessor processor(200.0F); + testing::internal::CaptureStdout(); + table->accept(processor); + const std::string stdout_log = testing::internal::GetCapturedStdout(); + + EXPECT_EQ(stdout_log.find("[WARNING]"), std::string::npos); + } + // Review bug #8 (a): table cell content in absolute position mode must be // rejected the same way the cell itself already is -- resolving it to a raw // page coordinate silently ignores the cell's own layout entirely. From b431005e81418affeb1c5d90996022c93f0101cf Mon Sep 17 00:00:00 2001 From: cadons Date: Sun, 16 Aug 2026 18:38:56 +0200 Subject: [PATCH 2/4] fix(loom): stop double-subtracting cell padding from table width kCellPaddingX is a per-cell content inset already folded into every cell's own measured_size (and therefore into each column's natural width), but resolve_table_column_widths (and the matching wrap-budget estimate in Measure) additionally subtracted 2*kCellPaddingX from available_width as if it were a table-wide margin like table.padding(). That extra 5pt was never reintroduced anywhere, so a width-constrained table always rendered 5pt narrower than its available width -- and since the table is positioned flush against the container's leading edge, the missing width showed up entirely as a gap on the table's trailing (right) edge. Co-Authored-By: Claude Sonnet 5 --- .../pipeline/docraft_loom_layout_processor.cc | 8 +++-- .../docraft_loom_measure_processor.cc | 6 ++-- .../loom/nodes/docraft_loom_table_test.cc | 35 ++++++++++--------- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc b/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc index 49432c6..26b5986 100644 --- a/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc +++ b/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc @@ -622,9 +622,13 @@ namespace docraft::loom::pipeline { float sum_natural = 0.0F; for (float w : geometry.natural_widths) sum_natural += w; + // kCellPaddingX is a per-cell content inset -- Measure already folds it into every + // cell's own measured_size (and therefore into geometry.natural_widths), so it must + // NOT also be subtracted here as if it were a table-wide margin like table.padding(): + // doing so shrank the table by a flat 2*kCellPaddingX with nothing to reclaim it, + // leaving a gap on the table's trailing edge. const float available_width = incoming_width > 0.0F - ? incoming_width - (2.0F * nodes::DocraftLoomTable::kCellPaddingX) - - (2.0F * table.padding()) + ? incoming_width - (2.0F * table.padding()) : sum_natural; // A column is "fixed" if the author gave it an explicit width, "flexible" diff --git a/docraft/src/docraft/loom/pipeline/docraft_loom_measure_processor.cc b/docraft/src/docraft/loom/pipeline/docraft_loom_measure_processor.cc index 7434ba1..f640291 100644 --- a/docraft/src/docraft/loom/pipeline/docraft_loom_measure_processor.cc +++ b/docraft/src/docraft/loom/pipeline/docraft_loom_measure_processor.cc @@ -503,8 +503,10 @@ namespace docraft::loom::pipeline { // the natural-width-floor sizing of cells that already fit. std::vector column_wrap_budget(static_cast(cols), 0.0F); if (cols > 0 && incoming_width > 0.0F) { - const float available_width = - incoming_width - (2.0F * nodes::DocraftLoomTable::kCellPaddingX) - (2.0F * table->padding()); + // See the matching comment in DocraftLoomLayoutProcessor::resolve_table_column_widths: + // kCellPaddingX is a per-cell inset, already reflected in each cell's own + // measured_size -- it must not also be subtracted here as a table-wide margin. + const float available_width = incoming_width - (2.0F * table->padding()); if (available_width > 0.0F) { const auto shares = distribute_weighted_amounts(available_width, table->column_weights(), cols); for (int c = 0; c < cols; ++c) { diff --git a/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc b/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc index ea0c7c9..4180095 100644 --- a/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc +++ b/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc @@ -51,9 +51,11 @@ namespace docraft::test { loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); table->accept(layout); - // available_width = 200 - 2*2.5 = 195, split evenly -> ~97.5 each (floored at natural 10) - EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.width, 97.5F); - EXPECT_FLOAT_EQ(table->cell(0, 1)->layout_box().frame.size.width, 97.5F); + // available_width = 200 (table padding=0; kCellPaddingX is a per-cell inset already + // folded into each cell's own measured_size, not a table-wide margin), split evenly + // -> 100 each (floored at natural 10) + EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.width, 100.0F); + EXPECT_FLOAT_EQ(table->cell(0, 1)->layout_box().frame.size.width, 100.0F); } TEST_F(DocraftLoomTableTest, ExplicitWidthIsRespectedAndRemainderRedistributed) @@ -73,11 +75,11 @@ namespace docraft::test { EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.width, 40.0F); // column 1 (the only flexible column) gets everything available_width has left - // over after column 0's explicit width is deducted (195 - 40 = 155), not a share + // over after column 0's explicit width is deducted (200 - 40 = 160), not a share // of the full available_width diluted by column 0 -- otherwise the table's total - // resolved width would exceed available_width (40 + 97.5 = 137.5 happened to fit + // resolved width would exceed available_width (40 + 100 = 140 happened to fit // here, but the same dilution overflows the margin with more columns/less slack). - EXPECT_FLOAT_EQ(table->cell(0, 1)->layout_box().frame.size.width, 155.0F); + EXPECT_FLOAT_EQ(table->cell(0, 1)->layout_box().frame.size.width, 160.0F); } TEST_F(DocraftLoomTableTest, NaturalWidthFloorIsRespectedWhenNoRescaleIsNeeded) @@ -100,14 +102,14 @@ namespace docraft::test { table->add_row({make_cell("a", true), make_cell("b", true)}); table->accept(*measure_); - // available_width = 200 - 2*2.5 = 195; weight share = 97.5 each, which already + // available_width = 200 (table padding=0); weight share = 100 each, which already // exceeds the natural width of 75, so the floor never has to compete with the // rescale here -- both columns simply get their even weight-based share. loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); table->accept(layout); - EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.width, 97.5F); - EXPECT_FLOAT_EQ(table->cell(0, 1)->layout_box().frame.size.width, 97.5F); + EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.width, 100.0F); + EXPECT_FLOAT_EQ(table->cell(0, 1)->layout_box().frame.size.width, 100.0F); } TEST_F(DocraftLoomTableTest, NarrowPageRescalesColumnsProportionally) @@ -132,10 +134,10 @@ namespace docraft::test { table->accept(layout); // Cell natural widths include the automatic content padding (2*2.5 = 5), so - // natural widths are 150+5=155 and 10+5=15. available_width = 20 - 5 = 15; - // scale = 15 / (155+15) = 15/170. - EXPECT_NEAR(table->cell(0, 0)->layout_box().frame.size.width, 155.0F * 15.0F / 170.0F, 0.001F); - EXPECT_NEAR(table->cell(0, 1)->layout_box().frame.size.width, 15.0F * 15.0F / 170.0F, 0.001F); + // natural widths are 150+5=155 and 10+5=15. available_width = 20 (table padding=0); + // scale = 20 / (155+15) = 20/170. + EXPECT_NEAR(table->cell(0, 0)->layout_box().frame.size.width, 155.0F * 20.0F / 170.0F, 0.001F); + EXPECT_NEAR(table->cell(0, 1)->layout_box().frame.size.width, 15.0F * 20.0F / 170.0F, 0.001F); } TEST_F(DocraftLoomTableTest, RowHeightIsTallestCellPlusPadding) @@ -225,10 +227,11 @@ namespace docraft::test { auto text = std::dynamic_pointer_cast(table->cell(0, 0)->content()); ASSERT_TRUE(text); - // available_width = 20 - 2*2.5 = 15; single column -> budget = 15 - 2*2.5 = 10. - EXPECT_FLOAT_EQ(text->wrap_width(), 10.0F); + // available_width = 20 (table padding=0); single column -> share = 20; + // budget = share - 2*kCellPaddingX = 20 - 5 = 15. + EXPECT_FLOAT_EQ(text->wrap_width(), 15.0F); EXPECT_GT(text->wrapped_lines().size(), 1U); - EXPECT_FLOAT_EQ(text->layout_box().measured_size.width, 10.0F); + EXPECT_FLOAT_EQ(text->layout_box().measured_size.width, 15.0F); } TEST_F(DocraftLoomTableTest, ShortCellTextKeepsNaturalWidthEvenWithAColumnBudget) From b8e09ec9c153d75f66f1cf9c509912612117dcdf Mon Sep 17 00:00:00 2001 From: cadons Date: Sun, 16 Aug 2026 18:45:16 +0200 Subject: [PATCH 3/4] fix(loom): set default table padding to zero for flush alignment --- docraft/include/docraft/loom/nodes/docraft_loom_table.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docraft/include/docraft/loom/nodes/docraft_loom_table.h b/docraft/include/docraft/loom/nodes/docraft_loom_table.h index 2cb919a..97d8015 100644 --- a/docraft/include/docraft/loom/nodes/docraft_loom_table.h +++ b/docraft/include/docraft/loom/nodes/docraft_loom_table.h @@ -155,8 +155,9 @@ namespace docraft::loom::nodes { std::vector column_weights_; std::optional default_cell_background_; float baseline_offset_ = 0.25F; - // Light default so a Table never sits flush against surrounding content out of - // the box; still fully overridable via the `padding` attribute. - float padding_ = 6.0F; + // No padding by default -- a Table's grid/border sits flush with whatever width + // its container hands it (e.g. the body's margins), matching every other node's + // default. Still fully overridable via the `padding` attribute. + float padding_ = 0.0F; }; } // docraft \ No newline at end of file From ac095ef1c0ddf07d804c77c41c4cbe2d86f6e437 Mon Sep 17 00:00:00 2001 From: cadons Date: Sun, 16 Aug 2026 19:33:51 +0200 Subject: [PATCH 4/4] test(loom): refactor tests to use expect_uniform_text_metrics for consistency --- .../loom/nodes/docraft_loom_table_test.cc | 76 +++++++++---------- 1 file changed, 36 insertions(+), 40 deletions(-) diff --git a/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc b/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc index 4180095..c7a45cc 100644 --- a/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc +++ b/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc @@ -34,21 +34,38 @@ namespace docraft::test { return cell; } + // The width/height pair most tests below mock the text backend with -- kept as + // one call so a test only states the two numbers that actually matter to it, + // not the boilerplate EXPECT_CALL wiring around them. + void expect_uniform_text_metrics(float width, float height = 10.0F) + { + EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(width)); + EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(height)); + } + + // Runs Measure over `table`, then hands back a not-yet-accepted Layout processor + // for `page_width` -- callers that don't need anything special between the two + // passes just do `table->accept(prepare_layout(*table, W));`; callers that do + // (e.g. reset_cursor()) call that on the returned processor first. + loom::pipeline::DocraftLoomLayoutProcessor prepare_layout(loom::nodes::DocraftLoomTable& table, + float page_width) + { + table.accept(*measure_); + return loom::pipeline::DocraftLoomLayoutProcessor(page_width); + } + std::shared_ptr text_backend_; std::unique_ptr measure_; }; TEST_F(DocraftLoomTableTest, EqualWeightColumnsDistributeEvenly) { - EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(10.0F)); - EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F)); + expect_uniform_text_metrics(10.0F); auto table = std::make_shared(); table->set_padding(0.0F); // isolate column-width math from the table's own outer padding table->add_row({make_cell("a", true), make_cell("b", true)}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); + auto layout = prepare_layout(*table, 200.0F); table->accept(layout); // available_width = 200 (table padding=0; kCellPaddingX is a per-cell inset already @@ -60,17 +77,14 @@ namespace docraft::test { TEST_F(DocraftLoomTableTest, ExplicitWidthIsRespectedAndRemainderRedistributed) { - EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(10.0F)); - EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F)); + expect_uniform_text_metrics(10.0F); auto table = std::make_shared(); table->set_padding(0.0F); // isolate column-width math from the table's own outer padding auto c0 = make_cell("a", true); c0->set_explicit_width(40.0F); table->add_row({c0, make_cell("b", true)}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); + auto layout = prepare_layout(*table, 200.0F); table->accept(layout); EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.width, 40.0F); @@ -94,18 +108,16 @@ namespace docraft::test { // shrunk back down along with everything else -- the floor is not an absolute // per-column guarantee once a rescale is triggered, only a preference that // holds when it doesn't need to fight the rescale. - EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(75.0F)); - EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F)); + expect_uniform_text_metrics(75.0F); auto table = std::make_shared(); table->set_padding(0.0F); // isolate column-width math from the table's own outer padding table->add_row({make_cell("a", true), make_cell("b", true)}); - table->accept(*measure_); // available_width = 200 (table padding=0); weight share = 100 each, which already // exceeds the natural width of 75, so the floor never has to compete with the // rescale here -- both columns simply get their even weight-based share. - loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); + auto layout = prepare_layout(*table, 200.0F); table->accept(layout); EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.width, 100.0F); @@ -128,9 +140,7 @@ namespace docraft::test { auto table = std::make_shared(); table->set_padding(0.0F); // isolate column-width math from the table's own outer padding table->add_row({make_cell("very long title text", true), make_cell("b", true)}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(20.0F); // deliberately too narrow + auto layout = prepare_layout(*table, 20.0F); // deliberately too narrow table->accept(layout); // Cell natural widths include the automatic content padding (2*2.5 = 5), so @@ -149,9 +159,7 @@ namespace docraft::test { auto table = std::make_shared(); table->add_row({make_cell("a"), make_cell("b")}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); + auto layout = prepare_layout(*table, 200.0F); table->accept(layout); EXPECT_FLOAT_EQ(table->cell(0, 0)->layout_box().frame.size.height, 35.0F); // 30 + 2*2.5 @@ -159,14 +167,11 @@ namespace docraft::test { TEST_F(DocraftLoomTableTest, LayoutDoesNotOverlapColumns) { - EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(10.0F)); - EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F)); + expect_uniform_text_metrics(10.0F); auto table = std::make_shared(); table->add_row({make_cell("a"), make_cell("b"), make_cell("c")}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(300.0F); + auto layout = prepare_layout(*table, 300.0F); table->accept(layout); const float col0_right = table->cell(0, 0)->layout_box().frame.position.x @@ -184,15 +189,12 @@ namespace docraft::test { // away from wherever the parent positioned this table -- not just inset cell // content within an unmoved border, which would leave the table sitting flush // against whatever precedes it. - EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(10.0F)); - EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F)); + expect_uniform_text_metrics(10.0F); auto table = std::make_shared(); table->set_padding(6.0F); table->add_row({make_cell("a"), make_cell("b")}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); + auto layout = prepare_layout(*table, 200.0F); layout.reset_cursor(10.0F, 20.0F); table->accept(layout); @@ -265,14 +267,11 @@ namespace docraft::test { TEST_F(DocraftLoomTableTest, FrameSizeMatchesMeasuredSizeForTableAndCells) { - EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(10.0F)); - EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F)); + expect_uniform_text_metrics(10.0F); auto table = std::make_shared(); table->add_row({make_cell("a"), make_cell("b")}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); + auto layout = prepare_layout(*table, 200.0F); table->accept(layout); EXPECT_GT(table->layout_box().frame.size.width, 0.0F); @@ -283,15 +282,12 @@ namespace docraft::test { TEST_F(DocraftLoomTableTest, RowTitleGridProducesCorrectLayout) { // "vertical"-style usage: column 0 holds row labels (is_title), column 1 holds values. - EXPECT_CALL(*text_backend_, measure_text_width(_, _, _)).WillRepeatedly(Return(10.0F)); - EXPECT_CALL(*text_backend_, measure_text_height(_, _)).WillRepeatedly(Return(10.0F)); + expect_uniform_text_metrics(10.0F); auto table = std::make_shared(); table->add_row({make_cell("Name", true), make_cell("Alice")}); table->add_row({make_cell("Age", true), make_cell("30")}); - table->accept(*measure_); - - loom::pipeline::DocraftLoomLayoutProcessor layout(200.0F); + auto layout = prepare_layout(*table, 200.0F); table->accept(layout); EXPECT_FLOAT_EQ(table->cell(1, 0)->layout_box().frame.position.y,