Merge pull request #1054 from ckeditor/t/1053

Fixed: Fixed a bug when additional list item has been created when undoing applying block quote to a list followed by splitting list item in that list. Closes #1053.

Author: Reinmar

src/model/delta/transform.js

@@ -444,21 +444,21 @@ function _setForceNotSticky( context ) {
 // always treats `RemoveOperation` as a stronger one, no matter how `context.isStrong` is set. It is like this
 // to provide better results when transformations happen.
 //
-// This is however works fine only when additional context is not used.
+// This, however, works fine only when additional context is not used.
 //
 // When additional context is used, we need a better way to decide whether `RemoveOperation` is "dominating" (or in other
 // words, whether nodes removed by given operation should stay in graveyard if other operation wants to move them).
 //
 // The answer to this is easy: if `RemoveOperation` has been already undone, we are not forcing given nodes to stay
 // in graveyard. In such scenario, we set `context.forceWeakRemove` to `true`. However, if the `RemoveOperation` has
-// not been undone, we set `context.forceWeakRemove` to `false` because we want remove to be "dominating".
+// not been undone, we set `context.forceWeakRemove` to `false` because we want the operation to be "dominating".
 function _setForceWeakRemove( b, context ) {
 	const history = context.document.history;
 	const originalB = context.originalDelta.get( b );
 
-	// If `b` delta is a remove delta, that has not been undone yet, forceWeakRemove should be `false`.
-	// It should be `true` in any other case, if additional context is used.
-	context.forceWeakRemove = !( originalB instanceof RemoveDelta && !history.isUndoneDelta( originalB ) );
+	// If `b` delta has not been undone yet, forceWeakRemove should be `false`.
+	// It should be `true`, in any other case, if additional context is used.
+	context.forceWeakRemove = history.isUndoneDelta( originalB );
 }
 
 // Sets `context.wasAffected` which holds context information about how transformed deltas are related. `context.wasAffected`

src/model/operation/transform.js

@@ -377,13 +377,17 @@ const ot = {
 			const rangeA = Range.createFromPositionAndShift( a.sourcePosition, a.howMany );
 			const rangeB = Range.createFromPositionAndShift( b.sourcePosition, b.howMany );
 
+			// Assign `context.isStrong` to a different variable, because the value may change during execution of
+			// this algorithm and we do not want to override original `context.isStrong` that will be used in later transformations.
+			let isStrong = context.isStrong;
+
 			// Whether range moved by operation `b` is includable in operation `a` move range.
 			// For this, `a` operation has to be sticky (so `b` sticks to the range) and context has to allow stickiness.
 			const includeB = a.isSticky && !context.forceNotSticky;
 
 			// Evaluate new target position for transformed operation.
-			// Check whether there is a forced order of nodes or use `context.isStrong` flag for conflict resolving.
-			const insertBefore = context.insertBefore === undefined ? !context.isStrong : context.insertBefore;
+			// Check whether there is a forced order of nodes or use `isStrong` flag for conflict resolving.
+			const insertBefore = context.insertBefore === undefined ? !isStrong : context.insertBefore;
 
 			// `a.targetPosition` could be affected by the `b` operation. We will transform it.
 			const newTargetPosition = a.targetPosition._getTransformedByMove(
@@ -425,7 +429,7 @@ const ot = {
 				rangeA.start = rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
 				rangeA.end = rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
 
-				return makeMoveOperationsFromRanges( a, [ rangeA ], newTargetPosition );
+				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
 
 			//
@@ -441,7 +445,7 @@ const ot = {
 				rangeA.start = rangeA.start._getCombined( b.sourcePosition, b.getMovedRangeStart() );
 				rangeA.end = rangeA.end._getCombined( b.sourcePosition, b.getMovedRangeStart() );
 
-				return makeMoveOperationsFromRanges( a, [ rangeA ], newTargetPosition );
+				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
 			//
 			// End of special case #2.
@@ -463,7 +467,7 @@ const ot = {
 				rangeA.start = rangeA.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
 				rangeA.end = rangeA.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
 
-				return makeMoveOperationsFromRanges( a, [ rangeA ], newTargetPosition );
+				return makeMoveOperationsFromRanges( [ rangeA ], newTargetPosition, a );
 			}
 			//
 			// End of special case #3.
@@ -473,39 +477,43 @@ const ot = {
 			// Default case - ranges are on the same level or are not connected with each other.
 			//
 			// Modifier for default case.
-			// Modifies `context.isStrong` in certain conditions.
+			// Modifies `isStrong` flag in certain conditions.
 			//
 			// If only one of operations is a remove operation, we force remove operation to be the "stronger" one
 			// to provide more expected results. This is done only if `context.forceWeakRemove` is set to `false`.
 			// `context.forceWeakRemove` is set to `true` in certain conditions when transformation takes place during undo.
 			if ( !context.forceWeakRemove ) {
 				if ( a instanceof RemoveOperation && !( b instanceof RemoveOperation ) ) {
-					context.isStrong = true;
+					isStrong = true;
 				} else if ( !( a instanceof RemoveOperation ) && b instanceof RemoveOperation ) {
-					context.isStrong = false;
+					isStrong = false;
 				}
 			}
 
 			// Handle operation's source ranges - check how `rangeA` is affected by `b` operation.
 			// This will aggregate transformed ranges.
-			let ranges = [];
+			const ranges = [];
 
 			// Get the "difference part" of `a` operation source range.
 			// This is an array with one or two ranges. Two ranges if `rangeB` is inside `rangeA`.
 			const difference = rangeA.getDifference( rangeB );
 
 			for ( const range of difference ) {
 				// Transform those ranges by `b` operation. For example if `b` moved range from before those ranges, fix those ranges.
-				range.start = range.start._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, !includeB );
-				range.end = range.end._getTransformedByMove( b.sourcePosition, b.targetPosition, b.howMany, includeB );
+				range.start = range.start._getTransformedByDeletion( b.sourcePosition, b.howMany );
+				range.end = range.end._getTransformedByDeletion( b.sourcePosition, b.howMany );
+
+				// If `b` operation targets into `rangeA` on the same level, spread `rangeA` into two ranges.
+				const shouldSpread = compareArrays( range.start.getParentPath(), b.getMovedRangeStart().getParentPath() ) == 'same';
+				const newRanges = range._getTransformedByInsertion( b.getMovedRangeStart(), b.howMany, shouldSpread, includeB );
 
-				ranges.push( range );
+				ranges.push( ...newRanges );
 			}
 
 			// Then, we have to manage the "common part" of both move ranges.
 			const common = rangeA.getIntersection( rangeB );
 
-			if ( common !== null && context.isStrong && !bTargetsToA ) {
+			if ( common !== null && isStrong && !bTargetsToA ) {
 				// Calculate the new position of that part of original range.
 				common.start = common.start._getCombined( b.sourcePosition, b.getMovedRangeStart() );
 				common.end = common.end._getCombined( b.sourcePosition, b.getMovedRangeStart() );
@@ -521,7 +529,7 @@ const ot = {
 				}
 				// If there is one difference range, we need to check whether common part was before it or after it.
 				else if ( ranges.length == 1 ) {
-					if ( rangeB.start.isBefore( rangeA.start ) ) {
+					if ( rangeB.start.isBefore( rangeA.start ) || rangeB.start.isEqual( rangeA.start ) ) {
 						ranges.unshift( common );
 					} else {
 						ranges.push( common );
@@ -536,34 +544,11 @@ const ot = {
 
 			if ( ranges.length === 0 ) {
 				// If there are no "source ranges", nothing should be changed.
-				// Note that this can happen only if `context.isStrong == false` and `rangeA.isEqual( rangeB )`.
+				// Note that this can happen only if `isStrong == false` and `rangeA.isEqual( rangeB )`.
 				return [ new NoOperation( a.baseVersion ) ];
 			}
 
-			// At this moment we have some ranges and a target position, to which those ranges should be moved.
-			// Order in `ranges` array is the go-to order of after transformation. We can reverse the `ranges`
-			// array so the last range will be moved first. This will ensure that each transformed `MoveOperation`
-			// can have same `targetPosition` and the order of ranges will be kept.
-			ranges = ranges.reverse();
-
-			// We are almost done. We have `ranges` and `targetPosition` to make operations from.
-			// Unfortunately, those operations may affect each other. Precisely, first operation after move
-			// may affect source range of second and third operation. Same with second operation affecting third.
-			// We need to fix those source ranges once again, before converting `ranges` to operations.
-			// Keep in mind that `targetPosition` is already set in stone thanks to the trick above.
-			for ( let i = 1; i < ranges.length; i++ ) {
-				for ( let j = 0; j < i; j++ ) {
-					const howMany = ranges[ j ].end.offset - ranges[ j ].start.offset;
-
-					// Thankfully, all ranges in `ranges` array are:
-					// * non-intersecting (these are part of original `a` operation source range), and
-					// * `newTargetPosition` does not target into them (opposite would mean that `a` operation targets "inside itself").
-					// This means that the transformation will be "clean" and always return one result.
-					ranges[ i ] = ranges[ i ]._getTransformedByMove( ranges[ j ].start, newTargetPosition, howMany )[ 0 ];
-				}
-			}
-
-			return makeMoveOperationsFromRanges( a, ranges, newTargetPosition );
+			return makeMoveOperationsFromRanges( ranges, newTargetPosition, a );
 		}
 	}
 };
@@ -646,29 +631,66 @@ function joinRanges( ranges ) {
 	}
 }
 
-// Map transformed range(s) to operations and return them.
-function makeMoveOperationsFromRanges( a, ranges, targetPosition ) {
-	return ranges.map( range => {
-		// We want to keep correct operation class.
-		let OperationClass;
-
-		if ( targetPosition.root.rootName == '$graveyard' ) {
-			OperationClass = RemoveOperation;
-		} else if ( range.start.root.rootName == '$graveyard' ) {
-			OperationClass = ReinsertOperation;
-		} else {
-			OperationClass = MoveOperation;
+// Helper function for `MoveOperation` x `MoveOperation` transformation.
+// Convert given ranges and target position to move operations and return them.
+// Ranges and target position will be transformed on-the-fly when generating operations.
+// Given `ranges` should be in the order of how they were in the original transformed operation.
+// Given `targetPosition` is the target position of the first range from `ranges`.
+function makeMoveOperationsFromRanges( ranges, targetPosition, a ) {
+	// At this moment we have some ranges and a target position, to which those ranges should be moved.
+	// Order in `ranges` array is the go-to order of after transformation.
+	//
+	// We are almost done. We have `ranges` and `targetPosition` to make operations from.
+	// Unfortunately, those operations may affect each other. Precisely, first operation after move
+	// may affect source range and target position of second and third operation. Same with second
+	// operation affecting third.
+	//
+	// We need to fix those source ranges and target positions once again, before converting `ranges` to operations.
+	const operations = [];
+
+	// Keep in mind that nothing will be transformed if there is just one range in `ranges`.
+	for ( let i = 0; i < ranges.length; i++ ) {
+		// Create new operation out of a range and target position.
+		const op = makeMoveOperation( ranges[ i ], targetPosition, a.isSticky );
+
+		operations.push( op );
+
+		// Transform other ranges by the generated operation.
+		for ( let j = i + 1; j < ranges.length; j++ ) {
+			// All ranges in `ranges` array should be:
+			// * non-intersecting (these are part of original operation source range), and
+			// * `targetPosition` does not target into them (opposite would mean that transformed operation targets "inside itself").
+			//
+			// This means that the transformation will be "clean" and always return one result.
+			ranges[ j ] = ranges[ j ]._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany )[ 0 ];
 		}
 
-		const result = new OperationClass(
-			range.start,
-			range.end.offset - range.start.offset,
-			targetPosition,
-			0 // Is corrected anyway later.
-		);
+		targetPosition = targetPosition._getTransformedByMove( op.sourcePosition, op.targetPosition, op.howMany, true, false );
+	}
+
+	return operations;
+}
+
+function makeMoveOperation( range, targetPosition, isSticky ) {
+	// We want to keep correct operation class.
+	let OperationClass;
+
+	if ( targetPosition.root.rootName == '$graveyard' ) {
+		OperationClass = RemoveOperation;
+	} else if ( range.start.root.rootName == '$graveyard' ) {
+		OperationClass = ReinsertOperation;
+	} else {
+		OperationClass = MoveOperation;
+	}
+
+	const result = new OperationClass(
+		range.start,
+		range.end.offset - range.start.offset,
+		targetPosition,
+		0 // Is corrected anyway later.
+	);
 
-		result.isSticky = a.isSticky;
+	result.isSticky = isSticky;
 
-		return result;
-	} );
+	return result;
 }

tests/model/delta/transform/transform.js

@@ -17,6 +17,7 @@ import InsertDelta from '../../../../src/model/delta/insertdelta';
 import RemoveDelta from '../../../../src/model/delta/removedelta';
 import MoveDelta from '../../../../src/model/delta/movedelta';
 import SplitDelta from '../../../../src/model/delta/splitdelta';
+import UnwrapDelta from '../../../../src/model/delta/unwrapdelta';
 
 import NoOperation from '../../../../src/model/operation/nooperation';
 import MoveOperation from '../../../../src/model/operation/moveoperation';
@@ -30,7 +31,8 @@ import {
 	getSplitDelta,
 	getRemoveDelta,
 	getMoveDelta,
-	getMergeDelta
+	getMergeDelta,
+	getUnwrapDelta
 } from '../../../../tests/model/delta/transform/_utils/utils';
 
 describe( 'transform', () => {
@@ -115,7 +117,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 1, 0 ] ),
+						sourcePosition: new Position( root, [ 0, 2 ] ),
 						howMany: 1,
 						baseVersion: 3
 					}
@@ -127,7 +129,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 0, 2 ] ),
+						sourcePosition: new Position( root, [ 1, 0 ] ),
 						howMany: 1,
 						baseVersion: 4
 					}
@@ -163,7 +165,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 1, 0 ] ),
+						sourcePosition: new Position( root, [ 0, 2 ] ),
 						howMany: 1,
 						baseVersion: 3
 					}
@@ -175,7 +177,7 @@ describe( 'transform', () => {
 				operations: [
 					{
 						type: RemoveOperation,
-						sourcePosition: new Position( root, [ 0, 2 ] ),
+						sourcePosition: new Position( root, [ 1, 0 ] ),
 						howMany: 1,
 						baseVersion: 4
 					}
@@ -341,6 +343,51 @@ describe( 'transform', () => {
 				} );
 			} );
 
+			// #1053.
+			it( 'remove operation importance should be set correctly also for deltas other than remove delta', () => {
+				const unwrapDelta = getUnwrapDelta( new Position( root, [ 0 ] ), 2, baseVersion );
+
+				const nodeCopy = new Element( 'p' );
+				const splitDelta = getSplitDelta( new Position( root, [ 0, 0, 1 ] ), nodeCopy, 1, baseVersion );
+				const mergeDelta = splitDelta.getReversed();
+
+				const { deltasA } = transformDeltaSets( [ unwrapDelta ], [ splitDelta, mergeDelta ], doc );
+
+				expect( deltasA.length ).to.equal( 2 );
+
+				expectDelta( deltasA[ 0 ], {
+					type: MoveDelta,
+					operations: [
+						{
+							type: MoveOperation,
+							sourcePosition: new Position( root, [ 0, 0 ] ),
+							howMany: 1,
+							targetPosition: new Position( root, [ 0 ] ),
+							baseVersion: baseVersion + 4
+						}
+					]
+				} );
+
+				expectDelta( deltasA[ 1 ], {
+					type: UnwrapDelta,
+					operations: [
+						{
+							type: MoveOperation,
+							sourcePosition: new Position( root, [ 1, 0 ] ),
+							howMany: 1,
+							targetPosition: new Position( root, [ 1 ] ),
+							baseVersion: baseVersion + 5
+						},
+						{
+							type: RemoveOperation,
+							sourcePosition: new Position( root, [ 2 ] ),
+							howMany: 1,
+							baseVersion: baseVersion + 6
+						}
+					]
+				} );
+			} );
+
 			it( 'should keep correct order of nodes if additional context is used (insert before)', () => {
 				// Assume document state: 'a^bcd'. User presses 'delete' key twice.
 				const removeDelta1 = getRemoveDelta( new Position( root, [ 1 ] ), 1, baseVersion ); // 'acd'.