Merge pull request #1573 from ckeditor/t/ckeditor5/1278

Fix: `Range#_getTransformedBySplitOperation()` will expand the range if `insertionPosition` is equal to the range end. Modified transformations to align with that change. Closes ckeditor/ckeditor5#1278.

Author: Reinmar

src/model/operation/transform.js

@@ -497,6 +497,10 @@ class ContextFactory {
 							this._setRelation( opA, opB, 'mergeTargetNotMoved' );
 						}
 
+						if ( opA.sourcePosition.isEqual( opB.sourcePosition ) ) {
+							this._setRelation( opA, opB, 'mergeSameElement' );
+						}
+
 						break;
 					}
 				}
@@ -905,8 +909,10 @@ setTransformation( AttributeOperation, SplitOperation, ( a, b ) => {
 	// After attribute change:
 	// <listItem type="numbered">foo</listItem><listItem type="numbered">foo</listItem>
 	//
-	if ( a.range.end.isEqual( b.insertionPosition ) && !b.graveyardPosition ) {
-		a.range.end.offset++;
+	if ( a.range.end.isEqual( b.insertionPosition ) ) {
+		if ( !b.graveyardPosition ) {
+			a.range.end.offset++;
+		}
 
 		return [ a ];
 	}
@@ -1320,6 +1326,23 @@ setTransformation( MergeOperation, SplitOperation, ( a, b, context ) => {
 		}
 	}
 
+	// Case 2:
+	//
+	// Merge source is at the same position as split position. This sometimes happen during undo. This merge operation
+	// might have been earlier transformed by a merge operation which both merged the same element. See case in
+	// `MergeOperation` x `MergeOperation` transformation. In that case, if the merge operation has been undone, the special
+	// case is not applied.
+	//
+	// In this scenario the merge operation is now transformed by the split which has undone the previous merge operation.
+	// So now we are fixing situation which was skipped in `MergeOperation` x `MergeOperation` case.
+	//
+	if ( a.sourcePosition.isEqual( b.splitPosition ) && context.abRelation == 'mergeSameElement' ) {
+		a.sourcePosition = Position.createFromPosition( b.moveTargetPosition );
+		a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );
+
+		return [ a ];
+	}
+
 	// The default case.
 	//
 	if ( a.sourcePosition.hasSameParentAs( b.splitPosition ) ) {
@@ -1555,13 +1578,16 @@ setTransformation( MoveOperation, SplitOperation, ( a, b, context ) => {
 	//
 	// In this case the default range transformation will not work correctly as the element created by
 	// split operation would be outside the range. The range to move needs to be fixed manually.
-	// Do it only if this is a "natural" split, not a one that comes from undo.
-	// TODO: this should be done on relations
 	//
 	const moveRange = Range.createFromPositionAndShift( a.sourcePosition, a.howMany );
 
-	if ( moveRange.end.isEqual( b.insertionPosition ) && !b.graveyardPosition ) {
-		a.howMany++;
+	if ( moveRange.end.isEqual( b.insertionPosition ) ) {
+		// Do it only if this is a "natural" split, not a one that comes from undo.
+		// If this is undo split, only `targetPosition` needs to be changed (if the move is a remove).
+		if ( !b.graveyardPosition ) {
+			a.howMany++;
+		}
+
 		a.targetPosition = newTargetPosition;
 
 		return [ a ];

src/model/range.js

@@ -517,7 +517,14 @@ export default class Range {
 	 */
 	_getTransformedBySplitOperation( operation ) {
 		const start = this.start._getTransformedBySplitOperation( operation );
-		const end = this.end._getTransformedBySplitOperation( operation );
+
+		let end;
+
+		if ( this.end.isEqual( operation.insertionPosition ) ) {
+			end = this.end.getShiftedBy( 1 );
+		} else {
+			end = this.end._getTransformedBySplitOperation( operation );
+		}
 
 		return new Range( start, end );
 	}

tests/model/operation/transform/undo.js

@@ -275,4 +275,50 @@ describe( 'transform', () => {
 		john.redo();
 		expectClients( '<paragraph>Foo</paragraph>' );
 	} );
+
+	it( 'undo pasting', () => {
+		john.setData( '<paragraph>Foo[]Bar</paragraph>' );
+
+		// Below simulates pasting.
+		john.editor.model.change( () => {
+			john.split();
+			john.setSelection( [ 1 ] );
+
+			john.insert( '<paragraph>1</paragraph>' );
+			john.setSelection( [ 1 ] );
+			john.merge();
+
+			john.setSelection( [ 1 ] );
+			john.insert( '<paragraph>2</paragraph>' );
+			john.setSelection( [ 2 ] );
+			john.merge();
+		} );
+
+		expectClients( '<paragraph>Foo1</paragraph><paragraph>2Bar</paragraph>' );
+
+		john.undo();
+
+		expectClients( '<paragraph>FooBar</paragraph>' );
+	} );
+
+	it( 'selection attribute setting: split, bold, merge, undo, undo, undo', () => {
+		// This test is ported from undo to keep 100% CC in engine.
+		john.setData( '<paragraph>Foo[]</paragraph><paragraph>Bar</paragraph>' );
+
+		john.split();
+		john.setSelection( [ 1, 0 ] );
+		john._processExecute( 'bold' );
+		john._processExecute( 'forwardDelete' );
+
+		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph selection:bold="true"></paragraph><paragraph>Bar</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph></paragraph><paragraph>Bar</paragraph>' );
+
+		john.undo();
+		expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
+	} );
 } );

tests/model/operation/transform/utils.js

@@ -1,6 +1,7 @@
 import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
 
 import ListEditing from '@ckeditor/ckeditor5-list/src/listediting';
+import BoldEditing from '@ckeditor/ckeditor5-basic-styles/src/bold/boldediting';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import Typing from '@ckeditor/ckeditor5-typing/src/typing';
 import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting';
@@ -30,7 +31,8 @@ export class Client {
 			// Typing is needed for delete command.
 			// UndoEditing is needed for undo command.
 			// Block plugins are needed for proper data serializing.
-			plugins: [ Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing, HeadingEditing ]
+			// BoldEditing is needed for bold command.
+			plugins: [ Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing, HeadingEditing, BoldEditing ]
 		} ).then( editor => {
 			this.editor = editor;
 			this.document = editor.model.document;

tests/model/range.js

@@ -1038,6 +1038,17 @@ describe( 'Range', () => {
 				expect( transformed[ 0 ].start.path ).to.deep.equal( [ 1, 3 ] );
 				expect( transformed[ 0 ].end.path ).to.deep.equal( [ 1, 3 ] );
 			} );
+
+			it( 'split element which is the last element in the range', () => {
+				range = new Range( new Position( root, [ 1 ] ), new Position( root, [ 3 ] ) );
+
+				const op = new SplitOperation( new Position( root, [ 2, 0 ] ), 6, gyPos, 1 );
+				const transformed = range.getTransformedByOperation( op );
+
+				expect( transformed.length ).to.equal( 1 );
+				expect( transformed[ 0 ].start.path ).to.deep.equal( [ 1 ] );
+				expect( transformed[ 0 ].end.path ).to.deep.equal( [ 4 ] );
+			} );
 		} );
 
 		describe( 'by MergeOperation', () => {