Merge pull request #885 from ckeditor/t/884

Other: Default conversion.Mapper position mapping algorithms are now added as callbacks with low priority and are fired only if earlier callbacks did not provide a result. Closes #884.

BREAKING CHANGES: Since default position mapping algorithms are attached with low priority, custom position mapping callbacks added with higher priority won't receive position calculated by default algorithms in data. To execute default position mapping algorithms and use their value, hook custom callback with lower priority.

Author: Piotr Jasiun

src/conversion/mapper.js

@@ -61,6 +61,36 @@ export default class Mapper {
 		 * @member {Map}
 		 */
 		this._viewToModelLengthCallbacks = new Map();
+
+		// Default mapper algorithm for mapping model position to view position.
+		this.on( 'modelToViewPosition', ( evt, data ) => {
+			if ( data.viewPosition ) {
+				return;
+			}
+
+			let viewContainer = this._modelToViewMapping.get( data.modelPosition.parent );
+
+			data.viewPosition = this._findPositionIn( viewContainer, data.modelPosition.offset );
+		}, { priority: 'low' } );
+
+		// Default mapper algorithm for mapping view position to model position.
+		this.on( 'viewToModelPosition', ( evt, data ) => {
+			if ( data.modelPosition ) {
+				return;
+			}
+
+			let viewBlock = data.viewPosition.parent;
+			let modelParent = this._viewToModelMapping.get( viewBlock );
+
+			while ( !modelParent ) {
+				viewBlock = viewBlock.parent;
+				modelParent = this._viewToModelMapping.get( viewBlock );
+			}
+
+			let modelOffset = this._toModelOffset( data.viewPosition.parent, data.viewPosition.offset, viewBlock );
+
+			data.modelPosition = ModelPosition.createFromParentAndOffset( modelParent, modelOffset );
+		}, { priority: 'low' } );
 	}
 
 	/**
@@ -159,7 +189,6 @@ export default class Mapper {
 	toModelPosition( viewPosition ) {
 		const data = {
 			viewPosition: viewPosition,
-			modelPosition: this._defaultToModelPosition( viewPosition ),
 			mapper: this
 		};
 
@@ -168,19 +197,6 @@ export default class Mapper {
 		return data.modelPosition;
 	}
 
-	/**
-	 * Maps model position to view position using default mapper algorithm.
-	 *
-	 * @private
-	 * @param {module:engine/model/position~Position} modelPosition
-	 * @returns {module:engine/view/position~Position} View position mapped from model position.
-	 */
-	_defaultToViewPosition( modelPosition ) {
-		let viewContainer = this._modelToViewMapping.get( modelPosition.parent );
-
-		return this._findPositionIn( viewContainer, modelPosition.offset );
-	}
-
 	/**
 	 * Gets the corresponding view position.
 	 *
@@ -190,7 +206,6 @@ export default class Mapper {
 	 */
 	toViewPosition( modelPosition ) {
 		const data = {
-			viewPosition: this._defaultToViewPosition( modelPosition ),
 			modelPosition: modelPosition,
 			mapper: this
 		};
@@ -200,27 +215,6 @@ export default class Mapper {
 		return data.viewPosition;
 	}
 
-	/**
-	 * Maps view position to model position using default mapper algorithm.
-	 *
-	 * @private
-	 * @param {module:engine/view/position~Position} viewPosition
-	 * @returns {module:engine/model/position~Position} Model position mapped from view position.
-	 */
-	_defaultToModelPosition( viewPosition ) {
-		let viewBlock = viewPosition.parent;
-		let modelParent = this._viewToModelMapping.get( viewBlock );
-
-		while ( !modelParent ) {
-			viewBlock = viewBlock.parent;
-			modelParent = this._viewToModelMapping.get( viewBlock );
-		}
-
-		let modelOffset = this._toModelOffset( viewPosition.parent, viewPosition.offset, viewBlock );
-
-		return ModelPosition.createFromParentAndOffset( modelParent, modelOffset );
-	}
-
 	/**
 	 * Registers a callback that evaluates the length in the model of a view element with given name.
 	 *
@@ -442,65 +436,89 @@ export default class Mapper {
 		// Otherwise, just return the given position.
 		return viewPosition;
 	}
-}
 
-mix( Mapper, EmitterMixin );
+	/**
+	 * Fired for each model-to-view position mapping request. The purpose of this event is to enable custom model-to-view position
+	 * mapping. Callbacks added to this event take {@link module:engine/model/position~Position model position} and are expected to calculate
+	 * {@link module:engine/view/position~Position view position}. Calculated view position should be added as `viewPosition` value in
+	 * `data` object that is passed as one of parameters to the event callback.
+	 *
+	 * 		// Assume that "captionedImage" model element is converted to <img> and following <span> elements in view,
+	 * 		// and the model element is bound to <img> element. Force mapping model positions inside "captionedImage" to that <span> element.
+	 *		mapper.on( 'modelToViewPosition', ( evt, data ) => {
+	 *			const positionParent = modelPosition.parent;
+	 *
+	 *			if ( positionParent.name == 'captionedImage' ) {
+	 *				const viewImg = data.mapper.toViewElement( positionParent );
+	 *				const viewCaption = viewImg.nextSibling; // The <span> element.
+	 *
+	 *				data.viewPosition = new ViewPosition( viewCaption, modelPosition.offset );
+	 *
+	 *				// Stop the event if other callbacks should not modify calculated value.
+	 *				evt.stop();
+	 *			}
+	 *		} );
+	 *
+	 * **Note:** keep in mind that custom callback provided for this event should use provided `data.modelPosition` only to check
+	 * what is before the position (or position's parent). This is important when model-to-view position mapping is used in
+	 * remove change conversion. Model after the removed position (that is being mapped) does not correspond to view, so it cannot be used:
+	 *
+	 *		// Incorrect:
+	 *		const modelElement = data.modelPosition.nodeAfter;
+	 *		const viewElement = data.mapper.toViewElement( modelElement );
+	 *		// ... Do something with `viewElement` and set `data.viewPosition`.
+	 *
+	 *		// Correct:
+	 *		const prevModelElement = data.modelPosition.nodeBefore;
+	 *		const prevViewElement = data.mapper.toViewElement( prevModelElement );
+	 *		// ... Use `prevViewElement` to find correct `data.viewPosition`.
+	 *
+	 * **Note:** default mapping callback is provided with `low` priority setting and does not cancel the event, so it is possible to attach
+	 * a custom callback after default callback and also use `data.viewPosition` calculated by default callback (for example to fix it).
+	 *
+	 * **Note:** default mapping callback will not fire if `data.viewPosition` is already set.
+	 *
+	 * **Note:** these callbacks are called **very often**. For efficiency reasons, it is advised to use them only when position
+	 * mapping between given model and view elements is unsolvable using just elements mapping and default algorithm. Also,
+	 * the condition that checks if special case scenario happened should be as simple as possible.
+	 *
+	 * @event modelToViewPosition
+	 * @param {Object} data Data pipeline object that can store and pass data between callbacks. The callback should add
+	 * `viewPosition` value to that object with calculated {@link module:engine/view/position~Position view position}.
+	 * @param {module:engine/conversion/mapper~Mapper} data.mapper Mapper instance that fired the event.
+	 */
 
-/**
- * Fired for each model-to-view position mapping request. The purpose of this event is to enable custom model-to-view position
- * mapping. Callbacks added to this event take {@link module:engine/model/position~Position model position} and are expected to calculate
- * {@link module:engine/view/position~Position view position}. Calculated view position should be added as `viewPosition` value in
- * `data` object that is passed as one of parameters to the event callback.
- *
- * 		// Assume that "captionedImage" model element is converted to <img> and following <span> elements in view,
- * 		// and the model element is bound to <img> element. Force mapping model positions inside "captionedImage" to that <span> element.
- *		mapper.on( 'modelToViewPosition', ( evt, data ) => {
- *			const positionParent = modelPosition.parent;
- *
- *			if ( positionParent.name == 'captionedImage' ) {
- *				const viewImg = mapper.toViewElement( positionParent );
- *				const viewCaption = viewImg.nextSibling; // The <span> element.
- *
- *				data.viewPosition = new ViewPosition( viewCaption, modelPosition.offset );
- *				evt.stop();
- *			}
- *		} );
- *
- * **Note:** these callbacks are called **very often**. For efficiency reasons, it is advised to use them only when position
- * mapping between given model and view elements is unsolvable using just elements mapping and default algorithm. Also,
- * the condition that checks if special case scenario happened should be as simple as possible.
- *
- * @event modelToViewPosition
- * @param {Object} data Data pipeline object that can store and pass data between callbacks. The callback should add
- * `viewPosition` value to that object with calculated {@link module:engine/view/position~Position view position}.
- * @param {module:engine/model/position~Position} data.modelPosition Model position to be mapped.
- * @param {module:engine/view/position~Position} data.viewPosition View position that is a result of mapping
- * `modelPosition` using `Mapper` default algorithm.
- * @param {module:engine/conversion/mapper~Mapper} data.mapper Mapper instance that fired the event.
- */
+	/**
+	 * Fired for each view-to-model position mapping request. See {@link module:engine/conversion/mapper~Mapper#event:modelToViewPosition}.
+	 *
+	 * 		// See example in `modelToViewPosition` event description.
+	 * 		// This custom mapping will map positions from <span> element next to <img> to the "captionedImage" element.
+	 *		mapper.on( 'viewToModelPosition', ( evt, data ) => {
+	 *			const positionParent = viewPosition.parent;
+	 *
+	 *			if ( positionParent.hasClass( 'image-caption' ) ) {
+	 *				const viewImg = positionParent.previousSibling;
+	 *				const modelImg = data.mapper.toModelElement( viewImg );
+	 *
+	 *				data.modelPosition = new ModelPosition( modelImg, viewPosition.offset );
+	 *				evt.stop();
+	 *			}
+	 *		} );
+	 *
+	 * **Note:** default mapping callback is provided with `low` priority setting and does not cancel the event, so it is possible to attach
+	 * a custom callback after default callback and also use `data.modelPosition` calculated by default callback (for example to fix it).
+	 *
+	 * **Note:** default mapping callback will not fire if `data.modelPosition` is already set.
+	 *
+	 * **Note:** these callbacks are called **very often**. For efficiency reasons, it is advised to use them only when position
+	 * mapping between given model and view elements is unsolvable using just elements mapping and default algorithm. Also,
+	 * the condition that checks if special case scenario happened should be as simple as possible.
+	 *
+	 * @event viewToModelPosition
+	 * @param {Object} data Data pipeline object that can store and pass data between callbacks. The callback should add
+	 * `modelPosition` value to that object with calculated {@link module:engine/model/position~Position model position}.
+	 * @param {module:engine/conversion/mapper~Mapper} data.mapper Mapper instance that fired the event.
+	 */
+}
 
-/**
- * Fired for each view-to-model position mapping request. See {@link module:engine/conversion/mapper~Mapper#event:modelToViewPosition}.
- *
- * 		// See example in `modelToViewPosition` event description.
- * 		// This custom mapping will map positions from <span> element next to <img> to the "captionedImage" element.
- *		mapper.on( 'viewToModelPosition', ( evt, data ) => {
- *			const positionParent = viewPosition.parent;
- *
- *			if ( positionParent.hasClass( 'image-caption' ) ) {
- *				const viewImg = positionParent.previousSibling;
- *				const modelImg = mapper.toModelElement( viewImg );
- *
- *				data.modelPosition = new ModelPosition( modelImg, viewPosition.offset );
- *				evt.stop();
- *			}
- *		} );
- *
- * @event viewToModelPosition
- * @param {Object} data Data pipeline object that can store and pass data between callbacks. The callback should add
- * `modelPosition` value to that object with calculated {@link module:engine/model/position~Position model position}.
- * @param {module:engine/view/position~Position} data.viewPosition View position to be mapped.
- * @param {module:engine/model/position~Position} data.modelPosition Model position that is a result of mapping
- * `viewPosition` using `Mapper` default algorithm.
- * @param {module:engine/conversion/mapper~Mapper} data.mapper Mapper instance that fired the event.
- */
+mix( Mapper, EmitterMixin );

src/conversion/model-to-view-converters.js

@@ -3,10 +3,10 @@
  * For licensing, see LICENSE.md.
  */
 
-import ModelRange from '../model/range';
-
 import ViewElement from '../view/element';
 import ViewText from '../view/text';
+import ViewRange from '../view/range';
+import ViewTreeWalker from '../view/treewalker';
 import viewWriter from '../view/writer';
 
 /**
@@ -436,9 +436,27 @@ export function remove() {
 			return;
 		}
 
-		const modelRange = ModelRange.createFromPositionAndShift( data.sourcePosition, data.item.offsetSize );
-		const viewRange = conversionApi.mapper.toViewRange( modelRange );
+		// We cannot map non-existing positions from model to view. Since a range was removed
+		// from the model, we cannot recreate that range and map it to view, because
+		// end of that range is incorrect.
+		// Instead we will use `data.sourcePosition` as this is the last correct model position and
+		// it is a position before the removed item. Then, we will calculate view range to remove "manually".
+		const viewPosition = conversionApi.mapper.toViewPosition( data.sourcePosition );
+		let viewRange;
+
+		if ( data.item.is( 'element' ) ) {
+			// Note: in remove conversion we cannot use model-to-view element mapping because `data.item` may be
+			// already mapped to another element (this happens when move change is converted).
+			// In this case however, `viewPosition` is the position before view element that corresponds to removed model element.
+			viewRange = ViewRange.createOn( viewPosition.nodeAfter );
+		} else {
+			// If removed item is a text node, we need to traverse view tree to find the view range to remove.
+			// Range to remove will start `viewPosition` and should contain amount of characters equal to the amount of removed characters.
+			const viewRangeEnd = _shiftViewPositionByCharacters( viewPosition, data.item.offsetSize );
+			viewRange = new ViewRange( viewPosition, viewRangeEnd );
+		}
 
+		// Trim the range to remove in case some UI elements are on the view range boundaries.
 		viewWriter.remove( viewRange.getTrimmed() );
 
 		// Unbind this element only if it was moved to graveyard.
@@ -460,6 +478,26 @@ export function remove() {
 	};
 }
 
+// Helper function that shifts given view `position` in a way that returned position is after `howMany` characters compared
+// to the original `position`.
+// Because in view there might be view ui elements splitting text nodes, we cannot simply use `ViewPosition#getShiftedBy()`.
+function _shiftViewPositionByCharacters( position, howMany ) {
+	// Create a walker that will walk the view tree starting from given position and walking characters one-by-one.
+	const walker = new ViewTreeWalker( { startPosition: position, singleCharacters: true } );
+	// We will count visited characters and return the position after `howMany` characters.
+	let charactersFound = 0;
+
+	for ( let value of walker ) {
+		if ( value.type == 'text' ) {
+			charactersFound++;
+
+			if ( charactersFound == howMany ) {
+				return walker.position;
+			}
+		}
+	}
+}
+
 /**
  * Function factory, creates a default model-to-view converter for removing {@link module:engine/view/uielement~UIElement ui element}
  * basing on marker remove change.

src/model/delta/insertdelta.js

@@ -8,10 +8,11 @@
  */
 
 import Delta from './delta';
-import DeltaFactory from './deltafactory';
 import RemoveDelta from './removedelta';
-import { register } from '../batch';
+import DeltaFactory from './deltafactory';
 import InsertOperation from '../operation/insertoperation';
+import { register } from '../batch';
+import { normalizeNodes } from './../writer';
 
 import DocumentFragment from '../documentfragment';
 import Range from '../../model/range.js';
@@ -95,8 +96,15 @@ export default class InsertDelta extends Delta {
  * @param {module:engine/model/node~NodeSet} nodes The list of nodes to be inserted.
  */
 register( 'insert', function( position, nodes ) {
+	const normalizedNodes = normalizeNodes( nodes );
+
+	// If nothing is inserted do not create delta and operation.
+	if ( normalizedNodes.length === 0 ) {
+		return this;
+	}
+
 	const delta = new InsertDelta();
-	const insert = new InsertOperation( position, nodes, this.document.version );
+	const insert = new InsertOperation( position, normalizedNodes, this.document.version );
 
 	this.addDelta( delta );
 	delta.addOperation( insert );