From 553c18f044ee4a92f7d5e78783739e7857574fe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 7 Aug 2019 16:14:31 +0200 Subject: [PATCH 1/3] Added validation for position's getters in order to achieve better errors.. --- src/model/position.js | 29 ++++- tests/model/position.js | 257 +++++++++++++++++++++++++++------------- 2 files changed, 198 insertions(+), 88 deletions(-) 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/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: