浏览代码

Changed: `RemoveOperation` now does not create `$holderElement` and does not set `targetPosition` on its own.
Added: `RemoveOperation#isPermanent`.
Changed: `Batch#remove` now creates separate delta for each `RemoveOperation` instead of creating one delta with multiple operations.
Added: `isPermanent` parameter in `Batch#remove`.

Szymon Cofalik 8 年之前
父节点
当前提交
6b04950bc6
共有 25 个文件被更改,包括 155 次插入217 次删除
  1. 1 10
      packages/ckeditor5-engine/src/model/delta/basic-transformations.js
  2. 4 1
      packages/ckeditor5-engine/src/model/delta/mergedelta.js
  3. 15 9
      packages/ckeditor5-engine/src/model/delta/removedelta.js
  4. 4 1
      packages/ckeditor5-engine/src/model/delta/unwrapdelta.js
  5. 4 1
      packages/ckeditor5-engine/src/model/operation/insertoperation.js
  6. 1 7
      packages/ckeditor5-engine/src/model/operation/reinsertoperation.js
  7. 20 95
      packages/ckeditor5-engine/src/model/operation/removeoperation.js
  8. 4 2
      packages/ckeditor5-engine/tests/conversion/modelconversiondispatcher.js
  9. 18 12
      packages/ckeditor5-engine/tests/dev-utils/enableenginedebug.js
  10. 1 0
      packages/ckeditor5-engine/tests/model/delta/delta.js
  11. 1 0
      packages/ckeditor5-engine/tests/model/delta/deltafactory.js
  12. 2 2
      packages/ckeditor5-engine/tests/model/delta/mergedelta.js
  13. 12 3
      packages/ckeditor5-engine/tests/model/delta/removedelta.js
  14. 13 3
      packages/ckeditor5-engine/tests/model/delta/transform/_utils/utils.js
  15. 1 1
      packages/ckeditor5-engine/tests/model/delta/transform/insertdelta.js
  16. 1 1
      packages/ckeditor5-engine/tests/model/delta/transform/mergedelta.js
  17. 1 1
      packages/ckeditor5-engine/tests/model/delta/transform/movedelta.js
  18. 1 1
      packages/ckeditor5-engine/tests/model/delta/transform/removedelta.js
  19. 3 3
      packages/ckeditor5-engine/tests/model/delta/transform/unwrapdelta.js
  20. 1 1
      packages/ckeditor5-engine/tests/model/delta/transform/wrapdelta.js
  21. 2 2
      packages/ckeditor5-engine/tests/model/delta/unwrapdelta.js
  22. 3 5
      packages/ckeditor5-engine/tests/model/document/change-event.js
  23. 3 0
      packages/ckeditor5-engine/tests/model/liveselection.js
  24. 5 13
      packages/ckeditor5-engine/tests/model/operation/reinsertoperation.js
  25. 34 43
      packages/ckeditor5-engine/tests/model/operation/removeoperation.js

+ 1 - 10
packages/ckeditor5-engine/src/model/delta/basic-transformations.js

@@ -320,19 +320,10 @@ addTransformationCase( UnwrapDelta, SplitDelta, ( a, b, isStrong ) => {
 	// If incoming unwrap delta tries to unwrap node that got split we should unwrap the original node and the split copy.
 	// This can be achieved either by reverting split and applying unwrap to singular node, or creating additional unwrap delta.
 	if ( compareArrays( a.position.path, b.position.getParentPath() ) === 'same' ) {
-		const transformed = [
+		return [
 			b.getReversed(),
 			a.clone()
 		];
-
-		// It's a kind of magic-magic-magic-maaaaagiiic!
-		transformed[ 1 ].operations[ 1 ].targetPosition.path[ 0 ]++;
-		// But seriously, we have to fix RemoveOperation in the second delta because reversed UnwrapDelta creates
-		// MergeDelta which also has RemoveOperation. Those two operations cannot point to the same "holder" element
-		// in the graveyard, so we fix it by hand. This is the only case where it happens in "special" transformation
-		// cases, and it won't happen for "default" transformation apart of RemoveDelta, where it is okay.
-
-		return transformed;
 	}
 
 	return defaultTransform( a, b, isStrong );

+ 4 - 1
packages/ckeditor5-engine/src/model/delta/mergedelta.js

@@ -131,7 +131,10 @@ register( 'merge', function( position ) {
 	delta.addOperation( move );
 	this.document.applyOperation( move );
 
-	const remove = new RemoveOperation( position, 1, this.document.version );
+	const graveyard = this.document.graveyard;
+	const gyPosition = new Position( graveyard, [ 0 ] );
+
+	const remove = new RemoveOperation( position, 1, gyPosition, this.document.version );
 	delta.addOperation( remove );
 	this.document.applyOperation( remove );
 

+ 15 - 9
packages/ckeditor5-engine/src/model/delta/removedelta.js

@@ -28,8 +28,15 @@ export default class RemoveDelta extends MoveDelta {
 	}
 }
 
-function addRemoveOperation( batch, delta, position, howMany ) {
-	const operation = new RemoveOperation( position, howMany, batch.document.version );
+function addRemoveDelta( batch, position, howMany, isPermanent ) {
+	const delta = new RemoveDelta();
+	batch.addDelta( delta );
+
+	const graveyard = batch.document.graveyard;
+	const gyPosition = new Position( graveyard, [ 0 ] );
+
+	const operation = new RemoveOperation( position, howMany, gyPosition, batch.document.version );
+	operation.isPermanent = isPermanent;
 	delta.addOperation( operation );
 	batch.document.applyOperation( operation );
 }
@@ -40,20 +47,19 @@ function addRemoveOperation( batch, delta, position, howMany ) {
  * @chainable
  * @method module:engine/model/batch~Batch#remove
  * @param {module:engine/model/item~Item|module:engine/model/range~Range} itemOrRange Model item or range to remove.
+ * @param {Boolean} isPermanent If set to `true`, creates
+ * {@link module:engine/model/operation/removeoperation~RemoveOperation#isPermanent a permanent RemoveOperation}.
  */
-register( 'remove', function( itemOrRange ) {
-	const delta = new RemoveDelta();
-	this.addDelta( delta );
-
+register( 'remove', function( itemOrRange, isPermanent = false ) {
 	if ( itemOrRange instanceof Range ) {
-		// The array is reversed, so the ranges are correct and do not have to be updated.
+		// The array is reversed, so the ranges to remove are in correct order and do not have to be updated.
 		const ranges = itemOrRange.getMinimalFlatRanges().reverse();
 
 		for ( const flat of ranges ) {
-			addRemoveOperation( this, delta, flat.start, flat.end.offset - flat.start.offset );
+			addRemoveDelta( this, flat.start, flat.end.offset - flat.start.offset, isPermanent );
 		}
 	} else {
-		addRemoveOperation( this, delta, Position.createBefore( itemOrRange ), 1 );
+		addRemoveDelta( this, Position.createBefore( itemOrRange ), 1, isPermanent );
 	}
 
 	return this;

+ 4 - 1
packages/ckeditor5-engine/src/model/delta/unwrapdelta.js

@@ -106,7 +106,10 @@ register( 'unwrap', function( element ) {
 
 	// Computing new position because we moved some nodes before `element`.
 	// If we would cache `Position.createBefore( element )` we remove wrong node.
-	const remove = new RemoveOperation( Position.createBefore( element ), 1, this.document.version );
+	const graveyard = this.document.graveyard;
+	const gyPosition = new Position( graveyard, [ 0 ] );
+
+	const remove = new RemoveOperation( Position.createBefore( element ), 1, gyPosition, this.document.version );
 	delta.addOperation( remove );
 	this.document.applyOperation( remove );
 

+ 4 - 1
packages/ckeditor5-engine/src/model/operation/insertoperation.js

@@ -72,7 +72,10 @@ export default class InsertOperation extends Operation {
 	 * @returns {module:engine/model/operation/removeoperation~RemoveOperation}
 	 */
 	getReversed() {
-		return new RemoveOperation( this.position, this.nodes.maxOffset, this.baseVersion + 1 );
+		const graveyard = this.position.root.document.graveyard;
+		const gyPosition = new Position( graveyard, [ 0 ] );
+
+		return new RemoveOperation( this.position, this.nodes.maxOffset, gyPosition, this.baseVersion + 1 );
 	}
 
 	/**

+ 1 - 7
packages/ckeditor5-engine/src/model/operation/reinsertoperation.js

@@ -46,13 +46,7 @@ export default class ReinsertOperation extends MoveOperation {
 	 * @returns {module:engine/model/operation/removeoperation~RemoveOperation}
 	 */
 	getReversed() {
-		const removeOp = new RemoveOperation( this.targetPosition, this.howMany, this.baseVersion + 1 );
-
-		// Make sure that nodes are put back into the `$graveyardHolder` from which they got reinserted.
-		removeOp.targetPosition = this.sourcePosition;
-		removeOp._needsHolderElement = false;
-
-		return removeOp;
+		return new RemoveOperation( this.targetPosition, this.howMany, this.sourcePosition, this.baseVersion + 1 );
 	}
 
 	/**

+ 20 - 95
packages/ckeditor5-engine/src/model/operation/removeoperation.js

@@ -8,9 +8,8 @@
  */
 
 import MoveOperation from './moveoperation';
-import Position from '../position';
-import Element from '../element';
 import ReinsertOperation from './reinsertoperation';
+import NoOperation from './nooperation';
 
 /**
  * Operation to remove a range of nodes.
@@ -19,36 +18,23 @@ export default class RemoveOperation extends MoveOperation {
 	/**
 	 * Creates a remove operation.
 	 *
-	 * @param {module:engine/model/position~Position} position Position before the first {@link module:engine/model/item~Item model item} to
-	 * remove.
-	 * @param {Number} howMany Offset size of removed range. {@link module:engine/model/item~Item Model items} will be removed starting
-	 * from `sourcePosition`, up to a `sourcePosition` with offset shifted by `howMany`.
+	 * @param {module:engine/model/position~Position} sourcePosition Position before the first
+	 * {@link module:engine/model/item~Item model item} to move.
+	 * @param {Number} howMany Offset size of moved range. Moved range will start from `sourcePosition` and end at
+	 * `sourcePosition` with offset shifted by `howMany`.
+	 * @param {module:engine/model/position~Position} targetPosition Position at which moved nodes will be inserted.
 	 * @param {Number} baseVersion {@link module:engine/model/document~Document#version} on which operation can be applied.
 	 */
-	constructor( position, howMany, baseVersion ) {
-		const graveyard = position.root.document.graveyard;
-		const graveyardPosition = new Position( graveyard, [ graveyard.maxOffset, 0 ] );
-
-		super( position, howMany, graveyardPosition, baseVersion );
+	constructor( sourcePosition, howMany, targetPosition, baseVersion ) {
+		super( sourcePosition, howMany, targetPosition, baseVersion );
 
 		/**
-		 * Flag informing whether this operation should insert "holder" element (`true`) or should move removed nodes
-		 * into existing "holder" element (`false`).
-		 *
-		 * The flag should be set to `true` for each "new" `RemoveOperation` that is each `RemoveOperation` originally
-		 * created to remove some nodes from document (most likely created through `Batch` API).
-		 *
-		 * The flag should be set to `false` for each `RemoveOperation` that got created by splitting the original
-		 * `RemoveOperation`, for example during operational transformation.
-		 *
-		 * The flag should be set to `false` whenever removing nodes that were re-inserted from graveyard. This will
-		 * ensure correctness of all other operations that might change something on those nodes. This will also ensure
-		 * that redundant empty graveyard holder elements are not created.
+		 * If `RemoveOperation` is permanent (`true`), nodes removed by it cannot be reinserted back to the model. This
+		 * setting affects operational transformation and {@link #getReversed reversing}.
 		 *
-		 * @protected
-		 * @type {Boolean}
+		 * @member {Boolean} module:engine/model/operation/removeoperation~RemoveOperation#isPermanent
 		 */
-		this._needsHolderElement = true;
+		this.isPermanent = false;
 	}
 
 	/**
@@ -59,67 +45,16 @@ export default class RemoveOperation extends MoveOperation {
 	}
 
 	/**
-	 * Offset of the graveyard "holder" element, in which nodes removed by this operation are stored.
-	 *
-	 * @protected
-	 * @type {Number}
-	 */
-	get _holderElementOffset() {
-		return this.targetPosition.path[ 0 ];
-	}
-
-	/**
-	 * Sets {@link module:engine/model/operation/removeoperation~RemoveOperation#_holderElementOffset}.
-	 *
-	 * @protected
-	 * @param {Number} offset
-	 */
-	set _holderElementOffset( offset ) {
-		this.targetPosition.path[ 0 ] = offset;
-	}
-
-	/**
 	 * See {@link module:engine/model/operation/operation~Operation#getReversed `Operation#getReversed()`}.
 	 *
-	 * @returns {module:engine/model/operation/reinsertoperation~ReinsertOperation}
+	 * @returns {module:engine/model/operation/reinsertoperation~ReinsertOperation|module:engine/model/operation/nooperation~NoOperation}
 	 */
 	getReversed() {
-		return new ReinsertOperation( this.targetPosition, this.howMany, this.sourcePosition, this.baseVersion + 1 );
-	}
-
-	/**
-	 * Creates and returns an operation that has the same parameters as this operation.
-	 *
-	 * @returns {module:engine/model/operation/removeoperation~RemoveOperation} Clone of this operation.
-	 */
-	clone() {
-		const removeOperation = new RemoveOperation( this.sourcePosition, this.howMany, this.baseVersion );
-		removeOperation.targetPosition = Position.createFromPosition( this.targetPosition );
-
-		return removeOperation;
-	}
-
-	/**
-	 * @inheritDoc
-	 */
-	_execute() {
-		// Insert "holder" element in graveyard root, if the operation needs it.
-		if ( this._needsHolderElement ) {
-			const graveyard = this.targetPosition.root;
-			const holderElement = new Element( '$graveyardHolder' );
-
-			graveyard.insertChildren( this._holderElementOffset, holderElement );
-
-			// If the operation removes nodes that are already in graveyard, it may happen that
-			// the operation's source position is invalidated by inserting new holder element into the graveyard.
-			// If that's the case, we need to fix source position path.
-			if ( this.sourcePosition.root == graveyard && this.sourcePosition.path[ 0 ] >= this._holderElementOffset ) {
-				this.sourcePosition.path[ 0 ]++;
-			}
+		if ( this.isPermanent ) {
+			return new NoOperation( this.baseVersion + 1 );
+		} else {
+			return new ReinsertOperation( this.targetPosition, this.howMany, this.sourcePosition, this.baseVersion + 1 );
 		}
-
-		// Then, execute as a move operation.
-		return super._execute();
 	}
 
 	/**
@@ -129,21 +64,11 @@ export default class RemoveOperation extends MoveOperation {
 		return 'engine.model.operation.RemoveOperation';
 	}
 
-	/**
-	 * Creates `RemoveOperation` object from deserilized object, i.e. from parsed JSON string.
-	 *
-	 * @param {Object} json Deserialized JSON object.
-	 * @param {module:engine/model/document~Document} document Document on which this operation will be applied.
-	 * @returns {module:engine/model/operation/removeoperation~RemoveOperation}
-	 */
 	static fromJSON( json, document ) {
-		const sourcePosition = Position.fromJSON( json.sourcePosition, document );
-
-		const removeOp = new RemoveOperation( sourcePosition, json.howMany, json.baseVersion );
+		const op = super.fromJSON( json, document );
 
-		removeOp.targetPosition = Position.fromJSON( json.targetPosition, document );
-		removeOp._needsHolderElement = json._needsHolderElement;
+		op.isPermanent = json.isPermanent;
 
-		return removeOp;
+		return op;
 	}
 }

+ 4 - 2
packages/ckeditor5-engine/tests/conversion/modelconversiondispatcher.js

@@ -13,12 +13,14 @@ import RemoveOperation from '../../src/model/operation/removeoperation';
 import { wrapInDelta } from '../../tests/model/_utils/utils';
 
 describe( 'ModelConversionDispatcher', () => {
-	let dispatcher, doc, root;
+	let dispatcher, doc, root, gyPos;
 
 	beforeEach( () => {
 		doc = new ModelDocument();
 		dispatcher = new ModelConversionDispatcher( doc );
 		root = doc.createRoot();
+
+		gyPos = new ModelPosition( doc.graveyard, [ 0 ] );
 	} );
 
 	describe( 'constructor()', () => {
@@ -69,7 +71,7 @@ describe( 'ModelConversionDispatcher', () => {
 
 			// We will just create reinsert operation by reverting remove operation
 			// because creating reinsert change is tricky and not available through batch API.
-			const removeOperation = new RemoveOperation( imagePos, 1, 0 );
+			const removeOperation = new RemoveOperation( imagePos, 1, gyPos, 0 );
 
 			// Let's apply remove operation so reinsert operation won't break.
 			doc.applyOperation( wrapInDelta( removeOperation ) );

+ 18 - 12
packages/ckeditor5-engine/tests/dev-utils/enableenginedebug.js

@@ -324,9 +324,15 @@ describe( 'debug tools', () => {
 
 				otherRoot.appendChildren( [ firstEle, removedEle ] );
 
+				const graveyard = modelDoc.graveyard;
 				const delta = new MergeDelta();
 				const move = new MoveOperation( ModelPosition.createAt( removedEle, 0 ), 3, ModelPosition.createAt( firstEle, 0 ), 0 );
-				const remove = new RemoveOperation( ModelPosition.createBefore( removedEle ), 1, 1 );
+				const remove = new RemoveOperation(
+					ModelPosition.createBefore( removedEle ),
+					1,
+					ModelPosition.createAt( graveyard, 0 ),
+					1
+				);
 
 				delta.addOperation( move );
 				delta.addOperation( remove );
@@ -400,9 +406,10 @@ describe( 'debug tools', () => {
 
 				otherRoot.appendChildren( [ unwrapEle ] );
 
+				const graveyard = modelDoc.graveyard;
 				const delta = new UnwrapDelta();
 				const move = new MoveOperation( ModelPosition.createAt( unwrapEle, 0 ), 3, ModelPosition.createAt( otherRoot, 0 ), 0 );
-				const remove = new RemoveOperation( ModelPosition.createAt( otherRoot, 3 ), 1, 1 );
+				const remove = new RemoveOperation( ModelPosition.createAt( otherRoot, 3 ), 1, ModelPosition.createAt( graveyard, 0 ), 1 );
 
 				delta.addOperation( move );
 				delta.addOperation( remove );
@@ -609,7 +616,8 @@ describe( 'debug tools', () => {
 			const insert = new InsertOperation( ModelPosition.createAt( modelRoot, 0 ), new ModelText( 'foobar' ), 0 );
 			model.applyOperation( wrapInDelta( insert ) );
 
-			const remove = new RemoveOperation( ModelPosition.createAt( modelRoot, 1 ), 2, 1 );
+			const graveyard = model.graveyard;
+			const remove = new RemoveOperation( ModelPosition.createAt( modelRoot, 1 ), 2, ModelPosition.createAt( graveyard, 0 ), 1 );
 			model.applyOperation( wrapInDelta( remove ) );
 
 			log.reset();
@@ -631,9 +639,7 @@ describe( 'debug tools', () => {
 			model.log( 2 );
 			expectLog(
 				'<$graveyard>' +
-				'\n\t<$graveyardHolder>' +
-				'\n\t\too' +
-				'\n\t</$graveyardHolder>' +
+				'\n\too' +
 				'\n</$graveyard>' +
 				'\n<main>' +
 				'\n\tfbar' +
@@ -643,9 +649,7 @@ describe( 'debug tools', () => {
 			model.log();
 			expectLog(
 				'<$graveyard>' +
-				'\n\t<$graveyardHolder>' +
-				'\n\t\too' +
-				'\n\t</$graveyardHolder>' +
+				'\n\too' +
 				'\n</$graveyard>' +
 				'\n<main>' +
 				'\n\tfbar' +
@@ -728,8 +732,9 @@ describe( 'debug tools', () => {
 			otherRoot.appendChildren( [ firstEle, removedEle ] );
 
 			const delta = new MergeDelta();
+			const graveyard = modelDoc.graveyard;
 			const move = new MoveOperation( ModelPosition.createAt( removedEle, 0 ), 3, ModelPosition.createAt( firstEle, 0 ), 0 );
-			const remove = new RemoveOperation( ModelPosition.createBefore( removedEle ), 1, 1 );
+			const remove = new RemoveOperation( ModelPosition.createBefore( removedEle ), 1, ModelPosition.createAt( graveyard, 0 ), 1 );
 
 			delta.addOperation( move );
 			delta.addOperation( remove );
@@ -752,8 +757,9 @@ describe( 'debug tools', () => {
 			otherRoot.appendChildren( [ firstEle, removedEle ] );
 
 			const delta = new MergeDelta();
+			const graveyard = modelDoc.graveyard;
 			const move = new MoveOperation( ModelPosition.createAt( removedEle, 0 ), 3, ModelPosition.createAt( firstEle, 0 ), 0 );
-			const remove = new RemoveOperation( ModelPosition.createBefore( removedEle ), 1, 1 );
+			const remove = new RemoveOperation( ModelPosition.createBefore( removedEle ), 1, ModelPosition.createAt( graveyard, 0 ), 1 );
 
 			delta.addOperation( move );
 			delta.addOperation( remove );
@@ -888,7 +894,7 @@ describe( 'debug tools', () => {
 				transformedBy: JSON.stringify( deltaC ),
 				wasImportant: true,
 				resultIndex: 0,
-				resultsTotal: 1
+				resultsTotal: 2
 			} );
 		} );
 

+ 1 - 0
packages/ckeditor5-engine/tests/model/delta/delta.js

@@ -240,6 +240,7 @@ describe( 'Delta', () => {
 			const operation = new RemoveOperation(
 				new Position( root, [ 2 ] ),
 				2,
+				new Position( doc.graveyard, [ 0 ] ),
 				doc.version
 			);
 

+ 1 - 0
packages/ckeditor5-engine/tests/model/delta/deltafactory.js

@@ -120,6 +120,7 @@ describe( 'DeltaFactory', () => {
 			delta.addOperation( new RemoveOperation(
 				new Position( root, [ 2 ] ),
 				2,
+				new Position( doc.graveyard, [ 0 ] ),
 				doc.version
 			) );
 

+ 2 - 2
packages/ckeditor5-engine/tests/model/delta/mergedelta.js

@@ -103,7 +103,7 @@ describe( 'MergeDelta', () => {
 
 		it( 'should be equal to the position between merged nodes', () => {
 			mergeDelta.operations.push( new MoveOperation( new Position( root, [ 1, 2, 0 ] ), 4, new Position( root, [ 1, 1, 4 ] ) ) );
-			mergeDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 2, 0 ] ), 1 ) );
+			mergeDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 2, 0 ] ), 1, new Position( doc.graveyard, [ 0 ] ) ) );
 
 			expect( mergeDelta.position.root ).to.equal( root );
 			expect( mergeDelta.position.path ).to.deep.equal( [ 1, 2, 0 ] );
@@ -120,7 +120,7 @@ describe( 'MergeDelta', () => {
 
 		it( 'should return correct SplitDelta', () => {
 			mergeDelta.operations.push( new MoveOperation( new Position( root, [ 1, 2, 0 ] ), 4, new Position( root, [ 1, 1, 4 ] ) ) );
-			mergeDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 2, 0 ] ), 1 ) );
+			mergeDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 2, 0 ] ), 1, new Position( doc.graveyard, [ 0 ] ) ) );
 
 			const reversed = mergeDelta.getReversed();
 

+ 12 - 3
packages/ckeditor5-engine/tests/model/delta/removedelta.js

@@ -49,11 +49,12 @@ describe( 'Batch', () => {
 			expect( getNodesAndText( Range.createIn( root.getChild( 1 ) ) ) ).to.equal( 'abcxyz' );
 		} );
 
-		it( 'should create minimal number of operations when removing a range', () => {
+		it( 'should create minimal number of remove deltas, each with only one operation', () => {
 			batch.remove( range );
 
-			expect( batch.deltas.length ).to.equal( 1 );
-			expect( batch.deltas[ 0 ].operations.length ).to.equal( 2 );
+			expect( batch.deltas.length ).to.equal( 2 );
+			expect( batch.deltas[ 0 ].operations.length ).to.equal( 1 );
+			expect( batch.deltas[ 1 ].operations.length ).to.equal( 1 );
 		} );
 
 		it( 'should be chainable', () => {
@@ -72,6 +73,14 @@ describe( 'Batch', () => {
 
 			expect( doc.applyOperation.calledWith( correctDeltaMatcher ) ).to.be.true;
 		} );
+
+		it( 'should create permanent RemoveOperation', () => {
+			batch.remove( div, true );
+
+			const operation = batch.deltas[ 0 ].operations[ 0 ];
+
+			expect( operation.isPermanent ).to.be.true;
+		} );
 	} );
 } );
 

+ 13 - 3
packages/ckeditor5-engine/tests/model/delta/transform/_utils/utils.js

@@ -71,7 +71,11 @@ export function getMergeDelta( position, howManyInPrev, howManyInNext, version )
 	move.isSticky = true;
 
 	delta.addOperation( move );
-	delta.addOperation( new RemoveOperation( position, 1, version + 1 ) );
+
+	const gy = sourcePosition.root.document.graveyard;
+	const gyPos = Position.createAt( gy, 0 );
+
+	delta.addOperation( new RemoveOperation( position, 1, gyPos, version + 1 ) );
 
 	return delta;
 }
@@ -88,7 +92,10 @@ export function getMoveDelta( sourcePosition, howMany, targetPosition, baseVersi
 export function getRemoveDelta( sourcePosition, howMany, baseVersion ) {
 	const delta = new RemoveDelta();
 
-	const remove = new RemoveOperation( sourcePosition, howMany, baseVersion );
+	const gy = sourcePosition.root.document.graveyard;
+	const gyPos = Position.createAt( gy, 0 );
+
+	const remove = new RemoveOperation( sourcePosition, howMany, gyPos, baseVersion );
 	delta.addOperation( remove );
 
 	return delta;
@@ -150,7 +157,10 @@ export function getUnwrapDelta( positionBefore, howManyChildren, version ) {
 	const removePosition = Position.createFromPosition( positionBefore );
 	removePosition.offset += howManyChildren;
 
-	const remove = new RemoveOperation( removePosition, 1, version + 1 );
+	const gy = sourcePosition.root.document.graveyard;
+	const gyPos = Position.createAt( gy, 0 );
+
+	const remove = new RemoveOperation( removePosition, 1, gyPos, version + 1 );
 
 	delta.addOperation( move );
 	delta.addOperation( remove );

+ 1 - 1
packages/ckeditor5-engine/tests/model/delta/transform/insertdelta.js

@@ -90,7 +90,7 @@ describe( 'transform', () => {
 					operations: [
 						{
 							type: ReinsertOperation,
-							sourcePosition: new Position( gy, [ 0, 0 ] ),
+							sourcePosition: new Position( gy, [ 0 ] ),
 							howMany: 1,
 							targetPosition: new Position( root, [ 3, 3, 3 ] ),
 							baseVersion

+ 1 - 1
packages/ckeditor5-engine/tests/model/delta/transform/mergedelta.js

@@ -111,7 +111,7 @@ describe( 'transform', () => {
 							type: RemoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 3 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0, 0 ] ),
+							targetPosition: new Position( gy, [ 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]

+ 1 - 1
packages/ckeditor5-engine/tests/model/delta/transform/movedelta.js

@@ -68,7 +68,7 @@ describe( 'transform', () => {
 							// is treated in OT as `MoveOperation` and might be converted to it. This is why we have to
 							// check whether the operation type is `MoveOperation`. This is all perfectly valid.
 							type: MoveOperation,
-							sourcePosition: new Position( gy, [ 0, 0 ] ),
+							sourcePosition: new Position( gy, [ 0 ] ),
 							howMany: 1,
 							targetPosition: new Position( root, [ 3, 3, 3 ] ),
 							baseVersion

+ 1 - 1
packages/ckeditor5-engine/tests/model/delta/transform/removedelta.js

@@ -60,7 +60,7 @@ describe( 'transform', () => {
 					operations: [
 						{
 							type: MoveOperation,
-							sourcePosition: new Position( gy, [ 0, 0 ] ),
+							sourcePosition: new Position( gy, [ 0 ] ),
 							howMany: 1,
 							targetPosition: new Position( root, [ 3, 3, 3 ] ),
 							baseVersion

+ 3 - 3
packages/ckeditor5-engine/tests/model/delta/transform/unwrapdelta.js

@@ -69,7 +69,7 @@ describe( 'transform', () => {
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 4 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0, 0 ] ),
+							targetPosition: new Position( gy, [ 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]
@@ -89,7 +89,7 @@ describe( 'transform', () => {
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 15 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 1, 0 ] ),
+							targetPosition: new Position( gy, [ 0 ] ),
 							baseVersion: baseVersion + 3
 						}
 					]
@@ -130,7 +130,7 @@ describe( 'transform', () => {
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 4, 12 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0, 0 ] ),
+							targetPosition: new Position( gy, [ 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]

+ 1 - 1
packages/ckeditor5-engine/tests/model/delta/transform/wrapdelta.js

@@ -73,7 +73,7 @@ describe( 'transform', () => {
 							type: MoveOperation,
 							sourcePosition: new Position( root, [ 3, 3, 4 ] ),
 							howMany: 1,
-							targetPosition: new Position( gy, [ 0, 0 ] ),
+							targetPosition: new Position( gy, [ 0 ] ),
 							baseVersion: baseVersion + 1
 						}
 					]

+ 2 - 2
packages/ckeditor5-engine/tests/model/delta/unwrapdelta.js

@@ -91,7 +91,7 @@ describe( 'UnwrapDelta', () => {
 
 		it( 'should be equal to the position before unwrapped node', () => {
 			unwrapDelta.operations.push( new MoveOperation( new Position( root, [ 1, 2, 0 ] ), 4, new Position( root, [ 1, 2 ] ) ) );
-			unwrapDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 6 ] ), 1 ) );
+			unwrapDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 6 ] ), 1, new Position( doc.graveyard, [ 0 ] ) ) );
 
 			expect( unwrapDelta.position.root ).to.equal( root );
 			expect( unwrapDelta.position.path ).to.deep.equal( [ 1, 2 ] );
@@ -108,7 +108,7 @@ describe( 'UnwrapDelta', () => {
 
 		it( 'should return correct WrapDelta', () => {
 			unwrapDelta.operations.push( new MoveOperation( new Position( root, [ 1, 2, 0 ] ), 4, new Position( root, [ 1, 2 ] ) ) );
-			unwrapDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 6 ] ), 1 ) );
+			unwrapDelta.operations.push( new RemoveOperation( new Position( root, [ 1, 6 ] ), 1, new Position( doc.graveyard, [ 0 ] ) ) );
 
 			const reversed = unwrapDelta.getReversed();
 

+ 3 - 5
packages/ckeditor5-engine/tests/model/document/change-event.js

@@ -83,7 +83,7 @@ describe( 'Document change event', () => {
 	it( 'should be fired when multiple nodes are removed and reinserted', () => {
 		root.insertChildren( 0, new Text( 'foo' ) );
 
-		const removeOperation = new RemoveOperation( new Position( root, [ 0 ] ), 3, doc.version );
+		const removeOperation = new RemoveOperation( new Position( root, [ 0 ] ), 3, new Position( doc.graveyard, [ 0 ] ), doc.version );
 		doc.applyOperation( wrapInDelta( removeOperation ) );
 
 		const reinsertOperation = removeOperation.getReversed();
@@ -91,15 +91,13 @@ describe( 'Document change event', () => {
 
 		expect( changes ).to.have.length( 2 );
 
-		const holderElement = graveyard.getChild( 0 );
-
 		expect( types[ 0 ] ).to.equal( 'remove' );
-		expect( changes[ 0 ].range ).to.deep.equal( Range.createFromParentsAndOffsets( holderElement, 0, holderElement, 3 ) );
+		expect( changes[ 0 ].range ).to.deep.equal( Range.createFromParentsAndOffsets( graveyard, 0, graveyard, 3 ) );
 		expect( changes[ 0 ].sourcePosition ).to.deep.equal( Position.createFromParentAndOffset( root, 0 ) );
 
 		expect( types[ 1 ] ).to.equal( 'reinsert' );
 		expect( changes[ 1 ].range ).to.deep.equal( Range.createFromParentsAndOffsets( root, 0, root, 3 ) );
-		expect( changes[ 1 ].sourcePosition ).to.deep.equal( Position.createFromParentAndOffset( holderElement, 0 ) );
+		expect( changes[ 1 ].sourcePosition ).to.deep.equal( Position.createFromParentAndOffset( graveyard, 0 ) );
 	} );
 
 	it( 'should be fired when attribute is inserted', () => {

+ 3 - 0
packages/ckeditor5-engine/tests/model/liveselection.js

@@ -635,6 +635,7 @@ describe( 'LiveSelection', () => {
 					new RemoveOperation(
 						new Position( root, [ 1, 2 ] ),
 						2,
+						new Position( doc.graveyard, [ 0 ] ),
 						doc.version
 					)
 				) );
@@ -649,6 +650,7 @@ describe( 'LiveSelection', () => {
 					new RemoveOperation(
 						new Position( root, [ 1, 2 ] ),
 						2,
+						new Position( doc.graveyard, [ 0 ] ),
 						doc.version
 					)
 				) );
@@ -663,6 +665,7 @@ describe( 'LiveSelection', () => {
 					new RemoveOperation(
 						new Position( root, [ 1 ] ),
 						2,
+						new Position( doc.graveyard, [ 0 ] ),
 						doc.version
 					)
 				) );

+ 5 - 13
packages/ckeditor5-engine/tests/model/operation/reinsertoperation.js

@@ -20,7 +20,7 @@ describe( 'ReinsertOperation', () => {
 		root = doc.createRoot();
 		graveyard = doc.graveyard;
 
-		graveyardPosition = new Position( graveyard, [ 0, 0 ] );
+		graveyardPosition = new Position( graveyard, [ 0 ] );
 		rootPosition = new Position( root, [ 0 ] );
 
 		operation = new ReinsertOperation(
@@ -62,8 +62,7 @@ describe( 'ReinsertOperation', () => {
 	} );
 
 	it( 'should create a correct RemoveOperation as a reverse', () => {
-		// Test reversed operation's target position.
-		graveyard.appendChildren( new Element( '$graveyardHolder' ) );
+		graveyard.appendChildren( new Element( 'x' ) );
 
 		const reverse = operation.getReversed();
 
@@ -71,32 +70,25 @@ describe( 'ReinsertOperation', () => {
 		expect( reverse.baseVersion ).to.equal( 1 );
 		expect( reverse.howMany ).to.equal( 2 );
 		expect( reverse.sourcePosition.isEqual( rootPosition ) ).to.be.true;
-
-		// Reversed `ReinsertOperation` should target back to the same graveyard holder.
 		expect( reverse.targetPosition.isEqual( graveyardPosition ) ).to.be.true;
-
-		// Reversed `ReinsertOperation` should not create new graveyard holder.
-		expect( reverse._needsHolderElement ).to.be.false;
 	} );
 
 	it( 'should undo reinsert set of nodes by applying reverse operation', () => {
 		const reverse = operation.getReversed();
 
-		const element = new Element();
-		element.insertChildren( 0, new Text( 'xx' ) );
-		graveyard.insertChildren( 0, element );
+		graveyard.insertChildren( 0, new Text( 'xx' ) );
 
 		doc.applyOperation( wrapInDelta( operation ) );
 
 		expect( doc.version ).to.equal( 1 );
 		expect( root.maxOffset ).to.equal( 2 );
-		expect( element.maxOffset ).to.equal( 0 );
+		expect( graveyard.maxOffset ).to.equal( 0 );
 
 		doc.applyOperation( wrapInDelta( reverse ) );
 
 		expect( doc.version ).to.equal( 2 );
 		expect( root.maxOffset ).to.equal( 0 );
-		// Don't check `element` - nodes are moved to new holder element.
+		expect( graveyard.maxOffset ).to.equal( 2 );
 	} );
 
 	describe( 'toJSON', () => {

+ 34 - 43
packages/ckeditor5-engine/tests/model/operation/removeoperation.js

@@ -5,6 +5,7 @@
 
 import Document from '../../../src/model/document';
 import ReinsertOperation from '../../../src/model/operation/reinsertoperation';
+import NoOperation from '../../../src/model/operation/nooperation';
 import RemoveOperation from '../../../src/model/operation/removeoperation';
 import MoveOperation from '../../../src/model/operation/moveoperation';
 import Position from '../../../src/model/position';
@@ -25,6 +26,7 @@ describe( 'RemoveOperation', () => {
 		const op = new RemoveOperation(
 			new Position( root, [ 2 ] ),
 			2,
+			new Position( doc.graveyard, [ 0 ] ),
 			doc.version
 		);
 
@@ -35,6 +37,7 @@ describe( 'RemoveOperation', () => {
 		const op = new RemoveOperation(
 			new Position( root, [ 2 ] ),
 			2,
+			new Position( doc.graveyard, [ 0 ] ),
 			doc.version
 		);
 
@@ -45,19 +48,21 @@ describe( 'RemoveOperation', () => {
 		const operation = new RemoveOperation(
 			new Position( root, [ 2 ] ),
 			2,
+			new Position( doc.graveyard, [ 0 ] ),
 			doc.version
 		);
 
 		expect( operation ).to.be.instanceof( MoveOperation );
 	} );
 
-	it( 'should be able to remove set of nodes and append them to holder element in graveyard root', () => {
+	it( 'should be able to remove set of nodes and append them to graveyard root', () => {
 		root.insertChildren( 0, new Text( 'fozbar' ) );
 
 		doc.applyOperation( wrapInDelta(
 			new RemoveOperation(
 				new Position( root, [ 2 ] ),
 				2,
+				new Position( doc.graveyard, [ 0 ] ),
 				doc.version
 			)
 		) );
@@ -66,45 +71,26 @@ describe( 'RemoveOperation', () => {
 		expect( root.maxOffset ).to.equal( 4 );
 		expect( root.getChild( 0 ).data ).to.equal( 'foar' );
 
-		expect( graveyard.maxOffset ).to.equal( 1 );
-		expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'zb' );
-	} );
-
-	it( 'should be able to remove set of nodes and append them to existing element in graveyard root', () => {
-		root.insertChildren( 0, new Text( 'fozbar' ) );
-		graveyard.appendChildren( new Element( '$graveyardHolder' ) );
-
-		const op = new RemoveOperation(
-			new Position( root, [ 0 ] ),
-			1,
-			doc.version
-		);
-
-		// Manually set holder element properties.
-		op._needsHolderElement = false;
-		op._holderElementOffset = 0;
-
-		doc.applyOperation( wrapInDelta( op ) );
-
-		expect( graveyard.maxOffset ).to.equal( 1 );
-		expect( graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'f' );
+		expect( graveyard.maxOffset ).to.equal( 2 );
+		expect( graveyard.getChild( 0 ).data ).to.equal( 'zb' );
 	} );
 
 	it( 'should create RemoveOperation with same parameters when cloned', () => {
 		const pos = new Position( root, [ 2 ] );
 
-		const operation = new RemoveOperation( pos, 2, doc.version );
+		const operation = new RemoveOperation( pos, 2, new Position( doc.graveyard, [ 0 ] ), doc.version );
 		const clone = operation.clone();
 
 		expect( clone ).to.be.instanceof( RemoveOperation );
 		expect( clone.sourcePosition.isEqual( pos ) ).to.be.true;
+		expect( clone.targetPosition.isEqual( operation.targetPosition ) ).to.be.true;
 		expect( clone.howMany ).to.equal( operation.howMany );
 		expect( clone.baseVersion ).to.equal( operation.baseVersion );
 	} );
 
-	it( 'should create a ReinsertOperation as a reverse', () => {
+	it( 'should create ReinsertOperation when reversed', () => {
 		const position = new Position( root, [ 0 ] );
-		const operation = new RemoveOperation( position, 2, 0 );
+		const operation = new RemoveOperation( position, 2, new Position( doc.graveyard, [ 0 ] ), 0 );
 		const reverse = operation.getReversed();
 
 		expect( reverse ).to.be.an.instanceof( ReinsertOperation );
@@ -114,9 +100,19 @@ describe( 'RemoveOperation', () => {
 		expect( reverse.targetPosition.isEqual( position ) ).to.be.true;
 	} );
 
+	it( 'should create NoOperation when reversed if was permanent', () => {
+		const position = new Position( root, [ 0 ] );
+		const operation = new RemoveOperation( position, 2, new Position( doc.graveyard, [ 0 ] ), 0 );
+		operation.isPermanent = true;
+		const reverse = operation.getReversed();
+
+		expect( reverse ).to.be.an.instanceof( NoOperation );
+		expect( reverse.baseVersion ).to.equal( 1 );
+	} );
+
 	it( 'should undo remove set of nodes by applying reverse operation', () => {
 		const position = new Position( root, [ 0 ] );
-		const operation = new RemoveOperation( position, 3, 0 );
+		const operation = new RemoveOperation( position, 3, new Position( doc.graveyard, [ 0 ] ), 0 );
 		const reverse = operation.getReversed();
 
 		root.insertChildren( 0, new Text( 'bar' ) );
@@ -134,18 +130,17 @@ describe( 'RemoveOperation', () => {
 	} );
 
 	it( 'should properly remove a node that is already in a graveyard', () => {
-		doc.graveyard.appendChildren( new Element( '$graveyardHolder', {}, [ new Text( 'foo' ) ] ) );
-
-		const position = new Position( doc.graveyard, [ 0, 0 ] );
-		const operation = new RemoveOperation( position, 1, 0 );
+		doc.graveyard.appendChildren( [ new Element( 'x' ), new Element( 'y' ), new Element( 'z' ) ] );
 
-		operation.targetPosition.path = [ 0, 0 ];
+		const position = new Position( doc.graveyard, [ 2 ] );
+		const operation = new RemoveOperation( position, 1, new Position( doc.graveyard, [ 0 ] ), 0 );
 
 		doc.applyOperation( wrapInDelta( operation ) );
 
-		expect( doc.graveyard.childCount ).to.equal( 2 );
-		expect( doc.graveyard.getChild( 0 ).getChild( 0 ).data ).to.equal( 'f' );
-		expect( doc.graveyard.getChild( 1 ).getChild( 0 ).data ).to.equal( 'oo' );
+		expect( doc.graveyard.childCount ).to.equal( 3 );
+		expect( doc.graveyard.getChild( 0 ).name ).to.equal( 'z' );
+		expect( doc.graveyard.getChild( 1 ).name ).to.equal( 'x' );
+		expect( doc.graveyard.getChild( 2 ).name ).to.equal( 'y' );
 	} );
 
 	describe( 'toJSON', () => {
@@ -153,11 +148,10 @@ describe( 'RemoveOperation', () => {
 			const op = new RemoveOperation(
 				new Position( root, [ 2 ] ),
 				2,
+				new Position( doc.graveyard, [ 0 ] ),
 				doc.version
 			);
 
-			op._needsHolderElement = false;
-
 			const serialized = jsonParseStringify( op );
 
 			expect( serialized ).to.deep.equal( {
@@ -165,9 +159,9 @@ describe( 'RemoveOperation', () => {
 				baseVersion: 0,
 				howMany: 2,
 				isSticky: false,
+				isPermanent: false,
 				sourcePosition: jsonParseStringify( op.sourcePosition ),
-				targetPosition: jsonParseStringify( op.targetPosition ),
-				_needsHolderElement: false
+				targetPosition: jsonParseStringify( op.targetPosition )
 			} );
 		} );
 	} );
@@ -177,13 +171,10 @@ describe( 'RemoveOperation', () => {
 			const op = new RemoveOperation(
 				new Position( root, [ 2 ] ),
 				2,
+				new Position( doc.graveyard, [ 0 ] ),
 				doc.version
 			);
 
-			op._needsHolderElement = false;
-
-			doc.graveyard.appendChildren( [ new Element( '$graveyardHolder' ), new Element( '$graveyardHolder' ) ] );
-
 			const serialized = jsonParseStringify( op );
 			const deserialized = RemoveOperation.fromJSON( serialized, doc );