Przeglądaj źródła

Added: `DowncastDispatcher` will fire an additional event for the whole marker range before events for items in that range are fired.

Szymon Cofalik 7 lat temu
rodzic
commit
8de73abd22

+ 6 - 2
packages/ckeditor5-engine/src/conversion/downcast-converters.js

@@ -564,6 +564,10 @@ export function remove() {
  */
 export function insertUIElement( elementCreator ) {
 	return ( evt, data, conversionApi ) => {
+		if ( !data.markerRange ) {
+			return;
+		}
+
 		// Create two view elements. One will be inserted at the beginning of marker, one at the end.
 		// If marker is collapsed, only "opening" element will be inserted.
 		data.isOpening = true;
@@ -858,7 +862,7 @@ export function wrap( elementCreator ) {
  */
 export function highlightText( highlightDescriptor ) {
 	return ( evt, data, conversionApi ) => {
-		if ( data.markerRange.isCollapsed ) {
+		if ( !data.item ) {
 			return;
 		}
 
@@ -921,7 +925,7 @@ export function highlightText( highlightDescriptor ) {
  */
 export function highlightElement( highlightDescriptor ) {
 	return ( evt, data, conversionApi ) => {
-		if ( data.markerRange.isCollapsed ) {
+		if ( !data.item ) {
 			return;
 		}
 

+ 14 - 8
packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

@@ -321,22 +321,28 @@ export default class DowncastDispatcher {
 		// In markers' case, event name == consumable name.
 		const eventName = 'addMarker:' + markerName;
 
-		// When range is collapsed - fire single event with collapsed range in consumable.
-		if ( markerRange.isCollapsed ) {
-			const consumable = new Consumable();
-			consumable.add( markerRange, eventName );
+		//
+		// First, fire an event for the whole marker.
+		//
+		const consumable = new Consumable();
+		consumable.add( markerRange, eventName );
 
-			this.conversionApi.consumable = consumable;
+		this.conversionApi.consumable = consumable;
 
-			this.fire( eventName, { markerName, markerRange }, this.conversionApi );
+		this.fire( eventName, { markerName, markerRange }, this.conversionApi );
 
+		//
+		// Do not fire events for each item inside the range if the range got consumed.
+		//
+		if ( !consumable.test( markerRange, eventName ) ) {
 			return;
 		}
 
-		// Create consumable for each item in range.
+		//
+		// Then, fire an event for each item inside the marker range.
+		//
 		this.conversionApi.consumable = this._createConsumableForRange( markerRange, eventName );
 
-		// Create separate event for each node in the range.
 		for ( const item of markerRange.getItems() ) {
 			// Do not fire event for already consumed items.
 			if ( !this.conversionApi.consumable.test( item, eventName ) ) {

+ 62 - 13
packages/ckeditor5-engine/tests/conversion/downcastdispatcher.js

@@ -446,22 +446,29 @@ describe( 'DowncastDispatcher', () => {
 	} );
 
 	describe( 'convertMarkerAdd', () => {
-		let range, element, text;
+		let element, text;
 
 		beforeEach( () => {
 			text = new ModelText( 'foo bar baz' );
 			element = new ModelElement( 'paragraph', null, [ text ] );
 			root._appendChild( [ element ] );
-
-			range = model.createRange( model.createPositionAt( element, 0 ), model.createPositionAt( element, 4 ) );
 		} );
 
-		it( 'should fire addMarker event', () => {
-			sinon.spy( dispatcher, 'fire' );
+		it( 'should fire addMarker event for whole collapsed marker', () => {
+			const range = model.createRange( model.createPositionAt( element, 2 ), model.createPositionAt( element, 2 ) );
+
+			const spy = sinon.spy();
+
+			dispatcher.on( 'addMarker:name', ( evt, data ) => {
+				spy();
+
+				expect( data.markerName ).to.equal( 'name' );
+				expect( data.markerRange.isEqual( range ) ).to.be.true;
+			} );
 
 			dispatcher.convertMarkerAdd( 'name', range );
 
-			expect( dispatcher.fire.calledWith( 'addMarker:name' ) ).to.be.true;
+			expect( spy.calledOnce ).to.be.true;
 		} );
 
 		it( 'should not convert marker if it is in graveyard', () => {
@@ -483,27 +490,69 @@ describe( 'DowncastDispatcher', () => {
 			expect( dispatcher.fire.called ).to.be.false;
 		} );
 
-		it( 'should fire conversion for each item in the range', () => {
-			range = model.createRangeIn( root );
+		it( 'should fire addMarker event for whole non-collapsed marker and for each item in the range', () => {
+			const range = model.createRangeIn( root );
+
+			const spyWholeRange = sinon.spy();
+
+			dispatcher.on( 'addMarker:name', ( evt, data ) => {
+				if ( !data.item ) {
+					spyWholeRange();
 
+					expect( data.markerName ).to.equal( 'name' );
+					expect( data.markerRange.isEqual( range ) ).to.be.true;
+				}
+			} );
+
+			const spyItems = sinon.spy();
 			const items = [];
 
 			dispatcher.on( 'addMarker:name', ( evt, data, conversionApi ) => {
-				expect( data.markerName ).to.equal( 'name' );
-				expect( data.markerRange.isEqual( range ) ).to.be.true;
-				expect( conversionApi.consumable.test( data.item, 'addMarker:name' ) );
+				if ( data.item ) {
+					spyItems();
 
-				items.push( data.item );
+					expect( data.markerName ).to.equal( 'name' );
+					expect( data.markerRange.isEqual( range ) ).to.be.true;
+					expect( conversionApi.consumable.test( data.item, 'addMarker:name' ) );
+
+					items.push( data.item );
+				}
 			} );
 
 			dispatcher.convertMarkerAdd( 'name', range );
 
+			expect( spyWholeRange.calledOnce ).to.be.true;
+			expect( spyItems.calledTwice ).to.be.true;
+
 			expect( items[ 0 ] ).to.equal( element );
 			expect( items[ 1 ].data ).to.equal( text.data );
 		} );
 
+		it( 'should not fire conversion for non-collapsed marker items if marker was consumed in earlier event', () => {
+			const range = model.createRangeIn( root );
+
+			dispatcher.on( 'addMarker:name', ( evt, data, conversionApi ) => {
+				if ( !data.item ) {
+					conversionApi.consumable.consume( data.markerRange, evt.name );
+				}
+			}, { priority: 'high' } );
+
+			const spyItems = sinon.spy();
+			const items = [];
+
+			dispatcher.on( 'addMarker:name', ( evt, data ) => {
+				if ( data.item ) {
+					spyItems();
+				}
+			} );
+
+			dispatcher.convertMarkerAdd( 'name', range );
+
+			expect( spyItems.called ).to.be.false;
+		} );
+
 		it( 'should be possible to override', () => {
-			range = model.createRangeIn( root );
+			const range = model.createRangeIn( root );
 
 			const addMarkerSpy = sinon.spy();
 			const highAddMarkerSpy = sinon.spy();