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

Bug: history should work with custom TextNodes containing non-standard properties #6409

Closed
CanRau opened this issue Jul 16, 2024 · 1 comment · Fixed by #6420 · May be fixed by phazingazrael/Terra-Logger#34
Closed

Bug: history should work with custom TextNodes containing non-standard properties #6409

CanRau opened this issue Jul 16, 2024 · 1 comment · Fixed by #6420 · May be fixed by phazingazrael/Terra-Logger#34

Comments

@CanRau
Copy link
Contributor

CanRau commented Jul 16, 2024

I created my own TextNode to have random css class support via the added property __className and some methods like addClass. While trying it out via a button in my toolbar I realized that the editor wouldn't allow me to undo this change, while all other changes work as expected, checked lexical source code, playground code, issues and searched discord until I asked directly on Discord and after poking around a little @etrepum discovered the hardcoded isTextNodeUnchanged helper function which won't consider custom properties.

I think it would be nice to move isTextNodeUnchanged as a method maybe isUnchanged (or maybe isChanged or hasChanged or hasChanges) into TextNode so subclasses can override it when they need to support custom properties. Then the history plugin would call that method here instead of the hardcoded function.

This method might even live in LexicalNode so other nodes could also make use of it, tho I don't have a direct need for this right now I think.

Lexical version: 0.16.1

Steps To Reproduce

  1. Create a CustomTextNode extending TextNode
  2. Add a non-standard property like __className
  3. Duplicate a method like setFormat from TextNode to set your property from 2
  4. trigger this method somehow
  5. Realize that undo won't work 😅

My addClass method

addClass(className: string): this {
	const self = this.getWritable();
	const classes = self.__className?.split(" ").map((s) => s.trim());
	classes.push(className);
	self.__className = classes.join(" ");
	return self;
}

The button onClick that calls it (my actual code is a little longer and inspired by $patchStyleText, tho this is enough to reproduce the problem)

onClick={() => {
    activeEditor.update(() => {
        const selection = $getSelection();
        if ($isRangeSelection(selection)) {
            const selectedNodes = selection.getNodes();
            if ($isTextNode(selectedNodes[0])) {
                selectedNodes[0].addClass("text-red-500");
            }
        }
    });
}}
@etrepum
Copy link
Collaborator

etrepum commented Jul 18, 2024

Adding some more context here, this hack came from #2258 where an older version of the playground's MaxLengthPlugin did not use $restoreEditorState (added in #2273). It may actually be possible to remove the hack entirely.

etrepum added a commit to etrepum/lexical that referenced this issue Jul 18, 2024
Add delay option to HistoryPlugin
Add test for facebook#6409
Fix lint issues
Fix $cloneWithProperties and test expectations
Targeted fix for TextNode leaf change
etrepum added a commit to etrepum/lexical that referenced this issue Jul 18, 2024
Add delay option to HistoryPlugin
Add test for facebook#6409
Fix lint issues
Fix $cloneWithProperties and test expectations
Targeted fix for TextNode leaf change
github-merge-queue bot pushed a commit that referenced this issue Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants