8
0
Просмотр исходного кода

Fixed: Corrected how change items in `model.Differ` are dismissed if they are in inserted/removed parent.

Szymon Cofalik 8 лет назад
Родитель
Сommit
31f90fa249

+ 51 - 47
packages/ckeditor5-engine/src/model/differ.js

@@ -229,13 +229,6 @@ export default class Differ {
 
 		// Check all changed elements.
 		for ( const element of this._changesInElement.keys() ) {
-			// Each item in `this._changesInElement` describes changes of the _children_ of that element.
-			// If the element itself has been inserted we should skip all the changes in it because the element will be reconverted.
-			// If the element itself has been removed we should skip all the changes in it because they would be incorrect.
-			if ( this._isInsertedOrRemoved( element ) ) {
-				continue;
-			}
-
 			// Get changes for this element and sort them.
 			const changes = this._changesInElement.get( element ).sort( ( a, b ) => {
 				if ( a.offset === b.offset ) {
@@ -394,46 +387,6 @@ export default class Differ {
 		this._cachedChanges = null;
 	}
 
-	/**
-	 * Checks whether a given element is inserted or removed or one of its ancestors is inserted or removed. Used to
-	 * filter out sub-changes in elements that are changed.
-	 *
-	 * @private
-	 * @param {module:engine/model/element~Element} element An element to check.
-	 * @returns {Boolean}
-	 */
-	_isInsertedOrRemoved( element ) {
-		let parent = element.parent;
-
-		// Check all ancestors of given element.
-		while ( parent ) {
-			// Get the checked element's offset.
-			const offset = element.startOffset;
-
-			if ( this._changesInElement.has( parent ) ) {
-				const changes = this._changesInElement.get( parent );
-
-				// If there were changes in that element's ancestor, check all of them.
-				for ( const change of changes ) {
-					// Skip attribute changes. We are interested only if the element was inserted or removed.
-					if ( change.type == 'attribute' ) {
-						continue;
-					}
-
-					if ( change.offset <= offset && change.offset + change.howMany > offset ) {
-						return true;
-					}
-				}
-			}
-
-			// Move up.
-			parent = parent.parent;
-			element = element.parent;
-		}
-
-		return false;
-	}
-
 	/**
 	 * Saves and handles an insert change.
 	 *
@@ -443,6 +396,10 @@ 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 );
@@ -457,9 +414,15 @@ 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 );
+
+		this._removeAllNestedChanges( parent, offset, howMany );
 	}
 
 	/**
@@ -469,6 +432,10 @@ 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 );
@@ -831,6 +798,43 @@ export default class Differ {
 
 		return diffs;
 	}
+
+	_isInInsertedElement( element ) {
+		const parent = element.parent;
+
+		if ( !parent ) {
+			return false;
+		}
+
+		const changes = this._changesInElement.get( parent );
+		const offset = element.startOffset;
+
+		if ( changes ) {
+			for ( const change of changes ) {
+				if ( change.type == 'insert' && offset >= change.offset && offset < change.offset + change.howMany ) {
+					return true;
+				}
+			}
+		}
+
+		return this._isInInsertedElement( parent );
+	}
+
+	_removeAllNestedChanges( parent, offset, howMany ) {
+		const range = Range.createFromParentsAndOffsets( parent, offset, parent, offset + howMany );
+
+		for ( const item of range.getItems( { shallow: true } ) ) {
+			if ( item.is( 'element' ) ) {
+				this._removeChangesInElement( item );
+				this._removeAllNestedChanges( item, 0, item.maxOffset );
+			}
+		}
+	}
+
+	_removeChangesInElement( element ) {
+		this._elementSnapshots.delete( element );
+		this._changesInElement.delete( element );
+	}
 }
 
 // Returns an array that is a copy of passed child list with the exception that text nodes are split to one or more

+ 57 - 0
packages/ckeditor5-engine/tests/model/differ.js

@@ -1193,6 +1193,63 @@ describe( 'Differ', () => {
 				] );
 			} );
 		} );
+
+		// ckeditor5#733.
+		it( 'proper filtering of changes in removed elements', () => {
+			// Before fix there was a buggy scenario described in ckeditor5#733.
+			// There was this structure: `<paragraph>foo[</paragraph><image /><blockQuote><p>te]xt</p></blockQuote>`
+			// On delete of above selection `image` and `paragraph` inside `blockQuote` are removed (it gets merged).
+			// However, since `image` was removed first, when checking if `paragraph` is in a removed element,
+			// it appeared that `blockQuote` looks like it is removed because it had the same path as the already removed `<image>`.
+			// In a result, removing `paragraph` was discarded.
+			// The mistake was that the checking for removing was done at incorrect moment.
+			root.removeChildren( 0, root.childCount );
+			root.appendChildren( [
+				new Element( 'paragraph', null, new Text( 'foo' ) ),
+				new Element( 'image' ),
+				new Element( 'blockQuote', null, [
+					new Element( 'paragraph', null, new Text( 'text' ) )
+				] )
+			] );
+
+			model.change( () => {
+				// Remove `"te"`.
+				remove( new Position( root, [ 2, 0, 0 ] ), 2 );
+				// Remove `image` before `blockQuote`.
+				remove( new Position( root, [ 1 ] ), 1 );
+				// Move `"xt"` to first `paragraph` (merging).
+				move( new Position( root, [ 1, 0, 0 ] ), 2, new Position( root, [ 0, 3 ] ) );
+				// Remove `paragraph` inside `blockQuote`.
+				remove( new Position( root, [ 1, 0 ] ), 1 );
+
+				expectChanges( [
+					{ type: 'insert', name: '$text', length: 2, position: new Position( root, [ 0, 3 ] ) },
+					{ type: 'remove', name: 'image', length: 1, position: new Position( root, [ 1 ] ) },
+					{ type: 'remove', name: 'paragraph', length: 1, position: new Position( root, [ 1, 0 ] ) }
+				] );
+			} );
+		} );
+
+		it( 'proper filtering of changes in inserted elements', () => {
+			root.removeChildren( 0, root.childCount );
+			root.appendChildren( new Element( 'image' ) );
+
+			const blockQuote = new Element( 'blockQuote', null, new Element( 'paragraph' ) );
+
+			model.change( () => {
+				// Insert `blockQuote` with `paragraph` after `image`.
+				insert( blockQuote, new Position( root, [ 1 ] ) );
+				// Remove `image` from before `blockQuote`.
+				remove( new Position( root, [ 0 ] ), 1 );
+				// Insert text into `paragraph` in `blockQuote`.
+				insert( new Text( 'foo' ), new Position( root, [ 0, 0, 0 ] ) );
+
+				expectChanges( [
+					{ type: 'remove', name: 'image', length: 1, position: new Position( root, [ 0 ] ) },
+					{ type: 'insert', name: 'blockQuote', length: 1, position: new Position( root, [ 0 ] ) }
+				] );
+			} );
+		} );
 	} );
 
 	describe( 'getChanges()', () => {