feat(spreadsheet): write a cell of several runs

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

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`.
    `<is><t>` 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
 

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.

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

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;
   }
 

test/browser/sheet/editing.html

@@ -10,7 +10,7 @@
     <!-- A sheet as `translate_sheet` writes one, with the styles it hoists into
          classes written inline: every column states a width, so a string is cut
          where the next cell shows something. B3 holds a formula, C3 a run of
-         its own, D3 two of them. -->
+         its own, D3 a link. -->
     <table class="odr-sheet" data-odr-editable="true" data-odr-sheet="2">
       <col class="odr-sheet-gutter" />
       <col style="width: 80px" />
@@ -46,7 +46,7 @@
           <td style="max-width:0;white-space:nowrap"></td>
           <td class="odr-value-type-float odr-locked" data-odr-lock="formula" style="max-width:0;white-space:nowrap">7</td>
           <td style="max-width:0;white-space:nowrap"><x-p><x-s style="font-weight:bold">bold</x-s></x-p></td>
-          <td class="odr-locked" data-odr-lock="rich" style="max-width:0;white-space:nowrap"><x-p><x-s>two</x-s><x-s>runs</x-s></x-p></td>
+          <td class="odr-locked" data-odr-lock="rich" style="max-width:0;white-space:nowrap"><x-p><a href="https://x.example"><x-s>x</x-s></a></x-p></td>
         </tr>
         <tr>
           <th class="odr-sheet-row-header">4</th>
@@ -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";

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"(<table:table-cell office:value-type="string">)"
+                         R"(<text:p>two <text:span>runs</text:span></text:p>)"
+                         R"(</table:table-cell>)")),
+      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"(<table:table-cell office:value-type="string">)"
+                         R"(<text:p><text:a xlink:href="https://x.example">)"
+                         R"(x</text:a></text:p></table:table-cell>)")),
+      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(