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.

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-history][lexical-selection][lexical-react] Fix: #6409 TextNode change detection #26

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions packages/lexical-clipboard/src/clipboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ function $appendNodesToJSON(
let target = currentNode;

if (selection !== null) {
let clone = $cloneWithProperties<LexicalNode>(currentNode);
let clone = $cloneWithProperties(currentNode);
clone =
$isTextNode(clone) && selection !== null
? $sliceSelectedTextNodeContent(selection, clone)
Expand All @@ -267,11 +267,11 @@ function $appendNodesToJSON(

const serializedNode = exportNodeToJSON(target);

// TODO: TextNode calls getTextContent() (NOT node.__text) within it's exportJSON method
// TODO: TextNode calls getTextContent() (NOT node.__text) within its exportJSON method
// which uses getLatest() to get the text from the original node with the same key.
// This is a deeper issue with the word "clone" here, it's still a reference to the
// same node as far as the LexicalEditor is concerned since it shares a key.
// We need a way to create a clone of a Node in memory with it's own key, but
// We need a way to create a clone of a Node in memory with its own key, but
// until then this hack will work for the selected text extract use case.
if ($isTextNode(target)) {
const text = target.__text;
Expand Down
142 changes: 133 additions & 9 deletions packages/lexical-history/src/__tests__/unit/LexicalHistory.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import {HistoryPlugin} from '@lexical/react/LexicalHistoryPlugin';
import {RichTextPlugin} from '@lexical/react/LexicalRichTextPlugin';
import {$createQuoteNode} from '@lexical/rich-text';
import {$setBlocksType} from '@lexical/selection';
import {$restoreEditorState} from '@lexical/utils';
import {
$applyNodeReplacement,
$createNodeSelection,
$createParagraphNode,
$createRangeSelection,
Expand All @@ -26,17 +28,81 @@ import {
CAN_UNDO_COMMAND,
CLEAR_HISTORY_COMMAND,
COMMAND_PRIORITY_CRITICAL,
type KlassConstructor,
LexicalEditor,
LexicalNode,
type NodeKey,
REDO_COMMAND,
SerializedElementNode,
SerializedTextNode,
type SerializedTextNode,
type Spread,
TextNode,
UNDO_COMMAND,
} from 'lexical/src';
} from 'lexical';
import {createTestEditor, TestComposer} from 'lexical/src/__tests__/utils';
import React from 'react';
import {createRoot, Root} from 'react-dom/client';
import * as ReactTestUtils from 'shared/react-test-utils';

type SerializedCustomTextNode = Spread<
{type: ReturnType<typeof CustomTextNode.getType>; classes: string[]},
SerializedTextNode
>;

class CustomTextNode extends TextNode {
['constructor']!: KlassConstructor<typeof CustomTextNode>;

__classes: Set<string>;
constructor(text: string, classes: Iterable<string>, key?: NodeKey) {
super(text, key);
this.__classes = new Set(classes);
}
static getType(): 'custom-text' {
return 'custom-text';
}
static clone(node: CustomTextNode): CustomTextNode {
return new CustomTextNode(node.__text, node.__classes, node.__key);
}
addClass(className: string): this {
const self = this.getWritable();
self.__classes.add(className);
return self;
}
removeClass(className: string): this {
const self = this.getWritable();
self.__classes.delete(className);
return self;
}
setClasses(classes: Iterable<string>): this {
const self = this.getWritable();
self.__classes = new Set(classes);
return self;
}
getClasses(): ReadonlySet<string> {
return this.getLatest().__classes;
}
static importJSON({text, classes}: SerializedCustomTextNode): CustomTextNode {
return $createCustomTextNode(text, classes);
}
exportJSON(): SerializedCustomTextNode {
return {
...super.exportJSON(),
classes: Array.from(this.getClasses()),
type: this.constructor.getType(),
};
}
}
function $createCustomTextNode(
text: string,
classes: string[] = [],
): CustomTextNode {
return $applyNodeReplacement(new CustomTextNode(text, classes));
}
function $isCustomTextNode(
node: LexicalNode | null | undefined,
): node is CustomTextNode {
return node instanceof CustomTextNode;
}

describe('LexicalHistory tests', () => {
let container: HTMLDivElement | null = null;
let reactRoot: Root;
Expand All @@ -59,13 +125,12 @@ describe('LexicalHistory tests', () => {
// Shared instance across tests
let editor: LexicalEditor;

function TestPlugin() {
// Plugin used just to get our hands on the Editor object
[editor] = useLexicalComposerContext();
return null;
}
function Test(): JSX.Element {
function TestPlugin() {
// Plugin used just to get our hands on the Editor object
[editor] = useLexicalComposerContext();
return null;
}

return (
<TestComposer>
<RichTextPlugin
Expand Down Expand Up @@ -313,6 +378,65 @@ describe('LexicalHistory tests', () => {
await editor_.dispatchCommand(UNDO_COMMAND, undefined);
expect($isNodeSelection(editor_.getEditorState()._selection)).toBe(true);
});

test('Changes to TextNode leaf are detected properly #6409', async () => {
editor = createTestEditor({
nodes: [CustomTextNode],
});
const sharedHistory = createEmptyHistoryState();
registerHistory(editor, sharedHistory, 0);
editor.update(
() => {
$getRoot()
.clear()
.append(
$createParagraphNode().append(
$createCustomTextNode('Initial text'),
),
);
},
{discrete: true},
);
expect(sharedHistory.undoStack).toHaveLength(0);

editor.update(
() => {
// Mark dirty with no changes
for (const node of $getRoot().getAllTextNodes()) {
node.getWritable();
}
// Restore the editor state and ensure the history did not change
$restoreEditorState(editor, editor.getEditorState());
},
{discrete: true},
);
expect(sharedHistory.undoStack).toHaveLength(0);
editor.update(
() => {
// Mark dirty with text change
for (const node of $getRoot().getAllTextNodes()) {
if ($isCustomTextNode(node)) {
node.setTextContent(node.getTextContent() + '!');
}
}
},
{discrete: true},
);
expect(sharedHistory.undoStack).toHaveLength(1);

editor.update(
() => {
// Mark dirty with only a change to the class
for (const node of $getRoot().getAllTextNodes()) {
if ($isCustomTextNode(node)) {
node.addClass('updated');
}
}
},
{discrete: true},
);
expect(sharedHistory.undoStack).toHaveLength(2);
});
});

const $createParagraphNodeWithText = (text: string) => {
Expand Down
35 changes: 18 additions & 17 deletions packages/lexical-history/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,25 +193,26 @@ function isTextNodeUnchanged(

const prevSelection = prevEditorState._selection;
const nextSelection = nextEditorState._selection;
let isDeletingLine = false;

if ($isRangeSelection(prevSelection) && $isRangeSelection(nextSelection)) {
isDeletingLine =
prevSelection.anchor.type === 'element' &&
prevSelection.focus.type === 'element' &&
nextSelection.anchor.type === 'text' &&
nextSelection.focus.type === 'text';
}
const isDeletingLine =
$isRangeSelection(prevSelection) &&
$isRangeSelection(nextSelection) &&
prevSelection.anchor.type === 'element' &&
prevSelection.focus.type === 'element' &&
nextSelection.anchor.type === 'text' &&
nextSelection.focus.type === 'text';

if (!isDeletingLine && $isTextNode(prevNode) && $isTextNode(nextNode)) {
if (
!isDeletingLine &&
$isTextNode(prevNode) &&
$isTextNode(nextNode) &&
prevNode.__parent === nextNode.__parent
) {
// This has the assumption that object key order won't change if the
// content did not change, which should normally be safe given
// the manner in which nodes and exportJSON are typically implemented.
return (
prevNode.__type === nextNode.__type &&
prevNode.__text === nextNode.__text &&
prevNode.__mode === nextNode.__mode &&
prevNode.__detail === nextNode.__detail &&
prevNode.__style === nextNode.__style &&
prevNode.__format === nextNode.__format &&
prevNode.__parent === nextNode.__parent
JSON.stringify(prevEditorState.read(() => prevNode.exportJSON())) ===
JSON.stringify(nextEditorState.read(() => nextNode.exportJSON()))
);
Comment on lines 213 to 216
Copy link
Author

Choose a reason for hiding this comment

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

category Functionality

The current implementation of isTextNodeUnchanged uses JSON.stringify and exportJSON to compare text nodes. This approach might miss some subtle changes or introduce unnecessary comparisons. Consider implementing a more direct comparison of the relevant properties of the text nodes, such as __text, __mode, __format, and __style. This will ensure more accurate change detection and potentially improve performance.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

}
return false;
Expand Down
2 changes: 1 addition & 1 deletion packages/lexical-html/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function $appendNodesToHTML(
let target = currentNode;

if (selection !== null) {
let clone = $cloneWithProperties<LexicalNode>(currentNode);
let clone = $cloneWithProperties(currentNode);
clone =
$isTextNode(clone) && selection !== null
? $sliceSelectedTextNodeContent(selection, clone)
Expand Down
3 changes: 0 additions & 3 deletions packages/lexical-react/src/LexicalContentEditable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {forwardRef, Ref, useLayoutEffect, useState} from 'react';
import {ContentEditableElement} from './shared/LexicalContentEditableElement';
import {useCanShowPlaceholder} from './shared/useCanShowPlaceholder';

/* eslint-disable @typescript-eslint/ban-types */
export type Props = Omit<ElementProps, 'editor'> & {
editor__DEPRECATED?: LexicalEditor;
} & (
Expand All @@ -31,8 +30,6 @@ export type Props = Omit<ElementProps, 'editor'> & {
}
);

/* eslint-enable @typescript-eslint/ban-types */

export const ContentEditable = forwardRef(ContentEditableImpl);

function ContentEditableImpl(
Expand Down
4 changes: 3 additions & 1 deletion packages/lexical-react/src/LexicalHistoryPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@ export {createEmptyHistoryState} from '@lexical/history';
export type {HistoryState};

export function HistoryPlugin({
delay,
externalHistoryState,
}: {
delay?: number;
externalHistoryState?: HistoryState;
}): null {
const [editor] = useLexicalComposerContext();

useHistory(editor, externalHistoryState);
useHistory(editor, externalHistoryState, delay);

return null;
}
Comment on lines 19 to 31
Copy link
Author

Choose a reason for hiding this comment

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

category Functionality

The new delay parameter has been added to the HistoryPlugin function and passed to the useHistory hook. However, there's no documentation or explanation about what this parameter does or how it affects the plugin's behavior. Could you please add a comment or JSDoc explaining the purpose of the delay parameter and how it's used in the history functionality? This will help other developers understand how to use this new option correctly.

Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

26 changes: 11 additions & 15 deletions packages/lexical-selection/src/lexical-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,37 +36,35 @@ import {
function $updateElementNodeProperties<T extends ElementNode>(
target: T,
source: ElementNode,
): T {
): void {
target.__first = source.__first;
target.__last = source.__last;
target.__size = source.__size;
target.__format = source.__format;
target.__indent = source.__indent;
target.__dir = source.__dir;
return target;
}

function $updateTextNodeProperties<T extends TextNode>(
target: T,
source: TextNode,
): T {
): void {
target.__format = source.__format;
target.__style = source.__style;
target.__mode = source.__mode;
target.__detail = source.__detail;
return target;
}

function $updateParagraphNodeProperties<T extends ParagraphNode>(
target: T,
source: ParagraphNode,
): T {
): void {
target.__textFormat = source.__textFormat;
return target;
}

/**
* Returns a copy of a node, but generates a new key for the copy.
* Returns a clone of a node with the same key and parent/next/prev pointers and other
* properties that are not set by the KlassConstructor.clone (format, style, etc.).
* @param node - The node to be cloned.
* @returns The clone of the node.
*/
Expand All @@ -79,16 +77,14 @@ export function $cloneWithProperties<T extends LexicalNode>(node: T): T {
clone.__prev = node.__prev;

if ($isElementNode(node) && $isElementNode(clone)) {
return $updateElementNodeProperties(clone, node);
}

if ($isTextNode(node) && $isTextNode(clone)) {
return $updateTextNodeProperties(clone, node);
$updateElementNodeProperties(clone, node);
if ($isParagraphNode(node) && $isParagraphNode(clone)) {
$updateParagraphNodeProperties(clone, node);
}
} else if ($isTextNode(node) && $isTextNode(clone)) {
$updateTextNodeProperties(clone, node);
}

if ($isParagraphNode(node) && $isParagraphNode(clone)) {
return $updateParagraphNodeProperties(clone, node);
}
return clone;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/lexical-website/src/components/Gallery/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,6 @@ export function useFilteredExamples(examples: Array<Example>) {
searchName,
tags,
}),
[examples, searchName],
[examples, searchName, tags],
);
}