瀏覽代碼

Added: Added `isPhantom` parameter to `conversion.Mapper#toViewPosition()`.
Changed: `conversion.Mapper` will not unbind elements that got rebound to other elements.

Szymon Cofalik 8 年之前
父節點
當前提交
a6ebe6a855
共有 2 個文件被更改,包括 100 次插入28 次删除
  1. 52 28
      packages/ckeditor5-engine/src/conversion/mapper.js
  2. 48 0
      packages/ckeditor5-engine/tests/conversion/mapper.js

+ 52 - 28
packages/ckeditor5-engine/src/conversion/mapper.js

@@ -110,23 +110,43 @@ export default class Mapper {
 	/**
 	 * Unbinds given {@link module:engine/view/element~Element view element} from the map.
 	 *
+	 * **Note:** view-to-model binding will be removed, if it existed. However, corresponding model-to-view binding
+	 * will be removed only if model element is still bound to passed `viewElement`.
+	 *
+	 * This behavior lets for re-binding model element to another view element without fear of losing the new binding
+	 * when the previously bound view element is unbound.
+	 *
 	 * @param {module:engine/view/element~Element} viewElement View element to unbind.
 	 */
 	unbindViewElement( viewElement ) {
 		const modelElement = this.toModelElement( viewElement );
 
-		this._unbindElements( modelElement, viewElement );
+		this._viewToModelMapping.delete( viewElement );
+
+		if ( this._modelToViewMapping.get( modelElement ) == viewElement ) {
+			this._modelToViewMapping.delete( modelElement );
+		}
 	}
 
 	/**
 	 * Unbinds given {@link module:engine/model/element~Element model element} from the map.
 	 *
+	 * **Note:** model-to-view binding will be removed, if it existed. However, corresponding view-to-model binding
+	 * will be removed only if view element is still bound to passed `modelElement`.
+	 *
+	 * This behavior lets for re-binding view element to another model element without fear of losing the new binding
+	 * when the previously bound model element is unbound.
+	 *
 	 * @param {module:engine/model/element~Element} modelElement Model element to unbind.
 	 */
 	unbindModelElement( modelElement ) {
 		const viewElement = this.toViewElement( modelElement );
 
-		this._unbindElements( modelElement, viewElement );
+		this._modelToViewMapping.delete( modelElement );
+
+		if ( this._viewToModelMapping.get( viewElement ) == modelElement ) {
+			this._viewToModelMapping.delete( viewElement );
+		}
 	}
 
 	/**
@@ -202,12 +222,15 @@ export default class Mapper {
 	 *
 	 * @fires modelToViewPosition
 	 * @param {module:engine/model/position~Position} modelPosition Model position.
+	 * @param {Boolean} [isPhantom=false] Should be set to `true` if the model position to map is pointing to a place
+	 * in model tree which no longer exists. For example, it could be an end of a removed model range.
 	 * @returns {module:engine/view/position~Position} Corresponding view position.
 	 */
-	toViewPosition( modelPosition ) {
+	toViewPosition( modelPosition, isPhantom = false ) {
 		const data = {
 			modelPosition,
-			mapper: this
+			mapper: this,
+			isPhantom
 		};
 
 		this.fire( 'modelToViewPosition', data );
@@ -293,18 +316,6 @@ export default class Mapper {
 	}
 
 	/**
-	 * Removes binding between given elements.
-	 *
-	 * @private
-	 * @param {module:engine/model/element~Element} modelElement Model element to unbind.
-	 * @param {module:engine/view/element~Element} viewElement View element to unbind.
-	 */
-	_unbindElements( modelElement, viewElement ) {
-		this._viewToModelMapping.delete( viewElement );
-		this._modelToViewMapping.delete( modelElement );
-	}
-
-	/**
 	 * Gets the length of the view element in the model.
 	 *
 	 * The length is calculated as follows:
@@ -368,7 +379,7 @@ export default class Mapper {
 	 *		We are in the text node so we can simple find the offset.
 	 *		<p>fo<b>ba|r</b>bom</p> -> expected offset: 2, actual offset: 2 -> position found
 	 *
-	 * @private
+	 * @protected
 	 * @param {module:engine/view/element~Element} viewParent Tree view element in which we are looking for the position.
 	 * @param {Number} expectedOffset Expected offset.
 	 * @returns {module:engine/view/position~Position} Found position.
@@ -460,19 +471,32 @@ export default class Mapper {
 	 *			}
 	 *		} );
 	 *
-	 * **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:
+	 * **Note:** keep in mind that sometimes a "phantom" model position is being converted. "Phantom" model position is
+	 * a position that points to a non-existing place in model. Such position might still be valid for conversion, though
+	 * (it would point to a correct place in view when converted). One example of such situation is when a range is
+	 * removed from model, there may be a need to map the range's end (which is no longer valid model position). To
+	 * handle such situation, check `data.isPhantom` flag:
 	 *
-	 *		// Incorrect:
-	 *		const modelElement = data.modelPosition.nodeAfter;
-	 *		const viewElement = data.mapper.toViewElement( modelElement );
-	 *		// ... Do something with `viewElement` and set `data.viewPosition`.
+	 * 		// Assume that there is "customElement" model element and whenever position is before it, we want to move it
+	 * 		// to the inside of the view element bound to "customElement".
+	 *		mapper.on( 'modelToViewPosition', ( evt, data ) => {
+	 *			if ( data.isPhantom ) {
+	 *				return;
+	 *			}
+	 *
+	 *			const sibling = modelPosition.nodeBefore; // Might crash for phantom position that does not exist in model.
+	 *
+	 *			// Check if this is the element we are interested in.
+	 *			if ( !sibling.is( 'customElement' ) ) {
+	 *				return;
+	 *			}
 	 *
-	 *		// Correct:
-	 *		const prevModelElement = data.modelPosition.nodeBefore;
-	 *		const prevViewElement = data.mapper.toViewElement( prevModelElement );
-	 *		// ... Use `prevViewElement` to find correct `data.viewPosition`.
+	 *			const viewElement = data.mapper.toViewElement( sibling );
+	 *
+	 *			data.viewPosition = new ViewPosition( sibling, 0 );
+	 *
+	 *			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.viewPosition` calculated by default callback

+ 48 - 0
packages/ckeditor5-engine/tests/conversion/mapper.js

@@ -69,6 +69,24 @@ describe( 'Mapper', () => {
 			expect( mapper.toModelElement( viewA ) ).to.be.undefined;
 			expect( mapper.toViewElement( modelA ) ).to.be.undefined;
 		} );
+
+		it( 'should not remove binding between view and model element if view element got rebound', () => {
+			const viewA = new ViewElement( 'a' );
+			const modelA = new ModelElement( 'a' );
+			const modelB = new ModelElement( 'b' );
+
+			const mapper = new Mapper();
+			mapper.bindElements( modelA, viewA );
+			mapper.bindElements( modelB, viewA );
+
+			// `modelA` is still bound to `viewA` even though `viewA` got rebound.
+			expect( mapper.toViewElement( modelA ) ).to.equal( viewA );
+
+			mapper.unbindModelElement( modelA );
+
+			expect( mapper.toViewElement( modelA ) ).to.be.undefined;
+			expect( mapper.toModelElement( viewA ) ).to.equal( modelB );
+		} );
 	} );
 
 	describe( 'unbindViewElement', () => {
@@ -87,6 +105,24 @@ describe( 'Mapper', () => {
 			expect( mapper.toModelElement( viewA ) ).to.be.undefined;
 			expect( mapper.toViewElement( modelA ) ).to.be.undefined;
 		} );
+
+		it( 'should not remove binding between model and view element if model element got rebound', () => {
+			const viewA = new ViewElement( 'a' );
+			const viewB = new ViewElement( 'b' );
+			const modelA = new ModelElement( 'a' );
+
+			const mapper = new Mapper();
+			mapper.bindElements( modelA, viewA );
+			mapper.bindElements( modelA, viewB );
+
+			// `viewA` is still bound to `modelA`, even though `modelA` got rebound.
+			expect( mapper.toModelElement( viewA ) ).to.equal( modelA );
+
+			mapper.unbindViewElement( viewA );
+
+			expect( mapper.toModelElement( viewA ) ).to.be.undefined;
+			expect( mapper.toViewElement( modelA ) ).to.equal( viewB );
+		} );
 	} );
 
 	describe( 'Standard mapping', () => {
@@ -574,6 +610,18 @@ describe( 'Mapper', () => {
 		}
 	} );
 
+	it( 'should pass isPhantom flag to model-to-view position mapping callback', () => {
+		const mapper = new Mapper();
+
+		mapper.on( 'modelToViewPosition', ( evt, data ) => {
+			expect( data.isPhantom ).to.be.true;
+
+			evt.stop();
+		} );
+
+		mapper.toViewPosition( {}, true );
+	} );
+
 	describe( 'getModelLength', () => {
 		let mapper;