Browse Source

Changed: Fire marker conversion if marker is reinserted from graveyard or inserted with model nodes.
Changed: Do not fire marker conversion if marker is in graveyard or not in model root.

Szymon Cofalik 9 years ago
parent
commit
c2f9dbe41b

+ 2 - 2
packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -15,7 +15,7 @@ import {
 	remove,
 	move,
 	rename,
-	insertIntoMarker,
+	insertMarkerOrIntoMarker,
 	moveInOutOfMarker
 } from '../conversion/model-to-view-converters';
 import { convertSelectionChange } from '../conversion/view-selection-to-model-converters';
@@ -123,7 +123,7 @@ export default class EditingController {
 		this.modelToView.on( 'rename', rename(), { priority: 'low' } );
 
 		// Attach default markers converters.
-		this.modelToView.on( 'insert', insertIntoMarker( this.model.markers ), { priority: 'lowest' } );
+		this.modelToView.on( 'insert', insertMarkerOrIntoMarker( this.model.markers ), { priority: 'lowest' } );
 		this.modelToView.on( 'move', moveInOutOfMarker( this.model.markers ), { priority: 'lowest' } );
 
 		// Attach default selection converters.

+ 5 - 2
packages/ckeditor5-engine/src/conversion/model-to-view-converters.js

@@ -492,7 +492,8 @@ export function rename() {
 }
 
 /**
- * Function factory, creates a default converter for inserting {@link module:engine/model/item~Item model item} into a marker range.
+ * Function factory, creates a default converter for inserting {@link module:engine/model/item~Item model item} into a marker range
+ * or inserting a part of a model that contains a marker (this happens when reinserting nodes from graveyard).
  *
  *		modelDispatcher.on( 'insert', insertIntoRange( modelDocument.markers ) );
  *
@@ -500,13 +501,15 @@ export function rename() {
  * inserting.
  * @returns {Function}
  */
-export function insertIntoMarker( markerCollection ) {
+export function insertMarkerOrIntoMarker( markerCollection ) {
 	return ( evt, data, consumable, conversionApi ) => {
 		for ( let marker of markerCollection ) {
 			const range = marker.getRange();
 
 			if ( range.containsPosition( data.range.start ) ) {
 				conversionApi.dispatcher.convertMarker( 'addMarker', marker.name, data.range );
+			} else if ( data.range.containsRange( range ) || data.range.isEqual( range ) ) {
+				conversionApi.dispatcher.convertMarker( 'addMarker', marker.name, range );
 			}
 		}
 	};

+ 5 - 0
packages/ckeditor5-engine/src/conversion/modelconversiondispatcher.js

@@ -338,6 +338,11 @@ export default class ModelConversionDispatcher {
 	 * @param {module:engine/model/range~Range} range Marker range.
 	 */
 	convertMarker( type, name, range ) {
+		// Do not convert if range is in graveyard or not in the document (e.g. in DocumentFragment).
+		if ( !range.root.document || range.root.rootName == '$graveyard' ) {
+			return;
+		}
+
 		const consumable = this._createMarkerConsumable( type, range );
 		const data = { name, range };
 

+ 56 - 7
packages/ckeditor5-engine/tests/controller/editingcontroller.js

@@ -18,8 +18,8 @@ import buildModelConverter from '../../src/conversion/buildmodelconverter';
 import ModelDocument from '../../src/model/document';
 import ModelPosition from '../../src/model/position';
 import ModelElement from '../../src/model/element';
+import ModelText from '../../src/model/text';
 import ModelRange from '../../src/model/range';
-import ModelLiveRange from '../../src/model/liverange';
 import ModelDocumentFragment from '../../src/model/documentfragment';
 
 import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement';
@@ -55,6 +55,7 @@ describe( 'EditingController', () => {
 
 		afterEach( () => {
 			editing.destroy();
+			model.markers.destroy();
 		} );
 
 		it( 'should create root', () => {
@@ -274,7 +275,7 @@ describe( 'EditingController', () => {
 		} );
 
 		it( 'should forward add marker event if content is inserted into a marker range', () => {
-			const markerRange = ModelLiveRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 3 );
+			const markerRange = ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 3 );
 			const innerRange = ModelRange.createFromParentsAndOffsets( modelRoot, 1, modelRoot, 2 );
 			const consumableMock = {
 				consume: () => true,
@@ -294,8 +295,56 @@ describe( 'EditingController', () => {
 			editing.modelToView.convertMarker.restore();
 		} );
 
+		it( 'should forward add marker event if inserted content has a marker (reinsert from graveyard)', () => {
+			const gyHolder = new ModelElement( '$graveyardHolder', [], new ModelText( 'foo' ) );
+			model.graveyard.appendChildren( gyHolder );
+
+			const markerRange = ModelRange.createIn( gyHolder );
+			const consumableMock = {
+				consume: () => true,
+				test: () => true
+			};
+
+			model.markers.set( 'name', markerRange );
+
+			sinon.spy( editing.modelToView, 'convertMarker' );
+
+			editing.modelToView.fire( 'insert', {
+				range: markerRange
+			}, consumableMock, { dispatcher: editing.modelToView } );
+
+			expect( editing.modelToView.convertMarker.calledWithExactly( 'addMarker', 'name', markerRange ) ).to.be.true;
+
+			editing.modelToView.convertMarker.restore();
+		} );
+
+		it( 'should forward add marker event if inserted content has a marker (inserted element from outside of tree)', () => {
+			const element = new ModelElement( new ModelText( 'foo' ) );
+			modelRoot.appendChildren( element );
+
+			const markerRange = ModelRange.createFromParentsAndOffsets( element, 1, element, 2 );
+			const outerRange = ModelRange.createOn( element );
+
+			const consumableMock = {
+				consume: () => true,
+				test: () => true
+			};
+
+			model.markers.set( 'name', markerRange );
+
+			sinon.spy( editing.modelToView, 'convertMarker' );
+
+			editing.modelToView.fire( 'insert', {
+				range: outerRange
+			}, consumableMock, { dispatcher: editing.modelToView } );
+
+			expect( editing.modelToView.convertMarker.calledWithExactly( 'addMarker', 'name', markerRange ) ).to.be.true;
+
+			editing.modelToView.convertMarker.restore();
+		} );
+
 		it( 'should not start marker conversion if content is not inserted into any marker range', () => {
-			const markerRange = ModelLiveRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 3 );
+			const markerRange = ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 3 );
 			const insertRange = ModelRange.createFromParentsAndOffsets( modelRoot, 6, modelRoot, 8 );
 			const consumableMock = {
 				consume: () => true,
@@ -316,7 +365,7 @@ describe( 'EditingController', () => {
 		} );
 
 		it( 'should forward remove marker event if part of marker range is moved - intersecting', () => {
-			const markerRange = ModelLiveRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 );
+			const markerRange = ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 );
 			const consumableMock = {
 				consume: () => true,
 				test: () => true
@@ -342,7 +391,7 @@ describe( 'EditingController', () => {
 				model.batch().insert( ModelPosition.createAt( model.getRoot(), 'end' ), new ModelElement( 'paragraph' ) );
 			} );
 
-			const markerRange = ModelLiveRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 2 );
+			const markerRange = ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 2 );
 			const consumableMock = {
 				consume: () => true,
 				test: () => true
@@ -368,7 +417,7 @@ describe( 'EditingController', () => {
 				model.batch().insert( ModelPosition.createAt( model.getRoot(), 'end' ), new ModelElement( 'paragraph' ) );
 			} );
 
-			const markerRange = ModelLiveRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 3 );
+			const markerRange = ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 3 );
 			const consumableMock = {
 				consume: () => true,
 				test: () => true
@@ -390,7 +439,7 @@ describe( 'EditingController', () => {
 		} );
 
 		it( 'should not start marker conversion if moved content does not affect the marker', () => {
-			const markerRange = ModelLiveRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 );
+			const markerRange = ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 );
 			const consumableMock = {
 				consume: () => true,
 				test: () => true

+ 34 - 17
packages/ckeditor5-engine/tests/conversion/modelconversiondispatcher.js

@@ -515,36 +515,53 @@ describe( 'ModelConversionDispatcher', () => {
 		it( 'should fire event based on passed parameters', () => {
 			sinon.spy( dispatcher, 'fire' );
 
-			const data = {
-				name: 'name',
-				range: range
-			};
+			dispatcher.convertMarker( 'addMarker', 'name', range );
 
-			dispatcher.convertMarker( 'addMarker', data );
+			expect( dispatcher.fire.calledWith( 'addMarker:name', 'name', range ) );
 
-			expect( dispatcher.fire.calledWith( 'addMarker:name', data ) );
+			dispatcher.convertMarker( 'removeMarker', 'name', range );
 
-			dispatcher.convertMarker( 'removeMarker', data );
+			expect( dispatcher.fire.calledWith( 'removeMarker:name', 'name', range ) );
+		} );
+
+		it( 'should not convert marker if it is added in graveyard', () => {
+			const gyRange = ModelRange.createFromParentsAndOffsets( doc.graveyard, 0, doc.graveyard, 0 );
+			sinon.spy( dispatcher, 'fire' );
 
-			expect( dispatcher.fire.calledWith( 'removeMarker:name', data ) );
+			dispatcher.convertMarker( 'addMarker', 'name', gyRange );
+
+			expect( dispatcher.fire.called ).to.be.false;
+
+			dispatcher.convertMarker( 'removeMarker', 'name', gyRange );
+
+			expect( dispatcher.fire.called ).to.be.false;
 		} );
 
-		it( 'should prepare consumable values', () => {
-			const data = {
-				name: 'name',
-				range: range
-			};
+		it( 'should not convert marker if it is not in model root', () => {
+			const element = new ModelElement( 'element', null, new ModelText( 'foo' ) );
+			const eleRange = ModelRange.createFromParentsAndOffsets( element, 1, element, 2 );
+			sinon.spy( dispatcher, 'fire' );
+
+			dispatcher.convertMarker( 'addMarker', 'name', eleRange );
+
+			expect( dispatcher.fire.called ).to.be.false;
+
+			dispatcher.convertMarker( 'removeMarker', 'name', eleRange );
 
+			expect( dispatcher.fire.called ).to.be.false;
+		} );
+
+		it( 'should prepare consumable values', () => {
 			dispatcher.on( 'addMarker:name', ( evt, data, consumable ) => {
-				expect( consumable.test( data.range, 'range' ) ).to.be.true;
+				expect( consumable.test( data.range, 'addMarker' ) ).to.be.true;
 			} );
 
 			dispatcher.on( 'removeMarker:name', ( evt, data, consumable ) => {
-				expect( consumable.test( data.range, 'range' ) ).to.be.true;
+				expect( consumable.test( data.range, 'removeMarker' ) ).to.be.true;
 			} );
 
-			dispatcher.convertMarker( 'addMarker', data );
-			dispatcher.convertMarker( 'removeMarker', data );
+			dispatcher.convertMarker( 'addMarker', 'name', range );
+			dispatcher.convertMarker( 'removeMarker', 'name', range );
 		} );
 	} );
 } );