From 12310b4972bf907e236ac73e9218ef6f5a07492b Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 10 Sep 2026 14:05:10 +0200 Subject: [PATCH] feat(html)!: address an edit by element id, not by document path An operation names its element by the id the render writes into the page, `data-odr-id`, and the envelope moves to version 2. A path is positional, so it names a different element the moment an operation inserts one - which the structural edits this prepares for do. An id does not move. An editable render addresses its paragraphs as well as its runs, because a paragraph is what a split or an insert anchors on. `Element::identifier()` and `Document::element_by_id()` are the two ends of the address, in C++ and in the python, jni and apple bindings. `docs/design/document-editing.md` records the decisions this work rests on. It sits beside `spreadsheet-editing.md`: one document per editor, named after the view it edits. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KKFKbUVCYF2VhujdmjhhPW --- CHANGELOG.md | 12 +- .../include/OdrCoreObjC/ODRDocumentElement.h | 2 + apple/src/ODRDocumentElement.mm | 4 + docs/design/document-editing.md | 278 ++++++++++++++++++ docs/design/editing.md | 27 +- docs/design/spreadsheet-editing.md | 12 +- jni/java/app/opendocument/core/Element.java | 7 + jni/src/jni_document.cpp | 7 + .../app/opendocument/core/DocumentTest.java | 12 +- python/src/bind_document.cpp | 3 + python/tests/test_document.py | 5 +- src/odr/document.cpp | 40 ++- src/odr/document.hpp | 12 +- src/odr/document_element.cpp | 2 + src/odr/document_element.hpp | 4 + src/odr/internal/html/document_element.cpp | 28 +- src/odr/internal/html/frontend/document.js | 30 +- test/browser/text/tests.html | 32 +- test/src/document_edit_test.cpp | 52 +++- test/src/document_test.cpp | 53 +++- test/src/html_test.cpp | 32 +- wasm/tests/edit.test.mjs | 21 +- wasm/tests/render.test.mjs | 4 +- 23 files changed, 560 insertions(+), 119 deletions(-) create mode 100644 docs/design/document-editing.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b2a7a9d8..d81ba4afa 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 +- **Breaking** (wire only): an edit operation names its element by the id the + render writes into the page, `data-odr-id`, not by a document path. The + envelope is `{"version": 2, ...}`; version 1 is refused. An editable render + addresses its paragraphs as well as its runs. + +- `Element::identifier()` and `Document::element_by_id()` are the two ends of + that address, in C++ and in the python, jni and apple bindings. + - `odr.editing` is on every document view, not only on a sheet's. The mode, the refusals, the log and the `odr.onEdit*` callbacks are one surface a host wires per document, and each format attaches its own editor to it. @@ -108,11 +116,11 @@ The release run heads these entries with the version and opens a fresh beside it. - A sheet rendered with `HtmlConfig::editable` carries no `contenteditable` - and no `data-odr-path`: its editing is an overlay, so the markup states + and no `data-odr-id`: its editing is an overlay, so the markup states none. A cell's runs fold into the `td` as they do read-only. - **Breaking** (wire only) `Document::edit` takes an op envelope, - `{"version": 1, "ops": [...]}`, with `setCell` writing a sheet cell by + `{"version": 2, "ops": [...]}`, with `setCell` writing a sheet cell by position and `setText` carrying what the `modifiedText` map carried. - `Sheet::set_cell` writes a repeated `.ods` cell: the run is cut into the diff --git a/apple/include/OdrCoreObjC/ODRDocumentElement.h b/apple/include/OdrCoreObjC/ODRDocumentElement.h index 0299eba03..bf8ddc572 100644 --- a/apple/include/OdrCoreObjC/ODRDocumentElement.h +++ b/apple/include/OdrCoreObjC/ODRDocumentElement.h @@ -82,6 +82,8 @@ NS_SWIFT_NAME(Element) @interface ODRElement : NSObject @property(nonatomic, readonly) ODRElementType type; +/// The address an edit operation names this element by. +@property(nonatomic, readonly) uint64_t identifier; /// `NO` for the element you get past the end of the tree. @property(nonatomic, readonly) BOOL exists; diff --git a/apple/src/ODRDocumentElement.mm b/apple/src/ODRDocumentElement.mm index aab40313a..d90e4d1bc 100644 --- a/apple/src/ODRDocumentElement.mm +++ b/apple/src/ODRDocumentElement.mm @@ -237,6 +237,10 @@ - (BOOL)isEditable { return guarded_value([&] { return _handle.is_editable() ? YES : NO; }, NO); } +- (uint64_t)identifier { + return guarded_value([&] { return _handle.identifier(); }, 0); +} + @end #pragma mark - typed views diff --git a/docs/design/document-editing.md b/docs/design/document-editing.md new file mode 100644 index 000000000..b33c2a35a --- /dev/null +++ b/docs/design/document-editing.md @@ -0,0 +1,278 @@ +# Document editing design + +The editor of the **document view** — `frontend/document.js` — built on the +mode frame in [`editing.md`](editing.md), and the decisions that are its own. +[`spreadsheet-editing.md`](spreadsheet-editing.md) is the sibling document for +the sheet view. Text documents, presentations and drawings share this one: +what it edits is runs and paragraphs, wherever the format puts them. + +Status: **the schema and the replay are landing; the browser editor follows.** +This document is written ahead of the code, and each section says what is in +and what is not. + +Scope of this work: an edit that spans several runs, a new paragraph, and a +delete or a replace that reaches across both. Inline formatting (bold, italic, +highlight) is *not* in it — decision 5 below says why the schema takes it later +without changing. + +## What the editor has to express + +A reader does four things the old `setText` could not say: + +1. **Type over a selection that spans two runs.** The selection starts in one + run and ends in another, so one `setText` cannot name it. +2. **Press Enter.** The paragraph splits, and everything after the caret moves + into a new paragraph. +3. **Press Backspace at the start of a paragraph.** The paragraph merges into + the one before it. +4. **Delete a selection that spans paragraphs.** Runs disappear, paragraphs + disappear, and the two ends become one paragraph. + +Every one of them creates or destroys elements, which is why the schema and the +adapters both change. + +## Decisions + +### 1. An op names an element by its id, not by its path + +`data-odr-path` is replaced by `data-odr-id`, carrying the +`ElementIdentifier` the registry already assigns. `Document::element_by_id` +turns it back into an `Element` at replay. This is decision 4 of +[`editing.md`](editing.md), now that there is something that needs it. + +**Why:** a `DocumentPath` is positional. The moment an op inserts a paragraph, +every path recorded after it in the same log names a different element, so a +log of more than one structural op cannot be replayed. Ids do not move. + +**Why it is safe:** an id has to hold for one `translate → edit → save`, and a +registry id is the index of a `std::deque` that only grows. The ops below +append and unlink; none of them renumbers. The sheet write side already creates +elements after the parse and leaves the old ones unreachable +([`odf/AGENTS.md`](../../src/odr/internal/odf/AGENTS.md)), so the discipline is +one the engines keep already. + +**The cost:** an id is meaningless outside the render that wrote it. A path was +readable and could be written by hand; an id cannot. `back_translate` replays +against a fresh decode of the same file, and a fresh decode of the same bytes +assigns the same ids, because parsing is deterministic. Nothing else read the +attribute. + +### 2. The wire carries no character offsets + +An op names whole elements and whole strings. There is no `(id, start, +length)`. A reader typing in the middle of a run produces `setText` with the +run's new text, not an insertion at an offset. + +**Why:** JavaScript counts a string in UTF-16 code units and `std::string` +counts bytes, so an offset on the wire needs a conversion on one side and a +rule about which side that is. An emoji, a combining accent and a `text:s` +run-of-spaces each make the two disagree. Nothing in the feature needs the +offset: the browser owns the model (decision 8 of [`editing.md`](editing.md)), +so it already knows the text each run ends up with, and handing that text over +is both shorter to write and impossible to misread. + +**What it costs:** the log is longer. Typing one character in the middle of a +long run sends the whole run. The log is coalesced before it is emitted +(decision 6 of [`editing.md`](editing.md)), so the length is per run and per +save, not per keystroke. + +**Why it does not paint us into a corner:** see decision 5. + +### 3. A split point is a run boundary, so `splitParagraph` needs no offset + +Enter in the middle of a run is three ops, not one: + +1. `setText` — the run keeps the text before the caret. +2. `insertText` — a new run after it holds the text after the caret. +3. `splitParagraph` — the paragraph splits after the first run. + +**Why:** it keeps decision 2, and it is what the file formats do anyway. ODF +and OOXML both represent a styled stretch of text as its own run, so a split +inside one *is* a split of the run followed by a split of the paragraph. Doing +it in that order makes the second step a pure move of whole children. + +### 4. A created element is addressed by a negative id + +An op that creates an element carries `"id": -1`, `-2`, … . A later op in the +same log names the created element by the same negative number. A positive id +is one the render wrote into the page. + +**Why an explicit number rather than a returned one:** replay stays a pure +function of the log. Returning minted ids to the browser is the round trip +architecture A exists to avoid (decision 1 of [`editing.md`](editing.md)). + +**Why negative rather than a reserved high range:** `ElementIdentifier` is 64 +bits and odf already spends the top bit on a positional cell id +([`odf/AGENTS.md`](../../src/odr/internal/odf/odf_element_registry.hpp)), so a +reserved range means an engine-by-engine collision argument. A sign has no such +argument to make, and the address stays one integer. + +Replay keeps a per-log map from the negative number to the id it minted. A +number used before it was created, or created twice, throws. + +### 5. Formatting fits this schema unchanged, which is why it is not in it yet + +Toggling bold on part of a run is, in both formats, "split the run, restyle the +middle one". The split is decision 3's first two ops, and what is left is one +op naming whole runs — `setMark {ids, mark, on}`. No offsets, no new +addressing. + +**Why it is not in this work:** the split is the same machinery either way, and +ODF reaches a mark through a named automatic style it may have to create, which +is a style-registry change with nothing to do with the ops. Landing it here +would double the size of the change for a feature nobody asked for yet. + +### 6. The browser applies the edit itself, so undo becomes ours + +The editor cancels `beforeinput` and mutates the DOM itself, rather than +letting the browser apply the edit and reading the run back afterwards +(decision 13 of [`editing.md`](editing.md) as it was first written). + +**Why it has to change:** reading the run back only works when the edit stayed +inside one run. Contenteditable's answer to a selection spanning two paragraphs +is browser-specific — a `
` wrapper here, a merged `` there — and none +of it maps onto the element tree. What we could read back afterwards would not +be what we would have to replay. + +**The consequence: the browser's undo stack goes empty**, because we cancel +every edit it was going to apply. So this work has to carry undo/redo, which +until now was honestly refused (`canUndo` answered false and a host's button +stayed grey). That is phase 3 item 2 of [`editing.md`](editing.md), and it +arrives here because it is no longer optional. + +### 7. Read-only engines say nothing + +The new adapter hooks default to throwing `UnsupportedOperation`, rather than +being pure virtual like `text_set_content`. + +**Why:** ten engines have a `TextAdapter` and two of them can write. A pure +virtual costs eight identical throwing bodies and grows every time the surface +does. `FrameAdapter` already defaults its three shape readers for the same +reason. + +## The op envelope + +Version **2**. Version 1 is refused rather than read: it addressed by path, and +a path in a version-2 world names the wrong element rather than none. + +```json +{"version": 2, "ops": [{"op": "setText", "id": 41, "text": "typed"}]} +``` + +| op | fields | what it does | +|---|---|---| +| `setText` | `id`, `text` | replaces the whole text of one run | +| `insertText` | `after` or `before`, `text`, `id` | a new run beside the named one, in the same parent, so it takes the same style | +| `removeElement` | `id` | unlinks the element and removes its nodes | +| `splitParagraph` | `paragraph`, `after` (optional), `id` | the children after `after` move into a new paragraph that copies the style; no `after` moves all of them | +| `mergeParagraph` | `paragraph` | takes the children of the next sibling paragraph and removes it | +| `insertParagraph` | `after`, `id` | a fresh empty paragraph after the named one, copying its style | +| `setCell` | `sheet`, `column`, `row`, `value` | unchanged; see [`spreadsheet-editing.md`](spreadsheet-editing.md) | + +Every `id` field on an op that creates an element is negative (decision 4). +Every other id is one the page wrote. + +### The four reader gestures, as ops + +**Type over a selection spanning two runs** — `a[bc` … `de]f` becoming `aXf`: + +```json +[{"op": "setText", "id": 10, "text": "aX"}, + {"op": "setText", "id": 11, "text": "f"}] +``` + +**Enter in the middle of a run** — decision 3: + +```json +[{"op": "setText", "id": 10, "text": "head"}, + {"op": "insertText", "after": 10, "text": "tail", "id": -1}, + {"op": "splitParagraph", "paragraph": 9, "after": 10, "id": -2}] +``` + +**Backspace at the start of a paragraph:** + +```json +[{"op": "mergeParagraph", "paragraph": 9}] +``` + +**Delete a selection spanning three paragraphs:** + +```json +[{"op": "setText", "id": 10, "text": "head"}, + {"op": "removeElement", "id": 11}, + {"op": "removeElement", "id": 20}, + {"op": "setText", "id": 31, "text": "tail"}, + {"op": "mergeParagraph", "paragraph": 9}, + {"op": "mergeParagraph", "paragraph": 9}] +``` + +## The adapter surface + +Alongside `TextAdapter::text_set_content`, all defaulting to +`UnsupportedOperation` (decision 7): + +```cpp +// ElementAdapter +virtual void element_remove(ElementIdentifier id) const; + +// TextAdapter +virtual ElementIdentifier text_insert(ElementIdentifier anchor_id, + Placement where, + const std::string &text) const; + +// ParagraphAdapter +virtual ElementIdentifier paragraph_split(ElementIdentifier id, + ElementIdentifier after_id) const; +virtual void paragraph_merge_next(ElementIdentifier id) const; +virtual ElementIdentifier paragraph_insert_after(ElementIdentifier id) const; +``` + +Each engine does the same three things it already does for a text edit: +**resolve the id to its registry entry, splice the pugixml subtree, fix up the +registry links.** Only the tag names differ — `text:p` / `text:span` against +`w:p` / `w:r` / `a:p` / `a:r`. + +The shared `internal::ElementRegistry` grows the links the structural ops need: +`unlink_child`, `insert_child_after` and `insert_child_before`. It has only +`append_child` today, because until now nothing built a tree except a parser +reading forward. + +## Which formats + +| Format | Engine | State | +|---|---|---| +| `.odt`, `.odp`, `.ods`, `.odg` | `odf` | edits and saves today; the new ops land here | +| `.docx` | `ooxml/text` | edits and saves today; the new ops land here | +| `.pptx` | `ooxml/presentation` | **read-only today.** It already has `text_set_content` and keeps its slide DOMs resident; what it lacks is `save`, the two flags and the capability row | +| everything else | — | read-only, and says so by decision 7 | + +`.odp` needs nothing of its own: a presentation is the same odf `Document` as a +text document, and a run inside a slide's frame is the same `text` element. + +## Order of work + +Each step is a pull request that builds and tests on its own. + +1. **Address by id.** `data-odr-id` on runs and paragraphs, + `Document::element_by_id`, `setText` by id, envelope version 2. +2. **Runs come and go.** `insertText` and `removeElement`, the registry links + they need, odf and ooxml text. A selection spanning runs is replayable. +3. **Paragraphs split and merge.** `splitParagraph`, `mergeParagraph`, + `insertParagraph`. +4. **The browser editor.** Model-first, owns the DOM mutation, records the ops, + and carries undo/redo (decision 6). +5. **pptx writes.** `save`, `is_editable`, `is_savable`, the capability row and + the new hooks over `a:p` / `a:r`. + +## Open questions + +- A run inside a **link** or a **bookmark** splits differently: splitting the + paragraph has to decide whether the link follows the tail. Today it would, + because the link is a child that moves whole. Whether that is right is a + question for step 3. +- **A list item** is a paragraph in a list. Enter at the end of one should make + a new list item, not a bare paragraph. Step 3 splits what the element tree + says is a paragraph; the list case is not covered. +- The **plain-text view** (`html/text_file.cpp`) is still its own editor and + still answers to nobody. Unchanged by this work, and still the open question + at the end of [`editing.md`](editing.md). diff --git a/docs/design/editing.md b/docs/design/editing.md index 1aa3e3340..c21c21a59 100644 --- a/docs/design/editing.md +++ b/docs/design/editing.md @@ -1,15 +1,16 @@ # Editing design -Status: **the mode frame is landed for every format; the text editor behind it -is not.** This records the architecture we chose for in-browser editing of ODF -and OOXML documents, the alternatives we weighed, and *why* we took each -decision. Decisions 9 to 12 are the frame every format shares, and they are in -the code. The phases below them are the text editor, and they are **not -scheduled yet** — they are captured here so the decisions and the grounding -survive until we pick the work up later. +Status: **the mode frame is landed for every format, and the text editor behind +it is being built.** This records the architecture we chose for in-browser +editing of ODF and OOXML documents, the alternatives we weighed, and *why* we +took each decision. Decisions 9 to 12 are the frame every format shares, and +they are in the code. [`spreadsheet-editing.md`](spreadsheet-editing.md) is the first editor built on the frame, and it is where a sheet's own decisions live. +[`document-editing.md`](document-editing.md) is the second, and it is where the phases +below are being carried out — an edit across runs, a new paragraph, and the +delete and replace that reach across both. This builds on the existing principle in [`README.md`](README.md): @@ -26,8 +27,8 @@ edits back into the original ODF/OOXML file. Today the frame is there and the text editor is not: - `html::translate(..., config.editable)` writes the editing scaffolding: the - page-level state on ``, `data-odr-path` on every editable run, and the - scripts that carry the mode (`internal/html/document_element.cpp`, + page-level state on ``, `data-odr-id` on every editable run and + paragraph, and the scripts that carry the mode (`internal/html/document_element.cpp`, `internal/html/document.cpp`). - `frontend/editing.js` owns `odr.editing` — the mode, the refusals, the log a save reads and the callbacks a host wires. Every format's editor attaches to @@ -236,7 +237,7 @@ editing: | `data-odr-editable="true" \| "readOnly"` | whether `enable()` can succeed at all | | `data-odr-keyboard="navigation shortcuts"` | which key classes the scripts may take (decision 12) | -Per element the page states only the exceptions: `data-odr-path` addresses an +Per element the page states only the exceptions: `data-odr-id` addresses an editable run, and `odr-locked` plus `data-odr-lock=""` marks what refuses. Everything unmarked is editable. @@ -272,7 +273,7 @@ than tracking what it rendered with, and `odr.generateDiff()` — the name the apps and the wasm package already call — never goes missing. What the flag keeps out of a read-only render is what actually costs: the -`data-odr-path` attribute on every editable run, the lock class on every locked +`data-odr-id` attribute on every editable run, the lock class on every locked cell, and the editor script (`document.js`, `sheet-editing.js`). The mode script itself is small and buys the host one API for every format. @@ -285,7 +286,7 @@ mode would be the only way to skip that line, and nothing needs it. **Why not drop the flag and always write the scaffolding:** a read-only host would carry the editor's script bytes and an attribute on every editable run for -nothing. `data-odr-path` on the runs of a text document is the expensive half, +nothing. `data-odr-id` on the runs of a text document is the expensive half, and it cannot be added later — the mode can only turn on if the addresses are already in the page. @@ -372,7 +373,7 @@ saved wrong. Refusing something we could have allowed costs a reader one gesture; allowing something we cannot replay costs them their document. **The address is the whole guard.** No element is marked non-editable: an edit is -allowed because it lands inside a `[data-odr-path]` run, so a picture, a table's +allowed because it lands inside a `x-s[data-odr-id]` run, so a picture, a table's furniture, the gap between two paragraphs and the page box are all refused without a single attribute of their own. That is decision 10's rule — mark the exceptions, not the rest — applied to the caret instead of to a cell. diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index 7d9059f62..3c6540347 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -50,7 +50,7 @@ results go stale the moment an input changes. | Formulas | `sheet_cell_value` | The expression is read and handed out as a string (step 0.1, landed); nothing parses or evaluates it. XLSX shows the cached ``, ODS the cached `text:p`. `xls` and `numbers` drop the expression at parse time | | Browser: sheet script | `html/frontend/spreadsheet.js` | Hover/pin, raise a clipped cell over its neighbours, sort rows in the DOM. Sorting reorders ``s, so a row's identity is its `` label, not its index. Publishes `odr.sheet` (step 1.1, landed), and the value and reflow half of it (steps 1.2/1.3, landed) | | Browser: the mode | `html/frontend/editing.js` | `odr.editing` — the mode, the refusal table, the log a save reads and the `odr.on*` callbacks, generic over every format. An editor attaches to it ([`editing.md`](editing.md) decision 9, step 1.6, landed) | -| Browser: text editor | `html/frontend/document.js` | The skeleton, attached to the mode: `contenteditable` runs keyed by `data-odr-path`, a `MutationObserver`, one `setText` op per changed run. No undo | +| Browser: text editor | `html/frontend/document.js` | The skeleton, attached to the mode: the whole view editable, runs keyed by `data-odr-id`, one `setText` op per changed run. No undo | | Browser: sheet editor | `html/frontend/sheet-editing.js` | The cell overlay, the locks and the position map (steps 1.1 to 1.4, landed), attached to the mode as one editor | | Wire format | `document.cpp::Document::edit` | The op envelope, `setCell` and `setText` (step 0.4, landed) | | Addressing | `DocumentPath` | Already spells a cell by position: `/child:0/cell:A1/...` | @@ -78,7 +78,7 @@ value beside the op. The whole log is idempotent, which decision 5 leans on. ```json { - "version": 1, + "version": 2, "ops": [ {"op": "setCell", "sheet": 0, "column": 1, "row": 2, "value": {"type": "number", "number": 12.5, "text": "12.5"}}, @@ -94,10 +94,10 @@ value beside the op. The whole log is idempotent, which decision 5 leans on. op it cannot apply, leaving the ones before it applied — a document is decoded fresh by `DocumentFile::document()`, so the host replays onto a copy by construction; the wasm session, which holds one document, has to replay onto a -fresh decode too. `setText {path, text}` carries what `modifiedText` carried -and is what `generateDiff()` now emits; it gains the id form when -[`editing.md`](editing.md) phase 1 lands. The bindings pass a string through -and did not change. +fresh decode too. `setText {id, text}` carries what `modifiedText` carried and +is what `generateDiff()` now emits, addressing its run by the id the page +states ([`document-editing.md`](document-editing.md) decision 1). The bindings pass a +string through and did not change. `version` is the wire version. A document stamp (decision 7 in `editing.md`) is deferred: a sheet op names a position, and a position is meaningful against diff --git a/jni/java/app/opendocument/core/Element.java b/jni/java/app/opendocument/core/Element.java index dc6d13c8f..ec540de32 100644 --- a/jni/java/app/opendocument/core/Element.java +++ b/jni/java/app/opendocument/core/Element.java @@ -56,6 +56,11 @@ public boolean isSame(Element other) { } } + /** The address an edit operation names this element by. */ + public long identifier() { + return identifierNative(handle()); + } + public DocumentPath documentPath() { return new DocumentPath(documentPathNative(handle())); } @@ -208,6 +213,8 @@ final List wrapAll(long[] handles) { private native boolean isSameNative(long handle, long otherHandle); + private native long identifierNative(long handle); + private native long documentPathNative(long handle); private native long navigatePathNative(long handle, long pathHandle); diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 780061058..2a7a99b9b 100644 --- a/jni/src/jni_document.cpp +++ b/jni/src/jni_document.cpp @@ -222,6 +222,13 @@ extern "C" JNIEXPORT jint JNICALL Java_app_opendocument_core_Element_typeNative( [&] { return static_cast(element(handle).type()); }); } +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Element_identifierNative(JNIEnv *env, jobject, + jlong handle) { + return guarded( + env, [&] { return static_cast(element(handle).identifier()); }); +} + extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_Element_parentNative(JNIEnv *env, jobject, jlong handle) { diff --git a/jni/tests/app/opendocument/core/DocumentTest.java b/jni/tests/app/opendocument/core/DocumentTest.java index 61f843ba0..f17f0715e 100644 --- a/jni/tests/app/opendocument/core/DocumentTest.java +++ b/jni/tests/app/opendocument/core/DocumentTest.java @@ -98,10 +98,10 @@ void editAppliesADiff() throws IOException { assertTrue(document.isEditable()); Element paragraph = document.rootElement().firstChild(); - DocumentPath text = paragraph.firstChild().documentPath(); + long text = paragraph.firstChild().identifier(); - document.edit("{\"version\":1,\"ops\":[{\"op\":\"setText\",\"path\":\"" - + text + "\",\"text\":\"edited by the diff\"}]}"); + document.edit("{\"version\":2,\"ops\":[{\"op\":\"setText\",\"id\":" + + text + ",\"text\":\"edited by the diff\"}]}"); assertTrue(walkText(document.rootElement()).contains("edited by the diff")); } @@ -111,9 +111,9 @@ void saveToMemoryRoundTripsAnEdit() throws IOException { Document document = openDocument(); Element paragraph = document.rootElement().firstChild(); - DocumentPath text = paragraph.firstChild().documentPath(); - document.edit("{\"version\":1,\"ops\":[{\"op\":\"setText\",\"path\":\"" - + text + "\",\"text\":\"saved to memory\"}]}"); + long text = paragraph.firstChild().identifier(); + document.edit("{\"version\":2,\"ops\":[{\"op\":\"setText\",\"id\":" + + text + ",\"text\":\"saved to memory\"}]}"); byte[] saved = document.saveToMemory(); assertTrue(saved.length > 0); diff --git a/python/src/bind_document.cpp b/python/src/bind_document.cpp index 38ab64741..dd7839e0f 100644 --- a/python/src/bind_document.cpp +++ b/python/src/bind_document.cpp @@ -146,6 +146,7 @@ void odr_python::bind_document(py::module_ &m) { }, py::is_operator()) .def("type", &odr::Element::type) + .def("identifier", &odr::Element::identifier) .def("parent", &odr::Element::parent, keep_self_alive) .def("first_child", &odr::Element::first_child, keep_self_alive) .def("previous_sibling", &odr::Element::previous_sibling, keep_self_alive) @@ -322,6 +323,8 @@ void odr_python::bind_document(py::module_ &m) { }, py::arg("operations"), "Apply the operations our browser-side editor produces.") + .def("element_by_id", &odr::Document::element_by_id, + py::arg("identifier"), keep_self_alive) .def("is_savable", &odr::Document::is_savable, py::arg("encrypted") = false) // saving serialises the whole document; holding the GIL for it blocks diff --git a/python/tests/test_document.py b/python/tests/test_document.py index 093bcd448..6b2359d02 100644 --- a/python/tests/test_document.py +++ b/python/tests/test_document.py @@ -136,9 +136,10 @@ def test_save_to_memory_round_trips(odt_path, tmp_path): def test_save_to_memory_carries_an_edit(odt_path, tmp_path): document = pyodr.open(str(odt_path)).as_document_file().document() + run = document.root_element().first_child().first_child() diff = ( - '{"version":1,"ops":[{"op":"setText","path":"/child:0/child:0",' - '"text":"edited in python"}]}' + '{"version":2,"ops":[{"op":"setText","id":%d,' + '"text":"edited in python"}]}' % run.identifier() ) document.edit(diff) diff --git a/src/odr/document.cpp b/src/odr/document.cpp index c876903de..302214867 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -1,7 +1,6 @@ #include #include -#include #include #include #include @@ -124,10 +123,22 @@ Sheet sheet_at(const Element root, const std::uint32_t ordinal) { void Document::edit(const std::string_view operations, const Logger & /*logger*/) const { const nlohmann::json json = nlohmann::json::parse(operations); - if (json.value("version", 0) != 1) { + if (json.value("version", 0) != 2) { throw std::invalid_argument("unsupported edit version"); } + // the element @p field names, checked to be one this document holds + const auto element_of = [&](const nlohmann::json &operation, + const char *field) { + const auto identifier = operation.at(field).get(); + const Element element = element_by_id(identifier); + if (!element) { + throw std::invalid_argument("element " + std::to_string(identifier) + + " not found"); + } + return element; + }; + for (const nlohmann::json &operation : json.at("ops")) { const auto name = operation.at("op").get(); @@ -140,16 +151,14 @@ void Document::edit(const std::string_view operations, } if (name == "setText") { - const auto path = operation.at("path").get(); - const Element element = root_element().navigate_path(DocumentPath(path)); - if (!element) { - throw std::invalid_argument("element with path " + path + " not found"); - } - if (!element.as_text()) { - throw std::invalid_argument("element with path " + path + + const Element element = element_of(operation, "id"); + const Text text = element.as_text(); + if (!text) { + throw std::invalid_argument("element " + + std::to_string(element.identifier()) + " is not a text element"); } - element.as_text().set_content(operation.at("text").get()); + text.set_content(operation.at("text").get()); continue; } @@ -161,6 +170,17 @@ Element Document::root_element() const { return {m_impl->element_adapter(), m_impl->root_element()}; } +Element Document::element_by_id(const ElementIdentifier identifier) const { + const internal::abstract::ElementAdapter *adapter = m_impl->element_adapter(); + try { + // throwing is how a registry answers an id it does not hold + static_cast(adapter->element_type(identifier)); + } catch (const std::out_of_range &) { + return {}; + } + return {adapter, identifier}; +} + Filesystem Document::as_filesystem() const { if (std::shared_ptr files = m_impl->as_filesystem()) { diff --git a/src/odr/document.hpp b/src/odr/document.hpp index 69e942096..c286896e6 100644 --- a/src/odr/document.hpp +++ b/src/odr/document.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include @@ -46,11 +47,12 @@ class Document final { /// @brief Applies @p operations to the document, in order. /// /// The wire format our browser-side editor produces: - /// `{"version": 1, "ops": [{"op": "setCell", "sheet": 0, "column": 1, + /// `{"version": 2, "ops": [{"op": "setCell", "sheet": 0, "column": 1, /// "row": 2, "value": {"type": "number", "number": 12.5, "text": "12.5"}}]}`. /// A value is typed `number`, `string` or `empty`; `setText` names a text - /// element by `path` instead. Editing a single element in process is - /// @ref Text::set_content and needs none of this. + /// element by the `id` the render wrote into the page instead + /// (`docs/design/document-editing.md`). Editing a single element in process + /// is @ref Text::set_content and needs none of this. /// @throws std::invalid_argument on the first operation it cannot apply, /// leaving the ones before it applied - a host replays onto a fresh /// decode. @@ -59,6 +61,10 @@ class Document final { [[nodiscard]] Element root_element() const; + /// The element @ref Element::identifier handed out, or one that does not + /// exist where this document holds no such id. + [[nodiscard]] Element element_by_id(ElementIdentifier identifier) const; + /// The files the document is packaged from; empty for a document that is /// one file. [[nodiscard]] Filesystem as_filesystem() const; diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index 362fd7d68..6ac8129ef 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -106,6 +106,8 @@ ElementType Element::type() const { return exists_() ? m_adapter->element_type(m_identifier) : ElementType::none; } +ElementIdentifier Element::identifier() const noexcept { return m_identifier; } + Element Element::parent() const { return exists_() ? Element(m_adapter, m_adapter->element_parent(m_identifier)) : Element(); diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index 0ee81e353..fdb55b0ee 100644 --- a/src/odr/document_element.hpp +++ b/src/odr/document_element.hpp @@ -205,6 +205,10 @@ class Element { [[nodiscard]] ElementType type() const; + /// The address an edit operation names this element by; unique within one + /// document, and the same for every decode of the same bytes. + [[nodiscard]] ElementIdentifier identifier() const noexcept; + [[nodiscard]] Element parent() const; [[nodiscard]] Element first_child() const; [[nodiscard]] Element previous_sibling() const; diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index 2c95c39b0..eacf3811f 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -241,6 +241,15 @@ bool writes_edit_markup(const Element &element, element.is_editable(); } +/// The address an edit operation names @p element by, where the render offers +/// editing at all. +void write_edit_address(const Element &element, const html::WritingState &state, + const html::HtmlAttributeWriterCallback &clb) { + if (writes_edit_markup(element, state)) { + clb("data-odr-id", std::to_string(element.identifier())); + } +} + /// 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, @@ -778,9 +787,7 @@ void html::translate_text(const Element &element, const WritingState &state) { HtmlElementOptions() .set_inline(true) .set_attributes([&](const HtmlAttributeWriterCallback &clb) { - if (writes_edit_markup(element, state)) { - clb("data-odr-path", element.document_path().to_string()); - } + write_edit_address(element, state, clb); }) .set_style(translate_text_style(text.style()), state.styles())); state.out().out() << escape_text(text.content()); @@ -817,11 +824,16 @@ void html::translate_paragraph(const Element &element, state.out().write_element_begin( "x-p", - HtmlElementOptions().set_inline(true).set_style( - "display:block;" + - translate_paragraph_style(paragraph.style(), state.direction()) + - translate_block_font_style(paragraph.text_style()), - state.styles())); + HtmlElementOptions() + .set_inline(true) + .set_attributes([&](const HtmlAttributeWriterCallback &clb) { + write_edit_address(element, state, clb); + }) + .set_style("display:block;" + + translate_paragraph_style(paragraph.style(), + state.direction()) + + translate_block_font_style(paragraph.text_style()), + state.styles())); if (!marker.empty()) { state.out().write_element_begin( "x-s", HtmlElementOptions() diff --git a/src/odr/internal/html/frontend/document.js b/src/odr/internal/html/frontend/document.js index 422370c8a..2781a1f85 100644 --- a/src/odr/internal/html/frontend/document.js +++ b/src/odr/internal/html/frontend/document.js @@ -7,24 +7,28 @@ var odr = (window.odr = window.odr || {}); // A view whose runs carry no address is not this editor's: a sheet's editing - // is an overlay, and its cells state no path. - if (document.querySelector("[data-odr-path]") === null) { + // is an overlay, and its cells state no id. + if (document.querySelector("x-s[data-odr-id]") === null) { return; } var root = document.body; var modified = {}; + function idOf(run) { + return Number(run.getAttribute("data-odr-id")); + } + function operations() { var ops = []; - for (var path in modified) { - if (Object.prototype.hasOwnProperty.call(modified, path)) { + for (var id in modified) { + if (Object.prototype.hasOwnProperty.call(modified, id)) { ops.push({ op: "setText", - path: path, + id: Number(id), // Not `innerText`: that is the rendered text, and it drops the // trailing space a reader just typed. - text: modified[path].textContent, + text: modified[id].textContent, }); } } @@ -37,7 +41,7 @@ return null; } var element = node.nodeType === 1 ? node : node.parentElement; - return element === null ? null : element.closest("[data-odr-path]"); + return element === null ? null : element.closest("x-s[data-odr-id]"); } // The input types that only ever change the text of one run. @@ -64,7 +68,7 @@ var named = { insertParagraph: "newLine", insertLineBreak: "newLine" }; /// Where an edit lands: `run` is the one run it is confined to, null where - /// it spans two or lands outside every run. `path` is where it starts. + /// it spans two or lands outside every run. `id` is where it starts. function target(event) { var ranges = typeof event.getTargetRanges === "function" ? event.getTargetRanges() : []; @@ -78,12 +82,12 @@ } } if (range === undefined) { - return { run: null, path: null }; + return { run: null, id: null }; } var start = runOf(range.startContainer); return { run: start !== null && start === runOf(range.endContainer) ? start : null, - path: start === null ? null : start.getAttribute("data-odr-path"), + id: start === null ? null : idOf(start), }; } @@ -91,9 +95,9 @@ if (event.cancelable) { event.preventDefault(); } - // The path keeps two refusals apart, so Enter in one run and then in + // The id keeps two refusals apart, so Enter in one run and then in // another is heard twice. - odr.editing.refuse(reason, { path: at.path }); + odr.editing.refuse(reason, { id: at.id }); } // Where the edit the gate just allowed will land, for `input` to record. @@ -164,7 +168,7 @@ odr.onError(9, "an edit landed where no operation can name it"); return; } - modified[run.getAttribute("data-odr-path")] = run; + modified[idOf(run)] = run; odr.editing.changed(); }); diff --git a/test/browser/text/tests.html b/test/browser/text/tests.html index 0d0141979..8d4060f15 100644 --- a/test/browser/text/tests.html +++ b/test/browser/text/tests.html @@ -15,18 +15,18 @@ a run under a style-only wrapper, and a paragraph holding a picture and no run at all. -->
- first run first run a link and a taila link and a tail - boldbold - + +#include #include #include #include @@ -42,12 +43,17 @@ Sheet first_sheet(const Document &document) { return (*document.root_element().children().begin()).as_sheet(); } +/// The one run the @p column -th cell of the first row holds. +Element run_of_cell(const Document &document, const std::uint32_t column) { + return first_sheet(document).cell(column, 0).first_child().first_child(); +} + } // namespace TEST(DocumentEdit, the_ops_are_applied_in_order) { const Document document = three_cell_sheet(); - document.edit(R"({"version":1,"ops":[)" + document.edit(R"({"version":2,"ops":[)" R"({"op":"setCell","sheet":0,"column":0,"row":0,)" R"("value":{"type":"string","text":"first"}},)" R"({"op":"setCell","sheet":0,"column":0,"row":0,)" @@ -59,7 +65,7 @@ TEST(DocumentEdit, the_ops_are_applied_in_order) { TEST(DocumentEdit, a_number_op_states_the_number_and_the_text) { const Document document = three_cell_sheet(); - document.edit(R"({"version":1,"ops":[)" + document.edit(R"({"version":2,"ops":[)" R"({"op":"setCell","sheet":0,"column":1,"row":0,)" R"("value":{"type":"number","number":12.5,"text":"12,50"}}]})"); @@ -74,7 +80,7 @@ TEST(DocumentEdit, a_number_op_states_the_number_and_the_text) { TEST(DocumentEdit, a_number_op_without_text_spells_itself) { const Document document = three_cell_sheet(); - document.edit(R"({"version":1,"ops":[)" + document.edit(R"({"version":2,"ops":[)" R"({"op":"setCell","sheet":0,"column":1,"row":0,)" R"("value":{"type":"number","number":12.5}}]})"); @@ -84,27 +90,49 @@ TEST(DocumentEdit, a_number_op_without_text_spells_itself) { TEST(DocumentEdit, an_empty_op_clears_the_cell) { const Document document = three_cell_sheet(); - document.edit(R"({"version":1,"ops":[)" + document.edit(R"({"version":2,"ops":[)" R"({"op":"setCell","sheet":0,"column":2,"row":0,)" R"("value":{"type":"empty"}}]})"); EXPECT_EQ(first_sheet(document).cell(2, 0).value().text(), ""); } -TEST(DocumentEdit, a_text_op_names_its_element_by_path) { +TEST(DocumentEdit, a_text_op_names_its_element_by_id) { const Document document = three_cell_sheet(); + const Element run = run_of_cell(document, 0); - document.edit( - R"({"version":1,"ops":[{"op":"setText",)" - R"("path":"/child:0/cell:A1/child:0/child:0","text":"typed"}]})"); + document.edit(R"({"version":2,"ops":[{"op":"setText","id":)" + + std::to_string(run.identifier()) + R"(,"text":"typed"}]})"); EXPECT_EQ(first_sheet(document).cell(0, 0).value().text(), "typed"); } +TEST(DocumentEdit, a_text_op_naming_an_element_that_is_not_there_refuses) { + const Document document = three_cell_sheet(); + + EXPECT_THROW(document.edit(R"({"version":2,"ops":[)" + R"({"op":"setText","id":9999,"text":"typed"}]})"), + std::invalid_argument); + EXPECT_THROW(document.edit(R"({"version":2,"ops":[)" + R"({"op":"setText","id":0,"text":"typed"}]})"), + std::invalid_argument); +} + +TEST(DocumentEdit, a_text_op_naming_something_that_is_not_a_run_refuses) { + const Document document = three_cell_sheet(); + const ElementIdentifier cell = first_sheet(document).cell(0, 0).identifier(); + + EXPECT_THROW(document.edit(R"({"version":2,"ops":[{"op":"setText","id":)" + + std::to_string(cell) + R"(,"text":"typed"}]})"), + std::invalid_argument); +} + TEST(DocumentEdit, an_unknown_version_refuses) { const Document document = three_cell_sheet(); - EXPECT_THROW(document.edit(R"({"version":2,"ops":[]})"), + EXPECT_THROW(document.edit(R"({"version":1,"ops":[]})"), + std::invalid_argument); + EXPECT_THROW(document.edit(R"({"version":3,"ops":[]})"), std::invalid_argument); EXPECT_THROW(document.edit(R"({"ops":[]})"), std::invalid_argument); } @@ -112,14 +140,14 @@ TEST(DocumentEdit, an_unknown_version_refuses) { TEST(DocumentEdit, an_unknown_op_refuses) { const Document document = three_cell_sheet(); - EXPECT_THROW(document.edit(R"({"version":1,"ops":[{"op":"setStyle"}]})"), + EXPECT_THROW(document.edit(R"({"version":2,"ops":[{"op":"setStyle"}]})"), std::invalid_argument); } TEST(DocumentEdit, an_op_naming_a_sheet_that_is_not_there_refuses) { const Document document = three_cell_sheet(); - EXPECT_THROW(document.edit(R"({"version":1,"ops":[)" + EXPECT_THROW(document.edit(R"({"version":2,"ops":[)" R"({"op":"setCell","sheet":3,"column":0,"row":0,)" R"("value":{"type":"empty"}}]})"), std::invalid_argument); @@ -130,7 +158,7 @@ TEST(DocumentEdit, the_ops_before_a_refusal_are_applied) { const Document document = three_cell_sheet(); EXPECT_ANY_THROW( - document.edit(R"({"version":1,"ops":[)" + document.edit(R"({"version":2,"ops":[)" R"({"op":"setCell","sheet":0,"column":0,"row":0,)" R"("value":{"type":"string","text":"written"}},)" R"({"op":"setCell","sheet":9,"column":0,"row":0,)" diff --git a/test/src/document_test.cpp b/test/src/document_test.cpp index 4ac62dc3e..b3c14a085 100644 --- a/test/src/document_test.cpp +++ b/test/src/document_test.cpp @@ -11,10 +11,14 @@ #include +#include + #include #include #include #include +#include +#include using namespace odr; using namespace odr::test; @@ -66,9 +70,25 @@ void expect_text_at(const Document &document, const std::string &path, .content()); } -/// Applies `diff` to `path`'s document, saves to `output_name` in the working +using TextEdits = std::vector>; + +/// One `setText` op per @p edits entry. A test states the path because that is +/// what a reader can check; an op states the id the element carries. +std::string set_text_ops(const Document &document, const TextEdits &edits) { + nlohmann::json ops = nlohmann::json::array(); + for (const auto &[path, text] : edits) { + const Element element = + document.root_element().navigate_path(DocumentPath(path)); + EXPECT_TRUE(element) << "no element at " << path; + ops.push_back( + {{"op", "setText"}, {"id", element.identifier()}, {"text", text}}); + } + return nlohmann::json{{"version", 2}, {"ops", ops}}.dump(); +} + +/// Applies @p edits to `path`'s document, saves to `output_name` in the working /// directory and reopens it, so the assertions see what was written. -Document edit_and_reload(const std::string &path, const char *diff, +Document edit_and_reload(const std::string &path, const TextEdits &edits, const std::string &output_name) { const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); @@ -76,7 +96,7 @@ Document edit_and_reload(const std::string &path, const char *diff, open(TestData::test_file_path(path), {}, logger).as_document_file(); const Document document = document_file.document(); - document.edit(diff); + document.edit(set_text_ops(document, edits)); const std::string output_path = (std::filesystem::current_path() / output_name).string(); @@ -349,10 +369,11 @@ TEST(Document, edit_docx) { } TEST(Document, edit_odt_diff) { - const char *diff = - R"({"version":1,"ops":[{"op":"setText","path":"/child:16/child:0","text":"Outasdfsdafdline"},{"op":"setText","path":"/child:24/child:0","text":"Colorasdfasdfasdfed Line"},{"op":"setText","path":"/child:6/child:0","text":"Text hello world!"}]})"; const Document document = - edit_and_reload("odr-public/odt/style-various-1.odt", diff, + edit_and_reload("odr-public/odt/style-various-1.odt", + {{"/child:16/child:0", "Outasdfsdafdline"}, + {"/child:24/child:0", "Colorasdfasdfasdfed Line"}, + {"/child:6/child:0", "Text hello world!"}}, "style-various-1_edit_diff.odt"); expect_text_at(document, "/child:16/child:0", "Outasdfsdafdline"); @@ -363,11 +384,14 @@ TEST(Document, edit_odt_diff) { // Asserted in memory: `pages.ods` is password-protected, and a decrypted // package is not savable — see `a_decrypted_package_is_not_savable`. TEST(Document, edit_ods_diff) { - const char *diff = - R"({"version":1,"ops":[{"op":"setText","path":"/child:0/cell:A1/child:0/child:0","text":"Page 1 hi"},{"op":"setText","path":"/child:1/cell:A1/child:0/child:0","text":"Page 2 hihi"},{"op":"setText","path":"/child:2/cell:A1/child:0/child:0","text":"Page 3 hihihi"},{"op":"setText","path":"/child:3/cell:A1/child:0/child:0","text":"Page 4 hihihihi"},{"op":"setText","path":"/child:4/cell:A1/child:0/child:0","text":"Page 5 hihihihihi"}]})"; const Document document = decrypted_pages_ods(); - document.edit(diff); + document.edit(set_text_ops( + document, {{"/child:0/cell:A1/child:0/child:0", "Page 1 hi"}, + {"/child:1/cell:A1/child:0/child:0", "Page 2 hihi"}, + {"/child:2/cell:A1/child:0/child:0", "Page 3 hihihi"}, + {"/child:3/cell:A1/child:0/child:0", "Page 4 hihihihi"}, + {"/child:4/cell:A1/child:0/child:0", "Page 5 hihihihihi"}})); expect_text_at(document, "/child:0/cell:A1/child:0/child:0", "Page 1 hi"); expect_text_at(document, "/child:1/cell:A1/child:0/child:0", "Page 2 hihi"); @@ -397,11 +421,12 @@ TEST(Document, a_decrypted_package_is_not_savable) { } TEST(Document, edit_docx_diff) { - const char *diff = - R"({"version":1,"ops":[{"op":"setText","path":"/child:16/child:0/child:0","text":"Outasdfsdafdline"},{"op":"setText","path":"/child:24/child:0/child:0","text":"Colorasdfasdfasdfed Line"},{"op":"setText","path":"/child:6/child:0/child:0","text":"Text hello world!"}]})"; - const Document document = - edit_and_reload("odr-public/docx/style-various-1.docx", diff, - "style-various-1_edit_diff.docx"); + const Document document = edit_and_reload( + "odr-public/docx/style-various-1.docx", + {{"/child:16/child:0/child:0", "Outasdfsdafdline"}, + {"/child:24/child:0/child:0", "Colorasdfasdfasdfed Line"}, + {"/child:6/child:0/child:0", "Text hello world!"}}, + "style-various-1_edit_diff.docx"); expect_text_at(document, "/child:16/child:0/child:0", "Outasdfsdafdline"); expect_text_at(document, "/child:24/child:0/child:0", diff --git a/test/src/html_test.cpp b/test/src/html_test.cpp index 5839dee81..d56d46a98 100644 --- a/test/src/html_test.cpp +++ b/test/src/html_test.cpp @@ -208,6 +208,24 @@ std::string render_odt(const HtmlConfig &config) { return render("odr-public/odt/about.odt", config); } +/// Whether any `<@p tag …>` in @p page states @p attribute. The writer puts +/// class and style first, so the two are not adjacent. +bool a_tag_states(const std::string &page, const std::string &tag, + const std::string &attribute) { + const std::string open = "<" + tag + " "; + for (std::size_t at = page.find(open); at != std::string::npos; + at = page.find(open, at + 1)) { + const std::size_t end = page.find('>', at); + if (end == std::string::npos) { + return false; + } + if (page.find(attribute, at) < end) { + return true; + } + } + return false; +} + } // namespace // Reflowed to the viewport there is no page box to inset the text. @@ -976,20 +994,26 @@ TEST(html, a_plain_cell_carries_no_lock) { EXPECT_EQ(page.find(R"(class="odr-locked")"), std::string::npos); } -// An op names a run, so the markup addresses one. The mode writes the -// `contenteditable`, so one page serves both modes. +// The mode writes the `contenteditable`, so one page serves both modes. TEST(html, an_editable_text_document_addresses_its_runs) { const std::string page = render_odt(editing_config()); - EXPECT_NE(page.find("data-odr-path"), std::string::npos); + EXPECT_TRUE(a_tag_states(page, "x-s", "data-odr-id=")); EXPECT_EQ(page.find(R"(contenteditable="true")"), std::string::npos); } +// A paragraph is what a split or an insert anchors on. +TEST(html, an_editable_text_document_addresses_its_paragraphs) { + const std::string page = render_odt(editing_config()); + + EXPECT_TRUE(a_tag_states(page, "x-p", "data-odr-id=")); +} + // The address is the expensive half, and a read-only render pays none of it. TEST(html, a_read_only_text_document_addresses_no_run) { const std::string page = render_odt(HtmlConfig()); - EXPECT_EQ(page.find("data-odr-path"), std::string::npos); + EXPECT_EQ(page.find("data-odr-id"), std::string::npos); } // #822: a sheet cell does not break its text into lines unless the file says diff --git a/wasm/tests/edit.test.mjs b/wasm/tests/edit.test.mjs index ecd73b15b..b4415dc08 100644 --- a/wasm/tests/edit.test.mjs +++ b/wasm/tests/edit.test.mjs @@ -5,11 +5,12 @@ import { after, before, describe, it } from 'node:test'; import { Odr, OdrError, minimalOds, minimalOdt } from './helper.mjs'; -// Read out of the html rather than spelled, as the browser does. -function firstEditablePath(html) { - const match = html.match(/data-odr-path="([^"]+)"/); - assert.ok(match, 'the editable render carries no data-odr-path'); - return match[1]; +// Read out of the html rather than spelled, as the browser does. The runs +// carry the ids an op names; a paragraph carries one too, so the tag counts. +function firstEditableRunId(html) { + const match = html.match(/]*data-odr-id="(\d+)"/); + assert.ok(match, 'the editable render carries no run id'); + return Number(match[1]); } describe('edit', () => { @@ -34,10 +35,10 @@ describe('edit', () => { const doc = odr.open(minimalOdt('hello'), { editable: true }); try { const { html } = doc.render(0); - const path = firstEditablePath(html); + const id = firstEditableRunId(html); doc.edit(JSON.stringify({ - version: 1, - ops: [{ op: 'setText', path, text: 'edited in the browser' }], + version: 2, + ops: [{ op: 'setText', id, text: 'edited in the browser' }], })); // the edit is in the document, so the same service renders it @@ -64,7 +65,7 @@ describe('edit', () => { const doc = odr.open(minimalOds('hello')); try { doc.edit(JSON.stringify({ - version: 1, + version: 2, ops: [{ op: 'setCell', sheet: 0, column: 0, row: 0, value: { type: 'number', number: 12.5, text: '12.5' }, @@ -100,7 +101,7 @@ describe('edit', () => { assert.equal(error.name, 'NoDocumentFile'); return true; }); - assert.throws(() => doc.edit('{"version":1,"ops":[]}'), OdrError); + assert.throws(() => doc.edit('{"version":2,"ops":[]}'), OdrError); } finally { doc.close(); } diff --git a/wasm/tests/render.test.mjs b/wasm/tests/render.test.mjs index 04a52d74b..d891b074c 100644 --- a/wasm/tests/render.test.mjs +++ b/wasm/tests/render.test.mjs @@ -102,8 +102,8 @@ describe('render', () => { try { // `editable` writes the address the mode needs; the mode writes the // `contenteditable`. - assert.ok(!plain.render(0).html.includes('data-odr-path')); - assert.ok(editable.render(0).html.includes('data-odr-path')); + assert.ok(!plain.render(0).html.includes('data-odr-id')); + assert.ok(editable.render(0).html.includes('data-odr-id')); } finally { plain.close(); editable.close();