Repository navigation
[lexical][lexical-clipboard][lexical-website][lexical-y] Bug Fix: Keep a link a collaborator removes removed when another collaborator formats text covering it - #9601
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
|
AI review status for this pull request.
|
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for tracking this down. The core fix works: LinkRemovedWhileFormatted and LinkWithMarkRemovedWhileFormatted fail on main and pass here in both client id orders, the same races with DOM-attached (jsdom) editors also keep the link removed, and the new S7-26 expectation matches intent, since B undid its own link and A never set it.
Needs a change before merge
Peers no longer converge when a link's text sits in an inline element (packages/lexical-y/src/internal/Schema.ts:2004). The new gate skips textFormat/textStyle whenever text is anywhere below the wrapper, but ElementNode#shouldSerializeTextStyles (packages/lexical/src/nodes/LexicalElementNode.ts:904-922) still serializes them for an element with no direct text child. With before + link(mark(linked words)) + after, after B bolds a range covering the link:
- headless peers: B exports the link with
textFormat: 1; A and a fresh reader export none; - DOM-attached peers: A and B agree, but a fresh headless reader and
readYStatefrom@lexical/y/headlessexport none; - changing the URL of a link whose stored value already carries
textFormat(a document written before this change) diverges the same way.
main converges in all three cases, and the collab Y guide says every client converges to the same document (packages/lexical-website/docs/collaboration/y.md, Limits). The package's own harness reproduces it:
// paragraph: 'before ' + link(mark(['c1'], 'linked words')) + ' after'
const t = await setup($nestedLink, {ids});
t.b.update(() => {
$select('before', 'words').formatText('bold');
$setSelection(null);
});
await t.sync();
await expectConverged([t.b, t.a]); // A lacks "textFormat": 1 on the linkAs the description already notes, the storage rule and shouldSerializeTextStyles have to agree: either a descendant-aware shouldSerializeTextStyles in core (with JSON that carries the field still importing), or shouldSerializeTextStyles() as the gate again (as in 31e8705) with the nested-link case as a follow-up. Which one is a maintainer decision.
Optional suggestions
- The new tests compare only link lists. Building the peers with the harness (
setup,freshReader,expectConvergedinpackages/lexical-y/src/__tests__/utils/harness.ts) would also compare both peers' and a fresh reader's JSON with the self-check on; a format-only case over link > mark > text would pin whichever rule lands. Write.tsalready has$holdsText(packages/lexical-y/src/internal/Write.ts:230-246); exporting one copy keeps the storage and follow-up rules from drifting.- The changeset (
.changeset/lexical-y-link-textformat.md:7) and the description describe the direct-children rule from the first commit; reword them to the rule that lands, since CONTRIBUTING.md asks for an entry written for people who use Lexical. - The Test plan's After output is from the previous commit (183 files); AGENTS.md asks for the real Before and After output of the command that was run, including
LinkWithMarkRemovedWhileFormatted.test.ts.
| // whole value again over the wrapper's text, which a collaborator's | ||
| // concurrent removal of the wrapper does not delete (the removed link | ||
| // came back). | ||
| if (!$holdsText(node.getLatest())) { |
There was a problem hiding this comment.
Peers diverge for link > mark > text. This skips the link's textFormat/textStyle whenever text is anywhere below it, but ElementNode#shouldSerializeTextStyles still serializes them when the link has no direct TextNode child. After B bolds a range covering link(mark('linked words')), B exports the link with textFormat: 1 while A, a fresh reader and readYState export none, so the harness's expectConverged([t.b, t.a]) fails here and passes on main. Changing the URL of a link whose stored value already carries textFormat diverges the same way. The two rules need to agree: a descendant-aware shouldSerializeTextStyles in core, or shouldSerializeTextStyles() as the gate again (as in 31e8705).
| } | ||
|
|
||
| /** Whether `node` holds a text node, directly or inside its descendants. */ | ||
| function $holdsText(node: ElementNode): boolean { |
There was a problem hiding this comment.
nit: Write.ts already has this predicate ($holdsText, lines 230-246), used for the lines and containers whose text fields follow-ups do not write. Exporting one copy keeps the storage and follow-up rules from drifting.
| expect(links(a.editor)).toEqual([]); | ||
| expect(links(b.editor)).toEqual([]); |
There was a problem hiding this comment.
Optional: these assertions compare link lists only. Building the peers with the harness (setup, freshReader, expectConverged in src/__tests__/utils/harness.ts) also compares both peers' and a fresh reader's JSON with the self-check on, which is where this head still differs for a nested link; a format-only case (no removal) would pin the rule that lands.
|
|
||
| [fix] Keep a link a collaborator removes removed when another collaborator formats text covering it at the same time | ||
|
|
||
| A format change over a link also sets the link's textFormat (`RangeSelection#formatText`), and the binding stored a link's textFormat and textStyle in the value its text carries: the format change wrote the link's whole value again over its text, and that write brought a concurrently removed link back for everyone. A wrapper's textFormat and textStyle are now stored only while it has no text of its own, as Lexical serializes them; with text, they follow it. |
There was a problem hiding this comment.
Optional: "only while it has no text of its own, as Lexical serializes them" describes the direct-children rule from the first commit. At this head the fields are stored only while no text is below the wrapper at any depth, which is not how shouldSerializeTextStyles decides; worth rewording once the rule settles.
…another collaborator formats text covering it ## Description A removes a link while B, at the same time, makes a range covering the whole link code (#9597): after the sync the link was back on every client. B's format also sets the link's textFormat (`RangeSelection#formatText` sets it on element nodes the selection covers), and the binding stored a wrapper's textFormat and textStyle in the value its text carries (`w:link`). B's write of the new value went over the link's text as a new opener and a restore of the old value after it, and A's concurrent removal, which deleted the opener B had seen, did not reach B's new opener: the link came back. A wrapper's textFormat and textStyle are now stored only while it has no text of its own (`shouldSerializeTextStyles`), as Lexical serializes them: with text, they follow it. A format change over a link then writes no link value. Documents that hold them in a link's value still read as before. `regressions/Formats.test.ts` S7-26 asserted the old result of the same mechanism (B links "t7z t8z t9z" and undoes it while A formats a range over the link's start, and the link stayed): it now expects the link gone on every client, as B undid it and A did not touch the link. Closes #9597 ## Test plan `packages/lexical-y/src/__tests__/unit/LinkRemovedWhileFormatted.test.ts` (new): the issue's repro, both client id orders. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/LinkRemovedWhileFormatted.test.ts Tests 2 failed (2) AssertionError: expected [ 't29z t30z' ] to deeply equal [] ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/LinkRemovedWhileFormatted.test.ts Tests 2 passed (2) $ pnpm exec vitest run --project unit packages/lexical-y packages/lexical-mark Test Files 183 passed | 2 skipped (185) Tests 8688 passed | 46 expected fail | 4 skipped (8738) ``` `pnpm run ci-check` passes.
…its in an inline element inside it
## Description
Follow-up to the previous commit (store a wrapper's textFormat and
textStyle only while it has no text). That checked Lexical's
`shouldSerializeTextStyles`, which looks at the wrapper's direct children
only: a link whose text sits in a mark (link > mark > text) still stored
them, B's format change over it still wrote the link's whole value again,
and A's concurrent removal of the link was lost the same way. A wrapper
now stores them only when it holds no text node anywhere inside it,
through inline elements too. Reported in review.
## Test plan
`LinkWithMarkRemovedWhileFormatted.test.ts` (new, from the review): the
issue's steps with the link's text inside a mark, both client id orders.
### Before
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/LinkWithMarkRemovedWhileFormatted.test.ts
Tests 2 failed (2)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/LinkWithMarkRemovedWhileFormatted.test.ts packages/lexical-y/src/__tests__/unit/LinkRemovedWhileFormatted.test.ts
Tests 4 passed (4)
$ pnpm exec vitest run --project unit packages/lexical-y packages/lexical-mark packages/lexical-link
Test Files 196 passed | 2 skipped (198)
Tests 8977 passed | 46 expected fail | 4 skipped (9027)
```
`pnpm run ci-check` passes.
… sits in a mark the same JSON for its author and a fresh reader ## Description Follow-up to the previous commit, which stores a wrapper's textFormat and textStyle only while it holds no text anywhere inside it, so that a link whose text sits in a mark (link > mark > text) stays removed under a concurrent format. Lexical's `ElementNode#shouldSerializeTextStyles` looked at direct TextNode children only, so after an ordinary format the author's link still wrote `textFormat` while the binding no longer stored it: the author's JSON and a fresh reader's differed. `shouldSerializeTextStyles` now checks an inline element (a link, an autolink, a mark) for a TextNode anywhere inside it. A reconcile already derives an element's textFormat and textStyle from its first text at any depth, and the JSON of an inline element now follows the same rule. A block keeps the direct-children check, so its JSON is unchanged. The binding stores a wrapper's textFormat and textStyle exactly when `shouldSerializeTextStyles` is true, so the two agree by construction (including for a subclass that overrides it). Exported JSON changes only for a link, autolink or mark whose children are all elements: it no longer writes those two fields. JSON that has them still reads into the same state. Recorded as D81 in `packages/lexical-y/DECISIONS.md`. A link that holds no text at all (only an inline decorator or a line break) still has to store them, and still comes back in the same race: #9627, marked `test.fails` in the new matrix. ## Test plan `packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts` (new): link, autolink, mark (two ids) and overflow nested one to two deep in every order (three with `MATRIX_FULL=1`), around plain, bold, styled text, two texts, a line break, text next to a nested mark or an inline decorator, a decorator alone and a line break alone, with and without text fields of their own. It checks that the author's JSON and a peer's equal a fresh reader's after a format or style change over the paragraph or the wrapped text. It also checks that every wrapper one client removes stays removed while another formats over it, in both client id orders. `packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts` (new): which elements persist the two fields, and stored JSON that has them. ### Before On the previous commit: ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 42 failed | 1848 passed (1890) an author and a fresh reader have the same JSON > link>mark>text AssertionError: format-all: author: expected { root: { …(8) } } to deeply equal { root: { …(8) } } ``` On main (before this pull request): ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 196 failed | 1694 passed (1890) a wrapper one client removes stays removed while another formats over it > link>text: remove link, ids 1,2 AssertionError: a: expected 1 to be +0 // Object.is equality $ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts Tests 2 failed | 4 passed (6) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 1890 passed (1890) $ MATRIX_FULL=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 7182 passed (7182) $ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts Tests 6 passed (6) $ pnpm exec vitest run --project unit --project scripts-unit Test Files 639 passed | 4 skipped (643) Tests 27462 passed | 246 expected fail | 135 skipped (27843) ```
bf22fb9 to
4072e4f
Compare
zurfyx
changed the title
… inside each other linear ## Description The previous commit made `shouldSerializeTextStyles` look for text anywhere inside an inline element. Each inline element asked that of its whole subtree, so serializing N inline elements nested inside each other, each with a textFormat or textStyle, walked O(N^2) nodes (review: 33,155 `getFirstChild` calls at depth 256, where main makes 515). For a frozen EditorState, the answer for every element the walk visits is now kept (keyed by the state), so each element is walked at most once per state. An update's pending state is not cached, since it can still change. The binding asks only when the wrapper has a textFormat or textStyle to store, as Lexical's serializer does. `WrapperTextFields.test.ts` also records #9629 (a link one client removes comes back when another sets NodeState on it, on main too) as `test.fails`, for a link, a link holding a mark and a link in a mark, both client id orders. ## Test plan `InlineElementTextFields.test.ts`: a new test counts `getFirstChild` calls while serializing 64, 128 and 256 nested inline elements, each with text fields, and requires each doubling to stay under 3x. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts × stays linear in the depth when each carries text fields AssertionError: expected 3.9063809967396366 to be less than 3 Tests 1 failed | 6 passed (7) ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Test Files 2 passed (2) Tests 1841 passed | 62 expected fail (1903) $ MATRIX_FULL=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 6982 passed | 206 expected fail (7188) ``` `getFirstChild` calls while serializing nested marks with text fields, at depth 32 / 64 / 128 / 256: 67 / 131 / 259 / 515, as on main.
…ide an update too
## Description
The previous commit cached, per frozen EditorState, whether each element
holds text, so serializing inline elements nested inside each other stays
linear. A copy serializes inside an update (`$getLexicalContent`,
`$generateJSONFromSelectedNodes`), on the pending state, which was not
cached. There it was still quadratic (review: 32,896 `getFirstChild` calls
at depth 256 for a select-all copy of nested marks with text fields, where
main makes 256).
The cache now covers every state, pending ones included. `getWritable`
drops the active state's entry, as it already drops the selection's cached
nodes, since a change to any node can change what its ancestors hold.
## Test plan
`InlineElementTextFields.test.ts` counts `getFirstChild` calls while
serializing 64, 128 and 256 nested inline elements with text fields, in a
committed state and inside an update, and requires each doubling to stay
under 3x. A new test checks that a change inside an update is seen by the
next serialization in it.
### Before
```
$ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts
× stays linear in the depth when each carries text fields, in an update
AssertionError: expected 3.9063809967396366 to be less than 3
Tests 1 failed | 8 passed (9)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts
Tests 9 passed (9)
```
`getFirstChild` calls inside `shouldSerializeTextStyles` for a select-all
`$generateJSONFromSelectedNodes` inside an update, nested marks with text
fields at depth 32 / 64 / 128 / 256: 32 / 64 / 128 / 256, as on main.
…y for states that cannot change
## Description
The previous commit cached, for pending states too, whether each element
holds text, and dropped a pending state's entries in `getWritable`. A
read made while a node is being cloned (an `afterCloneFrom` that calls
`exportJSON` on the node it is made from, which is allowed) ran after that
drop and before the change, and refilled the cache from the children the
change was about to replace. An inline element whose only text was then
removed kept answering "holds text", and lost its `textFormat` in JSON,
inside the update and after it (review).
The cache is now correct by construction: it holds entries only for
committed (frozen) states, which never change. A pending state uses the
cache of the committed state it was made from only while no node in it
has been marked dirty (written, created or removed): until then it holds
exactly that state's nodes. A copy serializes in such an update, so it
stays linear. An update that has changed nodes walks without a cache.
`getWritable` is unchanged.
## Test plan
`InlineElementTextFields.test.ts`: a new test with an inline element whose
`afterCloneFrom` reads the JSON of the node it is made from; removing its
only text keeps its `textFormat` in JSON, inside the update and once
committed.
### Before
```
$ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts
× a clone that reads JSON before the change it is made for does not keep what the change replaces
AssertionError: expected undefined to be 1 // Object.is equality
Tests 1 failed | 9 passed (10)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts
Tests 10 passed (10)
```
`getFirstChild` calls inside `shouldSerializeTextStyles`, nested marks with
text fields at depth 32 / 64 / 128 / 256: a select-all
`$generateJSONFromSelectedNodes` inside an update 32 / 64 / 128 / 256, and
`toJSON` of the committed state 67 / 131 / 259 / 515, as on main.
Renumbers the decision row this pull request adds to D82 (D81 is taken by
another pull request).
…ge in the same update
## Description
The previous commit caches whether each element holds text for committed
states, and for an update's pending state only until a node in it changes.
A copy made in an update that had already changed nodes (for example a
format change batched with the copy) walked without a cache. That was
quadratic for inline elements nested inside each other, each with text
fields (review: 528 / 2,080 / 8,256 / 32,896 visits at depth 32 / 64 / 128 /
256, where main makes 32 / 64 / 128 / 256).
An export walk (`EditorState#toJSON`, the clipboard's selection export) now
runs with a memo of its own (`INTERNAL_$withExportMemo`). It lives for the
walk alone, which changes no node. `getWritable` drops it for the rest of
the walk before cloning, so a clone hook that reads cannot refill it, and it
cannot answer for content that changed.
## Test plan
`InlineElementTextFields.test.ts`: the counted scaling test also copies
(`$generateJSONFromSelectedNodes`) after a format change in the same update.
### Before
```
$ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts
× stays linear in the depth when each carries text fields, in a copy after a change in the same update
AssertionError: expected 3.741687979539642 to be less than 3
Tests 1 failed | 10 passed (11)
```
### After
```
$ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts packages/lexical-clipboard
Test Files 6 passed (6)
Tests 50 passed (50)
```
`getFirstChild` calls inside `shouldSerializeTextStyles` for a copy after a
format change in the same update, nested marks with text fields at depth
32 / 64 / 128 / 256: 32 / 64 / 128 / 256, as on main.
zurfyx
changed the title
…leaves without text, and look stored formats up once per update ## Description Two follow-ups to settling a wrapper's text fields in a client's own delete (review): - **Fields the update sets.** The transform resets only a wrapper whose text fields are the ones it had before the update. It compared them inside a read of the previous state, where the wrapper resolved to its previous version, so the check always held: text fields the same update set on the link (or an app's transform set) were reset to the stored ones. The pending fields are now taken before that read. - **Cost.** The transform runs once for each changed stored element and looked their stored formats up one at a time: O(n²) for an update that empties n marks inside links. The formats at the stored nodes of a sequence are now noted in one pass, once per update. ## Test plan - `WrapperTextFieldsOwnDelete.test.ts`: 6 new cells. One update removes the link's text and sets its textFormat (0, 2, 16) and textStyle. The author, a fresh reader and the peer show them. - `WrapperTextFieldsCost.test.ts`: a second test counts the items checked while a client empties 32, 64 and 128 such marks in one update of its own. ### Before On 2b9d341: ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFieldsOwnDelete.test.ts packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFieldsCost.test.ts AssertionError: expected { format: +0, style: '' } to deeply equal { format: 2, style: 'color: red' } × wrappers a local delete leaves without text are settled in linear time AssertionError: expected 3.3445378151260505 to be less than 3 ``` (6 of the 12 own-delete cells fail; the remote cost test passes.) ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFieldsOwnDelete.test.ts packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFieldsCost.test.ts Tests 14 passed (14) $ MATRIX_FULL=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 9062 passed | 206 expected fail (9268) $ pnpm exec vitest run --project unit --project scripts-unit Tests 30301 passed | 230 expected fail | 135 skipped (30666) $ pnpm run ci-check Found 0 errors ```
There was a problem hiding this comment.
This looks good to me. It fixes #9597 at the root: a format over a link no longer writes the link's value, so a concurrent removal sticks. Inline elements' JSON now follows the rule the reconciler already uses, and the textless-link convergence gap from the previous revision is closed for both remote reads and the client's own delete. The new tests fail on main and pass here in both client id orders. The test.fails cells for #9627 and #9629 record the agreed follow-ups well.
A few optional, non-blocking suggestions:
.changeset/lexical-inline-element-text-fields.md:9. TheVerify:line names onlylink,autolinkandmark. The rule keys onisInline(), though, so an app's own inlineElementNodewhose children are all elements changes the same way..changeset/README.mdsaysVerify:tells readers what to test, so consider:Verify: JSON snapshots of an inline element (link, autolink, mark, or your own inline ElementNode) whose children are all elements no longer hold textFormat or textStyle.packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts. The file only exercises editor state and the clipboard's JSON export, and it passes unchanged with// @vitest-environment nodeadded after the header. AGENTS.md (Testing Strategy) asks for that line on such tests.packages/lexical-y/src/internal/Read.ts:2930.$settleWrapperTextFieldswas inserted between$setChildren's one-line JSDoc and$setChildren. Two doc blocks now sit back to back, and$setChildrenlost its comment. Moving that line down to just above$setChildrenfixes it.- Test plan. It ends with "The adversarial GPT review and the downstream suites are running." AGENTS.md asks the Test plan to carry the actual output of what was run, so paste those results once they finish, or drop the line.
Merging also ratifies D82 in packages/lexical-y/DECISIONS.md, the exported-JSON change for an inline element whose children are all elements. Older JSON still reads into the same state, so this is a maintainer sign-off rather than a migration.
|
|
||
| An element persists its textFormat and textStyle only when it has no text to derive them from (#7968), and a reconcile derives them from its first text at any depth. The JSON of an inline element (a link, an autolink, a mark) now follows the same rule: a link whose text sits in a mark no longer writes them, as a link with text of its own already did not. JSON that holds them still reads the same, and a block keeps the direct-children check, so the JSON of a paragraph whose text is all inside links, and of every other node, is unchanged. | ||
|
|
||
| Verify: JSON snapshots of a `link`, `autolink` or `mark` whose children are all elements no longer hold `textFormat` or `textStyle`. |
There was a problem hiding this comment.
Optional: the rule keys on isInline(), so an app's own inline ElementNode whose children are all elements changes the same way. .changeset/README.md says Verify: tells readers what to test, so consider naming it:
Verify: JSON snapshots of an inline element (link, autolink, mark, or your own inline ElementNode) whose children are all elements no longer hold textFormat or textStyle.
| @@ -2901,7 +2928,29 @@ export class Reader { | |||
| } | |||
|
|
|||
| /** Make `el`'s children exactly `children`, keeping unchanged ends. */ | |||
There was a problem hiding this comment.
Nit: this is $setChildren's doc comment, but $settleWrapperTextFields now sits between it and $setChildren. Two doc blocks are back to back here and $setChildren has none. Move this line down to just above $setChildren.
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| */ | ||
|
|
There was a problem hiding this comment.
Optional: this file only exercises editor state and the clipboard's JSON export, and it passes unchanged with // @vitest-environment node here. AGENTS.md (Testing Strategy) asks for that line on such tests.
…hangeset, run the core test in node, and put a doc comment back
## Description
Review suggestions, no behavior change:
- The `lexical` changeset's `Verify:` names an app's own inline `ElementNode` too, since the rule keys on `isInline()`.
- `InlineElementTextFields.test.ts` runs in the node environment (it only uses editor state and the clipboard's JSON export).
- `$setChildren`'s doc comment is back above it.
## Test plan
### Before
The test ran under jsdom.
### After
```
$ pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/InlineElementTextFields.test.ts
Tests 11 passed (11)
$ pnpm run ci-check
Found 0 errors
```
|
Thanks. In 4861932:
The older inline notes (
|
There was a problem hiding this comment.
This looks good to me. The binding now stores a wrapper's textFormat/textStyle only when Lexical serializes them, so a format over a link that holds text no longer restates the link and a collaborator's concurrent removal sticks. Core's inline-element JSON now omits exactly what a reconcile derives from deeper text, and JSON that still carries the two keys imports into the same state. The new tests fail on main and pass here in both client id orders. Merging also ratifies D82, the exported-JSON change for a link, autolink or mark whose children are all elements.
A few optional, non-blocking suggestions:
- Update the collab Y docs.
packages/lexical-website/docs/collaboration/y.md:1190-1193still says that undoing a link while a collaborator formats text in it keeps the link, because the format "writes the link there again"; S7-26 inregressions/Formats.test.tsnow expects that link gone on every peer. The Open issues list still carries #9597 (y.md:1325-1329), which this pull request closes, and #9521 (y.md:1313-1315), whose format-over-the-link's-start case is S7-26 (only its same-URL relink case remains).packages/lexical-y/AGENTS.mdasks a pull request that changes a recorded limit to update it in the same pull request. - Documents written before this change. A link that holds text and whose stored value already carries
textFormat(what the previous binding wrote after a format over a range covering the link) still comes back once in the same race: the value$wrapperAttrsnow computes omits the key (packages/lexical-y/src/internal/Schema.ts:2005-2010), differs from the stored one, and is written over the link's text again. The first such write (typing inside the link is enough) clears it. Since@lexical/yhas only nightly prereleases so far, a sentence in D82 seems enough. - Name #9627 in the known-failure titles. In
packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts:474-475the 56test.failscells name the issue only inknownTextlessLink's doc comment, while the #9629 cells carry(#9629)in their title.packages/lexical-y/AGENTS.mdasks a known failure to be atest.failsthat names its issue.
| [true, RACES.filter(knownTextlessLink)], | ||
| ] as const) { | ||
| (known ? test.fails : test).each(races)( | ||
| '$label: remove $removed, ids $ids.0,$ids.1', |
There was a problem hiding this comment.
Optional nit: these 56 test.fails cells name #9627 only in knownTextlessLink's doc comment, while the #9629 cells below carry (#9629) in their title. packages/lexical-y/AGENTS.md asks a known failure to name its issue; adding (#9627) to the title when known is true lets an unexpected pass say which issue it closes.
## Description Merges main at f59f74d (#9631, #9642 and #9641). There were no conflicts. ## Test plan ### Before The branch was on main 2e2acb7. ### After ``` $ pnpm exec vitest run --project unit --project scripts-unit Tests 30437 passed | 282 expected fail | 135 skipped (30854) $ MATRIX_FULL=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 9062 passed | 206 expected fail (9268) $ pnpm run ci-check Found 0 errors ```
…er protects, naming #9626 ## Description When two collaborators write a format at one spot at the same time, client id order decides which write wins, and part of a range can end plain (#9626). Before #9597's fix, a format over a link also wrote the link's value again, which opened the format inside the link. That protected the link's first word in some of these races. A link now behaves like a mark there (a comment on the same words loses the format on main the same way). Two shapes are pinned as expected failures naming #9626: - `BoldAcrossLinkEnd.test.ts`: A links t5z t6z, then bolds t4z..t7z, while B bolds t3z..t6z. t7z ends plain for both client id orders (main: for ids 1,2 only). The same race without a link fails for both orders on main too. - `BoldIntoLinkBehind.test.ts`: A bolds into a link's first word while B, one update behind, types inside the old bold run. t7z ends plain for ids 1,2 (main keeps it, and loses the space before the link in the same run). ## Test plan ### Before On main (f59f74d), the cells marked here are expected failures: - `BoldAcrossLinkEnd.test.ts`: the link cells fail for ids 1,2 and pass for ids 2,1; the no-link cells fail for both orders. - `BoldIntoLinkBehind.test.ts`: the typing cells pass. - The same shape with a comment instead of the link fails its 2 typing cells for ids 1,2 on main. ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/BoldAcrossLinkEnd.test.ts packages/lexical-y/src/__tests__/unit/regressions/BoldIntoLinkBehind.test.ts Tests 6 passed | 6 expected fail (12) ```
There was a problem hiding this comment.
This looks good to me. A format over a link that holds text no longer writes the link's value, so a link a collaborator removes at the same time stays removed. The core rule for inline elements now matches what the reconciler derives. The new tests fail against main and pass here, including the race in both client id orders and the author/fresh-reader JSON matrix.
A few optional, non-blocking suggestions:
-
Update the collab Y docs in this PR. In
packages/lexical-website/docs/collaboration/y.md:- The Undo paragraph under Limits (
:1201-1204) still says that formatting text in a link writes the link there again. - The Open issues list still carries #9597 (
:1336-1340) and the format-over-the-link's-start case of #9521 (:1324-1326). That case is S7-26, which now expects the link gone on every peer.
packages/lexical-y/AGENTS.mdasks a pull request that changes or accepts a limit to update itsDECISIONS.mdrow and y.md's Limits in the same pull request. The #9626 widening this PR accepts (at a link's edge, and on a link's first word) would fit as one sentence in D82 and one in Other races (:1282). - The Undo paragraph under Limits (
-
Mention values stored by earlier builds in D82. Take a link whose stored value already carries
textFormat, written by an earlier prerelease after a format over the link. It still comes back once in this race, because the first write of the link writes that value again. Any later write of the link clears it. -
Name #9627 in the expected-failure titles (
packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts:474-475), as the #9629 and #9626 pins do.packages/lexical-y/AGENTS.mdasks a known failure to be atest.failsthat names its issue.
The JSON change for an inline element whose children are all elements (D82) reads as a compatible behavior change in the sense of .changeset/README.md's Behavior changes section. Older JSON still imports into the same state, and the changeset's Verify: line tells snapshot owners what to expect.
| | D30 | **Diffed rewrites snap to whole words**; keystrokes and IME composition keep the exact diff. Scripts without spaces treat each character as a boundary. **A diffed insertion or deletion without a caret that the diff would put inside a word goes to the first place in the run of equal text it could be in where both its ends are at word edges** (a word deleted with the separator before it); a caret's placement, and one already at word edges, stay. | Concurrent rewrites otherwise split words. Real word segmentation is a follow-up ([#9467]). Without a caret a diff takes the common start first and put deletions inside words, so a collaborator's concurrent delete of the same word took a letter of the next one ([#9636]); the first edge keeps a word typed after a deleted word apart from the next one, as typing goes (a space, then a word). | `DECIDED` | 10-30 | [#9400], [#9467], [#9636] | | ||
| | D31 | **A list item's nested list gets an item of its own.** `NodeStructureExtension`'s list rule prevents an item holding a nested list next to other content when it is written; the binding does not repair it afterwards. | That shape lost text in outdents, Backspaces and offline merges. Preventing it at write time follows D20. | `DECIDED` | 10-26 | [#9452], [y.md: Limits][limits] | | ||
| | D32 | **List merging ignores `start`.** | Existing Lexical behaviour, intended. | `DECIDED` | 10-23 | [#9452] | | ||
| | D82 | **An inline element's textFormat and textStyle are persisted only while it holds no text at any depth, and the binding stores a wrapper's exactly when Lexical serializes them.** Lexical's `shouldSerializeTextStyles` looks through the elements inside an inline element (a mark in a link), as a reconcile already derives the two from its first text there; a block keeps the direct-children check. Exported JSON changes only for a link, autolink or mark whose children are all elements: those two fields are no longer written (they are still read, into the same state). | Stored while the wrapper holds text, a format change over a link writes the link's value again, and a collaborator's concurrent removal of the link is lost ([#9597]); left unstored while Lexical serializes them, the author's JSON differs from a fresh reader's. One rule, owned by core, keeps both, and the JSON of blocks and of every other node is unchanged (D01). A link with no text at all (only an inline decorator or a line break) still stores them and still comes back ([#9627]). | `DECIDED` | 10-30 | [#9601] | |
There was a problem hiding this comment.
Optional: two facts this row could carry. First, the trade this PR accepts: #9626 now also reaches a link's edge and a link's first word (pinned in BoldAcrossLinkEnd.test.ts and BoldIntoLinkBehind.test.ts). Second, a link value an earlier prerelease stored with textFormat still comes back once in the #9597 race, until the link's next write. packages/lexical-y/AGENTS.md asks a pull request that accepts a limit to record it here and in y.md's Limits.
| [true, RACES.filter(knownTextlessLink)], | ||
| ] as const) { | ||
| (known ? test.fails : test).each(races)( | ||
| '$label: remove $removed, ids $ids.0,$ids.1', |
There was a problem hiding this comment.
…text fields no longer stored change in the collaboration docs and D82 ## Description Review suggestions, no code change: - `docs/collaboration/y.md`: - The Undo limits now say that undoing a link while a collaborator formats text inside it removes the link on every client; a link with no text still keeps it (#9627). - Other races name #9626 for overlapping ranges, and its reach to a link's edge and a link's first word. - Open issues drop #9597 and #9521's format-over-the-link's-start case, which this pull request fixes. - D82 adds two facts. A value stored by an earlier build can come back once, until the link's next write. The #9626 trade this pull request accepts is formatting only. - The `WrapperTextFields.test.ts` expected failures for a link with no text name #9627 in their titles. ## Test plan ### Before The docs said a format in a link writes the link again, and listed #9597 as open. ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 2570 passed | 62 expected fail (2632) $ pnpm run ci-check Found 0 errors ```
There was a problem hiding this comment.
Thanks. The #9597 fix itself holds up: a format over a link that holds text no longer writes the link's value, and the new tests fail on main and pass here in both client id orders. One regression of the same kind needs addressing before merge, plus two small suggestions.
1. Deleting a formatted link's text now writes the link again, so a concurrent unlink or URL change is lost (packages/lexical-y/src/internal/Schema.ts:2005-2011)
Repro on the package's harness:
- Start with
"before " + link("wrapped", line break) + " after". - B bolds the line, and the bold syncs.
- At the same time, A unlinks the link (
$toggleLink(null)) or callssetURL('https://new.test'), while B selectswrappedand deletes it. The line break stays in the link. - Sync.
At this head the link comes back, or keeps its old URL, on every peer in both client id orders. On main, A's change survives in all eight cells. expectConverged passes, so A's edit is lost silently. An inline decorator in place of the line break gives the same result.
Cause: the bold set the link's textFormat while the link held text, so it was never stored. Once the delete leaves the link without text, shouldSerializeTextStyles() is true and $wrapperAttrs adds that textFormat. The value then differs from the stored one, and B writes the link's whole value again over the line break. That is the #9597 mechanism, triggered by a delete instead of a format. The link > mark > text version stays correct because settleTextFields resets the link's fields in the same update. Nothing does that for text directly inside the link.
Suggested fix: in the same update, show the stored text fields on a wrapper whose own text the update deleted, as settleTextFields already does for a wrapper above a stored element. Then no value is written. Also add a concurrent unlink and a concurrent retarget to the delete cells in WrapperTextFields.test.ts (:393-441), which today assert convergence only. If you would rather accept this shape, packages/lexical-y/AGENTS.md asks for a test.fails that names an issue, plus a line in D82 and in y.md's Limits.
A related, rarer shape: appending a text node into a link that holds only a line break or a decorator and stores its own textFormat also rewrites the value, because the stored fields drop out once it holds text. A concurrent unlink then comes back, and a concurrent retarget keeps the old URL on all or part of the link.
2. Nit: packages/lexical-y/src/internal/Binding.ts:1227 reads the committed state with editor.getEditorState().read(...). The root AGENTS.md (Reconciliation and Updates) asks new code to prefer editor.read('latest', ...). It reads the same state and also passes the editor, so a subclass whose isInline() or shouldSerializeTextStyles() calls $getEditor() keeps working.
3. Optional: S7-26 is the case #9521 was left open for, and it now passes; y.md already drops that bullet. Consider closing #9521 with this pull request too.
The JSON change for inline elements whose children are all elements (D82) reads as a compatible behavior change under the Backwards Compatibility section of AGENTS.md. Older JSON still imports into the same state, and the changeset's Verify: line tells snapshot owners what to expect. The accepted #9626 widening is recorded in D82 and y.md.
| if ( | ||
| (latest.getTextFormat() !== 0 || latest.getTextStyle() !== '') && | ||
| latest.shouldSerializeTextStyles() | ||
| ) { | ||
| $putTextFields(latest, own); | ||
| } |
There was a problem hiding this comment.
must fix: a delete that leaves a formatted link without text (its line break or inline decorator stays) makes this branch add a textFormat that was never stored. The value then differs from the stored one, and the link's whole value is written again over what remains. With the bold synced first, a concurrent $toggleLink(null) comes back and a concurrent setURL is lost on every peer, in both client id orders; main keeps both. The link > mark > text case is fine because settleTextFields resets the fields in the same update. Applying the stored fields to a wrapper whose own text the update deleted would avoid the write.
| // Only a wrapper this update left without text, with the text fields | ||
| // it had before the update: ones this update set (on a wrapper | ||
| // without text, or in a transform) are stored by the write. | ||
| const stale = editor.getEditorState().read(() => |
There was a problem hiding this comment.
nit: the root AGENTS.md asks new code to prefer editor.read('latest', ...). It reads the same committed state and passes the editor, so a subclass whose isInline() or shouldSerializeTextStyles() calls $getEditor() keeps working.
| expect(json(a.editor), 'a = fresh').toEqual(expected); | ||
| expect(json(b.editor), 'b = fresh').toEqual(expected); |
There was a problem hiding this comment.
consider: these cells check that both peers equal a fresh reader. Racing a concurrent unlink or URL change against the delete here would catch the case in Schema.ts:2005-2011, where the peers converge with the link back or with its old URL.
Description
A removes a link while B, at the same time, makes a range covering the whole link code (#9597): after the sync the link was back on every client. B's format also sets the link's textFormat (
RangeSelection#formatTextsets it on the element nodes the selection covers), and the binding stored a wrapper's textFormat and textStyle in the value its text carries (w:link). B's write of the new value went over the link's text as a new opener with a restore of the old value after it. A's concurrent removal deleted the opener B had seen and did not reach B's new one, so the link came back. Since #9598 (remade lines keep their common start and end) the same mechanism also brings a link back when a stale format meets a rewrite of the line.This pull request makes three changes:
Binding: a wrapper's textFormat and textStyle are stored only when Lexical serializes them (
ElementNode#shouldSerializeTextStyles). With text, they follow the text, and a format change over a link writes no link value. Documents that hold them in a link's value still read as before.Core:
shouldSerializeTextStylesnow checks an inline element (a link, an autolink, a mark) for a TextNode anywhere inside it, instead of only among its direct children. A reconcile already derives an element's textFormat and textStyle from its first text at any depth, so the JSON of an inline element now follows the same rule. A block keeps the direct-children check, so its JSON is unchanged.Consequence: because the binding uses that predicate, it agrees with Lexical by construction, including for a subclass that overrides it. The author's JSON and a fresh reader's agree for every nesting.
Textless wrappers: a wrapper that holds no text shows the text fields its value stores. So a delete that leaves a formatted link with only a line break or an inline decorator converges, whoever deletes: the formatting client's own value was set while the link held text, and was never stored.
Cost: an inline element now asks about its whole subtree. The answer for every element the walk visits is cached, and the cache is correct by construction:
EditorState#toJSON, the clipboard's selection export) in an update that has already changed nodes runs with a memo of its own (INTERNAL_$withExportMemo). The memo lives for the walk alone, which changes no node, andgetWritabledrops it for the rest of the walk before cloning.Serializing inline elements nested inside each other therefore stays linear, both in a committed state and when copying, including a copy after a change in the same update.
toJSONtakes 67/131/259/515getFirstChildcalls, and a select-all$generateJSONFromSelectedNodesinside an update takes 32/64/128/256. Both match main.Why the core change (the earlier "needs a core change" note)
It is still needed. To stop the resurrection when the link's text sits in a mark (link > mark > text), the binding must not store that link's textFormat. Before this change, Lexical still serialized it, because the link has no direct TextNode child. The author's JSON then had
textFormat: 16and a fresh reader's had none (the earlier head's open review item). There are two other ways to make them agree, and both are worse:So the rule belongs in core, and it is narrowed to inline elements to keep every other node's JSON unchanged.
Decision for review: exported JSON changes for one shape (D82)
This changes what
exportJSONwrites for exactly one shape: a link, autolink or mark whose children are all elements (for example a link whose text sits in a mark). That element no longer writestextFormat/textStyle. JSON that has them still reads into the same state.link(690),autolink(690) ormark(2,316) whose children are all elements, and each loses only those two keys.overflowis not inline, so it keeps the old rule.Recorded as D82 in
packages/lexical-y/DECISIONS.md(the number may shift at landing). Merging this pull request ratifies it.Not fixed here
A link that holds no text at all (only an inline decorator or a line break) has to keep storing the two fields, because Lexical serializes them. It still comes back in the same race (#9627). That needs a storage change. The new matrix marks those cells
test.failswith the issue number.The same mechanism also brings a removed link back when a collaborator deliberately changes the link at the same time, for example by setting NodeState on it (#9629, on main too). That is a change to the link itself, not a text format, so it is out of this pull request's contract. The matrix records it as
test.fails(link, link > mark, mark > link, both client id orders).Related: #9568 (element text fields depend on reconcile history). This change moves inline elements' JSON toward "a function of the document" and does not change that issue's block cases.
regressions/Formats.test.tsS7-26 asserted the old result of the same mechanism (B links "t7z t8z t9z" and undoes it while A formats a range over the link's start, and the link stayed). It now expects the link gone on every client, since B undid it and A did not touch the link.The collaboration docs (
docs/collaboration/y.md) say the same: the Undo limits, Other races (#9626 at a link's edge and first word), and Open issues (#9597 and #9521's link-start case dropped).Closes #9597
Known trade
This is a regression against main in one family of races, accepted for what the pull request fixes.
What changes. When two collaborators write the same format at one spot at the same time, client id order decides which write wins, and part of a range can end plain (#9626, open). Before this change, a format over a link also wrote the link's value again, which opened the format inside the link. That protected the link's first word in some of these races. A link now behaves like a mark there: a comment on the same words loses the format on main the same way. So this pull request widens #9626 to the other client id order when a range crosses a link edge, and to a link's first word when a collaborator one update behind types inside the old run. It loses formatting only, never text. Both shapes are pinned as expected failures naming #9626 (
BoldAcrossLinkEnd.test.ts,BoldIntoLinkBehind.test.ts).What it fixes.
LinkRetargetedWhileFormatted.test.ts).Numbers (randomized format and typing races, partial delivery, with links):
Changeset
lexical-y-link-textformat.md(@lexical/y, fix): Keep a link a collaborator removes removed when another collaborator formats text covering it at the same time.lexical-inline-element-text-fields.md(lexical, fix): Stop serializing the textFormat and textStyle of an inline element whose text sits in another element inside it. It has aVerify:line for JSON snapshots.Test plan
New tests:
unit/LinkRemovedWhileFormatted.test.ts: the issue's repro, both client id orders.unit/LinkWithMarkRemovedWhileFormatted.test.ts: the same with the link's text inside a mark.unit/regressions/WrapperTextFields.test.ts, a matrix:MATRIX_FULL=1). Inside them: plain, bold or styled text, two texts, a line break, text next to a nested mark or an inline decorator, a decorator alone, or a line break alone. Each with and without text fields of its own.unit/regressions/WrapperTextFieldsOwnDelete.test.ts:unit/regressions/LinkRetargetedWhileFormatted.test.ts: a client extends a link over more text with a new URL while another formats text covering it or inside it. Every character shows the new link (on main the old URL comes back over the first word, and the extension is cut).unit/regressions/RemadeTextStaleLink.test.ts(from [lexical-website][lexical-y] Bug Fix: Keep every unchanged word of a line whose text nodes an update makes again with several words changed #9625): its 20 cells for Bug: [lexical-y] A link a collaborator removes comes back when another collaborator formats a range covering it at the same time #9597 are plain tests now.unit/regressions/BoldAcrossLinkEnd.test.tsandunit/regressions/BoldIntoLinkBehind.test.ts: the races above, pinned as expected failures naming Bug: [lexical-y] Two collaborators bolding overlapping ranges at the same time leave part of the text plain #9626 (see Known trade).unit/regressions/WrapperTextFieldsCost.test.ts: counts the items checked while settling 32, 64 and 128 links left without text, by a remote read or by one local update. Each doubling must stay under 3x.lexical/src/__tests__/unit/InlineElementTextFields.test.ts: which elements persist the two fields, stored JSON that has them, counted scaling tests in a committed state, inside an update and for a copy after a change in the same update (each doubling of the nesting depth must stay under 3x thegetFirstChildcalls), a change inside an update seen by the next serialization in it, and a clone hook that reads JSON before the change it is made for.Before
On main (1853707):
(The
MATRIX_FULLcounts on main are from the matrix before the delete cells were added.)On bf0e800, before the reader change:
On main (551fe4a),
LinkRetargetedWhileFormatted.test.ts: 8 failed | 4 passed (12).On 7c1b49f,
WrapperTextFieldsOwnDelete.test.ts: 6 failed (no commit of the binding follows: expected 13 to be +0).On 4457a2f, before the own-delete and one-pass changes:
On the previous head (bf22fb9, rebased):
After
On 0e66653 (merged with main f59f74d):
The expected failures in the new matrix are #9627 (56 cells by default, 200 in the full run) and #9629 (6 cells).
Before the linear-walk commits, on 4072e4f (committed state) and ecf1f41 (inside an update); and on 37ed6f4, a clone hook that reads JSON before the change it is made for: