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 #1141 from ckeditor/t/1139b
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@Reinmar
Reinmar authored Sep 15, 2017
2 parents 1be7ed1 + 7a90bc2 commit dec9c28
Show file tree
Hide file tree
Showing 9 changed files with 32 additions and 48 deletions.
18 changes: 2 additions & 16 deletions src/dev-utils/model.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
}
Expand Down
4 changes: 3 additions & 1 deletion src/dev-utils/view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.' );
}
Expand Down
5 changes: 5 additions & 0 deletions src/model/documentfragment.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
5 changes: 5 additions & 0 deletions src/model/element.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
5 changes: 5 additions & 0 deletions src/view/documentfragment.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
5 changes: 5 additions & 0 deletions src/view/element.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
6 changes: 0 additions & 6 deletions tests/conversion/advanced-converters.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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( '<image src="bar.jpg" title="bar"></image>' );
} );
Expand All @@ -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( '<image src="bar.jpg" title="bar"><caption>foobar</caption></image>' );
} );
Expand Down Expand Up @@ -595,7 +591,6 @@ describe( 'advanced-converters', () => {
);

const modelElement = viewDispatcher.convert( viewElement );
modelRoot.appendChildren( modelElement );

expect( modelToString( modelElement ) ).to.equal( '<quote linkHref="foo.html" linkTitle="Foo source">foo</quote>' );
} );
Expand Down Expand Up @@ -761,7 +756,6 @@ describe( 'advanced-converters', () => {
] );

const modelElement = viewDispatcher.convert( viewElement );
modelRoot.appendChildren( modelElement );

expect( modelToString( modelElement ) ).to.equal(
'<table cellpadding="5" cellspacing="5">' +
Expand Down
30 changes: 6 additions & 24 deletions tests/conversion/buildviewconverter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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.
Expand All @@ -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( '<paragraph>foo</paragraph>' );
} );
Expand All @@ -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( '<image src="foo.jpg"></image>' );
} );
Expand All @@ -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</$text></$root>' );
expect( modelToString( conversionResult ) ).to.equal( '<$text bold="true">foo</$text>' );
} );

it( 'should convert from view element to model attributes using creator function', () => {
Expand All @@ -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</$text></$root>' );
expect( modelToString( conversionResult ) ).to.equal( '<$text linkHref="foo.html">foo</$text>' );
} );

it( 'should convert from view attribute to model attribute', () => {
Expand All @@ -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( '<paragraph class="myClass">foo</paragraph>' );
} );
Expand Down Expand Up @@ -200,7 +191,6 @@ describe( 'View converter builder', () => {
] );

const conversionResult = dispatcher.convert( viewElement, objWithContext );
modelRoot.appendChildren( conversionResult );

expect( modelToString( conversionResult ) ).to.equal( '<paragraph><$text bold="true">aaabbbcccddd</$text></paragraph>' );
} );
Expand Down Expand Up @@ -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( '<span>foo</span>' );

// Almost a megatron. Missing a head.
Expand All @@ -346,7 +336,6 @@ describe( 'View converter builder', () => {
objWithContext
);

modelRoot.appendChildren( result );
expect( modelToString( result ) ).to.equal( '<span>foo</span>' );

// This would be a megatron but is a paragraph.
Expand All @@ -359,7 +348,6 @@ describe( 'View converter builder', () => {
objWithContext
);

modelRoot.appendChildren( result );
expect( modelToString( result ) ).to.equal( '<paragraph>foo</paragraph>' );

// At last we have a megatron!
Expand All @@ -372,7 +360,6 @@ describe( 'View converter builder', () => {
objWithContext
);

modelRoot.appendChildren( result );
expect( modelToString( result ) ).to.equal( '<MEGATRON>foo</MEGATRON>' );
} );

Expand All @@ -392,7 +379,6 @@ describe( 'View converter builder', () => {

const conversionResult = dispatcher.convert( viewElement, objWithContext );

modelRoot.appendChildren( conversionResult );
expect( modelToString( conversionResult ) ).to.equal( '<span transformer="megatron">foo</span>' );
} );

Expand All @@ -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( '<paragraph class="myClass">foo</paragraph>' );
Expand All @@ -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( '<paragraph class="myClass">foo</paragraph>' );

buildViewConverter().for( dispatcher )
Expand All @@ -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( '<customP>foo</customP>' );
} );

Expand All @@ -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
Expand All @@ -487,7 +470,6 @@ describe( 'View converter builder', () => {
] );

const conversionResult = dispatcher.convert( viewStructure, objWithContext );
modelRoot.appendChildren( conversionResult );

expect( modelToString( conversionResult ) ).to.equal( '<div class="myClass"><abcd>foo</abcd></div>' );
} );
Expand Down
2 changes: 1 addition & 1 deletion tests/model/node.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ describe( 'Node', () => {
one, two, three,
textBA, textR, img;

before( () => {
beforeEach( () => {
node = new Node();

one = new Element( 'one' );
Expand Down

0 comments on commit dec9c28

Please sign in to comment.