Merge pull request #1066 from ckeditor/t/1065

Fix: Prevented editor throwing during SplitDelta x RemoveDelta transformation when SplitDelta's first operation was neither InsertOperation nor ReinsertOperation. Closes #1065.
Fix: Fixed remove model-to-view converter for some edge cases. Closes #1068.

Author: Piotr Jasiun

src/conversion/model-to-view-converters.js

@@ -6,6 +6,7 @@
 import ViewElement from '../view/element';
 import ViewText from '../view/text';
 import ViewRange from '../view/range';
+import ViewPosition from '../view/position';
 import ViewTreeWalker from '../view/treewalker';
 import viewWriter from '../view/writer';
 
@@ -441,13 +442,22 @@ export function remove() {
 		// end of that range is incorrect.
 		// Instead we will use `data.sourcePosition` as this is the last correct model position and
 		// it is a position before the removed item. Then, we will calculate view range to remove "manually".
-		const viewPosition = conversionApi.mapper.toViewPosition( data.sourcePosition );
+		let viewPosition = conversionApi.mapper.toViewPosition( data.sourcePosition );
 		let viewRange;
 
 		if ( data.item.is( 'element' ) ) {
 			// Note: in remove conversion we cannot use model-to-view element mapping because `data.item` may be
 			// already mapped to another element (this happens when move change is converted).
 			// In this case however, `viewPosition` is the position before view element that corresponds to removed model element.
+			//
+			// First, fix the position. Traverse the tree forward until the container element is found. The `viewPosition`
+			// may be before a ui element, before attribute element or at the end of text element.
+			viewPosition = viewPosition.getLastMatchingPosition( value => !value.item.is( 'containerElement' ) );
+
+			if ( viewPosition.parent.is( 'text' ) && viewPosition.isAtEnd ) {
+				viewPosition = ViewPosition.createAfter( viewPosition.parent );
+			}
+
 			viewRange = ViewRange.createOn( viewPosition.nodeAfter );
 		} else {
 			// If removed item is a text node, we need to traverse view tree to find the view range to remove.

src/model/delta/basic-transformations.js

@@ -390,7 +390,13 @@ addTransformationCase( SplitDelta, RenameDelta, ( a, b, context ) => {
 // Add special case for RemoveDelta x SplitDelta transformation.
 addTransformationCase( RemoveDelta, SplitDelta, ( a, b, context ) => {
 	const deltas = defaultTransform( a, b, context );
-	const insertPosition = b._cloneOperation.position;
+	// The "clone operation" may be InsertOperation, ReinsertOperation, MoveOperation or NoOperation.
+	const insertPosition = b._cloneOperation.position || b._cloneOperation.targetPosition;
+
+	// NoOperation.
+	if ( !insertPosition ) {
+		return defaultTransform( a, b, context );
+	}
 
 	// In case if `defaultTransform` returned more than one delta.
 	for ( const delta of deltas ) {
@@ -413,9 +419,16 @@ addTransformationCase( SplitDelta, RemoveDelta, ( a, b, context ) => {
 	// This case is very trickily solved.
 	// Instead of fixing `a` delta, we change `b` delta for a while and fire default transformation with fixed `b` delta.
 	// Thanks to that fixing `a` delta will be differently (correctly) transformed.
-	b = b.clone();
+	//
+	// The "clone operation" may be InsertOperation, ReinsertOperation, MoveOperation or NoOperation.
+	const insertPosition = a._cloneOperation.position || a._cloneOperation.targetPosition;
 
-	const insertPosition = a._cloneOperation.position;
+	// NoOperation.
+	if ( !insertPosition ) {
+		return defaultTransform( a, b, context );
+	}
+
+	b = b.clone();
 	const operation = b._moveOperation;
 	const rangeEnd = operation.sourcePosition.getShiftedBy( operation.howMany );
 

tests/conversion/model-to-view-converters.js

@@ -1028,8 +1028,8 @@ describe( 'model-to-view-converters', () => {
 		} );
 
 		it( 'should not unbind element that has not been moved to graveyard', () => {
-			const modelElement = new ModelElement( 'a' );
-			const viewElement = new ViewElement( 'a' );
+			const modelElement = new ModelElement( 'paragraph' );
+			const viewElement = new ViewContainerElement( 'p' );
 
 			modelRoot.appendChildren( [ modelElement, new ModelText( 'b' ) ] );
 			viewRoot.appendChildren( [ viewElement, new ViewText( 'b' ) ] );
@@ -1056,8 +1056,8 @@ describe( 'model-to-view-converters', () => {
 		} );
 
 		it( 'should unbind elements if model element was moved to graveyard', () => {
-			const modelElement = new ModelElement( 'a' );
-			const viewElement = new ViewElement( 'a' );
+			const modelElement = new ModelElement( 'paragraph' );
+			const viewElement = new ViewContainerElement( 'p' );
 
 			modelRoot.appendChildren( [ modelElement, new ModelText( 'b' ) ] );
 			viewRoot.appendChildren( [ viewElement, new ViewText( 'b' ) ] );
@@ -1125,5 +1125,62 @@ describe( 'model-to-view-converters', () => {
 			expect( mapper.toModelElement( viewWElement ) ).to.be.undefined;
 			expect( mapper.toViewElement( modelWElement ) ).to.be.undefined;
 		} );
+
+		it( 'should work correctly if container element after ui element is removed', () => {
+			const modelP1 = new ModelElement( 'paragraph' );
+			const modelP2 = new ModelElement( 'paragraph' );
+
+			const viewP1 = new ViewContainerElement( 'p' );
+			const viewUi1 = new ViewUIElement( 'span' );
+			const viewUi2 = new ViewUIElement( 'span' );
+			const viewP2 = new ViewContainerElement( 'p' );
+
+			modelRoot.appendChildren( [ modelP1, modelP2 ] );
+			viewRoot.appendChildren( [ viewP1, viewUi1, viewUi2, viewP2 ] );
+
+			mapper.bindElements( modelP1, viewP1 );
+			mapper.bindElements( modelP2, viewP2 );
+
+			dispatcher.on( 'remove', remove() );
+
+			modelWriter.move(
+				ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 2 ),
+				ModelPosition.createAt( modelDoc.graveyard, 'end' )
+			);
+
+			dispatcher.convertRemove(
+				ModelPosition.createFromParentAndOffset( modelRoot, 1 ),
+				ModelRange.createFromParentsAndOffsets( modelDoc.graveyard, 0, modelDoc.graveyard, 1 )
+			);
+
+			expect( viewToString( viewRoot ) ).to.equal( '<div><p></p><span></span><span></span></div>' );
+		} );
+
+		it( 'should work correctly if container element after text node is removed', () => {
+			const modelText = new ModelText( 'foo' );
+			const modelP = new ModelElement( 'paragraph' );
+
+			const viewText = new ViewText( 'foo' );
+			const viewP = new ViewContainerElement( 'p' );
+
+			modelRoot.appendChildren( [ modelText, modelP ] );
+			viewRoot.appendChildren( [ viewText, viewP ] );
+
+			mapper.bindElements( modelP, viewP );
+
+			dispatcher.on( 'remove', remove() );
+
+			modelWriter.move(
+				ModelRange.createFromParentsAndOffsets( modelRoot, 3, modelRoot, 4 ),
+				ModelPosition.createAt( modelDoc.graveyard, 'end' )
+			);
+
+			dispatcher.convertRemove(
+				ModelPosition.createFromParentAndOffset( modelRoot, 3 ),
+				ModelRange.createFromParentsAndOffsets( modelDoc.graveyard, 0, modelDoc.graveyard, 1 )
+			);
+
+			expect( viewToString( viewRoot ) ).to.equal( '<div>foo</div>' );
+		} );
 	} );
 } );

tests/model/delta/transform/removedelta.js

@@ -184,6 +184,30 @@ describe( 'transform', () => {
 					]
 				} );
 			} );
+
+			it( 'should not throw if clone operation is NoOperation and use default transformation in that case', () => {
+				const noOpSplitDelta = new SplitDelta();
+				noOpSplitDelta.addOperation( new NoOperation( 0 ) );
+				noOpSplitDelta.addOperation( new MoveOperation( new Position( root, [ 1, 2 ] ), 3, new Position( root, [ 2, 0 ] ), 1 ) );
+
+				const removeDelta = getRemoveDelta( new Position( root, [ 3 ] ), 1, 0 );
+
+				const transformed = transform( removeDelta, noOpSplitDelta, context );
+
+				expect( transformed.length ).to.equal( 1 );
+
+				expectDelta( transformed[ 0 ], {
+					type: RemoveDelta,
+					operations: [
+						{
+							type: RemoveOperation,
+							sourcePosition: new Position( root, [ 3 ] ),
+							howMany: 1,
+							baseVersion: 2
+						}
+					]
+				} );
+			} );
 		} );
 	} );
 } );

tests/model/delta/transform/splitdelta.js

@@ -814,6 +814,35 @@ describe( 'transform', () => {
 					]
 				} );
 			} );
+
+			it( 'should not throw if clone operation is NoOperation and use default transformation in that case', () => {
+				const noOpSplitDelta = new SplitDelta();
+				noOpSplitDelta.addOperation( new NoOperation( 0 ) );
+				noOpSplitDelta.addOperation( new MoveOperation( new Position( root, [ 1, 2 ] ), 3, new Position( root, [ 2, 0 ] ), 1 ) );
+
+				const removeDelta = getRemoveDelta( new Position( root, [ 0 ] ), 1, 0 );
+
+				const transformed = transform( noOpSplitDelta, removeDelta, context );
+
+				expect( transformed.length ).to.equal( 1 );
+
+				expectDelta( transformed[ 0 ], {
+					type: SplitDelta,
+					operations: [
+						{
+							type: NoOperation,
+							baseVersion: 1
+						},
+						{
+							type: MoveOperation,
+							sourcePosition: new Position( root, [ 0, 2 ] ),
+							howMany: 3,
+							targetPosition: new Position( root, [ 1, 0 ] ),
+							baseVersion: 2
+						}
+					]
+				} );
+			} );
 		} );
 	} );
 } );