RichText: Revise onSplit prop

[ellatrix]

Description

Fixes #10761, see the issue for more information. Also fixes:

• Using Enter to split a paragraph block requires two Ctrl-Z's to undo - closes Using Enter to split a paragraph block requires two Ctrl-Z's to undo #8882
• Pasting should bring you to end of content, not beginning - closes Pasting should bring you to end of content, not beginning #5317
• Pressing enter at the start of a heading block should push it down and create a paragraph above - closes Pressing enter at the start of a heading block should push it down and create a paragraph above. #10326

This not only fixes the issues above, but also fixes a bunch of issues around paste because some core blocks do not take into account any null values passed. It's therefore better to built this into RichText.

It also fixes an issue with lists instances where pasted content would split the block. Since these two are now separate handlers, pasted content will now appear as list items inside lists.

How has this been tested?

Ensure pasting and splitting inside RichText works.

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[ellatrix]

I think the only way to deprecate this is to use another name, and deprecate onSplit. Maybe onInsertAfter?

[ellatrix]

To be honest, I would be fine just logging a message if we detect the onSplit prop but take no further action. This seems to be an advanced feature that even only use by four of our core blocks. At most splitting will stop working and can be easily reimplemented.

[youknowriad]

Maybe just a deprecated call when onSplit is used. (Doesn't have to be working)

I'd appreciate an opinion from @aduth / @mcsf
What are the impacts on undo/redo. Having a single callback has the advantage of allowing us to group the changes in one action (it's not the case right now I think)

[ellatrix]

What are the impacts on undo/redo. Having a single callback has the advantage of allowing us to group the changes in one action (it's not the case right now I think)

Before there were multiple actions as well, but worth investigation more. Before it was insert after + update attributes. Now there may be two insert after calls.

[ellatrix]

I just checked, and in master undoing splitting e.g. a paragraph is also a problem. It will undo setting the attributes of the original block, then remove the inserted block. I think resolving this issue is outside the scope of this PR.

[jorgefilipecosta]

Hi @iseulde the code looks good, and it would be helpful to provide block developers with this simplification. I did some testing, and I noticed a regression when comparing to master. On the master branch, if we paste an image block in the middle of a list the list block gets divided, and the image block gets inserted in the middle (the same behavior as google docs). Here the image block is discarded, and a new list empty list item is inserted.

Regarding the undo problems it is a fact that the problem already exists in master and it looks out of the scope of this PR. But I think it is helpful to analyse the implications of this change on a potential solution for undo problems during the split operation.
On the previous approach to onSplit, we had a function that was called with the before and after content, so we could solve the undo problem by calling onReplace (one undo operation) instead of updating the block content plus calling insert after (two operations). The PR #6964 that applied this change was not merged because we tried to pursue a general solution to the undo problems #8119, at the time the most promising path looked like a transaction system that would allow undoing multiple actions part of the same operation.

I'm not sure maybe @mcsf can confirm, but I think the approach to solve undo problem that we are trying to follow now is ignoring some attributes on undo and having temporary attributes for image blob urls, etc. If that is the case, and given that our new approach to onSplit is rich text updating the content plus the block handling the insert after, it may become impossible to make the split operation a single undo action, and we may be without a possible solution to the undo problem in the splitting case.

[ellatrix]

I did some testing, and I noticed a regression when comparing to master. On the master branch, if we paste an image block in the middle of a list the list block gets divided, and the image block gets inserted in the middle (the same behavior as google docs). Here the image block is discarded, and a new list empty list item is inserted.

This is intentional, it also adjusts paste for multiline values to normalise as lines of text. E.g. if you copy some paragraphs and paste in a list, it will be pasted as list items. I can remove this if it's not wanted.

[ellatrix]

If that is the case, and given that our new approach to onSplit is rich text updating the content plus the block handling the insert after, it may become impossible to make the split operation a single undo action, and we may be without a possible solution to the undo problem in the splitting case.

@jorgefilipecosta This is already the case in master. We update the RichText value and insert blocks after. So I don't see what the problem would be for an undo solution.

[jorgefilipecosta]

This is intentional, it also adjusts paste for multiline values to normalize as lines of text. E.g. if you copy some paragraphs and paste in a list, it will be pasted as list items. I can remove this if it's not wanted.

For example for text blocks, e.g: pasting and heading or quote and pasting the items as part of the list this behavior seems better. But when pasting media blocks like images or videos we totally discard the paste content I'm not sure if it is a blocker but if we could mix both behaviors split when pasting complex media blocks like images and add as list items when pasting test it would be perfect.

@jorgefilipecosta This is already the case in master. We update the RichText value and insert blocks after. So I don't see what the problem would be for an undo solution.

Yes, that is true on master the also do an update plus an insert after, but as I referred on master we can easily solve the problem, we can start calling onReplace instead of updating the first block plus calling insertAfter during the onSplit handler the blocks implement. Here we are forced to have two undo levels without an easy path to have just one undo level during the split operation as the updates are dispacthed from two different places, one inside Rich Text, and other one at the block level.
But there is hope, maybe the last approach to address undo problem can deal with this situation :)
@mcsf would it be possible to have a check and let us know if the approach being worked on for undo problems can address this kind of issues?

[ellatrix]

For example for text blocks, e.g: pasting and heading or quote and pasting the items as part of the list this behavior seems better. But when pasting media blocks like images or videos we totally discard the paste content I'm not sure if it is a blocker but if we could mix both behaviors split when pasting complex media blocks like images and add as list items when pasting test it would be perfect.

This is the same for e.g. pasting in an image caption. We discard everything. If you want all pasted content, you should paste in a the main content instead of a nested item. This seems expected to me. Pasting something in a list = I want to paste as list items.

[jorgefilipecosta]

This is the same for e.g. pasting in an image caption. We discard everything. If you want all pasted content, you should paste in a the main content instead of a nested item. This seems expected to me. Pasting something in a list = I want to paste as list items.

I understand this point of view and it is better than what we had for the list case. It felt strange at the start that the image was discarded but now I understand the reason and if a caption exists the text is used so I think it is an acceptable behavior, thank you for clarifying.

[ellatrix]

Removing milestone as parent issue is on it.

[gziolo]

What's the status of this one? I see some merge conflicts and no activity since December. I'm marking it as Stale to make triaging and reviewing easier. Should we apply In progress instead?

[ellatrix]

Needs to be rebased whenever I have time.

[ellatrix]

Closing in favour of #14765.