Merge pull request #1339 from ckeditor/t/1337

Fix: It should not be possible to move a `model.Node` from a `model.Document` to a `model.DocumentFragment`. Closes #1337.

Author: szymonkups

src/model/writer.js

@@ -144,6 +144,9 @@ export default class Writer {
 	 *
 	 * Note that if the item already has parent it will be removed from the previous parent.
 	 *
+	 * Note that you cannot re-insert a node from a document to a different document or document fragment. In this case,
+	 * `model-writer-insert-forbidden-move` is thrown.
+	 *
 	 * If you want to move {@link module:engine/model/range~Range range} instead of an
 	 * {@link module:engine/model/item~Item item} use {@link module:engine/model/writer~Writer#move move}.
 	 *
@@ -172,8 +175,14 @@ export default class Writer {
 			}
 			// If it isn't the same root.
 			else {
-				// We need to remove this item from old position first.
-				this.remove( item );
+				if ( item.root.document ) {
+					// It is forbidden to move a node that was already in a document outside of it.
+					throw new Error( 'model-writer-insert-forbidden-move: Cannot move a node from a document to a different tree.' );
+				} else {
+					// Move between two different document fragments or from document fragment to a document is possible.
+					// In that case, remove the item from it's original parent.
+					this.remove( item );
+				}
 			}
 		}
 

tests/model/writer.js

@@ -258,29 +258,6 @@ describe( 'Writer', () => {
 			expect( spy.firstCall.args[ 0 ].delta.batch ).to.equal( batch );
 		} );
 
-		it( 'should move element from one parent to the other within different document (document -> docFrag)', () => {
-			const root = doc.createRoot();
-			const docFrag = createDocumentFragment();
-			const node = createText( 'foo' );
-
-			insert( node, root );
-
-			const spy = sinon.spy( model, 'applyOperation' );
-
-			insert( node, docFrag );
-
-			// Verify result.
-			expect( Array.from( root.getChildren() ) ).to.deep.equal( [] );
-			expect( Array.from( docFrag.getChildren() ) ).to.deep.equal( [ node ] );
-
-			// Verify deltas and operations.
-			sinon.assert.calledTwice( spy );
-			expect( spy.firstCall.args[ 0 ].type ).to.equal( 'remove' );
-			expect( spy.firstCall.args[ 0 ].delta.batch ).to.equal( batch );
-			expect( spy.secondCall.args[ 0 ].type ).to.equal( 'insert' );
-			expect( spy.secondCall.args[ 0 ].delta.batch ).to.equal( batch );
-		} );
-
 		it( 'should move element from one parent to the other within different document (docFragA -> docFragB)', () => {
 			const docFragA = createDocumentFragment();
 			const docFragB = createDocumentFragment();
@@ -304,6 +281,18 @@ describe( 'Writer', () => {
 			expect( spy.secondCall.args[ 0 ].delta.batch ).to.equal( batch );
 		} );
 
+		it( 'should throw when moving element from document to document fragment', () => {
+			const root = doc.createRoot();
+			const docFrag = createDocumentFragment();
+			const node = createText( 'foo' );
+
+			insert( node, root );
+
+			expect( () => {
+				insert( node, docFrag );
+			} ).to.throw();
+		} );
+
 		it( 'should transfer markers from given DocumentFragment', () => {
 			const root = doc.createRoot();
 
@@ -608,7 +597,7 @@ describe( 'Writer', () => {
 			expect( spy.lastCall.args[ 0 ].delta.batch ).to.equal( batch );
 		} );
 
-		it( 'should move element from one parent to the other within the same document (documentA -> documentA)', () => {
+		it( 'should move element from one parent to the other within the same root (rootA -> rootA)', () => {
 			const rootA = doc.createRoot();
 
 			const parent1 = createElement( 'parent' );
@@ -633,7 +622,7 @@ describe( 'Writer', () => {
 			expect( spy.firstCall.args[ 0 ].delta.batch ).to.equal( batch );
 		} );
 
-		it( 'should move element from one parent to the other within the same document (documentA -> documentB)', () => {
+		it( 'should move element from one parent to the other within the same document (rootA -> rootB)', () => {
 			const rootA = doc.createRoot( '$root', 'A' );
 			const rootB = doc.createRoot( '$root', 'B' );
 			const node = createText( 'foo' );
@@ -654,7 +643,7 @@ describe( 'Writer', () => {
 			expect( spy.firstCall.args[ 0 ].delta.batch ).to.equal( batch );
 		} );
 
-		it( 'should move element from one parent to the other within the same document (docFragA -> docFragA)', () => {
+		it( 'should move element from one parent to the other within the same document fragment (docFragA -> docFragA)', () => {
 			const docFragA = createDocumentFragment();
 			const parent1 = createElement( 'parent' );
 			const parent2 = createElement( 'parent' );
@@ -678,30 +667,7 @@ describe( 'Writer', () => {
 			expect( spy.firstCall.args[ 0 ].delta.batch ).to.equal( batch );
 		} );
 
-		it( 'should move element from one parent to the other within different document (document -> docFrag)', () => {
-			const root = doc.createRoot();
-			const docFrag = createDocumentFragment();
-			const node = createText( 'foo' );
-
-			insert( node, root );
-
-			const spy = sinon.spy( model, 'applyOperation' );
-
-			append( node, docFrag );
-
-			// Verify result.
-			expect( Array.from( root.getChildren() ) ).to.deep.equal( [] );
-			expect( Array.from( docFrag.getChildren() ) ).to.deep.equal( [ node ] );
-
-			// Verify deltas and operations.
-			sinon.assert.calledTwice( spy );
-			expect( spy.firstCall.args[ 0 ].type ).to.equal( 'remove' );
-			expect( spy.firstCall.args[ 0 ].delta.batch ).to.equal( batch );
-			expect( spy.secondCall.args[ 0 ].type ).to.equal( 'insert' );
-			expect( spy.secondCall.args[ 0 ].delta.batch ).to.equal( batch );
-		} );
-
-		it( 'should move element from one parent to the other within different document (docFragA -> docFragB)', () => {
+		it( 'should move element from one parent to the other within different document fragments (docFragA -> docFragB)', () => {
 			const docFragA = createDocumentFragment();
 			const docFragB = createDocumentFragment();
 			const node = createText( 'foo' );
@@ -723,6 +689,18 @@ describe( 'Writer', () => {
 			expect( spy.secondCall.args[ 0 ].type ).to.equal( 'insert' );
 			expect( spy.secondCall.args[ 0 ].delta.batch ).to.equal( batch );
 		} );
+
+		it( 'should throw when moving element from document to document fragment', () => {
+			const root = doc.createRoot();
+			const docFrag = createDocumentFragment();
+			const node = createText( 'foo' );
+
+			insert( node, root );
+
+			expect( () => {
+				append( node, docFrag );
+			} ).to.throw();
+		} );
 	} );
 
 	describe( 'appendText()', () => {