[lexical-rich-text][lexical-playground][lexical-react]: Update selection when something is selected inside a DecoratorNode #7072

GermanJablo · 2025-01-20T21:07:25Z

Closes:

Bug: Pasting in block results in pasting the content twice in Firefox #7071
Bug: Selection is not updated when something is selected inside a DecoratorNode #6632

This PR:

Update selection when something is selected inside a DecoratorNode.
The logic is centralized in RichTextPlugin so that it does not have to be repeated in all nodes. This had caused some nodes to forget to implement this logic, or to be inconsistent:
- Sometimes it seems that the intention was to deselect a decorator node if it was already selected with a second click.
- Other times it was intended to deselect if shift + click was pressed.
- Other times clicking on a selected decorator node leaves the selection unchanged. This is the default behavior chosen in this PR, but if anyone prefers either of the above two options please say so.
I took the opportunity to improve a couple of details in Excalidraw. The option to open with double click was removed for reasons of simplicity and consistency, considering that the component already has a button at the top right to open the editor.

By having the behavior of the click command centralized, fixing other bugs later should be easier, such as:

vercel · 2025-01-20T21:07:31Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 20, 2025 9:35pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 20, 2025 9:35pm

github-actions · 2025-01-20T21:09:56Z

size-limit report 📦

Path	Size
lexical - cjs	29.07 KB (0%)
lexical - esm	28.89 KB (0%)
@lexical/rich-text - cjs	38.08 KB (+0.11% 🔺)
@lexical/rich-text - esm	30.98 KB (-0.01% 🔽)
@lexical/plain-text - cjs	36.57 KB (0%)
@lexical/plain-text - esm	28.26 KB (0%)
@lexical/react - cjs	39.84 KB (0%)
@lexical/react - esm	32.34 KB (0%)

GermanJablo · 2025-01-20T21:10:45Z

packages/lexical-playground/src/nodes/ExcalidrawNode/ExcalidrawComponent.tsx

@@ -234,9 +200,8 @@ export default function ExcalidrawComponent({
            height={height}
          />
          {isSelected && isEditable && (
-            <div
+            <button
              className="image-edit-button"


Even considering that double click was allowed, it's odd that the container was a button, and the button a div.
It was confusing to understand the click command I was deleting until I realized this (I thought buttonRef was the small button)

GermanJablo · 2025-01-20T21:15:03Z

packages/lexical-playground/src/nodes/ImageComponent.tsx

@@ -300,11 +299,6 @@ export default function ImageComponent({
        },
        COMMAND_PRIORITY_LOW,
      ),
-      editor.registerCommand<MouseEvent>(
-        CLICK_COMMAND,
-        onClick,


I'm not removing the onClick handler because it's used for the right button. See #5056

GermanJablo · 2025-01-20T21:17:06Z

packages/lexical/src/LexicalEditor.ts

@@ -786,7 +786,7 @@ export class LexicalEditor {
   * deterministically in the order of registration.
   *
   * @param command - the command that will trigger the callback.
-   * @param listener - the function that will execute when the command is dispatched.
+   * @param listener - the function that will execute when the command is dispatched. Returns true to stop propagation, false to continue.


Forgive me for this change that is irrelevant to the objective of the PR. But I always have to think about what true and false mean 😅

This is a common source of confusion 👍

etrepum

It looks like there are failing tests

etrepum · 2025-01-21T01:08:30Z

packages/lexical-rich-text/src/index.ts

+          if ($isDecoratorNode(node)) {
+            const selection = $createNodeSelection();
+            selection.add(node.getKey());
+            $setSelection(selection);
+          }


This seems like a breaking change because now it works differently and only one node can be selected, where before you could select multiple nodes. This is also a bit tricky to override without having its own command.

This seems like a breaking change because now it works differently and only one node can be selected

For me this is more of a bug fix than a breaking change. Currently the selection was being set to null which is definitely worse and doesn't make much sense. Probably many people haven't noticed this because the most used nodes like ImageNode fix the behavior with their commands.

where before you could select multiple nodes

From the frontend (let's say the playground), I know how to select multiple nodes with a rangeSelection, but I don't know of any way I could select multiple nodes with a NodeSelection, neither before nor after this PR. I think that it's a good thing, since I don't know in what scenario someone would want something like that.

This is also a bit tricky to override without having its own command.

Same as above. I don't see this PR changing anything in that regard. Customizing the behavior used to require a command, now it does too.

I get that this behavior makes more sense but it’s probably not backwards compatible, anyone upgrading to this PR is going to have to audit their code to be compliant. Without a backwards compatible upgrade path it might be tricky or at least take longer to get meta to accept it

To select multiple nodes with a NodeSelection you click one and then shift-click the others. They don't even have to be adjacent. Try creating three divider nodes, click the first one, and then shift-click the third one. I don't think this makes a lot of sense to do for this particular kind of node, but it's how the editor works now and changing that would break compatibility.

The tricky thing about breaking compatibility is we don't know what we don't know. The unit and e2e test don't even come close to covering what people are doing in their own projects. Based on the scope of this affecting events for all decorators I think this would require at least some Meta folks looking closely at how all of their projects are using events and decorators. If we're going to just patch the problem I suspect there's a way we can do it that just brings Firefox behavior in alignment with the other platforms and doesn't change anything fundamental.

yes, it makes sense! 👌👍

I'm making a PR in our codebase to fix this on our end.
Feel free to close this PR if you feel the breaking change isn't worth it!
If, on the other hand, it is a change that you do want to incorporate, let me know and I'll take a look at the failing tests.

GermanJablo added 2 commits January 20, 2025 16:57

fix selection in decoratorNodes

0a2bd75

improve excalidraw

0f1690c

GermanJablo requested review from zurfyx, fantactuka, acywatson, Fetz, ivailop7, Sahejkm and potatowagon as code owners January 20, 2025 21:07

facebook-github-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 Jan 20, 2025

vercel bot deployed to Preview – lexical-playground January 20, 2025 21:08 View deployment

vercel bot deployed to Preview – lexical January 20, 2025 21:08 View deployment

GermanJablo commented Jan 20, 2025

View reviewed changes

remove unused refs

8ab088d

vercel bot deployed to Preview – lexical-playground January 20, 2025 21:35 View deployment

vercel bot deployed to Preview – lexical January 20, 2025 21:35 View deployment

GermanJablo mentioned this pull request Jan 20, 2025

Pasting in block results in pasting the content twice in Firefox payloadcms/payload#10634

Closed

etrepum added the extended-tests Run extended e2e tests on a PR label Jan 21, 2025

etrepum reviewed Jan 21, 2025

View reviewed changes

etrepum mentioned this pull request Jan 21, 2025

Bug: Pasting in block results in pasting the content twice in Firefox #7071

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lexical-rich-text][lexical-playground][lexical-react]: Update selection when something is selected inside a DecoratorNode #7072

[lexical-rich-text][lexical-playground][lexical-react]: Update selection when something is selected inside a DecoratorNode #7072

GermanJablo commented Jan 20, 2025

vercel bot commented Jan 20, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

GermanJablo Jan 20, 2025

GermanJablo Jan 20, 2025

GermanJablo Jan 20, 2025

etrepum Jan 21, 2025

etrepum left a comment

etrepum Jan 21, 2025

GermanJablo Jan 21, 2025

etrepum Jan 21, 2025

etrepum Jan 21, 2025

GermanJablo Jan 21, 2025

GermanJablo Jan 22, 2025

[lexical-rich-text][lexical-playground][lexical-react]: Update selection when something is selected inside a DecoratorNode #7072

Are you sure you want to change the base?

[lexical-rich-text][lexical-playground][lexical-react]: Update selection when something is selected inside a DecoratorNode #7072

Conversation

GermanJablo commented Jan 20, 2025

vercel bot commented Jan 20, 2025 • edited Loading

github-actions bot commented Jan 20, 2025 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etrepum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jan 20, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading