Merge pull request #1581 from ckeditor/t/1580

Fix: `MoveOperation` x `SplitOperation` transformation for a case when graveyard element is moved. Closes #1580.

Author: Piotr Jasiun

src/model/differ.js

@@ -423,7 +423,7 @@ export default class Differ {
 			// If change happens at the same position...
 			if ( a.position.isEqual( b.position ) ) {
 				// Keep chronological order of operations.
-				return a.changeCount < b.changeCount ? -1 : 1;
+				return a.changeCount - b.changeCount;
 			}
 
 			// If positions differ, position "on the left" should be earlier in the result.

src/model/operation/transform.js

@@ -1669,12 +1669,21 @@ setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => {
 	// The default case.
 	//
 	const transformed = moveRange._getTransformedBySplitOperation( b );
+	const ranges = [ transformed ];
 
-	a.sourcePosition = transformed.start;
-	a.howMany = transformed.end.offset - transformed.start.offset;
-	a.targetPosition = newTargetPosition;
+	// Case 5:
+	//
+	// Moved range contains graveyard element used by split operation. Add extra move operation to the result.
+	//
+	if ( b.graveyardPosition ) {
+		const movesGraveyardElement = moveRange.start.isEqual( b.graveyardPosition ) || moveRange.containsPosition( b.graveyardPosition );
 
-	return [ a ];
+		if ( a.howMany > 1 && movesGraveyardElement ) {
+			ranges.push( Range.createFromPositionAndShift( b.insertionPosition, 1 ) );
+		}
+	}
+
+	return _makeMoveOperationsFromRanges( ranges, newTargetPosition );
 } );
 
 setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => {
@@ -1692,16 +1701,30 @@ setTransformation( MoveOperation, MergeOperation, ( a, b, context ) => {
 			// removed nodes might be unexpected. This means that in this scenario we will reverse merging and remove the element.
 			//
 			if ( !context.aWasUndone ) {
-				const gyMoveTarget = Position.createFromPosition( b.graveyardPosition );
-				const gyMove = new MoveOperation( b.graveyardPosition, 1, gyMoveTarget, 0 );
+				const results = [];
 
-				const targetPositionPath = b.graveyardPosition.path.slice();
+				let gyMoveSource = Position.createFromPosition( b.graveyardPosition );
+				let splitNodesMoveSource = Position.createFromPosition( b.targetPosition );
+
+				if ( a.howMany > 1 ) {
+					results.push( new MoveOperation( a.sourcePosition, a.howMany - 1, a.targetPosition, 0 ) );
+					gyMoveSource = gyMoveSource._getTransformedByInsertion( a.targetPosition, a.howMany - 1 );
+					splitNodesMoveSource = splitNodesMoveSource._getTransformedByMove( a.sourcePosition, a.targetPosition, a.howMany - 1 );
+				}
+
+				const gyMoveTarget = b.deletionPosition._getCombined( a.sourcePosition, a.targetPosition );
+				const gyMove = new MoveOperation( gyMoveSource, 1, gyMoveTarget, 0 );
+
+				const targetPositionPath = gyMove.getMovedRangeStart().path.slice();
 				targetPositionPath.push( 0 );
 
-				return [
-					gyMove,
-					new MoveOperation( b.targetPosition, b.howMany, new Position( a.targetPosition.root, targetPositionPath ), 0 )
-				];
+				const splitNodesMoveTarget = new Position( gyMove.targetPosition.root, targetPositionPath );
+				const splitNodesMove = new MoveOperation( splitNodesMoveSource, b.howMany, splitNodesMoveTarget, 0 );
+
+				results.push( gyMove );
+				results.push( splitNodesMove );
+
+				return results;
 			}
 		} else {
 			// Case 2:
@@ -1934,14 +1957,21 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => {
 	if ( a.graveyardPosition ) {
 		// Case 1:
 		//
-		// Split operation graveyard node was moved. In this case move operation is stronger and the split insertion position
-		// should be corrected.
+		// Split operation graveyard node was moved. In this case move operation is stronger. Since graveyard element
+		// is already moved to the correct position, we need to only move the nodes after the split position.
+		// This will be done by `MoveOperation` instead of `SplitOperation`.
 		//
-		if ( rangeToMove.containsPosition( a.graveyardPosition ) || rangeToMove.start.isEqual( a.graveyardPosition ) ) {
-			a.insertionPosition = Position.createFromPosition( b.targetPosition );
-			a.splitPosition = a.splitPosition._getTransformedByMoveOperation( b );
+		if ( rangeToMove.start.isEqual( a.graveyardPosition ) || rangeToMove.containsPosition( a.graveyardPosition ) ) {
+			const sourcePosition = a.splitPosition._getTransformedByMoveOperation( b );
 
-			return [ a ];
+			const newParentPosition = a.graveyardPosition._getTransformedByMoveOperation( b );
+			const newTargetPath = newParentPosition.path.slice();
+			newTargetPath.push( 0 );
+
+			const newTargetPosition = new Position( newParentPosition.root, newTargetPath );
+			const moveOp = new MoveOperation( sourcePosition, a.howMany, newTargetPosition, 0 );
+
+			return [ moveOp ];
 		}
 
 		a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b );
@@ -2172,7 +2202,14 @@ function _makeMoveOperationsFromRanges( ranges, targetPosition ) {
 	for ( let i = 0; i < ranges.length; i++ ) {
 		// Create new operation out of a range and target position.
 		const range = ranges[ i ];
-		const op = new MoveOperation( range.start, range.end.offset - range.start.offset, targetPosition, 0 );
+		const op = new MoveOperation(
+			range.start,
+			range.end.offset - range.start.offset,
+			// If the target is the end of the move range this operation doesn't really move anything.
+			// In this case, it is better for OT to use range start instead of range end.
+			targetPosition.isEqual( range.end ) ? range.start : targetPosition,
+			0
+		);
 
 		operations.push( op );
 

src/model/range.js

@@ -517,13 +517,17 @@ export default class Range {
 	 */
 	_getTransformedBySplitOperation( operation ) {
 		const start = this.start._getTransformedBySplitOperation( operation );
-
-		let end;
+		let end = this.end._getTransformedBySplitOperation( operation );
 
 		if ( this.end.isEqual( operation.insertionPosition ) ) {
 			end = this.end.getShiftedBy( 1 );
-		} else {
-			end = this.end._getTransformedBySplitOperation( operation );
+		}
+
+		// Below may happen when range contains graveyard element used by split operation.
+		if ( start.root != end.root ) {
+			// End position was next to the moved graveyard element and was moved with it.
+			// Fix it by using old `end` which has proper `root`.
+			end = this.end.getShiftedBy( -1 );
 		}
 
 		return new Range( start, end );

tests/model/operation/transform/merge.js

@@ -174,9 +174,7 @@ describe( 'transform', () => {
 				kate.undo();
 				syncClients();
 
-				expectClients(
-					'<paragraph>Foo</paragraph>'
-				);
+				expectClients( '<paragraph>Foo</paragraph>' );
 			} );
 		} );
 
@@ -222,6 +220,40 @@ describe( 'transform', () => {
 
 				expectClients( '<paragraph>FooBar</paragraph>' );
 			} );
+
+			it( 'remove merged element then undo #3', () => {
+				john.setData( '[<paragraph>A</paragraph><paragraph>B</paragraph>]<paragraph>C</paragraph>' );
+				kate.setData( '<paragraph>A</paragraph>[]<paragraph>B</paragraph><paragraph>C</paragraph>' );
+
+				kate.merge();
+				john.remove();
+
+				syncClients();
+				expectClients( '<paragraph>C</paragraph>' );
+
+				john.undo();
+				kate.undo();
+
+				syncClients();
+				expectClients( '<paragraph>A</paragraph><paragraph>B</paragraph><paragraph>C</paragraph>' );
+			} );
+
+			it( 'remove merged element then undo #4', () => {
+				john.setData( '<paragraph>A</paragraph>[<paragraph>B</paragraph><paragraph>C</paragraph>]' );
+				kate.setData( '<paragraph>A</paragraph>[]<paragraph>B</paragraph><paragraph>C</paragraph>' );
+
+				kate.merge();
+				john.remove();
+
+				syncClients();
+				expectClients( '<paragraph>A</paragraph>' );
+
+				john.undo();
+				kate.undo();
+
+				syncClients();
+				expectClients( '<paragraph>A</paragraph><paragraph>B</paragraph><paragraph>C</paragraph>' );
+			} );
 		} );
 
 		describe( 'by delete', () => {
@@ -409,5 +441,23 @@ describe( 'transform', () => {
 				);
 			} );
 		} );
+
+		describe( 'by split', () => {
+			it( 'merge element which got split (the element is in blockquote) and undo', () => {
+				john.setData( '<paragraph>Foo</paragraph><blockQuote><paragraph>[]Bar</paragraph></blockQuote>' );
+				kate.setData( '<paragraph>Foo</paragraph><blockQuote><paragraph>B[]ar</paragraph></blockQuote>' );
+
+				john._processExecute( 'delete' );
+				kate.split();
+
+				syncClients();
+				expectClients( '<paragraph>FooB</paragraph><paragraph>ar</paragraph>' );
+
+				john.undo();
+
+				syncClients();
+				expectClients( '<paragraph>Foo</paragraph><blockQuote><paragraph>B</paragraph><paragraph>ar</paragraph></blockQuote>' );
+			} );
+		} );
 	} );
 } );