Merge pull request #873 from ckeditor/t/847

Fix: Fixed various issues with the move and unwrap deltas conversion. Closes #847.

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

@@ -440,7 +440,23 @@ export function remove() {
 		const viewRange = conversionApi.mapper.toViewRange( modelRange );
 
 		viewWriter.remove( viewRange.getTrimmed() );
-		conversionApi.mapper.unbindModelElement( data.item );
+
+		// Unbind this element only if it was moved to graveyard.
+		// The dispatcher#remove event will also be fired if the element was moved to another place (remove+insert are fired).
+		// Let's say that <b> is moved before <a>. The view will be changed like this:
+		//
+		// 1) start:    <a></a><b></b>
+		// 2) insert:   <b (new)></b><a></a><b></b>
+		// 3) remove:   <b (new)></b><a></a>
+		//
+		// If we'll unbind the <b> element in step 3 we'll also lose binding of the <b (new)> element in the view,
+		// because unbindModelElement() cancels both bindings – (model <b> => view <b (new)>) and (view <b (new)> => model <b>).
+		// We can't lose any of these.
+		//
+		// See #847.
+		if ( data.item.root.rootName == '$graveyard' ) {
+			conversionApi.mapper.unbindModelElement( data.item );
+		}
 	};
 }
 

src/conversion/modelconversiondispatcher.js

@@ -241,8 +241,18 @@ export default class ModelConversionDispatcher {
 	 * @param {module:engine/model/range~Range} range The range containing the moved content.
 	 */
 	convertMove( sourcePosition, range ) {
-		this.convertRemove( sourcePosition, range );
-		this.convertInsertion( range );
+		// Move left – convert insertion first (#847).
+		if ( range.start.isBefore( sourcePosition ) ) {
+			this.convertInsertion( range );
+
+			const sourcePositionAfterInsertion
+				= sourcePosition._getTransformedByInsertion( range.start, range.end.offset - range.start.offset );
+
+			this.convertRemove( sourcePositionAfterInsertion, range );
+		} else {
+			this.convertRemove( sourcePosition, range );
+			this.convertInsertion( range );
+		}
 	}
 
 	/**

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

@@ -1002,5 +1002,104 @@ describe( 'model-to-view-converters', () => {
 
 			expect( viewToString( viewRoot ) ).to.equal( '<div>foo<span></span>ar</div>' );
 		} );
+
+		it( 'should not unbind element that has not been moved to graveyard', () => {
+			const modelElement = new ModelElement( 'a' );
+			const viewElement = new ViewElement( 'a' );
+
+			modelRoot.appendChildren( [ modelElement, new ModelText( 'b' ) ] );
+			viewRoot.appendChildren( [ viewElement, new ViewText( 'b' ) ] );
+
+			mapper.bindElements( modelElement, viewElement );
+
+			dispatcher.on( 'remove', remove() );
+
+			// Move <a></a> after "b". Can be e.g. a part of an unwrap delta (move + remove).
+			modelWriter.move(
+				ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ),
+				ModelPosition.createAt( modelRoot, 'end' )
+			);
+
+			dispatcher.convertRemove(
+				ModelPosition.createFromParentAndOffset( modelRoot, 0 ),
+				ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 2 )
+			);
+
+			expect( viewToString( viewRoot ) ).to.equal( '<div>b</div>' );
+
+			expect( mapper.toModelElement( viewElement ) ).to.equal( modelElement );
+			expect( mapper.toViewElement( modelElement ) ).to.equal( viewElement );
+		} );
+
+		it( 'should unbind elements if model element was moved to graveyard', () => {
+			const modelElement = new ModelElement( 'a' );
+			const viewElement = new ViewElement( 'a' );
+
+			modelRoot.appendChildren( [ modelElement, new ModelText( 'b' ) ] );
+			viewRoot.appendChildren( [ viewElement, new ViewText( 'b' ) ] );
+
+			mapper.bindElements( modelElement, viewElement );
+
+			dispatcher.on( 'remove', remove() );
+
+			// Move <a></a> to graveyard.
+			modelWriter.move(
+				ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ),
+				ModelPosition.createAt( modelDoc.graveyard, 'end' )
+			);
+
+			dispatcher.convertRemove(
+				ModelPosition.createFromParentAndOffset( modelRoot, 0 ),
+				ModelRange.createFromParentsAndOffsets( modelDoc.graveyard, 0, modelDoc.graveyard, 1 )
+			);
+
+			expect( viewToString( viewRoot ) ).to.equal( '<div>b</div>' );
+
+			expect( mapper.toModelElement( viewElement ) ).to.be.undefined;
+			expect( mapper.toViewElement( modelElement ) ).to.be.undefined;
+		} );
+
+		// TODO move to conversion/integration.js one day.
+		it( 'should not break when remove() is used as part of unwrapping', () => {
+			// The whole process looks like this:
+			// <w><a></a></w> => <a></a><w><a></a></w> => <a></a><w></w> => <a></a>
+			// The <a> is duplicated for a while in the view.
+
+			const modelAElement = new ModelElement( 'a' );
+			const modelWElement = new ModelElement( 'w' );
+			const viewAElement = new ViewContainerElement( 'a' );
+			const viewA2Element = new ViewContainerElement( 'a2' );
+			const viewWElement = new ViewContainerElement( 'w' );
+
+			modelRoot.appendChildren( modelWElement );
+			viewRoot.appendChildren( viewWElement );
+
+			modelWElement.appendChildren( modelAElement );
+			viewWElement.appendChildren( viewAElement );
+
+			mapper.bindElements( modelWElement, viewWElement );
+			mapper.bindElements( modelAElement, viewAElement );
+
+			dispatcher.on( 'remove', remove() );
+			dispatcher.on( 'insert', insertElement( () => viewA2Element ) );
+
+			modelDoc.on( 'change', ( evt, type, changes ) => {
+				dispatcher.convertChange( type, changes );
+			} );
+
+			modelDoc.batch().unwrap( modelWElement );
+
+			expect( viewToString( viewRoot ) ).to.equal( '<div><a2></a2></div>' );
+
+			expect( mapper.toModelElement( viewA2Element ) ).to.equal( modelAElement );
+			expect( mapper.toViewElement( modelAElement ) ).to.equal( viewA2Element );
+
+			// This is a bit unfortunate, but we think we can live with this.
+			// The viewAElement is not in the tree and there's a high chance that all reference to it are gone.
+			expect( mapper.toModelElement( viewAElement ) ).to.equal( modelAElement );
+
+			expect( mapper.toModelElement( viewWElement ) ).to.be.undefined;
+			expect( mapper.toViewElement( modelWElement ) ).to.be.undefined;
+		} );
 	} );
 } );

tests/conversion/modelconversiondispatcher.js

@@ -289,6 +289,87 @@ describe( 'ModelConversionDispatcher', () => {
 	} );
 
 	describe( 'convertMove', () => {
+		let loggedEvents;
+
+		beforeEach( () => {
+			loggedEvents = [];
+
+			dispatcher.on( 'remove', ( evt, data ) => {
+				const log = 'remove:' + data.sourcePosition.path + ':' + data.item.offsetSize;
+				loggedEvents.push( log );
+			} );
+
+			dispatcher.on( 'insert', ( evt, data ) => {
+				const log = 'insert:' + data.range.start.path + ':' + data.range.end.path;
+				loggedEvents.push( log );
+			} );
+		} );
+
+		it( 'should first fire remove and then insert if moving "right"', () => {
+			// <root>[ab]cd^ef</root> -> <root>cdabef</root>
+			root.appendChildren( new ModelText( 'cdabef' ) );
+
+			const sourcePosition = ModelPosition.createFromParentAndOffset( root, 0 );
+			const movedRange = ModelRange.createFromParentsAndOffsets( root, 2, root, 4 );
+
+			dispatcher.convertMove( sourcePosition, movedRange );
+
+			// after remove: cdef
+			// after insert: cd[ab]ef
+			expect( loggedEvents ).to.deep.equal( [ 'remove:0:2', 'insert:2:4' ] );
+		} );
+
+		it( 'should first fire insert and then remove if moving "left"', () => {
+			// <root>ab^cd[ef]</root> -> <root>abefcd</root>
+			root.appendChildren( new ModelText( 'abefcd' ) );
+
+			const sourcePosition = ModelPosition.createFromParentAndOffset( root, 4 );
+			const movedRange = ModelRange.createFromParentsAndOffsets( root, 2, root, 4 );
+
+			dispatcher.convertMove( sourcePosition, movedRange );
+
+			// after insert: ab[ef]cd[ef]
+			// after remove: ab[ef]cd
+			expect( loggedEvents ).to.deep.equal( [ 'insert:2:4', 'remove:6:2' ] );
+		} );
+
+		it( 'should first fire insert and then remove when moving like in unwrap', () => {
+			// <root>a^<w>[xyz]</w>b</root> -> <root>axyz<w></w>b</root>
+			root.appendChildren( [
+				new ModelText( 'axyz' ),
+				new ModelElement( 'w' ),
+				new ModelText( 'b' )
+			] );
+
+			const sourcePosition = new ModelPosition( root, [ 1, 0 ] );
+			const movedRange = ModelRange.createFromParentsAndOffsets( root, 1, root, 4 );
+
+			dispatcher.convertMove( sourcePosition, movedRange );
+
+			// before:       a<w>[xyz]</w>b
+			// after insert: a[xyz]<w>[xyz]</w>b
+			// after remove: a[xyz]<w></w>b
+			expect( loggedEvents ).to.deep.equal( [ 'insert:1:4', 'remove:4,0:3' ] );
+		} );
+
+		it( 'should first fire remove and then insert when moving like in wrap', () => {
+			// <root>a[xyz]<w>^</w>b</root> -> <root>a<w>xyz</w>b</root>
+			root.appendChildren( [
+				new ModelText( 'a' ),
+				new ModelElement( 'w', null, [ new ModelText( 'xyz' ) ] ),
+				new ModelText( 'b' )
+			] );
+
+			const sourcePosition = ModelPosition.createFromParentAndOffset( root, 1 );
+			const movedRange = ModelRange.createFromPositionAndShift( new ModelPosition( root, [ 1, 0 ] ), 3 );
+
+			dispatcher.convertMove( sourcePosition, movedRange );
+
+			// before:       a[xyz]<w></w>b
+			// after remove: a<w></w>b
+			// after insert: a<w>[xyz]</w>b
+			expect( loggedEvents ).to.deep.equal( [ 'remove:1:3', 'insert:1,0:1,3' ] );
+		} );
 	} );
 
 	describe( 'convertRemove', () => {