OT fix for transforming split by move

packages/ckeditor5-engine/src/model/operation/mergeoperation.js

@@ -127,10 +127,7 @@ export default class MergeOperation extends Operation {
 		const path = this.sourcePosition.path.slice( 0, -1 );
 		const insertionPosition = new Position( this.sourcePosition.root, path )._getTransformedByMergeOperation( this );
 
-		const split = new SplitOperation( targetPosition, this.howMany, this.graveyardPosition, this.baseVersion + 1 );
-		split.insertionPosition = insertionPosition;
-
-		return split;
+		return new SplitOperation( targetPosition, this.howMany, insertionPosition, this.graveyardPosition, this.baseVersion + 1 );
 	}
 
 	/**

packages/ckeditor5-engine/src/model/operation/splitoperation.js

@@ -28,12 +28,14 @@ export default class SplitOperation extends Operation {
 	 *
 	 * @param {module:engine/model/position~Position} splitPosition Position at which an element should be split.
 	 * @param {Number} howMany Total offset size of elements that are in the split element after `position`.
+	 * @param {module:engine/model/position~Position} insertionPosition Position at which the clone of split element
+	 * (or element from graveyard) will be inserted.
 	 * @param {module:engine/model/position~Position|null} graveyardPosition Position in the graveyard root before the element which
 	 * should be used as a parent of the nodes after `position`. If it is not set, a copy of the the `position` parent will be used.
 	 * @param {Number|null} baseVersion Document {@link module:engine/model/document~Document#version} on which operation
 	 * can be applied or `null` if the operation operates on detached (non-document) tree.
 	 */
-	constructor( splitPosition, howMany, graveyardPosition, baseVersion ) {
+	constructor( splitPosition, howMany, insertionPosition, graveyardPosition, baseVersion ) {
 		super( baseVersion );
 
 		/**
@@ -58,7 +60,7 @@ export default class SplitOperation extends Operation {
 		 *
 		 * @member {module:engine/model/position~Position} module:engine/model/operation/splitoperation~SplitOperation#insertionPosition
 		 */
-		this.insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+		this.insertionPosition = insertionPosition;
 
 		/**
 		 * Position in the graveyard root before the element which should be used as a parent of the nodes after `position`.
@@ -116,10 +118,7 @@ export default class SplitOperation extends Operation {
 	 * @returns {module:engine/model/operation/splitoperation~SplitOperation} Clone of this operation.
 	 */
 	clone() {
-		const split = new this.constructor( this.splitPosition, this.howMany, this.graveyardPosition, this.baseVersion );
-		split.insertionPosition = this.insertionPosition;
-
-		return split;
+		return new this.constructor( this.splitPosition, this.howMany, this.insertionPosition, this.graveyardPosition, this.baseVersion );
 	}
 
 	/**
@@ -244,10 +243,7 @@ export default class SplitOperation extends Operation {
 		const insertionPosition = Position.fromJSON( json.insertionPosition, document );
 		const graveyardPosition = json.graveyardPosition ? Position.fromJSON( json.graveyardPosition, document ) : null;
 
-		const split = new this( splitPosition, json.howMany, graveyardPosition, json.baseVersion );
-		split.insertionPosition = insertionPosition;
-
-		return split;
+		return new this( splitPosition, json.howMany, insertionPosition, graveyardPosition, json.baseVersion );
 	}
 
 	// @if CK_DEBUG_ENGINE // toString() {

packages/ckeditor5-engine/src/model/operation/transform.js

@@ -497,9 +497,16 @@ class ContextFactory {
 					case MoveOperation: {
 						if ( opA.splitPosition.isEqual( opB.sourcePosition ) || opA.splitPosition.isBefore( opB.sourcePosition ) ) {
 							this._setRelation( opA, opB, 'splitBefore' );
-						}
+						} else {
+							const range = Range._createFromPositionAndShift( opB.sourcePosition, opB.howMany );
 
-						break;
+							if ( opA.splitPosition.hasSameParentAs( opB.sourcePosition ) && range.containsPosition( opA.splitPosition ) ) {
+								const howMany = range.end.offset - opA.splitPosition.offset;
+								const offset = opA.splitPosition.offset - range.start.offset;
+
+								this._setRelation( opA, opB, { howMany, offset } );
+							}
+						}
 					}
 				}
 
@@ -2046,8 +2053,7 @@ setTransformation( SplitOperation, MergeOperation, ( a, b, context ) => {
 		const splitPosition = new Position( b.graveyardPosition.root, splitPath );
 		const insertionPosition = SplitOperation.getInsertionPosition( new Position( b.graveyardPosition.root, splitPath ) );
 
-		const additionalSplit = new SplitOperation( splitPosition, 0, null, 0 );
-		additionalSplit.insertionPosition = insertionPosition;
+		const additionalSplit = new SplitOperation( splitPosition, 0, insertionPosition, null, 0 );
 
 		a.splitPosition = a.splitPosition._getTransformedByMergeOperation( b );
 		a.insertionPosition = SplitOperation.getInsertionPosition( a.splitPosition );
@@ -2107,6 +2113,32 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
 
 	// Case 2:
 	//
+	// Split is at a position where nodes were moved.
+	//
+	// This is a scenario described in `MoveOperation` x `SplitOperation` transformation but from the
+	// "split operation point of view".
+	//
+	const splitAtTarget = a.splitPosition.isEqual( b.targetPosition );
+
+	if ( splitAtTarget && ( context.baRelation == 'insertAtSource' || context.abRelation == 'splitBefore' ) ) {
+		a.howMany += b.howMany;
+		a.splitPosition = a.splitPosition._getTransformedByDeletion( b.sourcePosition, b.howMany );
+		a.insertionPosition = SplitOperation.getInsertionPosition( a.splitPosition );
+
+		return [ a ];
+	}
+
+	if ( splitAtTarget && context.abRelation && context.abRelation.howMany ) {
+		const { howMany, offset } = context.abRelation;
+
+		a.howMany += howMany;
+		a.splitPosition = a.splitPosition.getShiftedBy( offset );
+
+		return [ a ];
+	}
+
+	// Case 3:
+	//
 	// If the split position is inside the moved range, we need to shift the split position to a proper place.
 	// The position cannot be moved together with moved range because that would result in splitting of an incorrect element.
 	//
@@ -2136,23 +2168,6 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
 		return [ a ];
 	}
 
-	// Case 3:
-	//
-	// Split is at a position where nodes were moved.
-	//
-	// This is a scenario described in `MoveOperation` x `SplitOperation` transformation but from the
-	// "split operation point of view".
-	//
-	const splitAtTarget = a.splitPosition.isEqual( b.targetPosition );
-
-	if ( splitAtTarget && ( context.baRelation == 'insertAtSource' || context.abRelation == 'splitBefore' ) ) {
-		a.howMany += b.howMany;
-		a.splitPosition = a.splitPosition._getTransformedByDeletion( b.sourcePosition, b.howMany );
-		a.insertionPosition = SplitOperation.getInsertionPosition( a.splitPosition );
-
-		return [ a ];
-	}
-
 	// The default case.
 	// Don't change `howMany` if move operation does not really move anything.
 	//

packages/ckeditor5-engine/src/model/writer.js

@@ -784,7 +784,9 @@ export default class Writer {
 		do {
 			const version = splitElement.root.document ? splitElement.root.document.version : null;
 			const howMany = splitElement.maxOffset - position.offset;
-			const split = new SplitOperation( position, howMany, null, version );
+
+			const insertionPosition = SplitOperation.getInsertionPosition( position );
+			const split = new SplitOperation( position, howMany, insertionPosition, null, version );
 
 			this.batch.addOperation( split );
 			this.model.applyOperation( split );

packages/ckeditor5-engine/tests/model/differ.js

@@ -1407,7 +1407,9 @@ describe( 'Differ', () => {
 
 			model.change( () => {
 				const position = new Position( root, [ 0, 3 ] );
-				const operation = new SplitOperation( position, 3, new Position( doc.graveyard, [ 0 ] ), doc.version );
+				const insertionPosition = SplitOperation.getInsertionPosition( position );
+
+				const operation = new SplitOperation( position, 3, insertionPosition, new Position( doc.graveyard, [ 0 ] ), doc.version );
 
 				model.applyOperation( operation );
 
@@ -1991,7 +1993,8 @@ describe( 'Differ', () => {
 
 	function split( position ) {
 		const howMany = position.parent.maxOffset - position.offset;
-		const operation = new SplitOperation( position, howMany, null, doc.version );
+		const insertionPosition = SplitOperation.getInsertionPosition( position );
+		const operation = new SplitOperation( position, howMany, insertionPosition, null, doc.version );
 
 		model.applyOperation( operation );
 	}

packages/ckeditor5-engine/tests/model/documentselection.js

@@ -1808,7 +1808,10 @@ describe( 'DocumentSelection', () => {
 				} );
 
 				const batch = new Batch();
-				const splitOperation = new SplitOperation( new Position( root, [ 1, 2 ] ), 4, null, 0 );
+				const splitPosition = new Position( root, [ 1, 2 ] );
+				const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+				const splitOperation = new SplitOperation( splitPosition, 4, insertionPosition, null, 0 );
 
 				batch.addOperation( splitOperation );
 				model.applyOperation( splitOperation );

packages/ckeditor5-engine/tests/model/operation/splitoperation.js

@@ -24,25 +24,37 @@ describe( 'SplitOperation', () => {
 	} );
 
 	it( 'should have proper type', () => {
-		const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
+		const splitPosition = new Position( root, [ 1, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );
 
 		expect( split.type ).to.equal( 'split' );
 	} );
 
 	it( 'should have proper insertionPosition', () => {
-		const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
+		const splitPosition = new Position( root, [ 1, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );
 
 		expect( split.insertionPosition.path ).to.deep.equal( [ 2 ] );
 	} );
 
 	it( 'should have proper moveTargetPosition', () => {
-		const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
+		const splitPosition = new Position( root, [ 1, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );
 
 		expect( split.moveTargetPosition.path ).to.deep.equal( [ 2, 0 ] );
 	} );
 
 	it( 'should have proper movedRange', () => {
-		const split = new SplitOperation( new Position( root, [ 1, 3 ] ), 2, null, 1 );
+		const splitPosition = new Position( root, [ 1, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		const split = new SplitOperation( splitPosition, 2, insertionPosition, null, 1 );
 
 		expect( split.movedRange.start.path ).to.deep.equal( [ 1, 3 ] );
 		expect( split.movedRange.end.path ).to.deep.equal( [ 1, Number.POSITIVE_INFINITY ] );
@@ -53,7 +65,10 @@ describe( 'SplitOperation', () => {
 
 		root._insertChild( 0, [ p1 ] );
 
-		model.applyOperation( new SplitOperation( new Position( root, [ 0, 3 ] ), 3, null, doc.version ) );
+		const splitPosition = new Position( root, [ 0, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		model.applyOperation( new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version ) );
 
 		expect( doc.version ).to.equal( 1 );
 		expect( root.maxOffset ).to.equal( 2 );
@@ -74,7 +89,10 @@ describe( 'SplitOperation', () => {
 		root._insertChild( 0, [ p1 ] );
 		gy._insertChild( 0, [ p2 ] );
 
-		model.applyOperation( new SplitOperation( new Position( root, [ 0, 3 ] ), 3, gyPos, doc.version ) );
+		const splitPosition = new Position( root, [ 0, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		model.applyOperation( new SplitOperation( splitPosition, 3, insertionPosition, gyPos, doc.version ) );
 
 		expect( doc.version ).to.equal( 1 );
 		expect( root.maxOffset ).to.equal( 2 );
@@ -91,7 +109,10 @@ describe( 'SplitOperation', () => {
 	} );
 
 	it( 'should create a proper MergeOperation as a reverse', () => {
-		const operation = new SplitOperation( new Position( root, [ 1, 3 ] ), 3, null, doc.version );
+		const splitPosition = new Position( root, [ 1, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );
 		const reverse = operation.getReversed();
 
 		expect( reverse ).to.be.an.instanceof( MergeOperation );
@@ -107,7 +128,10 @@ describe( 'SplitOperation', () => {
 
 		root._insertChild( 0, [ p1 ] );
 
-		const operation = new SplitOperation( new Position( root, [ 0, 3 ] ), 3, null, doc.version );
+		const splitPosition = new Position( root, [ 0, 3 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+		const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );
 
 		model.applyOperation( operation );
 		model.applyOperation( operation.getReversed() );
@@ -124,7 +148,10 @@ describe( 'SplitOperation', () => {
 
 			root._insertChild( 0, [ p1 ] );
 
-			const operation = new SplitOperation( new Position( root, [ 0, 8 ] ), 3, null, doc.version );
+			const splitPosition = new Position( root, [ 0, 8 ] );
+			const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+			const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );
 
 			expectToThrowCKEditorError( () => operation._validate(), /split-operation-position-invalid/, model );
 		} );
@@ -134,7 +161,10 @@ describe( 'SplitOperation', () => {
 
 			root._insertChild( 0, [ p1 ] );
 
-			const operation = new SplitOperation( new Position( root, [ 0, 0 ] ), 3, null, doc.version );
+			const splitPosition = new Position( root, [ 0, 0 ] );
+			const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+			const operation = new SplitOperation( splitPosition, 3, insertionPosition, null, doc.version );
 			operation.splitPosition = new Position( root, [ 1 ] );
 
 			expectToThrowCKEditorError( () => operation._validate(), /split-operation-split-in-root/, model );
@@ -145,7 +175,10 @@ describe( 'SplitOperation', () => {
 
 			root._insertChild( 0, [ p1 ] );
 
-			const operation = new SplitOperation( new Position( root, [ 0, 2 ] ), 6, null, doc.version );
+			const splitPosition = new Position( root, [ 0, 2 ] );
+			const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+			const operation = new SplitOperation( splitPosition, 6, insertionPosition, null, doc.version );
 
 			expectToThrowCKEditorError( () => operation._validate(), /split-operation-how-many-invalid/, model );
 		} );
@@ -155,18 +188,22 @@ describe( 'SplitOperation', () => {
 
 			root._insertChild( 0, [ p1 ] );
 
-			const operation = new SplitOperation( new Position( root, [ 0, 2 ] ), 4, gyPos, doc.version );
+			const splitPosition = new Position( root, [ 0, 2 ] );
+			const insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
+
+			const operation = new SplitOperation( splitPosition, 4, insertionPosition, gyPos, doc.version );
 
 			expectToThrowCKEditorError( () => operation._validate(), /split-operation-graveyard-position-invalid/, model );
 		} );
 	} );
 
 	it( 'should create SplitOperation with the same parameters when cloned #1', () => {
 		const position = new Position( root, [ 1, 2 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( position );
 		const howMany = 4;
 		const baseVersion = doc.version;
 
-		const op = new SplitOperation( position, howMany, null, baseVersion );
+		const op = new SplitOperation( position, howMany, insertionPosition, null, baseVersion );
 
 		const clone = op.clone();
 
@@ -183,10 +220,11 @@ describe( 'SplitOperation', () => {
 
 	it( 'should create SplitOperation with the same parameters when cloned #2', () => {
 		const position = new Position( root, [ 1, 2 ] );
+		const insertionPosition = SplitOperation.getInsertionPosition( position );
 		const howMany = 4;
 		const baseVersion = doc.version;
 
-		const op = new SplitOperation( position, howMany, gyPos, baseVersion );
+		const op = new SplitOperation( position, howMany, insertionPosition, gyPos, baseVersion );
 
 		const clone = op.clone();
 
@@ -204,7 +242,8 @@ describe( 'SplitOperation', () => {
 	describe( 'toJSON', () => {
 		it( 'should create proper json object #1', () => {
 			const position = new Position( root, [ 0, 3 ] );
-			const op = new SplitOperation( position, 2, null, doc.version );
+			const insertionPosition = SplitOperation.getInsertionPosition( position );
+			const op = new SplitOperation( position, 2, insertionPosition, null, doc.version );
 
 			const serialized = op.toJSON();
 
@@ -220,7 +259,8 @@ describe( 'SplitOperation', () => {
 
 		it( 'should create proper json object #2', () => {
 			const position = new Position( root, [ 0, 3 ] );
-			const op = new SplitOperation( position, 2, gyPos, doc.version );
+			const insertionPosition = SplitOperation.getInsertionPosition( position );
+			const op = new SplitOperation( position, 2, insertionPosition, gyPos, doc.version );
 
 			const serialized = op.toJSON();
 
@@ -238,7 +278,8 @@ describe( 'SplitOperation', () => {
 	describe( 'fromJSON', () => {
 		it( 'should create proper SplitOperation from json object #1', () => {
 			const position = new Position( root, [ 0, 3 ] );
-			const op = new SplitOperation( position, 2, null, doc.version );
+			const insertionPosition = SplitOperation.getInsertionPosition( position );
+			const op = new SplitOperation( position, 2, insertionPosition, null, doc.version );
 
 			const serialized = op.toJSON();
 
@@ -249,7 +290,8 @@ describe( 'SplitOperation', () => {
 
 		it( 'should create proper SplitOperation from json object #2', () => {
 			const position = new Position( root, [ 0, 3 ] );
-			const op = new SplitOperation( position, 2, gyPos, doc.version );
+			const insertionPosition = SplitOperation.getInsertionPosition( position );
+			const op = new SplitOperation( position, 2, insertionPosition, gyPos, doc.version );
 
 			const serialized = op.toJSON();