Ckeditor5: Pasting an element that cannot be inserted into table moves the selection

[pomek]

Is this a bug report or feature request? (choose one)

🐞 Bug report

💻 Version of CKEditor

Latest master.

📋 Steps to reproduce

1. http://localhost:8080/ckeditor5/latest/examples/builds/classic-editor.html (in your env. it can be a little different URL)
2. Insert a table (e.g. 3x3)
3. Paste somewhere a media: https://www.youtube.com/watch?v=ZVv7UMQPEWk
4. Copy the media.
5. Paste into a cell in the table.

✅ Expected result

The selection won't move.

❎ Actual result

Selection is moved to previous or next cell.

📃 Other details that might be useful

I could do it on Firefox and Chrome.

[f1ames]

☝️ I have noticed the same behaviour on Safari@iOS.

[Reinmar]

Still reproducible when pasting a table into a table.

[msamsel]

Currently I was able to find out the reason of such situation:

1. When selection is placed in empty table cell, insertContent has special side effect.
2. First there is try to insert table inside table cell
3. During this process there is removed empty paragraph from current selection to "cleanup" markup:
   https://github.com/ckeditor/ckeditor5-engine/blob/01ff6e6b5bcfbe0b7619e95b0b764e5e393fb6e6/src/model/utils/insertcontent.js#L556-L567
4. Then new range and selection is made:
   https://github.com/ckeditor/ckeditor5-engine/blob/01ff6e6b5bcfbe0b7619e95b0b764e5e393fb6e6/src/model/utils/insertcontent.js#L76
5. Right now previously selected table cell doesn't contain paragraph so we cannot move back selection to origin point. That's why there are search some nearest places where selection might be located:
   https://github.com/ckeditor/ckeditor5-engine/blob/01ff6e6b5bcfbe0b7619e95b0b764e5e393fb6e6/src/model/utils/insertcontent.js#L202-L208
6. This result with selecting previous or next table cell
7. And after that there is run post-fixer overt table, which restores removed paragraph.

[jodator]

~This method is so entangled and not so easy to follow. Now it seems that something is allowing table inside a table: https://github.com/ckeditor/ckeditor5-engine/blob/9a40550a7425670264974edc9252f3dcf888ce2f/src/model/utils/insertcontent.js#L544
So fixing this so it will return false for table in table would be enough to fix a bug.~

~Nevertheless I can see that next step is a bit too optimistic - I don't like that checkAndSplit does check and split. The other part I don't like is when we're entering this path even if paste results in no nodes added. But I think that this is a reason because the _getAllowedIn() returns some disallowed node.~

~In a case of $root > table > tableRow > tableCell > paragraph the _getAllowedIn() should return $root not the tableCell or anything else.~

OK after more fidling with this. Basically I see a flaw in while() loop: https://github.com/ckeditor/ckeditor5-engine/blob/9a40550a7425670264974edc9252f3dcf888ce2f/src/model/utils/insertcontent.js#L550

AFAICS it tries to break elements up to so limit element and it does it pretty well but fails when allowedElement is above limit element - like in this case.

We have:

          $root > table > tableRow > tableCell > paragraph
            ^                            ^
allowed: ---+                            |
limit:   --------------------------------+

In such scenario we shouldn't do paste as the schema disallow to paste there.

Also removing anything (special case but anyway) if we cannot paste is also wrong.

Probably it is safe to remove empty node if and only if it's parent is allowedIn (parent.isEmpty && parent.parent === allowedIn).

[jodator]

ps.: it looks like it fixes the issue, but it might need tests for previous special cases (most likely empty paragraph).

[msamsel]

Thx for suggestion. I've implement it and it seems to work fine.