Merge pull request #1574 from ckeditor/t/1554

Feature: The `model.insertContent()` method will accept offset parameter. Made `Position.createAt()` require the `offset` when the first parameter is an model/view item. See breaking changes. Closes #1554. BREAKING CHANGE: The `offset` parameter of the following methods does not default to `0` and hence is no longer optional when `itemOrPosition` is a model/view item: * `model.Position.createAt()` * `model.Range.createCollapsedAt()` * `model.Selection#setFocus()` * `model.Writer#insert()` * `model.Writer#insertText()` * `model.Writer#insertElement()` * `model.Writer#move()` * `model.Writer#setSelectionFocus()` * `view.Writer#setSelectionFocus()` * `view.Position.createAt()` * `view.Range.createCollapsedAt()` * `view.Selection#setFocus()`
ckeditor · Oct 26, 2018 · 00dd70c · 00dd70c
2 parents 80392ad + 0dca79e
commit 00dd70c
Show file tree

Hide file tree

Showing 31 changed files with 181 additions and 115 deletions.
diff --git a/src/controller/datacontroller.js b/src/controller/datacontroller.js
@@ -201,7 +201,7 @@ export default class DataController {
 		const modelRoot = this.model.document.getRoot( rootName );
 
 		this.model.enqueueChange( 'transparent', writer => {
-			writer.insert( this.parse( data, modelRoot ), modelRoot );
+			writer.insert( this.parse( data, modelRoot ), modelRoot, 0 );
 		} );
 
 		return Promise.resolve();
@@ -228,7 +228,7 @@ export default class DataController {
 			writer.removeSelectionAttribute( this.model.document.selection.getAttributeKeys() );
 
 			writer.remove( ModelRange.createIn( modelRoot ) );
-			writer.insert( this.parse( data, modelRoot ), modelRoot );
+			writer.insert( this.parse( data, modelRoot ), modelRoot, 0 );
 		} );
 	}
 

diff --git a/src/conversion/upcast-converters.js b/src/conversion/upcast-converters.js
@@ -358,7 +358,7 @@ function _prepareToElementConverter( config ) {
 		conversionApi.writer.insert( modelElement, splitResult.position );
 
 		// Convert children and insert to element.
-		const childrenResult = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( modelElement ) );
+		const childrenResult = conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( modelElement, 0 ) );
 
 		// Consume appropriate value from consumable values list.
 		conversionApi.consumable.consume( data.viewItem, match.match );
@@ -380,7 +380,7 @@ function _prepareToElementConverter( config ) {
 		// before: <allowed><notAllowed>[]</notAllowed></allowed>
 		// after:  <allowed><notAllowed></notAllowed><converted></converted><notAllowed>[]</notAllowed></allowed>
 		if ( splitResult.cursorParent ) {
-			data.modelCursor = ModelPosition.createAt( splitResult.cursorParent );
+			data.modelCursor = ModelPosition.createAt( splitResult.cursorParent, 0 );
 
 			// Otherwise just continue after inserted element.
 		} else {

diff --git a/src/conversion/upcastdispatcher.js b/src/conversion/upcastdispatcher.js
@@ -71,7 +71,7 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';
  *  	 				conversionApi.writer.insert( paragraph, splitResult.position );
  *
  *  	 				// Convert children to paragraph.
- *  	 				const { modelRange } = conversionApi.convertChildren( data.viewItem, Position.createAt( paragraph ) );
+ *  	 				const { modelRange } = conversionApi.convertChildren( data.viewItem, Position.createAt( paragraph, 0 ) );
  *
  * 						// Set as conversion result, attribute converters may use this property.
  *  	 				data.modelRange = new Range( Position.createBefore( paragraph ), modelRange.end );
@@ -419,7 +419,7 @@ function createContextTree( contextDefinition, writer ) {
 			writer.append( current, position );
 		}
 
-		position = ModelPosition.createAt( current );
+		position = ModelPosition.createAt( current, 0 );
 	}
 
 	return position;

diff --git a/src/dev-utils/model.js b/src/dev-utils/model.js
@@ -391,7 +391,7 @@ function convertToModelElement() {
 
 		conversionApi.mapper.bindElements( element, data.viewItem );
 
-		conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( element ) );
+		conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( element, 0 ) );
 
 		data.modelRange = ModelRange.createOn( element );
 		data.modelCursor = data.modelRange.end;

diff --git a/src/model/model.js b/src/model/model.js
@@ -307,6 +307,9 @@ export default class Model {
 	 *
 	 *		// Insert text at given position - document selection will not be modified.
 	 *		editor.model.change( writer => {
+	 *			editor.model.insertContent( writer.createText( 'x' ), doc.getRoot(), 2 );
+	 *
+	 *			// Which is a shorthand for:
 	 *			editor.model.insertContent( writer.createText( 'x' ), Position.createAt( doc.getRoot(), 2 ) );
 	 *		} );
 	 *
@@ -327,12 +330,14 @@ export default class Model {
 	 * @fires insertContent
 	 * @param {module:engine/model/documentfragment~DocumentFragment|module:engine/model/item~Item} content The content to insert.
 	 * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
-	 * module:engine/model/position~Position|module:engine/model/element~Element|
+	 * module:engine/model/position~Position|module:engine/model/item~Item|
 	 * Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} [selectable=model.document.selection]
 	 * Selection into which the content should be inserted. If not provided the current model document selection will be used.
+	 * @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] To be used when a model item was passed as `selectable`.
+	 * This param defines a position in relation to that item.
 	 */
-	insertContent( content, selectable ) {
-		insertContent( this, content, selectable );
+	insertContent( content, selectable, placeOrOffset ) {
+		insertContent( this, content, selectable, placeOrOffset );
 	}
 
 	/**

diff --git a/src/model/position.js b/src/model/position.js
@@ -816,7 +816,7 @@ export default class Position {
 	 * * {@link module:engine/model/position~Position.createFromPosition}.
 	 *
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/model/item~Item model item}.
 	 */
 	static createAt( itemOrPosition, offset ) {
@@ -831,8 +831,16 @@ export default class Position {
 				return this.createBefore( node );
 			} else if ( offset == 'after' ) {
 				return this.createAfter( node );
-			} else if ( !offset ) {
-				offset = 0;
+			} else if ( offset !== 0 && !offset ) {
+				/**
+				 * {@link module:engine/model/position~Position.createAt `Position.createAt()`}
+				 * requires the offset to be specified when the first parameter is a model item.
+				 *
+				 * @error model-position-createAt-offset-required
+				 */
+				throw new CKEditorError(
+					'model-position-createAt-offset-required: ' +
+					'Position.createAt() requires the offset when the first parameter is a model item.' );
 			}
 
 			return this.createFromParentAndOffset( node, offset );

diff --git a/src/model/range.js b/src/model/range.js
@@ -835,7 +835,7 @@ export default class Range {
 	 * or on the given {@link module:engine/model/item~Item item}.
 	 *
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/model/item~Item model item}.
 	 */
 	static createCollapsedAt( itemOrPosition, offset ) {

diff --git a/src/model/schema.js b/src/model/schema.js
@@ -1162,7 +1162,7 @@ export class SchemaContext {
  *		schema.checkChild( contextDefinition, childToCheck );
  *
  *		// Also check in [ rootElement, blockQuoteElement, paragraphElement ].
- *		schema.checkChild( Position.createAt( paragraphElement ), 'foo' );
+ *		schema.checkChild( Position.createAt( paragraphElement, 0 ), 'foo' );
  *
  *		// Check in [ rootElement, paragraphElement ].
  *		schema.checkChild( [ rootElement, paragraphElement ], 'foo' );

diff --git a/src/model/selection.js b/src/model/selection.js
@@ -485,7 +485,7 @@ export default class Selection {
 	 *
 	 * @fires change:range
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/model/item~Item model item}.
 	 */
 	setFocus( itemOrPosition, offset ) {
@@ -665,7 +665,7 @@ export default class Selection {
 			const endBlock = getParentBlock( range.end, visited );
 
 			// #984. Don't return the end block if the range ends right at its beginning.
-			if ( endBlock && !range.end.isTouching( Position.createAt( endBlock ) ) ) {
+			if ( endBlock && !range.end.isTouching( Position.createAt( endBlock, 0 ) ) ) {
 				yield endBlock;
 			}
 		}
@@ -683,7 +683,7 @@ export default class Selection {
 	 * @returns {Boolean}
 	 */
 	containsEntireContent( element = this.anchor.root ) {
-		const limitStartPosition = Position.createAt( element );
+		const limitStartPosition = Position.createAt( element, 0 );
 		const limitEndPosition = Position.createAt( element, 'end' );
 
 		return limitStartPosition.isTouching( this.getFirstPosition() ) &&

diff --git a/src/model/utils/deletecontent.js b/src/model/utils/deletecontent.js
@@ -208,7 +208,7 @@ function replaceEntireContentWithParagraph( writer, selection ) {
 	const limitElement = writer.model.schema.getLimitElement( selection );
 
 	writer.remove( Range.createIn( limitElement ) );
-	insertParagraph( writer, Position.createAt( limitElement ), selection );
+	insertParagraph( writer, Position.createAt( limitElement, 0 ), selection );
 }
 
 // We want to replace the entire content with a paragraph when:

diff --git a/src/model/utils/getselectedcontent.js b/src/model/utils/getselectedcontent.js
@@ -99,7 +99,7 @@ export default function getSelectedContent( model, selection ) {
 			// Find the position of the original range in the cloned fragment.
 			const newRange = range._getTransformedByMove( flatSubtreeRange.start, Position.createAt( frag, 0 ), howMany )[ 0 ];
 
-			const leftExcessRange = new Range( Position.createAt( frag ), newRange.start );
+			const leftExcessRange = new Range( Position.createAt( frag, 0 ), newRange.start );
 			const rightExcessRange = new Range( newRange.end, Position.createAt( frag, 'end' ) );
 
 			removeRangeContent( rightExcessRange, writer );

diff --git a/src/model/utils/insertcontent.js b/src/model/utils/insertcontent.js
@@ -33,8 +33,9 @@ import Selection from '../selection';
  * module:engine/model/position~Position|module:engine/model/element~Element|
  * Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} [selectable=model.document.selection]
  * Selection into which the content should be inserted.
+ * @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] Sets place or offset of the selection.
  */
-export default function insertContent( model, content, selectable ) {
+export default function insertContent( model, content, selectable, placeOrOffset ) {
 	model.change( writer => {
 		let selection;
 
@@ -43,7 +44,7 @@ export default function insertContent( model, content, selectable ) {
 		} else if ( selectable instanceof Selection || selectable instanceof DocumentSelection ) {
 			selection = selectable;
 		} else {
-			selection = new Selection( selectable );
+			selection = new Selection( selectable, placeOrOffset );
 		}
 
 		if ( !selection.isCollapsed ) {

diff --git a/src/model/utils/selection-post-fixer.js b/src/model/utils/selection-post-fixer.js
@@ -199,8 +199,11 @@ function tryFixingNonCollapsedRage( range, schema ) {
 	if ( isStartInLimit || isEndInLimit ) {
 		// Although we've already found limit element on start/end positions we must find the outer-most limit element.
 		// as limit elements might be nested directly inside (ie table > tableRow > tableCell).
-		const fixedStart = isStartInLimit ? expandSelectionOnIsLimitNode( Position.createAt( startLimitElement ), schema, 'start' ) : start;
-		const fixedEnd = isEndInLimit ? expandSelectionOnIsLimitNode( Position.createAt( endLimitElement ), schema, 'end' ) : end;
+		const startPosition = Position.createAt( startLimitElement, 0 );
+		const endPosition = Position.createAt( endLimitElement, 0 );
+
+		const fixedStart = isStartInLimit ? expandSelectionOnIsLimitNode( startPosition, schema, 'start' ) : start;
+		const fixedEnd = isEndInLimit ? expandSelectionOnIsLimitNode( endPosition, schema, 'end' ) : end;
 
 		return new Range( fixedStart, fixedEnd );
 	}

diff --git a/src/model/writer.js b/src/model/writer.js
@@ -153,10 +153,10 @@ export default class Writer {
 	 * @param {module:engine/model/item~Item|module:engine/model/documentfragment~DocumentFragment} item Item or document
 	 * fragment to insert.
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * second parameter is a {@link module:engine/model/item~Item model item}.
 	 */
-	insert( item, itemOrPosition, offset ) {
+	insert( item, itemOrPosition, offset = 0 ) {
 		this._assertWriterUsedCorrectly();
 
 		const position = Position.createAt( itemOrPosition, offset );
@@ -198,7 +198,7 @@ export default class Writer {
 		if ( item instanceof DocumentFragment ) {
 			for ( const [ markerName, markerRange ] of item.markers ) {
 				// We need to migrate marker range from DocumentFragment to Document.
-				const rangeRootPosition = Position.createAt( markerRange.root );
+				const rangeRootPosition = Position.createAt( markerRange.root, 0 );
 				const range = new Range(
 					markerRange.start._getCombined( rangeRootPosition, position ),
 					markerRange.end._getCombined( rangeRootPosition, position )
@@ -230,7 +230,7 @@ export default class Writer {
 	 * @param {String} data Text data.
 	 * @param {Object} [attributes] Text attributes.
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * third parameter is a {@link module:engine/model/item~Item model item}.
 	 */
 	insertText( text, attributes, itemOrPosition, offset ) {
@@ -262,7 +262,7 @@ export default class Writer {
 	 * @param {String} name Name of the element.
 	 * @param {Object} [attributes] Elements attributes.
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * third parameter is a {@link module:engine/model/item~Item model item}.
 	 */
 	insertElement( name, attributes, itemOrPosition, offset ) {
@@ -440,7 +440,7 @@ export default class Writer {
 	 *
 	 * @param {module:engine/model/range~Range} range Source range.
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * second parameter is a {@link module:engine/model/item~Item model item}.
 	 */
 	move( range, itemOrPosition, offset ) {
@@ -668,7 +668,7 @@ export default class Writer {
 
 		return {
 			position,
-			range: new Range( Position.createAt( firstSplitElement, 'end' ), Position.createAt( firstCopyElement ) )
+			range: new Range( Position.createAt( firstSplitElement, 'end' ), Position.createAt( firstCopyElement, 0 ) )
 		};
 	}
 

diff --git a/src/view/documentselection.js b/src/view/documentselection.js
@@ -357,7 +357,7 @@ export default class DocumentSelection {
 	 * @protected
 	 * @fires change
 	 * @param {module:engine/view/item~Item|module:engine/view/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/view/item~Item view item}.
 	 */
 	_setFocus( itemOrPosition, offset ) {

diff --git a/src/view/downcastwriter.js b/src/view/downcastwriter.js
@@ -123,7 +123,7 @@ export default class DowncastWriter {
 	 * The location can be specified in the same form as {@link module:engine/view/position~Position.createAt} parameters.
 	 *
 	 * @param {module:engine/view/item~Item|module:engine/view/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/view/item~Item view item}.
 	 */
 	setSelectionFocus( itemOrPosition, offset ) {
@@ -915,7 +915,7 @@ export default class DowncastWriter {
 		const newElement = new ContainerElement( newName, viewElement.getAttributes() );
 
 		this.insert( Position.createAfter( viewElement ), newElement );
-		this.move( Range.createIn( viewElement ), Position.createAt( newElement ) );
+		this.move( Range.createIn( viewElement ), Position.createAt( newElement, 0 ) );
 		this.remove( Range.createOn( viewElement ) );
 
 		return newElement;

diff --git a/src/view/position.js b/src/view/position.js
@@ -288,8 +288,8 @@ export default class Position {
 	 * * {@link module:engine/view/position~Position.createAfter},
 	 * * {@link module:engine/view/position~Position.createFromPosition}.
 	 *
-	 * @param {module:engine/view/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {module:engine/view/item~Item|module:engine/view/position~Position} itemOrPosition
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/view/item~Item view item}.
 	 */
 	static createAt( itemOrPosition, offset ) {
@@ -304,8 +304,16 @@ export default class Position {
 				return this.createBefore( node );
 			} else if ( offset == 'after' ) {
 				return this.createAfter( node );
-			} else if ( !offset ) {
-				offset = 0;
+			} else if ( offset !== 0 && !offset ) {
+				/**
+				 * {@link module:engine/view/position~Position.createAt `Position.createAt()`}
+				 * requires the offset to be specified when the first parameter is a view item.
+				 *
+				 * @error view-position-createAt-offset-required
+				 */
+				throw new CKEditorError(
+					'view-position-createAt-offset-required: ' +
+					'Position.createAt() requires the offset when the first parameter is a view item.' );
 			}
 
 			return new Position( node, offset );

diff --git a/src/view/range.js b/src/view/range.js
@@ -454,7 +454,7 @@ export default class Range {
 	 * or on the given {@link module:engine/view/item~Item item}.
 	 *
 	 * @param {module:engine/view/item~Item|module:engine/view/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/view/item~Item view item}.
 	 */
 	static createCollapsedAt( itemOrPosition, offset ) {

diff --git a/src/view/selection.js b/src/view/selection.js
@@ -548,7 +548,7 @@ export default class Selection {
 	 *
 	 * @fires change
 	 * @param {module:engine/view/item~Item|module:engine/view/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/view/item~Item view item}.
 	 */
 	setFocus( itemOrPosition, offset ) {

diff --git a/tests/conversion/upcast-converters.js b/tests/conversion/upcast-converters.js
@@ -841,7 +841,7 @@ describe( 'upcast-converters', () => {
 					const paragraph = conversionApi.writer.createElement( 'paragraph' );
 
 					conversionApi.writer.insert( paragraph, data.modelCursor );
-					conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( paragraph ) );
+					conversionApi.convertChildren( data.viewItem, ModelPosition.createAt( paragraph, 0 ) );
 
 					data.modelRange = ModelRange.createOn( paragraph );
 					data.modelCursor = data.modelRange.end;
@@ -863,7 +863,7 @@ describe( 'upcast-converters', () => {
 				new ViewContainerElement( 'div', null, [ new ViewText( 'abc' ), new ViewContainerElement( 'foo' ) ] ),
 				new ViewContainerElement( 'bar' )
 			] );
-			const position = ModelPosition.createAt( new ModelElement( 'element' ) );
+			const position = ModelPosition.createAt( new ModelElement( 'element' ), 0 );
 
 			dispatcher.on( 'documentFragment', convertToModelFragment() );
 			dispatcher.on( 'element', convertToModelFragment(), { priority: 'lowest' } );