Procházet zdrojové kódy

Changed: Move markers removal conversion from `controller.EditingController` to `conversion.DowncastDispatcher`.
Changed: Use markers binding provided by `conversion.Mapper` in `conversion.downcast-converters`.

Szymon Cofalik před 7 roky
rodič
revize
4e55a7a22d

+ 7 - 75
packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -9,13 +9,9 @@
 
 import RootEditableElement from '../view/rooteditableelement';
 import View from '../view/view';
-import ViewWriter from '../view/writer';
 import Mapper from '../conversion/mapper';
 import DowncastDispatcher from '../conversion/downcastdispatcher';
-import {
-	insertText,
-	remove
-} from '../conversion/downcast-converters';
+import { insertText, remove } from '../conversion/downcast-converters';
 import { convertSelectionChange } from '../conversion/upcast-selection-converters';
 import {
 	convertRangeSelection,
@@ -28,7 +24,7 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';
 
 /**
  * Controller for the editing pipeline. The editing pipeline controls {@link ~EditingController#model model} rendering,
- * including selection handling. It also creates the {@link ~EditingController#view view document} which builds a
+ * including selection handling. It also creates the {@link ~EditingController#view view} which builds a
  * browser-independent virtualization over the DOM elements. The editing controller also attaches default converters.
  *
  * @mixes module:utils/observablemixin~ObservableMixin
@@ -41,7 +37,7 @@ export default class EditingController {
 	 */
 	constructor( model ) {
 		/**
-		 * Editing model.
+		 * Editor model.
 		 *
 		 * @readonly
 		 * @member {module:engine/model/model~Model}
@@ -78,6 +74,9 @@ export default class EditingController {
 		const selection = doc.selection;
 		const markers = this.model.markers;
 
+		// Whenever model document is changed, convert those changes to the view (using model.Document#differ).
+		// Do it on 'low' priority, so changes are converted after other listeners did their job.
+		// Also convert model selection.
 		this.listenTo( doc, 'change', () => {
 			this.view.change( writer => {
 				this.downcastDispatcher.convertChanges( doc.differ, writer );
@@ -85,7 +84,7 @@ export default class EditingController {
 			} );
 		}, { priority: 'low' } );
 
-		// Convert selection from view to model.
+		// Convert selection from the view to the model when it changes in the view.
 		this.listenTo( this.view.document, 'selectionChange', convertSelectionChange( this.model, this.mapper ) );
 
 		// Attach default model converters.
@@ -97,53 +96,6 @@ export default class EditingController {
 		this.downcastDispatcher.on( 'selection', convertRangeSelection(), { priority: 'low' } );
 		this.downcastDispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );
 
-		// Convert markers removal.
-		//
-		// Markers should be removed from the view before changes to the model are applied. This is because otherwise
-		// it would be impossible to map some markers to the view (if, for example, the marker's boundary parent got removed).
-		//
-		// `removedMarkers` keeps information which markers already has been removed to prevent removing them twice.
-		const removedMarkers = new Set();
-
-		// We don't want to render view when markers are converted, so we need to create view writer
-		// manually instead of using `View#change` block. See https://github.com/ckeditor/ckeditor5-engine/issues/1323.
-		const viewWriter = new ViewWriter( this.view.document );
-
-		this.listenTo( model, 'applyOperation', ( evt, args ) => {
-			// Before operation is applied...
-			const operation = args[ 0 ];
-
-			for ( const marker of model.markers ) {
-				// Check all markers, that aren't already removed...
-				if ( removedMarkers.has( marker.name ) ) {
-					continue;
-				}
-
-				const markerRange = marker.getRange();
-
-				if ( _operationAffectsMarker( operation, marker ) ) {
-					// And if the operation in any way modifies the marker, remove the marker from the view.
-					removedMarkers.add( marker.name );
-					this.downcastDispatcher.convertMarkerRemove( marker.name, markerRange, viewWriter );
-					// TODO: This stinks but this is the safest place to have this code.
-					this.model.document.differ.bufferMarkerChange( marker.name, markerRange, markerRange );
-				}
-			}
-		}, { priority: 'high' } );
-
-		// If an existing marker is updated through `model.Model#markers` directly (not through operation), just remove it.
-		this.listenTo( model.markers, 'update', ( evt, marker, oldRange ) => {
-			if ( oldRange && !removedMarkers.has( marker.name ) ) {
-				removedMarkers.add( marker.name );
-				this.downcastDispatcher.convertMarkerRemove( marker.name, oldRange, viewWriter );
-			}
-		} );
-
-		// When all changes are done, clear `removedMarkers` set.
-		this.listenTo( model, '_change', () => {
-			removedMarkers.clear();
-		}, { priority: 'low' } );
-
 		// Binds {@link module:engine/view/document~Document#roots view roots collection} to
 		// {@link module:engine/model/document~Document#roots model roots collection} so creating
 		// model root automatically creates corresponding view root.
@@ -174,23 +126,3 @@ export default class EditingController {
 }
 
 mix( EditingController, ObservableMixin );
-
-// Helper function which checks whether given operation will affect given marker after the operation is applied.
-function _operationAffectsMarker( operation, marker ) {
-	const range = marker.getRange();
-
-	if ( operation.type == 'insert' || operation.type == 'rename' ) {
-		return _positionAffectsRange( operation.position, range );
-	} else if ( operation.type == 'move' || operation.type == 'remove' || operation.type == 'reinsert' ) {
-		return _positionAffectsRange( operation.targetPosition, range ) || _positionAffectsRange( operation.sourcePosition, range );
-	} else if ( operation.type == 'marker' && operation.name == marker.name ) {
-		return true;
-	}
-
-	return false;
-}
-
-// Helper function which checks whether change at given position affects given range.
-function _positionAffectsRange( position, range ) {
-	return range.containsPosition( position ) || !range.start._getTransformedByInsertion( position, 1, true ).isEqual( range.start );
-}

+ 67 - 75
packages/ckeditor5-engine/src/conversion/downcast-converters.js

@@ -517,15 +517,16 @@ export function remove() {
 
 /**
  * Function factory, creates a converter that converts marker adding change to the view ui element.
- * The view ui element that will be added to the view depends on passed parameter. See {@link ~insertElement}.
- * In a case of collapsed range element will not wrap range but separate elements will be placed at the beginning
+ *
+ * The view ui element, that will be added to the view, depends on passed parameter. See {@link ~insertElement}.
+ * In a case of a non-collapsed range, the ui element will not wrap nodes but separate elements will be placed at the beginning
  * and at the end of the range.
  *
- * **Note:** unlike {@link ~insertElement}, the converter does not bind view element to model, because this converter
- * uses marker as "model source of data". This means that view ui element does not have corresponding model element.
+ * This converter binds created {@link module:engine/view/uielement~UIElement}s with marker name using
+ * the {@link module:engine/conversion/mapper~Mapper#bindElementToMarker}.
  *
- * @param {module:engine/view/uielement~UIElement|Function} elementCreator View ui element, or function returning a view element, which
- * will be inserted.
+ * @param {module:engine/view/uielement~UIElement|Function} elementCreator View ui element or a function returning a view element
+ * which will be inserted.
  * @returns {Function} Insert element event converter.
  */
 export function insertUIElement( elementCreator ) {
@@ -563,10 +564,12 @@ export function insertUIElement( elementCreator ) {
 
 		// Add "opening" element.
 		viewWriter.insert( mapper.toViewPosition( markerRange.start ), viewStartElement );
+		conversionApi.mapper.bindElementToMarker( viewStartElement, data.markerName );
 
 		// Add "closing" element only if range is not collapsed.
 		if ( !markerRange.isCollapsed ) {
 			viewWriter.insert( mapper.toViewPosition( markerRange.end ), viewEndElement );
+			conversionApi.mapper.bindElementToMarker( viewEndElement, data.markerName );
 		}
 
 		evt.stop();
@@ -574,39 +577,29 @@ export function insertUIElement( elementCreator ) {
 }
 
 /**
- * Function factory, creates a default downcast converter for removing {@link module:engine/view/uielement~UIElement ui element}
+ * Function factory, returns a default downcast converter for removing {@link module:engine/view/uielement~UIElement ui element}
  * basing on marker remove change.
  *
- * @param {module:engine/view/uielement~UIElement|Function} elementCreator View ui element, or function returning
- * a view ui element, which will be used as a pattern when look for element to remove at the marker start position.
+ * This converter unbinds elements from marker name.
+ *
  * @returns {Function} Remove ui element converter.
  */
-export function removeUIElement( elementCreator ) {
+export function removeUIElement() {
 	return ( evt, data, conversionApi ) => {
-		// Create two view elements. One will be used to remove "opening element", the other for "closing element".
-		// If marker was collapsed, only "opening" element will be removed.
-		data.isOpening = true;
-		const viewStartElement = elementCreator( data, conversionApi.writer );
-
-		data.isOpening = false;
-		const viewEndElement = elementCreator( data, conversionApi.writer );
+		const elements = conversionApi.mapper.markerNameToElements( data.markerName );
 
-		if ( !viewStartElement || !viewEndElement ) {
+		if ( !elements ) {
 			return;
 		}
 
-		const markerRange = data.markerRange;
+		conversionApi.mapper.unbindElementsFromMarkerName( data.markerName );
+
 		const viewWriter = conversionApi.writer;
 
-		// When removing the ui elements, we map the model range to view twice, because that view range
-		// may change after the first clearing.
-		if ( !markerRange.isCollapsed ) {
-			viewWriter.clear( conversionApi.mapper.toViewRange( markerRange ).getEnlarged(), viewEndElement );
+		for ( const element of elements ) {
+			viewWriter.clear( ViewRange.createOn( element ), element );
 		}
 
-		// Remove "opening" element.
-		viewWriter.clear( conversionApi.mapper.toViewRange( markerRange ).getEnlarged(), viewStartElement );
-
 		evt.stop();
 	};
 }
@@ -778,6 +771,9 @@ export function wrap( elementCreator ) {
  *
  * If the highlight descriptor will not provide `id` property, name of the marker will be used.
  *
+ * This converter binds created {@link module:engine/view/attributeelement~AttributeElement}s with marker name using
+ * the {@link module:engine/conversion/mapper~Mapper#bindElementToMarker}.
+ *
  * @param {module:engine/conversion/downcast-converters~HighlightDescriptor|Function} highlightDescriptor
  * @return {Function}
  */
@@ -809,7 +805,13 @@ export function highlightText( highlightDescriptor ) {
 			viewWriter.wrap( viewSelection.getFirstRange(), viewElement, viewSelection );
 		} else {
 			const viewRange = conversionApi.mapper.toViewRange( data.range );
-			viewWriter.wrap( viewRange, viewElement );
+			const rangeAfterWrap = viewWriter.wrap( viewRange, viewElement );
+
+			for ( const element of rangeAfterWrap.getItems() ) {
+				if ( element.is( 'attributeElement' ) && element.isSimilar( viewElement ) ) {
+					conversionApi.mapper.bindElementToMarker( element, data.markerName );
+				}
+			}
 		}
 	};
 }
@@ -828,6 +830,9 @@ export function highlightText( highlightDescriptor ) {
  *
  * If the highlight descriptor will not provide `id` property, name of the marker will be used.
  *
+ * This converter binds altered {@link module:engine/view/containerelement~ContainerElement}s with marker name using
+ * the {@link module:engine/conversion/mapper~Mapper#bindElementToMarker}.
+ *
  * @param {module:engine/conversion/downcast-converters~HighlightDescriptor|Function} highlightDescriptor
  * @return {Function}
  */
@@ -863,14 +868,16 @@ export function highlightElement( highlightDescriptor ) {
 			}
 
 			viewElement.getCustomProperty( 'addHighlight' )( viewElement, descriptor, conversionApi.writer );
+
+			conversionApi.mapper.bindElementToMarker( viewElement, data.markerName );
 		}
 	};
 }
 
 /**
- * Function factory, creates a converter that converts model marker remove to the view.
+ * Function factory, creates a converter that converts removing model marker to the view.
  *
- * Both text nodes and elements are handled by this converter by they are handled a bit differently.
+ * Both text nodes and elements are handled by this converter but they are handled a bit differently.
  *
  * Text nodes are unwrapped using {@link module:engine/view/attributeelement~AttributeElement} created from provided
  * highlight descriptor. See {link module:engine/conversion/downcast-converters~highlightDescriptorToAttributeElement}.
@@ -886,6 +893,8 @@ export function highlightElement( highlightDescriptor ) {
  *
  * If the highlight descriptor will not provide `id` property, name of the marker will be used.
  *
+ * This converter unbinds elements from marker name.
+ *
  * @param {module:engine/conversion/downcast-converters~HighlightDescriptor|Function} highlightDescriptor
  * @return {Function}
  */
@@ -902,33 +911,37 @@ export function removeHighlight( highlightDescriptor ) {
 			return;
 		}
 
-		const viewRange = conversionApi.mapper.toViewRange( data.markerRange );
-
-		// Retrieve all items in the affected range. We will process them and remove highlight from them appropriately.
-		const items = new Set( viewRange.getItems() );
-
-		// First, iterate through all items and remove highlight from those container elements that have custom highlight handling.
-		for ( const item of items ) {
-			if ( item.is( 'containerElement' ) && item.getCustomProperty( 'removeHighlight' ) ) {
-				item.getCustomProperty( 'removeHighlight' )( item, descriptor.id, conversionApi.writer );
+		// View element that will be used to unwrap `AttributeElement`s.
+		const viewHighlightElement = createViewElementFromHighlightDescriptor( descriptor );
 
-				// If container element had custom handling, remove all it's children from further processing.
-				for ( const descendant of ViewRange.createIn( item ) ) {
-					items.delete( descendant );
+		// Get all elements bound with given marker name.
+		const elements = conversionApi.mapper.markerNameToElements( data.markerName );
+		conversionApi.mapper.unbindElementsFromMarkerName( data.markerName );
+
+		for ( const element of elements ) {
+			if ( element.is( 'attributeElement' ) ) {
+				// If the bound element is an `AttributeElement`, get all other `AttributeElement`s that were created "from it"
+				// (when view.Writer broke it when handling other `AttributeElement`s).
+				const allAttributeElements = conversionApi.writer.getAllBrokenSiblings( element );
+
+				// Handle all those elements.
+				for ( const attributeElement of allAttributeElements ) {
+					// Filter out elements which got removed. For example, when converting from model to view,
+					// converter might have created two `AttributeElement`s, split by some other element (for
+					// example another marker). Then, that splitting element might got removed and the marker parts
+					// might got merged. In this case, previously bound element might got removed and now has to be filtered.
+					if ( attributeElement.parent ) {
+						// If the element is still in the tree, unwrap it.
+						conversionApi.writer.unwrap( ViewRange.createOn( attributeElement ), viewHighlightElement );
+					}
 				}
+			} else {
+				// If the bound element is a ContainerElement, just use `removeHighlight` function on it.
+				element.getCustomProperty( 'removeHighlight' )( element, descriptor.id, conversionApi.writer );
 			}
 		}
 
-		// Then, iterate through all other items. Look for text nodes and unwrap them. Start from the end
-		// to prevent errors when view structure changes when unwrapping (and, for example, some attributes are merged).
-		const viewHighlightElement = createViewElementFromHighlightDescriptor( descriptor );
-		const viewWriter = conversionApi.writer;
-
-		for ( const item of Array.from( items ).reverse() ) {
-			if ( item.is( 'textProxy' ) ) {
-				viewWriter.unwrap( ViewRange.createOn( item ), viewHighlightElement );
-			}
-		}
+		evt.stop();
 	};
 }
 
@@ -962,10 +975,10 @@ function _prepareDescriptor( highlightDescriptor, data, conversionApi ) {
  * is not provided in descriptor - default priority will be used.
  *
  * @param {module:engine/conversion/downcast-converters~HighlightDescriptor} descriptor
- * @return {module:engine/conversion/downcast-converters~HighlightAttributeElement}
+ * @returns {module:engine/view/attributeelement~AttributeElement}
  */
 export function createViewElementFromHighlightDescriptor( descriptor ) {
-	const viewElement = new HighlightAttributeElement( 'span', descriptor.attributes );
+	const viewElement = new ViewAttributeElement( 'span', descriptor.attributes );
 
 	if ( descriptor.class ) {
 		viewElement._addClass( descriptor.class );
@@ -975,33 +988,12 @@ export function createViewElementFromHighlightDescriptor( descriptor ) {
 		viewElement._priority = descriptor.priority;
 	}
 
-	viewElement._setCustomProperty( 'highlightDescriptorId', descriptor.id );
+	viewElement._id = 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/downcast-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 the marker highlight should be represented in the view.
  *
  * Each text node contained in a highlighted range will be wrapped in a `span` {@link module:engine/view/attributeelement~AttributeElement}

+ 6 - 1
packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

@@ -123,6 +123,11 @@ export default class DowncastDispatcher {
 	 * @param {module:engine/view/writer~Writer} writer View writer that should be used to modify view document.
 	 */
 	convertChanges( differ, writer ) {
+		// Before the view is updated, remove markers which have changed.
+		for ( const change of differ.getMarkersToRemove() ) {
+			this.convertMarkerRemove( change.name, change.range, writer );
+		}
+
 		// Convert changes that happened on model tree.
 		for ( const entry of differ.getChanges() ) {
 			if ( entry.type == 'insert' ) {
@@ -135,7 +140,7 @@ export default class DowncastDispatcher {
 			}
 		}
 
-		// After the view is updated, convert markers which has changed.
+		// After the view is updated, convert markers which have changed.
 		for ( const change of differ.getMarkersToAdd() ) {
 			this.convertMarkerAdd( change.name, change.range, writer );
 		}

+ 0 - 220
packages/ckeditor5-engine/tests/controller/editingcontroller.js

@@ -341,226 +341,6 @@ describe( 'EditingController', () => {
 		} );
 	} );
 
-	describe( 'marker clearing', () => {
-		let model, modelRoot, editing, domRoot, mcd, p1;
-
-		beforeEach( () => {
-			model = new Model();
-			modelRoot = model.document.createRoot();
-
-			editing = new EditingController( model );
-
-			domRoot = document.createElement( 'div' );
-			domRoot.contentEditable = true;
-
-			document.body.appendChild( domRoot );
-
-			model.schema.register( 'paragraph', { inheritAllFrom: '$block' } );
-			model.schema.register( 'div', { inheritAllFrom: '$block' } );
-
-			downcastElementToElement( { model: 'paragraph', view: 'p' } )( editing.downcastDispatcher );
-			downcastElementToElement( { model: 'div', view: 'div' } )( editing.downcastDispatcher );
-			downcastMarkerToHighlight( { model: 'marker', view: {} } )( editing.downcastDispatcher );
-
-			const modelData = new ModelDocumentFragment( parse(
-				'<paragraph>foo</paragraph>' +
-				'<paragraph>bar</paragraph>',
-				model.schema
-			)._children );
-
-			model.change( writer => {
-				writer.insert( modelData, modelRoot );
-				p1 = modelRoot.getChild( 0 );
-
-				writer.setSelection( ModelRange.createFromParentsAndOffsets( p1, 0, p1, 0 ) );
-			} );
-
-			mcd = editing.downcastDispatcher;
-			sinon.spy( mcd, 'convertMarkerRemove' );
-		} );
-
-		afterEach( () => {
-			document.body.removeChild( domRoot );
-			editing.destroy();
-		} );
-
-		it( 'should remove marker from view if it will be affected by insert operation', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
-			model.on( 'applyOperation', () => {
-				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
-				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-			}, { priority: 'high' } );
-
-			model.change( writer => {
-				writer.insertText( 'a', p1, 0 );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>af<span>o</span>o</p><p>bar</p>' );
-		} );
-
-		it( 'should remove marker from view if it will be affected by remove operation', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
-			model.on( 'applyOperation', () => {
-				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
-				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-			}, { priority: 'high' } );
-
-			model.change( writer => {
-				writer.remove( ModelRange.createFromParentsAndOffsets( p1, 0, p1, 1 ) );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p><span>o</span>o</p><p>bar</p>' );
-		} );
-
-		it( 'should remove marker from view if it will be affected by move operation', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
-			model.on( 'applyOperation', () => {
-				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
-				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-			}, { priority: 'high' } );
-
-			model.change( writer => {
-				const p2 = p1.nextSibling;
-
-				writer.move( ModelRange.createFromParentsAndOffsets( p2, 0, p2, 2 ), p1, 0 );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>baf<span>o</span>o</p><p>r</p>' );
-		} );
-
-		it( 'should remove marker from view if it will be affected by rename operation', () => {
-			model.change( writer => {
-				writer.setMarker(
-					'marker',
-					ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ),
-					{ usingOperation: true }
-				);
-			} );
-
-			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
-			model.on( 'applyOperation', () => {
-				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
-				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-			}, { priority: 'high' } );
-
-			model.change( writer => {
-				writer.rename( p1, 'div' );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<div><span>foo</span></div><p>bar</p>' );
-		} );
-
-		it( 'should remove marker from view if it will be affected by marker operation', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
-			model.on( 'applyOperation', () => {
-				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
-				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-			}, { priority: 'high' } );
-
-			model.change( writer => {
-				const p2 = p1.nextSibling;
-
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p2, 1, p2, 2 ), { usingOperation: true } );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>b<span>a</span>r</p>' );
-		} );
-
-		it( 'should remove marker from view if it is removed through marker collection', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
-			model.markers.on( 'remove:marker', () => {
-				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
-				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-			}, { priority: 'low' } );
-
-			model.change( writer => {
-				writer.removeMarker( 'marker' );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-		} );
-
-		it( 'should not remove marker if applied operation is an attribute operation', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
-			model.on( 'applyOperation', () => {
-				expect( mcd.convertMarkerRemove.calledOnce ).to.be.false;
-				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>f<span>o</span>o</p><p>bar</p>' );
-			}, { priority: 'high' } );
-
-			model.change( writer => {
-				writer.setAttribute( 'foo', 'bar', ModelRange.createFromParentsAndOffsets( p1, 0, p1, 2 ) );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>f<span>o</span>o</p><p>bar</p>' );
-		} );
-
-		it( 'should not crash if multiple operations affect a marker', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			model.change( writer => {
-				writer.insertText( 'a', p1, 0 );
-				writer.insertText( 'a', p1, 0 );
-				writer.insertText( 'a', p1, 0 );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>aaaf<span>o</span>o</p><p>bar</p>' );
-		} );
-
-		it( 'should not crash if marker is removed, added and removed #1', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			model.change( writer => {
-				writer.insertText( 'a', p1, 0 );
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 3, p1, 4 ), { usingOperation: true } );
-				writer.insertText( 'a', p1, 0 );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>aafo<span>o</span></p><p>bar</p>' );
-		} );
-
-		it( 'should not crash if marker is removed, added and removed #2', () => {
-			model.change( writer => {
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ), { usingOperation: true } );
-			} );
-
-			model.change( writer => {
-				writer.removeMarker( 'marker', { usingOperation: true } );
-				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 0, p1, 1 ), { usingOperation: true } );
-				writer.removeMarker( 'marker', { usingOperation: true } );
-			} );
-
-			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
-		} );
-	} );
-
 	describe( 'destroy()', () => {
 		it( 'should remove listenters', () => {
 			const model = new Model();

+ 113 - 32
packages/ckeditor5-engine/tests/conversion/downcast-converters.js

@@ -1279,6 +1279,119 @@ describe( 'downcast-converters', () => {
 
 				expect( viewToString( viewRoot ) ).to.equal( '<div><p>foo</p><p>bar</p></div>' );
 			} );
+
+			it( 'should correctly wrap and unwrap multiple, intersecting markers', () => {
+				const descriptorFoo = { class: 'foo' };
+				const descriptorBar = { class: 'bar' };
+				const descriptorXyz = { class: 'xyz' };
+
+				dispatcher.on( 'addMarker:markerFoo', highlightText( descriptorFoo ) );
+				dispatcher.on( 'addMarker:markerBar', highlightText( descriptorBar ) );
+				dispatcher.on( 'addMarker:markerXyz', highlightText( descriptorXyz ) );
+
+				dispatcher.on( 'removeMarker:markerFoo', removeHighlight( descriptorFoo ) );
+				dispatcher.on( 'removeMarker:markerBar', removeHighlight( descriptorBar ) );
+				dispatcher.on( 'removeMarker:markerXyz', removeHighlight( descriptorXyz ) );
+
+				const p1 = modelRoot.getChild( 0 );
+				const p2 = modelRoot.getChild( 1 );
+
+				model.change( writer => {
+					writer.setMarker( 'markerFoo', ModelRange.createFromParentsAndOffsets( p1, 0, p1, 3 ) );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal(
+					'<div>' +
+						'<p>' +
+							'<span class="foo">foo</span>' +
+						'</p>' +
+						'<p>bar</p>' +
+					'</div>'
+				);
+
+				model.change( writer => {
+					writer.setMarker( 'markerBar', ModelRange.createFromParentsAndOffsets( p1, 1, p2, 2 ) );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal(
+					'<div>' +
+						'<p>' +
+							'<span class="foo">f</span>' +
+							'<span class="bar">' +
+								'<span class="foo">oo</span>' +
+							'</span>' +
+						'</p>' +
+						'<p>' +
+							'<span class="bar">ba</span>' +
+							'r' +
+						'</p>' +
+					'</div>'
+				);
+
+				model.change( writer => {
+					writer.setMarker( 'markerXyz', ModelRange.createFromParentsAndOffsets( p1, 2, p2, 3 ) );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal(
+					'<div>' +
+						'<p>' +
+							'<span class="foo">f</span>' +
+							'<span class="bar">' +
+								'<span class="foo">' +
+									'o' +
+									'<span class="xyz">o</span>' +
+								'</span>' +
+							'</span>' +
+						'</p>' +
+						'<p>' +
+							'<span class="bar">' +
+								'<span class="xyz">ba</span>' +
+							'</span>' +
+							'<span class="xyz">r</span>' +
+						'</p>' +
+					'</div>'
+				);
+
+				model.change( writer => {
+					writer.removeMarker( 'markerBar' );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal(
+					'<div>' +
+						'<p>' +
+							'<span class="foo">' +
+								'fo' +
+								'<span class="xyz">o</span>' +
+							'</span>' +
+						'</p>' +
+						'<p>' +
+							'<span class="xyz">bar</span>' +
+						'</p>' +
+					'</div>'
+				);
+
+				model.change( writer => {
+					writer.removeMarker( 'markerFoo' );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal(
+					'<div>' +
+						'<p>' +
+							'fo' +
+							'<span class="xyz">o</span>' +
+						'</p>' +
+						'<p>' +
+							'<span class="xyz">bar</span>' +
+						'</p>' +
+					'</div>'
+				);
+
+				model.change( writer => {
+					writer.removeMarker( 'markerXyz' );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal( '<div><p>foo</p><p>bar</p></div>' );
+			} );
 		} );
 
 		describe( 'on element', () => {
@@ -1490,37 +1603,5 @@ describe( 'downcast-converters', () => {
 			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;
-		} );
 	} );
 } );