Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1339 from ckeditor/t/1337
Browse files Browse the repository at this point in the history 
Fix: It should not be possible to move a `model.Node` from a `model.Document` to a `model.DocumentFragment`. Closes #1337.
  • Loading branch information
@szymonkups
szymonkups authored Mar 6, 2018
2 parents a0ad8fe + 2811575 commit 24b97f5
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 52 deletions.
13 changes: 11 additions & 2 deletions src/model/writer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}.
*
Expand Down Expand Up @@ -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 );
}
}
}

Expand Down
78 changes: 28 additions & 50 deletions tests/model/writer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();

Expand Down Expand Up @@ -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' );
Expand All @@ -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' );
Expand All @@ -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' );
Expand All @@ -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' );
Expand All @@ -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()', () => {
Expand Down

0 comments on commit 24b97f5

Please sign in to comment.