Handle 'Backspace' block elements removal on Android

[f1ames]

Suggested merge commit message (convention)

Type: Handle Backspace block elements removal on Android. Closes ckeditor/ckeditor5/issues/1106. Closes ckeditor/ckeditor5/issues/1130.

---

Additional information

The main issues with the Backsapce on Android are described in ckeditor/ckeditor5#614 (comment) and ckeditor/ckeditor5#614 (comment) and they are the main reason why the solution is implemented in a way it is (of course apart from the fact that every key press has 229 key code on Android).

To sum up how the flow looks like in a general perspective:

1. Special injectAndroidBackspaceMutationsHandling is initialised inside Delete plugin.
2. It has selection change and mutations listener:
   • selection change listener stores previous and current selection every time it changes, it also stores timestamp of the latest change
   • mutations listener checks if given mutations are the block removal mutations and acts accordingly
3. On every block removal mutation selected content is removed. It might be current or previous selected content depending on selection state.

There is also one unrelated but similar issue - ckeditor/ckeditor5#1127

[coveralls]

Coverage remained the same at 100.0% when pulling e399808 on t/1106 into 6ffa9d5 on master.

[f1ames]

I have also added Closes ckeditor/ckeditor5#1130. The above solution fixes this issue, however there is a side effect of 2 undo steps being created instead of one (see .gif attached in the ckeditor/ckeditor5#1130 issue) - still it should be reported as separate issue after we merge this PR, especially that I'm not sure it is an undo or typing issue.

[Reinmar]

Does this patch is somehow targetted at Android? Or will it handle mutations on all browsers? I'm curious if this may help us with some other potential issues where we're not able to handle some changes. But I'm also worried if it might handle too much.

@Mgsy, could you test this PR? Ideally, on Android, but also on some desktop browser to see that there are no collisions.

We're interested in all typing related scenarios – not only backspace, but also enter and simple typing. I'm curious about the general status of things on Android.

[f1ames]

Does this patch is somehow targeted at Android? Or will it handle mutations on all browsers?

There is no environment/device detection so it will be fired on all devices/browsers. However, it handles very specific cases so it shouldn't interfere with the current flow for other browsers (and if such case will happen it will be handled same as on Android which seems to make sense as it will provide consistent behaviour).

Of course there always may be some strange/edge cases... So if something gets removed unexpectedly (not only on Backspace, but in general), it means it might be caused by this fix (cc @Mgsy).

Two issues which are likely to be encounter while testing this PR - https://github.com/ckeditor/ckeditor5-typing/issues/166, ckeditor/ckeditor5#1127.

[Mgsy]

Android

Steps to reproduce

1. Open the article sample.
2. Select the word Paragraph.
3. Activate Bold.
4. Press Backspace.

Current result

The whole block is removed and the selection moves to the previous block.

Expected result

Only word Paragraph is removed.

[Mgsy]

Android

Steps to reproduce

1. Open the article sample.
2. Put the caret at the beginning of the word Paragraph.
3. Activate Bold.
4. Press Backspace.

Current result

Nothing happens.

Expected result

The paragraph is merged to the previous block.

Notes

It requires 8x backspace to merge the blocks.

[Reinmar]

It requires 8x backspace to merge the blocks.

Ouch... That's the inline filler. Nothing blocking for this PR but we'll need to figure out how to get rid of it.

[Mgsy]

Android

Steps to reproduce

1. Open the article sample.
2. Make the following selection:

Bold Italic Link[
* UL List] item 1

3. Press Backspace.
   Optional further steps if works as expected:
4. Undo.
5. Click on the selected part of the UL List item 1 (selection should become as in step 2).
6. Press Backspace.

Current result

Link and part of a Italic are deleted.

[f1ames]

It requires 8x backspace to merge the blocks.

It might be related to https://github.com/ckeditor/ckeditor5-typing/issues/160, where reference to inline filler is lost, I will check it.

[Mgsy]

I've tested it in all desktop browsers and didn't notice any bugs related to this change. On Android also works good, except cases mentioned above 👌

[f1ames]

It looks like Android is a rather tricky environment 😭 I'm able to reproduce all three issues on the current master (so without this fix/PR). I decided to describe each issue in a separate comment as it got quite long along the way.

First issue - #165 (comment):

It happens randomly with and without this PR. There are basically two different flows - one results in correct paragraph removal (so empty paragraph is left) and the other one in entire paragraph removal.

Correct flow:

1. Keydown handler is fired and stopped because selection is collapsed.
2. Android mutations handler is fired, but not triggered because the only mutation does not contain container removal: [ { type: "children", oldChildren: Array(1), newChildren: Array(1), node: ContainerElement } ] - AttributeElement:strong is replaced with Element:br here.
3. Regular mutations handler is fired processing given mutation which results in executing input command.

Incorrect flow:

1. Keydown handler is fired. Selection is not collapsed so the whole selected content gets removed (here the paragraph gets lost).
2. Android mutations handler is fired. The mutations show container removal so it is processed and delete command is triggered. The only effect here is the change of selection position.
3. Because Android mutations handler was fired, the regular one is not. However, even without stoping mutations event, regular mutations handler will not be executed because mutations are not safe for text mutation as they contain both children and text type - [ { type: "text", oldText: "Heading 1", newText: "H", node: Text }, { type: "children", oldChildren: Array(7), newChildren: Array(6), node: RootEditableElement } ]

So it seems the problem is related to unstable selection which is different for identical set of operations, this might be the same problem as ckeditor/ckeditor5#1127. The other thing is that maybe adjusting how keydown handler removes content may help, but I'm not sure about this

This .gif was recorded on latest master without this PR.

[f1ames]

Second issue - #165 (comment):

This is related to #160 - after last step (pressing Backsace), the Uncaught CKEditorError: view-renderer-filler-was-lost: The inline filler node was lost. error is thrown. The inline filler reference gets lost in the Renderer so you have to delete it manually which makes 8th press merging lines (as 7 presses before were deleting 7 ZWS'es).

In this scenario the above fix is not triggered at all because merging lines is handled natively. 🤔

This .gif was recorded on latest master without this PR.

[f1ames]

Third issue - #165 (comment):

This is the same case as ckeditor/ckeditor5#1127. Selection sometimes is collapse, sometimes not which triggers removing selected content by injectUnsafeKeystrokesHandling. See ckeditor/ckeditor5#1127 for detailed description.

The Android mutations handler added in this PR is not triggered in the invalid case (non-collapsed selection) at all.

This .gif was recorded on latest master without this PR.