diff --git a/src/model/model.js b/src/model/model.js index 8e8b747a3..f6532d549 100644 --- a/src/model/model.js +++ b/src/model/model.js @@ -349,7 +349,6 @@ export default class Model { * @fires deleteContent * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection * Selection of which the content should be deleted. - * @param {module:engine/model/batch~Batch} batch Batch to which the operations will be added. * @param {Object} [options] * @param {Boolean} [options.leaveUnmerged=false] Whether to merge elements after removing the content of the selection. * diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 02a4992fe..51852430b 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -505,6 +505,12 @@ class ContextFactory { break; } + + case SplitOperation: { + if ( opA.sourcePosition.isEqual( opB.splitPosition ) ) { + this._setRelation( opA, opB, 'splitAtSource' ); + } + } } break; @@ -1159,7 +1165,10 @@ setTransformation( MergeOperation, MergeOperation, ( a, b, context ) => { // // If neither or both operations point to graveyard, then let `aIsStrong` decide. // - if ( a.sourcePosition.isEqual( b.sourcePosition ) && !a.targetPosition.isEqual( b.targetPosition ) && !context.bWasUndone ) { + if ( + a.sourcePosition.isEqual( b.sourcePosition ) && !a.targetPosition.isEqual( b.targetPosition ) && + !context.bWasUndone && context.abRelation != 'splitAtSource' + ) { const aToGraveyard = a.targetPosition.root.rootName == '$graveyard'; const bToGraveyard = b.targetPosition.root.rootName == '$graveyard'; @@ -1338,7 +1347,7 @@ setTransformation( MergeOperation, SplitOperation, ( a, b, context ) => { // 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' ) { + if ( a.sourcePosition.isEqual( b.splitPosition ) && ( context.abRelation == 'mergeSameElement' || a.sourcePosition.offset > 0 ) ) { a.sourcePosition = Position.createFromPosition( b.moveTargetPosition ); a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b ); @@ -1396,9 +1405,9 @@ setTransformation( MoveOperation, MoveOperation, ( a, b, context ) => { let insertBefore = !context.aIsStrong; // If the relation is set, then use it to decide nodes order. - if ( context.abRelation == 'insertBefore' ) { + if ( context.abRelation == 'insertBefore' || context.baRelation == 'insertAfter' ) { insertBefore = true; - } else if ( context.abRelation == 'insertAfter' ) { + } else if ( context.abRelation == 'insertAfter' || context.baRelation == 'insertBefore' ) { insertBefore = false; } @@ -1683,7 +1692,16 @@ 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 ) { - return [ b.getReversed(), a ]; + const gyMoveTarget = Position.createFromPosition( b.graveyardPosition ); + const gyMove = new MoveOperation( b.graveyardPosition, 1, gyMoveTarget, 0 ); + + const targetPositionPath = b.graveyardPosition.path.slice(); + targetPositionPath.push( 0 ); + + return [ + gyMove, + new MoveOperation( b.targetPosition, b.howMany, new Position( a.targetPosition.root, targetPositionPath ), 0 ) + ]; } } else { // Case 2: @@ -1911,11 +1929,25 @@ setTransformation( SplitOperation, MergeOperation, ( a, b, context ) => { } ); setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { + const rangeToMove = Range.createFromPositionAndShift( b.sourcePosition, b.howMany ); + 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. + // + if ( rangeToMove.containsPosition( a.graveyardPosition ) || rangeToMove.start.isEqual( a.graveyardPosition ) ) { + a.insertionPosition = Position.createFromPosition( b.targetPosition ); + a.splitPosition = a.splitPosition._getTransformedByMoveOperation( b ); + + return [ a ]; + } + a.graveyardPosition = a.graveyardPosition._getTransformedByMoveOperation( b ); } - // Case 1: + // Case 2: // // 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. @@ -1932,8 +1964,6 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { // After split: // AdXbcyz // - const rangeToMove = Range.createFromPositionAndShift( b.sourcePosition, b.howMany ); - if ( a.splitPosition.hasSameParentAs( b.sourcePosition ) && rangeToMove.containsPosition( a.splitPosition ) ) { const howManyRemoved = b.howMany - ( a.splitPosition.offset - b.sourcePosition.offset ); a.howMany -= howManyRemoved; @@ -1948,7 +1978,7 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { return [ a ]; } - // Case 2: + // Case 3: // // Split is at a position where nodes were moved. // @@ -1966,13 +1996,16 @@ setTransformation( SplitOperation, MoveOperation, ( a, b, context ) => { } // The default case. + // Don't change `howMany` if move operation does not really move anything. // - if ( a.splitPosition.hasSameParentAs( b.sourcePosition ) && a.splitPosition.offset <= b.sourcePosition.offset ) { - a.howMany -= b.howMany; - } + if ( !b.sourcePosition.isEqual( b.targetPosition ) ) { + if ( a.splitPosition.hasSameParentAs( b.sourcePosition ) && a.splitPosition.offset <= b.sourcePosition.offset ) { + a.howMany -= b.howMany; + } - if ( a.splitPosition.hasSameParentAs( b.targetPosition ) && a.splitPosition.offset < b.targetPosition.offset ) { - a.howMany += b.howMany; + if ( a.splitPosition.hasSameParentAs( b.targetPosition ) && a.splitPosition.offset < b.targetPosition.offset ) { + a.howMany += b.howMany; + } } // Change position stickiness to force a correct transformation. diff --git a/tests/model/operation/transform/undo.js b/tests/model/operation/transform/undo.js index 0ebe051a9..63b328337 100644 --- a/tests/model/operation/transform/undo.js +++ b/tests/model/operation/transform/undo.js @@ -276,7 +276,7 @@ describe( 'transform', () => { expectClients( 'Foo' ); } ); - it( 'undo pasting', () => { + it( 'pasting on collapsed selection undo and redo', () => { john.setData( 'Foo[]Bar' ); // Below simulates pasting. @@ -297,8 +297,16 @@ describe( 'transform', () => { expectClients( 'Foo12Bar' ); john.undo(); + expectClients( 'FooBar' ); + john.redo(); + expectClients( 'Foo12Bar' ); + + john.undo(); expectClients( 'FooBar' ); + + john.redo(); + expectClients( 'Foo12Bar' ); } ); it( 'selection attribute setting: split, bold, merge, undo, undo, undo', () => { @@ -344,4 +352,40 @@ describe( 'transform', () => { 'XABCD' ); } ); + + // https://github.com/ckeditor/ckeditor5/issues/1287 TC1 + it( 'pasting on non-collapsed selection undo and redo', () => { + john.setData( 'Fo[oB]ar' ); + + // Below simulates pasting. + john.editor.model.change( () => { + john.editor.model.deleteContent( john.document.selection ); + + john.setSelection( [ 0, 2 ] ); + john.split(); + + john.setSelection( [ 1 ] ); + john.insert( '1' ); + + john.setSelection( [ 1 ] ); + john.merge(); + + john.setSelection( [ 1 ] ); + john.insert( '2' ); + + john.setSelection( [ 2 ] ); + john.merge(); + } ); + + expectClients( 'Fo12ar' ); + + john.undo(); + expectClients( 'FooBar' ); + + john.redo(); + expectClients( 'Fo12ar' ); + + john.undo(); + expectClients( 'FooBar' ); + } ); } );