diff --git a/.changeset/y-undo-keeps-last-run-end.md b/.changeset/y-undo-keeps-last-run-end.md new file mode 100644 index 00000000000..7c4e8e48358 --- /dev/null +++ b/.changeset/y-undo-keeps-last-run-end.md @@ -0,0 +1,7 @@ +--- +'@lexical/y': patch +--- + +[fix] An undo of typing inside a comment or a format keeps the value ending where it did when a collaborator deleted the rest of the run, instead of letting it cover everything after it + +Verify: comment or format a few words, type plain text inside them, let a collaborator delete the text after yours up to past the run's end, then undo your typing: the value ends where the deleted text began on every client. diff --git a/packages/lexical-y/DECISIONS.md b/packages/lexical-y/DECISIONS.md index 5393d6811fd..2b420127016 100644 --- a/packages/lexical-y/DECISIONS.md +++ b/packages/lexical-y/DECISIONS.md @@ -58,6 +58,7 @@ issues and docs that carry it. | D24 | **Yjs's format cleanup is off** in `@lexical/y` containers and on every relay that loads a state (`createYDoc`). A delete keeps the ends of each deleted formatted run, within a bound per format key. Not reported upstream. | The cleanup deletes format items a concurrent writer may still rely on. Upstream already knows that skipping it is the app's trade-off. | `DECIDED` | 10-28 | [#9400], [y.md: Servers][y-servers] | | D25 | **Undo is Yjs's `UndoManager`**, scoped to the binding's root and this client's own edits (500 ms capture by default). It never deletes a container a collaborator has put content in (its `deleteFilter`). Do not combine it with `HistoryExtension`. | Collaborative undo must not revert other people's work; keeping a container whole once someone else wrote in it matches y-prosemirror's protected nodes and is cheap to maintain. The `UndoManager` was chosen on 10-23; keeping containers whole was confirmed on 10-30. | `DECIDED` | 10-30 | [#9400], [y.md: Limits][limits] | | D26 | **Undo favours people taking turns over concurrent races.** Where a fix makes undo correct for sequential edits at the cost of rare races between an undo and a simultaneous edit of the same text, the sequential case wins. | Taking turns is the common case; a race with a simultaneous edit of the same text is rare. Picked on 10-28, confirmed on 10-30. | `DECIDED` | 10-30 | [#9400] | +| D84 | **An undo keeps a null that is the last end of a run still open before it.** Where an undo or redo would delete a null of its change (the null by which typed text was made plain inside a run of a value) and that null is the run's only end left (no later live item of the key ends the run, and content shows after the null), the null is kept, and the run ends there. | A collaborator's delete can take the run's own end and the restore the typing wrote after its text; deleting the null then left the value open over everything after it, on every client ([#9624]). It follows D26 (taking turns) and writes nothing: it keeps an item. Cost: one pass over each container that holds a null the undo would delete. Without the delete's collapse of format items (D24) the run's end stays and this does not arise. | `DECIDED` | 10-30 | [#9624] | | D27 | **Moves are a delete and a re-insert** at launch; the binding rewrites the cheaper side. Real moves within a container ([#9449], records after Yjs's unmerged `ContentMove` design) are paused and land last, backward compatible: a new layout version that older clients open read only. Move records leave tombstones, as every Yjs binding does. Moves across containers stay copies. | Every Yjs binding turns moves into delete plus insert; Yjs dropped its own move for complexity. Reliability of the rest came first. | `DECIDED` | 10-25 | [#9449], [#9455], [y.md: Limits][limits] | | D28 | **Text identity is fixed at the source.** While a binding is attached, core records text edits (`spliceText`, insert and remove, split and merge), and text entity plugins record a grow as a move and a remake as a retype. Text only `setTextContent` or the DOM changed is diffed. The binding adds no per-case heuristics. | No binding can recover intent an API dropped; special-case rules were fragile and regressed neighbouring cases. | `DECIDED` | 10-27 | [#9483], [y.md: Edits made by code][y-code] | | D29 | **Remade text keeps its common start and end by character**, whatever the node boundaries (y-prosemirror's `updateYText` rule), and never brings back a collaborator's delete. It replaces the word matching of [#9565]. | It is the industry-standard behaviour, and word matching glued words together and merged numbers oddly. | `DECIDED` | 10-30 | [#9565] | @@ -198,3 +199,4 @@ issues and docs that carry it. [#9608]: https://github.com/meta/lexical/issues/9608 [#9609]: https://github.com/meta/lexical/issues/9609 [#9610]: https://github.com/meta/lexical/issues/9610 +[#9624]: https://github.com/meta/lexical/issues/9624 diff --git a/packages/lexical-y/src/__tests__/unit/regressions/UndoKeepsLastRunEnd.test.ts b/packages/lexical-y/src/__tests__/unit/regressions/UndoKeepsLastRunEnd.test.ts new file mode 100644 index 00000000000..5a1b97742e6 --- /dev/null +++ b/packages/lexical-y/src/__tests__/unit/regressions/UndoKeepsLastRunEnd.test.ts @@ -0,0 +1,387 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ +// @vitest-environment node + +// A collaborator types plain text inside a run of a value (a comment, a +// format, a style or a NodeState value); another deletes the text after it, +// which takes the run's own end and the restore the plain text's change +// wrote after it; the collaborator undoes its typing. The value ends where +// the deleted text began, never running on over the text after it (#9624): +// the undo keeps the null before the typed text when it is the last end of +// a run still open before it. + +// The value an undo brings back covers exactly what it covered before: +// a collaborator removes a value, its author undoes it (and may write the +// same key later), a third client may delete text around the value's end, +// and the collaborator then undoes the removal. On main, with garbage +// collection on, the restored value runs on past its end (#9623), and with +// many comment ids or a concurrent undo of the end's change too (#9620): +// those cells are pinned as test.fails until the fix lands. +import {$toggleLink, LinkNode} from '@lexical/link'; +import {$addMarkID, $getNodeMarkIDs, $removeMarkID} from '@lexical/mark'; +import {$patchStyleText} from '@lexical/selection'; +import { + $createParagraphNode, + $createTextNode, + $getRoot, + $getState, + $isTextNode, + $setState, + type ElementNode, + type LexicalNode, + type TextNode, +} from 'lexical'; +import {expect, test} from 'vitest'; + +import { + $block, + $p, + $selectIn, + expectConverged, + freshReader, + type Peer, + type Setup, + setup, +} from '../../utils/harness'; +import {markState, prepareMarkState} from '../../utils/sizeOps'; + +const STATE = markState('lastend'); +const TEXT = 'abcd Kilo bb cc Delta end'; +type Kind = + | 'comment' + | 'bold' + | 'italic' + | 'underline' + | 'color' + | 'state' + | 'link'; +function $give(kind: Kind, start: number, end: number, on: boolean, count = 1) { + const s = $selectIn([0], start, end); + if (kind === 'comment') { + for (let i = 0; i < count; i++) { + if (on) { + $addMarkID(`c${i}`, s); + } else { + $removeMarkID(`c${i}`, s); + } + } + } else if (kind === 'color') $patchStyleText(s, {color: on ? 'red' : null}); + else if (kind === 'state') { + for (const n of s.extract()) if ($isTextNode(n)) $setState(n, STATE, on); + } else if (kind === 'link') $toggleLink(on ? 'https://a.test' : null); + else s.formatText(kind, on ? {bold: 1, italic: 2, underline: 8}[kind] : 0); +} +function chars(peer: Peer, kind: Kind): [string, boolean][][] { + return peer.read(() => + $getRoot() + .getChildren() + .map(block => + (block as ElementNode).getAllTextNodes().flatMap((n: TextNode) => { + const on = + kind === 'comment' + ? $getNodeMarkIDs(n).includes('c0') + : kind === 'color' + ? n.getStyle().includes('red') + : kind === 'state' + ? $getState(n, STATE) + : kind === 'link' + ? n.getParents().some(parent => parent instanceof LinkNode) + : n.hasFormat(kind); + return [...n.getTextContent()].map((c): [string, boolean] => [c, on]); + }), + ), + ); +} +async function check( + t: Setup, + kind: Kind, + expected: string[], + label: string, + expectedText = [TEXT, 'untouched tail'], +) { + const got = chars(t.a, kind); + for (const p of t.peers) { + expect(p.doc.store.pendingStructs, label + ' pending structs').toBe(null); + expect(p.doc.store.pendingDs, label + ' pending deletes').toBe(null); + expect(chars(p, kind), label).toEqual(got); + } + expect( + got.map(cs => cs.map(([c]) => c).join('')), + label + ' text', + ).toEqual(expectedText); + const fresh = await freshReader(t.a); + try { + expect(chars(fresh, kind), label + ' fresh').toEqual(got); + } finally { + fresh.stop(); + } + expect( + got.map(cs => cs.map(([c, on]) => (on ? '^' : c)).join('')), + label, + ).toEqual(expected); +} +function mask(text: string, start: number, end: number) { + return text.slice(0, start) + '^'.repeat(end - start) + text.slice(end); +} +async function make(gc: boolean, ids: number[]) { + const t = await setup( + () => + $getRoot().append( + $createParagraphNode().append($createTextNode(TEXT)), + $createParagraphNode().append($createTextNode('untouched tail')), + ), + {ids, prepare: prepareMarkState}, + ); + for (const p of t.peers) p.doc.gc = gc; + const sync = t.sync; + t.sync = async () => { + const before = t.peers.map(p => p.doc.store.getClock(p.doc.clientID)); + await sync(); + expect( + t.peers.map(p => p.doc.store.getClock(p.doc.clientID)), + 'no writes while applying remote updates', + ).toEqual(before); + }; + return t; +} + +/** Plain text (no format, style, state or comment) at `offset` of the first line. */ +function $plainAt(offset: number, text: string) { + let left = offset; + for (const node of $block([0]).getAllTextNodes() as TextNode[]) { + const size = node.getTextContentSize(); + if (left <= size) { + const plain = $createTextNode(text); + if (left === size) { + node.insertAfter(plain); + } else if (left === 0) { + node.insertBefore(plain); + } else { + node.splitText(left)[1].insertBefore(plain); + } + return; + } + left -= size; + } +} + +// "Kilo bb cc" has the value; "XY" is typed after "Kilo". +const RUN: [number, number] = [5, 15]; +const AT = 9; +const TYPED = 'XY'; +const WITH = TEXT.slice(0, AT) + TYPED + TEXT.slice(AT); +const DELETES: [number, number][][] = [ + [[11, 17]], + [[11, 19]], + [[11, 27]], + [[12, 17]], + [ + [11, 14], + [11, 14], + ], +]; + +/** The first line after the deletes and the undo of the typing, `^` where the value is. */ +function extentAfter(deletes: [number, number][]) { + let marked = [...WITH].map((c, i) => + (i >= RUN[0] && i < AT) || + (i >= AT + TYPED.length && i < RUN[1] + TYPED.length) + ? '^' + : c, + ); + let typed = [...WITH].map((_, i) => i >= AT && i < AT + TYPED.length); + for (const [from, to] of deletes) { + marked = [...marked.slice(0, from), ...marked.slice(to)]; + typed = [...typed.slice(0, from), ...typed.slice(to)]; + } + const text = [...WITH]; + let plain = text; + for (const [from, to] of deletes) { + plain = [...plain.slice(0, from), ...plain.slice(to)]; + } + return { + marked: marked.filter((_, i) => !typed[i]).join(''), + text: plain.filter((_, i) => !typed[i]).join(''), + }; +} + +for (const gc of [false, true]) { + for (const ids of [ + [1, 2, 3], + [3, 2, 1], + ]) { + for (const kind of [ + 'comment', + 'bold', + 'italic', + 'underline', + 'color', + 'state', + ] as Kind[]) { + for (const owner of ['the typist', 'a collaborator'] as const) { + test(`plain text typed inside a ${kind} run by ${owner}, its undo with no delete closes the hole gc=${gc} ids=${ids}`, async () => { + const t = await make(gc, ids); + try { + (owner === 'the typist' ? t.a : t.b).update(() => + $give(kind, RUN[0], RUN[1], true), + ); + await t.sync(); + t.a.update(() => $plainAt(AT, TYPED)); + await t.sync(); + t.a.undo(); + await t.sync(); + await check( + t, + kind, + [mask(TEXT, RUN[0], RUN[1]), 'untouched tail'], + 'hole closed', + ); + } finally { + for (const p of t.peers) p.stop(); + } + }); + for (const deletes of DELETES) { + test(`plain text typed inside a ${kind} run by ${owner}, the rest of the run deleted ${JSON.stringify(deletes)}, the typing undone gc=${gc} ids=${ids}`, async () => { + const t = await make(gc, ids); + try { + (owner === 'the typist' ? t.a : t.b).update(() => + $give(kind, RUN[0], RUN[1], true), + ); + await t.sync(); + t.a.update(() => $plainAt(AT, TYPED)); + await t.sync(); + for (const [from, to] of deletes) { + t.b.update(() => void $selectIn([0], from, to).removeText()); + await t.sync(); + } + t.a.undo(); + await t.sync(); + const {marked, text} = extentAfter(deletes); + await check(t, kind, [marked, 'untouched tail'], 'run end', [ + text, + 'untouched tail', + ]); + } finally { + for (const p of t.peers) p.stop(); + } + }); + } + } + } + } +} + +// The run's own value written again after the typed text (a collaborator's +// value over part of the range, then the typist's over all of it) is no +// end: undoing the typing joins the run again. +for (const gc of [false, true]) { + for (const ids of [ + [1, 2, 3], + [3, 2, 1], + ]) { + for (const kind of [ + 'comment', + 'bold', + 'italic', + 'color', + 'state', + ] as Kind[]) { + test(`a ${kind} written again over a run with plain text typed in it, the typing undone gc=${gc} ids=${ids}`, async () => { + const t = await make(gc, ids); + try { + t.b.update(() => $give(kind, 0, 4, true)); + await t.sync(); + t.a.update(() => $plainAt(3, 'xq')); + await t.sync(); + t.a.update(() => $give(kind, 0, 10, true)); + await t.sync(); + t.a.update(() => $plainAt(5, 'zz')); + await t.sync(); + t.a.undo(); + await t.sync(); + const text = TEXT.slice(0, 3) + 'xq' + TEXT.slice(3); + await check( + t, + kind, + [mask(text, 0, 10), 'untouched tail'], + 'one run', + [text, 'untouched tail'], + ); + } finally { + for (const p of t.peers) p.stop(); + } + }); + } + } +} + +// #9624 as reported: three comments and an italic word around the typed text. +function $showLine(peer: Peer): string { + return peer.read(() => + $getRoot() + .getChildren() + .map((_, i) => + ($block([i]).getAllTextNodes() as TextNode[]) + .map(n => { + const ids = $getNodeMarkIDs(n as LexicalNode); + return ( + JSON.stringify(n.getTextContent()) + + (ids.length > 0 ? `[${ids.join(',')}]` : '') + ); + }) + .join(''), + ) + .join(' / '), + ); +} + +for (const ids of [ + [1, 2], + [2, 1], +]) { + test(`a comment does not run on after a collaborator deleted its end and the typing inside it is undone (#9624, ids ${ids})`, async () => { + // one text node per word; "cccc " is italic + const t = await setup( + () => + $getRoot().append( + $createParagraphNode().append( + ...['aaa ', 'bbbb ', 'cccc ', 'ddddd ', 'eeeee ', 'ffff'].map(w => + $createTextNode(w).setFormat(w === 'cccc ' ? 'italic' : 0), + ), + ), + $p('tail line'), + ), + {ids, prepare: prepareMarkState}, + ); + // B comments "bbbb cccc" (c1); A comments "cccc" (c2) and "cccc dd" (c3); B comments "dd ee" (c4) + t.b.update(() => void $addMarkID('c1', $selectIn([0], 4, 13))); + t.a.update(() => void $addMarkID('c2', $selectIn([0], 9, 13))); + t.a.update(() => void $addMarkID('c3', $selectIn([0], 9, 16))); + t.b.update(() => void $addMarkID('c4', $selectIn([0], 17, 22))); + // A types plain (not italic, uncommented) "XYZ" right after "cccc", before the italic space + t.a.update(() => { + // the comments split "cccc " into "cccc" and " ": XYZ goes between them + const node = ($block([0]).getAllTextNodes() as TextNode[]).find( + n => n.getTextContent() === 'cccc', + )!; + node.insertAfter($createTextNode('XYZ')); + }); + expect($showLine(t.a).split(' / ')[0]).toBe( + '"aaa ""bbbb "[c1]"cccc"[c1,c2,c3]"XYZ"" "[c3]"dd"[c3]"d""dd ee"[c4]"eee ffff"', + ); + // B deletes " dd" after "XYZ", then the next character; A undoes its "XYZ" + t.b.update(() => void $selectIn([0], 16, 19).removeText()); + t.b.update(() => void $selectIn([0], 16, 17).removeText()); + t.a.undo(); + await t.sync(); + await expectConverged(t.peers); + // nothing past "eeeee" was commented by anyone + const [, tail] = $showLine(t.a).split(' / '); + expect(tail).toBe('"tail line"'); + }); +} diff --git a/packages/lexical-y/src/internal/Binding.ts b/packages/lexical-y/src/internal/Binding.ts index 671394ceee2..49b45271ae0 100644 --- a/packages/lexical-y/src/internal/Binding.ts +++ b/packages/lexical-y/src/internal/Binding.ts @@ -810,6 +810,8 @@ export class YBinding { private restoring: Set | null = null; /** Stored nodes the running undo/redo deletes (it inserted them). */ private removing: Set | null = null; + /** Nulls the running undo/redo keeps (see lastRunEnds). */ + private keptNulls: Set | null = null; /** The items the running undo/redo deletes (see isKeptByUndo). */ private undoing: Set | null = null; private wasEditable = true; @@ -5039,6 +5041,9 @@ export class YBinding { * leave a malformed container). */ private isKeptByUndo(item: Item): boolean { + if (this.keptNulls !== null && this.keptNulls.has(item)) { + return true; + } // A null a format change wrote after its range to end its value there: // kept where without it the content after it would take a value (that // of a collaborator's change that ended inside the range, closed with @@ -5095,6 +5100,89 @@ export class YBinding { return false; } + /** + * The nulls the running undo would delete that are the last end of a run + * still open before them, and so are kept (see isKeptByUndo): the nearest + * live item of the null's key before it (leaving out what this undo + * deletes) gives a value, and content shows after the null before the next + * live item of its key, which gives another value (one giving the run's own + * value again joins the run), or before the end of the container. Why: a null by which a change made text inside a run plain + * (typed plain text) relied on the run's own end after it, and once a + * collaborator's delete took that end and the change's own restore, the + * null was the run's only end; the undo of that change deleted it and the + * value ran on over everything after it (#9624). One pass over each + * container holding such a null. + */ + private lastRunEnds(undoing: Set): Set | null { + const parents = new Set(); + for (const it of undoing) { + if ( + !it.deleted && + it.content instanceof ContentFormat && + it.content.value === null + ) { + parents.add(it.parent as YNode); + } + } + if (parents.size === 0) { + return null; + } + const kept = new Set(); + for (const parent of parents) { + // Of each key, the value of the last live item not undone, and the + // nulls this undo deletes after it, with the content count at each. + const last = new Map(); + const pending = new Map(); + let content = 0; + // `next` is the value of the next live item of the key (undefined at + // the end of the container): a null before it is the run's last end + // where content shows between them and the run would take a value + // there it did not have (a restate of the run's own value joins it). + const resolve = (key: string, next: unknown) => { + for (const [item, at, open] of pending.get(key) ?? []) { + if ( + content > at && + (next === undefined || + (next !== null && !equalFormatValue(next, open))) + ) { + kept.add(item); + } + } + pending.delete(key); + }; + for ( + let it = (parent as unknown as {_start: Item | null})._start; + it !== null; + it = it.right + ) { + if (it.deleted) { + continue; + } + const undone = undoing.has(it); + if (it.content instanceof ContentFormat) { + const {key, value} = it.content; + if (!undone) { + resolve(key, value); + last.set(key, value); + } else if (value === null) { + const before = last.get(key); + if (before !== undefined && before !== null) { + const list = pending.get(key) ?? []; + list.push([it, content, before]); + pending.set(key, list); + } + } + } else if (!undone && it.countable) { + content++; + } + } + for (const key of [...pending.keys()]) { + resolve(key, undefined); + } + } + return kept; + } + /** * Undo this client's last change in the document (collaborative undo). * Returns whether there was one; false while the binding is not active. @@ -5170,6 +5258,7 @@ export class YBinding { this.restoring = null; this.removing = null; this.undoing = null; + this.keptNulls = null; this.stepCursor = null; } } @@ -5339,6 +5428,7 @@ export class YBinding { this.restoring = null; this.removing = null; this.undoing = null; + this.keptNulls = null; if (top === undefined) { return; } @@ -5370,6 +5460,7 @@ export class YBinding { this.restoring = names; this.removing = removing; this.undoing = undoing; + this.keptNulls = this.lastRunEnds(undoing); } // -------------------------------------------------------------------------