Merge pull request #1105 from ckeditor/t/1103

Fix: Fixed a bug when editor crashed during MergeDelta transformation in a specific case. Closes #1103.

Author: Piotr Jasiun

src/model/delta/basic-transformations.js

@@ -97,6 +97,11 @@ addTransformationCase( AttributeDelta, SplitDelta, ( a, b, context ) => {
 
 // Add special case for InsertDelta x MergeDelta transformation.
 addTransformationCase( InsertDelta, MergeDelta, ( a, b, context ) => {
+	// Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation.
+	if ( !b.position ) {
+		return defaultTransform( a, b, context );
+	}
+
 	const undoMode = context.aWasUndone || context.bWasUndone;
 
 	// If insert is applied at the same position where merge happened, we reverse the merge (we treat it like it
@@ -136,10 +141,14 @@ addTransformationCase( MarkerDelta, RenameDelta, transformMarkerDelta );
 
 // Add special case for MoveDelta x MergeDelta transformation.
 addTransformationCase( MoveDelta, MergeDelta, ( a, b, context ) => {
+	// Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation.
+	if ( !b.position ) {
+		return defaultTransform( a, b, context );
+	}
+
 	// If move delta is supposed to move a node that has been merged, we reverse the merge (we treat it like it
 	// didn't happen) and then apply the original move operation. This is "mirrored" in MergeDelta x MoveDelta
 	// transformation below, where we simply do not apply MergeDelta.
-
 	const operateInSameParent =
 		a.sourcePosition.root == b.position.root &&
 		compareArrays( a.sourcePosition.getParentPath(), b.position.getParentPath() ) === 'same';
@@ -158,6 +167,11 @@ addTransformationCase( MoveDelta, MergeDelta, ( a, b, context ) => {
 
 // Add special case for MergeDelta x InsertDelta transformation.
 addTransformationCase( MergeDelta, InsertDelta, ( a, b, context ) => {
+	// Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation.
+	if ( !a.position ) {
+		return defaultTransform( a, b, context );
+	}
+
 	const undoMode = context.aWasUndone || context.bWasUndone;
 
 	// If merge is applied at the same position where we inserted a range of nodes we cancel the merge as it's results
@@ -171,9 +185,13 @@ addTransformationCase( MergeDelta, InsertDelta, ( a, b, context ) => {
 
 // Add special case for MergeDelta x MoveDelta transformation.
 addTransformationCase( MergeDelta, MoveDelta, ( a, b, context ) => {
+	// Do not apply special transformation case if `MergeDelta` has `NoOperation` as the second operation.
+	if ( !a.position ) {
+		return defaultTransform( a, b, context );
+	}
+
 	// If merge is applied at the position between moved nodes we cancel the merge as it's results may be unexpected and
 	// very weird. Even if we do some "magic" we don't know what really are users' expectations.
-
 	const operateInSameParent =
 		a.position.root == b.sourcePosition.root &&
 		compareArrays( a.position.getParentPath(), b.sourcePosition.getParentPath() ) === 'same';

tests/model/delta/transform/insertdelta.js

@@ -18,6 +18,7 @@ import SplitDelta from '../../../../src/model/delta/splitdelta';
 import InsertOperation from '../../../../src/model/operation/insertoperation';
 import MoveOperation from '../../../../src/model/operation/moveoperation';
 import ReinsertOperation from '../../../../src/model/operation/reinsertoperation';
+import NoOperation from '../../../../src/model/operation/nooperation';
 
 import { getNodesAndText } from '../../../../tests/model/_utils/utils';
 
@@ -156,6 +157,30 @@ describe( 'transform', () => {
 				} );
 			} );
 
+			it( 'merge in same position as insert - undo mode', () => {
+				// If MergeDelta second operation is NoOperation, default transformation algorithm should be used.
+				const mergeDelta = getMergeDelta( insertPosition, 4, 12, baseVersion );
+				mergeDelta.operations[ 1 ] = new NoOperation( 1 );
+
+				const transformed = transform( insertDelta, mergeDelta, context );
+
+				baseVersion = mergeDelta.operations.length;
+
+				expect( transformed.length ).to.equal( 1 );
+
+				expectDelta( transformed[ 0 ], {
+					type: InsertDelta,
+					operations: [
+						{
+							type: InsertOperation,
+							position: Position.createFromPosition( insertPosition ),
+							nodes: [ nodeA, nodeB ],
+							baseVersion
+						}
+					]
+				} );
+			} );
+
 			it( 'merge the node that is parent of insert position (sticky move test)', () => {
 				const mergeDelta = getMergeDelta( new Position( root, [ 3, 3 ] ), 1, 4, baseVersion );
 				const transformed = transform( insertDelta, mergeDelta, context );

tests/model/delta/transform/mergedelta.js

@@ -120,6 +120,34 @@ describe( 'transform', () => {
 				} );
 			} );
 
+			it( 'insert at same position as merge - merge second operation is NoOperation', () => {
+				// If MergeDelta second operation is NoOperation, default transformation algorithm should be used.
+				mergeDelta.operations[ 1 ] = new NoOperation( 1 );
+				const insertDelta = getInsertDelta( mergePosition, [ nodeA, nodeB ], baseVersion );
+				const transformed = transform( mergeDelta, insertDelta, context );
+
+				baseVersion = insertDelta.operations.length;
+
+				expect( transformed.length ).to.equal( 1 );
+
+				expectDelta( transformed[ 0 ], {
+					type: MergeDelta,
+					operations: [
+						{
+							type: MoveOperation,
+							sourcePosition: new Position( root, [ 3, 3, 5, 0 ] ),
+							howMany: mergeDelta.operations[ 0 ].howMany,
+							targetPosition: mergeDelta.operations[ 0 ].targetPosition,
+							baseVersion
+						},
+						{
+							type: NoOperation,
+							baseVersion: baseVersion + 1
+						}
+					]
+				} );
+			} );
+
 			it( 'insert inside merged node (sticky move test)', () => {
 				const insertDelta = getInsertDelta( new Position( root, [ 3, 3, 3, 12 ] ), [ nodeA, nodeB ], baseVersion );
 				const transformed = transform( mergeDelta, insertDelta, context );
@@ -227,6 +255,35 @@ describe( 'transform', () => {
 				// MoveDelta is applied. MergeDelta is discarded.
 				expect( nodesAndText ).to.equal( 'DIVXabcdabcfoobarxyzXXXXXDIV' );
 			} );
+
+			it( 'node on the left side of merge was moved - second merge operation is NoOperation', () => {
+				// If second MergeDelta operation is NoOperation default algorithm should be used.
+				mergeDelta.operations[ 1 ] = new NoOperation( 1 );
+
+				const moveDelta = getMoveDelta( new Position( root, [ 3, 3, 2 ] ), 1, new Position( root, [ 3, 3, 0 ] ), baseVersion );
+				const transformed = transform( mergeDelta, moveDelta, context );
+
+				expect( transformed.length ).to.equal( 1 );
+
+				baseVersion = moveDelta.operations.length;
+
+				expectDelta( transformed[ 0 ], {
+					type: MergeDelta,
+					operations: [
+						{
+							type: MoveOperation,
+							sourcePosition: new Position( root, [ 3, 3, 3, 0 ] ),
+							howMany: 12,
+							targetPosition: new Position( root, [ 3, 3, 0, 4 ] ),
+							baseVersion
+						},
+						{
+							type: NoOperation,
+							baseVersion: baseVersion + 1
+						}
+					]
+				} );
+			} );
 		} );
 
 		describe( 'MergeDelta', () => {

tests/model/delta/transform/movedelta.js

@@ -15,6 +15,7 @@ import MoveDelta from '../../../../src/model/delta/movedelta';
 import SplitDelta from '../../../../src/model/delta/splitdelta';
 
 import MoveOperation from '../../../../src/model/operation/moveoperation';
+import NoOperation from '../../../../src/model/operation/nooperation';
 
 import { getNodesAndText } from '../../../../tests/model/_utils/utils';
 
@@ -159,6 +160,32 @@ describe( 'transform', () => {
 					]
 				} );
 			} );
+
+			it( 'node on the right side of merge was moved - second merge operation is NoOperation', () => {
+				// If second MergeDelta operation is NoOperation default algorithm should be used.
+				const mergePosition = new Position( root, [ 3, 3, 3 ] );
+				const mergeDelta = getMergeDelta( mergePosition, 4, 12, baseVersion );
+				mergeDelta.operations[ 1 ] = new NoOperation( 1 );
+
+				const transformed = transform( moveDelta, mergeDelta, context );
+
+				expect( transformed.length ).to.equal( 1 );
+
+				baseVersion = mergeDelta.operations.length;
+
+				expectDelta( transformed[ 0 ], {
+					type: MoveDelta,
+					operations: [
+						{
+							type: MoveOperation,
+							sourcePosition: moveDelta._moveOperation.sourcePosition,
+							howMany: moveDelta._moveOperation.howMany,
+							targetPosition: moveDelta._moveOperation.targetPosition,
+							baseVersion
+						}
+					]
+				} );
+			} );
 		} );
 	} );
 } );