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