瀏覽代碼

Fixed: Items inserted into an already highlighted element with custom highlight handling should not get additional highlight.

Szymon Cofalik 8 年之前
父節點
當前提交
4e53a40428

+ 34 - 8
packages/ckeditor5-engine/src/conversion/modelconversiondispatcher.js

@@ -217,15 +217,11 @@ export default class ModelConversionDispatcher {
 
 		for ( const marker of this._modelDocument.markers ) {
 			const markerRange = marker.getRange();
+			const intersection = markerRange.getIntersection( range );
 
 			// Check if inserted content is inserted into a marker.
-			if ( markerRange.containsPosition( range.start ) ) {
-				this.convertMarker( 'addMarker', marker.name, markerRange.getIntersection( range ) );
-			}
-
-			// Check if inserted content contains a marker.
-			if ( range.containsRange( markerRange, true ) ) {
-				this.convertMarker( 'addMarker', marker.name, markerRange );
+			if ( intersection && shouldMarkerChangeBeConverted( range.start, marker, this.conversionApi.mapper ) ) {
+				this.convertMarker( 'addMarker', marker.name, intersection );
 			}
 		}
 	}
@@ -367,10 +363,16 @@ export default class ModelConversionDispatcher {
 		this.fire( 'selection', { selection }, consumable, this.conversionApi );
 
 		for ( const marker of markers ) {
+			const markerRange = marker.getRange();
+
+			if ( !shouldMarkerChangeBeConverted( selection.getFirstPosition(), marker, this.conversionApi.mapper ) ) {
+				continue;
+			}
+
 			const data = {
 				selection,
 				markerName: marker.name,
-				markerRange: marker.getRange()
+				markerRange
 			};
 
 			if ( consumable.test( selection, 'selectionMarker:' + marker.name ) ) {
@@ -717,3 +719,27 @@ export default class ModelConversionDispatcher {
 }
 
 mix( ModelConversionDispatcher, EmitterMixin );
+
+// Helper function, checks whether change of `marker` at `modelPosition` should be converted. Marker changes are not
+// converted if they happen inside an element with custom conversion method.
+//
+// @param {module:engine/model/position~Position} modelPosition
+// @param {module:engine/model/markercollection~Marker} marker
+// @param {module:engine/conversion/mapper~Mapper} mapper
+// @returns {Boolean}
+function shouldMarkerChangeBeConverted( modelPosition, marker, mapper ) {
+	const range = marker.getRange();
+	const ancestors = Array.from( modelPosition.getAncestors() );
+	ancestors.shift(); // Remove root element. It cannot be passed to `model.Range#containsItem`.
+	ancestors.reverse();
+
+	const hasCustomHandling = ancestors.some( element => {
+		if ( range.containsItem( element ) ) {
+			const viewElement = mapper.toViewElement( element );
+
+			return !!viewElement.getCustomProperty( 'addHighlight' );
+		}
+	} );
+
+	return !hasCustomHandling;
+}

+ 11 - 0
packages/ckeditor5-engine/src/model/range.js

@@ -118,6 +118,17 @@ export default class Range {
 	}
 
 	/**
+	 * Checks whether given {@link module:engine/model/item~Item} is inside this range.
+	 *
+	 * @param {module:engine/model/item~Item} item Model item to check.
+	 */
+	containsItem( item ) {
+		const pos = Position.createBefore( item );
+
+		return this.containsPosition( pos ) || this.start.isEqual( pos );
+	}
+
+	/**
 	 * Two ranges are equal if their {@link #start} and {@link #end} positions are equal.
 	 *
 	 * @param {module:engine/model/range~Range} otherRange Range to compare with.

+ 114 - 0
packages/ckeditor5-engine/tests/conversion/modelconversiondispatcher.js

@@ -15,6 +15,8 @@ import RenameOperation from '../../src/model/operation/renameoperation';
 import AttributeOperation from '../../src/model/operation/attributeoperation';
 import { wrapInDelta } from '../../tests/model/_utils/utils';
 
+import ViewContainerElement from '../../src/view/containerelement';
+
 describe( 'ModelConversionDispatcher', () => {
 	let dispatcher, doc, root, gyPos;
 
@@ -360,6 +362,78 @@ describe( 'ModelConversionDispatcher', () => {
 			expect( callArgs[ 1 ] ).to.equal( 'marker' );
 			expect( callArgs[ 2 ].isEqual( markerRange ) ).to.be.true;
 		} );
+
+		it( 'should not fire marker conversion if content is inserted into element with custom highlight handling', () => {
+			sinon.spy( dispatcher, 'convertMarker' );
+
+			const text = new ModelText( 'abc' );
+			const caption = new ModelElement( 'caption', null, text );
+			const image = new ModelElement( 'image', null, caption );
+			root.appendChildren( [ image ] );
+
+			// Create view elements that will be "mapped" to model elements.
+			const viewCaption = new ViewContainerElement( 'caption' );
+			const viewFigure = new ViewContainerElement( 'figure', null, viewCaption );
+
+			// Create custom highlight handler mock.
+			viewFigure.setCustomProperty( 'addHighlight', () => {} );
+			viewFigure.setCustomProperty( 'removeHighlight', () => {} );
+
+			// Create mapper mock.
+			dispatcher.conversionApi.mapper = {
+				toViewElement( modelElement ) {
+					if ( modelElement == image ) {
+						return viewFigure;
+					} else if ( modelElement == caption ) {
+						return viewCaption;
+					}
+				}
+			};
+
+			const markerRange = ModelRange.createFromParentsAndOffsets( root, 0, root, 1 );
+			doc.markers.set( 'marker', markerRange );
+
+			const insertionRange = ModelRange.createFromParentsAndOffsets( caption, 1, caption, 2 );
+			dispatcher.convertInsertion( insertionRange );
+
+			expect( dispatcher.convertMarker.called ).to.be.false;
+		} );
+
+		it( 'should fire marker conversion if inserted into element with highlight handling but element is not in marker range', () => {
+			sinon.spy( dispatcher, 'convertMarker' );
+
+			const text = new ModelText( 'abc' );
+			const caption = new ModelElement( 'caption', null, text );
+			const image = new ModelElement( 'image', null, caption );
+			root.appendChildren( [ image ] );
+
+			// Create view elements that will be "mapped" to model elements.
+			const viewCaption = new ViewContainerElement( 'caption' );
+			const viewFigure = new ViewContainerElement( 'figure', null, viewCaption );
+
+			// Create custom highlight handler mock.
+			viewFigure.setCustomProperty( 'addHighlight', () => {} );
+			viewFigure.setCustomProperty( 'removeHighlight', () => {} );
+
+			// Create mapper mock.
+			dispatcher.conversionApi.mapper = {
+				toViewElement( modelElement ) {
+					if ( modelElement == image ) {
+						return viewFigure;
+					} else if ( modelElement == caption ) {
+						return viewCaption;
+					}
+				}
+			};
+
+			const markerRange = ModelRange.createFromParentsAndOffsets( caption, 0, caption, 3 );
+			doc.markers.set( 'marker', markerRange );
+
+			const insertionRange = ModelRange.createFromParentsAndOffsets( caption, 2, caption, 3 );
+			dispatcher.convertInsertion( insertionRange );
+
+			expect( dispatcher.convertMarker.called ).to.be.true;
+		} );
 	} );
 
 	describe( 'convertMove', () => {
@@ -600,6 +674,46 @@ describe( 'ModelConversionDispatcher', () => {
 			expect( dispatcher.fire.calledWith( 'selectionMarker:name' ) ).to.be.true;
 		} );
 
+		it( 'should not fire event for marker if selection is in a element with custom highlight handling', () => {
+			// Clear after `beforeEach`.
+			root.removeChildren( 0, root.childCount );
+
+			const text = new ModelText( 'abc' );
+			const caption = new ModelElement( 'caption', null, text );
+			const image = new ModelElement( 'image', null, caption );
+			root.appendChildren( [ image ] );
+
+			// Create view elements that will be "mapped" to model elements.
+			const viewCaption = new ViewContainerElement( 'caption' );
+			const viewFigure = new ViewContainerElement( 'figure', null, viewCaption );
+
+			// Create custom highlight handler mock.
+			viewFigure.setCustomProperty( 'addHighlight', () => {} );
+			viewFigure.setCustomProperty( 'removeHighlight', () => {} );
+
+			// Create mapper mock.
+			dispatcher.conversionApi.mapper = {
+				toViewElement( modelElement ) {
+					if ( modelElement == image ) {
+						return viewFigure;
+					} else if ( modelElement == caption ) {
+						return viewCaption;
+					}
+				}
+			};
+
+			doc.markers.set( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 1 ) );
+			doc.selection.setRanges( [ ModelRange.createFromParentsAndOffsets( caption, 1, caption, 1 ) ] );
+
+			sinon.spy( dispatcher, 'fire' );
+
+			const markers = Array.from( doc.markers.getMarkersAtPosition( doc.selection.getFirstPosition() ) );
+
+			dispatcher.convertSelection( doc.selection, markers );
+
+			expect( dispatcher.fire.calledWith( 'selectionMarker:name' ) ).to.be.false;
+		} );
+
 		it( 'should not fire events if information about marker has been consumed', () => {
 			doc.markers.set( 'foo', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) );
 			doc.markers.set( 'bar', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) );

+ 32 - 0
packages/ckeditor5-engine/tests/model/range.js

@@ -412,6 +412,12 @@ describe( 'Range', () => {
 
 			expect( range.containsPosition( position ) ).to.be.true;
 		} );
+	} );
+
+	describe( 'containsRange()', () => {
+		beforeEach( () => {
+			range = new Range( new Position( root, [ 1 ] ), new Position( root, [ 3 ] ) );
+		} );
 
 		it( 'should return true if ranges are equal and check is not strict', () => {
 			const otherRange = Range.createFromRange( range );
@@ -441,6 +447,32 @@ describe( 'Range', () => {
 		} );
 	} );
 
+	describe( 'containsItem()', () => {
+		let a, b, c, d, xxx;
+
+		beforeEach( () => {
+			a = new Element( 'a' );
+			b = new Element( 'b' );
+			c = new Element( 'c' );
+			d = new Element( 'd' );
+
+			xxx = new Text( 'xxx' );
+			b.appendChildren( xxx );
+
+			root.appendChildren( [ a, b, c, d ] );
+		} );
+
+		it( 'should return true if element is inside range and false when it is not inside range', () => {
+			const range = Range.createFromParentsAndOffsets( root, 1, root, 3 ); // Range over `b` and `c`.
+
+			expect( range.containsItem( a ) ).to.be.false;
+			expect( range.containsItem( b ) ).to.be.true;
+			expect( range.containsItem( xxx ) ).to.be.true;
+			expect( range.containsItem( c ) ).to.be.true;
+			expect( range.containsItem( d ) ).to.be.false;
+		} );
+	} );
+
 	describe( '_getTransformedByInsertion()', () => {
 		it( 'should return an array of Range objects', () => {
 			const range = new Range( new Position( root, [ 0 ] ), new Position( root, [ 1 ] ) );