Bladeren bron

Fixed: ModelConversionDispatcher#convertMove and #convertRemove.
Fixed: model-to-view-converters checks consumable and does not stop event.
Fixed: model-to-view-converters move and remove aligned to changes in ModelConversionDispatcher.

Szymon Cofalik 9 jaren geleden
bovenliggende
commit
06c3cab82e

+ 105 - 59
packages/ckeditor5-engine/src/conversion/model-to-view-converters.js

@@ -7,8 +7,11 @@ import ModelRange from '../model/range.js';
 import ModelPosition from '../model/position.js';
 import ModelElement from '../model/element.js';
 
+import ViewRange from '../view/range.js';
 import ViewElement from '../view/element.js';
 import ViewText from '../view/text.js';
+import ViewTextProxy from '../view/textproxy.js';
+import ViewTreeWalker from '../view/treewalker.js';
 import viewWriter from '../view/writer.js';
 
 /**
@@ -49,7 +52,9 @@ import viewWriter from '../view/writer.js';
  */
 export function insertElement( elementCreator ) {
 	return ( evt, data, consumable, conversionApi ) => {
-		consumable.consume( data.item, 'insert' );
+		if ( !consumable.consume( data.item, 'insert' ) ) {
+			return;
+		}
 
 		const viewPosition = conversionApi.mapper.toViewPosition( data.range.start );
 		const viewElement = ( elementCreator instanceof ViewElement ) ?
@@ -58,8 +63,6 @@ export function insertElement( elementCreator ) {
 
 		conversionApi.mapper.bindElements( data.item, viewElement );
 		viewWriter.insert( viewPosition, viewElement );
-
-		evt.stop();
 	};
 }
 
@@ -77,14 +80,14 @@ export function insertElement( elementCreator ) {
  */
 export function insertText() {
 	return ( evt, data, consumable, conversionApi ) => {
-		consumable.consume( data.item, 'insert' );
+		if ( !consumable.consume( data.item, 'insert' ) ) {
+			return;
+		}
 
 		const viewPosition = conversionApi.mapper.toViewPosition( data.range.start );
 		const viewText = new ViewText( data.item.data );
 
 		viewWriter.insert( viewPosition, viewText );
-
-		evt.stop();
 	};
 }
 
@@ -125,12 +128,13 @@ export function setAttribute( attributeCreator ) {
 	attributeCreator = attributeCreator || ( ( value, key ) => ( { value, key } ) );
 
 	return ( evt, data, consumable, conversionApi ) => {
+		if ( !consumable.consume( data.item, eventNameToConsumableType( evt.name ) ) ) {
+			return;
+		}
+
 		const { key, value } = attributeCreator( data.attributeNewValue, data.attributeKey, data, consumable, conversionApi );
 
-		consumable.consume( data.item, eventNameToConsumableType( evt.name ) );
 		conversionApi.mapper.toViewElement( data.item ).setAttribute( key, value );
-
-		evt.stop();
 	};
 }
 
@@ -172,12 +176,13 @@ export function removeAttribute( attributeCreator ) {
 	attributeCreator = attributeCreator || ( ( value, key ) => ( { key } ) );
 
 	return ( evt, data, consumable, conversionApi ) => {
+		if ( !consumable.consume( data.item, eventNameToConsumableType( evt.name ) ) ) {
+			return;
+		}
+
 		const { key } = attributeCreator( data.attributeOldValue, data.attributeKey, data, consumable, conversionApi );
 
-		consumable.consume( data.item, eventNameToConsumableType( evt.name ) );
 		conversionApi.mapper.toViewElement( data.item ).removeAttribute( key );
-
-		evt.stop();
 	};
 }
 
@@ -218,7 +223,9 @@ export function wrap( elementCreator ) {
 			elementCreator( data.attributeNewValue, data, consumable, conversionApi );
 
 		if ( viewElement ) {
-			consumable.consume( data.item, eventNameToConsumableType( evt.name ) );
+			if ( !consumable.consume( data.item, eventNameToConsumableType( evt.name ) ) ) {
+				return;
+			}
 
 			// If this is a change event (because old value is not empty) and the creator is a function (so
 			// it may create different view elements basing on attribute value) we have to create
@@ -229,8 +236,6 @@ export function wrap( elementCreator ) {
 			}
 
 			viewWriter.wrap( viewRange, viewElement );
-
-			evt.stop();
 		}
 	};
 }
@@ -261,7 +266,9 @@ export function wrap( elementCreator ) {
  */
 export function unwrap( elementCreator ) {
 	return ( evt, data, consumable, conversionApi ) => {
-		consumable.consume( data.item, eventNameToConsumableType( evt.name ) );
+		if ( !consumable.consume( data.item, eventNameToConsumableType( evt.name ) ) ) {
+			return;
+		}
 
 		const viewRange = conversionApi.mapper.toViewRange( data.range );
 		const viewNode = ( elementCreator instanceof ViewElement ) ?
@@ -269,8 +276,6 @@ export function unwrap( elementCreator ) {
 			elementCreator( data.attributeOldValue, data, consumable, conversionApi );
 
 		viewWriter.unwrap( viewRange, viewNode );
-
-		evt.stop();
 	};
 }
 
@@ -285,14 +290,24 @@ export function unwrap( elementCreator ) {
  */
 export function move() {
 	return ( evt, data, consumable, conversionApi ) => {
-		if ( consumable.consume( data.item, 'move' ) ) {
-			const sourceModelRange = ModelRange.createFromPositionAndShift( data.sourcePosition, data.item.offsetSize );
-			const sourceViewRange = conversionApi.mapper.toViewRange( sourceModelRange );
+		if ( !consumable.consume( data.item, 'move' ) ) {
+			return;
+		}
+
+		let sourceViewRange;
 
-			const targetViewPosition = conversionApi.mapper.toViewPosition( data.targetPosition );
+		if ( data.item instanceof ModelElement ) {
+			const viewElement = conversionApi.mapper.toViewElement( data.item );
+			sourceViewRange = ViewRange.createOn( viewElement );
+		} else {
+			const viewPosition = conversionApi.mapper.toViewPosition( data.sourcePosition );
 
-			viewWriter.move( sourceViewRange, targetViewPosition );
+			sourceViewRange = findViewTextRange( viewPosition, data.item.offsetSize );
 		}
+
+		const targetViewPosition = conversionApi.mapper.toViewPosition( data.targetPosition );
+
+		viewWriter.move( sourceViewRange, targetViewPosition );
 	};
 }
 
@@ -307,17 +322,46 @@ export function move() {
  */
 export function remove() {
 	return ( evt, data, consumable, conversionApi ) => {
-		if ( consumable.consume( data.item, 'remove' ) ) {
-			const sourceModelRange = ModelRange.createFromPositionAndShift( data.sourcePosition, data.item.offsetSize );
-			const sourceViewRange = conversionApi.mapper.toViewRange( sourceModelRange );
+		if ( !consumable.consume( data.item, 'remove' ) ) {
+			return;
+		}
+
+		let viewRange = null;
 
-			viewWriter.remove( sourceViewRange );
+		if ( data.item instanceof ModelElement ) {
+			const viewElement = conversionApi.mapper.toViewElement( data.item );
+			viewRange = ViewRange.createOn( viewElement );
+		} else {
+			const viewPosition = conversionApi.mapper.toViewPosition( data.sourcePosition );
 
-			conversionApi.mapper.unbindModelElement( data.item );
+			viewRange = findViewTextRange( viewPosition, data.item.offsetSize );
 		}
+
+		viewWriter.remove( viewRange );
+		conversionApi.mapper.unbindModelElement( data.item );
 	};
 }
 
+// Helper function for `remove` and `move` converters. It returns a ViewRange that starts at ViewPosition `start` and
+// includes `size` characters.
+// This method is used to find a ViewRange basing on ModelPosition and ModelTextProxy item size in `move` and `remove`
+// converters where it is impossible to just map positions because those positions already are invalid in model
+// (because they got moved or removed).
+function findViewTextRange( start, size ) {
+	const walker = new ViewTreeWalker( { startPosition: start, singleCharacters: true, ignoreElementEnd: true } );
+	let offset = 0;
+
+	for ( let value of walker ) {
+		if ( value.item instanceof ViewTextProxy ) {
+			offset++;
+
+			if ( offset == size ) {
+				return new ViewRange( start, walker.position );
+			}
+		}
+	}
+}
+
 /**
  * Function factory, creates default model-to-view converter for elements which name has changed.
  *
@@ -334,38 +378,40 @@ export function remove() {
  */
 export function rename() {
 	return ( evt, data, consumable, conversionApi ) => {
-		if ( consumable.test( data.element, 'rename' ) ) {
-			// Create fake model element that will represent "old version" of renamed element.
-			const fakeElement = new ModelElement( data.oldName, data.element.getAttributes() );
-			// Append the fake element to model document to enable making range on it.
-			data.element.parent.insertChildren( data.element.index, fakeElement );
-
-			// Check what was bound to renamed element.
-			const oldViewElement = conversionApi.mapper.toViewElement( data.element );
-			// Unbind renamed element.
-			conversionApi.mapper.unbindModelElement( data.element );
-			// Bind view element to the fake element.
-			conversionApi.mapper.bindElements( fakeElement, oldViewElement );
-
-			// The range that includes only the renamed element. Will be used to insert an empty element in the view.
-			const insertRange = ModelRange.createFromParentsAndOffsets( data.element.parent, data.element.startOffset, data.element, 0 );
-
-			// Move source position and range of moved nodes. Will be used to move nodes from original view element to renamed one.
-			const moveSourcePosition = ModelPosition.createAt( fakeElement, 0 );
-			const moveRange = ModelRange.createIn( data.element );
-
-			// Remove range containing the fake element. Will be used to remove original view element from the view.
-			const removeRange = ModelRange.createOn( fakeElement );
-
-			// Start the conversion. Use already defined converters by firing insertion, move and remove conversion
-			// on correct ranges / positions.
-			conversionApi.dispatcher.convertInsertion( insertRange );
-			conversionApi.dispatcher.convertMove( moveSourcePosition, moveRange );
-			conversionApi.dispatcher.convertRemove( removeRange.start, removeRange );
-
-			// Cleanup.
-			fakeElement.remove();
+		if ( !consumable.consume( data.element, 'rename' ) ) {
+			return;
 		}
+
+		// Create fake model element that will represent "old version" of renamed element.
+		const fakeElement = new ModelElement( data.oldName, data.element.getAttributes() );
+		// Append the fake element to model document to enable making range on it.
+		data.element.parent.insertChildren( data.element.index, fakeElement );
+
+		// Check what was bound to renamed element.
+		const oldViewElement = conversionApi.mapper.toViewElement( data.element );
+		// Unbind renamed element.
+		conversionApi.mapper.unbindModelElement( data.element );
+		// Bind view element to the fake element.
+		conversionApi.mapper.bindElements( fakeElement, oldViewElement );
+
+		// The range that includes only the renamed element. Will be used to insert an empty element in the view.
+		const insertRange = ModelRange.createFromParentsAndOffsets( data.element.parent, data.element.startOffset, data.element, 0 );
+
+		// Move source position and range of moved nodes. Will be used to move nodes from original view element to renamed one.
+		const moveSourcePosition = ModelPosition.createAt( fakeElement, 0 );
+		const moveRange = ModelRange.createIn( data.element );
+
+		// Remove range containing the fake element. Will be used to remove original view element from the view.
+		const removeRange = ModelRange.createOn( fakeElement );
+
+		// Start the conversion. Use already defined converters by firing insertion, move and remove conversion
+		// on correct ranges / positions.
+		conversionApi.dispatcher.convertInsertion( insertRange );
+		conversionApi.dispatcher.convertMove( moveSourcePosition, moveRange );
+		conversionApi.dispatcher.convertRemove( removeRange.start, removeRange );
+
+		// Cleanup.
+		fakeElement.remove();
 	};
 }
 

+ 12 - 10
packages/ckeditor5-engine/src/conversion/modelconversiondispatcher.js

@@ -5,7 +5,6 @@
 
 import Consumable from './modelconsumable.js';
 import Range from '../model/range.js';
-import Position from '../model/position.js';
 import TextProxy from '../model/textproxy.js';
 import EmitterMixin from '../../utils/emittermixin.js';
 import mix from '../../utils/mix.js';
@@ -199,18 +198,24 @@ export default class ModelConversionDispatcher {
 	 * @param {engine.model.Range} range Moved range (after move).
 	 */
 	convertMove( sourcePosition, range ) {
+		// Keep in mind that move dispatcher expects flat range.
 		const consumable = this._createConsumableForRange( range, 'move' );
 
-		// Fire a separate event for each top-most node and text fragment contained in the range.
-		const items = Array.from( range.getItems( { shallow: true } ) ).reverse();
+		const items = Array.from( range.getItems( { shallow: true } ) );
+		const rangeSize = range.end.offset - range.start.offset;
+		const inSameParent = sourcePosition.parent == range.start.parent;
+
+		let offset = 0;
 
 		for ( let item of items ) {
 			const data = {
-				sourcePosition: sourcePosition.getShiftedBy( item.startOffset - range.start.offset ),
-				targetPosition: Position.createAt( range.start ),
+				sourcePosition: sourcePosition,
+				targetPosition: inSameParent ? range.start.getShiftedBy( rangeSize ) : range.start.getShiftedBy( offset ),
 				item: item
 			};
 
+			offset += data.item.offsetSize;
+
 			this._testAndFire( 'move', data, consumable );
 		}
 	}
@@ -225,12 +230,9 @@ export default class ModelConversionDispatcher {
 	convertRemove( sourcePosition, range ) {
 		const consumable = this._createConsumableForRange( range, 'remove' );
 
-		// Fire a separate event for each top-most node and text fragment contained in the range.
-		const items = Array.from( range.getItems( { shallow: true } ) ).reverse();
-
-		for ( let item of items ) {
+		for ( let item of range.getItems( { shallow: true } ) ) {
 			const data = {
-				sourcePosition: sourcePosition.getShiftedBy( item.startOffset - range.start.offset ),
+				sourcePosition: sourcePosition,
 				item: item
 			};
 

+ 101 - 2
packages/ckeditor5-engine/tests/conversion/model-to-view-converters.js

@@ -97,6 +97,18 @@ describe( 'insertText', () => {
 
 		expect( viewToString( viewRoot ) ).to.equal( '<div>நிலைக்கு</div>' );
 	} );
+
+	it( 'should be possible to override it', () => {
+		modelRoot.appendChildren( new ModelText( 'foobar' ) );
+		dispatcher.on( 'insert:$text', insertText() );
+		dispatcher.on( 'insert:$text', ( evt, data, consumable ) => {
+			consumable.consume( data.item, 'insert' );
+		}, { priority: 'high' } );
+
+		dispatcher.convertInsertion( ModelRange.createIn( modelRoot ) );
+
+		expect( viewToString( viewRoot ) ).to.equal( '<div></div>' );
+	} );
 } );
 
 describe( 'insertElement', () => {
@@ -196,6 +208,48 @@ describe( 'setAttribute/removeAttribute', () => {
 
 		expect( viewToString( viewRoot ) ).to.equal( '<div><p>foobar</p><div class="nice"></div></div>' );
 	} );
+
+	it( 'should be possible to override setAttribute', () => {
+		const modelElement = new ModelElement( 'paragraph', { class: 'foo' }, new ModelText( 'foobar' ) );
+		const viewElement = new ViewContainerElement( 'p' );
+
+		modelRoot.appendChildren( modelElement );
+		dispatcher.on( 'insert:paragraph', insertElement( viewElement ) );
+		dispatcher.on( 'insert:$text', insertText() );
+		dispatcher.on( 'addAttribute:class', setAttribute() );
+		dispatcher.on( 'addAttribute:class', ( evt, data, consumable ) => {
+			consumable.consume( data.item, 'addAttribute:class' );
+		}, { priority: 'high' } );
+
+		dispatcher.convertInsertion( ModelRange.createIn( modelRoot ) );
+
+		// No attribute set.
+		expect( viewToString( viewRoot ) ).to.equal( '<div><p>foobar</p></div>' );
+	} );
+
+	it( 'should be possible to override removeAttribute', () => {
+		const modelElement = new ModelElement( 'paragraph', { class: 'foo' }, new ModelText( 'foobar' ) );
+		const viewElement = new ViewContainerElement( 'p' );
+
+		modelRoot.appendChildren( modelElement );
+		dispatcher.on( 'insert:paragraph', insertElement( viewElement ) );
+		dispatcher.on( 'insert:$text', insertText() );
+		dispatcher.on( 'addAttribute:class', setAttribute() );
+		dispatcher.on( 'removeAttribute:class', removeAttribute() );
+		dispatcher.on( 'removeAttribute:class', ( evt, data, consumable ) => {
+			consumable.consume( data.item, 'removeAttribute:class' );
+		}, { priority: 'high' } );
+
+		dispatcher.convertInsertion( ModelRange.createIn( modelRoot ) );
+
+		expect( viewToString( viewRoot ) ).to.equal( '<div><p class="foo">foobar</p></div>' );
+
+		modelElement.removeAttribute( 'class' );
+		dispatcher.convertAttribute( 'removeAttribute', createRangeOnElementOnly( modelElement ), 'class', 'bar', null );
+
+		// Nothing changed.
+		expect( viewToString( viewRoot ) ).to.equal( '<div><p class="foo">foobar</p></div>' );
+	} );
 } );
 
 describe( 'wrap/unwrap', () => {
@@ -305,6 +359,50 @@ describe( 'wrap/unwrap', () => {
 
 		expect( viewToString( viewRoot ) ).to.equal( '<div><p>நிலைக்கு</p></div>' );
 	} );
+
+	it( 'should be possible to override wrap', () => {
+		const modelElement = new ModelElement( 'paragraph', null, new ModelText( 'foobar', { bold: true } ) );
+		const viewP = new ViewContainerElement( 'p' );
+		const viewB = new ViewAttributeElement( 'b' );
+
+		modelRoot.appendChildren( modelElement );
+		dispatcher.on( 'insert:paragraph', insertElement( viewP ) );
+		dispatcher.on( 'insert:$text', insertText() );
+		dispatcher.on( 'addAttribute:bold', wrap( viewB ) );
+		dispatcher.on( 'addAttribute:bold', ( evt, data, consumable ) => {
+			consumable.consume( data.item, 'addAttribute:bold' );
+		}, { priority: 'high' } );
+
+		dispatcher.convertInsertion( ModelRange.createIn( modelRoot ) );
+
+		expect( viewToString( viewRoot ) ).to.equal( '<div><p>foobar</p></div>' );
+	} );
+
+	it( 'should be possible to override unwrap', () => {
+		const modelElement = new ModelElement( 'paragraph', null, new ModelText( 'foobar', { bold: true } ) );
+		const viewP = new ViewContainerElement( 'p' );
+		const viewB = new ViewAttributeElement( 'b' );
+
+		modelRoot.appendChildren( modelElement );
+		dispatcher.on( 'insert:paragraph', insertElement( viewP ) );
+		dispatcher.on( 'insert:$text', insertText() );
+		dispatcher.on( 'addAttribute:bold', wrap( viewB ) );
+		dispatcher.on( 'removeAttribute:bold', unwrap( viewB ) );
+		dispatcher.on( 'removeAttribute:bold', ( evt, data, consumable ) => {
+			consumable.consume( data.item, 'removeAttribute:bold' );
+		}, { priority: 'high' } );
+
+		dispatcher.convertInsertion( ModelRange.createIn( modelRoot ) );
+
+		expect( viewToString( viewRoot ) ).to.equal( '<div><p><b>foobar</b></p></div>' );
+
+		modelWriter.removeAttribute( ModelRange.createIn( modelElement ), 'bold' );
+
+		dispatcher.convertAttribute( 'removeAttribute', ModelRange.createIn( modelElement ), 'bold', true, null );
+
+		// Nothing changed.
+		expect( viewToString( viewRoot ) ).to.equal( '<div><p><b>foobar</b></p></div>' );
+	} );
 } );
 
 describe( 'move', () => {
@@ -505,11 +603,12 @@ describe( 'rename', () => {
 	} );
 
 	it( 'should enable default rename conversion, that uses already registered callbacks', () => {
+		const insertCallCount = converters.insert.callCount;
+
 		expect( viewRoot.getChild( 0 ).name ).to.equal( 'oldName' );
 		dispatcher.convertRename( element, oldName );
 
-		// Called twice, first time when renamed element was originally inserted to model and converted to view.
-		expect( converters.insert.calledTwice ).to.be.true;
+		expect( converters.insert.callCount - insertCallCount ).to.equal( 1 );
 		expect( converters.move.calledOnce ).to.be.true;
 		expect( converters.remove.calledOnce ).to.be.true;
 

+ 4 - 4
packages/ckeditor5-engine/tests/conversion/modelconversiondispatcher.js

@@ -287,7 +287,7 @@ describe( 'ModelConversionDispatcher', () => {
 	} );
 
 	describe( 'convertMove', () => {
-		it( 'should fire single event for moved range', () => {
+		it( 'should fire event for moved range', () => {
 			root.appendChildren( new ModelText( 'barfoo' ) );
 
 			const range = ModelRange.createFromParentsAndOffsets( root, 0, root, 3 );
@@ -298,14 +298,14 @@ describe( 'ModelConversionDispatcher', () => {
 				loggedEvents.push( log );
 			} );
 
-			dispatcher.convertMove( ModelPosition.createFromParentAndOffset( root , 3 ), range );
+			dispatcher.convertMove( ModelPosition.createFromParentAndOffset( root , 0 ), range );
 
-			expect( loggedEvents ).to.deep.equal( [ 'move:3:0:3' ] );
+			expect( loggedEvents ).to.deep.equal( [ 'move:0:3:3' ] );
 		} );
 	} );
 
 	describe( 'convertRemove', () => {
-		it( 'should fire single event for removed range', () => {
+		it( 'should fire event for removed range', () => {
 			root.appendChildren( new ModelText( 'foo' ) );
 			doc.graveyard.appendChildren( new ModelText( 'bar' ) );