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 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 f12cdb3..8482221 100644 --- a/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc +++ b/docraft/src/docraft/loom/pipeline/docraft_loom_layout_processor.cc @@ -622,11 +622,34 @@ 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; + // Warn (rather than silently clip) when the author's own explicit column widths + // already exceed available_width before any flexible column even gets a share -- + // resolve_fixed_and_flexible_amounts() below still keeps fixed columns verbatim + // and clamps flexible ones to 0, so the table overflows the page margin visibly + // instead of crashing, but that's worth surfacing to the caller. + float explicit_total = 0.0F; + for (float w : geometry.explicit_widths) + if (w > 0.0F) + explicit_total += w; + if (explicit_total > available_width) + { + 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, explicit_total, available_width)); + } + // Fixed columns (explicit_widths[c] > 0) keep their own width verbatim and // reserve it out of available_width before flexible columns split what's left // by weight, floored at each flexible column's own natural width so content 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 50c4cea..24ed65b 100644 --- a/docraft/src/docraft/loom/pipeline/docraft_loom_measure_processor.cc +++ b/docraft/src/docraft/loom/pipeline/docraft_loom_measure_processor.cc @@ -526,10 +526,14 @@ namespace docraft::loom::pipeline { // explicit width, even when available_width below is 0 -- so a cell with its // own width() still gets a real wrap budget with no page/content width set at // all; only the flexible columns then get no budget (0), same as before. + // + // kCellPaddingX is a per-cell content inset, already reflected in each cell's + // own measured_size -- see the matching comment in + // DocraftLoomLayoutProcessor::resolve_table_column_widths -- so it must not + // also be subtracted here as a table-wide margin. float available_width = 0.0F; if (incoming_width > 0.0F) { - available_width = std::max( - 0.0F, incoming_width - (2.0F * nodes::DocraftLoomTable::kCellPaddingX) - (2.0F * table->padding())); + available_width = std::max(0.0F, incoming_width - (2.0F * table->padding())); } const auto resolved_widths = resolve_fixed_and_flexible_amounts({ .available_amount = available_width, 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 5ec0d02..c7a45cc 100644 --- a/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc +++ b/docraft/test/docraft/loom/nodes/docraft_loom_table_test.cc @@ -68,9 +68,11 @@ namespace docraft::test { auto layout = prepare_layout(*table, 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) @@ -87,11 +89,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) @@ -112,14 +114,14 @@ namespace docraft::test { 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)}); - // 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. auto layout = prepare_layout(*table, 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) @@ -142,10 +144,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) @@ -227,10 +229,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) 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.