Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+25 −149
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a2bd75
fix selection in decoratorNodes
GermanJablo Jan 20, 2025
0f1690c
improve excalidraw
GermanJablo Jan 20, 2025
8ab088d
remove unused refs
GermanJablo Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 3 additions & 38 deletions packages/lexical-playground/src/nodes/ExcalidrawNode/ExcalidrawComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,7 @@ import {useLexicalNodeSelection} from '@lexical/react/useLexicalNodeSelection';
import {mergeRegister} from '@lexical/utils';
import {
$getNodeByKey,
CLICK_COMMAND,
COMMAND_PRIORITY_LOW,
isDOMNode,
KEY_BACKSPACE_COMMAND,
KEY_DELETE_COMMAND,
} from 'lexical';
Expand Down Expand Up @@ -47,7 +45,6 @@ export default function ExcalidrawComponent({
data === '[]' && editor.isEditable(),
);
const imageContainerRef = useRef<HTMLDivElement | null>(null);
const buttonRef = useRef<HTMLButtonElement | null>(null);
const captionButtonRef = useRef<HTMLButtonElement | null>(null);
const [isSelected, setSelected, clearSelection] =
useLexicalNodeSelection(nodeKey);
Expand Down Expand Up @@ -75,35 +72,6 @@ export default function ExcalidrawComponent({
return;
}
return mergeRegister(
editor.registerCommand(
CLICK_COMMAND,
(event: MouseEvent) => {
const buttonElem = buttonRef.current;
const eventTarget = event.target;

if (isResizing) {
return true;
}

if (
buttonElem !== null &&
isDOMNode(eventTarget) &&
buttonElem.contains(eventTarget)
) {
if (!event.shiftKey) {
clearSelection();
}
setSelected(!isSelected);
if (event.detail > 1) {
setModalOpen(true);
}
return true;
}

return false;
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand(
KEY_DELETE_COMMAND,
$onDelete,
Expand Down Expand Up @@ -221,9 +189,7 @@ export default function ExcalidrawComponent({
/>
)}
{elements.length > 0 && (
<button
ref={buttonRef}
className={`excalidraw-button ${isSelected ? 'selected' : ''}`}>
<div className={`excalidraw-button ${isSelected ? 'selected' : ''}`}>
<ExcalidrawImage
imageContainerRef={imageContainerRef}
className="image"
Expand All @@ -234,9 +200,8 @@ export default function ExcalidrawComponent({
height={height}
/>
{isSelected && isEditable && (
<div
<button
className="image-edit-button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

role="button"
tabIndex={0}
onMouseDown={(event) => event.preventDefault()}
onClick={openModal}
Expand All @@ -254,7 +219,7 @@ export default function ExcalidrawComponent({
captionsEnabled={true}
/>
)}
</button>
</div>
)}
</>
);
Expand Down
6 changes: 0 additions & 6 deletions packages/lexical-playground/src/nodes/ImageComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import {
$isNodeSelection,
$isRangeSelection,
$setSelection,
CLICK_COMMAND,
COMMAND_PRIORITY_LOW,
createCommand,
DRAGSTART_COMMAND,
Expand Down Expand Up @@ -300,11 +299,6 @@ export default function ImageComponent({
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand<MouseEvent>(
CLICK_COMMAND,
onClick,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

COMMAND_PRIORITY_LOW,
),
editor.registerCommand<MouseEvent>(
RIGHT_CLICK_IMAGE_COMMAND,
onClick,
Expand Down
19 changes: 0 additions & 19 deletions packages/lexical-playground/src/nodes/InlineImageNode/InlineImageComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
$getSelection,
$isNodeSelection,
$setSelection,
CLICK_COMMAND,
COMMAND_PRIORITY_LOW,
DRAGSTART_COMMAND,
KEY_BACKSPACE_COMMAND,
Expand Down Expand Up @@ -288,24 +287,6 @@ export default function InlineImageComponent({
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand<MouseEvent>(
CLICK_COMMAND,
(payload) => {
const event = payload;
if (event.target === imageRef.current) {
if (event.shiftKey) {
setSelected(!isSelected);
} else {
clearSelection();
setSelected(true);
}
return true;
}

return false;
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand(
DRAGSTART_COMMAND,
(event) => {
Expand Down
18 changes: 0 additions & 18 deletions packages/lexical-playground/src/nodes/PageBreakNode/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {mergeRegister} from '@lexical/utils';
import {
$getSelection,
$isNodeSelection,
CLICK_COMMAND,
COMMAND_PRIORITY_HIGH,
COMMAND_PRIORITY_LOW,
DecoratorNode,
Expand Down Expand Up @@ -53,23 +52,6 @@ function PageBreakComponent({nodeKey}: {nodeKey: NodeKey}) {

useEffect(() => {
return mergeRegister(
editor.registerCommand(
CLICK_COMMAND,
(event: MouseEvent) => {
const pbElem = editor.getElementByKey(nodeKey);

if (event.target === pbElem) {
if (!event.shiftKey) {
clearSelection();
}
setSelected(!isSelected);
return true;
}

return false;
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand(
KEY_DELETE_COMMAND,
$onDelete,
Expand Down
23 changes: 1 addition & 22 deletions packages/lexical-playground/src/nodes/PollComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
$getSelection,
$isNodeSelection,
BaseSelection,
CLICK_COMMAND,
COMMAND_PRIORITY_LOW,
KEY_BACKSPACE_COMMAND,
KEY_DELETE_COMMAND,
Expand Down Expand Up @@ -142,7 +141,6 @@ export default function PollComponent({
const [isSelected, setSelected, clearSelection] =
useLexicalNodeSelection(nodeKey);
const [selection, setSelection] = useState<BaseSelection | null>(null);
const ref = useRef(null);

const $onDelete = useCallback(
(payload: KeyboardEvent) => {
Expand All @@ -166,23 +164,6 @@ export default function PollComponent({
editor.registerUpdateListener(({editorState}) => {
setSelection(editorState.read(() => $getSelection()));
}),
editor.registerCommand<MouseEvent>(
CLICK_COMMAND,
(payload) => {
const event = payload;

if (event.target === ref.current) {
if (!event.shiftKey) {
clearSelection();
}
setSelected(!isSelected);
return true;
}

return false;
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand(
KEY_DELETE_COMMAND,
$onDelete,
Expand Down Expand Up @@ -220,9 +201,7 @@ export default function PollComponent({
const isFocused = $isNodeSelection(selection) && isSelected;

return (
<div
className={`PollNode__container ${isFocused ? 'focused' : ''}`}
ref={ref}>
<div className={`PollNode__container ${isFocused ? 'focused' : ''}`}>
<div className="PollNode__inner">
<h2 className="PollNode__heading">{question}</h2>
{options.map((option, index) => {
Expand Down
22 changes: 1 addition & 21 deletions packages/lexical-react/src/LexicalBlockWithAlignableContents.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,13 @@ import {
$isDecoratorNode,
$isNodeSelection,
$isRangeSelection,
CLICK_COMMAND,
COMMAND_PRIORITY_LOW,
FORMAT_ELEMENT_COMMAND,
KEY_BACKSPACE_COMMAND,
KEY_DELETE_COMMAND,
} from 'lexical';
import * as React from 'react';
import {ReactNode, useCallback, useEffect, useRef} from 'react';
import {ReactNode, useCallback, useEffect} from 'react';

type Props = Readonly<{
children: ReactNode;
Expand All @@ -50,7 +49,6 @@ export function BlockWithAlignableContents({

const [isSelected, setSelected, clearSelection] =
useLexicalNodeSelection(nodeKey);
const ref = useRef(null);

const $onDelete = useCallback(
(event: KeyboardEvent) => {
Expand Down Expand Up @@ -102,23 +100,6 @@ export function BlockWithAlignableContents({
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand<MouseEvent>(
CLICK_COMMAND,
(event) => {
if (event.target === ref.current) {
event.preventDefault();
if (!event.shiftKey) {
clearSelection();
}

setSelected(!isSelected);
return true;
}

return false;
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand(
KEY_DELETE_COMMAND,
$onDelete,
Expand All @@ -137,7 +118,6 @@ export function BlockWithAlignableContents({
className={[className.base, isSelected ? className.focus : null]
.filter(Boolean)
.join(' ')}
ref={ref}
style={{
textAlign: format ? format : undefined,
}}>
Expand Down
18 changes: 0 additions & 18 deletions packages/lexical-react/src/LexicalHorizontalRuleNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
$applyNodeReplacement,
$getSelection,
$isNodeSelection,
CLICK_COMMAND,
COMMAND_PRIORITY_LOW,
createCommand,
DecoratorNode,
Expand Down Expand Up @@ -66,23 +65,6 @@ function HorizontalRuleComponent({nodeKey}: {nodeKey: NodeKey}) {

useEffect(() => {
return mergeRegister(
editor.registerCommand(
CLICK_COMMAND,
(event: MouseEvent) => {
const hrElem = editor.getElementByKey(nodeKey);

if (event.target === hrElem) {
if (!event.shiftKey) {
clearSelection();
}
setSelected(!isSelected);
return true;
}

return false;
},
COMMAND_PRIORITY_LOW,
),
editor.registerCommand(
KEY_DELETE_COMMAND,
$onDelete,
Expand Down
25 changes: 19 additions & 6 deletions packages/lexical-rich-text/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
} from '@lexical/utils';
import {
$applyNodeReplacement,
$createNodeSelection,
$createParagraphNode,
$createRangeSelection,
$createTabNode,
Expand Down Expand Up @@ -565,13 +566,25 @@ export function registerRichText(editor: LexicalEditor): () => void {
const removeListener = mergeRegister(
editor.registerCommand(
CLICK_COMMAND,
(payload) => {
const selection = $getSelection();
if ($isNodeSelection(selection)) {
selection.clear();
return true;
(event) => {
if (!(event.target instanceof Element)) {
return false;
}
return false;
const decorator = event.target.closest(
'[data-lexical-decorator="true"]',
);
if (!decorator) {
return false;
}
editor.update(() => {
const node = $getNearestNodeFromDOMNode(decorator);
if ($isDecoratorNode(node)) {
const selection = $createNodeSelection();
selection.add(node.getKey());
$setSelection(selection);
}
Comment on lines +581 to +585
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it makes sense! 👌👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

});
return true;
},
0,
),
Expand Down
2 changes: 1 addition & 1 deletion packages/lexical/src/LexicalEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a common source of confusion 👍

* @param priority - the relative priority of the listener. 0 | 1 | 2 | 3 | 4
* (or {@link COMMAND_PRIORITY_EDITOR} |
* {@link COMMAND_PRIORITY_LOW} |
Expand Down
Loading