From 99db1dc73c60fe3dc939ddd8fe3d0f304d085337 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Wed, 9 Sep 2026 20:33:50 +0200 Subject: [PATCH] feat(spreadsheet): write a cell of several runs A cell whose paragraph held several runs was locked `rich`, because the write went through one text run and would have lost the others. What it loses is the formatting of a text the user is replacing anyway, which is what every spreadsheet does. `text_run_of` descends through a single span before it looks for the run, so a cell that holds one run writes through it and keeps its style - the same walk `spreadsheet.js::runOf` makes over the page, which is what keeps the two showing the same thing. Where the paragraph holds several runs, they are replaced by one, and the elements over the old children keep their ids and stop being reachable, as the xlsx write already did. The `rich` lock stays on what a write would take away without the user seeing it go: a link, whose target is not what the cell shows, and a line break or several paragraphs, which are a second line the overlay cannot write. xlsx needed no engine change - `sheet_set_cell` rewrites the whole `c` - so the lock alone changes there. 1876 cells over 37 files lose the lock in the reference output. The pins are not advanced here; the regeneration goes with the merge. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW --- CHANGELOG.md | 5 ++ docs/design/spreadsheet-editing.md | 34 ++++---- src/odr/internal/html/document_element.cpp | 39 ++++++---- src/odr/internal/odf/AGENTS.md | 17 +++- src/odr/internal/odf/odf_document.cpp | 78 +++++++++++++++---- test/browser/sheet/editing.html | 6 +- test/src/html_test.cpp | 25 +++++- .../src/internal/odf/odf_sheet_write_test.cpp | 60 ++++++++++++++ .../ooxml/ooxml_spreadsheet_write_test.cpp | 14 ++++ 9 files changed, 229 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 659c0d3ec..fd61f4184 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- A cell of several runs is written rather than locked: the write replaces what + the cell shows with one run. A cell holding one run is written through it, so + that run keeps its style. The `rich` lock stays on what a write would take + away unseen: several paragraphs, a link, a line break. + - An `.xlsx` cell the file states no `c` for is written: the write states the `c` in its row in column order, the `row` in `sheetData` in row order where the file states none, and widens `dimension` to hold the new cell. A position diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index 8e90c54f6..9bcb53fa7 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -1,6 +1,6 @@ # Spreadsheet editing design -Status: **steps 0, 1, 2.1 and 2.2 landed; 2.3 is next.** This +Status: **steps 0, 1 and 2 landed; step 3 is next.** This records why spreadsheet editing is staged the way it is, what the code already gives us, and the order the steps go in. It is a plan, not a record — update it as steps land. @@ -109,9 +109,8 @@ stops changing what a sheet writes. The page carries only what the browser cannot work out for itself: - a **lock** on a cell that cannot be edited, as a class plus its reason — - `formula`, `repeated` (ODS, until step 2), `rich` (several runs, several - paragraphs, a link, a line break), `shapes` only where the cell is nothing - but its anchored drawings; + `formula`, `rich` (several paragraphs, a link, a line break), `shapes` only + where the cell is nothing but its anchored drawings; - whether the **document** can be edited at all, one attribute on the table, so `enable()` can refuse with a reason before the user clicks anything. @@ -325,12 +324,12 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`. `` for a string — and hands the registry a fresh text element; the old ones keep their ids and stop being reachable. A shared string is never written back into `sharedStrings.xml`, which is what `inlineStr` is for. - Refused, rather than written badly: a cell the file spells no element for, a - covered one (XLSX), one holding a formula, and one holding richer markup - than a single plain paragraph. Every refusal is decided before the engine - writes anything. **Writing a formula cell waits for step 4** — overwriting - one leaves every value computed from it stale. A repeated ODS cell was - refused here and is written since step 2.1. + Refused, rather than written badly: a covered one (XLSX), one holding a + formula, and one holding a link, a line break or several paragraphs. Every + refusal is decided before the engine writes anything. **Writing a formula + cell waits for step 4** — overwriting one leaves every value computed from + it stale. A repeated ODS cell, an empty one, a position past the sheet and a + cell of several runs were all refused here and are written since step 2. 3. **Landed.** XLSX `save`, mirroring docx: write back every worksheet and `workbook.xml` from their dom, byte-copy the rest, and put back the xml declaration pugixml never parsed. `fullCalcOnLoad` is set on every save @@ -415,9 +414,18 @@ Each step ships on its own. "Both" means `.ods` and `.xlsx`. position, so the insert is local and nothing is reindexed. A position a merge covers refuses before any of it, because Excel ignores what a covered `c` holds. -3. Rich cells: replace with one plain paragraph, keeping the cell style. The - `rich` lock stays on a cell with a link or a line break; it goes for - several runs of the same paragraph. +3. **Landed.** Rich cells: a paragraph of text and spans is replaced with one + run, and the cell keeps its own style because nothing above the runs is + touched. `text_run_of` descends through a single span first, so a cell that + holds one run writes through it and that run keeps its style — which is what + `spreadsheet.js::runOf` does to the page, so the two agree. Several runs are + replaced, and the elements over the old ones keep their ids and stop being + reachable, as XLSX already did. + + The `rich` lock stays on a link, a line break and several paragraphs: a link + target is not what the cell shows, and both of the others are a second line + the overlay cannot write. XLSX needed no engine change — `sheet_set_cell` + rewrites the whole `c` — so it is the lock alone there. ### Step 3 — Formulas, read side diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index 7c179d0fc..5cc9bde5b 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -288,23 +288,33 @@ bool is_blank(const SheetCell &cell) { return true; } -/// Empty, or one text run at most - what a write can replace. odf wraps a -/// cell's text in a `text:p`, ooxml hangs it under the `c` directly, so a -/// single paragraph is unwrapped once. -bool holds_one_run(const ElementRange &children, const bool unwrap = true) { +/// Text, and spans of text, and nothing else, all the way down. +bool holds_plain_runs(const ElementRange &children) { + for (const Element child : children) { + const ElementType type = child.type(); + if (type == ElementType::text) { + continue; + } + if (type != ElementType::span || !holds_plain_runs(child.children())) { + return false; + } + } + return true; +} + +/// What a write can replace: text and spans of it, under one paragraph at +/// most. odf wraps a cell's text in a `text:p` and ooxml hangs it under the +/// `c` directly, so a single paragraph is unwrapped once. Several are several +/// lines, which the overlay cannot write. +bool holds_plain_cell(const ElementRange &children) { ElementIterator child = children.begin(); if (child == children.end()) { return true; } - const Element only = *child; - if (++child != children.end()) { - return false; - } - if (only.type() == ElementType::text) { - return true; + if (const Element first = *child; first.type() == ElementType::paragraph) { + return ++child == children.end() && holds_plain_runs(first.children()); } - return unwrap && only.type() == ElementType::paragraph && - holds_one_run(only.children(), false); + return holds_plain_runs(children); } /// Its place among the document's sheets, which is how an op names one. @@ -327,8 +337,9 @@ const char *cell_lock(const SheetCell &cell, const bool anchors_shapes) { if (anchors_shapes) { return "shapes"; } - // a write replaces the cell's one run, so anything richer would be lost - return holds_one_run(cell.children()) ? nullptr : "rich"; + // a write replaces the cell's runs, and a link or a line break would go + // unseen with them + return holds_plain_cell(cell.children()) ? nullptr : "rich"; } /// A shape or picture anchored in a cell reaches past it by design. diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 406e63bc2..c59aef2df 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -186,10 +186,11 @@ The structural/foundational gaps, roughly by value: 2. **Spreadsheet editing is one cell value.** `sheet_set_cell` writes `office:value-type`/`office:value` *and* the `text:p` under the cell — the file states the value and shows a rendering of it, and setting one without - the other leaves it contradicting itself. It writes through the cell's - single text run, so a cell holding a formula or richer markup than one plain - paragraph refuses; a position never does, because the write reaches any of - them. + the other leaves it contradicting itself. It writes through the run the cell + already holds, so that run keeps its style, and replaces the runs of a + paragraph that holds several. A cell holding a formula, a link, a line break + or several paragraphs refuses; a position never does, because the write + reaches any of them. A **repeated** cell is written by cutting the run: `claim_cell` copies the `table:table-row` and the `table:table-cell` around the position and leaves @@ -204,6 +205,14 @@ The structural/foundational gaps, roughly by value: builds the element for it. A spanned cell that holds no paragraph takes one from `text_run_of` instead, because the page already reads it as editable. + `text_run_of` descends through a single span before it looks for the run, + so a cell of one styled run is written through rather than rebuilt — the + same walk `spreadsheet.js::runOf` makes over the page, which is what keeps + the two showing the same thing. Where it rebuilds, the old children are + removed from the dom while their elements keep their ids and stop being + reachable. Their `pugi::xml_node` is dangling from then on, which is the + cost `ooxml/spreadsheet` already pays for the same tombstoning. + A position the sheet stops before is reached by `grow_to_cell`, which appends the rows and the runs of empty cells it takes and declares the columns, so `dimensions` covers the new cell. A repeated row is cut before diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 634440ef1..169df9933 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -384,7 +384,7 @@ class ElementAdapter final : public AdapterBase { if (cell->node.attribute("table:formula")) { throw UnsupportedOperation(); // its dependants would go stale } - if (cell_id != null_element_id && !holds_one_run(cell_id)) { + if (cell_id != null_element_id && !holds_plain_paragraph(cell_id)) { throw UnsupportedOperation(); } @@ -1064,9 +1064,37 @@ class ElementAdapter final : public AdapterBase { return cell->element_id; } - /// Whether a write can go through the cell: one plain paragraph of one run - /// at most. Richer markup is kept rather than overwritten. - [[nodiscard]] bool holds_one_run(const ElementIdentifier cell_id) const { + /// The only child of @p element_id, null where it has none or several. + [[nodiscard]] ElementIdentifier + only_child(const ElementIdentifier element_id) const { + const ElementIdentifier child_id = element_first_child(element_id); + return child_id != null_element_id && + element_next_sibling(child_id) == null_element_id + ? child_id + : null_element_id; + } + + /// Text, and spans of text, and nothing else, all the way down. + [[nodiscard]] bool holds_plain_runs(const ElementIdentifier parent_id) const { + for (ElementIdentifier child_id = element_first_child(parent_id); + child_id != null_element_id; + child_id = element_next_sibling(child_id)) { + const ElementType type = element_type(child_id); + if (type == ElementType::text) { + continue; + } + if (type != ElementType::span || !holds_plain_runs(child_id)) { + return false; + } + } + return true; + } + + /// Whether a write can go through the cell: one paragraph, of text and spans + /// alone. A link, a line break or a second paragraph is content the write + /// would take away without the user seeing it go. + [[nodiscard]] bool + holds_plain_paragraph(const ElementIdentifier cell_id) const { const ElementIdentifier paragraph_id = element_first_child(cell_id); if (paragraph_id == null_element_id) { return true; // a spanned cell states no paragraph; the write states one @@ -1075,14 +1103,13 @@ class ElementAdapter final : public AdapterBase { element_type(paragraph_id) != ElementType::paragraph) { return false; } - const ElementIdentifier text_id = element_first_child(paragraph_id); - return text_id == null_element_id || - (element_next_sibling(text_id) == null_element_id && - element_type(text_id) == ElementType::text); + return holds_plain_runs(paragraph_id); } - /// That run, and the paragraph around it, created where the cell states - /// neither. @ref holds_one_run has to pass. + /// The run a write goes through: the one the cell holds, so it keeps its + /// style, and a fresh one where the cell holds none or several. The + /// paragraph too where the cell states none. @ref holds_plain_paragraph has + /// to pass. [[nodiscard]] ElementIdentifier text_run_of(const ElementIdentifier cell_id) const { ElementIdentifier paragraph_id = element_first_child(cell_id); @@ -1093,15 +1120,38 @@ class ElementAdapter final : public AdapterBase { m_registry->append_child(cell_id, new_id); paragraph_id = new_id; } - if (const ElementIdentifier text_id = element_first_child(paragraph_id); - text_id != null_element_id) { + + // the deepest element holding the whole content: writing through it keeps + // the style it carries, as `spreadsheet.js::runOf` does on the page + ElementIdentifier holder_id = paragraph_id; + for (ElementIdentifier only_id = only_child(holder_id); + only_id != null_element_id && + element_type(only_id) == ElementType::span; + only_id = only_child(holder_id)) { + holder_id = only_id; + } + + if (const ElementIdentifier text_id = only_child(holder_id); + text_id != null_element_id && + element_type(text_id) == ElementType::text) { return text_id; } + + // several runs: the elements over the old children keep their ids and stop + // being reachable + pugi::xml_node holder_node = get_node(holder_id); + while (const pugi::xml_node child = holder_node.first_child()) { + holder_node.remove_child(child); + } + ElementRegistry::Element &holder = m_registry->element_at(holder_id); + holder.first_child_id = null_element_id; + holder.last_child_id = null_element_id; + const pugi::xml_node text_node = - get_node(paragraph_id).append_child(pugi::xml_node_type::node_pcdata); + holder_node.append_child(pugi::xml_node_type::node_pcdata); const auto &[new_id, unused1, unused2] = m_registry->create_text_element(text_node, text_node); - m_registry->append_child(paragraph_id, new_id); + m_registry->append_child(holder_id, new_id); return new_id; } diff --git a/test/browser/sheet/editing.html b/test/browser/sheet/editing.html index c706b90ac..4906c8443 100644 --- a/test/browser/sheet/editing.html +++ b/test/browser/sheet/editing.html @@ -10,7 +10,7 @@ + its own, D3 a link. --> @@ -46,7 +46,7 @@ - + @@ -179,7 +179,7 @@ check("a formula cell cannot be edited", odr.editing.editAt(1, 2) === false && editor() === null); check("and says why", refusals.length === 1 && refusals[0] === "formula 2 at 1,2"); check("the cell is outlined for the tap it answers", cell(1, 2).classList.contains("odr-sheet-refused")); - check("a cell of several runs neither", odr.editing.editAt(3, 2) === false); + check("a cell holding a link neither", odr.editing.editAt(3, 2) === false); odr.editing.editAt(2, 2); editor().value = "plain"; diff --git a/test/src/html_test.cpp b/test/src/html_test.cpp index c6b9028ad..42b5fc82f 100644 --- a/test/src/html_test.cpp +++ b/test/src/html_test.cpp @@ -876,7 +876,30 @@ TEST(html, a_formula_cell_is_locked_with_its_reason) { EXPECT_NE(page.find("odr-locked"), std::string::npos); } -// A write replaces the cell's one run, so richer markup is locked rather than +// Several runs of one paragraph are one line, which a write replaces. +TEST(html, a_cell_of_several_runs_carries_no_lock) { + const std::string page = render_sheet( + fods_file(fods_row(R"()" + R"(two runs)" + R"()")), + HtmlConfig()); + + EXPECT_EQ(page.find(R"(data-odr-lock=")"), std::string::npos); +} + +// The target of a link is not what the cell shows, so a write would take it +// away without the user seeing it go. +TEST(html, a_cell_holding_a_link_is_locked_rich) { + const std::string page = render_sheet( + fods_file(fods_row(R"()" + R"()" + R"(x)")), + HtmlConfig()); + + EXPECT_NE(page.find(R"(data-odr-lock="rich")"), std::string::npos); +} + +// A write replaces the cell's line, so several of them are locked rather than // thrown away. TEST(html, a_cell_of_several_paragraphs_is_locked_rich) { const std::string page = render_sheet( diff --git a/test/src/internal/odf/odf_sheet_write_test.cpp b/test/src/internal/odf/odf_sheet_write_test.cpp index d14fd7330..aee195ed1 100644 --- a/test/src/internal/odf/odf_sheet_write_test.cpp +++ b/test/src/internal/odf/odf_sheet_write_test.cpp @@ -471,6 +471,66 @@ TEST(OdfSheetWrite, an_empty_cell_written_into_saves_and_reopens) { EXPECT_FALSE(sheet.cell(3, 0).value().has_text()); } +/// The write replaces what the cell shows, and several runs of one paragraph +/// are one line of it. +TEST(OdfSheetWrite, a_cell_of_several_runs_is_written) { + const Document document = document_of( + flat_sheet(R"()" + R"(two runs)" + R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("one")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "one"); + + std::ostringstream saved; + document.save(saved); + EXPECT_EQ(saved.str().find(")" + R"(b)" + R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("y")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "y"); + + std::ostringstream saved; + document.save(saved); + EXPECT_NE(saved.str().find(R"(y)"), + std::string::npos); +} + +/// The target of a link is not what the cell shows, so the write would take it +/// away without the user seeing it go. +TEST(OdfSheetWrite, a_cell_holding_a_link_refuses_to_be_written) { + const Document document = document_of( + flat_sheet(R"()" + R"(x)" + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); +} + +/// A line break is a second line, which one run cannot hold. +TEST(OdfSheetWrite, a_cell_holding_a_line_break_refuses_to_be_written) { + const Document document = + document_of(flat_sheet(R"()" + R"(ab)" + R"()")); + const Sheet sheet = first_sheet(document); + + EXPECT_THROW(sheet.set_cell(0, 0, CellValue("y")), UnsupportedOperation); +} + TEST(OdfSheetWrite, a_cell_of_several_paragraphs_refuses_to_be_written) { const Document document = document_of( flat_sheet(R"()" diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp index cd19369cf..759a84c7c 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_write_test.cpp @@ -77,6 +77,20 @@ TEST(OoxmlSpreadsheetWrite, EXPECT_EQ(sheet.cell(1, 0).value().text(), "same"); } +/// A write states one inline string over whatever the cell held, so several +/// `r` runs go with it. +TEST(OoxmlSpreadsheetWrite, a_cell_of_several_runs_is_written) { + const Document document = decode( + workbook(R"()" + R"(two runs)")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell(0, 0, CellValue("one")); + + EXPECT_EQ(sheet.cell(0, 0).value().text(), "one"); + EXPECT_EQ(worksheet_of(document).find(""), std::string::npos); +} + TEST(OoxmlSpreadsheetWrite, a_cleared_cell_states_nothing) { const Document document = decode(workbook(R"(7)"));
7 boldtworunsx
4