From c9d2c7518cc4aed01f5c9911c15ff9b910bbaafe Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 20 Aug 2026 11:03:51 +0200 Subject: [PATCH 1/3] fix(odf): keep the rows a table grouping element holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ODF 1.2 §9.1.6/§9.1.7 lets a table's rows and columns sit inside grouping elements rather than directly under ``, and LibreOffice writes `` for every table with a repeating header row. The row scan asked pugixml for direct children only, so those rows were never visited and their cells never appeared — data loss, not a styling problem, in both the text and the spreadsheet path. `table_rows()` / `table_columns()` flatten a table's rows and columns across every grouping element ODF allows, recursing since the `*-group` variants nest, and the four scans in the parser and the two in the document adapter go through them. Whether a header row should additionally be marked as one for `` output is a separate question; this is about not losing it. Closes #707 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU --- CHANGELOG.md | 5 + CMakeLists.txt | 1 + src/odr/internal/odf/odf_document.cpp | 7 +- src/odr/internal/odf/odf_parser.cpp | 9 +- src/odr/internal/odf/odf_table.cpp | 54 ++++++ src/odr/internal/odf/odf_table.hpp | 23 +++ test/CMakeLists.txt | 2 + test/src/internal/odf/odf_table_test.cpp | 202 +++++++++++++++++++++++ 8 files changed, 296 insertions(+), 7 deletions(-) create mode 100644 src/odr/internal/odf/odf_table.cpp create mode 100644 src/odr/internal/odf/odf_table.hpp create mode 100644 test/src/internal/odf/odf_table_test.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ce82238f..ff4afb2db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,11 @@ The release run heads these entries with the version and opens a fresh - `wasm/README.md` says what Content-Security-Policy the rendered output needs, and what each directive is for. The failures are quiet: a pdf whose `data:` fonts are blocked renders as tofu rather than falling back. +- An odf table keeps the rows a grouping element holds: rows and columns wrapped + in ``, ``, + `` and their column counterparts were dropped from the + output entirely. Any table LibreOffice gave a repeating header row lost it, + in both text documents and spreadsheets. ## v6.9.0 - 2026-08-18 diff --git a/CMakeLists.txt b/CMakeLists.txt index 249c2fae7..04feb2197 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -163,6 +163,7 @@ set(ODR_SOURCE_FILES "src/odr/internal/odf/odf_meta.cpp" "src/odr/internal/odf/odf_parser.cpp" "src/odr/internal/odf/odf_style.cpp" + "src/odr/internal/odf/odf_table.cpp" "src/odr/internal/oldms/text/doc_document.cpp" "src/odr/internal/oldms/text/doc_element_registry.cpp" diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 14f76fecc..1660bd74a 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -448,7 +449,7 @@ class ElementAdapter final : public abstract::ElementAdapter, TableDimensions result; TableCursor cursor; - for (auto row : node.children("table:table-row")) { + for (const pugi::xml_node row : table_rows(node)) { const auto rows_repeated = row.attribute("table:number-rows-repeated").as_uint(1); cursor.add_row(rows_repeated); @@ -710,7 +711,7 @@ class ElementAdapter final : public abstract::ElementAdapter, TableDimensions result; TableCursor cursor; - for (auto column : node.children("table:table-column")) { + for (const pugi::xml_node column : table_columns(node)) { const auto columns_repeated = column.attribute("table:number-columns-repeated").as_uint(1); cursor.add_column(columns_repeated); @@ -719,7 +720,7 @@ class ElementAdapter final : public abstract::ElementAdapter, result.columns = cursor.column(); cursor = {}; - for (auto row : node.children("table:table-row")) { + for (const pugi::xml_node row : table_rows(node)) { const auto rows_repeated = row.attribute("table:number-rows-repeated").as_uint(1); cursor.add_row(rows_repeated); diff --git a/src/odr/internal/odf/odf_parser.cpp b/src/odr/internal/odf/odf_parser.cpp index 02d62a744..57f9c7ac7 100644 --- a/src/odr/internal/odf/odf_parser.cpp +++ b/src/odr/internal/odf/odf_parser.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -117,7 +118,7 @@ parse_table(ElementRegistry ®istry, const pugi::xml_node node) { // TODO inflate table first? - for (const pugi::xml_node column_node : node.children("table:table-column")) { + for (const pugi::xml_node column_node : table_columns(node)) { const std::uint32_t repeat = column_node.attribute("table:number-columns-repeated").as_uint(1); for (std::uint32_t i = 0; i < repeat; ++i) { @@ -127,7 +128,7 @@ parse_table(ElementRegistry ®istry, const pugi::xml_node node) { } } - for (const pugi::xml_node row_node : node.children("table:table-row")) { + for (const pugi::xml_node row_node : table_rows(node)) { // TODO log warning if repeated auto [row_id, _] = parse_any_element_tree(registry, row_node); registry.append_child(element_id, row_id); @@ -153,7 +154,7 @@ parse_sheet(ElementRegistry ®istry, const pugi::xml_node node) { TableCursor cursor; - for (const pugi::xml_node column_node : node.children("table:table-column")) { + for (const pugi::xml_node column_node : table_columns(node)) { const std::uint32_t columns_repeated = column_node.attribute("table:number-columns-repeated").as_uint(1); @@ -165,7 +166,7 @@ parse_sheet(ElementRegistry ®istry, const pugi::xml_node node) { sheet.dimensions.columns = cursor.column(); cursor = {}; - for (const pugi::xml_node row_node : node.children("table:table-row")) { + for (const pugi::xml_node row_node : table_rows(node)) { const std::uint32_t rows_repeated = row_node.attribute("table:number-rows-repeated").as_uint(1); diff --git a/src/odr/internal/odf/odf_table.cpp b/src/odr/internal/odf/odf_table.cpp new file mode 100644 index 000000000..ec9d54570 --- /dev/null +++ b/src/odr/internal/odf/odf_table.cpp @@ -0,0 +1,54 @@ +#include + +#include + +#include +#include +#include +#include + +namespace odr::internal::odf { + +namespace { + +constexpr std::array row_group_names{ + std::string_view("table:table-header-rows"), + std::string_view("table:table-rows"), + std::string_view("table:table-row-group"), +}; + +constexpr std::array column_group_names{ + std::string_view("table:table-header-columns"), + std::string_view("table:table-columns"), + std::string_view("table:table-column-group"), +}; + +void collect(const pugi::xml_node parent, const std::string_view name, + const std::span group_names, + std::vector &out) { + for (const pugi::xml_node child : parent.children()) { + const std::string_view child_name = child.name(); + if (child_name == name) { + out.push_back(child); + } else if (std::ranges::find(group_names, child_name) != + std::end(group_names)) { + collect(child, name, group_names, out); + } + } +} + +} // namespace + +std::vector odf::table_rows(const pugi::xml_node table) { + std::vector result; + collect(table, "table:table-row", row_group_names, result); + return result; +} + +std::vector odf::table_columns(const pugi::xml_node table) { + std::vector result; + collect(table, "table:table-column", column_group_names, result); + return result; +} + +} // namespace odr::internal::odf diff --git a/src/odr/internal/odf/odf_table.hpp b/src/odr/internal/odf/odf_table.hpp new file mode 100644 index 000000000..1b742ee3c --- /dev/null +++ b/src/odr/internal/odf/odf_table.hpp @@ -0,0 +1,23 @@ +#pragma once + +#include + +namespace pugi { +class xml_node; +} // namespace pugi + +namespace odr::internal::odf { + +/// The `` children of a table, in document order, including +/// those a grouping element holds — ``, +/// `` and ``, which nest +/// ([ODF 1.2] 9.1.7). +[[nodiscard]] std::vector table_rows(pugi::xml_node table); + +/// The `` children of a table, in document order, +/// including those a grouping element holds — ``, +/// `` and ``, which nest +/// ([ODF 1.2] 9.1.6). +[[nodiscard]] std::vector table_columns(pugi::xml_node table); + +} // namespace odr::internal::odf diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 09157ede6..8f9ac2866 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -59,6 +59,8 @@ add_executable(odr_test "src/internal/svg/svg_file_test.cpp" "src/internal/xml/xml_file_test.cpp" + "src/internal/odf/odf_table_test.cpp" + "src/internal/oldms/doc_test.cpp" "src/internal/oldms/ppt_test.cpp" "src/internal/oldms/xls_test.cpp" diff --git a/test/src/internal/odf/odf_table_test.cpp b/test/src/internal/odf/odf_table_test.cpp new file mode 100644 index 000000000..0ab33f8aa --- /dev/null +++ b/test/src/internal/odf/odf_table_test.cpp @@ -0,0 +1,202 @@ +#include + +#include +#include +#include +#include +#include + +#include +#include +#include + +#include + +#include + +#include +#include +#include +#include +#include + +using namespace odr; +using namespace odr::internal; +using namespace odr::internal::odf; + +namespace { + +std::vector names_of(const std::vector &nodes) { + std::vector result; + for (const pugi::xml_node node : nodes) { + result.emplace_back(node.attribute("id").value()); + } + return result; +} + +pugi::xml_node parse_table(pugi::xml_document &document, + const std::string &xml) { + EXPECT_TRUE(document.load_string(xml.c_str())); + return document.child("table:table"); +} + +/// An odf package is a zip with a mimetype and a `content.xml`; nothing else is +/// needed to open one, which keeps the input to these tests a string. +std::string write_odf(const std::string &name, const std::string &mimetype, + const std::string &body) { + const std::string content = + R"()" + R"()" + R"()" + + body + R"()"; + + const std::string path = (std::filesystem::current_path() / name).string(); + + zip::ZipArchive zip; + zip.insert_file(std::end(zip), RelPath("mimetype"), + std::make_shared(mimetype)); + zip.insert_file(std::end(zip), RelPath("content.xml"), + std::make_shared(content)); + std::ofstream out(path); + zip.save(out); + + return path; +} + +void collect_text(const Element element, std::vector &out) { + if (!element) { + return; + } + if (const Text text = element.as_text()) { + out.push_back(text.content()); + } + for (const Element child : element.children()) { + collect_text(child, out); + } +} + +/// An `Element` points into the document rather than holding it, so the +/// document has to outlive the walk. +std::vector text_of(const Document &document) { + std::vector result; + collect_text(document.root_element(), result); + return result; +} + +constexpr auto header_row_table = R"( + + + + HEADER_A + HEADER_B + + + + BODY_A + BODY_B + +)"; + +} // namespace + +TEST(OdfTable, rows_directly_under_the_table) { + pugi::xml_document document; + const pugi::xml_node table = parse_table(document, R"( + + + + )"); + + EXPECT_EQ(names_of(table_rows(table)), (std::vector{"a", "b"})); +} + +TEST(OdfTable, rows_inside_a_grouping_element) { + pugi::xml_document document; + const pugi::xml_node table = parse_table(document, R"( + + + + + + + + )"); + + EXPECT_EQ(names_of(table_rows(table)), + (std::vector{"header", "body"})); +} + +TEST(OdfTable, row_groups_nest) { + pugi::xml_document document; + const pugi::xml_node table = parse_table(document, R"( + + + + + + + + + + + )"); + + EXPECT_EQ(names_of(table_rows(table)), + (std::vector{"a", "b", "c"})); +} + +TEST(OdfTable, columns_inside_a_grouping_element) { + pugi::xml_document document; + const pugi::xml_node table = parse_table(document, R"( + + + + + + + + + )"); + + EXPECT_EQ(names_of(table_columns(table)), + (std::vector{"a", "b", "c"})); +} + +TEST(OdfTable, a_row_in_a_group_is_not_dropped_from_a_text_document) { + const std::string path = write_odf( + "odf_table_header_rows.odt", "application/vnd.oasis.opendocument.text", + std::string("") + header_row_table + ""); + + const Document document = odr::open(path).as_document_file().document(); + const std::vector text = text_of(document); + + EXPECT_NE(std::ranges::find(text, "HEADER_A"), std::end(text)); + EXPECT_NE(std::ranges::find(text, "BODY_A"), std::end(text)); +} + +TEST(OdfTable, a_row_in_a_group_is_not_dropped_from_a_spreadsheet) { + const std::string path = + write_odf("odf_table_header_rows.ods", + "application/vnd.oasis.opendocument.spreadsheet", + std::string("") + header_row_table + + ""); + + // a sheet's cells are off-tree, so they are read by position rather than + // walked + const Document document = odr::open(path).as_document_file().document(); + const Sheet sheet = document.root_element().first_child().as_sheet(); + ASSERT_TRUE(sheet); + + EXPECT_EQ(sheet.dimensions().rows, 2); + + std::vector header; + collect_text(sheet.cell(0, 0), header); + EXPECT_EQ(header, (std::vector{"HEADER_A"})); + + std::vector body; + collect_text(sheet.cell(0, 1), body); + EXPECT_EQ(body, (std::vector{"BODY_A"})); +} From 80c4ad18b3b5bf573a2a072c9b7155898cda18d0 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 20 Aug 2026 11:51:29 +0200 Subject: [PATCH 2/3] fix(odf): bind the table helpers to the parent namespace, and honour a collapsed group Two review findings: - The definitions carried an `odf::` qualification inside `namespace odr::internal::odf`, which every compiler CI builds with rejects (`-Wextra-qualification`, `-Werror`). The convention in `AGENTS.md` is the qualified name inside the *parent* namespace, as `odf_parser.cpp` does it. - `table:display="false"` on a row or column group marks it collapsed ([ODF 1.2] 19.766). Flattening it unconditionally turned rows a reader had folded away into ordinary visible content; the group and anything under it is now skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XDs5aK3ZGSZsEvqUUwBBXU --- src/odr/internal/odf/odf_table.cpp | 16 ++++++++++++++-- test/src/internal/odf/odf_table_test.cpp | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/odr/internal/odf/odf_table.cpp b/src/odr/internal/odf/odf_table.cpp index ec9d54570..ffc05ee9f 100644 --- a/src/odr/internal/odf/odf_table.cpp +++ b/src/odr/internal/odf/odf_table.cpp @@ -23,6 +23,13 @@ constexpr std::array column_group_names{ std::string_view("table:table-column-group"), }; +/// A group carries the visibility of what it holds ([ODF 1.2] 19.766); a +/// collapsed one is not shown, and neither are the groups under it. +bool is_displayed(const pugi::xml_node group) { + const pugi::xml_attribute display = group.attribute("table:display"); + return !display || display.as_bool(true); +} + void collect(const pugi::xml_node parent, const std::string_view name, const std::span group_names, std::vector &out) { @@ -31,7 +38,8 @@ void collect(const pugi::xml_node parent, const std::string_view name, if (child_name == name) { out.push_back(child); } else if (std::ranges::find(group_names, child_name) != - std::end(group_names)) { + std::end(group_names) && + is_displayed(child)) { collect(child, name, group_names, out); } } @@ -39,6 +47,10 @@ void collect(const pugi::xml_node parent, const std::string_view name, } // namespace +} // namespace odr::internal::odf + +namespace odr::internal { + std::vector odf::table_rows(const pugi::xml_node table) { std::vector result; collect(table, "table:table-row", row_group_names, result); @@ -51,4 +63,4 @@ std::vector odf::table_columns(const pugi::xml_node table) { return result; } -} // namespace odr::internal::odf +} // namespace odr::internal diff --git a/test/src/internal/odf/odf_table_test.cpp b/test/src/internal/odf/odf_table_test.cpp index 0ab33f8aa..3dc1b15a3 100644 --- a/test/src/internal/odf/odf_table_test.cpp +++ b/test/src/internal/odf/odf_table_test.cpp @@ -148,6 +148,24 @@ TEST(OdfTable, row_groups_nest) { (std::vector{"a", "b", "c"})); } +TEST(OdfTable, a_collapsed_group_is_not_shown) { + pugi::xml_document document; + const pugi::xml_node table = parse_table(document, R"( + + + + + + + + + + + )"); + + EXPECT_EQ(names_of(table_rows(table)), (std::vector{"shown"})); +} + TEST(OdfTable, columns_inside_a_grouping_element) { pugi::xml_document document; const pugi::xml_node table = parse_table(document, R"( From 3931a4d32e70936f006c3719baff12c6452bced1 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 20 Aug 2026 14:39:08 +0200 Subject: [PATCH 3/3] test(odf): advance the private reference output, and trim the prose Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015d5RcmsA777vwXiuafjx6k --- CHANGELOG.md | 9 ++++----- src/odr/internal/odf/odf_table.cpp | 3 +-- src/odr/internal/odf/odf_table.hpp | 11 +++-------- test/data.cmake | 2 +- test/src/internal/odf/odf_table_test.cpp | 7 +++---- 5 files changed, 12 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff4afb2db..0cc792f39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,11 +32,10 @@ The release run heads these entries with the version and opens a fresh - `wasm/README.md` says what Content-Security-Policy the rendered output needs, and what each directive is for. The failures are quiet: a pdf whose `data:` fonts are blocked renders as tofu rather than falling back. -- An odf table keeps the rows a grouping element holds: rows and columns wrapped - in ``, ``, - `` and their column counterparts were dropped from the - output entirely. Any table LibreOffice gave a repeating header row lost it, - in both text documents and spreadsheets. +- An odf table keeps the rows and columns a grouping element holds, such as the + `` LibreOffice writes for a repeating header row. + They were dropped from the output entirely, in text documents and + spreadsheets alike. ## v6.9.0 - 2026-08-18 diff --git a/src/odr/internal/odf/odf_table.cpp b/src/odr/internal/odf/odf_table.cpp index ffc05ee9f..f08d12721 100644 --- a/src/odr/internal/odf/odf_table.cpp +++ b/src/odr/internal/odf/odf_table.cpp @@ -23,8 +23,7 @@ constexpr std::array column_group_names{ std::string_view("table:table-column-group"), }; -/// A group carries the visibility of what it holds ([ODF 1.2] 19.766); a -/// collapsed one is not shown, and neither are the groups under it. +/// A group carries the visibility of what it holds ([ODF 1.2] 19.766). bool is_displayed(const pugi::xml_node group) { const pugi::xml_attribute display = group.attribute("table:display"); return !display || display.as_bool(true); diff --git a/src/odr/internal/odf/odf_table.hpp b/src/odr/internal/odf/odf_table.hpp index 1b742ee3c..18c10da2f 100644 --- a/src/odr/internal/odf/odf_table.hpp +++ b/src/odr/internal/odf/odf_table.hpp @@ -8,16 +8,11 @@ class xml_node; namespace odr::internal::odf { -/// The `` children of a table, in document order, including -/// those a grouping element holds — ``, -/// `` and ``, which nest -/// ([ODF 1.2] 9.1.7). +/// A table's rows in document order, including those a grouping element holds +/// - the `table:table-*-rows` family, which nests ([ODF 1.2] 9.1.7). [[nodiscard]] std::vector table_rows(pugi::xml_node table); -/// The `` children of a table, in document order, -/// including those a grouping element holds — ``, -/// `` and ``, which nest -/// ([ODF 1.2] 9.1.6). +/// The column counterpart of `table_rows()` ([ODF 1.2] 9.1.6). [[nodiscard]] std::vector table_columns(pugi::xml_node table); } // namespace odr::internal::odf diff --git a/test/data.cmake b/test/data.cmake index 2196cd61d..05d914f32 100644 --- a/test/data.cmake +++ b/test/data.cmake @@ -22,4 +22,4 @@ odr_test_data( odr_test_data( PATH "reference-output/odr-private" URL "https://github.com/opendocument-app/OpenDocument.test-private.output.git" - REVISION "08b933720c39706541d2ba74f62e248d3d2a8b21") + REVISION "21f9c3f49de727acee20768d6177980577423764") diff --git a/test/src/internal/odf/odf_table_test.cpp b/test/src/internal/odf/odf_table_test.cpp index 3dc1b15a3..cf3869b55 100644 --- a/test/src/internal/odf/odf_table_test.cpp +++ b/test/src/internal/odf/odf_table_test.cpp @@ -40,8 +40,8 @@ pugi::xml_node parse_table(pugi::xml_document &document, return document.child("table:table"); } -/// An odf package is a zip with a mimetype and a `content.xml`; nothing else is -/// needed to open one, which keeps the input to these tests a string. +/// An odf package is a zip with a mimetype and a `content.xml`, which keeps +/// the input to these tests a string. std::string write_odf(const std::string &name, const std::string &mimetype, const std::string &body) { const std::string content = @@ -78,8 +78,7 @@ void collect_text(const Element element, std::vector &out) { } } -/// An `Element` points into the document rather than holding it, so the -/// document has to outlive the walk. +/// An `Element` points into the document, which has to outlive the walk. std::vector text_of(const Document &document) { std::vector result; collect_text(document.root_element(), result);