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

Create isolated elements inside Slate #3425

Closed
Francesco-Lanciana opened this issue Jan 13, 2020 · 8 comments · Fixed by #3436
Closed

Create isolated elements inside Slate #3425

Francesco-Lanciana opened this issue Jan 13, 2020 · 8 comments · Fixed by #3436

Comments

@Francesco-Lanciana
Copy link

Do you want to request a feature or report a bug?

Feature Request

What's the current behavior?

If you have an element inside the React Editor that doesn't contain a text node and can receive focus (and therefore steal it from Slate) then you get the error: Cannot resolve a Slate node from DOM node.

This is an issue when you want to add something like CodeMirror inside the editor as it needs focus.

What's the expected behavior?

It would be great if Slate gave you the ability to have isolated elements inside the Editor so you could have an escape hatch from slates event handlers. This would be useful for adding in elements that need to steal focus from the editor, for example a code editor like CodeMirror. It would also be useful for adding things like the hover toolbar that Notion has, where it needs to be positioned relative to one of slates elements.
Screen Shot 2020-01-07 at 11 42 09 am

Let me know if I'm just missing a key concept that makes this doable. Otherwise I'm happy to contribute a PR, I would just need a little guidance in terms of scoping the issue - i.e. what are all the things I haven't considered.

@cvlmtg
Copy link
Contributor

cvlmtg commented Jan 14, 2020

When you try to render your custom node inside a slate editable, are you wrapping them inside a <span {...attributes}> ? The only problem I had with focus and events was solved by correctly wrapping my nodes. If that doesn't work I suppose you'll need to provide some example code to show what you are trying to obtain

@BetaAI
Copy link

BetaAI commented Jan 15, 2020

I think the problem is with custom elements, and assumptions made by slate-react about text DOM nodes. It seems that ReactEditor.toSlatePoint ensures that a DOMPoint it receives is a text DOMPoint, but assumes that all text DOM nodes inside editor are under node with data-slate-node="text" attribute (react-editor.ts lines 380, 406). Since element render is called before leaves, this assumption does not hold for a custom element that adds its own text.

Here is minimal repro:
https://codesandbox.io/s/slate-reproductions-t32n1
Clicking/Selecting bold text will produce an exception in the console.

@LiuJi-Jim
Copy link

LiuJi-Jim commented Jan 17, 2020

I need this too.
I'm implementing <CodeBlockElement> with CodeMIrror, but slate swallowed all input events and key events so that CodeMirror received nothing.
Same thing happened when I'm building a <LinkElement> which opens a relatively positioned <input> to edit its href, something like:

<a href={element.href} {...attributes}>
  <span onClick={showEditor}>{children}</span>
  <HoverCard show={editing}>
    <input type="text" ......... />
  </HoverCard>
</a>

I thinks this is caused by slate handling all beforeinput events and in most of cases, it calls preventDefault (see onDOMBeforeInput function in editable.tsx).

Currently I use the same hack just like #3426 said. adddin data-slate-editor to my custom elements' editing UI.

@Francesco-Lanciana
Copy link
Author

I found another feature request from a while ago (#2072) that wanted to introduce an Island component to essentially achieve this isolated behaviour. The conversation seems to have dropped off but it bought up a few good things to be aware of/factor in.

In regards to a possible implementation, from what I can see in the codebase the Editable component has a method called hasEditableTarget. This is called as a check before a number of the event handlers (onDOMBeforeInput, OnKeyDown, ect). This method then calls ReactEditor.hasDOMNode which checks if one of the ancestors of the element in question is the editor element. I could see an Island/Isolate component just wrapping a component with a div that has an attribute data-isolate, and we add another check into ReactEditor.hasDOMNode that checks if an ancestor has this attribute. If it does than it returns false and the check fails, thereby telling Slate not to worry about this node.

This is still definitely overlooking some of the issues bought up in #2072 such as what happens to the focus when you click on this isolate component. I'm going to put in some time next week into seeing if I can get something working. I would also really like to explore what can be done to mitigate a few other issues I have discovered since integrating Slate and CodeMirror:

  1. Having arrow keys move focus from codemirror to slate and back is cumbersome
  2. Deleting CodeMirror and transferring focus back to the correct Slate node is also a little annoying (less so than 1 though)
  3. The with-history addon works with CodeMirror as long as you keep the state as a property on the element, however each letter is a seperate operation and they aren't merged like normal node operations which means you have to undo/redo individual letters. Would be nice if we could let Slate know which operations should be merged.
  4. Somewhat similar to @BetaAI's point, the code element still needs to render a child text node otherwise Slate complains when you try to remove the node. Isolated nodes shouldn't need children.

@LiuJi-Jim
Copy link

4. Somewhat similar to @BetaAI's point, the code element still needs to render a child text node otherwise Slate complains when you try to remove the node. Isolated nodes shouldn't need children.

I Agree. @Francesco-Lanciana
I think any void nodes e.g. <ImageElement> shouldn't need children. But so far we still need to render {children} even if it's useless to the render results.
Maybe this limitation came from slate's caret and selection locating algorithm, which somehow relies on data-slate-string and data-slate-leaf.

@ryanmitts
Copy link
Contributor

ryanmitts commented Jan 18, 2020

#3389 and #3436 will allow this behavior to work.

My bad, I think this ticket is asking for more than just void nodes that aren't interfered with by the event handlers.

@Francesco-Lanciana
Copy link
Author

@ryanmitts I had a look at your changes and it seems to me like they at least allow for elements to exist within slate that can steal focus from Slate and not cause an error to occur, which is a big step forward.

The main other thing that needs to be addressed is how we can allow for isolated nodes that don't need to contain a child text node. Since this is one of the underlying assumptions behind Slate it probably needs a bit of thought however, especially if we want to give an API that allows for focus to be transferred between the isolated element and Slate in a simple manner. My thoughts so far is that this needs to be a new concept that is seperate to a void element since they are aiming to do two seperate things.

@ryanmitts
Copy link
Contributor

@Francesco-Lanciana That makes sense, my biggest struggle is I don’t really know what a void node is supposed to behave like fully because it’s not really documented and the behavior is different between pre .50 and after (like why would a void Node need text).

My main intention is to be able to recreate what I have in the older Slate version in the newer. That probably gives me a bias to how void nodes behave.

I’d like to think about an implementation that accounts with maybe some other concept, I guess I might need some help comparing and contrasting void nodes vs. this new concept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants