Merge pull request #947 from ckeditor/t/946

Fix: Editor will no longer crash when `ReinsertOperation` is transformed by specific `RemoveOperation`. Closes #946.

Author: Reinmar

src/model/operation/transform.js

@@ -384,18 +384,30 @@ const ot = {
 				return [ b.getReversed() ];
 			}
 
-			// Special case when both operations are RemoveOperations. RemoveOperation not only moves nodes but also
-			// (usually) creates a "holder" element for them in graveyard. Each RemoveOperation should move nodes to different
-			// "holder" element. If `a` operation points after `b` operation, we move `a` offset to acknowledge
-			// "holder" element insertion.
-			if ( a instanceof RemoveOperation && b instanceof RemoveOperation ) {
-				const aTarget = a.targetPosition.path[ 0 ];
-				const bTarget = b.targetPosition.path[ 0 ];
-
-				if ( aTarget > bTarget || ( aTarget == bTarget && isStrong ) ) {
-					// Do not change original operation!
-					a = a.clone();
-					a.targetPosition.path[ 0 ]++;
+			// Special case when operation transformed by is RemoveOperation. RemoveOperation not only moves nodes but also
+			// (usually) creates a "holder" element for them in graveyard. This has to be taken into consideration when
+			// transforming operations that operate in graveyard root.
+			if ( b instanceof RemoveOperation && b._needsHolderElement ) {
+				a = a.clone();
+
+				const aSourceOffset = a.sourcePosition.path[ 0 ];
+				const aTargetOffset = a.targetPosition.path[ 0 ];
+
+				// We can't use Position#_getTransformedByInsertion because it works a bit differently and we would get wrong results.
+				// That's because `sourcePosition`/`targetPosition` is, for example, `[ 1, 0 ]`, while `insertionPosition` is
+				// `[ 1 ]`. In this case, `sourcePosition`/`targetPosition` would always be transformed but in fact, only
+				// "holderElement" offsets should be looked at (so `[ 1 ]` in case of `sourcePosition`/`targetPosition`, not `[ 1, 0 ]` ).
+				// This is why we are doing it "by hand".
+				if ( a.sourcePosition.root == b.targetPosition.root ) {
+					if ( aSourceOffset > b._holderElementOffset || aSourceOffset == b._holderElementOffset ) {
+						a.sourcePosition.path[ 0 ]++;
+					}
+				}
+
+				if ( a.targetPosition.root == b.targetPosition.root ) {
+					if ( aTargetOffset > b._holderElementOffset || ( aTargetOffset == b._holderElementOffset && isStrong ) ) {
+						a.targetPosition.path[ 0 ]++;
+					}
 				}
 			}
 

tests/model/operation/transform.js

@@ -18,6 +18,7 @@ import MarkerOperation from '../../../src/model/operation/markeroperation';
 import MoveOperation from '../../../src/model/operation/moveoperation';
 import RemoveOperation from '../../../src/model/operation/removeoperation';
 import RenameOperation from '../../../src/model/operation/renameoperation';
+import ReinsertOperation from '../../../src/model/operation/reinsertoperation';
 import NoOperation from '../../../src/model/operation/nooperation';
 
 describe( 'transform', () => {
@@ -2771,7 +2772,7 @@ describe( 'transform', () => {
 		} );
 
 		describe( 'by MoveOperation', () => {
-			it( 'should create not more than RemoveOperation that needs new graveyard holder', () => {
+			it( 'should create not more than one RemoveOperation that needs new graveyard holder', () => {
 				let op = new RemoveOperation( new Position( root, [ 1 ] ), 4, baseVersion );
 				let transformBy = new MoveOperation( new Position( root, [ 0 ] ), 2, new Position( root, [ 8 ] ), baseVersion );
 
@@ -2796,6 +2797,28 @@ describe( 'transform', () => {
 				expect( transOp[ 0 ]._needsHolderElement ).to.be.false;
 				expect( transOp[ 1 ]._needsHolderElement ).to.be.false;
 			} );
+
+			it( 'should force removing content even if was less important', () => {
+				let op = new RemoveOperation( new Position( root, [ 8 ] ), 2, baseVersion );
+
+				const targetPosition = Position.createFromPosition( op.targetPosition );
+
+				let transformBy = new MoveOperation( new Position( root, [ 8 ] ), 2, new Position( root, [ 1 ] ), baseVersion );
+
+				const sourcePosition = Position.createFromPosition( transformBy.targetPosition );
+
+				let transOp = transform( op, transformBy, false );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expectOperation( transOp[ 0 ], {
+					type: RemoveOperation,
+					howMany: 2,
+					sourcePosition: sourcePosition,
+					targetPosition: targetPosition,
+					baseVersion: baseVersion + 1
+				} );
+			} );
 		} );
 
 		describe( 'by RemoveOperation', () => {
@@ -2835,6 +2858,47 @@ describe( 'transform', () => {
 		} );
 	} );
 
+	describe( 'ReinsertOperation', () => {
+		describe( 'by RemoveOperation', () => {
+			it( 'should update sourcePosition if remove operation inserted holder offset before reinsert operation holder', () => {
+				let position = new Position( root, [ 2, 1 ] );
+				let transformBy = new RemoveOperation( position, 3, baseVersion );
+				let op = new ReinsertOperation( new Position( doc.graveyard, [ 0, 0 ] ), 3, position, baseVersion );
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expectOperation( transOp[ 0 ], {
+					type: ReinsertOperation,
+					howMany: 3,
+					sourcePosition: new Position( doc.graveyard, [ 1, 0 ] ),
+					targetPosition: position,
+					baseVersion: baseVersion + 1
+				} );
+			} );
+
+			it( 'should not update sourcePosition if remove operation inserted holder offset after reinsert operation source', () => {
+				let position = new Position( root, [ 2, 1 ] );
+				let transformBy = new RemoveOperation( position, 3, baseVersion );
+				transformBy.targetPosition = new Position( doc.graveyard, [ 3, 0 ] );
+				let op = new ReinsertOperation( new Position( doc.graveyard, [ 2, 0 ] ), 3, position, baseVersion );
+
+				let transOp = transform( op, transformBy );
+
+				expect( transOp.length ).to.equal( 1 );
+
+				expectOperation( transOp[ 0 ], {
+					type: ReinsertOperation,
+					howMany: 3,
+					sourcePosition: new Position( doc.graveyard, [ 2, 0 ] ),
+					targetPosition: position,
+					baseVersion: baseVersion + 1
+				} );
+			} );
+		} );
+	} );
+
 	describe( 'NoOperation', () => {
 		beforeEach( () => {
 			op = new NoOperation( baseVersion );