Fix paste to empty node losing structure of first block

[nemanja-tosic]

Description

When pasting into an empty block, insert the entire fragment as-is, thereby preserving the structure of the first block.

Issue

Fixes: #4486

Example

Right - current, left - PR.

Caveat

Since the first node is not handled in a special way anymore, there will be a change to how complex structures like lists behave (GIF below). I feel that slate cannot handle this well on it's own without knowing the semantics of those elements (is it a list, is it a table, how does one paste a table into a table etc), and it should be up to the "plugin" that defines what a list is to define that behavior. If this is not acceptable, i propose we allow the clients to choose between two insert strategies, as we really need the PR behavior to work and list behavior for us is solved by plate.

Note however that slate handles pasting to a list (a flat list too) nicely, whereas pasting outside of a list is not good, whereas the PR is the opposite - pasting outside the list looks good, whereas pasting into a list introduces a sublist.

Right - current, left - PR.

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: 86620e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[TheSpyder]

I'm not sure special casing this to empty blocks is a good idea and probably leads to some of the problems you've found. Instead, it should be based on what is being inserted.

My recommendation is:

• Adjust the match loop such that merge is disabled if the inserted fragment has more than one top-level block node (this is what I described in the Slack thread)
  • for example check if the fragment length is >= 2 and child 0 is a block node
• Keep the code that cleans up if the cursor was in an empty block and multiple blocks were inserted

This, in my experience, matches user expectation quite well.

[nemanja-tosic]

I'm not sure special casing this to empty blocks is a good idea and probably leads to some of the problems you've found. Instead, it should be based on what is being inserted.

I've found problems either way. In the example below on production, pasting two paragraphs breaks the example list by introducing an incorrect structure (ul > blockquote). This PR does not address this, but the example demonstrates that you probably can't fix all issues in insertFragment - the list (or in the abstract, the concrete block) implementation should handle paste as to not break it's structure (even if you were to be more smart on copy, you would have two blocks in your fragment here, so it becomes a paste thing). Since, then, you should introduce new behavior (normalization, overrides...) for lists anyway, this PR does not introduce a new issue to solve. QED (could not resist 😃)

Adjust the match loop such that merge is disabled if the inserted fragment has more than one top-level block node (this is what I described in the Slack thread)

This PR is trying to address a scenario where copying a heading (one top level block in the fragment) was getting inserted as plain text because it is the first node in the fragment. The only way I've found to copy a heading block is by copying something above it as well. The suggestion doesn't change that - we would still need to copy two things in order to paste the heading. I guess this is a preference so it's difficult to argue, but it's awkward.

A reasonable alternative, possibly the most robust solution, would be to express intent to copy a block or just the innards of a block when copying (notion does this i believe). If we were to go down that route, this PR would be rendered obsolete but there would be no change in behavior for empty blocks as the fragment would be pasted as-is, therefore this could be treated as an interim solution. IOW the PR assumes pasting into an empty node means "paste the fragment as-is" (plugins not taken into account, they would need overrides either way probably).

for example check if the fragment length is >= 2 and child 0 is a block node

Is it possible to copy a fragment with mixed blocks and inlines? I'm unable to make this happen. Also, the docs state: "But elements cannot contain some children that are blocks and some that are inlined.".

Keep the code that cleans up if the cursor was in an empty block and multiple blocks were inserted

Didn't understand this.

Thanks for the review!

[TheSpyder]

This PR is trying to address a scenario where copying a heading (one top level block in the fragment) was getting inserted as plain text because it is the first node in the fragment. The only way I've found to copy a heading block is by copying something above it as well. The suggestion doesn't change that - we would still need to copy two things in order to paste the heading. I guess this is a preference so it's difficult to argue, but it's awkward.

Ah I didn't realise you were trying to achieve that. I don't think copying a word out of a heading and pasting it into a paragraph (even an empty one) should insert a heading. Copying a heading and some text below it, though, that's clearly trying to insert structure.

A reasonable alternative, possibly the most robust solution, would be to express intent to copy a block

That might be ok for copying within Slate, but not pasting external content.

for example check if the fragment length is >= 2 and child 0 is a block node

Is it possible to copy a fragment with mixed blocks and inlines? I'm unable to make this happen. Also, the docs state: "But elements cannot contain some children that are blocks and some that are inlined.".

It isn't possible, which is the point I was trying to leverage in my suggestion to use fragment details. If there is more than one node in the fragment and the first is a block node they must all be block nodes. That's how I recommend detecting whether the user intended to paste as blocks or just block contents.

Keep the code that cleans up if the cursor was in an empty block and multiple blocks were inserted

Didn't understand this.

I meant if you implement my suggestion, keep this code, which deletes the block if it was empty and nothing was merged into it (although thinking about it now starts.length is a better indication of that than middles.length).

[nemanja-tosic]

Copying a heading and some text below it, though, that's clearly trying to insert structure.

As a user, I want to copy a heading and paste it in this scenario, not the heading and some text above it, or any text at all. Without the copy handler change, how do i tell slate this?

With the current implementation, i can't manually create a fragment that would insert the heading as a heading without hacks as far as i can tell, i need to instead override insertFragment. insertFragment does not have any variation points in it, so if i override it i can only copy/paste the entire thing and keep it up to date with slate.

That might be ok for copying within Slate, but not pasting external content.

External content would default to some behavior and maybe have some undesired effects depending on how it's structured, but presumably C/P from slate to slate should be a first-class citizen as we are in control of the selection at that point as opposed to an external source, IOW we can make a decision based on intent, rather than content.

That said, if i wanted to insert a fragment consisting of a single block from an external source to the end of a document (in this case an empty node), why would slate assume i wanted to insert the first node as text?

It isn't possible, which is the point I was trying to leverage in my suggestion to use fragment details. If there is more than one node in the fragment and the first is a block node they must all be block nodes. That's how I recommend detecting whether the user intended to paste as blocks or just block contents.

But whenever i copy from slate, the fragment is always the tree from root to selection (block > inline). Can you show me an example of copying where the fragment consist of inlines?

[TheSpyder]

Copying a heading and some text below it, though, that's clearly trying to insert structure.

As a user, I want to copy a heading and paste it in this scenario, not the heading and some text above it, or any text at all.

Sometimes yes, sometimes no. I can understand that you as a user would like to do this as the default. I'm trying to say that in my experience most users don't.

With the current implementation, i can't manually create a fragment that would insert the heading as a heading without hacks as far as i can tell, i need to instead override insertFragment. insertFragment does not have any variation points in it, so if i override it i can only copy/paste the entire thing and keep it up to date with slate.

If we're talking manual override, use insertNodes. This is the insertFragment method, in the text transform module, and I believe it should default to inserting text (perhaps with special cases as I've described).

That might be ok for copying within Slate, but not pasting external content.

External content would default to some behavior and maybe have some undesired effects depending on how it's structured, but presumably C/P from slate to slate should be a first-class citizen as we are in control of the selection at that point as opposed to an external source, IOW we can make a decision based on intent, rather than content.

Then slate-react can handle that at the clipboard paste level, with perhaps some special flag to indicate it's a within-editor clipboard event. That sort of thing shouldn't impact methods in slate core.

That said, if i wanted to insert a fragment consisting of a single block from an external source to the end of a document (in this case an empty node), why would slate assume i wanted to insert the first node as text?

Inserting into an empty node is a reasonable case to override the merge. I haven't been clear about that here, I think I only mentioned it on a related Slack thread talking about #4486. My apologies.

Your approach isn't totally wrong, but I don't believe it solves the general case issue described in #4486.

It isn't possible, which is the point I was trying to leverage in my suggestion to use fragment details. If there is more than one node in the fragment and the first is a block node they must all be block nodes. That's how I recommend detecting whether the user intended to paste as blocks or just block contents.

But whenever i copy from slate, the fragment is always the tree from root to selection (block > inline). Can you show me an example of copying where the fragment consist of inlines?

From Slate? No. But external content, or manually crafted API calls, could easily do it.

[dylans]

Copying a heading and some text below it, though, that's clearly trying to insert structure.

As a user, I want to copy a heading and paste it in this scenario, not the heading and some text above it, or any text at all.

Sometimes yes, sometimes no. I can understand that you as a user would like to do this as the default. I'm trying to say that in my experience most users don't.

The current behavior is the source of many bug reports and complaints from end users. I found at least 5 issues that were related (losing the first block of what gets pasted, getting converted into text instead).

With the current implementation, i can't manually create a fragment that would insert the heading as a heading without hacks as far as i can tell, i need to instead override insertFragment. insertFragment does not have any variation points in it, so if i override it i can only copy/paste the entire thing and keep it up to date with slate.

If we're talking manual override, use insertNodes. This is the insertFragment method, in the text transform module, and I believe it should default to inserting text (perhaps with special cases as I've described).

Just because it lives in the text transform doesn't mean insertFragment doesn't do much more than that. I'm not clear about the suggestion of using insertNodes when insertFragment is so closely connected to the copy/paste logic.

That might be ok for copying within Slate, but not pasting external content.

External content would default to some behavior and maybe have some undesired effects depending on how it's structured, but presumably C/P from slate to slate should be a first-class citizen as we are in control of the selection at that point as opposed to an external source, IOW we can make a decision based on intent, rather than content.

Then slate-react can handle that at the clipboard paste level, with perhaps some special flag to indicate it's a within-editor clipboard event. That sort of thing shouldn't impact methods in slate core.

By this argument a lot of the current insertFragment logic doesn't belong in slate core then.

In terms of the proposed change, to me it feels like the smallest, least impactful change that is consistent with the current codebase. It's a long standing issue that's frustrating to a lot of users. If we want to refactor and change the way this works to be purer, that's fine, but I wouldn't block the PR for that given the current state of things.

That said, if i wanted to insert a fragment consisting of a single block from an external source to the end of a document (in this case an empty node), why would slate assume i wanted to insert the first node as text?

Inserting into an empty node is a reasonable case to override the merge. I haven't been clear about that here, I think I only mentioned it on a related Slack thread talking about #4486. My apologies.

The relevant comment from slack for posterity was:

Perhaps it needs to be configurable? Currently my team's implementation completely avoids Slate's insertFragment method. I don't recall exactly but I can imagine the reason for this is less about specific concerns with the method and more that we have a fairly detailed logic flow we're trying to replicate.
The way we handle this in TinyMCE normally is the "insert block contents as text" logic only applies if there is one block, rather than any time both start and end are blocks. But there is nuance to this, because if inserting multiple blocks into an empty block the best course of action is to replace the empty block with the inserted one. Our Slate implementation replicates most of this, but it doesn't yet remove the empty block in the second case.

Your approach isn't totally wrong, but I don't believe it solves the general case issue described in #4486.

Possibly, though I would argue that it's significantly better as a default than what we're currently shipping. :)

It isn't possible, which is the point I was trying to leverage in my suggestion to use fragment details. If there is more than one node in the fragment and the first is a block node they must all be block nodes. That's how I recommend detecting whether the user intended to paste as blocks or just block contents.

But whenever i copy from slate, the fragment is always the tree from root to selection (block > inline). Can you show me an example of copying where the fragment consist of inlines?

From Slate? No. But external content, or manually crafted API calls, could easily do it.

Since we have no control at the point of copy of external content, the only point at which we can handle it is paste. When external content is pasted it won't be slate ast, but either html, markdown, or plain text presumably. This change doesn't remove the need for a paste deserializer. Or am I missing something?

In terms of manually crafted API calls, wouldn't it be better to suggest that such API calls should provide a valid slate tree or are there use cases I've not considered?

[TheSpyder]

Sometimes yes, sometimes no. I can understand that you as a user would like to do this as the default. I'm trying to say that in my experience most users don't.

The current behavior is the source of many bug reports and complaints from end users. I found at least 5 issues that were related (losing the first block of what gets pasted, getting converted into text instead).

Solving the problem of losing the first block in a multi-block paste is exactly the fix I'm suggesting. This PR doesn't fix that except in one specific 'paste into empty block' case.

If we're talking manual override, use insertNodes. This is the insertFragment method, in the text transform module, and I believe it should default to inserting text (perhaps with special cases as I've described).

Just because it lives in the text transform doesn't mean insertFragment doesn't do much more than that. I'm not clear about the suggestion of using insertNodes when insertFragment is so closely connected to the copy/paste logic.

Insert Fragment is useful for far more than just pasting. I can appreciate that slate-react only uses it for that but Slate is used in a wide range of places.

I offered insertNodes because @nemanja-tosic was talking about copy/pasting the entire insertFragment to make it work a different way. Insert Nodes already works this way.

Then slate-react can handle that at the clipboard paste level, with perhaps some special flag to indicate it's a within-editor clipboard event. That sort of thing shouldn't impact methods in slate core.

By this argument a lot of the current insertFragment logic doesn't belong in slate core then.

It's useful for the intended purpose, which is partly why in TinyMCE we wrote our own implementation that mostly follows what I've suggested (I suspect we rewrote it instead of PR it because I wasn't a contributor at the time).

In terms of the proposed change, to me it feels like the smallest, least impactful change that is consistent with the current codebase. It's a long standing issue that's frustrating to a lot of users. If we want to refactor and change the way this works to be purer, that's fine, but I wouldn't block the PR for that given the current state of things.

I'm not blocking the PR. It's approved and I haven't requested changes. But it doesn't completely fix the issue it claims to, as it only applies when pasting into empty blocks. I offered a suggestion that is probably only a small tweak and would cover the general case.

Your approach isn't totally wrong, but I don't believe it solves the general case issue described in #4486.

Possibly, though I would argue that it's significantly better as a default than what we're currently shipping. :)

That's fine, but it's marked as "will close 4486" :)

But whenever i copy from slate, the fragment is always the tree from root to selection (block > inline). Can you show me an example of copying where the fragment consist of inlines?

From Slate? No. But external content, or manually crafted API calls, could easily do it.

Since we have no control at the point of copy of external content, the only point at which we can handle it is paste. When external content is pasted it won't be slate ast, but either html, markdown, or plain text presumably. This change doesn't remove the need for a paste deserializer. Or am I missing something?

I'm not sure how that's relevant to insertFragment. The details of paste management happen in the react code.

In terms of manually crafted API calls, wouldn't it be better to suggest that such API calls should provide a valid slate tree or are there use cases I've not considered?

I'm sure the code assumes the inserted fragment provides a valid slate tree. But pre-normalising it might only cause more issues.

[dylans]

Sometimes yes, sometimes no. I can understand that you as a user would like to do this as the default. I'm trying to say that in my experience most users don't.

The current behavior is the source of many bug reports and complaints from end users. I found at least 5 issues that were related (losing the first block of what gets pasted, getting converted into text instead).

Solving the problem of losing the first block in a multi-block paste is exactly the fix I'm suggesting. This PR doesn't fix that except in one specific 'paste into empty block' case.

I think part of the current behavior is that if you paste into a non-empty block, the intent actually is that you want to merge the content of the first pasted block into the block. If you didn't want to merge it in, you'd paint into an empty block. At least that's my understanding/expectation and it seems to be how many other editors (e.g. Google Docs) behaves.

The current intent that feels wrong is pasting into an empty block which is what was addressed.

If we're talking manual override, use insertNodes. This is the insertFragment method, in the text transform module, and I believe it should default to inserting text (perhaps with special cases as I've described).

Just because it lives in the text transform doesn't mean insertFragment doesn't do much more than that. I'm not clear about the suggestion of using insertNodes when insertFragment is so closely connected to the copy/paste logic.

Insert Fragment is useful for far more than just pasting. I can appreciate that slate-react only uses it for that but Slate is used in a wide range of places.

I offered insertNodes because @nemanja-tosic was talking about copy/pasting the entire insertFragment to make it work a different way. Insert Nodes already works this way.

ok.

Then slate-react can handle that at the clipboard paste level, with perhaps some special flag to indicate it's a within-editor clipboard event. That sort of thing shouldn't impact methods in slate core.

By this argument a lot of the current insertFragment logic doesn't belong in slate core then.

It's useful for the intended purpose, which is partly why in TinyMCE we wrote our own implementation that mostly follows what I've suggested (I suspect we rewrote it instead of PR it because I wasn't a contributor at the time).

In terms of the proposed change, to me it feels like the smallest, least impactful change that is consistent with the current codebase. It's a long standing issue that's frustrating to a lot of users. If we want to refactor and change the way this works to be purer, that's fine, but I wouldn't block the PR for that given the current state of things.

I'm not blocking the PR. It's approved and I haven't requested changes. But it doesn't completely fix the issue it claims to, as it only applies when pasting into empty blocks. I offered a suggestion that is probably only a small tweak and would cover the general case.

ok... after exploring this for a few days, I no longer believe that any small tweak for this block of code is a small undertaking. 😂

Your approach isn't totally wrong, but I don't believe it solves the general case issue described in #4486.

Possibly, though I would argue that it's significantly better as a default than what we're currently shipping. :)

That's fine, but it's marked as "will close 4486" :)

But whenever i copy from slate, the fragment is always the tree from root to selection (block > inline). Can you show me an example of copying where the fragment consist of inlines?

From Slate? No. But external content, or manually crafted API calls, could easily do it.

Since we have no control at the point of copy of external content, the only point at which we can handle it is paste. When external content is pasted it won't be slate ast, but either html, markdown, or plain text presumably. This change doesn't remove the need for a paste deserializer. Or am I missing something?

I'm not sure how that's relevant to insertFragment. The details of paste management happen in the react code.

In terms of manually crafted API calls, wouldn't it be better to suggest that such API calls should provide a valid slate tree or are there use cases I've not considered?

I'm sure the code assumes the inserted fragment provides a valid slate tree. But pre-normalising it might only cause more issues.

ok

[nemanja-tosic]

I have asked how i can achieve copy/paste of a heading as a heading, where i copy just the heading. Multi-block specialization does not fix this, so what do you suggest given the following constraints:

As a user, I want to copy a heading and paste it in this scenario, not the heading and some text above it, or any text at all.

Slate, as far as i can tell cannot do this without overriding insertFragment, or can it?

[TheSpyder]

Solving the problem of losing the first block in a multi-block paste is exactly the fix I'm suggesting. This PR doesn't fix that except in one specific 'paste into empty block' case.

I think part of the current behavior is that if you paste into a non-empty block, the intent actually is that you want to merge the content of the first pasted block into the block. If you didn't want to merge it in, you'd paint into an empty block. At least that's my understanding/expectation and it seems to be how many other editors (e.g. Google Docs) behaves.

Google docs isn't a great example, because it's not a HTML editor. But that's getting a bit off topic.

I think if multiple blocks are pasted, regardless of where they are pasted, the expectation is to not merge the first block. There are certainly examples of both merge and not merge across the editor market.

The current intent that feels wrong is pasting into an empty block which is what was addressed.

So merge this and we'll make a new ticket if you think 4486 is fixed. I might even see if my team would be interested in a PR to integrate our custom insertFragment multi-block behaviour.

ok... after exploring this for a few days, I no longer believe that any small tweak for this block of code is a small undertaking. 😂

It's generated a lot of discussion, because I bring 20 years of experience in creating rich text editors 🤭 but it's not a lot of code. I'm just doing a bad job of explaining what I think we need and why.

@nemanja-tosic

I have asked how i can achieve copy/paste of a heading as a heading, where i copy just the heading. Multi-block specialization does not fix this, so what do you suggest given the following constraints:

As a user, I want to copy a heading and paste it in this scenario, not the heading and some text above it, or any text at all.

Slate, as far as i can tell cannot do this without overriding insertFragment, or can it?

Your PR does this as long as you abide by the restriction of pasting into an empty block. I know I started this mess by suggesting it not do that, but we've gone around in circles and I don't want to drag the discussion on any further.

[dylans]

Solving the problem of losing the first block in a multi-block paste is exactly the fix I'm suggesting. This PR doesn't fix that except in one specific 'paste into empty block' case.

I think part of the current behavior is that if you paste into a non-empty block, the intent actually is that you want to merge the content of the first pasted block into the block. If you didn't want to merge it in, you'd paint into an empty block. At least that's my understanding/expectation and it seems to be how many other editors (e.g. Google Docs) behaves.

Google docs isn't a great example, because it's not a HTML editor. But that's getting a bit off topic.

Ha, fair enough.

I think if multiple blocks are pasted, regardless of where they are pasted, the expectation is to not merge the first block. There are certainly examples of both merge and not merge across the editor market.

I think that was my original question. It feels like slate pretty deliberately made the choice to merge the first block (otherwise you could paste into an empty block below it). There's logic that seems to try to handle this. Originally I was thinking that maybe you only merge if the block is of the same type you're merging into (e.g. merge a list item into a list item).

My assumption is it was implemented as a merge with intent, but the insert into an empty block scenario wasn't considered given the lack of tests for it compared to the other scenarios.

The current intent that feels wrong is pasting into an empty block which is what was addressed.

So merge this and we'll make a new ticket if you think 4486 is fixed. I might even see if my team would be interested in a PR to integrate our custom insertFragment multi-block behaviour.

cool.

ok... after exploring this for a few days, I no longer believe that any small tweak for this block of code is a small undertaking. 😂

It's generated a lot of discussion, because I bring 20 years of experience in creating rich text editors 🤭 but it's not a lot of code. I'm just doing a bad job of explaining what I think we need and why.

It's funny, I worked on RTE in a browser from 2000-2003, felt like it was an exercise in futility and hoped to avoid it forever (other than some minimal work I did along the way on the Dojo editor). But here I am, 20 years later working on it again. :)

@nemanja-tosic

I have asked how i can achieve copy/paste of a heading as a heading, where i copy just the heading. Multi-block specialization does not fix this, so what do you suggest given the following constraints:

As a user, I want to copy a heading and paste it in this scenario, not the heading and some text above it, or any text at all.

Slate, as far as i can tell cannot do this without overriding insertFragment, or can it?

Your PR does this as long as you abide by the restriction of pasting into an empty block. I know I started this mess by suggesting it not do that, but we've gone around in circles and I don't want to drag the discussion on any further.

Fair enough. I think we'll land it later today and see what else can be done to improve things, and go from there. Thanks for making the time.