Repository navigation
[lexical-y] Bug Fix: Typing over a range formatted from inside an emoji keeps the node format and re-reads the line in place (#9717) - #9724
Conversation
…ji keeps the node format (meta#9717)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
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: |
![meta-code-review[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for tackling #9717. The in-place line re-read works, and the three format cases fail without the change and pass with it. One issue needs fixing before this can merge.
1. The line-relative selection restore also runs after full container re-reads (packages/lexical-y/src/internal/Binding.ts:5350)
$rereadRefused now tries $restoreLineRelativeSelection first for every queued re-read, not only for the lines in refusedLines. Containers in refused are read in full because the editor shows something that is not stored, for example two adjacent text nodes with equal attributes that are stored as one run (Write.ts:2987-2988). Their line keeps its key while its children change, so $resolveLinePoint follows the old child index into the new structure and the Yjs-cursor restore never runs. A repro in the shape of the existing object-NodeState case in SurrogateSelection.test.ts, using the harness and $typeZ from your test file:
const objectState = createState('object', {
parse: (v: unknown) => (v !== null && typeof v === 'object' ? v : null),
});
const t = await setup(() =>
$getRoot().append($createParagraphNode().append($createTextNode('x'))),
);
t.b.update(() => {
const second = $setState($createTextNode('cd'), objectState, {v: 1});
($getRoot().getFirstChild() as ElementNode).clear().append(
$setState($createTextNode('ab'), objectState, {v: 1}),
second,
$createTextNode('ef').toggleFormat('bold'),
);
const s = $createRangeSelection();
s.anchor.set(second.getKey(), 0, 'text');
s.focus.set(second.getKey(), 1, 'text'); // selects "c"
$setSelection(s);
});
await t.sync();
t.b.editor.update($typeZ, {discrete: true});
await t.sync();On main the selection stays on c and the paragraph becomes "ab" "Z" "d" "ef"{1}. On this branch the selection moves to e in the bold node and the paragraph becomes "abcd" "Zf"{1}: the unselected e is deleted and the selected c stays. A collapsed caret in "cd" moves into "ef" the same way. Restricting the line-relative restore to the lines this pass re-read in place (for example only when no container was read in full and both points' lines are among them), and keeping $selectionAfterRead(cursor, caret) otherwise, avoids this. packages/lexical-y/AGENTS.md asks every bug fix to land with a regression test in src/__tests__/unit/regressions/, so please add this case there as well.
Optional, non-blocking suggestions
SurrogateSelectionFormat.test.ts:121: the "re-reads only the touched line" test also passes onmain, because a full container read keeps every paragraph's key. Asserting thatReader#$readFullis not called (for example withvi.spyOn(Reader.prototype, '$readFull')) would make it fail without the in-place read.- The cursor capture and restore are wrapped in try/catch so that putting the selection back never fails the read it ends (
Binding.ts:6054-6060,:6202-6213).$lineRelativeSelectionand$restoreLineRelativeSelectioncould follow the same pattern and fall back to the cursor path. - When a remote transaction is read before the queued re-read runs, refused lines are re-read in place but the selection is still restored from the Yjs cursor (
Binding.ts:3069-3075), so the #9717 symptom can remain on that path. - The two new methods sit between
$caretOnText's JSDoc (Binding.ts:6075-6084) and$caretOnTextitself; moving them above that comment keeps it attached to the right method. - AGENTS.md ("Commits and Pull Requests") asks the Test plan to paste the actual before and after output of the command that was run, rather than describing it.
| }); | ||
| reader.$settle(settle); | ||
| this.$selectionAfterRead(cursor, caret); | ||
| if (!this.$restoreLineRelativeSelection(lineSel, caret)) { |
There was a problem hiding this comment.
This runs for every queued re-read, including the containers in refused that were just read in full (for example a line whose two equal-attribute text nodes are stored as one run). Those reads change the line's children while the line keeps its key, so the child path captured before the read can name a different node: with "ab" "cd" (equal object NodeState) and bold "ef", a selection over c comes back over e, and typing deletes e. Could the line-relative restore apply only to the lines this pass re-read in place, with $selectionAfterRead(cursor, caret) for everything else?
| }); | ||
| } | ||
|
|
||
| test('re-reads only the touched line without recreating other lines in the container', async () => { |
There was a problem hiding this comment.
Optional: this test also passes on main, because a full container read keeps every paragraph's key. Spying on Reader.prototype.$readFull and asserting it is not called would make it fail without the in-place read.
| * new paragraph), and Backspace deletes structure (a nested list of other | ||
| * users' text). | ||
| */ | ||
| private $lineRelativeSelection(): LineRelativeSelection | null { |
There was a problem hiding this comment.
Nit: the JSDoc just above belongs to $caretOnText (now further down); moving the two new methods above that comment keeps it attached to the right method.
Description
When a range selection with one end between the halves of a surrogate pair is formatted (e.g. bolding
[3, 6)or[2, 3)inab😀cd), Lexical splits the pair across adjacent text nodes or retains one half. Under D57,@lexical/ywrites lone surrogate halves asU+FFFDand queues a re-read so the author's editor reflects what other peers and fresh reads show.During this re-read:
$resolvePositionand$pointAtAtom). At the boundary between text nodes,$pointAtAtomresolved the point to the end of the preceding plain text node rather than offset 0 of the formatted text node. Consequently, typingZover the selection inserted text with the unformatted node's format.$writeLineAloneadded the containeru.cstothis.refused, causing the re-read to reconstruct the entire document container (such as every paragraph in the root) instead of re-reading only the affected line.This PR addresses both:
U+FFFDpreserves character length 1:1 and maintains the line's node hierarchy, the anchor and focus remain on the formatted text node.refusedLinesand usesReader#$readLineInPlaceto re-read only the touched line in place without reconstructing the rest of the container.Closes #9717
Changeset
.changeset/lexical-y-surrogate-reread-selection-format.mdincluded (patch for@lexical/y).Test plan
Before
"ab\ufffdZ"instead of"ab\ufffd" "Z"{bold}).After
SurrogateSelectionFormat.test.tspass:[3, 6),[6, 3), and[2, 3)) retain the bold format when typed over, matching plain Lexical behavior and converging across peers.@lexical/y(5,323 tests) pass.