From 6d95e3ed9951ba492b7f6bd59f18b9a020b30cc6 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 02:30:20 +0200 Subject: [PATCH 1/2] fix: clear inherited ODF backgrounds and borders --- CHANGELOG.md | 3 ++ src/odr/internal/odf/odf_style.cpp | 19 +++----- .../src/internal/odf/odf_sheet_style_test.cpp | 48 +++++++++++++++---- 3 files changed, 49 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 932eb3c9a..66f90fca8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Explicit transparent backgrounds and `none` borders override inherited ODF + styles, including after clearing a cell fill and reopening the document. + - ODF charts reject unrepresentable geometry, bound axis/label conversions, and draw complete single-slice pies and rings without overflowing totals. diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index 9c9b1305e..29064b52a 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -243,7 +243,7 @@ std::optional read_color(const pugi::xml_attribute attribute) { } const char *value = attribute.value(); if (std::strcmp("transparent", value) == 0) { - return {}; // TODO use alpha + return Color(0, 0, 0, 0); } if (value[0] == '#') { const std::uint32_t color = std::strtoull(&value[1], nullptr, 16); @@ -256,13 +256,6 @@ std::optional read_color(const pugi::xml_attribute attribute) { return {}; } -std::optional read_border(const pugi::xml_attribute attribute) { - if (attribute && std::strcmp("none", attribute.value()) != 0) { - return attribute.value(); - } - return {}; -} - PageLayout read_page_layout(const pugi::xml_node node) { PageLayout result; @@ -553,23 +546,23 @@ void Style::resolve_table_cell_style_(const pugi::xml_node node, result.padding.bottom = padding_bottom; } if (const std::optional border = - read_border(table_cell_properties.attribute("fo:border"))) { + read_string(table_cell_properties.attribute("fo:border"))) { result.border = DirectionalStyle(border); } if (const std::optional border_right = - read_border(table_cell_properties.attribute("fo:border-right"))) { + read_string(table_cell_properties.attribute("fo:border-right"))) { result.border.right = border_right; } if (const std::optional border_top = - read_border(table_cell_properties.attribute("fo:border-top"))) { + read_string(table_cell_properties.attribute("fo:border-top"))) { result.border.top = border_top; } if (const std::optional border_left = - read_border(table_cell_properties.attribute("fo:border-left"))) { + read_string(table_cell_properties.attribute("fo:border-left"))) { result.border.left = border_left; } if (const std::optional border_bottom = - read_border(table_cell_properties.attribute("fo:border-bottom"))) { + read_string(table_cell_properties.attribute("fo:border-bottom"))) { result.border.bottom = border_bottom; } } diff --git a/test/src/internal/odf/odf_sheet_style_test.cpp b/test/src/internal/odf/odf_sheet_style_test.cpp index 25581040a..7bbc0759c 100644 --- a/test/src/internal/odf/odf_sheet_style_test.cpp +++ b/test/src/internal/odf/odf_sheet_style_test.cpp @@ -201,15 +201,47 @@ TEST(OdfSheetStyle, one_delta_on_one_base_is_one_style) { } TEST(OdfSheetStyle, a_fill_taken_away_is_written_transparent) { - const Document document = - document_of(flat_sheet(string_cell("a", "ce1"), red_cell_style)); - const Sheet sheet = first_sheet(document); - - sheet.set_cell_style(0, 0, fill(Color(0, 0, 0, 0)), {}); + for (const bool named : {false, true}) { + const Document document = document_of( + flat_sheet(string_cell("a", "ce1"), named ? "" : red_cell_style, + named ? red_cell_style : "")); + const Sheet sheet = first_sheet(document); + sheet.set_cell_style(0, 0, fill(Color(0, 0, 0, 0)), {}); + + const auto check = [](const Sheet &sheet) { + const auto color = sheet.cell_style(0, 0).background_color; + ASSERT_TRUE(color); + EXPECT_EQ(color->alpha, 0); + }; + check(sheet); + check(first_sheet(document_of(saved(document)))); + } +} - EXPECT_EQ(fill_at(sheet, 0, 0), std::nullopt); - EXPECT_NE(saved(document).find(R"(fo:background-color="transparent")"), - std::string::npos); +TEST(OdfSheetStyle, explicit_transparency_and_no_border_override_inheritance) { + const Document document = document_of(flat_sheet( + string_cell("a", "child"), + R"()" + R"()" + R"()" + R"()", + R"()" + R"()" + R"()" + R"()")); + const Sheet sheet = first_sheet(document); + const TableCellStyle style = sheet.cell_style(0, 0); + ASSERT_TRUE(style.background_color); + EXPECT_EQ(style.background_color->alpha, 0); + EXPECT_EQ(style.border.top, "none"); + EXPECT_EQ(style.border.right, "none"); + EXPECT_EQ(style.border.bottom, "none"); + EXPECT_EQ(style.border.left, "1pt solid #000000"); + const TextStyle text = text_style_of(sheet, 0); + ASSERT_TRUE(text.background_color); + EXPECT_EQ(text.background_color->alpha, 0); } TEST(OdfSheetStyle, an_alignment_is_fixed_on_the_cell) { From ad33c721cf32cec265ed20ef8c454ec9d80c82c9 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 21:17:36 +0200 Subject: [PATCH 2/2] fix(odf): clear inherited paint without a transparent colour A transparent colour or a none border in the resolved style changed the public contract, so a removed highlight read back as a colour and the HTML gained no-op declarations. Clear the inherited value while the style resolves, and keep nullopt for no background and no border. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Gb1fLafqqzehpqPuU6uBfn --- src/odr/internal/odf/odf_style.cpp | 72 +++++++++++-------- .../src/internal/odf/odf_sheet_style_test.cpp | 25 +++---- 2 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index 29064b52a..1f7b62efb 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -243,7 +243,7 @@ std::optional read_color(const pugi::xml_attribute attribute) { } const char *value = attribute.value(); if (std::strcmp("transparent", value) == 0) { - return Color(0, 0, 0, 0); + return {}; // TODO use alpha } if (value[0] == '#') { const std::uint32_t color = std::strtoull(&value[1], nullptr, 16); @@ -256,6 +256,29 @@ std::optional read_color(const pugi::xml_attribute attribute) { return {}; } +/// A stated `transparent` clears the @p inherited background. +std::optional read_background(const pugi::xml_attribute attribute, + const std::optional &inherited) { + if (std::strcmp("transparent", attribute.value()) == 0) { + return {}; + } + const std::optional color = read_color(attribute); + return color ? color : inherited; +} + +/// A stated `none` clears the @p inherited border. +std::optional +read_border(const pugi::xml_attribute attribute, + const std::optional &inherited) { + if (!attribute) { + return inherited; + } + if (std::strcmp("none", attribute.value()) == 0) { + return {}; + } + return attribute.value(); +} + PageLayout read_page_layout(const pugi::xml_node node) { PageLayout result; @@ -389,10 +412,9 @@ void Style::resolve_text_style_(const StyleRegistry *registry, read_color(text_properties.attribute("fo:color"))) { result.font_color = font_color; } - if (const std::optional background_color = - read_color(text_properties.attribute("fo:background-color"))) { - result.background_color = background_color; - } + result.background_color = + read_background(text_properties.attribute("fo:background-color"), + result.background_color); if (const pugi::xml_attribute text_position = text_properties.attribute("style:text-position")) { if (const std::optional font_position = @@ -521,10 +543,9 @@ void Style::resolve_table_cell_style_(const pugi::xml_node node, table_cell_properties.attribute("style:wrap-option")) { result.wrap_text = std::strcmp("wrap", wrap_option.value()) == 0; } - if (const std::optional background_color = - read_color(table_cell_properties.attribute("fo:background-color"))) { - result.background_color = background_color; - } + result.background_color = + read_background(table_cell_properties.attribute("fo:background-color"), + result.background_color); if (const std::optional padding = read_measure(table_cell_properties.attribute("fo:padding"))) { result.padding = DirectionalStyle(padding); @@ -545,26 +566,19 @@ void Style::resolve_table_cell_style_(const pugi::xml_node node, read_measure(table_cell_properties.attribute("fo:padding-bottom"))) { result.padding.bottom = padding_bottom; } - if (const std::optional border = - read_string(table_cell_properties.attribute("fo:border"))) { - result.border = DirectionalStyle(border); - } - if (const std::optional border_right = - read_string(table_cell_properties.attribute("fo:border-right"))) { - result.border.right = border_right; - } - if (const std::optional border_top = - read_string(table_cell_properties.attribute("fo:border-top"))) { - result.border.top = border_top; - } - if (const std::optional border_left = - read_string(table_cell_properties.attribute("fo:border-left"))) { - result.border.left = border_left; - } - if (const std::optional border_bottom = - read_string(table_cell_properties.attribute("fo:border-bottom"))) { - result.border.bottom = border_bottom; - } + if (const pugi::xml_attribute border = + table_cell_properties.attribute("fo:border")) { + result.border = DirectionalStyle(read_border(border, {})); + } + result.border.right = read_border( + table_cell_properties.attribute("fo:border-right"), result.border.right); + result.border.top = read_border( + table_cell_properties.attribute("fo:border-top"), result.border.top); + result.border.left = read_border( + table_cell_properties.attribute("fo:border-left"), result.border.left); + result.border.bottom = + read_border(table_cell_properties.attribute("fo:border-bottom"), + result.border.bottom); } void Style::resolve_graphic_style_(const pugi::xml_node node, diff --git a/test/src/internal/odf/odf_sheet_style_test.cpp b/test/src/internal/odf/odf_sheet_style_test.cpp index 7bbc0759c..550ffd5d7 100644 --- a/test/src/internal/odf/odf_sheet_style_test.cpp +++ b/test/src/internal/odf/odf_sheet_style_test.cpp @@ -208,13 +208,11 @@ TEST(OdfSheetStyle, a_fill_taken_away_is_written_transparent) { const Sheet sheet = first_sheet(document); sheet.set_cell_style(0, 0, fill(Color(0, 0, 0, 0)), {}); - const auto check = [](const Sheet &sheet) { - const auto color = sheet.cell_style(0, 0).background_color; - ASSERT_TRUE(color); - EXPECT_EQ(color->alpha, 0); - }; - check(sheet); - check(first_sheet(document_of(saved(document)))); + EXPECT_EQ(fill_at(sheet, 0, 0), std::nullopt); + EXPECT_NE(saved(document).find(R"(fo:background-color="transparent")"), + std::string::npos); + EXPECT_EQ(fill_at(first_sheet(document_of(saved(document))), 0, 0), + std::nullopt); } } @@ -233,15 +231,12 @@ TEST(OdfSheetStyle, explicit_transparency_and_no_border_override_inheritance) { R"()")); const Sheet sheet = first_sheet(document); const TableCellStyle style = sheet.cell_style(0, 0); - ASSERT_TRUE(style.background_color); - EXPECT_EQ(style.background_color->alpha, 0); - EXPECT_EQ(style.border.top, "none"); - EXPECT_EQ(style.border.right, "none"); - EXPECT_EQ(style.border.bottom, "none"); + EXPECT_EQ(style.background_color, std::nullopt); + EXPECT_EQ(style.border.top, std::nullopt); + EXPECT_EQ(style.border.right, std::nullopt); + EXPECT_EQ(style.border.bottom, std::nullopt); EXPECT_EQ(style.border.left, "1pt solid #000000"); - const TextStyle text = text_style_of(sheet, 0); - ASSERT_TRUE(text.background_color); - EXPECT_EQ(text.background_color->alpha, 0); + EXPECT_EQ(text_style_of(sheet, 0).background_color, std::nullopt); } TEST(OdfSheetStyle, an_alignment_is_fixed_on_the_cell) {