From 14381762079700765823ad0bdc09f1f19950adf6 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 10 Sep 2026 17:39:36 +0200 Subject: [PATCH] feat(html): a range takes a picture with it, and undo of a split works A frame carries `data-odr-id` now, so `removeElement` can name a drawing and a selection reaching over a picture is taken rather than refused. A frame holding runs is a text box, and the text in it is text the reader meant to keep, so that one still refuses. Two things the check page was hiding, both found on the way. Undoing a paragraph split threw `insertBefore ... not a child of this node`, and every edit after it in the session was lost. `splitLevel` captured, per moved node, the sibling to put it back before - and for the last of them that sibling is the paragraph's line box, which `refreshLineBox` then replaces. What moved is now one list, put back in order ahead of whatever line box the paragraph has. And the page reported "48 checks, 0 failed" while running 48 of 93: the throw ended the script, and `checks.js` tallies as it goes, so a page that dies half way looks like a short green run. Both pages now turn an uncaught error into a failed check. The 45 checks that had never run brought three wrong assertions with them - a refusal needs a caret or the gate says `range` before it looks at the input type, two refusals of the same reason inside two seconds are one event, and an edit inside a paragraph holding no run is taken and changes nothing rather than refused. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW --- CHANGELOG.md | 8 ++ docs/design/document-editing.md | 12 +++ docs/design/editing.md | 3 +- src/odr/internal/html/document_element.cpp | 44 +++++++--- src/odr/internal/html/frontend/document.js | 96 ++++++++++++++++------ test/browser/plaintext/tests.html | 7 ++ test/browser/text/tests.html | 72 +++++++++++++++- 7 files changed, 201 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dce85b746..159ad3249 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/design/document-editing.md b/docs/design/document-editing.md index 9f5d84f0f..914b65ff3 100644 --- a/docs/design/document-editing.md +++ b/docs/design/document-editing.md @@ -346,6 +346,18 @@ Two details the checks pin down: paragraph holds nothing, `` 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 diff --git a/docs/design/editing.md b/docs/design/editing.md index e09736a1c..5cda776e6 100644 --- a/docs/design/editing.md +++ b/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` | diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index eacf3811f..24e32d0ac 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/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 plain_text(const Element &element, @@ -1045,9 +1054,10 @@ 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"); } @@ -1055,8 +1065,10 @@ void translate_plain_frame(const Frame &frame, const GraphicStyle &style, 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 path = frame.path(); path.has_value()) { diff --git a/src/odr/internal/html/frontend/document.js b/src/odr/internal/html/frontend/document.js index 1d19f4ca8..410dac885 100644 --- a/src/odr/internal/html/frontend/document.js +++ b/src/odr/internal/html/frontend/document.js @@ -56,14 +56,22 @@ // ------------------------------------------------------------------- page + /// The `
` or `` @p paragraph ends with, or null. A line break in + /// the middle of one is a `
` 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 `
` where it holds nothing and /// `` 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; diff --git a/test/browser/plaintext/tests.html b/test/browser/plaintext/tests.html index 3ac3dd1fa..0a0c718c0 100644 --- a/test/browser/plaintext/tests.html +++ b/test/browser/plaintext/tests.html @@ -27,6 +27,13 @@ @@ -44,6 +69,13 @@