Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ The release run heads these entries with the version and opens a fresh

## Unreleased

- A text document is edited the way a reader expects: typing, replacing and
deleting across runs and paragraphs, Enter, Backspace at a paragraph start,
and a plain-text paste that opens a paragraph per line.

- **Undo and redo are the editor's.** `odr.editing.undo()` / `redo()` answer
for a text document, `odr.onEditChange` reports `canUndo` / `canRedo`
truthfully, and ctrl/cmd+Z is taken.

- Enter is no longer refused. Reason `newLine` (code 1) now marks only a soft
line break, which no operation carries.

- A paragraph splits, merges and is inserted — `splitParagraph`,
`mergeParagraph`, `insertParagraph`, and the matching `Document` methods in
C++. Enter, Backspace at a paragraph start and a delete across paragraphs.
Expand Down
50 changes: 47 additions & 3 deletions docs/design/document-editing.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,27 @@ be what we would have to replay.
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.
arrives here because it is no longer optional. One `beforeinput` is one undo
step; a browser coalesces a word, and matching that is a later refinement.

### 6b. The page is the model, because the editor is the only one writing it

Decision 8 of [`editing.md`](editing.md) called for a structured model beside
the page, with the DOM as its projection. The editor keeps no such second
structure: the runs and paragraphs are addressed in the page by
`data-odr-id`, and **that is the model**.

**Why the second structure bought nothing:** what decision 8 was protecting
against is contenteditable inventing markup we cannot map back. Owning the
mutation removes that at the source — nothing but this editor writes the page,
so the page cannot drift into a shape the element tree has no name for. A
parallel model would have to be kept in step with the page anyway, and the
place the two could disagree is exactly the bug it was meant to catch.

**Where it earns its keep:** a composition cannot be cancelled, so the browser
*does* write inside a run. With the page as the model there is nothing to
reconcile — `compositionend` reads the run's text and that is the operation.
With a parallel model that same case would be a merge.

### 7. Read-only engines say nothing

Expand Down Expand Up @@ -272,8 +292,8 @@ Each step is a pull request that builds and tests on its own.
**Landed.**
3. **Paragraphs split and merge.** `splitParagraph`, `mergeParagraph`,
`insertParagraph`. **Landed.**
4. **The browser editor.** Model-first, owns the DOM mutation, records the ops,
and carries undo/redo (decision 6).
4. **The browser editor.** Owns the DOM mutation, records the ops, and carries
undo/redo (decisions 6 and 6b). **Landed.**
5. **pptx writes.** `save`, `is_editable`, `is_savable`, the capability row and
the new hooks over `a:p` / `a:r`.

Expand Down Expand Up @@ -302,6 +322,30 @@ behind. It is valid in both formats — the corpus is full of `<w:r><w:rPr/></w:
that producers wrote themselves — and pruning it would cost a branch to save
nothing a reader sees.

## What the editor does with a keystroke

Every edit is one of two shapes, and both come out of one function:

- **A range replaced by some text.** Typing, replacing a selection, every
delete, and each line of a paste. Inside one run it is a `setText`; across
runs it is a `setText` on each end and a `removeElement` between; across
paragraphs it is that plus a `mergeParagraph`.
- **A split where the caret sits.** Enter, and every line break in a paste.
The run is cut in two first (decision 3) unless the caret is already at a
run boundary.

Two details the checks pin down:

- **A delete whose range the browser did not state is one character**, in the
direction the key names — or, at the start of a paragraph, the boundary
itself, which merges and takes no character. A browser normally states the
range; an Android WebView is reported not to. The *word* and *line* deletes
are not extended this way: guessing where a word ends would take away text
the reader did not name, so nothing happens.
- **The line box is kept the way a fresh render writes it** — `<br>` where a
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.

## Open questions

- **A list item** is a paragraph in a list. Enter at the end of one should make
Expand Down
86 changes: 37 additions & 49 deletions docs/design/editing.md
Original file line number Diff line number Diff line change
Expand Up @@ -342,15 +342,18 @@ attribute.
### 13. One editable view, and every edit it cannot replay is refused

`enable()` puts `contenteditable` on the **body**, not on each run. The editor
then intercepts `beforeinput` and refuses everything that is not the text of one
addressed run:
then intercepts `beforeinput` and takes the edits it can express as operations:

| The edit | What happens |
|---|---|
| text typed, replaced, pasted plain, deleted, composed — inside one run | allowed, and the run joins the log |
| a new line (`insertParagraph`, `insertLineBreak`) | refused, reason `newLine` |
| text typed, replaced, deleted — inside one run, across runs, across paragraphs | taken |
| Enter | taken: the paragraph splits where the caret sits |
| Backspace at the start of a paragraph | taken: the paragraph merges into the one before it |
| 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 |
| anything else the browser offers (a mark, a list, a drop) | refused, reason `unsupportedEdit` |
| an edit spanning two runs, or landing outside every run | refused, reason `range` |
| an edit reaching over a picture or a table, or landing outside every run | refused, reason `range` |

**Why the whole view rather than a run at a time:** `contenteditable` per run
makes every run its own editing host, and a host is a wall. The caret cannot
Expand All @@ -365,52 +368,36 @@ over every run.
it says *what* the edit is (`inputType`), it says *where* (`getTargetRanges()`),
and it is cancelable. `text.js` already edits the plain-text view this way.

**The whitelist is closed, not open.** Only the input types that change the text
of one run are allowed; anything unrecognised is refused. An open list would let
a browser-specific `inputType` through to a `MutationObserver` that only watches
`characterData`, and a structural change would then be invisible to the log and
saved wrong. Refusing something we could have allowed costs a reader one
gesture; allowing something we cannot replay costs them their document.
**The whitelist is closed, not open.** Only the input types the editor can
express are taken; anything unrecognised is refused. 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 `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.

**`input` is what the log collects on, not a `MutationObserver`.** The browser
raises `input` when *it* applied an edit; a script rewriting the page raises
none. That is the whole difference: `search.js` wraps every match in a `<mark>`,
which an observer watching `characterData` reads as nine edits — measured, and
it lit the host's save button and put nine no-op `setText` ops in the log. The
run is the one `beforeinput` named, or the one the caret sits in where no
`beforeinput` arrived; a run the editor cannot name at all raises code 9 rather
than being dropped.
allowed because it lands inside a `x-s[data-odr-id]` run and reaches over
nothing but runs, so a picture, a table's furniture 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.

**The editor owns the edit.** It cancels the `beforeinput` and splices the page
itself, rather than letting the browser apply the change and reading the run
back. See decision 6 of [`document-editing.md`](document-editing.md) for why that had
to change.

**Undo is the editor's**, because cancelling every edit leaves the browser's own
stack empty. Each step holds the operations it puts on the wire and the two
halves of taking it back, so `canUndo` and the chord now agree and a host's undo
button is live. One `beforeinput` is one step.

**Known holes, both narrow.** A scripted `document.execCommand` can bypass the
gate, because Chrome does not fire a cancelable `beforeinput` for every command;
trusted input, which is all a reader has, is refused correctly. And a
composition cannot be cancelled at all — `insertCompositionText` is allowed and
reconciled afterwards, which is why the observer reports code 9 when text
changes where no op can name it, rather than dropping it in silence. Android
WebView's incomplete `beforeinput` (decision 8) is the reason that report
exists; verify it on a device before trusting the gate there.

**Two limits a reader meets, and phase 3 is where both go:**

- **Undo belongs to the browser, not to us.** Every allowed edit is one the
browser applied, so its own stack is the one that replays — ctrl+Z works, and
`chordKey` leaves the key alone because no editor claims it (decision 9). But
we cannot read that stack's depth, so `canUndo` is honestly false and a host's
undo *button* stays grey. Phase 3 item 2 is what fixes it: once the editor
records an op with its inverse, it answers `undo()` and joins the shared log.
Until then the button and the chord disagree, which is worse than either.
- **Backspace at the start of a run is refused.** Its target range reaches back
into the run before it, so the edit spans two and `range` refuses it —
merging two runs is not something `setText` can express. It did nothing under
the per-run hosts either; the difference is that it now says why. The op that
would fix it is `deleteRange` across runs, which needs the write-side adapter
work in phase 2, not a browser change.
trusted input, which is all a reader has, goes through it. And a composition
cannot be cancelled at all, so the editor lets it finish and reads the run back
on `compositionend`; a composition that landed where no run can name it raises
code 9 rather than being dropped. Android WebView's incomplete `beforeinput`
(decision 8) is the reason that report exists, and the reason a delete whose
range the browser did not state is extended by one character rather than
refused; verify both on a device.

## Preliminary implementation plan (ODF / OOXML)

Expand Down Expand Up @@ -458,9 +445,10 @@ Sequence within Phase 2: (a) delete element, (b) toggle mark on a range,

### Phase 3 — Browser editor

The frame is landed (decisions 9 to 12): the mode, the refusals, the callbacks
and the keyboard classes are in `frontend/editing.js`, and `document.js`
attaches the skeleton editor to it. What is left is the editor itself.
**Landed**, over [`document-editing.md`](document-editing.md)'s schema. The mode, the
refusals, the callbacks and the keyboard classes are in `frontend/editing.js`;
`document.js` holds the editor, which owns every edit and carries its own
undo.

1. Model keyed by `data-odr-id`; op recorder; composition-aware reconciliation.
`beforeinput` is already the gate (decision 13); what is left is owning the
Expand Down
27 changes: 24 additions & 3 deletions src/odr/document.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,14 +212,27 @@ void Document::edit(const std::string_view operations,
}

if (name == "insertText") {
const auto text = operation.at("text").get<std::string>();
const std::int64_t address = reserve(operation);

// `parent` appends into an element rather than naming a run to sit
// beside
if (operation.contains("parent")) {
if (operation.contains("after") || operation.contains("before")) {
throw std::invalid_argument(
"insertText names `parent` or a run to sit beside, not both");
}
const Element parent = element_of(operation, "parent");
minted.emplace(address, append_text(parent, text).identifier());
continue;
}

const bool after = operation.contains("after");
if (after == operation.contains("before")) {
throw std::invalid_argument(
"insertText names one of `after` and `before`");
"insertText names one of `after`, `before` and `parent`");
}
const std::int64_t address = reserve(operation);
const Text anchor = text_of(operation, after ? "after" : "before");
const auto text = operation.at("text").get<std::string>();
const Text created = after ? insert_text_after(anchor, text)
: insert_text_before(anchor, text);
minted.emplace(address, created.identifier());
Expand Down Expand Up @@ -308,6 +321,14 @@ Text Document::insert_text_(const Text &anchor, const Placement where,
return {adapter, identifier, adapter->text_adapter(identifier)};
}

Text Document::append_text(const Element &parent,
const std::string &text) const {
const internal::abstract::ElementAdapter *adapter = m_impl->element_adapter();
const ElementIdentifier identifier =
adapter->element_append_text(check_(parent), text);
return {adapter, identifier, adapter->text_adapter(identifier)};
}

/// The adapter @p paragraph answers to, refusing an element that is not a
/// paragraph of this document.
const internal::abstract::ParagraphAdapter *
Expand Down
5 changes: 5 additions & 0 deletions src/odr/document.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ class Document final {
[[nodiscard]] Text insert_text_after(const Text &anchor,
const std::string &text) const;

/// A run as the last child of @p parent. What a paragraph holding no run at
/// all is typed into.
[[nodiscard]] Text append_text(const Element &parent,
const std::string &text) const;

/// Splits @p paragraph after @p after - one of its descendants, or an
/// element that does not exist to move every child - into a new paragraph
/// of the same style. Refuses where an element between the two is one it
Expand Down
9 changes: 9 additions & 0 deletions src/odr/internal/abstract/document.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,15 @@ class ElementAdapter {
throw UnsupportedOperation();
}

/// A run holding @p text as the last child of @p element_id. What a
/// paragraph holding no run at all is typed into.
/// @throws UnsupportedOperation where the engine cannot write.
virtual ElementIdentifier
element_append_text([[maybe_unused]] const ElementIdentifier element_id,
[[maybe_unused]] const std::string &text) const {
throw UnsupportedOperation();
}

[[nodiscard]] virtual const TextRootAdapter *
text_root_adapter([[maybe_unused]] const ElementIdentifier element_id) const {
return nullptr;
Expand Down
Loading
Loading