Fix MarkerOperation x MoveOperation OT for undo cases

[scofalik]

Suggested merge commit message (convention)

Fix (engine): Fixed incorrect marker handling in some scenarios involving undo and real-time collaboration, which earlier lead to model-nodelist-offset-out-of-bounds error.

Fix (engine): Fixed incorrect handling of merge changes during undo in some scenarios involving real-time collaboration, which earlier led to model-nodelist-offset-out-of-bounds error.

Fix (engine): Fixed conflict resolution error, which led to editor crash in some scenarios where two users removed bigger intersecting part of the content and used undo.

Fix (engine): Fixed incorrect undo behavior leading to an editor crash when a user pressed enter key multiple times, then pressed backspace that many times, then undone all the changes. Closes #9296.

[scofalik]

This PR introduces new way to handle MarkerOperation during transformations.

To ensure proper transformation, I introduced breaking MarkerOperation into multiple operations if it is necessary. Similar solution already exists for AttributeOperation and MoveOperation, which also can be broken into multiple operations during transformation process.

This is necessary to correctly handle a case like this, and similar. Assume [] is a marker operation on given range and the model is:

<p>This [is my example]</p>

If we (re)move is my  then currently the marker operation would be transformed to:

<p>This [example]</p>

Which is natural and correct.

However, if during the same transformation process, the marker operation is transformed by reinsert, then we end up with:

<p>This is my [example]</p>

We had this and similar cases already covered, but current solution failed for more complex scenarios. Current solution directly saved paths to which the marker should be reverted if such scenario occured:

1: return "is my ":                    <p>This is my [example]</p>
2: revert range to saved path:        <p>This [is my example]</p>        (saved range is 0,5 - 0,18)

The problem is that these paths were not updated during transformation process. If for some reason (e.g. conflict resolution during RTC OT) some part of this paragraph was not returned during undo process, the saved paths may point to non-existing offsets:

1: return "is " instead "is my ":    <p>This is [example]</p>
2: revert range to saved path:        <p>This [is example</p>            (saved range 0,5 - 0,18 is incorrect)

Now, instead of saving ranges, we will create additional MarkerOperations for these parts of the marker range which were moved somewhere (in this case it would be the [is my ] part of the original range). Such extra marker operation will be processed through transformations just after the original marker operation. As I mentioned, we already use this solution for AttributeOperation and MoveOperation in similar case.

The difference is that you cannot have multiple ranges for one marker. If we create multiple MarkerOperation in place of an original MarkerOperation, they will overwrite each other, this is incorrect, in the very end we want to have just one MarkerOperation, after all, we want to have a solution for such case:

Initial:		<p>This [is my example]</p>		MarkerOperation 0,5 - 0,18
Remove:			<p>This [example]</p>			MarkerOperation 0,5 - 0,11
Undo remove:	<p>This is my [example]</p>		MarkerOperation 0,12 - 0,18
Fix marker:		<p>This [is my example]</p>		MarkerOperation 0,5 - 0,18

With multiple marker operations approach, the original MarkerOperation is broken into two operations, which, after all transformations, will end up next to each other:

<p>This {is my }[example]</p>

At the end of transformation process, we will handle these partial marker operations. As I mentioned, we want to have just one marker operation in the end. For each partial marker operation:

• If it is next to its original marker operation after all transformations, join them together.
• Otherwise, discard it. E.g. if is my would not be undone, then such partial marker operation would end up in the graveyard. Since is my was not returned next to my example, we don't want to revert original marker operation.

[scofalik]

There was a bug here, we go "left" in the array, so we should decrement i.

[scofalik]

I've wrapped unit tests from transform/undo.js into a separate describe() to show them better in unit tests logs but because of that the changes in that file are hard to digest. I added a unit test for each fix, four total. The unit tests are based on the scenarios from their related issues.

[scofalik]

This PR includes four fixes. I already described the first one. Although that fix is complex and wide, I have very strong confidence that it is correct. There are three more fixes.

Fix (2): c5a614e

This fix makes it so that MergeOperation becomes NoOperation more often than before. Earlier, !context.forceWeakRemove had to be true (forceWeakRemove = undo mode). This, in essence, means that merge became NoOperation only outside undo. During undo we wanted to keep merge operation and keep transforming it, but this caused errors down the road. Now, the merge operation will become NoOperation also during undo.

The side effect of this change is that sometimes the content may not be recovered exactly as we expected -- i.e. two elements will be removed instead of one, or we would expect an "enter" between some text (so, we get <p>Foobar</p> instead of <p>Foo</p><p>bar</p>).

I have lower confidence towards this fix. It fixes one of the issues, and it also fixes an issue found by QA team when testing fix (1). Also, all unit tests were passing. But I assume it was done like this for a reason and we might not know about some scenario, where it made sense. But it may be only to provide better UX during undo (as described above).

Also, there is a "mirror case" in MoveOperation x MergeOperation transformation, and that case still uses !context.forceWeakRemove in its condition, which seems to lead to disparity and may be a problem. I want to investigate it a little more.

Fix (3): 4a9d71d

This fix is more straightforward as it appeared that earlier solution lacked transformation of one of the MoveOperation properties. I have pretty strong confidence in this kind of fixes. It is an omission fix. It seems that we forgot/missed that it should be transformed too. In contrary, in fix (2), we are changing rules when a transformation is applied. I know that maybe the difference is not tangible to someone who does not know this system, but, again, I feel quite confident with this change.

Before this change, the MoveOperation was incorrectly transformed, and it brought back incorrect nodes from graveyard. As a result, after undo, we would be ending up with something like this in model: <p></p><p>XYZ</p>C instead of, e.g. <p>ABC</p><p>XYZ</p>.

Fix (4): 471390b

This causes a special case in MergeOperation x SplitOperation to be applied less often. Normally, merge operation targets the end of an element, e.g. you have <p>Foo</p><p>Bar</p> and merge operation source is [ 1, 0 ] and target is [ 0, 3 ].

During OT/undo when some nodes might have been removed, or splits may have happened, sometimes merge operation starts to target into inside of the element. This is a temporary situation for the merge operation and as the merge operation is further transformed, the target position should eventually end up pointing to an end of target element.

The special case was applied always when the merge operation was targeting into inside of an element instead of its end.

It appears that this assumption was rather not correct, because in the case I wanted to fix, it caused an error later during other transformations.

As you can see, I removed mergeInside from the if. Most importantly, in almost all unit tests (~10?) mergeInside was true only when the other condition (context.abRelation == 'mergeTargetNotMoved') was true as well (it's something I'd expect). We had only one unit test, where mergeInside was true and the other was false.

That unit test was still green, even after removing mergeInside, which means that it was indifferent to whether a special transformation case was applied or not. That merge operation eventually ended up in the same state, regardless if at that concrete step, we took special case or not. I followed this transformation and I concluded that the special transformation case made the merge operation into "final" state and the merge operation was not affected by the rest of operations. I concluded that this was a bit lucky. After removing mergeInside, the merge operation did not receive special case and was affected by following operations. The way how merge operation interacted with other operations made sense to me and was more in line with what I expected. The final result was the same, but the merge operation correctly interacted (as it should) with other operations during transformation process. So, I believe, that in this one case that we earlier had, where mergeInside made difference, it was also correct to not apply the special case.

This gives me a bit more confidence towards this fix, but it is similar to fix (2) where we change rules when we apply a fix. I still have more confidence than in fix (2) but less than for fix (1) or (3).

The unit test I described above is pasting on non-collapsed selection undo and redo from transform/undo.js.

[scofalik]

For clarity, below issues should be fixed by this PR:

• (Fix 1) cksource/ckeditor5-commercial#4371
• (Fix 2) cksource/ckeditor5-commercial#4341
• (Fix 3) cksource/ckeditor5-commercial#1939
• (Fix 4) Undo after splitting elements crashes the editor. #9296