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

Proposal: DecoratorElementNode #5930

Open
yf-yang opened this issue Apr 20, 2024 · 30 comments
Open

Proposal: DecoratorElementNode #5930

yf-yang opened this issue Apr 20, 2024 · 30 comments
Labels
core Reconciler, DOM, Selection, Node, Events, Composition enhancement Improvement over existing feature

Comments

@yf-yang
Copy link
Contributor

yf-yang commented Apr 20, 2024

I've wanted this feature for a long long time, today I get some ideas to implement it, so I'd like to discuss it here.

People need something like "ReactElementNode"

I've seen such questions again and again. People not only need a plain ElementNode which is a single DOM node, they want to also use other framework's generated DOM node as the parent of a set of Lexical nodes. However, this is not natively supported.

Let's first check other frameworks. Slate.js seamlessly supports it, TipTap breaks the element and the child into NodeViewWrapper and NodeViewComponent, check doc and example. All of them are handle those cases in one editorState.

Now let's go back to Lexical. Lexical offers nested editors and LexicalNestedComposer. However, after heavily develop use cases with it, I find it has a lot to improve.

Why nested editor is not a good idea?

  1. Moving contents between editors is painful.
    You can't directly move nodes between editors. Serialization is mandatory and I have to deal with at least two editor.updates.

  2. Create a plugin that supports multiple editors is painful.
    Due to previous reason, when I created a drag and drop plugin that supports nested editors, I have to consider if the editor to be dropped is the same with the dragging node's editor. There are two cases, the first one is one editor.update, the second one needs two update call and serialization.

  3. Modifying multiple editors SIMULTANEOUSLY is poorly supported.
    Suppose I move a node from one editor to the other. This is a transaction that involves two editors. However, both HistoryPlugin and CollaborationPlugin treats them as two separate transactions. I've spent lots of time modifying them.

I've been going deeper with those questions. I think, as an editor framework whose idea originates from React, the editorState should also be singleton, just like how React does. To be more precise, for a single LexicalComposer, all the nodes within it, including those nested editors, should share the same editorState.

Implementation Proposal

What nested editor is doing

I'd like to share some understanding of Lexical. First, Lexical (core) is a framework that only deals with DOM nodes. Each Lexical node is expected to represent exactly one DOM node, so when the editorState (Lexical node tree) updates, the Lexical node tree is mapped to the DOM tree. One Lexical node <=> one DOM node, element node maps to a DOM node contains a sequence of child DOM nodes. TextNode maps a DOM node with no child. Decorator node maps to a DOM node with no child as the portal, the portal can mount, for example, child React elements.

What nested editor does is simple: it adds another root DOM, then mounts its editor's DOM tree to that root. That's all.

It means, if we supports multiple root DOMs in Lexical core, then nested editors will no longer be needed, and we can merge nested node tree into one node tree. Now only one editorState, and we are able to implement DecoratorElementNode.

How the API will be like

Nested editor needs a nested LexicalContentEditable, which is for setting the root element during the first render. We can take the procedure in the core.

Example: CardNode with two editable areas title and body.

class CardNode extends DecoratorElementNode {
  _titleRoot = $createNestedRootNode();
  _bodyRoot = $createNestedRootNode();

  rootNodes() {
    return {
      title: this._titleRoot.getKey(),
      body: this._bodyRoot.getKey(),
    }
  }

  decorate(_editor, _config, setRootElement) {
    const setTitle = element => setRootElement('title', element);
    const setBody = element => setRootElement('body', element);
    return (
      <div>
        <div>Title:<div ref={setTitle} /></div>
        <div>Body:<div ref={setBody} /></div>
      </div>
    );
  }
}

The TypeScript signature of DecoratorElementNode is like:

class DecoratorElementNode<T, RootKeys extends string> extends ElementNode {
  // Both the keys and the values of the return value could change when the node updates
  // Just like how a decorator with nested editors could behave
  // LexicalKey must be key of LexicalNestedRootNode?
  rootNodes(): Record<RootKeys, LexicalKey> {
    return {};
  }

  decorate(
    editor: LexicalEditor,
    config: EditorConfig,
    setRootElement: (rootKey: RootKey, element: null | HTMLElement) => void
  ): T {}

  // Lexical core should avoid calling this method during updates
  // For tree traversal utilities only, the order is meaningless
  getChildren(): LexicalNestedRootNode[] {
    // Seems reorder and variable length is just fine if we do not call this method in Lexical core?
    // I'd like to recommend people to keep it fixed themselves, but not force to do so.
    return Object.values(this.rootNodes())
  }

  // similar to previous method, those indexed children accessor should be overridden
  // indexed children modifier should throw exceptions
  getChildAtIndex(index: number): LexicalNestedRootNode | T {}
}

class LexicalNestedRootNode extends ElementNode {
  isShadowRoot() { return true; }

  // Those positional methods should be overridden
  getNextSibling() {
    return this.getParentOrThrow().getChildAtIndex(this.getIndexWithinParent() + 1);
  }
}

How the editor update executes

I've not thought about it thoroughly, but the idea is the same with nested editors. During the first render, the DecoratorElementNode's rootElement is null, nothing happens. Later in another micro task, the decorators are rendered by frameworks like React and root element is now available, then queue another micro task to mount the node's children in Lexical. Later on, if the rootElement is available, directly update the children.

Summary

Only one editorState for all nested editors improves DX a lot. We can implement by moving how nested editors are currently rendered to Lexical core.

@yf-yang yf-yang added the enhancement Improvement over existing feature label Apr 20, 2024
@etrepum
Copy link
Collaborator

etrepum commented Apr 20, 2024

I don't have any specific feedback on the implementation proposal here but this would be a very welcome feature addition if it could be made to work well!

I wonder if this style of decoration would be better off as a protocol that any node could participate in (possibly excluding TextNode), rather than adding adding a separate superclass for this behavior

@yf-yang

This comment was marked as outdated.

@etrepum
Copy link
Collaborator

etrepum commented Apr 21, 2024

The "DecoratorElementNode" proposed in the issue sounded like some other class hierarchy where it extends ElementNode but also has Decorator functionality.

I think a challenge with the above comment's approach is the backwards incompatibility where code that has to traverse the document now has to look for children through decorators, children expect to have their parent be an ElementNode (and the related functionality to be able to remove themselves). Maybe for practical reasons these decorator's root nodes would have to be some other specific ElementNode class that behave like a shadow root?

@yf-yang

This comment was marked as outdated.

@etrepum
Copy link
Collaborator

etrepum commented Apr 22, 2024

I think that existing production code should be fine in any scenario where nodes using this new protocol or node type are just not even registered with the editor. My primary concern is that there is a fair amount of utility code that expects to be able to work with a Lexical document and currently covers all cases, but when a new way of traversing the tree is added they will need to be changed accordingly ($dfs, possibly anything related to selections, serialization/deserialization, etc.)

@yf-yang

This comment was marked as outdated.

@yf-yang

This comment was marked as outdated.

@yf-yang
Copy link
Contributor Author

yf-yang commented Apr 22, 2024

Proposal updated.

@StyleT StyleT added the core Reconciler, DOM, Selection, Node, Events, Composition label Apr 23, 2024
@StyleT
Copy link
Contributor

StyleT commented Apr 23, 2024

LGTM! I bet this can be implemented. The only question is who has capacity + knowledge for this :)

@yf-yang
Copy link
Contributor Author

yf-yang commented Apr 28, 2024

I've changed some API design and initiate an usable draft

#5981

@etrepum @StyleT You are welcome to comment and share opinions!

@LvChengbin
Copy link

To be honest, this problem will definitely become the main reason for me to abandon the use of Lexical one day.
I hope this can be implemented soon.

@LvChengbin
Copy link

@yf-yang I have no idea to handle selection between nested editors correctly, would you like to give me some advices?

image

As what you can see from the screenshot I make from the playground. Clicking a ImageNode in nested editor is not able to deselect the ImageNode in its' parent editor and vice versa.

Both SELECTION_CHANGE_COMMAND and registerUpdateListener cannot detect the selection changes between nested editor and I tried listening to the BLUR_COMMAND on editors and then checking if the relatedTarget is an element in another editor but still under the top editor (the editor with _parentEditor is null), but it can't handle the operation like click an ImageNode -> click on toolbar -> click an ImageNode in another editor.

@yf-yang
Copy link
Contributor Author

yf-yang commented May 15, 2024

To be honest, this problem will definitely become the main reason for me to abandon the use of Lexical one day.

TBH I'm losing my patience too. I've invested about 18 months here, and found nested editor/decorator element is really bad, I cannot have my own work done without these features if I want to heavily customize an editor based on Lexical.

However, that is still acceptable as I trust Facebook's brand. I think I have deep understanding of the core implementation, so I tried to create PRs and invested lots of time modifying the core.

The real big problem is, the founder of Lexical has left meta and went to Svelte, he contributes many lines of Lexical code, and most of the core. According to some Discord channel messages, current maintenance team has other stuff to focus on in meta, so they don't have enough time to follow those issues and PRs. Although there are many kind community people who can review some application-wise bugs, no one can even review the core change and give enough feedback, let alone merge or deny them.

I'm investigating plate which is built on top of slate now. Not sure how the core works, so not sure its performance comparison with Lexical, but it seems more mature, and I can write element nodes with React.

@yf-yang
Copy link
Contributor Author

yf-yang commented May 15, 2024

@LvChengbin For your selection case specifically, I've encountered similar issue before. I personally recommend you to create something like , which internally globally maintains a variable that records which editor is being selected. This plugin should be used for all the nested editors and listening to each one's selection change. Whenever one changes, check if the editor being changed is the same with the recorded one. If not, then unselect the previous stored editor.

Selection is bind with editor state and editor state is bind with editor. Therefore, it is not globally unique.

@LvChengbin
Copy link

@yf-yang Thank you, and yes, I considered about this method, but I thought there may be a better way for this problem, because it'll cause some other problems, for example if I want to select multiple ImageNodes by clicking them with Shift Key pressed. 😵

@yf-yang
Copy link
Contributor Author

yf-yang commented May 17, 2024

@LvChengbin dm u in the discord server

@LvChengbin
Copy link

@yf-yang I'm sorry I see your reply so late. I just joined into the discord server.

@CanRau
Copy link
Contributor

CanRau commented Jun 28, 2024

@yf-yang did you end up switching to plate/slate or something else?
Curious why plate and not something like tiptap/prosemirror?

A little worried about what you brought up regarding lexical founder, etc 😬

@yf-yang
Copy link
Contributor Author

yf-yang commented Jun 30, 2024

@CanRau Works well with plate.

Lexical is inspired by prosemirror, so prosemirror is more vanilla JS based.
Tiptap is also based on prosemirror. Also I am concerned with some priced extensions (although I may not need them now). Another concern is its not React centric, some examples are based on Vue.
Compared with them, slate is a complete React editor. Also afaik may big brands are using it, so that may be some sort of proof of its stability. Slate don't have many examples, but Plate has rich examples to start from. In my own case, I need to customize many blocks, instead of directly picking something directly from the framework.

@Sinongamess
Copy link

I also find this issue needing improvement. Whenever I attempt to wrap Element using existing components, I'm always torn between abandoning current progress or resorting to a more hacky approach of traversing the entire tree for repairs. I haven't used LexicalNestedComposer because it would increase my workload and lead me into brainstorming. I feel that having each Element inherit the decorate method is not in conflict with existing dependency-free solutions. I find Lexical and ProseMirror better in performance optimization compared to frameworks relying on existing views. I hope someone can empower users with the choice.

@GermanJablo
Copy link
Contributor

Why nested editor is not a good idea?

  1. Moving contents between editors is painful.
    You can't directly move nodes between editors. Serialization is mandatory and I have to deal with at least two editor.updates.

  2. Create a plugin that supports multiple editors is painful.
    Due to previous reason, when I created a drag and drop plugin that supports nested editors, I have to consider if the editor to be dropped is the same with the dragging node's editor. There are two cases, the first one is one editor.update, the second one needs two update call and serialization.

  3. Modifying multiple editors SIMULTANEOUSLY is poorly supported.
    Suppose I move a node from one editor to the other. This is a transaction that involves two editors. However, both HistoryPlugin and CollaborationPlugin treats them as two separate transactions. I've spent lots of time modifying them.

I think the 3 points can be summed up as 2 transactions instead of one.

This would mean that the user, on these arguably infrequent occasions, has to press ctrl+z 2 times instead of 1. It's not ideal, but in my opinion it's not such a big deal.

A solution that I have thought of is that a timestamp could be added to each undomanager transaction, and if they occurred in a very close interval (e.g. 500ms) they are combined (this is what Yjs does)

@etrepum
Copy link
Collaborator

etrepum commented Jul 12, 2024

The current HistoryStateEntry doesn't track timestamps, and the debouncing in the current implementation wouldn't be able to merge entries from multiple editors.

I think generally there are two use cases for nested editors: those that are used to work around deficiencies in Lexical's UI model (what this proposal is trying to solve), and editors that are ephemeral and/or not so related to the parent editor (maybe editing some state related to a specific node, but on save or update that state will be synced to the other document, like labeling an image or adding a comment).

The former case is not good DX today and is very error-prone. I think it would be worth it to decouple the DOM and model just enough to allow something like this to work, even if there are some caveats/boilerplate/restrictions, it would be easier than dealing with a "distributed system" of editors when you should really only have one logical root.

The latter type probably doesn't really need any infrastructure to support it, because it already works pretty well. The primary concern is just to make sure that events/focus/DOM lookups/etc. go to the correct editor.

@GermanJablo
Copy link
Contributor

The current HistoryStateEntry doesn't track timestamps, and the debouncing in the current implementation wouldn't be able to merge entries from multiple editors.

Exactly, the "current". But it is a doable solution.

I think generally there are two use cases for nested editors: those that are used to work around deficiencies in Lexical's UI model (what this proposal is trying to solve), and editors that are ephemeral and/or not so related to the parent editor (maybe editing some state related to a specific node, but on save or update that state will be synced to the other document, like labeling an image or adding a comment).

I have found one more use case.

A Lexical editor that can be embedded in two different parent Lexical documents.

In Notion, you can embed the same table/DB on two different pages, and if you edit on one, the changes should be reflected on the other.

The obvious way to do this is for the table to be a document/editor itself. Then the pages are two different editors that contain a node of type "table-reference" with an id to the subdocument, which is loaded lazy.

For reference, another good example is the "Synced block" in Notion.


I think there are 2 parallel discussions here:

  1. If nested editors are always a bad idea.
  2. How to solve the undo manager problem with nested editors.

In (1) my opinion is no. I.e., I think nested editors have use cases.

Discussion (2) is not something I have wanted to get into because of my position on (1). Even if an alternative was found to not use nested editors in 90% of the cases, if you still need it in 10% of the cases, you have to find a solution to the problem that works with nested editors.

@etrepum
Copy link
Collaborator

etrepum commented Jul 12, 2024

I think that's fair, I'm not advocating to get rid of nested editors, they certainly do have their use cases especially for large, block-based, and/or collaborative documents. My point is mostly that they are a very awkward solution to the sorts of use cases where you basically want to add one or more DOM nodes to the editor's DOM but you don't want them to have associated nodes in the document tree or participate directly in things like selection (usually the use case is something like CSS :before and/or :after pseudo-elements, but with interactivity). I think a proposal like this one would be a welcome solution to that problem.

Having a better undo manager (for those not already using yjs I guess, since that has a separate history implementation that works a bit better) would be a great idea and much simpler to implement and discuss than this. I think a separate issue or PR with an implementation would be a great place for that discussion.

@GermanJablo
Copy link
Contributor

Sure, but despite everything I still don't understand what problem a "DecoratorElementNode" would solve.

The only problem I see is that of the undomanager, which seems obvious that it should be solved in another way. Is there any other?

@yf-yang
Copy link
Contributor Author

yf-yang commented Jul 13, 2024

I think you miss the point here.

We are not talking about REPLACE nested editors, but adding another type of element node, which could be decorated via modern framework such as React.


I think the 3 points can be summed up as 2 transactions instead of one.

I mention the specific use case somewhere. When you are moving blocks around (especially in a drag and drop case), you need to:

  1. Identify the source editor and target editor and make a comparison
  2. If they are the same, just move.
  3. If they are not the same, OK, you need to serialize it and then deserialize it to another editor. Meanwhile you need to remove nodes from one editor, then add deserialized node to another editor. Think of the dev experience and how many bugs people can make. I am familiar enough with Lexical but other are not.

A solution that I have thought of is that a timestamp could be added to each undomanager transaction, and if they occurred in a very close interval (e.g. 500ms) they are combined

Current history plugin does not distinguish editorStates, so I actually tried to create my own version of history plugin to support similar stuff. For others, there are no out of the box utilities.
I use this modified version of history plugin until I need to apply yjs, yes yjs. Then, if you really check the yjs plugin, you'll find each nested editor is an independent doc. There is no way you can undo multiple nested editors in one go easily. I've checked MultiDocUndoManager and UndoManager's source code. Still there is no out of box solution.


Yes, we can patch all of these stuff (with so much efforts). However, I think the point is, current design is not aligned with real word abstraction, which is —— the document is treated by the user as a whole, so there should be only one editor state. If not, then nested editors is useful.

If we ignore the abstraction and think OK we can patch here, here and there, then new problems would occur again and again when the abstraction does not meet the real word demands.

@yf-yang
Copy link
Contributor Author

yf-yang commented Jul 13, 2024

I still don't understand what problem a "DecoratorElementNode" would solve

Let me show a direct example: A nested list

Each list item could either be one line of text, or a sub list. You should be able to drag a list to the another and it could become a sublist. It should support to as many as 5 nested lists. Nested list could also be moved out to become a non-nested list.

@yf-yang
Copy link
Contributor Author

yf-yang commented Jul 13, 2024

Let me take the core point to a standalone comment:

The document is treated by the user as a whole, so there should be only one editor state. This should be the correct abstraction of a rich text editor design.

@CanRau
Copy link
Contributor

CanRau commented Aug 9, 2024

Would really love to be able to decorate all nodes to re-use components between front-end and backend as we're manually rendering editorstate json on front-end 🤓

@etrepum
Copy link
Collaborator

etrepum commented Dec 2, 2024

I think now that we have the (experimental) getDOMSlot API introduced by scrollable tables (#6759) it should be possible to do something like this in a limited way with an ElementNode, although someone needs to do the work to come up with the additional use cases and test them to make sure that they are indeed fully compatible with how the reconciler works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Reconciler, DOM, Selection, Node, Events, Composition enhancement Improvement over existing feature
Projects
None yet
Development

No branches or pull requests

7 participants