diff --git a/src/dev-utils/model.js b/src/dev-utils/model.js index b9b106d2f..4259c8018 100644 --- a/src/dev-utils/model.js +++ b/src/dev-utils/model.js @@ -126,22 +126,8 @@ export function setData( document, data, options = {} ) { const ranges = []; for ( const range of selection.getRanges() ) { - let start, end; - - // Each range returned from `parse()` method has its root placed in DocumentFragment. - // Here we convert each range to have its root re-calculated properly and be placed inside - // model document root. - if ( range.start.parent.is( 'documentFragment' ) ) { - start = ModelPosition.createFromParentAndOffset( modelRoot, range.start.offset ); - } else { - start = ModelPosition.createFromParentAndOffset( range.start.parent, range.start.offset ); - } - - if ( range.end.parent.is( 'documentFragment' ) ) { - end = ModelPosition.createFromParentAndOffset( modelRoot, range.end.offset ); - } else { - end = ModelPosition.createFromParentAndOffset( range.end.parent, range.end.offset ); - } + const start = new ModelPosition( modelRoot, range.start.path ); + const end = new ModelPosition( modelRoot, range.end.path ); ranges.push( new ModelRange( start, end ) ); } diff --git a/src/dev-utils/view.js b/src/dev-utils/view.js index b6924af5c..fc6cacc3f 100644 --- a/src/dev-utils/view.js +++ b/src/dev-utils/view.js @@ -852,7 +852,9 @@ function _convertViewElements( rootNode ) { const convertedElement = rootNode.is( 'documentFragment' ) ? new ViewDocumentFragment() : _convertElement( rootNode ); // Convert all child nodes. - for ( const child of rootNode.getChildren() ) { + // Cache the nodes in array. Otherwise, we would skip some nodes because during iteration we move nodes + // from `rootNode` to `convertedElement`. This would interfere with iteration. + for ( const child of [ ...rootNode.getChildren() ] ) { if ( convertedElement.is( 'emptyElement' ) ) { throw new Error( 'Parse error - cannot parse inside EmptyElement.' ); } diff --git a/src/model/documentfragment.js b/src/model/documentfragment.js index 626c4e097..7f1c671ae 100644 --- a/src/model/documentfragment.js +++ b/src/model/documentfragment.js @@ -235,6 +235,11 @@ export default class DocumentFragment { nodes = normalize( nodes ); for ( const node of nodes ) { + // If node that is being added to this element is already inside another element, first remove it from the old parent. + if ( node.parent !== null ) { + node.remove(); + } + node.parent = this; } diff --git a/src/model/element.js b/src/model/element.js index 5892e6000..fcceac9f5 100644 --- a/src/model/element.js +++ b/src/model/element.js @@ -202,6 +202,11 @@ export default class Element extends Node { nodes = normalize( nodes ); for ( const node of nodes ) { + // If node that is being added to this element is already inside another element, first remove it from the old parent. + if ( node.parent !== null ) { + node.remove(); + } + node.parent = this; } diff --git a/src/view/documentfragment.js b/src/view/documentfragment.js index 9366c357d..8df44d9c5 100644 --- a/src/view/documentfragment.js +++ b/src/view/documentfragment.js @@ -150,6 +150,11 @@ export default class DocumentFragment { nodes = normalize( nodes ); for ( const node of nodes ) { + // If node that is being added to this element is already inside another element, first remove it from the old parent. + if ( node.parent !== null ) { + node.remove(); + } + node.parent = this; this._children.splice( index, 0, node ); diff --git a/src/view/element.js b/src/view/element.js index 9741af509..c1dc76fec 100644 --- a/src/view/element.js +++ b/src/view/element.js @@ -355,6 +355,11 @@ export default class Element extends Node { nodes = normalize( nodes ); for ( const node of nodes ) { + // If node that is being added to this element is already inside another element, first remove it from the old parent. + if ( node.parent !== null ) { + node.remove(); + } + node.parent = this; this._children.splice( index, 0, node ); diff --git a/tests/conversion/advanced-converters.js b/tests/conversion/advanced-converters.js index 65935b59a..02b7bbfee 100644 --- a/tests/conversion/advanced-converters.js +++ b/tests/conversion/advanced-converters.js @@ -287,8 +287,6 @@ describe( 'advanced-converters', () => { it( 'should convert view image to model', () => { const viewElement = new ViewContainerElement( 'img', { src: 'bar.jpg', title: 'bar' } ); const modelElement = viewDispatcher.convert( viewElement ); - // Attaching to tree so tree walker works fine in `modelToString`. - modelRoot.appendChildren( modelElement ); expect( modelToString( modelElement ) ).to.equal( '' ); } ); @@ -303,8 +301,6 @@ describe( 'advanced-converters', () => { ] ); const modelElement = viewDispatcher.convert( viewElement ); - // Attaching to tree so tree walker works fine in `modelToString`. - modelRoot.appendChildren( modelElement ); expect( modelToString( modelElement ) ).to.equal( 'foobar' ); } ); @@ -595,7 +591,6 @@ describe( 'advanced-converters', () => { ); const modelElement = viewDispatcher.convert( viewElement ); - modelRoot.appendChildren( modelElement ); expect( modelToString( modelElement ) ).to.equal( 'foo' ); } ); @@ -761,7 +756,6 @@ describe( 'advanced-converters', () => { ] ); const modelElement = viewDispatcher.convert( viewElement ); - modelRoot.appendChildren( modelElement ); expect( modelToString( modelElement ) ).to.equal( '' + diff --git a/tests/conversion/buildviewconverter.js b/tests/conversion/buildviewconverter.js index b381f08a6..0b485f10b 100644 --- a/tests/conversion/buildviewconverter.js +++ b/tests/conversion/buildviewconverter.js @@ -7,7 +7,6 @@ import buildViewConverter from '../../src/conversion/buildviewconverter'; import ModelSchema from '../../src/model/schema'; import ModelDocumentFragment from '../../src/model/documentfragment'; -import ModelDocument from '../../src/model/document'; import ModelElement from '../../src/model/element'; import ModelTextProxy from '../../src/model/textproxy'; import ModelRange from '../../src/model/range'; @@ -64,7 +63,7 @@ const textAttributes = [ undefined, 'linkHref', 'linkTitle', 'bold', 'italic', ' const pAttributes = [ undefined, 'class', 'important', 'theme', 'decorated', 'size' ]; describe( 'View converter builder', () => { - let dispatcher, modelDoc, modelRoot, schema, objWithContext; + let dispatcher, schema, objWithContext; beforeEach( () => { // `additionalData` parameter for `.convert` calls. @@ -91,16 +90,12 @@ describe( 'View converter builder', () => { dispatcher = new ViewConversionDispatcher( { schema } ); dispatcher.on( 'text', convertText() ); - - modelDoc = new ModelDocument(); - modelRoot = modelDoc.createRoot(); } ); it( 'should convert from view element to model element', () => { buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' ); const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( conversionResult ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); @@ -111,7 +106,6 @@ describe( 'View converter builder', () => { .toElement( viewElement => new ModelElement( 'image', { src: viewElement.getAttribute( 'src' ) } ) ); const conversionResult = dispatcher.convert( new ViewContainerElement( 'img', { src: 'foo.jpg' } ), objWithContext ); - modelRoot.appendChildren( conversionResult ); expect( modelToString( conversionResult ) ).to.equal( '' ); } ); @@ -122,10 +116,9 @@ describe( 'View converter builder', () => { const conversionResult = dispatcher.convert( new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( conversionResult ); // Have to check root because result is a ModelText. - expect( modelToString( modelRoot ) ).to.equal( '<$root><$text bold="true">foo' ); + expect( modelToString( conversionResult ) ).to.equal( '<$text bold="true">foo' ); } ); it( 'should convert from view element to model attributes using creator function', () => { @@ -136,10 +129,9 @@ describe( 'View converter builder', () => { const conversionResult = dispatcher.convert( new ViewAttributeElement( 'a', { href: 'foo.html' }, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( conversionResult ); // Have to check root because result is a ModelText. - expect( modelToString( modelRoot ) ).to.equal( '<$root><$text linkHref="foo.html">foo' ); + expect( modelToString( conversionResult ) ).to.equal( '<$text linkHref="foo.html">foo' ); } ); it( 'should convert from view attribute to model attribute', () => { @@ -152,7 +144,6 @@ describe( 'View converter builder', () => { const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( conversionResult ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); @@ -200,7 +191,6 @@ describe( 'View converter builder', () => { ] ); const conversionResult = dispatcher.convert( viewElement, objWithContext ); - modelRoot.appendChildren( conversionResult ); expect( modelToString( conversionResult ) ).to.equal( '<$text bold="true">aaabbbcccddd' ); } ); @@ -337,7 +327,7 @@ describe( 'View converter builder', () => { result = dispatcher.convert( new ViewContainerElement( 'span', { class: 'megatron' }, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( result ); + expect( modelToString( result ) ).to.equal( 'foo' ); // Almost a megatron. Missing a head. @@ -346,7 +336,6 @@ describe( 'View converter builder', () => { objWithContext ); - modelRoot.appendChildren( result ); expect( modelToString( result ) ).to.equal( 'foo' ); // This would be a megatron but is a paragraph. @@ -359,7 +348,6 @@ describe( 'View converter builder', () => { objWithContext ); - modelRoot.appendChildren( result ); expect( modelToString( result ) ).to.equal( 'foo' ); // At last we have a megatron! @@ -372,7 +360,6 @@ describe( 'View converter builder', () => { objWithContext ); - modelRoot.appendChildren( result ); expect( modelToString( result ) ).to.equal( 'foo' ); } ); @@ -392,7 +379,6 @@ describe( 'View converter builder', () => { const conversionResult = dispatcher.convert( viewElement, objWithContext ); - modelRoot.appendChildren( conversionResult ); expect( modelToString( conversionResult ) ).to.equal( 'foo' ); } ); @@ -415,7 +401,6 @@ describe( 'View converter builder', () => { const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( conversionResult ); // Element converter was fired first even though attribute converter was added first. expect( modelToString( conversionResult ) ).to.equal( 'foo' ); @@ -432,7 +417,7 @@ describe( 'View converter builder', () => { result = dispatcher.convert( new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( result ); + expect( modelToString( result ) ).to.equal( 'foo' ); buildViewConverter().for( dispatcher ) @@ -442,7 +427,7 @@ describe( 'View converter builder', () => { result = dispatcher.convert( new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext ); - modelRoot.appendChildren( result ); + expect( modelToString( result ) ).to.equal( 'foo' ); } ); @@ -461,9 +446,7 @@ describe( 'View converter builder', () => { .toAttribute( 'size', 'small' ); const viewElement = new ViewContainerElement( 'p', { class: 'decorated small' }, new ViewText( 'foo' ) ); - const conversionResult = dispatcher.convert( viewElement, objWithContext ); - modelRoot.appendChildren( conversionResult ); // P element and it's children got converted by the converter (1) and the converter (1) got fired // because P name was not consumed in converter (2). Converter (3) could consume class="small" because @@ -487,7 +470,6 @@ describe( 'View converter builder', () => { ] ); const conversionResult = dispatcher.convert( viewStructure, objWithContext ); - modelRoot.appendChildren( conversionResult ); expect( modelToString( conversionResult ) ).to.equal( '
foo
' ); } ); diff --git a/tests/model/node.js b/tests/model/node.js index 2710f30a0..d7c3430c8 100644 --- a/tests/model/node.js +++ b/tests/model/node.js @@ -16,7 +16,7 @@ describe( 'Node', () => { one, two, three, textBA, textR, img; - before( () => { + beforeEach( () => { node = new Node(); one = new Element( 'one' );