Bug
Currently, when you drop a file on a void node, nothing happens. This is because the code here returns early in the onDrop function in content. It seems like the isInEditor function was designed to return false on void nodes or their content.
We should be able to drop a file on the void node, and have the event continue as on a non-void node.
I'm not sure though if:
isInEditor to NOT return false on void nodes or their content.@ianstormtaylor Realized that the issue above didn't make the questions clearer. So, I've updated the issue above with clearer API questions for you. I could then take a stab at resolving this.
@oyeanuj good find. I think an easy way to solve this might be to make it so that we have two different methods to use:
isInEditor which is the current one without the editable check.isEditableInEditor which calls to isInEditor and then adds the editable check too.And change all the current use cases to use the new one, with the onDrop one not caring about whether the node is editable.
Are you down to make that change? Thanks!
Sounds good, I'll take a shot at it! This would also remove the need for checking for void nodes specifically.
Hi @oyeanuj, @ianstormtaylor.
Let me add my opinion.
I think the principle void nodes are based on, is their content should be outside the control of Slate.
In particular they should by themselves implement user interaction logic.
According to that, in this case, maybe, a better solution is to add onDrop handler on the component used to render the void node.
I think @AlbertHilb has a very good point. We've actually given the control of onDrop to our void nodes without even thinking about it. Until now, I haven't even thought about whose responsibility that should be. It has just felt natural to let the void nodes control its content.
Not saying this is always the case. Other people might use, or view, it differently. Just thought I'd share my thoughts since we're doing quite a lot of dnd inside the editor.
@AlbertHilb @tobiasandersen interesting. Personally, I have always thought of void nodes as being part of the editor that isn't editable more than them being completely untied to the editor. But I can see where you are coming from.
If you look at #1027, you'll notice that today void nodes (including inline voids) are behaving quite differently than other nodes when it comes to cut, copy, drag, paste etc. So the change that I've made is making that consistent. Without that, I think it leads to some UX issues by default, where some part of the editor behaves completely different from other parts.
Having said that, I do wonder if we need an overwrite mechanism when a user wants to control all behavior of void nodes themselves.
Hi @oyeanuj.
Let me explain more in details my opinion.
From the point of view of Slate editor, void nodes are nodes without internal structure, atoms.
To be clear, essentially, they should behave like a single character.
So, all the actions concerning the internal structure of the node - like onDrop, but also onCopy, onCut, onPaste, etc - should be handled directly by the component used to render it.
Even because the result of such an action can well depend on the precise point, inside the rendered content, where it takes place: consider, for example, a void node containing two or more HTML input elements.
On the other side, actions dealing with the node as a whole - pressing delete key when the caret is on the left of the node, for example - can be handled by the Slate editor itself.
@AlbertHilb I agree with void nodes behaving like a single element insofar as the editor is concerned.
So, with the delete key, the editor deletes the node by default, but we can overwrite it with rules. But in the case of onDrop, onCut, onPaste, there is no default functionality leaving it to userland to reimplement all that logic.
So, I wonder - should we not follow the delete pattern, and have a default which can then be overwritten?
I think this is possible now, because onDrop is exposed to plugins even from void nodes.
Most helpful comment
Sounds good, I'll take a shot at it! This would also remove the need for checking for void nodes specifically.