Merge pull request #1553 from ckeditor/t/1552

Internal: Improved merge x split transformations in undo and redo scenarios. Closes #1552.

src/model/operation/transform.js

@@ -501,6 +501,24 @@ class ContextFactory {
 
 				break;
 			}
+
+			case MergeOperation: {
+				switch ( opB.constructor ) {
+					case MergeOperation: {
+						if ( !opA.targetPosition.isEqual( opB.sourcePosition ) ) {
+							this._setRelation( opA, opB, 'mergeTargetNotMoved' );
+						}
+
+						if ( opA.sourcePosition.isEqual( opB.sourcePosition ) ) {
+							this._setRelation( opA, opB, 'mergeSameElement' );
+						}
+
+						break;
+					}
+				}
+
+				break;
+			}
 		}
 	}
 
@@ -1291,7 +1309,7 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => {
 	return [ a ];
 } );
 
-setTransformation( MergeOperation, SplitOperation, ( a, b ) => {
+setTransformation( MergeOperation, SplitOperation, ( a, b, context ) => {
 	if ( b.graveyardPosition ) {
 		// If `b` operation defines graveyard position, a node from graveyard will be moved. This means that we need to
 		// transform `a.graveyardPosition` accordingly.
@@ -1334,7 +1352,7 @@ setTransformation( MergeOperation, SplitOperation, ( a, b ) => {
 	// This means that `targetPosition` needs to be transformed. This is the default case though.
 	// For example, if the split would be after `F`, `targetPosition` should also be transformed.
 	//
-	// There are two exception, though, when we want to keep `targetPosition` as it was.
+	// There are three exceptions, though, when we want to keep `targetPosition` as it was.
 	//
 	// First exception is when the merge target position is inside an element (not at the end, as usual). This
 	// happens when the merge operation earlier was transformed by "the same" merge operation. If merge operation
@@ -1360,14 +1378,44 @@ setTransformation( MergeOperation, SplitOperation, ( a, b ) => {
 	//
 	// If `targetPosition` is transformed, it would become root [ 1, 0 ] as well. It has to be kept as it was.
 	//
+	// Third exception is connected with relations. If this happens during undo and we have explicit information
+	// that target position has not been affected by the operation which is undone by this split then this split should
+	// not move the target position either.
+	//
 	if ( a.targetPosition.isEqual( b.position ) ) {
-		if ( b.howMany != 0 || ( b.graveyardPosition && a.deletionPosition.isEqual( b.graveyardPosition ) ) ) {
+		const mergeInside = b.howMany != 0;
+		const mergeSplittingElement = b.graveyardPosition && a.deletionPosition.isEqual( b.graveyardPosition );
+
+		if ( mergeInside || mergeSplittingElement || context.abRelation == 'mergeTargetNotMoved' ) {
 			a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b );
 
 			return [ a ];
 		}
 	}
 
+	// Case 2:
+	//
+	// When merge operation source position is at the same place as split position it needs to be decided whether
+	// the source position should stay in the original node or it should be moved as well to the new parent.
+	//
+	// Split and merge happens at `[]`:
+	// <h2>Foo</h2><p>[]Bar</p>
+	//
+	// After split, two possible solutions where merge can happen:
+	// <h2>Foo</h2><p>[]</p><p>Bar</p>
+	// <h2>Foo</h2><p></p><p>[]Bar</p>
+	//
+	// For collaboration it doesn't matter, however for undo it makes a difference because target position may
+	// not be in the element on the left, so bigger precision is needed for correct undo process. We will use
+	// relations to save if the undone merge affected operation `a`, and if so, we will correctly transform `a`.
+	//
+	if ( a.sourcePosition.isEqual( b.position ) && context.abRelation == 'mergeSameElement' ) {
+		a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );
+		a.sourcePosition = Position.createFromPosition( b.moveTargetPosition );
+
+		return [ a ];
+	}
+
 	// The default case.
 	//
 	if ( a.sourcePosition.hasSameParentAs( b.position ) ) {

tests/model/operation/transform/undo.js

@@ -13,7 +13,7 @@ describe( 'transform', () => {
 		return john.destroy();
 	} );
 
-	it( 'split, remove, undo, undo', () => {
+	it( 'split, remove', () => {
 		john.setData( '<paragraph>Foo[]Bar</paragraph>' );
 
 		john.split();
@@ -25,7 +25,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>FooBar</paragraph>' );
 	} );
 
-	it( 'move, merge, undo, undo', () => {
+	it( 'move, merge', () => {
 		john.setData( '[<paragraph>Foo</paragraph>]<paragraph>Bar</paragraph>' );
 
 		john.move( [ 2 ] );
@@ -37,7 +37,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
 	} );
 
-	it.skip( 'move multiple, merge, undo, undo', () => {
+	it.skip( 'move multiple, merge', () => {
 		john.setData( '[<paragraph>Foo</paragraph><paragraph>Bar</paragraph>]<paragraph>Xyz</paragraph>' );
 
 		john.move( [ 3 ] );
@@ -59,7 +59,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph><paragraph>Xyz</paragraph>' );
 	} );
 
-	it( 'move inside unwrapped content then undo', () => {
+	it( 'move inside unwrapped content', () => {
 		john.setData( '<blockQuote>[<paragraph>Foo</paragraph>]<paragraph>Bar</paragraph></blockQuote>' );
 
 		john.move( [ 0, 2 ] );
@@ -76,7 +76,7 @@ describe( 'transform', () => {
 		);
 	} );
 
-	it( 'remove node, merge then undo', () => {
+	it( 'remove node, merge', () => {
 		john.setData( '<paragraph>Foo</paragraph><paragraph>[Bar]</paragraph>' );
 
 		john.remove();
@@ -88,7 +88,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
 	} );
 
-	it( 'merge, merge then undo #1', () => {
+	it( 'merge, merge #1', () => {
 		john.setData(
 			'<blockQuote>' +
 				'<paragraph>Foo</paragraph>' +
@@ -125,7 +125,7 @@ describe( 'transform', () => {
 		);
 	} );
 
-	it( 'merge, merge then undo #2', () => {
+	it( 'merge, merge #2', () => {
 		john.setData(
 			'<blockQuote>' +
 				'<paragraph>Foo</paragraph>' +
@@ -162,7 +162,7 @@ describe( 'transform', () => {
 		);
 	} );
 
-	it( 'merge, unwrap then undo', () => {
+	it( 'merge, unwrap', () => {
 		john.setData( '<paragraph></paragraph>[]<paragraph>Foo</paragraph>' );
 
 		john.merge();
@@ -175,7 +175,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph></paragraph><paragraph>Foo</paragraph>' );
 	} );
 
-	it( 'remove node at the split position then undo #1', () => {
+	it( 'remove node at the split position #1', () => {
 		john.setData( '<paragraph>Ab</paragraph>[]<paragraph>Xy</paragraph>' );
 
 		john.merge();
@@ -188,7 +188,7 @@ describe( 'transform', () => {
 		expectClients( '<paragraph>Ab</paragraph><paragraph>Xy</paragraph>' );
 	} );
 
-	it( 'remove node at the split position then undo #2', () => {
+	it( 'remove node at the split position #2', () => {
 		john.setData( '<paragraph>Ab</paragraph>[]<paragraph>Xy</paragraph>' );
 
 		john.merge();
@@ -219,4 +219,60 @@ describe( 'transform', () => {
 
 		expectClients( '<paragraph>Foobar</paragraph>' );
 	} );
+
+	it( 'remove text from paragraph and merge it', () => {
+		john.setData( '<paragraph>Foo</paragraph><paragraph>[Bar]</paragraph>' );
+
+		john.remove();
+		john.setSelection( [ 1 ] );
+		john.merge();
+
+		expectClients( '<paragraph>Foo</paragraph>' );
+
+		john.undo();
+
+		expectClients( '<paragraph>Foo</paragraph><paragraph></paragraph>' );
+
+		john.undo();
+
+		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
+	} );
+
+	it( 'delete split paragraphs', () => {
+		john.setData( '<paragraph>Foo</paragraph><paragraph>B[]ar</paragraph>' );
+
+		john.split();
+		john.setSelection( [ 2, 1 ] );
+		john.split();
+		john.setSelection( [ 1, 0 ], [ 3, 1 ] );
+		john.delete();
+		john.setSelection( [ 1 ] );
+		john.merge();
+
+		expectClients( '<paragraph>Foo</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph></paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>B</paragraph><paragraph>a</paragraph><paragraph>r</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>B</paragraph><paragraph>ar</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
+
+		john.redo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>B</paragraph><paragraph>ar</paragraph>' );
+
+		john.redo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>B</paragraph><paragraph>a</paragraph><paragraph>r</paragraph>' );
+
+		john.redo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph></paragraph>' );
+
+		john.redo();
+		expectClients( '<paragraph>Foo</paragraph>' );
+	} );
 } );