YousefED commented 1 year ago

Following the discussions here and here, I've started experimenting with whether it's possible to return nested elements from createDOM

Why?

This is useful for all scenarios where you want more find-grained control over the DOM output of Lexical, but don't want to do so by changing the data model (Lexical nodes, etc). E.g.:

A checkbox list item where the checkbox is an actual <input>
@LuciNyan mentioned it would be useful for draggable blocks
...

PR

In this PR, I modified the Reconciler to insert children in the spot indicated by an attribute data-gap. I've also modified the quote element in the playground to include a checkbox. This implementation is mainly meant for discussing the overall feature, I think this way (with an attribute) is too hacky / non type-safe to be considered a viable API.

Demo

https://user-images.githubusercontent.com/368857/192089088-14386e69-f1d3-48bb-a4c4-1a921aaf441d.mp4

Known bugs

~~Cursor handling: in the demo we can position the cursur around the checkbox~~ (fixed in https://github.com/facebook/lexical/pull/3062/commits/e81effef02bf1637328531f71d6e707be3625ee2)
~~backspace before the quote doesn't work~~ (fixed in https://github.com/facebook/lexical/pull/3062/commits/e81effef02bf1637328531f71d6e707be3625ee2)

Questions

Do you see other possible bugs with an implementation like this? I'm not an expert yet on the internals of Lexical, so I'd love to hear from someone who is whether it might conflict with other parts of the framework.
What would be a viable API for this? I think we could explore modifying the createDOM contract to return something like:

type LexicalNodeDomRepresentation = HTMLElement | { element:  HTMLElement; parentNodeOfChildren: HTMLElement; }

An alternative could be to provide functions to hook into the reconciler, where the "parentNode" can be returned on the fly. This might offer more flexibility, but is probably not ideal in terms of keeping the reconciler clean and performant

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
lexical	✅ Ready (Inspect)	Visit Preview	Oct 3, 2022 at 10:45AM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	Oct 3, 2022 at 10:45AM (UTC)

facebook-github-bot commented 1 year ago

Hi @YousefED!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

echarles commented 1 year ago

Hi @YousefED, I have read the discussion on discord you linked to, as well as the well-known blog post about the data model behind notion linked from those discussion https://www.notion.so/blog/data-model-behind-notion

I have the impression that the ElementNode already allows to return a DOM node from the createNode method that can have children (it is up to the developer to return any DOM node for its ElementNode as explained in this doc). So what does this PR adds as feature?

A second thing I am wondering is if this PR is willing to tackle an answer to move to a Notion data mode (I guess we are not there yet)?

YousefED commented 1 year ago

I have the impression that the ElementNode already allows to return a DOM node from the createNode method that can have children (it is up to the developer to return any DOM node for its ElementNode as explained in this doc). So what does this PR adds as feature?

Good question, might not have been clear from the PR. If you currently use Lexical to return a more advanced DOM, children will always be appended to the end. Let's say you'd implement createDOM to return <div><h1></h1></div>, you'd get:

Note how the <span> representing the text contents of the h1 is appended to the div, instead of what you actually want; the span as a child of the h1.

This PR allows you to tag the h1 as the "parentNodeOfChildren" (or "gap" node) so that children will be added by the reconciler in the correct position.

A second thing I am wondering is if this PR is willing to tackle an answer to move to a Notion data mode (I guess we are not there yet)?

Nope, this discussion is mostly unrelated to this discussion you're referring to. Let's try to keep this separate!

echarles commented 1 year ago

This PR allows you to tag the h1 as the "parentNodeOfChildren" (or "gap" node) so that children will be added by the reconciler in the correct position.

Could you also paste the structure without and with this PR so we can see the difference easily?

Apart from the DOM structure difference, what is the benefit for the user, or the additional capability this offers?

YousefED commented 1 year ago

Could you also paste the structure without and with this PR so we can see the difference easily?

I've edited my comment above to phrase things more clearly.

Apart from the DOM structure difference, what is the benefit for the user, or the additional capability this offers?

There is no instant benefit for the end user, but it allows plugin authors to create more advanced elements and have tighter control over the rendering of Lexical elements (see "why" in the opening post).

facebook-github-bot commented 1 year ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

trueadm commented 1 year ago

Why not take a step back here. Instead, what if we provided reconciler hooks as node method to handle the removal and appending of DOM child elements.

Example:

appendChildDOM(dom: HTMLElement, childDOM: HTMLElement): void {
  dom.firstChild.appendChild(childDOM)
}

You’d do the same for removals too and possibly replacements. Then the reconciler can just use those methods rather than it’s own logic. Simples.

YousefED commented 1 year ago

Why not take a step back here. Instead, what if we provided reconciler hooks as node method to handle the removal and appending of DOM child elements.

That'd be a very clean and flexible solution indeed, thanks! I can take a shot at this.

Any pointers on where to address the cursor issues listed at "known bugs" and / or do you foresee any other issues? I think it's smart to address those first before optimizing the API design.

acywatson commented 1 year ago

Any pointers on where to address the cursor issues listed at "known bugs" and / or do you foresee any other issues? I think it's smart to address those first before optimizing the API design.

Is this actually doing what you want? Don't you want to be able to change the text content of the H1 or the Input label or whatever other thing you append to the DOM inside the wrapper? I think we're going to have issues around Selection here, generally - how will Lexical be able to move Selection between DOM elements within the same node so you can insert text in one or the other? We can probably make it work, more just trying to figure out what the end goal here and not do something that only gets us part of the way there and doesn't address other pieces of the use case.

If all you're trying to do is add some static DOM nodes to what Lexical renders for a given, then this could work for that case, but we'll probably need to add some handling in the Selection code somewhere.

YousefED commented 1 year ago

Don't you want to be able to change the text content of the H1 or the Input label or whatever other thing you append to the DOM inside the wrapper?

The text content of the H1 is already editable - so that works fine
I don't think that in this case we also want a different element (like an input label) to be editable. If we'd want an input label to be editable as well, at this moment we'd need a different ElementNode with an editable TextNode, which I think is fine.
If we'd want multiple elements to be editable (i.e.: a CalloutElement with a title and body), I think there are other ways in Lexical to achieve this? i.e.: Transforms? (not too up to speed with this yet). In any case I'd consider this a separate issue, perhaps more closely related to the problem of enforcing schemas than controlling output rendering. What do you think?

If all you're trying to do is add some static DOM nodes to what Lexical renders for a given, then this could work for that case, but we'll probably need to add some handling in the Selection code somewhere.

Is there a function that signals to Lexical whether a given position is valid or not? Or do you think we'd need to set contenteditable to false for the input element? (I tried it, but comes with a new set of issues)

PS: Any idea why the playground build fails? I can't see the error Vercel is running into

acywatson commented 1 year ago

The text content of the H1 is already editable - so that works fine

Oh interesting - I wasn't able to get that to work when checking out your branch locally. To be clear, I'm talking about append an h1 tag to the wrapper before the blockquote, replacing your input node.

I don't think that in this case we also want a different element (like an input label) to be editable.

I was thinking this is how it works in notion, but it looks like I was mistaken there, actually.

In any case I'd consider this a separate issue, perhaps more closely related to the problem of enforcing schemas than controlling output rendering. What do you think?

Looking at how it works in other block editors and thinking this through again, I do think of these as separate technical concerns, though I'm not sure schema enforcement is directly related. Instead of thinking about this as "how can we enable this specific technical thing to happen in Lexical?" I'm trying to think of it as "what's the user problem and is this technical change the best way to solve it?"

In those terms, it sounds like this change will help in some concrete use case, I was just wondering about whether how it works will generally match expectations in userland.

Is there a function that signals to Lexical whether a given position is valid or not? Or do you think we'd need to set contenteditable to false for the input element? (I tried it, but comes with a new set of issues)

I suspect the issues are around Lexical trying to reconcile it's Selection back to the DOM Selection, but I really don't know off the top of my head.

For the problem with backspace deleting, I imagine this is something around how we calculate whether or not you should collapse into the previous block sibling, but I'd have to get in there with a debugger to figure it out.

PS: Any idea why the playground build fails? I can't see the error Vercel is running into

I have never seen that particular error, but it may have to do with your use of the safe navigation operator. We don't support that syntax in this codebase right now. Try removing that and just using null checks and see what happens.

YousefED commented 1 year ago

Oh interesting - I wasn't able to get that to work when checking out your branch locally. To be clear, I'm talking about append an h1 tag to the wrapper before the blockquote, replacing your input node.

My bad, that wouldn't work indeed as the current design is only intended to have a single editable element, the one indicated by "gap". If more input elements are required, that falls into the category discussed below.

I'm trying to think of it as "what's the user problem and is this technical change the best way to solve it?". (...) In those terms, it sounds like this change will help in some concrete use case, I was just wondering about whether how it works will generally match expectations in userland.

That makes sense, although it's difficult to talk about a single "user" when developing a framework. Anyway, I think we should distinguish two separate use cases:

A) Fine-grained control of the DOM output of single Elements

Users should be able to edit the contents of elements visually wrapped / prepended by (possibly interactive) dom elements.
Plugin authors should have full control of the DOM output of their Nodes, not being forced to fallback to CSS hacks

Examples:

checklist prepended with a checkbox element
an image / video with an editable caption (I see this is currently done with a DecoratorNode and a nested editor, which also works)
An extension that extends all elements to include a draghandle before it (note that this has already been solved differently in the Draggable Blocks extension)

Imo the fact that I can't easily design an Todo List with a browser-native checkbox is important enough to focus on this scenario first (unless there's another way to accomplish this in Lexical).

B) Forcing a DOM structure of ElementNodes with multiple children

(note: I think this can come after A / can be seen as independent)

Brainstorming possible use cases (perhaps some of these are already possible with Lexical in other ways? Haven't thought these through completely yet):

A Toggle List with a Title (visible when collapsed) and editable children (hidden when collapsed)
Nested items: in Notion, elements indented are considered "children" of the element above. In that case, I would use this functionality to wrap the children in a div that has a padding-left (i.e., I would render a HeadingNode with children as <div><h1>...h1contents...</h1><div style="padding-left:10px">...children...</div></div>)
A "callout" / "alert" element with a title and a body, both of which should be editable
...more ideas?

I suspect the issues are around Lexical trying to reconcile it's Selection back to the DOM Selection, but I really don't know off the top of my head.

I'll start learning more about Selections in lexical, thanks

I have never seen that particular error

The issue is that I can't access the Vercel logs, so I don't know what the current error is

acywatson commented 1 year ago

The issue is that I can't access the Vercel logs, so I don't know what the current error is

Same as the one here: https://github.com/facebook/lexical/actions/runs/3127485606/jobs/5075210787

Can you not access that?

YousefED commented 1 year ago

Can you not access that?

Thanks, that works. Unfortunately that's not what github links to automatically when it says "deployment failed" so I missed that one. Anyway, it was the operater indeed and the Playground on vercel now works :)

YousefED commented 1 year ago

Both issues have been fixed in https://github.com/facebook/lexical/pull/3062/commits/e81effef02bf1637328531f71d6e707be3625ee2; the trick was to make sure the parts where we don't want a cursor are not contentEditable.

https://user-images.githubusercontent.com/368857/192367939-665bd1d1-6058-4fd2-a033-5037d7b446c3.mov

There is a new issue that upon creation of an element by typing >, the cursor is not set to the correct TextNode, should be easily fixable

YousefED commented 1 year ago

Why not take a step back here. Instead, what if we provided reconciler hooks as node method to handle the removal and appending of DOM child elements.

Example:
appendChildDOM(dom: HTMLElement, childDOM: HTMLElement): void {
  dom.firstChild.appendChild(childDOM)
}
You’d do the same for removals too and possibly replacements. Then the reconciler can just use those methods rather than it’s own logic. Simples.

@trueadm I've modified the reconciler based on your suggestion. I think this can definitely work! Looking forward to your feedback

acywatson commented 1 year ago

it's difficult to talk about a single "user" when developing a framework.

In this case, "user" is an adjective, not a reference to a specific individual. Changing the library to allow rendering arbitrary DOM structures from nodes ("full control of rendering output") is a solution, but we try to think in terms of the problems that we're trying to go after with the solution to ultimately get to how people will actually use it. This helps inform API design.

Looks like you got that part, though, since you followed up with some use cases - just wanted to clarify my perspective here since it seems like there was some confusion.

perhaps some of these are already possible with Lexical in other ways?

Technically, all of the things you mentioned are already possible in Lexical in other ways, unless you impose the somewhat arbitrary constraint of (e.g.) wanting to use a browser native checkbox for checklists, right?

A few minor thoughts/concerns/trade-offs I see:

1) Expands the core in terms of both size and API surface. This is not a blocker (for me), I'm just pointing it out.

2) As far as I can tell, this should play nicely with all of the public APIs we have that deal with the DOM, such as getNearestNodeFromDOM and getNodeFromDOMNode, but maybe I'm missing something.

3) This works for cases where the user wants to add an inert DOM nodes to the node output and the solution for handling Selection gracefully is just to make the inert stuff contenteditable = false. I'm wondering how many people using this are going to struggle with that in different cases. That being said, I'd say this is mostly for more advanced users, so not terribly concerning.

Overall, I think this is generally a good change. Nice work.

acywatson commented 1 year ago

The current naming is inline with regular DOM methods; insertBefore, removeChild, replaceChild + "DOM". I thought about adding "Child" as well, but then either we'd get "removeChildDOMChild" / "DOMChildRemoveChild" which imo doesn't make sense, or we'd lose consistency with DOM method naming and only change insertBeforeDOM to insertChildBeforeDOM.

I somehow managed to delete my original comment here about the naming.

I see what you mean, I just find the whole thing to be terribly confusing for someone that's just approaching this without an understanding of how the reconciler works. It's super easy to lose track of what "dom" we're referring to in different contexts here. People already have issues with the updateDOM semantics, so I was trying to think generally about how we can make this more clear. Your inline docs go a long way. Might even consider an example section in the documentation if this ends up merged.

YousefED commented 1 year ago

Thanks for the feedback!

Technically, all of the things you mentioned are already possible in Lexical in other ways, unless you impose the somewhat arbitrary constraint of (e.g.) wanting to use a browser native checkbox for checklists, right?

I'm not sure, so would love to learn more. I think the most similar functionality is implemented with DecoratorNodes and nested editors, such as the Image with a Caption in the playground. However, that solution suffers from issues like selecting parts of the caption and adjacent nodes in the outer editor at the same time is not possible (i.e.: the caption really feels like an "outside editable element").

As far as I can tell, this should play nicely with all of the public APIs

Great. I'm fairly sure it should, but as this is my first contribution I'm not the best to confirm this, so more input welcome :)

I see what you mean, I just find the whole thing to be terribly confusing for someone that's just approaching this without an understanding of how the reconciler works

I'll give the naming some more thought. Another thought I had re. API design is that it might make sense to pass the concerning Element to the function as well, so it becomes clear which element's dom needs to be inserted. I.e.:

insertBeforeDOM(
    dom: HTMLElement,
    child: LexicalNode,
    childDOM: HTMLElement,
    referenceNode: Node | null,
  )

Taking this one step further might be to remove childDOM from the parameter list, and make obtaining / creating the childDOM a concern of the LexicalNode as well, but this might take away too much responsibility from the Reconciler and I'm not yet sure if it's possible / desirable

GermanJablo commented 1 year ago

Hi, I just read this thread 😁

Nope, this discussion is mostly unrelated to https://github.com/facebook/lexical/issues/1262#issuecomment-1186170960 you're referring to. Let's try to keep this separate!

It's not really unrelated. It seems to be just what I meant in the "Edit 2" section by extending my argument number 6. Repeating the important parts:

This is perhaps more personal, but I think that a functional API instead of a class-based API like the one I propose below would offer a much friendlier development experience. Even element nodes could be defined as React components (as slate does for example).

...

Looking a bit at the SlateJS API I mentioned in point 6 (do we agree that it's beautiful?), I think another improvement that could be made to ElementNode is that instead of forcing that 1 ElementNode = 1 HTML tag node, the following more flexible conditions could be imposed:

An ElementNode should always have a top-level element.

An ElementNode should not have text nodes (obviously I mean the DOM inherent to the node, not its child nodes).

The first condition would be to make deserialization much easier. The second condition would be to make selection much easier. Right now, if someone wants to implement a node that isn't 1-to-1 related to an HTML tag, their only option is to make a decoratorNode (which isn't always desirable), or to create a bunch of nodes in a very complex way ( as happened to @gutenye here (see the codesandbox) and is happening to me in other use cases).

What I was referring to, and perhaps not explaining very clearly, is that in Slate a node can be a React component. For example, this is how code blocks are made:

// Define a React component renderer for our code blocks.
const CodeElement = props => {
  return (
    <pre {...props.attributes}>
      <code>{props.children}</code>
    </pre>
  )
}

In the comment above I mentioned the two conditions that I think would make it easier to allow this behavior (selection-wise and deserializing-wise). Perhaps the second could be relaxed like this: "if an ElementNode has a text node inside it, it must be contenteditable false".

trueadm commented 1 year ago

I don't think we want to proceed with this. createDOM should remain straight-forward and making it more complex will only make for more issues around difficult problems (like selection and DOM mutation listening). I think the correct approach is to use plugins to position absolutely positioned elements over LexicalElement nodes, so it gives the impression they are one but really they are not.

facebook / lexical

PoC of more advanced elements in createDOM #3062

Why?

PR

Demo

Known bugs

Questions

Action Required

Process

A) Fine-grained control of the DOM output of single Elements

B) Forcing a DOM structure of ElementNodes with multiple children