Merge pull request #1113 from ckeditor/t/1112

Fix: Fixed incorrect markers transformations and conversions. Closes #1112. Closes #1080. Closes #1079.

Author: szymonkups

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;
+}

src/model/range.js

@@ -117,6 +117,17 @@ export default class Range {
 		return containsStart && containsEnd;
 	}
 
+	/**
+	 * 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.
 	 *
@@ -494,7 +505,7 @@ export default class Range {
 			// ^<p>xx</p><w>{<p>a[b</p>}</w><p>c]d</p>   -->   <p>a[b</p><p>xx</p><w></w><p>c]d</p>  // Note <p>xx</p> inclusion.
 			// <w>{<p>a[b</p>}</w>^<p>c]d</p>            -->   <w></w><p>a[b</p><p>c]d</p>
 			if (
-				sourceRange.containsPosition( this.start ) &&
+				( sourceRange.containsPosition( this.start ) || sourceRange.start.isEqual( this.start ) ) &&
 				this.containsPosition( sourceRange.end ) &&
 				this.end.isAfter( targetPosition )
 			) {

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 ) );

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 ] ) );
@@ -954,6 +986,19 @@ describe( 'Range', () => {
 				expect( transformed[ 0 ].end.path ).to.deep.equal( [ 1, 1 ] );
 			} );
 
+			it( 'split at the beginning of multi-element range', () => {
+				range.start = new Position( root, [ 0, 4 ] );
+				range.end = new Position( root, [ 1, 2 ] );
+
+				const delta = getSplitDelta( new Position( root, [ 0, 4 ] ), new Element( 'p' ), 3, 1 );
+
+				const transformed = range.getTransformedByDelta( delta );
+
+				expect( transformed.length ).to.equal( 1 );
+				expect( transformed[ 0 ].start.path ).to.deep.equal( [ 1, 0 ] );
+				expect( transformed[ 0 ].end.path ).to.deep.equal( [ 2, 2 ] );
+			} );
+
 			it( 'split inside range which starts at the beginning of split element', () => {
 				range.start = new Position( root, [ 0, 0 ] );
 				range.end = new Position( root, [ 0, 4 ] );