Fix for wrong deletion of mixed text and table content

[msamsel]

What is the purpose of this pull request?

Bug fix

This PR contains

• Unit tests
• Manual tests

What changes did you make?

• use editor.extractSelectedHtml instead of current logic to remove content from the editor after pressing Backspace or Delete
• correct bunch of test
• Unfortunately this solution has 2 minor things:
  • nested tables are still wrongly processed what is caused by this issue: extractSelectedHtml wrongly remove nested tables #1880
  • identical elements might be merged Provide possibility to not merge identical ancestor elements after text removing #1881 (because of that test #15 and #16 are changed)

Close #541

[Comandeer]

The fix is working, but is not coherent with native Firefox behavior. Firefox, in case of the second table, move text from/into table:

Not to mention IE/Edge that does not do anything at all ¯_(ツ)_/¯

There is also one more problem: failing test after the changes → http://tests.ckeditor.test:1030/tests/core/editable/keystrokes/keystrokes#tests%2Fcore%2Feditable%2Fkeystrokes%2Fkeystrokes%20test%20handle%20del%20with%20full%20table%20content%20selected

There is also one thing that makes me wonder: why didn't we use editor.extractSelectedHtml at first place? 🤔

[Comandeer]

I'm quite sure that when startBlock equals endBlock, then selection is contained inside one block.

[Comandeer]

The comment condradicts the code. false is returned if selectionInTwoDifferentBlocks is true, while comment stands the opposite.

[Comandeer]

That line of code became ugly. Let's separate if from variable declaration.

[mlewand]

Firefox

I had a look with @msamsel during a f2f talk and the Firefox thing looks clearly like their feature (if you have selected same element on both ends, here paragraph, and delete the content then merge it). We should not worry about that at this point.

Edge / IE

As of Edge it is clearly an upstream issue in Edge/IE. Normally I'd say that all we need to do here is report upstream issue and not worry about it, but it will look kind of silly if we say in changelog that we fixed this case, but it does not work in Edge / IE.

With above in mind, please:

• Create a feature request to add handling for IE / Edge.
• Report an upstream issue, and link it properly.

So this PR should not worry about IE/Edge support, that will be done in a follow-up PR.

[msamsel]

There is also one thing that makes me wonder: why didn't we use editor.extractSelectedHtml at first place?

I think because editor.extractSelectedHtml was developed 3 years ago, and this corrected part was developed 4 years ago ;P

Edge case was extracted to: #1919
Firefox was reported as inconsistent behaviour https://bugzilla.mozilla.org/show_bug.cgi?id=1456427

Please also remember that main point of this fix was to prevent of removing entire table cells from tables and inserting <p> tags under <tr> instead of <td> in WebKits browsers. Jumping content from paragraph to table cell and vice versa are different case and wasn't covered in issue description.

Another thing is correct behaviour. Jumping content from paragraph below table to its cell seems to be inappropriate. That's why paragraphs remain under table.

[Comandeer]

Seems like defining correct behavior is not trivial… I've create native [contenteditable] demo, which shows following things:

Chrome/Safari

1. When deleting from the beginning of the table, content outside the table is not merged with the content from the table.
2. When deleting from the end of the table, content outside is merged.
3. When overwriting with some letter, the content is merged for selection at the end of the table.

Firefox

1. When deleting from table without paragraphs, content outside the table is not merged into it.
2. When deleting from table with paragraphs, content outside the table is merged into it.
3. When overwriting with some letter, the content is merged in the second table.

So even in Chrome/Safari we end up with inconsistent behavior, as disallowing merge after deleting has totally different behavior than overwriting with new content. IMO it's an issue that should be addressed.

[msamsel]

@mlewand could you provide feedback what behaviour should be implemented in CKE4?

[mlewand]

@msamsel @Comandeer There's nothing wrong with unifying the results across the browser.

When deleting from the beginning of the table, content outside the table is not merged with the content from the table.

What is exactly the issue? I have used delbackspacequirks/manual/tables to repro it an Chrome, and it works the same for me in both cases.

---

When deleting from the end of the table, content outside is merged.

I find it to be a positive change not to merge the content in that case.

---

When overwriting with some letter, the content is merged for selection at the end of the table.

Same as above.

---

As for Firefox points, is the original issue related to Firefox? Or is the current PR changing Firefox behavior anyhow?

[mlewand]

Note that you could also give a try with document.execCommand approach, e.g. document.execComand( 'delete' ). Although, for me, it should be exact sam logic as pressing the delete key, after brief checking it looks like Chrome implements actually a different logic there.

[mlewand]

After f2f talk with @msamsel seems that delete command looks promising, the problem is that range.scrollIntoView() breaks the selection if anchored in a text (thus #1956). @msamsel as for fighting the dissapearing selection temporarily, could you try to make bookmark? That should help mitigate the issue.