[lexical-rich-text] Bug Fix: Don't deselect a selected node when clicking inside it - #8910
Conversation
|
Hi @alvorithm! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Will review after the CLA is signed |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Context
Fixes #8907. A
NodeSelectionset by an inlineDecoratorNode's own click handler (useLexicalNodeSelection+onClick, or any programmatic$setSelectionin the click event) is immediately cleared by@lexical/rich-text'sCLICK_COMMANDhandler, which deselects on any click. The mechanism is verified in the issue thread, starting from the request to submit a PR: React renders decorator content in a root inside the editor root, so the decorator'sonClickruns before Lexical's click handler; the click then reachesregisterRichText's deselect handler, which clears whatever NodeSelection was just committed. The deselect contract only makes sense for clicks away from the selected node; a click on a selected decorator is an interaction with that node (its own click handler may select it, open an editor, ...), not a deselect gesture.Base:
origin/main(a933222c4). Standalone repro used for the analysis: https://gist.github.com/alvorithm/73199ba7f08c65b9cf1586593a72345cOne finding from investigating e2e coverage (see Verification): the playground never hits this bug because every playground decorator consumes
CLICK_COMMANDat a priority above rich-text's deselect. Command listeners run in descending priority order (COMMAND_PRIORITY_CRITICAL= 4 first,COMMAND_PRIORITY_EDITOR= 0 last;LexicalUpdates.tsdispatchesfor (let i = 4; i >= 0; i--)), and a listener that returnstruestops propagation.ImageComponent,EquationComponent,PollComponent,ExcalidrawComponentandPageBreakNodeall registerCLICK_COMMANDatCOMMAND_PRIORITY_LOWor higher and returntruefor clicks on their own node, so rich-text's deselect handler never runs for those clicks. The deselect only fires when no higher-priority consumer handled the click, which is exactly the #8907 pattern: selection set from a ReactonClickinside decorator content, with noCLICK_COMMANDconsumer registered.Changes
6b7a8bebe[lexical-rich-text] Bug Fix: Don't deselect a selected node when clicking inside it (#8907)registerRichText'sCLICK_COMMANDhandler, when the selection is aNodeSelection, skip the deselect if the click target is anHTMLElementcontained in any selected node's DOM (editor.getElementByKey(...).contains(target)), and returnfalseso lower-priority handlers (e.g. the playground's decorator click handlers atCOMMAND_PRIORITY_LOW) still run. Clicks anywhere else keep the existing deselect behavior.RichTextNodeSelectionClick.test.ts(4 unit tests): click inside a selected inline decorator keeps the selection; same for a block decorator; click outside still deselects; a non-element target still deselects.Verification
npx vitest --project unit --run packages/lexical-rich-text; 13 files, 121 tests passed.RichTextNodeSelectionClick.test.tsfails exactly the two "click inside keeps the selection" cases; with the fix, 4/4 pass.npx tsc --noEmit -p tsconfig.jsonclean for both changed files.npx eslintclean on both changed files.CLICK_COMMANDhandler atCOMMAND_PRIORITY_LOWconsumes the click before rich-text's deselect runs (see Context). Any e2e for this fix would need a new playground decorator component that selects from a ReactonClickand does not consumeCLICK_COMMAND; that component does not exist today. The unit tests cover the same handler contract deterministically.