Merge pull request #1327 from ckeditor/t/1326

Fix: Improved how `model.Differ` checks whether the operation should be buffered or not. Closes #1326.

Author: Reinmar

src/model/differ.js

@@ -107,30 +107,54 @@ export default class Differ {
 	 */
 	bufferOperation( operation ) {
 		switch ( operation.type ) {
-			case 'insert':
+			case 'insert': {
+				if ( this._isInInsertedElement( operation.position.parent ) ) {
+					return;
+				}
+
 				this._markInsert( operation.position.parent, operation.position.offset, operation.nodes.maxOffset );
 
 				break;
+			}
 			case 'addAttribute':
 			case 'removeAttribute':
-			case 'changeAttribute':
+			case 'changeAttribute': {
 				for ( const item of operation.range.getItems() ) {
+					if ( this._isInInsertedElement( item.parent ) ) {
+						continue;
+					}
+
 					this._markAttribute( item );
 				}
 
 				break;
+			}
 			case 'remove':
 			case 'move':
-			case 'reinsert':
-				this._markRemove( operation.sourcePosition.parent, operation.sourcePosition.offset, operation.howMany );
-				this._markInsert( operation.targetPosition.parent, operation.getMovedRangeStart().offset, operation.howMany );
+			case 'reinsert': {
+				const sourceParentInserted = this._isInInsertedElement( operation.sourcePosition.parent );
+				const targetParentInserted = this._isInInsertedElement( operation.targetPosition.parent );
+
+				if ( !sourceParentInserted ) {
+					this._markRemove( operation.sourcePosition.parent, operation.sourcePosition.offset, operation.howMany );
+				}
+
+				if ( !targetParentInserted ) {
+					this._markInsert( operation.targetPosition.parent, operation.getMovedRangeStart().offset, operation.howMany );
+				}
 
 				break;
-			case 'rename':
+			}
+			case 'rename': {
+				if ( this._isInInsertedElement( operation.position.parent ) ) {
+					return;
+				}
+
 				this._markRemove( operation.position.parent, operation.position.offset, 1 );
 				this._markInsert( operation.position.parent, operation.position.offset, 1 );
 
 				break;
+			}
 		}
 
 		// Clear cache after each buffered operation as it is no longer valid.
@@ -396,10 +420,6 @@ export default class Differ {
 	 * @param {Number} howMany
 	 */
 	_markInsert( parent, offset, howMany ) {
-		if ( this._isInInsertedElement( parent ) ) {
-			return;
-		}
-
 		const changeItem = { type: 'insert', offset, howMany, count: this._changeCount++ };
 
 		this._markChange( parent, changeItem );
@@ -414,10 +434,6 @@ export default class Differ {
 	 * @param {Number} howMany
 	 */
 	_markRemove( parent, offset, howMany ) {
-		if ( this._isInInsertedElement( parent ) ) {
-			return;
-		}
-
 		const changeItem = { type: 'remove', offset, howMany, count: this._changeCount++ };
 
 		this._markChange( parent, changeItem );
@@ -432,10 +448,6 @@ export default class Differ {
 	 * @param {module:engine/model/item~Item} item
 	 */
 	_markAttribute( item ) {
-		if ( this._isInInsertedElement( item.parent ) ) {
-			return;
-		}
-
 		const changeItem = { type: 'attribute', offset: item.startOffset, howMany: item.offsetSize, count: this._changeCount++ };
 
 		this._markChange( item.parent, changeItem );

tests/model/differ.js

@@ -641,6 +641,18 @@ describe( 'Differ', () => {
 				] );
 			} );
 		} );
+
+		it( 'inside a new element', () => {
+			// Since the rename is inside a new element, it should not be listed on changes list.
+			model.change( () => {
+				insert( new Element( 'blockQuote', null, new Element( 'paragraph' ) ), new Position( root, [ 2 ] ) );
+				rename( root.getChild( 2 ).getChild( 0 ), 'listItem' );
+
+				expectChanges( [
+					{ type: 'insert', name: 'blockQuote', length: 1, position: new Position( root, [ 2 ] ) }
+				] );
+			} );
+		} );
 	} );
 
 	describe( 'attribute', () => {
@@ -1230,7 +1242,10 @@ describe( 'Differ', () => {
 			} );
 		} );
 
-		it( 'proper filtering of changes in inserted elements', () => {
+		// In this scenario we create a new element, then remove something from before it to mess up with offsets,
+		// finally we insert some content into a new element. Since we are inserting into a new element, the
+		// inserted children should not be shown on changes list.
+		it( 'proper filtering of changes in inserted elements #1', () => {
 			root.removeChildren( 0, root.childCount );
 			root.appendChildren( new Element( 'image' ) );
 
@@ -1250,6 +1265,26 @@ describe( 'Differ', () => {
 				] );
 			} );
 		} );
+
+		// In this scenario we create a new element, then move another element that was before the new element into
+		// the new element. This way we mess up with offsets and insert content into a new element in one operation.
+		// Since we are inserting into a new element, the insertion of moved element should not be shown on changes list.
+		it( 'proper filtering of changes in inserted elements #2', () => {
+			root.removeChildren( 0, root.childCount );
+			root.appendChildren( new Element( 'image' ) );
+
+			model.change( () => {
+				// Insert `div` after `image`.
+				insert( new Element( 'div' ), new Position( root, [ 1 ] ) );
+				// Move `image` to the new `div`.
+				move( new Position( root, [ 0 ] ), 1, new Position( root, [ 1, 0 ] ) );
+
+				expectChanges( [
+					{ type: 'remove', name: 'image', length: 1, position: new Position( root, [ 0 ] ) },
+					{ type: 'insert', name: 'div', length: 1, position: new Position( root, [ 0 ] ) }
+				] );
+			} );
+		} );
 	} );
 
 	describe( 'getChanges()', () => {
@@ -1410,6 +1445,8 @@ describe( 'Differ', () => {
 	function expectChanges( expected, includeChangesInGraveyard = false ) {
 		const changes = differ.getChanges( { includeChangesInGraveyard } );
 
+		expect( changes.length ).to.equal( expected.length );
+
 		for ( let i = 0; i < expected.length; i++ ) {
 			for ( const key in expected[ i ] ) {
 				if ( expected[ i ].hasOwnProperty( key ) ) {