fix(lexical): reconcile DOM mutations targeting the root element#8613
Conversation
facebook#8588 removed the `dom === rootElement` -> RootNode carveout from both `$getNearestManagedNodePairFromDOMNode` (mutation flush) and `$getNodeFromDOM`, replacing it with a `__lexicalKey_*` stash set on the root element in `resetEditor`. When a DOM mutation targets the root element directly — which the browser does when typing into a fresh / near-empty contenteditable, before a managed child has received the input — the stash is not reliably observable on the mutation target, so `$getNearestManagedNodePairFromDOMNode` returns undefined, the mutation is silently skipped, and the typed text never reconciles into the editor state (e.g. a word-count gate stays at 0 with no JS exception). Restore the root-element fallback in both functions as a belt-and- suspenders guard. The `__lexicalKey_*` stash from facebook#8588 is preserved, so the captureSelection / root-key-stash behavior is unchanged; the fallback only fires when the stash lookup misses on a root-targeted node. Adds a regression test that drives a root-targeted childList mutation with the root key stash cleared and asserts the foreign DOM is reverted (i.e. the mutation resolves to the RootNode). The test fails without this change and passes with it.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Author of #8588 here, did the sanity check you asked for. I don't doubt you hit this in your app, but I can't reproduce it here — not the mechanism, and not even the symptom (first input dropped, word count stuck at 0). On a normally-mounted editor the root carries the Could you share a repro where the stash is genuinely absent through the real typing/mount flow (or the app scenario you hit)? If it truly can't be observed there, knowing why is what decides between making the lookup robust at the source vs re-adding the carveout. This carveout re-introduces the per-site root special-case that #8588 removed to unify on the single key lookup, so I'd want to confirm the stash is actually unobservable before reverting that. |
|
Status + a verification ask. Quick recap of where this stands, since I want to be transparent about what's confirmed vs. still open: Confirmed (culprit + mechanism):
Confirmed downstream: the internal e2e that triggered the auto-revert reproduces on a clean OnDemand against the synced (post-#8588) Lexical — typing into a fresh editor doesn't register, leaving a word-count-gated control disabled. Exactly the signature. Where I'm blocked: I haven't been able to verify the fix-forward against that internal e2e locally. The synced Lexical in www is a generated bundle, and hand-patching it doesn't take effect — the dev server serves from a different source root than my checkout, so the change has to flow through the actual Lexical→www sync to be exercised by the e2e. We're keeping the sync reverted, so I can't easily re-run the failing test with the fix applied without re-syncing. Ask: does the JSDOM-level repro here look like it captures the right invariant to you, given you authored the stash? If you're confident in the root-mapping restoration, I'd propose we land this and let the next sync attempt serve as the e2e confirmation. If you'd rather make the stash lookup robust at the source instead of re-adding the carveout, happy to revise. Either way I don't want to merge a "fix" that hasn't been run against the actual failing test without flagging that clearly. cc @mayrang |
|
Agreed on the mechanism — #8588 swapping the carveout for the root stash isn't in question. Where I'm stuck is the link between your two pieces of evidence. The test gets to the failing state by clearing the stash up front — exactly the window we're after. What I can't place is a normal typing/mount path that produces that same window: the stash is stamped on the root the moment it's attached and only cleared on a root swap, for the old root. So the test pins down "stash absent → mutation skipped," and the open part is whether the real flow ever reaches stash-absent. The e2e shows the symptom but not the stash state at the mutation, which is the piece that connects the two. Since the sync is reverted, the cleanest read doesn't need a fix-forward at all: on the failing OnDemand, log whether the root carries Present → root-mapping isn't the gap and the cause is elsewhere. Absent → we've found the real window the stash goes missing, and that's what decides re-adding the carveout vs making the lookup robust at the source. |
|
One refinement on what to log: the same root-key lookup also feeds |
|
Also flagging in case I've framed this wrong — I've been pushing on the stash-absent repro, but I could just as easily be misreading the failure path, or missing something about how #8588 reshaped the lookup. If that's the case, point it out and I'll rework it rather than keep digging on a wrong premise. |
etrepum
left a comment
There was a problem hiding this comment.
What would be great to see is a reproduction for how the __lexicalKey_${editor._key} prop is reset on a DOM node while preserving the identity of that DOM node. I can only think of two ways:
- Explicitly clearing it from JavaScript, like this unit test, which would be very strange to do intentionally
- Mutating the editor._key to some other value after initial mount, also quite unusual
Porting the internal e2e test more directly would be best. If you want to merge and test this with the sync that's fine, but if it does happen to work I think all it's really doing it covering up a different issue.
|
I guess a third potential root cause would be directly manipulating editor._rootElement instead of going through the setter |
|
Thanks for the approve, and for the steady pushback — it kept me honest. Here's where my testing landed. What I could verify (against committed revisions):
So I have a clean broken↔passing signal — but only across whole committed revisions. What I could NOT cleanly isolate: "broken sync + only this PR's diff." Our internal Lexical is the generated/bundled sync artifact, and the test harness serves it from the built/committed state, not my working-copy edit to that bundle. I confirmed this the hard way: I dropped both the broken bundle and On your three "how does the stash go missing" candidates (explicit clear / Proposal, given the above: since I can't reach a strong enough isolated conclusion locally, we'll merge this, carry it on the next sync attempt, and run the real e2e there. If it greens the test, great (with the caveat that it may be covering a deeper issue, which I'll keep chasing via the |
|
Makes sense — with the harness serving the committed artifact, merge-then-sync is the right call, and the revert path keeps it cheap if it doesn't hold. One thing I'd keep front of mind for that run: the If it greens, probably worth a targeted test driving that early-return directly, so the thread doesn't get lost behind a passing e2e. |
…y-editor typing smoke test Replace LexicalRootMutation.test.tsx (which manufactured the failing state by clearing the root `__lexicalKey_*` stash up front — a state normal typing never reaches) with a black-box test that types into a freshly-mounted empty editor via real `beforeinput` insertText events and asserts the text reconciles. Honest scoping (documented in the test): this guards the basic "type into an empty editor -> text reconciles" contract, but it does NOT reproduce the facebook#8588 regression at the jsdom level — under jsdom the root stash is present and resolution succeeds with or without the rootElement carveout, so it passes on both pre- and post-facebook#8588 code. The regression only surfaces in a real browser, consistent with maintainer feedback that the failure depends on browser-specific mutation/selection timing. The rootElement-carveout fix in LexicalMutations/LexicalUtils is retained; the internal sync e2e is the authoritative verification and will confirm or refute it.
|
Pushed a test change based on your feedback, and it turned up a useful (if slightly anticlimactic) result. What changed:
The finding: that black-box test passes on both pre- and post-#8588 code. I verified by reverting just the carveout and re-running — still green. So the regression does not reproduce at the jsdom level: under jsdom the root So I'm treating the internal sync e2e as the authoritative check, per the earlier plan: merge → carry on the next sync → if it greens the e2e, keep it (while I keep chasing whether Thanks again for the careful review — it kept this honest and stopped me shipping a misleading test. |
|
Update: this wasn't the fix, and you both called it. Reverting the carveout in #8616 (keeping the typing smoke test). The real root cause was #8519, not #8588. Once this merged and rode the sync, the e2e still failed — and the browser console had the actual throw: The culprit is a downstream serialization helper (our const headless = createHeadlessClone(editor); // active editor inside the update = headless
headless.update(() => {
const selection = ...;
html = $generateHtmlFromNodes(editor, selection); // ← passes the ORIGINAL editor
});#8519 made Fix is a one-liner at that call site (pass the active/headless editor). Verified: the failing e2e (both configs) goes green with that fix, and stays green with this PR's carveout reverted. So exactly as you both said — the carveout was masking nothing and fixing nothing here; the regressor was #8519 surfacing in a downstream caller. Thanks for holding the line on the repro instead of letting me ship it as "the fix." Reverted in #8616. |
…p the typing test) (facebook#8616)
Description
#8588 removed the
dom === rootElement→ RootNode carveout from both$getNearestManagedNodePairFromDOMNode(the MutationObserver flush path) and$getNodeFromDOM, replacing it with a__lexicalKey_*stash set on the root element inresetEditor.In practice the stash is not reliably observable when a DOM mutation targets the root element directly — which the browser does when typing into a fresh / near-empty
contenteditable, before a managed child has received the input. In that window:$getNearestManagedNodePairFromDOMNodewalksstrayNode → rootElement, finds no key, and (with the carveout removed) returnsundefinedThis surfaced downstream as: type N words into an empty Lexical editor → editor word count stays
0→ a word-count-gated control never enables. The failure is at the typing/reconcile step, not at any later editing step.Fix
Restore the root-element fallback in both functions as a belt-and-suspenders guard:
$getNearestManagedNodePairFromDOMNode: re-add therootElementparam and theelse if (dom === rootElement) → [rootElement, internalGetRoot(editorState)]branch.$getNodeFromDOM: re-add thedom === editor.getRootElement() → $getNodeByKey('root')fallback when the keyed tree walk misses.The
__lexicalKey_*root stash andcaptureSelectionbehavior from #8588 are preserved untouched — the fallback only fires when the stash lookup misses on a root-targeted node, so there's no regression to the root-key-stash / captureSelection path.Test plan
Adds
LexicalRootMutation.test.tsx:__lexicalKey_rootstash after mount ([Breaking Change][lexical] Feature: Generalize captured selection via setDOMUnmanaged({captureSelection}) + root __lexicalKey_* stash #8588 invariant).childListmutation with the root key stash cleared (simulating the real-world window where the stash isn't observable on the mutation target) and asserts the foreign DOM is reverted — i.e. the mutation resolves to the RootNode.The second test fails on current
main(foreign DOM left in place → mutation skipped) and passes with this change.npx vitest run --project unit packages/lexical/src/__tests__/unit/→ all 568 unit tests pass (incl. the new ones).tsc -p tsconfig.build.json --noEmit→ clean.cc @mayrang (author of #8588 / #8519) — would appreciate a sanity check that the fallback doesn't conflict with the intended root-key-stash design; happy to instead make the stash lookup robust at the source if you'd prefer that over the carveout.