Skip to content

[Breaking Change][lexical] Bug Fix: Adjust selection when removeFromParent callers move a node out of its parent#8501

Merged
etrepum merged 2 commits into
facebook:mainfrom
mayrang:fix/6031-removeFromParent-selection
May 12, 2026
Merged

[Breaking Change][lexical] Bug Fix: Adjust selection when removeFromParent callers move a node out of its parent#8501
etrepum merged 2 commits into
facebook:mainfrom
mayrang:fix/6031-removeFromParent-selection

Conversation

@mayrang
Copy link
Copy Markdown
Contributor

@mayrang mayrang commented May 12, 2026

Description

LexicalNode.prototype.replace, insertAfter, and insertBefore move a node from one parent to another by calling the internal removeFromParent helper, then re-wiring the node into its new home. removeFromParent is a low-level pointer-only operation that does not touch the editor's selection, and the three callers also did not adjust element-anchored offsets in the old parent. An {key: oldParentKey, type: 'element', offset: N} selection with N > removedIndex was left pointing past the now-shrunk parent's last child, sometimes out of range entirely (e.g. cursor at end of source, mover at index 0).

Each caller now captures the moved node's old parent and index before removeFromParent, then calls $updateElementSelectionOnCreateDeleteNode(selection, oldParent, oldIndex, -1) so element-anchored offsets past the removed index shift by one. The existing post-insertion +1 adjustment in insertBefore/insertAfter is unchanged, so within-parent moves stay in range throughout — previously the cursor at end of source went out of range while the parent was momentarily shrunk before the re-insertion.

The fix lives in the callers rather than removeFromParent because importing $updateElementSelectionOnCreateDeleteNode into LexicalUtils.ts creates a circular dependency with LexicalSelection.ts (fails at TabNode extends TextNode load with Class extends value undefined). removeFromParent keeps its signature; its JSDoc now documents the caller contract and points at the four call sites ($removeNode, replace, insertBefore, insertAfter) that follow the pattern.

Per review feedback (#8501 (comment)) the same selection-tracks-node-mutation pattern is also applied inside replace's element-anchored case: instead of $moveSelectionPointToEnd relocating the point to the receiver's last descendant, the point now maps to {key: other.__key, offset: prevSize + originalOffset, type: 'element'} where prevSize is other.getChildrenSize() before the children transfer. Receivers with no pre-existing children (e.g. $setBlocksType's freshly-created receivers) see prevSize = 0 so the original offset is preserved end-to-end. $moveSelectionPointToEnd stays as the fallback for !includeChildren (children destroyed, no relative position to preserve) and non-element points (text-anchored selections inside transferred children follow their text node's key, preserved across the transfer).

Breaking Change

Public methods now adjust the editor's selection differently after a node move:

  • Cross-parent moves via replace / insertAfter / insertBefore. Previously, an element-anchored selection in the source parent with offset > removedIndex was left unadjusted, sometimes pointing past the parent's last child. Now the offset shifts by -1. Downstream code that read $getSelection() immediately after one of these calls and relied on the stale offset will observe a different value.
  • Within-parent moves via insertBefore / insertAfter / replace. Previously the cursor at the end of the parent could briefly be left out of range while removeFromParent shrank the parent before the re-insertion grew it back. Now the pre-removal -1 shift composes with the existing post-insertion +1, so element-anchored offsets stay in range. For replace, this also means a within-parent cursor past the removed slot now shifts by -1 instead of staying put, matching the cross-parent case.
  • replace(other, true) with an element-anchored point on the replaced node. Previously, $moveSelectionPointToEnd relocated the point to the receiver's last descendant, losing the relative position. Now the point maps to {key: other.__key, offset: prevSize + originalOffset, type: 'element'} where prevSize = other.getChildrenSize() before the children transfer. Receivers with no pre-existing children (e.g. $setBlocksType's freshly-created receivers) see prevSize = 0, so the original offset is preserved. The legacy $moveSelectionPointToEnd fallback is kept for !includeChildren and non-element points (where there's no children-mapping to preserve).

Closes #6031

Test plan

  • pnpm vitest run --project unit — 114 files / 2540 tests pass. Existing Element-anchored selection on old parent (#6031) block plus the new replace(other, includeChildren) selection mapping block in LexicalNode.test.ts:
    • removeFromParent doesn't readjust selection #6031 blocktest.for over the three methods covering: cross-parent past-boundary shift / boundary preservation / non-collapsed selection / unrelated-parent untouched / restoreSelection: false skip / within-parent no-op end-cursor preservation.
    • replace selection mapping block — element-anchored point maps to prevSize + originalOffset (etrepum's snippet) / non-collapsed anchor+focus both shift / text-anchored selection inside transferred children unaffected / !includeChildren falls back to legacy $moveSelectionPointToEnd / fresh-receiver (prevSize=0) preserves original offset.
  • pnpm tsc --noEmit -p tsconfig.json clean.
  • Prettier / eslint clean on the changed files.

…e a node out of its parent

`removeFromParent` is a low-level pointer-only operation that does not
touch the editor's selection. `replace`, `insertAfter`, and
`insertBefore` call it as part of moving a node from one parent to
another, but they did not adjust element-anchored offsets in the old
parent, leaving them stale (and sometimes out of range) once the old
parent's child count dropped.

Each of the three call sites now captures the moved node's old parent
and index before `removeFromParent`, then calls
`$updateElementSelectionOnCreateDeleteNode(..., -1)` so any
element-anchored offset past the removed index shifts by one. The
existing post-insertion adjustment is unchanged, so within-parent moves
also stay in range.

`removeFromParent` keeps its signature; its JSDoc now documents the
caller contract.

Closes facebook#6031.
@vercel
Copy link
Copy Markdown

vercel Bot commented May 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview, Comment May 12, 2026 5:41pm
lexical-playground Ready Ready Preview, Comment May 12, 2026 5:41pm

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 12, 2026
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label May 12, 2026
@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented May 12, 2026

It might make sense to extend this sort of behavior to other methods, e.g. replace which currently has a $moveSelectionPointToEnd workaround that we are probably better off not having because other functions end up having to undo it like $setBlocksType

const parent = $createParagraphNode().append($createLineBreakNode(), $createLineBreakNode(), $createLineBreakNode());
$getRoot().clear().append(parent);
parent.select(1,1);
const newParent = $createParagraphNode().append('before');
const prevSize = newParent.getChildrenSize();
parent.replace(newParent, true);
const selection = $getSelection();
assert($isRangeSelection(selection));
expect(selection.isCollapsed()).toBe(true);
expect(selection.anchor).toMatchObject({key: newParent.getKey(), offset: prevSize + 1});

…aps element-anchored points by prevSize + originalOffset

Per facebook#8501 review feedback (facebook#8501 (comment)):
extend the selection-tracks-node-mutation pattern from facebook#6031 to
`replace`'s element-anchored case.

Previously, an element-anchored point on the replaced node was
relocated to the receiver's last descendant via
`$moveSelectionPointToEnd`, losing the relative position. The point
now maps to `{key: other.__key, offset: prevSize + originalOffset,
type: 'element'}` where `prevSize = other.getChildrenSize()` before
the children transfer. Receivers with no pre-existing children
(`$setBlocksType`'s freshly-created receivers) see `prevSize = 0`, so
the original offset is preserved end-to-end.

Falls back to the legacy `$moveSelectionPointToEnd` for
`!includeChildren` (children destroyed, no relative position to
preserve) and for non-element points (text-anchored selections inside
transferred children follow their text node's key, which is preserved
across the transfer).

Tests cover the maintainer's snippet plus non-collapsed, text-anchored,
!includeChildren-legacy, and prevSize=0 shapes.
@mayrang
Copy link
Copy Markdown
Contributor Author

mayrang commented May 12, 2026

Applied the replace extension in this PR — element-anchored points on the replaced node now map to {key: other.__key, offset: prevSize + originalOffset, type: 'element'} instead of $moveSelectionPointToEnd. Your snippet's expect(selection.anchor).toMatchObject({key: newParent.getKey(), offset: prevSize + 1}) passes; tests cover collapsed / non-collapsed / text-anchored (unaffected, key preserved across child transfer) / !includeChildren (legacy fallback retained) / prevSize=0 (the $setBlocksType shape — original offset preserved).

Held back on the $setBlocksType cleanup itself for this PR. The cloned-newSelection + per-iteration override + selection.is($getSelection()) commit isn't actually redundant in the way it looks — it relies on input and current selection being the same object and getting mutated in place by replace. That's true for the base replace, but ListItemNode.replace (lexical-list) is a structurally different override (no clone, list.insertBefore/After(replaceWithNode) + this.remove()); its $removeNode → moveSelectionPointToSibling lands the selection on a sibling LI, and the $setBlocksType commit is what overrides that back to the new paragraph. Cleaning up $setBlocksType requires also teaching ListItemNode.replace to handle the selection-on-this case (same prevSize + offset pattern, or an explicit replaceWithNode redirect), which feels like a separate scope from #6031 / your base replace suggestion. Can follow up with a lexical-list PR for that.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented May 12, 2026

I think that's a good rationale for a follow-up, and maybe while we're in there it would make sense to do some general cleanup/optimization by using splice when we're moving all children instead of the pattern of getChildren().forEach(() => /*insert/append/etc.*/). splice is currently the primitive method so might as well call it once instead of N times.

@mayrang
Copy link
Copy Markdown
Contributor Author

mayrang commented May 12, 2026

Thanks — tried the swap locally on the replace site and looks clean (lexical / lexical-list / lexical-selection unit suites all pass). Will fold it in with the $setBlocksType / ListItemNode.replace cleanup as the follow-up.

@etrepum etrepum added this pull request to the merge queue May 12, 2026
Merged via the queue into facebook:main with commit 566dc4f May 12, 2026
42 checks passed
mayrang added a commit to mayrang/lexical that referenced this pull request May 13, 2026
…ntralize replace-area selection mapping + bulk splice

Follow-up to facebook#8501. The base LexicalNode.replace now maps element-anchored selections from the replaced node to {key: replacement, offset: prevSize + originalOffset, type: 'element'}. Two pieces were held back for this PR: the same mapping in ListItemNode.replace (an override that bypasses super.replace), and removal of the \$setBlocksType workaround that compensated for the lossy pre-facebook#8501 behavior.

ListItemNode.replace now mirrors the base prevSize + offset mapping in the includeChildren branch. The mapping runs before the trailing this.remove() so the subsequent \$removeNode → moveSelectionPointToSibling sees anchor/focus already redirected and is a no-op for those points. Text-anchored selections inside transferred children follow their text node's key across the children move, so no extra mapping is needed there.

\$setBlocksType drops the cloned-newSelection + per-iteration override + selection.is(\$getSelection()) commit. Both replace paths now handle the mapping, making the caller-side workaround redundant.

getChildren().forEach(parent.append) patterns collapse to parent.splice(parent.getChildrenSize(), 0, getChildren()) at four sites: base LexicalNode.replace, ListItemNode.replace, \$setBlocksType's wrap branch, and \$toggleLink's unlink branch. Since append is splice-of-1, this is one bookkeeping pass instead of N. \$toggleLink also switches to parentLink.getParentOrThrow() to keep the existence assertion loud.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

removeFromParent doesn't readjust selection

2 participants