feat(html): a range takes a picture with it, and undo of a split works

CHANGELOG.md

@@ -16,6 +16,14 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- A selection reaching over a picture is taken, and the picture goes with the
+  text: a frame carries `data-odr-id`, so an operation can name it. A frame
+  holding text of its own is still refused.
+
+- **Fix**: undoing a paragraph split threw, taking every later edit in the
+  session with it — the node it restored each moved child before could be the
+  line box, which the split had replaced.
+
 - **Fix**: deleting across several lines of a `.txt` left stray line numbers
   in the gutter. The renderer writes whitespace between the number cells, so
   `lastChild` there was a text node and the removal took that instead of a cell.

docs/design/document-editing.md

@@ -346,6 +346,18 @@ Two details the checks pin down:
   paragraph holds nothing, `<wbr>` where it holds something. An edited page
   then looks like a re-rendered one, which is what makes the two comparable.
 
+## A range that reaches over a picture
+
+`replaceRange` takes away the runs between its ends. A frame carries an
+address too, so it takes away **a frame that holds no run** — a picture, a
+plain shape. One that holds runs is a text box, and the text inside it is text
+the reader meant to keep, so it refuses.
+
+What it cannot reach is a picture **alone in its paragraph**, where both ends
+of the range land in a paragraph with no run: there is no run to anchor the
+edit to, so the edit is taken and changes nothing. Deleting one needs a gesture
+that names the frame rather than a range across text.
+
 ## Open questions
 
 - **A list item** is a paragraph in a list. Enter at the end of one should make

docs/design/editing.md

@@ -346,7 +346,8 @@ then intercepts `beforeinput` and takes the edits it can express as operations:
 | a paste of plain text, over as many lines as it holds | taken: each line after the first opens a paragraph |
 | a composition (CJK, autocorrect, dictation) | let through and reconciled on `compositionend` |
 | a soft line break (`insertLineBreak`) | refused, reason `newLine` - no operation carries one |
-| a range reaching over a picture or a table | refused, reason `range` - `replaceRange` takes runs and whole paragraphs away, so anything else caught between the ends would survive while the text around it went |
+| a range reaching over a picture | taken: the frame carries an address, so the picture goes with the text |
+| a range reaching over a text box or a table | refused, reason `range` - it holds text of its own, which the reader did not mean to lose |
 | anything else the browser offers (a mark, a list, a drop) | refused, reason `unsupportedEdit` |
 | an edit landing outside every run | refused, reason `range` |
 

src/odr/internal/html/document_element.cpp

@@ -250,6 +250,15 @@ void write_edit_address(const Element &element, const html::WritingState &state,
   }
 }
 
+/// The attributes a drawing's box takes: its address, so an operation names
+/// the drawing rather than the runs inside it.
+html::HtmlAttributeCallback frame_attributes(const Frame &frame,
+                                             const html::WritingState &state) {
+  return [&frame, &state](const html::HtmlAttributeWriterCallback &clb) {
+    write_edit_address(frame, state, clb);
+  };
+}
+
 /// A run whose style the box around it can carry instead. Not a background, a
 /// raised run or an addressed one: each means something else on the box.
 std::optional<Text> plain_text(const Element &element,
@@ -1045,18 +1054,21 @@ void translate_plain_frame(const Frame &frame, const GraphicStyle &style,
     background = "background-color:" + color(*style.fill_color) + ";";
   }
   state.out().write_element_begin(
-      "div", HtmlElementOptions().set_style(translate_frame_properties(frame) +
-                                            translate_drawing_style(style) +
-                                            background));
+      "div", HtmlElementOptions()
+                 .set_attributes(frame_attributes(frame, state))
+                 .set_style(translate_frame_properties(frame) +
+                            translate_drawing_style(style) + background));
   translate_children(frame.children(), state);
   state.out().write_element_end("div");
 }
 
 void translate_rect(const Frame &frame, const GraphicStyle &style,
                     const WritingState &state) {
   state.out().write_element_begin(
-      "div", HtmlElementOptions().set_style(translate_shape_properties(frame) +
-                                            translate_drawing_style(style)));
+      "div", HtmlElementOptions()
+                 .set_attributes(frame_attributes(frame, state))
+                 .set_style(translate_shape_properties(frame) +
+                            translate_drawing_style(style)));
   translate_children(frame.children(), state);
   state.out().write_new_line();
   state.out().write_raw(
@@ -1067,8 +1079,10 @@ void translate_rect(const Frame &frame, const GraphicStyle &style,
 void translate_ellipse(const Frame &frame, const GraphicStyle &style,
                        const WritingState &state) {
   state.out().write_element_begin(
-      "div", HtmlElementOptions().set_style(translate_shape_properties(frame) +
-                                            translate_drawing_style(style)));
+      "div", HtmlElementOptions()
+                 .set_attributes(frame_attributes(frame, state))
+                 .set_style(translate_shape_properties(frame) +
+                            translate_drawing_style(style)));
   state.out().write_new_line();
   translate_children(frame.children(), state);
   state.out().write_raw(
@@ -1082,10 +1096,12 @@ void translate_line(const Frame &frame, const GraphicStyle &style,
 
   state.out().write_element_begin(
       "svg", HtmlElementOptions()
-                 .set_attributes(HtmlAttributesVector{
-                     {"xmlns", "http://www.w3.org/2000/svg"},
-                     {"version", "1.1"},
-                     {"overflow", "visible"}})
+                 .set_attributes([&](const HtmlAttributeWriterCallback &clb) {
+                   clb("xmlns", "http://www.w3.org/2000/svg");
+                   clb("version", "1.1");
+                   clb("overflow", "visible");
+                   write_edit_address(frame, state, clb);
+                 })
                  .set_style("z-index:-1;position:absolute;top:0;left:0;" +
                             translate_drawing_style(style) +
                             translate_drawing_transform(frame.transform())));
@@ -1120,8 +1136,10 @@ void translate_line(const Frame &frame, const GraphicStyle &style,
 void translate_custom_shape(const Frame &frame, const GraphicStyle &style,
                             const WritingState &state) {
   state.out().write_element_begin(
-      "div", HtmlElementOptions().set_style(translate_shape_properties(frame) +
-                                            translate_drawing_style(style)));
+      "div", HtmlElementOptions()
+                 .set_attributes(frame_attributes(frame, state))
+                 .set_style(translate_shape_properties(frame) +
+                            translate_drawing_style(style)));
   translate_children(frame.children(), state);
 
   if (const std::optional<DrawingPath> path = frame.path(); path.has_value()) {

src/odr/internal/html/frontend/document.js

@@ -56,14 +56,22 @@
 
   // ------------------------------------------------------------------- page
 
+  /// The `<br>` or `<wbr>` @p paragraph ends with, or null. A line break in
+  /// the middle of one is a `<br>` too, so only the last child counts.
+  function lineBoxOf(paragraph) {
+    var last = paragraph.lastChild;
+    return last !== null && (last.nodeName === "BR" || last.nodeName === "WBR")
+      ? last
+      : null;
+  }
+
   /// The renderer ends a paragraph with `<br>` where it holds nothing and
   /// `<wbr>` where it holds something; keeping to that makes an edited page
   /// look like a freshly rendered one.
   function refreshLineBox(paragraph) {
-    var last = paragraph.lastChild;
-    while (last !== null && (last.nodeName === "BR" || last.nodeName === "WBR")) {
-      paragraph.removeChild(last);
-      last = paragraph.lastChild;
+    var box;
+    while ((box = lineBoxOf(paragraph)) !== null) {
+      paragraph.removeChild(box);
     }
     // a picture is content the same way text is, as the renderer has it
     var holds =
@@ -233,19 +241,24 @@
       copy.parentNode.removeChild(copy);
     });
 
+    // What moved, in order, rather than a sibling captured per node: the one
+    // after the last of them is the line box, which `refreshLineBox` replaces.
+    var moved = [];
     var node = stays === null ? element.firstChild : stays.nextSibling;
     while (node !== null) {
       var next = node.nextSibling;
       if (node.nodeName !== "BR" && node.nodeName !== "WBR") {
-        (function (moved, from, at) {
-          undoLog.push(function () {
-            from.insertBefore(moved, at);
-          });
-        })(node, element, next);
+        moved.push(node);
         copy.appendChild(node);
       }
       node = next;
     }
+    undoLog.push(function () {
+      var box = lineBoxOf(element);
+      for (var i = 0; i < moved.length; ++i) {
+        element.insertBefore(moved[i], box);
+      }
+    });
     return copy;
   }
 
@@ -522,39 +535,76 @@
     return caret;
   }
 
-  /// Removes the runs of @p paragraph that lie strictly between @p after and
-  /// @p before; a null end means from the first run, or to the last.
+  /// What @p paragraph holds that an operation can name, in document order:
+  /// its runs, and anything a range takes away whole.
+  function addressedIn(paragraph) {
+    return Array.prototype.filter.call(
+      paragraph.querySelectorAll("[data-odr-id]"),
+      function (element) {
+        return element.tagName === "X-S" || removableWhole(element);
+      }
+    );
+  }
+
+  /// Removes what @p paragraph holds strictly between @p after and @p before;
+  /// a null end means from the first, or to the last.
   function removeRunsBetween(paragraph, after, before) {
-    var runs = runsOf(paragraph);
-    var from = after === null ? 0 : runs.indexOf(after) + 1;
-    var to = before === null ? runs.length : runs.indexOf(before);
+    var held = addressedIn(paragraph);
+    var from = after === null ? 0 : held.indexOf(after) + 1;
+    var to = before === null ? held.length : held.indexOf(before);
     for (var i = from; i < to; ++i) {
-      perform(removeElement(runs[i]));
+      perform(removeElement(held[i]));
     }
   }
 
-  // What a range may reach over: a run, a wrapper around one, a paragraph of
-  // them, and the line box. A picture or a table is not, and no sequence of
-  // operations takes one away.
+  // The text a range reaches over: a run, a wrapper around one, a paragraph of
+  // them, and the line box.
   var reachable = { "X-S": 1, A: 1, "X-P": 1, BR: 1, WBR: 1 };
 
-  /// Whether everything between @p from and @p to is text.
+  /// Whether a range can take @p element away whole: it carries an address, so
+  /// `removeElement` can name it, and holds no run to orphan. A picture is one;
+  /// a text box is not.
+  function removableWhole(element) {
+    return (
+      element.getAttribute("data-odr-id") !== null &&
+      element.tagName !== "X-S" &&
+      element.tagName !== "X-P" &&
+      element.querySelector("x-s[data-odr-id]") === null
+    );
+  }
+
+  /// Whether everything between @p from and @p to is text, or sits in
+  /// something the range takes away whole.
   function coversOnlyText(from, to) {
     var probe = document.createRange();
     probe.setStartAfter(from);
     probe.setEndBefore(to);
-    var nodes = probe.cloneContents().querySelectorAll("*");
+    var fragment = probe.cloneContents();
+    var nodes = fragment.querySelectorAll("*");
     for (var i = 0; i < nodes.length; ++i) {
-      if (reachable[nodes[i].tagName] !== 1) {
+      if (!reaches(nodes[i], fragment)) {
         return false;
       }
     }
     return true;
   }
 
+  /// Whether @p element is text, or sits in something taken away whole.
+  function reaches(element, fragment) {
+    if (reachable[element.tagName] === 1) {
+      return true;
+    }
+    for (var at = element; at !== null && at !== fragment; at = at.parentNode) {
+      if (removableWhole(at)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   /// The paragraphs strictly between @p first and @p last, or null where
-  /// something that is not a paragraph lies between them - a table, a picture
-  /// - which is a range no sequence of operations can express.
+  /// something that is not a paragraph lies between them - a table, or a
+  /// drawing anchored beside them rather than inside one.
   function paragraphsBetween(first, last) {
     if (first.parentNode !== last.parentNode) {
       return null;

test/browser/plaintext/tests.html

@@ -27,6 +27,13 @@
     <script src="text.js"></script>
     <script src="checks.js"></script>
     <script>
+      // A throw inside an event listener does not stop the script that
+      // dispatched it, so a check page can pass over a live exception. This
+      // makes one a failed check.
+      window.addEventListener("error", function (event) {
+        check("threw: " + event.message, false);
+      });
+
       var refusals = [];
       var changes = [];
       odr.onEditRefused = function (event) {