Merge pull request #1052 from ckeditor/t/1051

Fixed: Fixed a bug when renaming followed by merge or split resulted in multiple elements being incorrectly renamed during undo. Closes #1051.

Author: Reinmar

src/model/delta/basic-transformations.js

@@ -348,15 +348,16 @@ addTransformationCase( WrapDelta, SplitDelta, ( a, b, context ) => {
 
 // Add special case for RenameDelta x SplitDelta transformation.
 addTransformationCase( RenameDelta, SplitDelta, ( a, b, context ) => {
-	const splitPosition = new Position( b.position.root, b.position.path.slice( 0, -1 ) );
+	const undoMode = context.aWasUndone || context.bWasUndone;
+	const posBeforeSplitParent = new Position( b.position.root, b.position.path.slice( 0, -1 ) );
 
 	const deltas = defaultTransform( a, b, context );
 
-	if ( a.operations[ 0 ].position.isEqual( splitPosition ) ) {
+	if ( !undoMode && a.operations[ 0 ].position.isEqual( posBeforeSplitParent ) ) {
 		// If a node that has been split has it's name changed, we should also change name of
 		// the node created during splitting.
 		const additionalRenameDelta = a.clone();
-		additionalRenameDelta.operations[ 0 ].position = a.operations[ 0 ].position.getShiftedBy( 1 );
+		additionalRenameDelta.operations[ 0 ].position = posBeforeSplitParent.getShiftedBy( 1 );
 
 		deltas.push( additionalRenameDelta );
 	}
@@ -365,20 +366,25 @@ addTransformationCase( RenameDelta, SplitDelta, ( a, b, context ) => {
 } );
 
 // Add special case for SplitDelta x RenameDelta transformation.
-addTransformationCase( SplitDelta, RenameDelta, ( a, b ) => {
-	a = a.clone();
-
-	const splitPosition = new Position( a.position.root, a.position.path.slice( 0, -1 ) );
-
-	if ( a._cloneOperation instanceof InsertOperation ) {
-		// If element to split had it's name changed, we have to reflect this change in an element
-		// that is in SplitDelta's InsertOperation.
-		if ( b.operations[ 0 ].position.isEqual( splitPosition ) ) {
-			a._cloneOperation.nodes.getNode( 0 ).name = b.operations[ 0 ].newName;
-		}
+addTransformationCase( SplitDelta, RenameDelta, ( a, b, context ) => {
+	const undoMode = context.aWasUndone || context.bWasUndone;
+	const posBeforeSplitParent = new Position( a.position.root, a.position.path.slice( 0, -1 ) );
+
+	// If element to split had it's name changed, we have to reflect this by creating additional rename operation.
+	if ( !undoMode && b.operations[ 0 ].position.isEqual( posBeforeSplitParent ) ) {
+		const additionalRenameDelta = b.clone();
+		additionalRenameDelta.operations[ 0 ].position = posBeforeSplitParent.getShiftedBy( 1 );
+
+		// `nodes` is a property that is available only if `SplitDelta` `a` has `InsertOperation`.
+		// `SplitDelta` may have `ReinsertOperation` instead of `InsertOperation`.
+		// However, such delta is only created when `MergeDelta` is reversed.
+		// So if this is not undo mode, it means that `SplitDelta` has `InsertOperation`.
+		additionalRenameDelta.operations[ 0 ].oldName = a._cloneOperation.nodes.getNode( 0 ).name;
+
+		return [ a.clone(), additionalRenameDelta ];
 	}
 
-	return [ a ];
+	return [ a.clone() ];
 } );
 
 // Add special case for RemoveDelta x SplitDelta transformation.

src/model/delta/transform.js

@@ -262,14 +262,18 @@ const transform = {
 							insertBefore: contextAB.insertBefore,
 							forceNotSticky: contextAB.forceNotSticky,
 							isStrong: contextAB.isStrong,
-							forceWeakRemove: contextAB.forceWeakRemove
+							forceWeakRemove: contextAB.forceWeakRemove,
+							aWasUndone: false,
+							bWasUndone: contextAB.bWasUndone
 						} );
 
 						const resultBA = transform.transform( deltaB[ l ], deltaA[ k ], {
 							insertBefore: !contextAB.insertBefore,
 							forceNotSticky: contextAB.forceNotSticky,
 							isStrong: !contextAB.isStrong,
-							forceWeakRemove: contextAB.forceWeakRemove
+							forceWeakRemove: contextAB.forceWeakRemove,
+							aWasUndone: contextAB.bWasUndone,
+							bWasUndone: false
 						} );
 
 						if ( useAdditionalContext ) {
@@ -350,10 +354,21 @@ function padWithNoOps( deltas, howMany ) {
 // Using data given in `context` object, sets `context.insertBefore` and `context.forceNotSticky` flags.
 // Also updates `context.wasAffected`.
 function _setContext( a, b, context ) {
+	_setBWasUndone( b, context );
 	_setWasAffected( a, b, context );
 	_setInsertBeforeContext( a, b, context );
 	_setForceWeakRemove( b, context );
-	_setForceNotSticky( b, context );
+	_setForceNotSticky( context );
+}
+
+// Sets `context.bWasUndone` basing on `context.document` history for `b` delta.
+//
+// `context.bWasUndone` is set to `true` if the (originally transformed) `b` delta was undone or was undoing delta.
+function _setBWasUndone( b, context ) {
+	const originalDelta = context.originalDelta.get( b );
+	const history = context.document.history;
+
+	context.bWasUndone = history.isUndoneDelta( originalDelta ) || history.isUndoingDelta( originalDelta );
 }
 
 // Sets `context.insertBefore` basing on `context.document` history for `a` by `b` transformation.
@@ -417,12 +432,8 @@ function _setInsertBeforeContext( a, b, context ) {
 // if delta `b` was already undone or if delta `b` is an undoing delta.
 //
 // This affects `MoveOperation` (and its derivatives).
-function _setForceNotSticky( b, context ) {
-	const history = context.document.history;
-	const originalB = context.originalDelta.get( b );
-
-	// If `b` delta is undoing or undone delta, stickiness should not be taken into consideration.
-	if ( history.isUndoingDelta( originalB ) || history.isUndoneDelta( originalB ) ) {
+function _setForceNotSticky( context ) {
+	if ( context.bWasUndone ) {
 		context.forceNotSticky = true;
 	}
 }

tests/model/delta/transform/splitdelta.js

@@ -625,14 +625,14 @@ describe( 'transform', () => {
 			it( 'renamed split element', () => {
 				const renameDelta = new RenameDelta();
 				renameDelta.addOperation( new RenameOperation(
-					new Position( root, [ 3, 3, 3 ] ), 'p', 'li', baseVersion
+					new Position( root, [ 3, 3, 3 ] ), 'h2', 'li', baseVersion
 				) );
 
 				const transformed = transform( splitDelta, renameDelta, context );
 
 				baseVersion = renameDelta.operations.length;
 
-				expect( transformed.length ).to.equal( 1 );
+				expect( transformed.length ).to.equal( 2 );
 
 				expectDelta( transformed[ 0 ], {
 					type: SplitDelta,
@@ -652,7 +652,18 @@ describe( 'transform', () => {
 					]
 				} );
 
-				expect( transformed[ 0 ].operations[ 0 ].nodes.getNode( 0 ).name ).to.equal( 'li' );
+				expectDelta( transformed[ 1 ], {
+					type: RenameDelta,
+					operations: [
+						{
+							type: RenameOperation,
+							position: new Position( root, [ 3, 3, 4 ] ),
+							oldName: 'p', // `oldName` taken from SplitDelta.
+							newName: 'li',
+							baseVersion: baseVersion + 2
+						}
+					]
+				} );
 			} );
 
 			it( 'split element is different than renamed element', () => {
@@ -699,6 +710,7 @@ describe( 'transform', () => {
 					new Position( root, [ 3, 3, 3 ] ), 'p', 'li', baseVersion
 				) );
 
+				context.aWasUndone = true;
 				const transformed = transform( splitDelta, renameDelta, context );
 
 				baseVersion = renameDelta.operations.length;