Repository navigation
Bug Fix: [lexical] Clamp committed selection points when a text node is shrunk - #9793
Rohan-Dedhia wants to merge 2 commits into
Conversation
|
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.
|
![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 picking up #9767. The commit-time bounds check in $beginUpdate is the right fix and resolves the reported throw. Unfortunately, the clamps added to the two setters break text splitting, so this needs changes before it can land.
1. setTextContent clamping the selection breaks splitText (blocking)
packages/lexical/src/nodes/LexicalTextNode.ts:1159-1173 clamps any selection point on the node as soon as its text is set. splitText calls self.setTextContent(firstPart) at :1411, and only afterwards (:1444-1445) reads the anchor/focus offsets it captured at :1384-1395 to move them into the right split node. Those offsets have now been clamped to the end of the first piece, so a caret or range after the split point collapses onto the first piece. Everything that splits a node under the caret is affected:
- Typing
hello #tag worldcharacter by character with the hashtag extension giveshello ag world#t. Typingsee https://example.com and morewith AutoLink givessee m and morehttps://example.co. The playground shows the same scrambling. - Existing unit tests that pass on
mainfail on this branch: thesplitText()+select()table inLexicalTextNode.test.tsx(6),MarkStateExtension.test.ts(7),Issue5366Repro.test.tsinlexical-markdown(5:##becomesh1), incremental typing inLexicalAutoLinkSamples.test.ts(2),MarkFlavors.test.ts(1) andMarkStateProperty.test.ts(1).
Suggested fix: remove the clamps from setTextContent and from spliceText (:1276-1294) and keep only the $beginUpdate check. With just that change, all seven of your new tests pass and the tests above keep passing.
2. The element-point clamp changes insertBefore/insertAfter(node, false)
The element branch in LexicalUpdates.ts:1435-1444 also rewrites committed element offsets. Four existing tests pin the current restoreSelection=false behavior and fail: in LexicalNode.test.ts and LexicalNodeState.test.ts, "Element-anchored selection on old parent (#6031) › … with restoreSelection=false skips the source-parent shift". #9767 only needs text points. Either drop the element branch, or keep it, update those tests, and describe the behavior change in the changeset. AGENTS.md ("Backwards Compatibility") asks that existing behavior be kept unless a change is deliberate.
3. Tests
The new tests reproduce the throw, which is great. expect(out!.focus).toBeLessThanOrEqual(out!.size) (Issue9767Repro.test.ts:83) also passes for offset 0, though. Asserting the exact committed point would pin the behavior. A case with the caret after a splitText boundary, or typing through a splitting transform, would have caught issue 1. Please also add a ## Test plan with real ### Before/### After output, as AGENTS.md ("Commits and Pull Requests") describes. Running pnpm run test-unit shows the failures above.
4. Optional nits
.changeset/clamp-selection-on-text-shrink.md: since this changes observable selection behavior, add aVerify:paragraph as AGENTS.md ("Changesets") asks. For example: "Verify: afterspliceText/setTextContentshortens a selected text node, the committed selection sits at the node's new end and a followingsplitTextno longer throws in dev."- In
$beginUpdate,point.set(key, size, type)would also mark the selection dirty and keep the composition key in sync, instead of assigningoffsetand clearing the caches by hand.
| const selection = $getSelection(); | ||
| if ($isRangeSelection(selection)) { | ||
| const key = self.__key; | ||
| let pointMutated = false; | ||
| for (const pt of [selection.anchor, selection.focus]) { | ||
| if (pt.type === 'text' && pt.key === key && pt.offset > text.length) { | ||
| pt.offset = text.length; | ||
| pointMutated = true; | ||
| } | ||
| } | ||
| if (pointMutated) { | ||
| selection._cachedNodes = null; | ||
| selection._cachedIsBackward = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
This clamp runs inside splitText too: splitText calls self.setTextContent(firstPart) (line 1411) and only then reads the anchor/focus offsets it saved (lines 1444-1445). A point that belonged in a later piece has already been pulled to the end of the first piece, so carets and ranges collapse there. Typing hello #tag world with the hashtag extension yields hello ag world#t, and the splitText() selection tests in LexicalTextNode.test.tsx fail. The commit-time check in $beginUpdate already fixes #9767 on its own, so this block (and the one in spliceText) can go.
| } else if (point.type === 'element' && $isElementNode(node)) { | ||
| const size = node.getChildrenSize(); | ||
| if (point.offset > size) { | ||
| point.offset = size; | ||
| selectionMutated = true; | ||
| } else if (point.offset < 0) { | ||
| point.offset = 0; | ||
| selectionMutated = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This element branch changes what insertBefore/insertAfter(node, false) commit. The four "#6031 … restoreSelection=false skips the source-parent shift" tests in LexicalNode.test.ts and LexicalNodeState.test.ts pin offset 3 here and now fail. #9767 only needs the text branch. If this one stays, please update those tests and mention it in the changeset.
| : null; | ||
| }); | ||
| expect(out).not.toBeNull(); | ||
| expect(out!.focus).toBeLessThanOrEqual(out!.size); |
There was a problem hiding this comment.
Optional: toBeLessThanOrEqual(size) also passes for offset 0. Asserting the exact committed points (anchor ab:1, focus c:1) would pin the intended clamp-to-end behavior.
|
Thanks for the thorough and helpful review! I have updated the PR to address all feedback:
All unit tests pass cleanly locally. |
There was a problem hiding this comment.
Thanks for the update, this looks good. Moving the fix into the commit-time check in $beginUpdate (packages/lexical/src/LexicalUpdates.ts:1421-1433) resolves #9767 without touching TextNode.setTextContent/spliceText, so splitText keeps the offsets it saved and element points keep their current behavior. Using Point.set there matches how @lexical/y clamps its own points to a node's size, and it keeps the selection's caches and dirty flag consistent. The new Issue9767Repro.test.ts fails on main and passes on this branch, and the tests the previous revision broke (splitText() selection, mark, heading-shortcut, AutoLink typing and the #6031 restoreSelection=false cases) pass again.
A few optional, non-blocking suggestions:
- Update the PR description. It still lists an "Active selection adjustment" in
spliceText/setTextContentand says$beginUpdateclamps element points withgetChildrenSize(). The diff now only clamps text points in$beginUpdate. The description becomes the commit message when this is merged, so it should describe the one change. AGENTS.md ("Commits and Pull Requests") also asks for the title in the form[lexical] Bug Fix: Clamp committed selection points when a text node is shrunk. - Add a Test plan. AGENTS.md ("Commits and Pull Requests") asks for a
## Test planwith### Beforeand### Aftersections holding the command and its real output, for examplepnpm run test-unit packages/lexical/src/__tests__/unit/Issue9767Repro.test.tsonmain(4 failed) and on this branch (7 passed). - Nit:
Issue9767Repro.test.ts:83checks only the focus offset. Also asserting the focus key, and that the anchor stays at offset 1 ofab, would pin the whole committed point.
The changeset looks right: 'lexical': patch, a [fix] summary and the Verify: paragraph CONTRIBUTING.md and .changeset/README.md ask for.
Fixes #9767.
Summary
When a text node is shortened programmatically (e.g. via
TextNode.spliceTextwithoutmoveSelection, or viaTextNode.setTextContent), an existingRangeSelectionpoint positioned inside that node can be left pointing to an offset beyond the node's new text length. In dev mode, a subsequent operation that normalizes or compares carets (such asPoint.isBefore/RangeSelection.isBackwardcalled fromsplitText) triggersinvariantin$getTextNodeOffsetwithinvalid offset X for size Y.Changes
TextNode.spliceText(when!moveSelection) andTextNode.setTextContent, clamp any text points in the activeRangeSelectionthat point into the modified node to the new text length, and invalidate cached selection nodes / backward state.$beginUpdate(LexicalUpdates.ts), validate and clamp text and element selection points against node boundaries (getTextContentSize()andgetChildrenSize()) so that all committed range selections remain strictly bounded.Issue9767Repro.test.tsand patch changeset in.changeset/clamp-selection-on-text-shrink.md.