Browse Source

Merge pull request #1141 from ckeditor/t/1139b

Fix: View and model nodes will now be removed from their old parents when they are added to a new parent to prevent having same node on multiple elements' children lists. Closes #1139.

BREAKING CHANGE: View and model nodes are now automatically removed from their old parents when they are inserted into new elements. This is important e.g. if you iterate through element's children and they are moved during that iteration. In that case, it's safest to cache the element's children in an array.
Piotrek Koszuliński 8 years ago
parent
commit
0f34e8a8f9

+ 2 - 16
packages/ckeditor5-engine/src/dev-utils/model.js

@@ -126,22 +126,8 @@ export function setData( document, data, options = {} ) {
 			const ranges = [];
 			const ranges = [];
 
 
 			for ( const range of selection.getRanges() ) {
 			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 ) );
 				ranges.push( new ModelRange( start, end ) );
 			}
 			}

+ 3 - 1
packages/ckeditor5-engine/src/dev-utils/view.js

@@ -852,7 +852,9 @@ function _convertViewElements( rootNode ) {
 		const convertedElement = rootNode.is( 'documentFragment' ) ? new ViewDocumentFragment() : _convertElement( rootNode );
 		const convertedElement = rootNode.is( 'documentFragment' ) ? new ViewDocumentFragment() : _convertElement( rootNode );
 
 
 		// Convert all child nodes.
 		// 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' ) ) {
 			if ( convertedElement.is( 'emptyElement' ) ) {
 				throw new Error( 'Parse error - cannot parse inside EmptyElement.' );
 				throw new Error( 'Parse error - cannot parse inside EmptyElement.' );
 			}
 			}

+ 5 - 0
packages/ckeditor5-engine/src/model/documentfragment.js

@@ -235,6 +235,11 @@ export default class DocumentFragment {
 		nodes = normalize( nodes );
 		nodes = normalize( nodes );
 
 
 		for ( const node of 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;
 			node.parent = this;
 		}
 		}
 
 

+ 5 - 0
packages/ckeditor5-engine/src/model/element.js

@@ -202,6 +202,11 @@ export default class Element extends Node {
 		nodes = normalize( nodes );
 		nodes = normalize( nodes );
 
 
 		for ( const node of 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;
 			node.parent = this;
 		}
 		}
 
 

+ 5 - 0
packages/ckeditor5-engine/src/view/documentfragment.js

@@ -150,6 +150,11 @@ export default class DocumentFragment {
 		nodes = normalize( nodes );
 		nodes = normalize( nodes );
 
 
 		for ( const node of 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;
 			node.parent = this;
 
 
 			this._children.splice( index, 0, node );
 			this._children.splice( index, 0, node );

+ 5 - 0
packages/ckeditor5-engine/src/view/element.js

@@ -355,6 +355,11 @@ export default class Element extends Node {
 		nodes = normalize( nodes );
 		nodes = normalize( nodes );
 
 
 		for ( const node of 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;
 			node.parent = this;
 
 
 			this._children.splice( index, 0, node );
 			this._children.splice( index, 0, node );

+ 0 - 6
packages/ckeditor5-engine/tests/conversion/advanced-converters.js

@@ -287,8 +287,6 @@ describe( 'advanced-converters', () => {
 		it( 'should convert view image to model', () => {
 		it( 'should convert view image to model', () => {
 			const viewElement = new ViewContainerElement( 'img', { src: 'bar.jpg', title: 'bar' } );
 			const viewElement = new ViewContainerElement( 'img', { src: 'bar.jpg', title: 'bar' } );
 			const modelElement = viewDispatcher.convert( viewElement );
 			const modelElement = viewDispatcher.convert( viewElement );
-			// Attaching to tree so tree walker works fine in `modelToString`.
-			modelRoot.appendChildren( modelElement );
 
 
 			expect( modelToString( modelElement ) ).to.equal( '<image src="bar.jpg" title="bar"></image>' );
 			expect( modelToString( modelElement ) ).to.equal( '<image src="bar.jpg" title="bar"></image>' );
 		} );
 		} );
@@ -303,8 +301,6 @@ describe( 'advanced-converters', () => {
 				]
 				]
 			);
 			);
 			const modelElement = viewDispatcher.convert( viewElement );
 			const modelElement = viewDispatcher.convert( viewElement );
-			// Attaching to tree so tree walker works fine in `modelToString`.
-			modelRoot.appendChildren( modelElement );
 
 
 			expect( modelToString( modelElement ) ).to.equal( '<image src="bar.jpg" title="bar"><caption>foobar</caption></image>' );
 			expect( modelToString( modelElement ) ).to.equal( '<image src="bar.jpg" title="bar"><caption>foobar</caption></image>' );
 		} );
 		} );
@@ -595,7 +591,6 @@ describe( 'advanced-converters', () => {
 			);
 			);
 
 
 			const modelElement = viewDispatcher.convert( viewElement );
 			const modelElement = viewDispatcher.convert( viewElement );
-			modelRoot.appendChildren( modelElement );
 
 
 			expect( modelToString( modelElement ) ).to.equal( '<quote linkHref="foo.html" linkTitle="Foo source">foo</quote>' );
 			expect( modelToString( modelElement ) ).to.equal( '<quote linkHref="foo.html" linkTitle="Foo source">foo</quote>' );
 		} );
 		} );
@@ -761,7 +756,6 @@ describe( 'advanced-converters', () => {
 			] );
 			] );
 
 
 			const modelElement = viewDispatcher.convert( viewElement );
 			const modelElement = viewDispatcher.convert( viewElement );
-			modelRoot.appendChildren( modelElement );
 
 
 			expect( modelToString( modelElement ) ).to.equal(
 			expect( modelToString( modelElement ) ).to.equal(
 				'<table cellpadding="5" cellspacing="5">' +
 				'<table cellpadding="5" cellspacing="5">' +

+ 6 - 24
packages/ckeditor5-engine/tests/conversion/buildviewconverter.js

@@ -7,7 +7,6 @@ import buildViewConverter from '../../src/conversion/buildviewconverter';
 
 
 import ModelSchema from '../../src/model/schema';
 import ModelSchema from '../../src/model/schema';
 import ModelDocumentFragment from '../../src/model/documentfragment';
 import ModelDocumentFragment from '../../src/model/documentfragment';
-import ModelDocument from '../../src/model/document';
 import ModelElement from '../../src/model/element';
 import ModelElement from '../../src/model/element';
 import ModelTextProxy from '../../src/model/textproxy';
 import ModelTextProxy from '../../src/model/textproxy';
 import ModelRange from '../../src/model/range';
 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' ];
 const pAttributes = [ undefined, 'class', 'important', 'theme', 'decorated', 'size' ];
 
 
 describe( 'View converter builder', () => {
 describe( 'View converter builder', () => {
-	let dispatcher, modelDoc, modelRoot, schema, objWithContext;
+	let dispatcher, schema, objWithContext;
 
 
 	beforeEach( () => {
 	beforeEach( () => {
 		// `additionalData` parameter for `.convert` calls.
 		// `additionalData` parameter for `.convert` calls.
@@ -91,16 +90,12 @@ describe( 'View converter builder', () => {
 
 
 		dispatcher = new ViewConversionDispatcher( { schema } );
 		dispatcher = new ViewConversionDispatcher( { schema } );
 		dispatcher.on( 'text', convertText() );
 		dispatcher.on( 'text', convertText() );
-
-		modelDoc = new ModelDocument();
-		modelRoot = modelDoc.createRoot();
 	} );
 	} );
 
 
 	it( 'should convert from view element to model element', () => {
 	it( 'should convert from view element to model element', () => {
 		buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' );
 		buildViewConverter().for( dispatcher ).fromElement( 'p' ).toElement( 'paragraph' );
 
 
 		const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ), objWithContext );
 		const conversionResult = dispatcher.convert( new ViewContainerElement( 'p', null, new ViewText( 'foo' ) ), objWithContext );
-		modelRoot.appendChildren( conversionResult );
 
 
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph>foo</paragraph>' );
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph>foo</paragraph>' );
 	} );
 	} );
@@ -111,7 +106,6 @@ describe( 'View converter builder', () => {
 			.toElement( viewElement => new ModelElement( 'image', { src: viewElement.getAttribute( 'src' ) } ) );
 			.toElement( viewElement => new ModelElement( 'image', { src: viewElement.getAttribute( 'src' ) } ) );
 
 
 		const conversionResult = dispatcher.convert( new ViewContainerElement( 'img', { src: 'foo.jpg' } ), objWithContext );
 		const conversionResult = dispatcher.convert( new ViewContainerElement( 'img', { src: 'foo.jpg' } ), objWithContext );
-		modelRoot.appendChildren( conversionResult );
 
 
 		expect( modelToString( conversionResult ) ).to.equal( '<image src="foo.jpg"></image>' );
 		expect( modelToString( conversionResult ) ).to.equal( '<image src="foo.jpg"></image>' );
 	} );
 	} );
@@ -122,10 +116,9 @@ describe( 'View converter builder', () => {
 		const conversionResult = dispatcher.convert(
 		const conversionResult = dispatcher.convert(
 			new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), objWithContext
 			new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) ), objWithContext
 		);
 		);
-		modelRoot.appendChildren( conversionResult );
 
 
 		// Have to check root because result is a ModelText.
 		// Have to check root because result is a ModelText.
-		expect( modelToString( modelRoot ) ).to.equal( '<$root><$text bold="true">foo</$text></$root>' );
+		expect( modelToString( conversionResult ) ).to.equal( '<$text bold="true">foo</$text>' );
 	} );
 	} );
 
 
 	it( 'should convert from view element to model attributes using creator function', () => {
 	it( 'should convert from view element to model attributes using creator function', () => {
@@ -136,10 +129,9 @@ describe( 'View converter builder', () => {
 		const conversionResult = dispatcher.convert(
 		const conversionResult = dispatcher.convert(
 			new ViewAttributeElement( 'a', { href: 'foo.html' }, new ViewText( 'foo' ) ), objWithContext
 			new ViewAttributeElement( 'a', { href: 'foo.html' }, new ViewText( 'foo' ) ), objWithContext
 		);
 		);
-		modelRoot.appendChildren( conversionResult );
 
 
 		// Have to check root because result is a ModelText.
 		// Have to check root because result is a ModelText.
-		expect( modelToString( modelRoot ) ).to.equal( '<$root><$text linkHref="foo.html">foo</$text></$root>' );
+		expect( modelToString( conversionResult ) ).to.equal( '<$text linkHref="foo.html">foo</$text>' );
 	} );
 	} );
 
 
 	it( 'should convert from view attribute to model attribute', () => {
 	it( 'should convert from view attribute to model attribute', () => {
@@ -152,7 +144,6 @@ describe( 'View converter builder', () => {
 		const conversionResult = dispatcher.convert(
 		const conversionResult = dispatcher.convert(
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 		);
 		);
-		modelRoot.appendChildren( conversionResult );
 
 
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph class="myClass">foo</paragraph>' );
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph class="myClass">foo</paragraph>' );
 	} );
 	} );
@@ -200,7 +191,6 @@ describe( 'View converter builder', () => {
 		] );
 		] );
 
 
 		const conversionResult = dispatcher.convert( viewElement, objWithContext );
 		const conversionResult = dispatcher.convert( viewElement, objWithContext );
-		modelRoot.appendChildren( conversionResult );
 
 
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph><$text bold="true">aaabbbcccddd</$text></paragraph>' );
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph><$text bold="true">aaabbbcccddd</$text></paragraph>' );
 	} );
 	} );
@@ -337,7 +327,7 @@ describe( 'View converter builder', () => {
 		result = dispatcher.convert(
 		result = dispatcher.convert(
 			new ViewContainerElement( 'span', { class: 'megatron' }, new ViewText( 'foo' ) ), objWithContext
 			new ViewContainerElement( 'span', { class: 'megatron' }, new ViewText( 'foo' ) ), objWithContext
 		);
 		);
-		modelRoot.appendChildren( result );
+
 		expect( modelToString( result ) ).to.equal( '<span>foo</span>' );
 		expect( modelToString( result ) ).to.equal( '<span>foo</span>' );
 
 
 		// Almost a megatron. Missing a head.
 		// Almost a megatron. Missing a head.
@@ -346,7 +336,6 @@ describe( 'View converter builder', () => {
 			objWithContext
 			objWithContext
 		);
 		);
 
 
-		modelRoot.appendChildren( result );
 		expect( modelToString( result ) ).to.equal( '<span>foo</span>' );
 		expect( modelToString( result ) ).to.equal( '<span>foo</span>' );
 
 
 		// This would be a megatron but is a paragraph.
 		// This would be a megatron but is a paragraph.
@@ -359,7 +348,6 @@ describe( 'View converter builder', () => {
 			objWithContext
 			objWithContext
 		);
 		);
 
 
-		modelRoot.appendChildren( result );
 		expect( modelToString( result ) ).to.equal( '<paragraph>foo</paragraph>' );
 		expect( modelToString( result ) ).to.equal( '<paragraph>foo</paragraph>' );
 
 
 		// At last we have a megatron!
 		// At last we have a megatron!
@@ -372,7 +360,6 @@ describe( 'View converter builder', () => {
 			objWithContext
 			objWithContext
 		);
 		);
 
 
-		modelRoot.appendChildren( result );
 		expect( modelToString( result ) ).to.equal( '<MEGATRON>foo</MEGATRON>' );
 		expect( modelToString( result ) ).to.equal( '<MEGATRON>foo</MEGATRON>' );
 	} );
 	} );
 
 
@@ -392,7 +379,6 @@ describe( 'View converter builder', () => {
 
 
 		const conversionResult = dispatcher.convert( viewElement, objWithContext );
 		const conversionResult = dispatcher.convert( viewElement, objWithContext );
 
 
-		modelRoot.appendChildren( conversionResult );
 		expect( modelToString( conversionResult ) ).to.equal( '<span transformer="megatron">foo</span>' );
 		expect( modelToString( conversionResult ) ).to.equal( '<span transformer="megatron">foo</span>' );
 	} );
 	} );
 
 
@@ -415,7 +401,6 @@ describe( 'View converter builder', () => {
 		const conversionResult = dispatcher.convert(
 		const conversionResult = dispatcher.convert(
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 		);
 		);
-		modelRoot.appendChildren( conversionResult );
 
 
 		// Element converter was fired first even though attribute converter was added first.
 		// Element converter was fired first even though attribute converter was added first.
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph class="myClass">foo</paragraph>' );
 		expect( modelToString( conversionResult ) ).to.equal( '<paragraph class="myClass">foo</paragraph>' );
@@ -432,7 +417,7 @@ describe( 'View converter builder', () => {
 		result = dispatcher.convert(
 		result = dispatcher.convert(
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 		);
 		);
-		modelRoot.appendChildren( result );
+
 		expect( modelToString( result ) ).to.equal( '<paragraph class="myClass">foo</paragraph>' );
 		expect( modelToString( result ) ).to.equal( '<paragraph class="myClass">foo</paragraph>' );
 
 
 		buildViewConverter().for( dispatcher )
 		buildViewConverter().for( dispatcher )
@@ -442,7 +427,7 @@ describe( 'View converter builder', () => {
 		result = dispatcher.convert(
 		result = dispatcher.convert(
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 			new ViewContainerElement( 'p', { class: 'myClass' }, new ViewText( 'foo' ) ), objWithContext
 		);
 		);
-		modelRoot.appendChildren( result );
+
 		expect( modelToString( result ) ).to.equal( '<customP>foo</customP>' );
 		expect( modelToString( result ) ).to.equal( '<customP>foo</customP>' );
 	} );
 	} );
 
 
@@ -461,9 +446,7 @@ describe( 'View converter builder', () => {
 			.toAttribute( 'size', 'small' );
 			.toAttribute( 'size', 'small' );
 
 
 		const viewElement = new ViewContainerElement( 'p', { class: 'decorated small' }, new ViewText( 'foo' ) );
 		const viewElement = new ViewContainerElement( 'p', { class: 'decorated small' }, new ViewText( 'foo' ) );
-
 		const conversionResult = dispatcher.convert( viewElement, objWithContext );
 		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
 		// 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
 		// 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 );
 		const conversionResult = dispatcher.convert( viewStructure, objWithContext );
-		modelRoot.appendChildren( conversionResult );
 
 
 		expect( modelToString( conversionResult ) ).to.equal( '<div class="myClass"><abcd>foo</abcd></div>' );
 		expect( modelToString( conversionResult ) ).to.equal( '<div class="myClass"><abcd>foo</abcd></div>' );
 	} );
 	} );

+ 1 - 1
packages/ckeditor5-engine/tests/model/node.js

@@ -16,7 +16,7 @@ describe( 'Node', () => {
 		one, two, three,
 		one, two, three,
 		textBA, textR, img;
 		textBA, textR, img;
 
 
-	before( () => {
+	beforeEach( () => {
 		node = new Node();
 		node = new Node();
 
 
 		one = new Element( 'one' );
 		one = new Element( 'one' );