facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
17.5k stars 1.45k forks source link

Proposal: DecoratorElementNode #5930

Open yf-yang opened 3 weeks ago

yf-yang commented 3 weeks ago

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.

etrepum commented 3 weeks ago

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 commented 3 weeks ago

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

I agree with you. Seems it is still compatible with Decorator node.

Let me further illustrate the last consideration:

Suppose I want to implement a CardNode, whose title and body are two separate editable areas. Then it may be like:

class CardNode extends DecoratorNode {
  _titleRoot = $createElementNode();
  _bodyRoot = $createBodyNode();

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

  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 type signature is like:

class DecoratorNode<T, RootKeys extends string | undefined = undefined> {

  // 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
  rootNodes: RootKeys extends string
    ? () => Record<RootKeys, LexicalNode>
    : undefined = undefined;

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

That setRootElement signature looks cumbersome, but it's enough to illustrate the idea.

etrepum commented 3 weeks ago

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 commented 3 weeks ago

children expect to have their parent be an ElementNode

You are right.

getParent usually involves positional methods like getPreviousSiblings, getNodesBetween, insertAfter. Taking the CardNode example above, title and body's parent is the card node and their positional methods make no sense. I am not sure how this should be modified. My intuitive implementation is like:

class CardNode extends DecoratorNode {
  _titleRoot = $createDecoratorRootNode();
  _bodyRoot = $createDecoratorRootNode();

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

  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>
    );
  }
}
class DecoratorNode<T, RootKeys extends string | undefined = undefined> {

  // 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
  rootNodes: RootKeys extends string
    ? () => Record<RootKeys, LexicalDecoratorRootNode>
    : undefined = undefined;

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

DecoratorRootNode's name is still confusing, that's for demo. The node extends ElementNode and its isShadowRoot() should always return true.

Backward compatibility is just fine because no production code is using this approach (no existing node is such node's children). I'd expect this one be marked as experimental for a while, then when people tries to switch from nested editor to this implementation, some bugs may occur and we can make them compatible then.

I've checked $isRootOrShadowRoot and $isRootNode's usage, not sure if this implementation is sound with each use case, experiments are needed. Existing production codes are fine, right?

etrepum commented 3 weeks ago

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 commented 3 weeks ago

Yes that's inevitable. This node should be experimental and it takes a while to be stable, but anyway those utility codes are easy to fix, just hard to find all the cases.

Or we rewind back to the first draft, but I'm curios how CardNode would be implemented.

I personally think Lexical should support "a node with known number named children".

yf-yang commented 3 weeks ago

Or what about this? Tuple (known number of children named by indexes) and objects are the same, so

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

  getChildren() {
    return [this._titleRoot, this._bodyRoot]
  }

  decorate(_editor, _config, setRootElement) {
    const setTitle = element => setRootElement(0, element);
    const setBody = element => setRootElement(1, element);
    return (
      <div>
        <div>Title:<div ref={setTitle} /></div>
        <div>Body:<div ref={setBody} /></div>
      </div>
    );
  }
}
class DecoratorElementNode<T> extends ElementNode {

  // Fixed length
  getChildren: () => LexicalDecoratorRootNode[];

  // Index modifiers should throw errors
  // Index accessors are remained to be compatible with other utility codes
  //   but they don't make any sense

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

That could make node traversal of existing utility functions easier to become compatible, but readability is a little bit worse.

yf-yang commented 3 weeks ago

Proposal updated.

StyleT commented 3 weeks ago

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

yf-yang commented 2 weeks ago

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

https://github.com/facebook/lexical/pull/5981

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

LvChengbin commented 2 days ago

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 commented 2 days ago

@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 commented 1 day ago

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 commented 1 day ago

@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 commented 1 day ago

@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. 😵