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
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.
There was a problem hiding this comment.
Done in b415918: the previous state is read with editor.read('latest', …).
| 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.
…text a local delete removes ## Description Review of 15bb1d0, a regression against main through a delete. Take "before " + a link ("wrapped", line break) + " after". B bolds the line, which syncs. Then, at the same time, A removes the link or gives it another URL, while B deletes "wrapped" (the line break stays in the link). The link came back, or kept its old URL, on every client, in both client id orders. Cause: once the delete left the link without text, Lexical serialized the text format B's bold had set on it, which was never stored. B's write then wrote the link's whole value again over the line break, over A's change. A link around a mark was already handled: the text fields were reset in the same update from the stored element the delete marks dirty. Nothing did that for text directly inside the link. The same transform now also runs on the wrapper itself. A wrapper this update left without text, with the text fields it had before the update, shows the value stored at its first atom. That value comes from one walk of its line, and the offsets of all the line's wrappers from one pass, both once per update. The previous state is read with `editor.read('latest', …)` (review nit). Also: - `WrapperTextFields.test.ts` adds the review's cells: the delete with a concurrent removal or URL change, for a line break and an inline decorator left in the wrapper, every nesting, both client id orders. - It also adds the rarer shape: a wrapper holding only a line break or an inline decorator, with text fields of its own, that one client appends text into while the other removes it or changes its URL. Where the text goes into the link itself, the link comes back or keeps its old URL (on main it does not). That is #9627's mechanism, a text-less link storing its text fields in its value: the append drops them and writes the value again. Those cells are expected failures naming #9627, and D82 and the docs say so. - `WrapperTextFieldsCost.test.ts` adds a counted test: a local delete in one line of 32, 64 and 128 links. ## Test plan ### Before On 15bb1d0: ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts -t 'removes or retargets' × link>line-break: remove link, ids 1,2 (and 63 more: every link shape, remove and retarget, both orders) Tests 64 failed | 376 passed | 2632 skipped (3072) ``` On main (f59f74d) the same cells pass 440 of 440. The append cells pass 220 of 220 on main; on this branch, 40 fail, the ones now marked for #9627. ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFieldsCost.test.ts Tests 3193 passed | 102 expected fail (3295) $ MATRIX_FULL=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 11562 passed | 310 expected fail (11872) $ pnpm exec vitest run --project unit --project scripts-unit Tests 31058 passed | 322 expected fail | 135 skipped (31515) $ pnpm run ci-check Found 0 errors ``` The counted test fails when the per-update cache of a line's atoms is turned off (ratio 3.6).
|
Thanks, fixed in b415918.
Gates:
|
…xt, so a concurrent removal or new URL stays ## Description A link holding only a line break or an inline decorator stores its text fields in its value (Lexical serializes them while it holds no text). One client appended text into it while the other removed it, or gave it another URL (review). On the previous head, the removal or new URL was lost on every client, while main keeps it: - the append left the link holding text, so its text fields were no longer part of the value the binding wants; - the writer wrote the link's value again, without them, over the line break it kept, over the concurrent change. A wrapper that holds text now writes no text fields and removes none stored there either. Its value is marked as such (`isTextHoldingWrapperValue`), and `formatAsStored` keeps the stored text fields of such a value, like members this editor does not own. So nothing is written for the append, and the writer, its inserts and the self-check agree. The reader's reuse check of a wrapper leaves the text fields out. They are stored only while the wrapper holds no text, which its children decide, and they are then shown from the stored value, also for a wrapper whose text fields were the default before (`$unstoredWrapperTextFields` no longer skips those). The 40 cells pinned for #9627 in the previous commit pass, and are plain tests now. D82 and the docs say so. ## Test plan ### Before On b415918: ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts -t 'appends text into' × link>no-text own: remove link, ids 1,2 (and 39 more: every shape where the text goes into the link itself, remove and retarget, both orders) ``` On main (f59f74d), the same 220 cells pass. ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFieldsOwnDelete.test.ts packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFieldsCost.test.ts packages/lexical-y/src/__tests__/unit/regressions/BoldAcrossLinkEnd.test.ts packages/lexical-y/src/__tests__/unit/regressions/BoldIntoLinkBehind.test.ts packages/lexical-y/src/__tests__/unit/regressions/LinkRetargetedWhileFormatted.test.ts Tests 3263 passed | 68 expected fail (3331) $ MATRIX_FULL=1 pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/WrapperTextFields.test.ts Tests 11666 passed | 206 expected fail (11872) $ pnpm exec vitest run --project unit --project scripts-unit Tests 31098 passed | 282 expected fail | 135 skipped (31515) $ pnpm run ci-check Found 0 errors ```
## Description Merges main at c0c7e42 (#9640, #9600, #9652, #9659, #9648 and #9657). The one conflict is in the link list of `packages/lexical-y/DECISIONS.md`, where both sides added a reference (#9597 here, #9606 on main); both are kept. ## Test plan ### Before The branch was on main f59f74d. ### After `pnpm run ci-check` and `pnpm exec vitest run --project unit --project scripts-unit` pass on the merged tree (outputs in the pull request).
|
Summary of the 15bb1d0 review, for the current head 524365a:
|
There was a problem hiding this comment.
This looks good to me. Storing a wrapper's textFormat/textStyle only where Lexical serializes them keeps a removed link removed in both client id orders, and the new core rule for inline elements matches what the reconciler already derives from an element's first text. The JSON change for an inline element whose children are all elements stays within the backwards-compatibility rule in AGENTS.md (JSON written by older versions still deserializes into the same state), and the changeset carries the Verify: paragraph CONTRIBUTING.md asks of a fix that changes output. The formatting trade in overlapping bold races at a link's edge (#9626) and the remaining gaps (#9627, #9629) are recorded in D82, y.md and pinned test.fails cells, so I'm not treating them as blockers; merging ratifies D82.
Optional, non-blocking suggestions:
packages/lexical-y/src/internal/Schema.ts:1986: the newtextHoldingValuesblock sits between$wrapperAttrs's doc comment (lines 1978-1985) and the function, so that comment now documents the WeakSet. Moving the new block above it gives$wrapperAttrsits description back. The comment on$putTextFields(lines 2173-2177) still says wrapper items always carry the text fields, which no longer holds.packages/lexical-y/src/internal/Schema.ts:2043andpackages/lexical-y/src/internal/Binding.ts:1240:shouldSerializeTextStyles()runs before the cheap field check in the transform, and twice in$wrapperAttrsfor a wrapper with text fields. CheckinggetTextFormat()/getTextStyle()first and reusing one result keeps the deep walk off the common path.packages/lexical-y/src/internal/Write.ts:3745:isTextHoldingWrapperValuedepends on the same object reachingformatAsStored; any copy of the value on the way drops the tag. An explicit flag or parameter would not depend on that.packages/lexical-clipboard/src/clipboard.tschanges without a changeset naming@lexical/clipboard, which.changeset/README.md("When a changeset is needed") says the Changesets job reports. Adding'@lexical/clipboard': patchto.changeset/lexical-inline-element-text-fields.mdcovers it.
| * format items: concurrent changes to two fields of one wrapper do not merge | ||
| * (the last one wins). | ||
| */ | ||
| /** |
There was a problem hiding this comment.
Nit (optional): this block now sits between $wrapperAttrs's doc comment (lines 1978-1985) and the function, so that comment documents textHoldingValues instead. Moving this block above line 1978 gives $wrapperAttrs its description back.
| if (instance !== null) { | ||
| own[INSTANCE_KEY] = instance; | ||
| } | ||
| if (!latest.shouldSerializeTextStyles()) { |
There was a problem hiding this comment.
Nit (optional): shouldSerializeTextStyles() is already computed at line 2023 when the wrapper has text fields; keeping one result avoids a second subtree walk. The transform in Binding.ts:1240 could also test getTextFormat()/getTextStyle() before calling it.
| // link, then text appended), its value is not written again over the | ||
| // content it kept, over a collaborator's concurrent removal or change | ||
| // of it (see $wrapperAttrs). | ||
| const keepsText = isTextHoldingWrapperValue(value); |
There was a problem hiding this comment.
Optional: this relies on the very object $wrapperAttrs tagged reaching formatAsStored; a copy of the value on the way would drop the tag and make the text fields owned again. An explicit flag on the value or a parameter would not depend on that.
…changeset and put two comments back in place ## Description Review suggestions, with no logic change: - The `lexical` changeset also names `@lexical/clipboard`, whose `$generateJSONFromSelectedNodes` this pull request changes. - In `Schema.ts`: - `$wrapperAttrs`'s doc comment is back above it (the text-holding value block sat between them); - `$putTextFields`'s comment says wrapper items carry the text fields only while the wrapper holds no text. ## Test plan ### Before The changeset named `lexical` only. ### After ``` $ pnpm run ci-check Found 0 errors ```
There was a problem hiding this comment.
This looks good to me. Storing a wrapper's textFormat/textStyle only where Lexical serializes them keeps a removed link removed in both client id orders, and the new rule for inline elements in ElementNode#shouldSerializeTextStyles matches what the reconciler already derives from an element's first text. The JSON change for an inline element whose children are all elements stays within the backwards-compatibility rule in AGENTS.md (JSON written by older versions still deserializes into the same state), and both changesets carry the Verify: paragraph that CONTRIBUTING.md and .changeset/README.md ask of a fix that changes output. The overlapping-bold trade at a link's edge (#9626) and the remaining gaps (#9627, #9629) are recorded in D82, y.md and pinned test.fails cells, and the two follow-ups in the description are agreed, so I'm not treating any of them as blockers; merging ratifies D82.
One optional, non-blocking suggestion:
packages/lexical-y/src/internal/Schema.ts:2043:$wrapperAttrscallsshouldSerializeTextStyles()for every wrapper it writes, to decide the text-holding tag, so the description's "the binding asks only when the wrapper has text fields to store" does not hold yet. Folding this call into the reorder already listed under Follow-ups (checkgetTextFormat()/getTextStyle()first and reuse one result) would make it true. The cost today is small, since a text-holding wrapper's walk stops at its first text.
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.
Read: when a read builds the wrapper's line, and when it changes the content of a stored element inside the wrapper (a mark inside a link) without reading its line.
Own delete: when a client's own delete leaves such a wrapper with the text fields it had before the update, the wrapper shows the stored ones in that same update, so no commit follows for an app to answer. This comes from a node transform:
Otherwise the write would write the link's whole value again over the line break or decorator it keeps, over a collaborator's concurrent removal or URL change. Text fields the update (or an app's transform) sets are the client's own and are stored, as are fields set on a wrapper that already holds no text.
Getting text: a wrapper that holds text writes no text fields and removes none stored there either. When a link holding only a line break gets text, its value is not written again over the line break, so a concurrent removal or URL change stays. The writer treats those two members of a text-holding wrapper's value like members it does not own (
formatAsStored).Cost: the stored values are found in one pass along each sequence (once per update for the transform).
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):
Follow-ups
Review suggestions left for a separate pull request:
$wrapperAttrs, checkgetTextFormat()/getTextStyle()beforeshouldSerializeTextStyles(), and reuse one result of it, to keep the deep walk off the common path.isTextHoldingWrapperValuedepends on the same value object reachingWriter#formatAsStored; an explicit flag or parameter would not depend on that.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(lexicaland@lexical/clipboard, 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 15bb1d0, the delete with a concurrent removal or URL change of the link (
WrapperTextFields.test.ts, "removes or retargets"): 64 failed | 376 passed (main passes all 440).On 4457a2f, before the own-delete and one-pass changes:
On the previous head (bf22fb9, rebased):
After
On 524365a (merged with main c0c7e42):
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: