Skip to content

[lexical][lexical-selection][lexical-list] Bug Fix: Skip named-slot values in block converters, diagnose replace() on a slot value, and delete slot-bearing decorator hosts at the selection boundary - #8905

Open
etrepum wants to merge 2 commits into
facebook:mainfrom
etrepum:claude/elementnode-replace-audit-ahvm8e

Conversation

@etrepum

@etrepum etrepum commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Description

Follow-up to #8901 (fix for #8894), which made $setBlocksType skip named slot values. The same crash class exists in every other selection-driven block converter that resolves the current block and replaces it through the tree API, and a direct replace() of a slot value still fails with the unhelpful generic "Expected node %s to have a parent" invariant. This audits and fixes the rest, applying the same rule: a slot value's assignment is managed by the node or extension that owns the slot, so generic converters leave it as-is. The eligibility checks test the slot up-link ($getSlotHost) rather than parentless-ness, so a detached node is never conflated with a slot value and keeps failing loudly.

  • lexical: LexicalNode.replace() on a slot value now throws a slot-specific invariant before any mutation, including the keys and types of the value and its host for debugging (replace: node %s (type %s) is slotted into host %s (type %s); … Use $setSlot on its host to assign a replacement.), mirroring the existing $removeFromParent guard for remove() on a slotted node.
  • @lexical/selection: the deprecated $wrapNodes no-ops when the selection lives in a bare-block slot frame (it previously threw in the sibling walk); a shadow-root slot value still wraps its own subtree normally.
  • @lexical/list: $insertList skips slot values in its empty-block branch, its empty-list-item branch, and its walk-up list-type-change branch; $removeList skips a slotted top-level list (they previously threw or vacated the slot).
  • @lexical/markdown: audited, already safe — the element/multiline shortcut runners bail unless the block's parent is a root or shadow root, which excludes slot values.

The rule is documented in the named-slots concepts page.

The audit also turned up the inverse problem: deleting a slot-bearing host from outside. Backspace with the caret at the boundary after a slot-bearing DecoratorNode host (e.g. the playground PullQuote) was a silent no-op — deleteCharacter special-cased slot-bearing decorators to never be removed via backspace, leaving them undeletable from the keyboard. That special case is removed, so a slot-bearing decorator behaves like any other block decorator at the boundary: backspace removes it as a whole unit (slots included), and from an adjacent empty paragraph it drops the paragraph and node-selects the host so the next backspace deletes it. The ElementNode-host merge guard is intentionally unchanged: a cross-block merge would keep the host's children while silently dropping its slots, so backspace at the start of a slot-bearing host's child still leaves the host in place.

Closes #8904

Test plan

Before

$insertList, $removeList, and $wrapNodes threw "Expected node %s to have a parent" (or silently vacated a slot) when the selection was inside a bare-block named-slot value or a slotted list; replace() on a slot value threw the same generic invariant. Backspace after a slot-bearing decorator host did nothing; the host could not be deleted from the keyboard.

After

The converters leave slot values as-is and replace() on a slot value throws a diagnostic pointing at $setSlot. Backspace removes a slot-bearing decorator host at the boundary (or node-selects it from an adjacent empty paragraph, matching normal block decorator behavior). New unit tests in LexicalSetBlocksTypeSlots.test.ts and LexicalFormatListSlots.test.ts, plus replace() and boundary-deletion tests in LexicalSlot.test.ts; the existing test that a slot-bearing ElementNode host is not merged away still passes.

@vercel

vercel Bot commented Aug 2, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
lexical Ready Ready Preview Aug 2, 2026 4:40pm
lexical-playground Ready Ready Preview Aug 2, 2026 4:40pm

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 Aug 2, 2026
@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Aug 2, 2026
@etrepum
etrepum marked this pull request as ready for review August 2, 2026 03:57
@etrepum
etrepum force-pushed the claude/elementnode-replace-audit-ahvm8e branch from 1d47702 to be0aa3c Compare August 2, 2026 04:25
@etrepum
etrepum force-pushed the claude/elementnode-replace-audit-ahvm8e branch from be0aa3c to 7ecc2f4 Compare August 2, 2026 04:27
@etrepum
etrepum force-pushed the claude/elementnode-replace-audit-ahvm8e branch from 7ecc2f4 to 438c50d Compare August 2, 2026 04:30
@etrepum
etrepum force-pushed the claude/elementnode-replace-audit-ahvm8e branch from 438c50d to da12bff Compare August 2, 2026 04:33
…alues in the remaining block converters and diagnose replace() on a slot value

## Description

Follow-up to facebook#8901 (fix for facebook#8894), which made $setBlocksType skip named slot values. The same crash class exists in every other selection-driven block converter that resolves the current block and replaces it through the tree API, and a direct replace() of a slot value still fails with the unhelpful generic "Expected node %s to have a parent" invariant. This audits and fixes the rest, applying the same rule: a slot value's assignment is managed by the node or extension that owns the slot, so generic converters leave it as-is. The eligibility checks test the slot up-link ($getSlotHost) rather than parentless-ness, so a detached node is never conflated with a slot value and keeps failing loudly.

- lexical: LexicalNode.replace() on a slot value now throws a slot-specific invariant before any mutation ("use $setSlot on its host to assign a replacement"), mirroring the existing $removeFromParent guard for remove() on a slotted node.
- @lexical/selection: the deprecated $wrapNodes no-ops when the selection lives in a bare-block slot frame (it previously threw in the sibling walk); a shadow-root slot value still wraps its own subtree normally.
- @lexical/list: $insertList skips slot values in its empty-block branch, its empty-list-item branch, and its walk-up list-type-change branch; $removeList skips a slotted top-level list (they previously threw or vacated the slot).
- @lexical/markdown: audited, already safe — the element/multiline shortcut runners bail unless the block's parent is a root or shadow root, which excludes slot values.

The rule is documented in the named-slots concepts page.

## Test plan

### Before

$insertList, $removeList, and $wrapNodes threw "Expected node %s to have a parent" (or silently vacated a slot) when the selection was inside a bare-block named-slot value or a slotted list; replace() on a slot value threw the same generic invariant.

### After

The converters leave slot values as-is and replace() on a slot value throws a diagnostic pointing at $setSlot. New unit tests in LexicalSetBlocksTypeSlots.test.ts and LexicalFormatListSlots.test.ts, plus a replace() test in LexicalSlot.test.ts.
…ion boundary

## Description

Backspace with the caret at the boundary after a slot-bearing DecoratorNode host (e.g. the playground PullQuote) was a silent no-op: deleteCharacter special-cased slot-bearing decorators to never be removed via backspace, leaving them undeletable from the keyboard. Remove that special case so a slot-bearing decorator behaves like any other block decorator at the boundary: backspace removes it as a whole unit (slots included), and from an adjacent empty paragraph it drops the paragraph and node-selects the host so the next backspace deletes it.

The ElementNode-host merge guard is intentionally unchanged: a cross-block merge would keep the host's children while silently dropping its slots, so backspace at the start of a slot-bearing host's child still leaves the host in place.

Closes facebook#8904

## Test plan

### Before

With the caret at the start of the block after a slot-bearing decorator host, Backspace did nothing; the host could not be deleted from the keyboard.

### After

Backspace removes the host (or node-selects it from an adjacent empty paragraph, matching normal block decorator behavior). New unit tests in LexicalSlot.test.ts cover both cases; the existing test that a slot-bearing ElementNode host is not merged away still passes.
@etrepum etrepum changed the title [lexical][lexical-selection][lexical-list] Bug Fix: Skip named-slot values in the remaining block converters and diagnose replace() on a slot value [lexical][lexical-selection][lexical-list] Bug Fix: Skip named-slot values in block converters, diagnose replace() on a slot value, and delete slot-bearing decorator hosts at the selection boundary Aug 2, 2026
@potatowagon

Copy link
Copy Markdown
Contributor

Ship-worthy. ✅ Checked out the PR, ran the tests (136/136 pass, incl. the 101-test LexicalSlot suite), and traced the trickiest claim against source. No blocking issues — clean, well-scoped finish to the named-slots audit #8901 started.

✅ What's right (verified)

  1. The deleteCharacter special-case removal is correct. Traced the fall-through: removing the slot-bearing-decorator branch drops it into the generic $isDecoratorNode branch, which does exactly what's described — empty adjacent paragraph → node-select the host; otherwise → caret.origin.remove() as a unit. Slots ride along because they hang off the host node itself.
  2. A detached node is NOT conflated with a slot value. The eligibility gates test $getSlotHost() !== null (the __slotHost up-link), so a truly parentless/detached node returns null and keeps failing loudly on the generic invariant. That's the subtle correctness point and it holds.
  3. The ElementNode-host merge guard is intentionally left in place. The asymmetry is deliberate and well-reasoned: a cross-block merge keeps children but silently drops slots, so backspace-at-start still refuses; the decorator case removes the whole unit. Both paths have tests.
  4. formatList.ts is disciplined. All three $insertList branches (empty-block, empty-list-item, walk-up) plus $removeList gate on $getSlotHost(...) === null with a comment at each site.
  5. Tests genuinely test the claim. The replace() test asserts "nothing was mutated by the failed replace" and matches the diagnostic; the two boundary-deletion tests cover both the remove-as-unit and node-select-from-empty-paragraph paths.

🟡 Minor (non-blocking)

  1. markdown was only audited, not tested. The description says @lexical/markdown is already safe because the block runners bail unless the parent is a root/shadow root. That's a real invariant, but there's no regression test pinning it — if someone later loosens that parent check, nothing fails. A one-line "markdown shortcut is a no-op in a slot value" test would lock it in.

🔵 Nits (cosmetic)

  1. The $runInEditor → runInEditor rename (8/8 in LexicalIsAtEdgeOfElement.test.ts) is unrelated lint-conformance churn bundled into a bug-fix PR. Harmless (grep confirms no leftover refs), just diff noise.

🔎 Uncaught cases considered — all OK

  • Shadow-root slot value (vs bare-block): $wrapNodes correctly still wraps its own subtree via the !$isRootOrShadowRoot(slotFrame) check. ✓
  • Selection crossing a slot boundary: a selection never crosses a slot boundary, so checking the anchor covers the whole selection. ✓
  • Isolated decorator: still short-circuits before the removed branch, unaffected. ✓
  • replace() with a text node onto root: errorOnInsertTextNodeOnRoot still runs first, before the new slot guard. ✓

@etrepum
etrepum added this pull request to the merge queue Aug 3, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Aug 3, 2026
@etrepum
etrepum added this pull request to the merge queue Aug 3, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Aug 3, 2026
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.

Bug: Slot host element is not deleted at the selection boundary

3 participants