diff --git a/src/model/operation/mergeoperation.js b/src/model/operation/mergeoperation.js index d6a101861..c20eafd26 100644 --- a/src/model/operation/mergeoperation.js +++ b/src/model/operation/mergeoperation.js @@ -141,16 +141,16 @@ export default class MergeOperation extends Operation { const targetElement = this.targetPosition.parent; // Validate whether merge operation has correct parameters. - if ( !sourceElement || !sourceElement.is( 'element' ) || !sourceElement.parent ) { + if ( !sourceElement.parent ) { /** - * Merge source position is invalid. + * Merge source position is invalid. The element to be merged must have a parent node. * * @error merge-operation-source-position-invalid */ throw new CKEditorError( 'merge-operation-source-position-invalid: Merge source position is invalid.', this ); - } else if ( !targetElement || !targetElement.is( 'element' ) || !targetElement.parent ) { + } else if ( !targetElement.parent ) { /** - * Merge target position is invalid. + * Merge target position is invalid. The element to be merged must have a parent node. * * @error merge-operation-target-position-invalid */ diff --git a/src/model/operation/moveoperation.js b/src/model/operation/moveoperation.js index 799a3d1da..98114db2e 100644 --- a/src/model/operation/moveoperation.js +++ b/src/model/operation/moveoperation.js @@ -123,16 +123,7 @@ export default class MoveOperation extends Operation { // Validate whether move operation has correct parameters. // Validation is pretty complex but move operation is one of the core ways to manipulate the document state. // We expect that many errors might be connected with one of scenarios described below. - if ( !sourceElement || !targetElement ) { - /** - * Source position or target position is invalid. - * - * @error move-operation-position-invalid - */ - throw new CKEditorError( - 'move-operation-position-invalid: Source position or target position is invalid.', this - ); - } else if ( sourceOffset + this.howMany > sourceElement.maxOffset ) { + if ( sourceOffset + this.howMany > sourceElement.maxOffset ) { /** * The nodes which should be moved do not exist. * diff --git a/src/model/position.js b/src/model/position.js index 861ad9ea5..c81fbf406 100644 --- a/src/model/position.js +++ b/src/model/position.js @@ -71,11 +71,11 @@ export default class Position { /** * Position path must be an array with at least one item. * - * @error model-position-path-incorrect + * @error model-position-path-incorrect-format * @param path */ throw new CKEditorError( - 'model-position-path-incorrect: Position path must be an array with at least one item.', + 'model-position-path-incorrect-format: Position path must be an array with at least one item.', root, { path } ); @@ -161,13 +161,36 @@ export default class Position { * Also it is a good idea to cache `parent` property if it is used frequently in an algorithm (i.e. in a long loop). * * @readonly - * @type {module:engine/model/element~Element} + * @type {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} */ get parent() { let parent = this.root; for ( let i = 0; i < this.path.length - 1; i++ ) { parent = parent.getChild( parent.offsetToIndex( this.path[ i ] ) ); + + if ( !parent ) { + throw new CKEditorError( 'model-position-path-incorrect: The position\'s path is incorrect.', this, { position: this } ); + } + } + + if ( parent.is( 'text' ) ) { + /** + * The position's path is incorrect. This means that a position does not point to + * a correct place in the tree and hence, some of its methods and getters cannot work correctly. + * + * **Note**: Unlike DOM and view positions, in the model, the + * {@link module:engine/model/position~Position#parent position's parent} is always an element or a document fragment. + * The last offset in the {@link module:engine/model/position~Position#path position's path} is the point in this element where + * this position points. + * + * Read more about model positions and offsets in + * the {@glink framework/guides/architecture/editing-engine#indexes-and-offsets Editing engine architecture guide}. + * + * @error position-incorrect-path + * @param {module:engine/model/position~Position} position The incorrect position. + */ + throw new CKEditorError( 'model-position-path-incorrect: The position\'s path is incorrect.', this, { position: this } ); } return parent; diff --git a/tests/model/operation/mergeoperation.js b/tests/model/operation/mergeoperation.js index d2ed0f202..6367b6b56 100644 --- a/tests/model/operation/mergeoperation.js +++ b/tests/model/operation/mergeoperation.js @@ -119,7 +119,7 @@ describe( 'MergeOperation', () => { doc.version ); - expectToThrowCKEditorError( () => operation._validate(), /merge-operation-target-position-invalid/, model ); + expectToThrowCKEditorError( () => operation._validate(), /model-position-path-incorrect/, model ); } ); it( 'should throw an error if source position is in root', () => { @@ -139,6 +139,23 @@ describe( 'MergeOperation', () => { expectToThrowCKEditorError( () => operation._validate(), /merge-operation-source-position-invalid/, model ); } ); + it( 'should throw an error if target position is in root', () => { + const p1 = new Element( 'p1', null, new Text( 'Foo' ) ); + const p2 = new Element( 'p2', null, new Text( 'bar' ) ); + + root._insertChild( 0, [ p1, p2 ] ); + + const operation = new MergeOperation( + new Position( root, [ 0, 3 ] ), + 3, + new Position( root, [ 0 ] ), + gyPos, + doc.version + ); + + expectToThrowCKEditorError( () => operation._validate(), /merge-operation-target-position-invalid/, model ); + } ); + it( 'should throw an error if target position is invalid', () => { const p1 = new Element( 'p1', null, new Text( 'Foo' ) ); const p2 = new Element( 'p2', null, new Text( 'bar' ) ); @@ -153,7 +170,7 @@ describe( 'MergeOperation', () => { doc.version ); - expectToThrowCKEditorError( () => operation._validate(), /merge-operation-source-position-invalid/, model ); + expectToThrowCKEditorError( () => operation._validate(), /model-position-path-incorrect/, model ); } ); it( 'should throw an error if number of nodes to move is invalid', () => { diff --git a/tests/model/operation/moveoperation.js b/tests/model/operation/moveoperation.js index 987b3cb35..0651cac3a 100644 --- a/tests/model/operation/moveoperation.js +++ b/tests/model/operation/moveoperation.js @@ -171,7 +171,7 @@ describe( 'MoveOperation', () => { root._removeChildren( 1 ); - expectToThrowCKEditorError( () => operation._validate(), /move-operation-position-invalid/, model ); + expectToThrowCKEditorError( () => operation._validate(), /model-position-path-incorrect/, model ); } ); it( 'should throw an error if operation tries to move a range between the beginning and the end of that range', () => { diff --git a/tests/model/position.js b/tests/model/position.js index 3f7bb9c5b..415f23818 100644 --- a/tests/model/position.js +++ b/tests/model/position.js @@ -38,7 +38,7 @@ describe( 'Position', () => { // |- b Before: [ 1, 1, 0 ] After: [ 1, 1, 1 ] // |- a Before: [ 1, 1, 1 ] After: [ 1, 1, 2 ] // |- r Before: [ 1, 1, 2 ] After: [ 1, 1, 3 ] - before( () => { + beforeEach( () => { model = new Model(); doc = model.document; @@ -110,11 +110,11 @@ describe( 'Position', () => { it( 'should throw error if given path is incorrect', () => { expectToThrowCKEditorError( () => { new Position( root, {} ); // eslint-disable-line no-new - }, /model-position-path-incorrect/, model ); + }, /model-position-path-incorrect-format/, model ); expectToThrowCKEditorError( () => { new Position( root, [] ); // eslint-disable-line no-new - }, /model-position-path-incorrect/, model ); + }, /model-position-path-incorrect-format/, model ); } ); it( 'should throw error if given root is invalid', () => { @@ -263,119 +263,206 @@ describe( 'Position', () => { } ); } ); - it( 'should have parent', () => { - expect( new Position( root, [ 0 ] ) ).to.have.property( 'parent' ).that.equals( root ); - expect( new Position( root, [ 1 ] ) ).to.have.property( 'parent' ).that.equals( root ); - expect( new Position( root, [ 2 ] ) ).to.have.property( 'parent' ).that.equals( root ); + describe( '#parent', () => { + it( 'should have parent', () => { + expect( new Position( root, [ 0 ] ) ).to.have.property( 'parent' ).that.equals( root ); + expect( new Position( root, [ 1 ] ) ).to.have.property( 'parent' ).that.equals( root ); + expect( new Position( root, [ 2 ] ) ).to.have.property( 'parent' ).that.equals( root ); - expect( new Position( root, [ 0, 0 ] ) ).to.have.property( 'parent' ).that.equals( p ); + expect( new Position( root, [ 0, 0 ] ) ).to.have.property( 'parent' ).that.equals( p ); - expect( new Position( root, [ 1, 0 ] ) ).to.have.property( 'parent' ).that.equals( ul ); - expect( new Position( root, [ 1, 1 ] ) ).to.have.property( 'parent' ).that.equals( ul ); - expect( new Position( root, [ 1, 2 ] ) ).to.have.property( 'parent' ).that.equals( ul ); + expect( new Position( root, [ 1, 0 ] ) ).to.have.property( 'parent' ).that.equals( ul ); + expect( new Position( root, [ 1, 1 ] ) ).to.have.property( 'parent' ).that.equals( ul ); + expect( new Position( root, [ 1, 2 ] ) ).to.have.property( 'parent' ).that.equals( ul ); - expect( new Position( root, [ 1, 0, 0 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); - expect( new Position( root, [ 1, 0, 1 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); - expect( new Position( root, [ 1, 0, 2 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); - expect( new Position( root, [ 1, 0, 3 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); - } ); + expect( new Position( root, [ 1, 0, 0 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); + expect( new Position( root, [ 1, 0, 1 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); + expect( new Position( root, [ 1, 0, 2 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); + expect( new Position( root, [ 1, 0, 3 ] ) ).to.have.property( 'parent' ).that.equals( li1 ); + } ); + + it( 'should work with positions rooted in document fragment', () => { + const docFrag = new DocumentFragment(); + + expect( new Position( docFrag, [ 0 ] ) ).to.have.property( 'parent' ).that.equals( docFrag ); + } ); - it( 'should have offset', () => { - expect( new Position( root, [ 0 ] ) ).to.have.property( 'offset' ).that.equals( 0 ); - expect( new Position( root, [ 1 ] ) ).to.have.property( 'offset' ).that.equals( 1 ); - expect( new Position( root, [ 2 ] ) ).to.have.property( 'offset' ).that.equals( 2 ); + it( 'should throw when path out of bounds', () => { + const position = new Position( root, [ 0, 0 ] ); - expect( new Position( root, [ 0, 0 ] ) ).to.have.property( 'offset' ).that.equals( 0 ); + expect( position ).to.have.property( 'parent' ).that.equals( p ); - expect( new Position( root, [ 1, 0 ] ) ).to.have.property( 'offset' ).that.equals( 0 ); - expect( new Position( root, [ 1, 1 ] ) ).to.have.property( 'offset' ).that.equals( 1 ); - expect( new Position( root, [ 1, 2 ] ) ).to.have.property( 'offset' ).that.equals( 2 ); + root._removeChildren( 0, 2 ); - expect( new Position( root, [ 1, 0, 0 ] ) ).to.have.property( 'offset' ).that.equals( 0 ); - expect( new Position( root, [ 1, 0, 1 ] ) ).to.have.property( 'offset' ).that.equals( 1 ); - expect( new Position( root, [ 1, 0, 2 ] ) ).to.have.property( 'offset' ).that.equals( 2 ); - expect( new Position( root, [ 1, 0, 3 ] ) ).to.have.property( 'offset' ).that.equals( 3 ); + expectToThrowCKEditorError( () => { + position.parent; + }, /^model-position-path-incorrect:/, position, { position } ); + } ); + + it( 'should throw when based on a path, the parent would be a text node', () => { + // 1,0,0 points at: