Skip to content

[lexical][lexical-react] Bug Fix: add ignoreFocusChange option to OnC…#8689

Open
Ranjan00001 wants to merge 3 commits into
facebook:mainfrom
Ranjan00001:fix/on-change-ignore-focus
Open

[lexical][lexical-react] Bug Fix: add ignoreFocusChange option to OnC…#8689
Ranjan00001 wants to merge 3 commits into
facebook:mainfrom
Ranjan00001:fix/on-change-ignore-focus

Conversation

@Ranjan00001

@Ranjan00001 Ranjan00001 commented Jun 13, 2026

Copy link
Copy Markdown
                                                                                                                                                                                                                                     Adds ignoreFocusChange option to OnChangePlugin. When true, onChange is not called for updates tagged with FOCUS_TAG (e.g., when editor.focus() runs via AutoFocusPlugin on mount). This prevents spurious onChange calls that aren't actual content changes.                                                                                                                                                                    

Defaults to false (backwards compatible).

Also exports FOCUS_TAG from lexical so consumers can check for it in the tags set received by onChange.

Closes #4493

Reproducing the issue

Without this fix, using AutoFocusPlugin triggers an unwanted onChange call on mount:

 <LexicalComposer initialConfig={...}>                                                                                                                                                                                                   
   <AutoFocusPlugin />                                                                                                                                                                                                                   
   <OnChangePlugin onChange={(editorState) => {                                                                                                                                                                                          
     // Called on mount due to AutoFocusPlugin's editor.focus(),                                                                                                                                                                         
     // even though no content changed                                                                                                                                                                                                   
   }} />                                                                                                                                                                                                                                 
 </LexicalComposer>                                                                                                                                                                                                                      

With ignoreFocusChange={true}, the focus-triggered call is suppressed:

What changed

  • lexical/src/index.ts — export FOCUS_TAG
  • @lexical/react/LexicalOnChangePlugin — add ignoreFocusChange prop (default false)
  • Unit tests — 7 tests total covering tag-based suppression, editor.focus() trigger path, and tags passthrough

…hangePlugin (facebook#4493)

When AutoFocusPlugin, CodeHighlightPlugin, and OnChangePlugin are combined,
a spurious onChange fires on load because editor.focus() triggers node
transforms that mark elements dirty. Export FOCUS_TAG from lexical and add
an opt-in ignoreFocusChange prop so users can suppress focus-triggered updates.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@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 Jun 13, 2026
@vercel

vercel Bot commented Jun 13, 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, Comment Jun 18, 2026 2:41pm
lexical-playground Ready Ready Preview, Comment Jun 18, 2026 2:41pm

Request Review

@potatowagon potatowagon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by Navi (Tater Thoughts Bobblehead) on behalf of @potatowagon.

Assessment: LGTM (minor suggestion)

This is a clean, well-scoped bug fix that follows the existing established pattern in OnChangePlugin.

What I checked:

  • FOCUS_TAG already exists in LexicalUpdateTags.ts and is used internally (e.g. in LexicalEditor.ts, LexicalEvents.ts) — this PR just promotes it to a public export
  • ✅ The ignoreFocusChange prop mirrors the exact same pattern as ignoreHistoryMergeTagChange: opt-in boolean defaulting to a safe value (false), tags.has(TAG) guard in the same conditional chain
  • ✅ Default false means no breaking change — existing behavior preserved
  • useEffect dependency array correctly updated with the new prop
  • ✅ No www compat concerns — additive export only, no removed/renamed APIs
  • ✅ CLA signed, Vercel previews green

Why it's safe: The change is minimal (2 files, ~12 lines net), follows a proven pattern, and is opt-in. The edge case it fixes (spurious onChange on focus due to AutoFocusPlugin + CodeHighlightPlugin interaction marking elements dirty) is a real user pain point.

Suggestion: Consider adding a unit test that verifies onChange is NOT called when ignoreFocusChange={true} and the update is tagged with FOCUS_TAG. The existing test suite for OnChangePlugin likely has a pattern you can follow. Not blocking — the code is correct as-is.

CI note: Full test matrix (unit/browser/e2e) has not yet run — only CLA and Vercel are green so far. Recommend waiting for full CI before merging.

@mayrang

mayrang commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Tried reproducing this locally. Sharing what I saw in case it's useful.

The bug only seemed to surface when editor.focus() lands in its own commit. When setRootElement and focus collapse into the same commit, that commit ends up with history-merge and the default OnChangePlugin skips it. Inserting await new Promise(r => setTimeout(r, 0)) between setRootElement and focus splits them apart, and the focus commit then arrives alone with just the focus tag, which the default OnChangePlugin doesn't skip. That seemed to match AutoFocusPlugin's useEffect timing in a real React mount.

For a regression test, mirroring OnChangePlugin's listener with and without the tags.has(FOCUS_TAG) skip on a setup that uses the macrotask gap might be enough — the default variant fires once with ["focus"], the ignoreFocusChange variant stays silent.

CodeNode / CodeHighlight didn't seem to be required in my setup — plain paragraph reproduced it too.

@Ranjan00001

Copy link
Copy Markdown
Author

@mayrang @potatowagon pleas check now. I hav added unit tests.

@mayrang

mayrang commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

The tests pass on --project unit, but every case calls $addUpdateTag(FOCUS_TAG) by hand inside editor.update. That only checks the option matching the tag — not the actual #4493 trigger, which is editor.focus() itself emitting FOCUS_TAG and causing a spurious onChange when AutoFocusPlugin runs focus on mount. None of the cases call editor.focus(), so that path isn't exercised.

A test that does — contenteditable rootElement set, one initial update, a microtask awaited, then editor.focus() in act — would cover it: true keeps onChange silent, false fires it once.

Also — could you flesh out the PR description a bit? It's still the template placeholders (Describe the changes in this pull request, Closes #<!-- issue number -->, Before/After). Even just a short description, Closes #4493, and a quick repro would make it much easier to review.

@mayrang

mayrang commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

The description needs a rewrite. Every line has leading whitespace that GitHub renders as a code block, and it doesn't follow the repo's PR template (## Description / ## Test plan / Before & After). The "What changed" section is a file list — other merged PRs in this repo explain the bug mechanism instead. For this PR, that's: editor.focus() internally calls $addUpdateTag(FOCUS_TAG), committing an update with no dirty nodes that OnChangePlugin doesn't skip, so onChange fires spuriously.

Also worth noting: FOCUS_TAG was internal-only before this PR. Now that it's exported, consumers can also filter directly in their onChange callback via tags.has(FOCUS_TAG) — the convenience prop mirrors ignoreHistoryMergeTagChange and is consistent, but the export is independently useful.

Left two inline comments on the code.

COMPOSITION_END_TAG,
COMPOSITION_START_TAG,
CUT_TAG,
FOCUS_TAG,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time FOCUS_TAG is exported as public API. It needs a matching declare export var FOCUS_TAG: 'focus'; and | typeof FOCUS_TAG in the UpdateTag union in packages/lexical/flow/Lexical.js.flow, plus ignoreFocusChange?: boolean in packages/lexical-react/flow/LexicalOnChangePlugin.js.flow. Without these, lint-flow CI will fail.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint-flow is currently more of a diagnostic tool not a CI gate, we don't have something like that enabled because there is not full flow coverage. Should still happen regardless.

});

afterEach(() => {
document.body.removeChild(container!);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afterEach removes the container but never calls reactRoot.unmount(). The registerUpdateListener cleanup won't run, and React 18 logs warnings about unmounted roots. Adding await act(async () => { reactRoot.unmount(); }); before the removeChild call fixes it.

@etrepum etrepum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR claims to resolve #4493 but it doesn't set up a test with AutofocusPlugin to show that it actually does

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: When AutoFocusPlugin is Combined with CodeHighlightPlugin and LexicalOnChangePlugin - an OnChange Event is raised on document load.

4 participants