Merge pull request #1109 from ckeditor/t/1108

Feature: Highlights on text nodes will be now unwrapped basing on descriptor id (which by default is marker name). Closes #1108.

Author: szymonkups

src/conversion/buildmodelconverter.js

@@ -59,7 +59,7 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
  *		buildModelConverter().for( dispatcher ).fromAttribute( 'bold' ).toElement( 'strong' );
  *
  * 4. Model marker to view highlight converter. This is a converter that converts model markers to view highlight
- * described by {@link module:engine/conversion/buildmodelconverter~HighlightDescriptor} object passed to
+ * described by {@link module:engine/conversion/model-to-view-converters~HighlightDescriptor} object passed to
  * {@link module:engine/conversion/buildmodelconverter~ModelConverterBuilder#toHighlight} method.
  *
  *		buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toHighlight( {
@@ -209,9 +209,9 @@ class ModelConverterBuilder {
 	 * from element, {@link module:engine/view/attributeelement~AttributeElement ViewAttributeElement} if you convert
 	 * from attribute and {@link module:engine/view/uielement~UIElement ViewUIElement} if you convert from marker.
 	 *
-	 * NOTE: When converting from model's marker, separate elements will be created at the beginning and at the end of the
+	 * **Note:** When converting from model's marker, separate elements will be created at the beginning and at the end of the
 	 * marker's range. If range is collapsed then only one element will be created. See how markers
-	 * {module:engine/model/buildviewconverter~ViewConverterBuilder#toMarker view -> model serialization}
+	 * {module:engine/model/buildviewconverter~ViewConverterBuilder#toMarker serialization from view to model}
 	 * works to find out what view element format is the best for you.
 	 *
 	 *		buildModelConverter().for( dispatcher ).fromElement( 'paragraph' ).toElement( 'p' );
@@ -286,10 +286,11 @@ class ModelConverterBuilder {
 	 *		viewElement.setCustomProperty( 'addHighlight', ( element, descriptor ) => {} );
 	 *		viewElement.setCustomProperty( 'removeHighlight', ( element, descriptor ) => {} );
 	 *
-	 * {@link module:engine/conversion/buildmodelconverter~HighlightDescriptor} will be used to create
+	 * {@link module:engine/conversion/model-to-view-converters~HighlightDescriptor} will be used to create
 	 * spans over text nodes and also will be provided to `addHighlight` and `removeHighlight` methods
 	 * each time highlight should be set or removed from view elements.
-	 * NOTE: When `addHighlight` and `removeHighlight` custom properties are present, converter assumes
+	 *
+	 * **Note:** When `addHighlight` and `removeHighlight` custom properties are present, converter assumes
 	 * that element itself is taking care of presenting highlight on its child nodes, so it won't convert them.
 	 *
 	 * Highlight descriptor can be provided as plain object:
@@ -307,7 +308,7 @@ class ModelConverterBuilder {
 	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError}
 	 * `build-model-converter-non-marker-to-highlight` when trying to convert not from marker.
 	 *
-	 * @param {function|module:engine/conversion/buildmodelconverter~HighlightDescriptor} highlightDescriptor
+	 * @param {function|module:engine/conversion/model-to-view-converters~HighlightDescriptor} highlightDescriptor
 	 */
 	toHighlight( highlightDescriptor ) {
 		const priority = this._from.priority === null ? 'normal' : this._from.priority;
@@ -423,28 +424,9 @@ export default function buildModelConverter() {
 }
 
 /**
- * @typedef MarkerViewElementCreatorData
- * @param {Object} data Additional information about the change.
- * @param {String} data.markerName Marker name.
- * @param {module:engine/model/range~Range} data.markerRange Marker range.
- * @param {Boolean} data.isOpening Defines if currently converted element is a beginning or end of the marker range.
- * @param {module:engine/conversion/modelconsumable~ModelConsumable} consumable Values to consume.
- * @param {Object} conversionApi Conversion interface to be used by callback, passed in `ModelConversionDispatcher` constructor.
- */
-
-/**
- * @typedef HighlightDescriptor
- * Object describing how content highlight should be created in the view. Each text node contained in highlight
- * will be wrapped with `span` element with CSS class, attributes and priority described by this object. Each element
- * can handle displaying highlight separately by providing `addHighlight` and `removeHighlight` custom
- * properties.
+ * @typedef {Object} module:engine/conversion/buildmodelconverter~MarkerViewElementCreatorData
  *
- * @property {String|Array.<String>} class CSS class or array of classes that will be added to `span`
- * {@link module:engine/view/attributeelement~AttributeElement} wrapping each text node in the highlighted content.
- * @property {String} [id] Descriptor identifier. If not provided, marker's name from which given highlight is created
- * will be used.
- * @property {Number} [priority] {@link module:engine/view/attributeelement~AttributeElement#priority} of the `span`
- * wrapping each text node in the highlighted content. If not provided, default 10 priority will be used.
- * @property {Object} [attributes] Attributes that will be added to `span`
- * {@link module:engine/view/attributeelement~AttributeElement} wrapping each text node it the highlighted content.
+ * @param {String} markerName Marker name.
+ * @param {module:engine/model/range~Range} markerRange Marker range.
+ * @param {Boolean} isOpening Defines if currently converted element is a beginning or end of the marker range.
  */

src/conversion/model-selection-to-view-converters.js

@@ -6,7 +6,7 @@
 import ViewElement from '../view/element';
 import ViewRange from '../view/range';
 import viewWriter from '../view/writer';
-import { highlightDescriptorToAttributeElement } from './model-to-view-converters';
+import { createViewElementFromHighlightDescriptor } from './model-to-view-converters';
 
 /**
  * Contains {@link module:engine/model/selection~Selection model selection} to
@@ -162,7 +162,7 @@ export function convertSelectionAttribute( elementCreator ) {
  *		modelDispatcher.on( 'selectionMarker:searchResult', convertSelectionMarker( { class: 'search' } ) );
  *
  * @see module:engine/conversion/model-selection-to-view-converters~convertSelectionAttribute
- * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor|Function} highlightDescriptor Highlight
+ * @param {module:engine/conversion/model-to-view-converters~HighlightDescriptor|Function} highlightDescriptor Highlight
  * descriptor object or function returning a descriptor object.
  * @returns {Function} Selection converter.
  */
@@ -176,7 +176,7 @@ export function convertSelectionMarker( highlightDescriptor ) {
 			return;
 		}
 
-		const viewElement = highlightDescriptorToAttributeElement( descriptor );
+		const viewElement = createViewElementFromHighlightDescriptor( descriptor );
 		const consumableName = 'selectionMarker:' + data.markerName;
 
 		wrapCollapsedSelectionPosition( data.selection, conversionApi.viewSelection, viewElement, consumable, consumableName );

src/conversion/model-to-view-converters.js

@@ -409,7 +409,7 @@ export function remove() {
  * {@link module:engine/view/attributeelement~AttributeElement} created from provided descriptor.
  * See {link module:engine/conversion/model-to-view-converters~highlightDescriptorToAttributeElement}.
  *
- * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor|Function} highlightDescriptor
+ * @param {module:engine/conversion/model-to-view-converters~HighlightDescriptor|Function} highlightDescriptor
  * @return {Function}
  */
 export function highlightText( highlightDescriptor ) {
@@ -428,7 +428,11 @@ export function highlightText( highlightDescriptor ) {
 			return;
 		}
 
-		const viewElement = highlightDescriptorToAttributeElement( descriptor );
+		if ( !descriptor.id ) {
+			descriptor.id = data.markerName;
+		}
+
+		const viewElement = createViewElementFromHighlightDescriptor( descriptor );
 		const viewRange = conversionApi.mapper.toViewRange( data.range );
 
 		if ( evt.name.split( ':' )[ 0 ] == 'addMarker' ) {
@@ -442,16 +446,17 @@ export function highlightText( highlightDescriptor ) {
 /**
  * Function factory, creates converter that converts all elements inside marker's range. Converter checks if element has
  * functions stored under `addHighlight` and `removeHighlight` custom properties and calls them passing
- * {@link module:engine/conversion/buildmodelconverter~HighlightDescriptor}. In such case converter will consume
+ * {@link module:engine/conversion/model-to-view-converters~HighlightDescriptor}. In such case converter will consume
  * all element's children, assuming that they were handled by element itself. If highlight descriptor will not provide
- * priority, priority 10 will be used as default, to be compliant with
+ * priority, priority `10` will be used as default, to be compliant with
  * {@link module:engine/conversion/model-to-view-converters~highlightText} method which uses default priority of
  * {@link module:engine/view/attributeelement~AttributeElement}.
- * If highlight descriptor will not provide id property, name of the marker will be used.
+ *
+ * If highlight descriptor will not provide `id` property, name of the marker will be used.
  * When `addHighlight` and `removeHighlight` custom properties are not present, element is not converted
  * in any special way. This means that converters will proceed to convert element's child nodes.
  *
- * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor|Function} highlightDescriptor
+ * @param {module:engine/conversion/model-to-view-converters~HighlightDescriptor|Function} highlightDescriptor
  * @return {Function}
  */
 export function highlightElement( highlightDescriptor ) {
@@ -562,29 +567,6 @@ export function eventNameToConsumableType( evtName ) {
 	return parts[ 0 ] + ':' + parts[ 1 ];
 }
 
-/**
- * Creates `span` {@link module:engine/view/attributeelement~AttributeElement view attribute element} from information
- * provided by {@link module:engine/conversion/buildmodelconverter~HighlightDescriptor} object. If priority
- * is not provided in descriptor - default priority will be used.
- *
- * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor } descriptor
- * @return {module:engine/view/attributeelement~AttributeElement}
- */
-export function highlightDescriptorToAttributeElement( descriptor ) {
-	const attributeElement = new ViewAttributeElement( 'span', descriptor.attributes );
-
-	if ( descriptor.class ) {
-		const cssClasses = Array.isArray( descriptor.class ) ? descriptor.class : [ descriptor.class ];
-		attributeElement.addClass( ...cssClasses );
-	}
-
-	if ( descriptor.priority ) {
-		attributeElement.priority = descriptor.priority;
-	}
-
-	return attributeElement;
-}
-
 // Helper function that shifts given view `position` in a way that returned position is after `howMany` characters compared
 // to the original `position`.
 // Because in view there might be view ui elements splitting text nodes, we cannot simply use `ViewPosition#getShiftedBy()`.
@@ -604,3 +586,79 @@ function _shiftViewPositionByCharacters( position, howMany ) {
 		}
 	}
 }
+
+/**
+ * Creates `span` {@link module:engine/view/attributeelement~AttributeElement view attribute element} from information
+ * provided by {@link module:engine/conversion/model-to-view-converters~HighlightDescriptor} object. If priority
+ * is not provided in descriptor - default priority will be used.
+ *
+ * @param {module:engine/conversion/model-to-view-converters~HighlightDescriptor} descriptor
+ * @return {module:engine/conversion/model-to-view-converters~HighlightAttributeElement}
+ */
+export function createViewElementFromHighlightDescriptor( descriptor ) {
+	const viewElement = new HighlightAttributeElement( 'span', descriptor.attributes );
+
+	if ( descriptor.class ) {
+		const cssClasses = Array.isArray( descriptor.class ) ? descriptor.class : [ descriptor.class ];
+		viewElement.addClass( ...cssClasses );
+	}
+
+	if ( descriptor.priority ) {
+		viewElement.priority = descriptor.priority;
+	}
+
+	viewElement.setCustomProperty( 'highlightDescriptorId', descriptor.id );
+
+	return viewElement;
+}
+
+/**
+ * Special kind of {@link module:engine/view/attributeelement~AttributeElement} that is created and used in
+ * marker-to-highlight conversion.
+ *
+ * The difference between `HighlightAttributeElement` and {@link module:engine/view/attributeelement~AttributeElement}
+ * is {@link module:engine/view/attributeelement~AttributeElement#isSimilar} method.
+ *
+ * For `HighlightAttributeElement` it checks just `highlightDescriptorId` custom property, that is set during marker-to-highlight
+ * conversion basing on {@link module:engine/conversion/model-to-view-converters~HighlightDescriptor} object.
+ * `HighlightAttributeElement`s with same `highlightDescriptorId` property are considered similar.
+ */
+class HighlightAttributeElement extends ViewAttributeElement {
+	isSimilar( otherElement ) {
+		if ( otherElement.is( 'attributeElement' ) ) {
+			return this.getCustomProperty( 'highlightDescriptorId' ) === otherElement.getCustomProperty( 'highlightDescriptorId' );
+		}
+
+		return false;
+	}
+}
+
+/**
+ * Object describing how content highlight should be created in the view.
+ *
+ * Each text node contained in highlight will be wrapped with `span` element with CSS class(es), attributes and priority
+ * described by this object.
+ *
+ * Each element can handle displaying highlight separately by providing `addHighlight` and `removeHighlight` custom
+ * properties. Those properties are passed `HighlightDescriptor` object upon conversion and should use it to
+ * change the element.
+ *
+ * @typedef {Object} module:engine/conversion/model-to-view-converters~HighlightDescriptor
+ *
+ * @property {String|Array.<String>} class CSS class or array of classes to set. If descriptor is used to
+ * create {@link module:engine/view/attributeelement~AttributeElement} over text nodes, those classes will be set
+ * on that {@link module:engine/view/attributeelement~AttributeElement}. If descriptor is applied to an element,
+ * usually those class will be set on that element, however this depends on how the element converts the descriptor.
+ *
+ * @property {String} [id] Descriptor identifier. If not provided, defaults to converted marker's name.
+ *
+ * @property {Number} [priority] Descriptor priority. If not provided, defaults to `10`. If descriptor is used to create
+ * {@link module:engine/view/attributeelement~AttributeElement}, it will be that element's
+ * {@link module:engine/view/attributeelement~AttributeElement#priority}. If descriptor is applied to an element,
+ * the priority will be used to determine which descriptor is more important.
+ *
+ * @property {Object} [attributes] Attributes to set. If descriptor is used to create
+ * {@link module:engine/view/attributeelement~AttributeElement} over text nodes, those attributes will be set on that
+ * {@link module:engine/view/attributeelement~AttributeElement}. If descriptor is applied to an element, usually those
+ * attributes will be set on that element, however this depends on how the element converts the descriptor.
+ */

tests/conversion/model-to-view-converters.js

@@ -30,7 +30,7 @@ import {
 	removeUIElement,
 	highlightText,
 	highlightElement,
-	highlightDescriptorToAttributeElement
+	createViewElementFromHighlightDescriptor
 } from '../../src/conversion/model-to-view-converters';
 
 import { createRangeOnElementOnly } from '../../tests/model/_utils/utils';
@@ -1224,14 +1224,14 @@ describe( 'model-to-view-converters', () => {
 		} );
 	} );
 
-	describe( 'highlightDescriptorToAttributeElement()', () => {
+	describe( 'createViewElementFromHighlightDescriptor()', () => {
 		it( 'should return attribute element from descriptor object', () => {
 			const descriptor = {
 				class: 'foo-class',
 				attributes: { one: 1, two: 2 },
 				priority: 7,
 			};
-			const element = highlightDescriptorToAttributeElement( descriptor );
+			const element = createViewElementFromHighlightDescriptor( descriptor );
 
 			expect( element.is( 'attributeElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'span' );
@@ -1249,7 +1249,7 @@ describe( 'model-to-view-converters', () => {
 				attributes: { one: 1, two: 2 },
 				priority: 7,
 			};
-			const element = highlightDescriptorToAttributeElement( descriptor );
+			const element = createViewElementFromHighlightDescriptor( descriptor );
 
 			expect( element.is( 'attributeElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'span' );
@@ -1267,7 +1267,7 @@ describe( 'model-to-view-converters', () => {
 				attributes: { one: 1, two: 2 },
 				priority: 7,
 			};
-			const element = highlightDescriptorToAttributeElement( descriptor );
+			const element = createViewElementFromHighlightDescriptor( descriptor );
 
 			expect( element.is( 'attributeElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'span' );
@@ -1283,7 +1283,7 @@ describe( 'model-to-view-converters', () => {
 				class: 'foo-class',
 				attributes: { one: 1, two: 2 },
 			};
-			const element = highlightDescriptorToAttributeElement( descriptor );
+			const element = createViewElementFromHighlightDescriptor( descriptor );
 
 			expect( element.is( 'attributeElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'span' );
@@ -1300,12 +1300,44 @@ describe( 'model-to-view-converters', () => {
 				class: 'foo-class',
 				priority: 7
 			};
-			const element = highlightDescriptorToAttributeElement( descriptor );
+			const element = createViewElementFromHighlightDescriptor( descriptor );
 
 			expect( element.is( 'attributeElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'span' );
 			expect( element.priority ).to.equal( 7 );
 			expect( element.hasClass( 'foo-class' ) ).to.be.true;
 		} );
+
+		it( 'should create similar elements if they are created using same descriptor id', () => {
+			const a = createViewElementFromHighlightDescriptor( {
+				id: 'id',
+				class: 'classA',
+				priority: 1
+			} );
+
+			const b = createViewElementFromHighlightDescriptor( {
+				id: 'id',
+				class: 'classB',
+				priority: 2
+			} );
+
+			expect( a.isSimilar( b ) ).to.be.true;
+		} );
+
+		it( 'should create non-similar elements if they have different descriptor id', () => {
+			const a = createViewElementFromHighlightDescriptor( {
+				id: 'a',
+				class: 'foo',
+				priority: 1
+			} );
+
+			const b = createViewElementFromHighlightDescriptor( {
+				id: 'b',
+				class: 'foo',
+				priority: 1
+			} );
+
+			expect( a.isSimilar( b ) ).to.be.false;
+		} );
 	} );
 } );