Merge pull request #1777 from ckeditor/t/1776

Other: Position getters (such as `#parent` or `#index`) will throw when position points at an incorrect place in its root. Closes #1776.

Author: jodator

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
 			 */

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.
 			 *

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;

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', () => {

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', () => {