From 027c73a5c7ead4efa745179578013b7e740cd221 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Wed, 7 Mar 2018 15:09:25 +0100
Subject: [PATCH 01/13] Added: Introduced `view.AttributeElement#id`.

---
 src/view/attributeelement.js   | 53 +++++++++++++++++++++++++++++-----
 tests/view/attributeelement.js | 26 +++++++++++++++++
 2 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/src/view/attributeelement.js b/src/view/attributeelement.js
index d1e622ce4..9deddd3b2 100644
--- a/src/view/attributeelement.js
+++ b/src/view/attributeelement.js
@@ -34,15 +34,22 @@ export default class AttributeElement extends Element {
 		super( name, attrs, children );
 
 		/**
-		 * Element priority. Attributes have to have the same priority to be
-		 * {@link module:engine/view/element~Element#isSimilar similar}. Setting different priorities on similar
- 		 * nodes may prevent merging, e.g. two `<abbr>` nodes next each other shouldn't be merged.
+		 * Element priority. Decides in what order elements are wrapped by {@link module:engine/view/writer~Writer}.
 		 *
 		 * @protected
 		 * @member {Number}
 		 */
 		this._priority = DEFAULT_PRIORITY;
 
+		/**
+		 * Element identifier. If set, it is used by {@link module:engine/view/element~Element#isSimilar},
+		 * and then two elements are considered similar if, and only if they have the same `_id`.
+		 *
+		 * @protected
+		 * @member {String|Number}
+		 */
+		this._id = null;
+
 		/**
 		 * Returns block {@link module:engine/view/filler filler} offset or `null` if block filler is not needed.
 		 *
@@ -53,15 +60,26 @@ export default class AttributeElement extends Element {
 	}
 
 	/**
-	 * Priority of this element.
+	 * Element priority. Decides in what order elements are wrapped by {@link module:engine/view/writer~Writer}.
 	 *
 	 * @readonly
-	 * @return {Number}
+	 * @returns {Number}
 	 */
 	get priority() {
 		return this._priority;
 	}
 
+	/**
+	 * Element identifier. If set, it is used by {@link module:engine/view/element~Element#isSimilar},
+	 * and then two elements are considered similar if, and only if they have the same `_id`.
+	 *
+	 * @readonly
+	 * @returns {String|Number}
+	 */
+	get id() {
+		return this._id;
+	}
+
 	/**
 	 * @inheritDoc
 	 */
@@ -75,13 +93,31 @@ export default class AttributeElement extends Element {
 
 	/**
 	 * Checks if this element is similar to other element.
-	 * Both elements should have the same name, attributes and priority to be considered as similar.
-	 * Two similar elements can contain different set of children nodes.
+	 *
+	 * If none of elements has set {@link module:engine/view/attributeelement~AttributeElement#id}, then both elements
+	 * should have the same name, attributes and priority to be considered as similar. Two similar elements can contain
+	 * different set of children nodes.
+	 *
+	 * If at least one element has {@link module:engine/view/attributeelement~AttributeElement#id} set, then both
+	 * elements have to have the same {@link module:engine/view/attributeelement~AttributeElement#id} value to be
+	 * considered similar.
+	 *
+	 * Similarity is important for {@link module:engine/view/writer~Writer}. For example:
+	 *
+	 * * two following similar elements can be merged together into one, longer element,
+	 * * {@link module:engine/view/writer~Writer#unwrap} checks similarity of passed element and processed element to
+	 * decide whether processed element should be unwrapped,
+	 * * etc.
 	 *
 	 * @param {module:engine/view/element~Element} otherElement
 	 * @returns {Boolean}
 	 */
 	isSimilar( otherElement ) {
+		// If any element has an `id` set, just compare the ids.
+		if ( this.id !== null || otherElement.id !== null ) {
+			return this.id === otherElement.id;
+		}
+
 		return super.isSimilar( otherElement ) && this.priority == otherElement.priority;
 	}
 
@@ -99,6 +135,9 @@ export default class AttributeElement extends Element {
 		// Clone priority too.
 		cloned._priority = this._priority;
 
+		// And id too.
+		cloned._id = this._id;
+
 		return cloned;
 	}
 }
diff --git a/tests/view/attributeelement.js b/tests/view/attributeelement.js
index a761dd9d4..f2aff5b77 100644
--- a/tests/view/attributeelement.js
+++ b/tests/view/attributeelement.js
@@ -80,6 +80,32 @@ describe( 'AttributeElement', () => {
 
 			expect( b1.isSimilar( b2 ) ).to.be.false;
 		} );
+
+		it( 'should return true if ids are the same even if other properties are different', () => {
+			const element1 = new AttributeElement( 'b' );
+			element1._id = 'xyz';
+
+			const element2 = new AttributeElement( 'b', { foo: 'bar' } );
+			element2._id = 'xyz';
+
+			const element3 = new AttributeElement( 'span' );
+			element3._id = 'xyz';
+
+			expect( element1.isSimilar( element2 ) ).to.be.true;
+			expect( element1.isSimilar( element3 ) ).to.be.true;
+		} );
+
+		it( 'should return false if ids are different even if other properties are same', () => {
+			const element1 = new AttributeElement( 'span', { foo: 'bar' } );
+			element1._priority = 3;
+			element1._id = 'foo';
+
+			const element2 = new AttributeElement( 'span', { foo: 'bar' } );
+			element2._priority = 3;
+			element2._id = 'bar';
+
+			expect( element1.isSimilar( element2 ) ).to.be.false;
+		} );
 	} );
 
 	describe( 'getFillerOffset', () => {

From 5e2f92b218d01846e8c0afaca5ec64e0f8acd174 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Wed, 7 Mar 2018 15:10:00 +0100
Subject: [PATCH 02/13] Other: Small code refactor.

---
 src/model/differ.js   | 2 +-
 src/model/document.js | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/model/differ.js b/src/model/differ.js
index 8e3398655..b76d02d4d 100644
--- a/src/model/differ.js
+++ b/src/model/differ.js
@@ -181,7 +181,7 @@ export default class Differ {
 			buffered.newRange = newRange;
 
 			if ( buffered.oldRange == null && buffered.newRange == null ) {
-				// The marker is going to be removed (`newRange == null`) but it did not exist before the change set
+				// The marker is going to be removed (`newRange == null`) but it did not exist before the first buffered change
 				// (`buffered.oldRange == null`). In this case, do not keep the marker in buffer at all.
 				this._changedMarkers.delete( markerName );
 			}
diff --git a/src/model/document.js b/src/model/document.js
index b8e350dff..a74b2fdce 100644
--- a/src/model/document.js
+++ b/src/model/document.js
@@ -165,8 +165,8 @@ export default class Document {
 			// Whenever marker is updated, buffer that change.
 			this.differ.bufferMarkerChange( marker.name, oldRange, newRange );
 
-			if ( !oldRange ) {
-				// Whenever marker changes, buffer that.
+			if ( oldRange === null ) {
+				// If this is a new marker, add a listener that will buffer change whenever marker changes.
 				marker.on( 'change', ( evt, oldRange ) => {
 					this.differ.bufferMarkerChange( marker.name, oldRange, marker.getRange() );
 				} );

From 871d9cef74e91520508454fcbb1f2971fd14df5b Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Wed, 7 Mar 2018 16:17:10 +0100
Subject: [PATCH 03/13] Added: Introduced marker-name-to-elements mapping in
 `conversion.Mapper`.

---
 src/conversion/mapper.js   | 49 ++++++++++++++++++++++++++++++++++++-
 tests/conversion/mapper.js | 50 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/src/conversion/mapper.js b/src/conversion/mapper.js
index 39dcf9397..22b955d43 100644
--- a/src/conversion/mapper.js
+++ b/src/conversion/mapper.js
@@ -18,7 +18,8 @@ import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 
 /**
- * Maps elements and positions between {@link module:engine/view/document~Document view} and {@link module:engine/model/model model}.
+ * Maps elements, positions and markers between {@link module:engine/view/document~Document the view} and
+ * {@link module:engine/model/model the model}.
  *
  * Mapper use bound elements to find corresponding elements and positions, so, to get proper results,
  * all model elements should be {@link module:engine/conversion/mapper~Mapper#bindElements bound}.
@@ -62,6 +63,17 @@ export default class Mapper {
 		 */
 		this._viewToModelLengthCallbacks = new Map();
 
+		/**
+		 * Model marker name to view elements mapping.
+		 *
+		 * Keys are `String`s while values are `Set`s with {@link module:engine/view/element~Element view elements}.
+		 * One marker (name) can be mapped to multiple elements.
+		 *
+		 * @private
+		 * @member {Map}
+		 */
+		this._markerNameToElements = new Map();
+
 		// Default mapper algorithm for mapping model position to view position.
 		this.on( 'modelToViewPosition', ( evt, data ) => {
 			if ( data.viewPosition ) {
@@ -149,12 +161,37 @@ export default class Mapper {
 		}
 	}
 
+	/**
+	 * Binds given marker name with given {@link module:engine/view/element~Element view element}. The element
+	 * will be added to the current set of elements bound with given marker name.
+	 *
+	 * @param {module:engine/view/element~Element} element Element to bind.
+	 * @param {String} name Marker name.
+	 */
+	bindElementToMarker( element, name ) {
+		const elements = this._markerNameToElements.get( name ) || new Set();
+
+		elements.add( element );
+
+		this._markerNameToElements.set( name, elements );
+	}
+
+	/**
+	 * Unbinds all elements from given marker name.
+	 *
+	 * @param {String} name Marker name.
+	 */
+	unbindElementsFromMarkerName( name ) {
+		this._markerNameToElements.delete( name );
+	}
+
 	/**
 	 * Removes all model to view and view to model bindings.
 	 */
 	clearBindings() {
 		this._modelToViewMapping = new WeakMap();
 		this._viewToModelMapping = new WeakMap();
+		this._markerNameToElements = new Map();
 	}
 
 	/**
@@ -239,6 +276,16 @@ export default class Mapper {
 		return data.viewPosition;
 	}
 
+	/**
+	 * Gets all view elements bound to the given marker name.
+	 *
+	 * @param {String} name Marker name.
+	 * @returns {Set.<module:engine/view/element~Element>} View elements bound with given marker name.
+	 */
+	markerNameToElements( name ) {
+		return this._markerNameToElements.get( name );
+	}
+
 	/**
 	 * Registers a callback that evaluates the length in the model of a view element with given name.
 	 *
diff --git a/tests/conversion/mapper.js b/tests/conversion/mapper.js
index db3687e2b..9414bcbc2 100644
--- a/tests/conversion/mapper.js
+++ b/tests/conversion/mapper.js
@@ -610,6 +610,56 @@ describe( 'Mapper', () => {
 		}
 	} );
 
+	describe( 'Markers mapping', () => {
+		let mapper;
+
+		beforeEach( () => {
+			mapper = new Mapper();
+		} );
+
+		it( 'should bind element to a marker name', () => {
+			const view = new ViewElement( 'a' );
+
+			mapper.bindElementToMarker( view, 'marker' );
+
+			const elements = mapper.markerNameToElements( 'marker' );
+
+			expect( elements ).to.be.instanceof( Set );
+			expect( elements.size ).to.equal( 1 );
+			expect( elements.has( view ) ).to.be.true;
+		} );
+
+		it( 'should bind multiple elements to a marker name', () => {
+			const viewA = new ViewElement( 'a' );
+			const viewB = new ViewElement( 'b' );
+			const viewC = new ViewElement( 'c' );
+
+			mapper.bindElementToMarker( viewA, 'marker' );
+			mapper.bindElementToMarker( viewB, 'marker' );
+			mapper.bindElementToMarker( viewC, 'marker' );
+
+			const elements = Array.from( mapper.markerNameToElements( 'marker' ) );
+
+			expect( elements ).to.deep.equal( [ viewA, viewB, viewC ] );
+		} );
+
+		it( 'should unbind all elements from a marker name', () => {
+			const viewA = new ViewElement( 'a' );
+			const viewB = new ViewElement( 'b' );
+			const viewC = new ViewElement( 'c' );
+
+			mapper.bindElementToMarker( viewA, 'marker' );
+			mapper.bindElementToMarker( viewB, 'marker' );
+			mapper.bindElementToMarker( viewC, 'marker' );
+
+			mapper.unbindElementsFromMarkerName( 'marker' );
+
+			const elements = mapper.markerNameToElements( 'marker' );
+
+			expect( elements ).to.be.undefined;
+		} );
+	} );
+
 	it( 'should pass isPhantom flag to model-to-view position mapping callback', () => {
 		const mapper = new Mapper();
 

From 1c346717069d79c8af66e7ef1968b2a66097eabe Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Wed, 7 Mar 2018 16:17:33 +0100
Subject: [PATCH 04/13] Tests: Added missing test for `view.Range`.

---
 tests/view/range.js | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/view/range.js b/tests/view/range.js
index b7159193b..c6c54dd6e 100644
--- a/tests/view/range.js
+++ b/tests/view/range.js
@@ -8,6 +8,7 @@ import Position from '../../src/view/position';
 import Element from '../../src/view/element';
 import DocumentFragment from '../../src/view/documentfragment';
 import Text from '../../src/view/text';
+import TextProxy from '../../src/view/textproxy';
 import TreeWalker from '../../src/view/treewalker';
 import { parse, stringify } from '../../src/dev-utils/view';
 
@@ -673,6 +674,17 @@ describe( 'Range', () => {
 				expect( range.end.parent ).to.equal( div );
 				expect( range.end.offset ).to.equal( 1 );
 			} );
+
+			it( 'should create a proper range on a text proxy', () => {
+				const text = new Text( 'foobar' );
+				const textProxy = new TextProxy( text, 2, 3 );
+				const range = Range.createOn( textProxy );
+
+				expect( range.start.parent ).to.equal( text );
+				expect( range.start.offset ).to.equal( 2 );
+				expect( range.end.parent ).to.equal( text );
+				expect( range.end.offset ).to.equal( 5 );
+			} );
 		} );
 
 		describe( 'createCollapsedAt()', () => {

From 6b37cab550de41ec7f7eb44f75776ad97363ab6d Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Wed, 7 Mar 2018 16:22:25 +0100
Subject: [PATCH 05/13] Changed: Allowed to pass `id` in
 `view.Writer#createAttributeElement`.

---
 src/view/writer.js          | 21 +++++++++++++++++----
 tests/view/writer/writer.js |  5 +++--
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/view/writer.js b/src/view/writer.js
index 4de5cd083..fb00d1f9f 100644
--- a/src/view/writer.js
+++ b/src/view/writer.js
@@ -135,15 +135,28 @@ export default class Writer {
 	 *		writer.createAttributeElement( 'strong' );
 	 *		writer.createAttributeElement( 'strong', { 'alignment': 'center' } );
 	 *
+	 *		// Make `<a>` element contain other attributes element so the `<a>` element is not broken.
+	 *		writer.createAttributeElement( 'a', { href: 'foo.bar' }, { priority: 5 } );
+	 *
+	 *		// Set `id` of a marker element so it is not joined or merged with "normal" elements.
+	 *		writer.createAttributeElement( 'span', { class: 'myMarker' }, { id: 'marker:my' } );
+	 *
 	 * @param {String} name Name of the element.
-	 * @param {Object} [attributes] Elements attributes.
+	 * @param {Object} [attributes] Element's attributes.
+	 * @param {Object} [options] Element's options.
+	 * @param {Number} [options.priority] Element's {@link module:engine/view/attributeelement~AttributeElement#priority priority}.
+	 * @param {Number|String} [options.id] Element's {@link module:engine/view/attributeelement~AttributeElement#id id}.
 	 * @returns {module:engine/view/attributeelement~AttributeElement} Created element.
 	 */
-	createAttributeElement( name, attributes, priority ) {
+	createAttributeElement( name, attributes, options = {} ) {
 		const attributeElement = new AttributeElement( name, attributes );
 
-		if ( priority ) {
-			attributeElement._priority = priority;
+		if ( options.priority ) {
+			attributeElement._priority = options.priority;
+		}
+
+		if ( options.id ) {
+			attributeElement._id = options.id;
 		}
 
 		return attributeElement;
diff --git a/tests/view/writer/writer.js b/tests/view/writer/writer.js
index 3a7ab930d..02377e444 100644
--- a/tests/view/writer/writer.js
+++ b/tests/view/writer/writer.js
@@ -68,12 +68,13 @@ describe( 'Writer', () => {
 			assertElementAttributes( element, attributes );
 		} );
 
-		it( 'should allow to pass priority', () => {
-			const element = writer.createAttributeElement( 'foo', attributes, 99 );
+		it( 'should allow to pass additional options', () => {
+			const element = writer.createAttributeElement( 'foo', attributes, { priority: 99, id: 'bar' } );
 
 			expect( element.is( 'attributeElement' ) ).to.be.true;
 			expect( element.name ).to.equal( 'foo' );
 			expect( element.priority ).to.equal( 99 );
+			expect( element.id ).to.equal( 'bar' );
 			assertElementAttributes( element, attributes );
 		} );
 	} );

From ea2d67ecf7dcc83bccc16341d12be9330ada89b1 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Thu, 8 Mar 2018 11:40:23 +0100
Subject: [PATCH 06/13] Added: Enabled `view.AttributeElement#id` in view dev
 utils.

---
 src/dev-utils/view.js   | 63 +++++++++++++++++++++++++++++++++++------
 tests/dev-utils/view.js | 18 ++++++++++++
 2 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/src/dev-utils/view.js b/src/dev-utils/view.js
index a2b7cc6fd..8b6077fab 100644
--- a/src/dev-utils/view.js
+++ b/src/dev-utils/view.js
@@ -47,6 +47,8 @@ const allowedTypes = {
  * instead of `<p>`, `<attribute:b>` instead of `<b>` and `<empty:img>` instead of `<img>`).
  * @param {Boolean} [options.showPriority=false] When set to `true`, attribute element's priority will be printed
  * (`<span view-priority="12">`, `<b view-priority="10">`).
+ * @param {Boolean} [options.showAttributeElementId=false] When set to `true`, attribute element's id will be printed
+ * (`<span id="marker:foo">`).
  * @returns {String} The stringified data.
  */
 export function getData( view, options = {} ) {
@@ -195,6 +197,13 @@ setData._parse = parse;
  *		attribute._priority = 20;
  *		getData( attribute, null, { showPriority: true } ); // <b view-priority="20"></b>
  *
+ * If `options.showAttributeElementId` is set to `true`, the attribute element's id will be displayed for all
+ * {@link module:engine/view/attributeelement~AttributeElement attribute elements} that have it set.
+ *
+ *		const attribute = new AttributeElement( 'span' );
+ *		attribute._id = 'marker:foo';
+ *		getData( attribute, null, { showAttributeElementId: true } ); // <span view-id="marker:foo"></span>
+ *
  * @param {module:engine/view/text~Text|module:engine/view/element~Element|module:engine/view/documentfragment~DocumentFragment}
  * node The node to stringify.
  * @param {module:engine/view/selection~Selection|module:engine/view/position~Position|module:engine/view/range~Range}
@@ -207,6 +216,8 @@ setData._parse = parse;
  * instead of `<p>`, `<attribute:b>` instead of `<b>` and `<empty:img>` instead of `<img>`).
  * @param {Boolean} [options.showPriority=false] When set to `true`,  the attribute element's priority will be printed
  * (`<span view-priority="12">`, `<b view-priority="10">`).
+ * @param {Boolean} [options.showAttributeElementId=false] When set to `true`, attribute element's id will be printed
+ * (`<span id="marker:foo">`).
  * @param {Boolean} [options.ignoreRoot=false] When set to `true`, the root's element opening and closing will not be printed.
  * Mainly used by the `getData` function to ignore the {@link module:engine/view/document~Document document's} root element.
  * @param {Boolean} [options.sameSelectionCharacters=false] When set to `true`, the selection inside the text will be marked as
@@ -604,6 +615,7 @@ class ViewStringify {
 
 		this.showType = !!options.showType;
 		this.showPriority = !!options.showPriority;
+		this.showAttributeElementId = !!options.showAttributeElementId;
 		this.ignoreRoot = !!options.ignoreRoot;
 		this.sameSelectionCharacters = !!options.sameSelectionCharacters;
 	}
@@ -744,9 +756,11 @@ class ViewStringify {
 
 	/**
 	 * Converts the passed {@link module:engine/view/element~Element element} to an opening tag.
+	 *
 	 * Depending on the current configuration, the opening tag can be simple (`<a>`), contain a type prefix (`<container:p>`,
-	 * `<attribute:a>` or `<empty:img>`) or contain priority information ( `<attribute:a view-priority="20">` ).
-	 * Element attributes will also be included (`<a href="https://ckeditor.com" name="foobar">`).
+	 * `<attribute:a>` or `<empty:img>`), contain priority information ( `<attribute:a view-priority="20">` ),
+	 * or contain element id ( `<attribute:span view-id="foo">` ). Element attributes will also be included
+	 * (`<a href="https://ckeditor.com" name="foobar">`).
 	 *
 	 * @private
 	 * @param {module:engine/view/element~Element} element
@@ -754,11 +768,12 @@ class ViewStringify {
 	 */
 	_stringifyElementOpen( element ) {
 		const priority = this._stringifyElementPriority( element );
+		const id = this._stringifyElementId( element );
 
 		const type = this._stringifyElementType( element );
 		const name = [ type, element.name ].filter( i => i !== '' ).join( ':' );
 		const attributes = this._stringifyElementAttributes( element );
-		const parts = [ name, priority, attributes ];
+		const parts = [ name, priority, id, attributes ];
 
 		return `<${ parts.filter( i => i !== '' ).join( ' ' ) }>`;
 	}
@@ -807,6 +822,7 @@ class ViewStringify {
 
 	/**
 	 * Converts the passed {@link module:engine/view/element~Element element} to its priority representation.
+	 *
 	 * The priority string representation will be returned when the passed element is an instance of
 	 * {@link module:engine/view/attributeelement~AttributeElement attribute element} and the current configuration allows to show the
 	 * priority. Otherwise returns an empty string.
@@ -823,6 +839,25 @@ class ViewStringify {
 		return '';
 	}
 
+	/**
+	 * Converts the passed {@link module:engine/view/element~Element element} to its id representation.
+	 *
+	 * The id string representation will be returned when the passed element is an instance of
+	 * {@link module:engine/view/attributeelement~AttributeElement attribute element}, the element has an id
+	 * and the current configuration allows to show the id. Otherwise returns an empty string.
+	 *
+	 * @private
+	 * @param {module:engine/view/element~Element} element
+	 * @returns {String}
+	 */
+	_stringifyElementId( element ) {
+		if ( this.showAttributeElementId && element.is( 'attributeElement' ) && element.id ) {
+			return `view-id="${ element.id }"`;
+		}
+
+		return '';
+	}
+
 	/**
 	 * Converts the passed {@link module:engine/view/element~Element element} attributes to their string representation.
 	 * If an element has no attributes, an empty string is returned.
@@ -901,8 +936,10 @@ function _convertViewElements( rootNode ) {
 // {@link module:engine/view/containerelement~ContainerElement container element},
 // {@link module:engine/view/emptyelement~EmptyElement empty element} or
 // {@link module:engine/view/uielement~UIElement UI element}.
-// If the element's name is in the format of `attribute:b` with `view-priority="11"` attribute, it will be converted to
+// If the element's name is in the format of `attribute:b`, it will be converted to
 // an {@link module:engine/view/attributeelement~AttributeElement attribute element} with a priority of 11.
+// Additionally, attribute elements may have specified priority (for example `view-priority="11"`) and/or
+// id (for example `view-id="foo"`).
 // If the element's name is in the format of `container:p`, it will be converted to
 // a {@link module:engine/view/containerelement~ContainerElement container element}.
 // If the element's name is in the format of `empty:img`, it will be converted to
@@ -918,7 +955,7 @@ function _convertViewElements( rootNode ) {
 // module:engine/view/containerelement~ContainerElement} A tree view
 // element converted according to its name.
 function _convertElement( viewElement ) {
-	const info = _convertElementNameAndPriority( viewElement );
+	const info = _convertElementNameAndInfo( viewElement );
 	const ElementConstructor = allowedTypes[ info.type ];
 	const newElement = ElementConstructor ? new ElementConstructor( info.name ) : new ViewElement( info.name );
 
@@ -926,6 +963,10 @@ function _convertElement( viewElement ) {
 		if ( info.priority !== null ) {
 			newElement._priority = info.priority;
 		}
+
+		if ( info.id !== null ) {
+			newElement._id = info.id;
+		}
 	}
 
 	// Move attributes.
@@ -949,16 +990,21 @@ function _convertElement( viewElement ) {
 // @returns {String} info.name The parsed name of the element.
 // @returns {String|null} info.type The parsed type of the element. It can be `attribute`, `container` or `empty`.
 // returns {Number|null} info.priority The parsed priority of the element.
-function _convertElementNameAndPriority( viewElement ) {
+function _convertElementNameAndInfo( viewElement ) {
 	const parts = viewElement.name.split( ':' );
+
 	const priority = _convertPriority( viewElement.getAttribute( 'view-priority' ) );
+	const id = viewElement.hasAttribute( 'view-id' ) ? viewElement.getAttribute( 'view-id' ) : null;
+
 	viewElement._removeAttribute( 'view-priority' );
+	viewElement._removeAttribute( 'view-id' );
 
 	if ( parts.length == 1 ) {
 		return {
 			name: parts[ 0 ],
 			type: priority !== null ? 'attribute' : null,
-			priority
+			priority,
+			id
 		};
 	}
 
@@ -969,7 +1015,8 @@ function _convertElementNameAndPriority( viewElement ) {
 		return {
 			name: parts[ 1 ],
 			type,
-			priority
+			priority,
+			id
 		};
 	}
 
diff --git a/tests/dev-utils/view.js b/tests/dev-utils/view.js
index efe9517c1..341f32998 100644
--- a/tests/dev-utils/view.js
+++ b/tests/dev-utils/view.js
@@ -254,6 +254,16 @@ describe( 'view test utils', () => {
 				.to.equal( '<p><b view-priority="10">foobar</b></p>' );
 		} );
 
+		it( 'should write elements id when needed', () => {
+			const text = new Text( 'foobar' );
+			const span = new AttributeElement( 'span', null, text );
+			span._id = 'foo';
+			const p = new ContainerElement( 'p', null, span );
+
+			expect( stringify( p, null, { showAttributeElementId: true } ) )
+				.to.equal( '<p><span view-id="foo">foobar</span></p>' );
+		} );
+
 		it( 'should parse DocumentFragment as root', () => {
 			const text1 = new Text( 'foobar' );
 			const text2 = new Text( 'bazqux' );
@@ -465,6 +475,14 @@ describe( 'view test utils', () => {
 			expect( parsed2.isSimilar( attribute2 ) ).to.be.true;
 		} );
 
+		it( 'should parse attribute element id', () => {
+			const parsed1 = parse( '<attribute:span view-id="foo"></attribute:span>' );
+			expect( parsed1.id ).to.equal( 'foo' );
+
+			const parsed2 = parse( '<container:div view-id="bar"></container:div>' );
+			expect( parsed2.id ).to.be.undefined;
+		} );
+
 		it( 'should paste nested elements and texts', () => {
 			const parsed = parse( '<container:p>foo<b view-priority="12">bar<i view-priority="25">qux</i></b></container:p>' );
 			expect( parsed.isSimilar( new ContainerElement( 'p' ) ) ).to.be.true;

From e19ff050c161d124a678c6887d0e6b18fecb4a06 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Thu, 8 Mar 2018 16:08:16 +0100
Subject: [PATCH 07/13] Added: `view.Writer` now links attribute elements
 together when they are broken during wrapping. Introduced
 `view.Writer#getAllBrokenSiblings`. Changed: Changed how `view.Writer` wraps
 and unwraps elements, now it bases on `view.AttributeElement#id` too.
 Removed: Removed `HighlightAttributeElement`.

---
 src/view/writer.js          | 351 +++++++++++++++++++++---------------
 tests/view/writer/unwrap.js |  61 +++++++
 tests/view/writer/wrap.js   |  45 ++++-
 tests/view/writer/writer.js |  55 ++++++
 4 files changed, 369 insertions(+), 143 deletions(-)

diff --git a/src/view/writer.js b/src/view/writer.js
index fb00d1f9f..4aab8bf76 100644
--- a/src/view/writer.js
+++ b/src/view/writer.js
@@ -179,10 +179,9 @@ export default class Writer {
 	/**
 	 * Creates new {@link module:engine/view/editableelement~EditableElement}.
 	 *
-	 *		writer.createEditableElement( document, 'div' );
-	 *		writer.createEditableElement( document, 'div', { 'alignment': 'center' } );
+	 *		writer.createEditableElement( 'div' );
+	 *		writer.createEditableElement( 'div', { 'alignment': 'center' } );
 	 *
-	 * @param {module:engine/view/document~Document} document View document.
 	 * @param {String} name Name of the element.
 	 * @param {Object} [attributes] Elements attributes.
 	 * @returns {module:engine/view/editableelement~EditableElement} Created element.
@@ -397,9 +396,9 @@ export default class Writer {
 	 */
 	breakAttributes( positionOrRange ) {
 		if ( positionOrRange instanceof Position ) {
-			return _breakAttributes( positionOrRange );
+			return this._breakAttributes( positionOrRange );
 		} else {
-			return _breakAttributesRange( positionOrRange );
+			return this._breakAttributesRange( positionOrRange );
 		}
 	}
 
@@ -506,6 +505,8 @@ export default class Writer {
 			const offset = positionParent.index;
 			positionParent._remove();
 
+			this._unlinkBrokenAttributeElement( positionParent );
+
 			return this.mergeAttributes( new Position( parent, offset ) );
 		}
 
@@ -521,12 +522,14 @@ export default class Writer {
 		if ( nodeBefore.is( 'text' ) && nodeAfter.is( 'text' ) ) {
 			return mergeTextNodes( nodeBefore, nodeAfter );
 		}
-		// When selection is between two same attribute elements.
+		// When position is between two same attribute elements.
 		else if ( nodeBefore.is( 'attributeElement' ) && nodeAfter.is( 'attributeElement' ) && nodeBefore.isSimilar( nodeAfter ) ) {
 			// Move all children nodes from node placed after selection and remove that node.
 			const count = nodeBefore.childCount;
 			nodeBefore._appendChildren( nodeAfter.getChildren() );
+
 			nodeAfter._remove();
+			this._unlinkBrokenAttributeElement( nodeAfter );
 
 			// New position is located inside the first node, before new nodes.
 			// Call this method recursively to merge again if needed.
@@ -613,7 +616,7 @@ export default class Writer {
 			throw new CKEditorError( 'view-writer-invalid-position-container' );
 		}
 
-		const insertionPosition = _breakAttributes( position, true );
+		const insertionPosition = this._breakAttributes( position, true );
 
 		const length = container._insertChildren( insertionPosition.offset, nodes );
 		const endPosition = insertionPosition.getShiftedBy( length );
@@ -654,7 +657,7 @@ export default class Writer {
 		}
 
 		// Break attributes at range start and end.
-		const { start: breakStart, end: breakEnd } = _breakAttributesRange( range, true );
+		const { start: breakStart, end: breakEnd } = this._breakAttributesRange( range, true );
 		const parentContainer = breakStart.parent;
 
 		const count = breakEnd.offset - breakStart.offset;
@@ -662,6 +665,10 @@ export default class Writer {
 		// Remove nodes in range.
 		const removed = parentContainer._removeChildren( breakStart.offset, count );
 
+		for ( const node of removed ) {
+			this._unlinkBrokenAttributeElement( node );
+		}
+
 		// Merge after removing.
 		const mergePosition = this.mergeAttributes( breakStart );
 		range.start = mergePosition;
@@ -746,12 +753,12 @@ export default class Writer {
 		let nodes;
 
 		if ( targetPosition.isAfter( sourceRange.end ) ) {
-			targetPosition = _breakAttributes( targetPosition, true );
+			targetPosition = this._breakAttributes( targetPosition, true );
 
 			const parent = targetPosition.parent;
 			const countBefore = parent.childCount;
 
-			sourceRange = _breakAttributesRange( sourceRange, true );
+			sourceRange = this._breakAttributesRange( sourceRange, true );
 
 			nodes = this.remove( sourceRange );
 
@@ -842,7 +849,7 @@ export default class Writer {
 		}
 
 		// Break attributes at range start and end.
-		const { start: breakStart, end: breakEnd } = _breakAttributesRange( range, true );
+		const { start: breakStart, end: breakEnd } = this._breakAttributesRange( range, true );
 
 		// Range around one element - check if AttributeElement can be unwrapped partially when it's not similar.
 		// For example:
@@ -904,6 +911,24 @@ export default class Writer {
 		return newElement;
 	}
 
+	/**
+	 * For the given attribute element, returns that element and all the elements that were cloned from the given element
+	 * when the element was broken by `Writer`.
+	 *
+	 * @param {module:engine/view/attributeelement~AttributeElement} element
+	 * @returns {Array.<module:engine/view/attributeelement~AttributeElement>}
+	 */
+	getAllBrokenSiblings( element ) {
+		const original = element.getCustomProperty( 'originalAttributeElement' ) || element;
+		const clones = original.getCustomProperty( 'clonedAttributeElements' );
+
+		if ( clones ) {
+			return Array.from( clones ).concat( original );
+		} else {
+			return [ element ];
+		}
+	}
+
 	/**
 	 * Wraps children with provided `attribute`. Only children contained in `parent` element between
 	 * `startOffset` and `endOffset` will be wrapped.
@@ -930,7 +955,7 @@ export default class Writer {
 				// Clone attribute.
 				const newAttribute = attribute._clone();
 
-				// Wrap current node with new attribute;
+				// Wrap current node with new attribute.
 				child._remove();
 				newAttribute._appendChildren( child );
 				parent._insertChildren( i, newAttribute );
@@ -993,6 +1018,8 @@ export default class Writer {
 
 				// Replace wrapper element with its children
 				child._remove();
+				this._unlinkBrokenAttributeElement( child );
+
 				parent._insertChildren( i, unwrapped );
 
 				// Save start and end position of moved items.
@@ -1062,7 +1089,7 @@ export default class Writer {
 		}
 
 		// Break attributes at range start and end.
-		const { start: breakStart, end: breakEnd } = _breakAttributesRange( range, true );
+		const { start: breakStart, end: breakEnd } = this._breakAttributesRange( range, true );
 
 		// Range around one element.
 		if ( breakEnd.isEqual( breakStart.getShiftedBy( 1 ) ) ) {
@@ -1165,6 +1192,10 @@ export default class Writer {
 	 * 	@returns {Boolean} Returns `true` if elements are merged.
 	 */
 	_wrapAttributeElement( wrapper, toWrap ) {
+		if ( !canBeJoined( wrapper, toWrap ) ) {
+			return false;
+		}
+
 		// Can't merge if name or priority differs.
 		if ( wrapper.name !== toWrap.name || wrapper.priority !== toWrap.priority ) {
 			return false;
@@ -1229,6 +1260,10 @@ export default class Writer {
 	 * @returns {Boolean} Returns `true` if elements are unwrapped.
 	 **/
 	_unwrapAttributeElement( wrapper, toUnwrap ) {
+		if ( !canBeJoined( wrapper, toUnwrap ) ) {
+			return false;
+		}
+
 		// Can't unwrap if name or priority differs.
 		if ( wrapper.name !== toUnwrap.name || wrapper.priority !== toUnwrap.priority ) {
 			return false;
@@ -1278,6 +1313,156 @@ export default class Writer {
 
 		return true;
 	}
+
+	_breakAttributesRange( range, forceSplitText = false ) {
+		const rangeStart = range.start;
+		const rangeEnd = range.end;
+
+		validateRangeContainer( range );
+
+		// Break at the collapsed position. Return new collapsed range.
+		if ( range.isCollapsed ) {
+			const position = this._breakAttributes( range.start, forceSplitText );
+
+			return new Range( position, position );
+		}
+
+		const breakEnd = this._breakAttributes( rangeEnd, forceSplitText );
+		const count = breakEnd.parent.childCount;
+		const breakStart = this._breakAttributes( rangeStart, forceSplitText );
+
+		// Calculate new break end offset.
+		breakEnd.offset += breakEnd.parent.childCount - count;
+
+		return new Range( breakStart, breakEnd );
+	}
+
+	_breakAttributes( position, forceSplitText = false ) {
+		const positionOffset = position.offset;
+		const positionParent = position.parent;
+
+		// If position is placed inside EmptyElement - throw an exception as we cannot break inside.
+		if ( position.parent.is( 'emptyElement' ) ) {
+			/**
+			 * Cannot break inside EmptyElement instance.
+			 *
+			 * @error view-writer-cannot-break-empty-element
+			 */
+			throw new CKEditorError( 'view-writer-cannot-break-empty-element' );
+		}
+
+		// If position is placed inside UIElement - throw an exception as we cannot break inside.
+		if ( position.parent.is( 'uiElement' ) ) {
+			/**
+			 * Cannot break inside UIElement instance.
+			 *
+			 * @error view-writer-cannot-break-ui-element
+			 */
+			throw new CKEditorError( 'view-writer-cannot-break-ui-element' );
+		}
+
+		// There are no attributes to break and text nodes breaking is not forced.
+		if ( !forceSplitText && positionParent.is( 'text' ) && isContainerOrFragment( positionParent.parent ) ) {
+			return Position.createFromPosition( position );
+		}
+
+		// Position's parent is container, so no attributes to break.
+		if ( isContainerOrFragment( positionParent ) ) {
+			return Position.createFromPosition( position );
+		}
+
+		// Break text and start again in new position.
+		if ( positionParent.is( 'text' ) ) {
+			return this._breakAttributes( breakTextNode( position ), forceSplitText );
+		}
+
+		const length = positionParent.childCount;
+
+		// <p>foo<b><u>bar{}</u></b></p>
+		// <p>foo<b><u>bar</u>[]</b></p>
+		// <p>foo<b><u>bar</u></b>[]</p>
+		if ( positionOffset == length ) {
+			const newPosition = new Position( positionParent.parent, positionParent.index + 1 );
+
+			return this._breakAttributes( newPosition, forceSplitText );
+		} else
+		// <p>foo<b><u>{}bar</u></b></p>
+		// <p>foo<b>[]<u>bar</u></b></p>
+		// <p>foo{}<b><u>bar</u></b></p>
+		if ( positionOffset === 0 ) {
+			const newPosition = new Position( positionParent.parent, positionParent.index );
+
+			return this._breakAttributes( newPosition, forceSplitText );
+		}
+		// <p>foo<b><u>b{}ar</u></b></p>
+		// <p>foo<b><u>b[]ar</u></b></p>
+		// <p>foo<b><u>b</u>[]<u>ar</u></b></p>
+		// <p>foo<b><u>b</u></b>[]<b><u>ar</u></b></p>
+		else {
+			const offsetAfter = positionParent.index + 1;
+
+			// Break element.
+			const clonedNode = positionParent._clone();
+
+			// Save that the `clonedNode` was created from `positionParent`.
+			this._linkBrokenAttributeElements( clonedNode, positionParent );
+
+			// Insert cloned node to position's parent node.
+			positionParent.parent._insertChildren( offsetAfter, clonedNode );
+
+			// Get nodes to move.
+			const count = positionParent.childCount - positionOffset;
+			const nodesToMove = positionParent._removeChildren( positionOffset, count );
+
+			// Move nodes to cloned node.
+			clonedNode._appendChildren( nodesToMove );
+
+			// Create new position to work on.
+			const newPosition = new Position( positionParent.parent, offsetAfter );
+
+			return this._breakAttributes( newPosition, forceSplitText );
+		}
+	}
+
+	/**
+	 * Links `clonedNode` element to `clonedFrom` element. Saves information that `clonedNode` was created as a clone
+	 * of `clonedFrom` when `clonedFrom` was broken into two elements by the `Writer`.
+	 *
+	 * @private
+	 * @param {module:engine/view/attributeelement~AttributeElement} clonedNode
+	 * @param {module:engine/view/attributeelement~AttributeElement} clonedFrom
+	 */
+	_linkBrokenAttributeElements( clonedNode, clonedFrom ) {
+		const original = clonedFrom.getCustomProperty( 'originalAttributeElement' ) || clonedFrom;
+		const clones = original.getCustomProperty( 'clonedAttributeElements' ) || new Set();
+
+		this.setCustomProperty( 'originalAttributeElement', original, clonedNode );
+
+		clones.add( clonedNode );
+		this.setCustomProperty( 'clonedAttributeElements', clones, original );
+	}
+
+	/**
+	 * Unlinks `element`. Removes information about how `element` was broken by the `Writer` from `element` and from
+	 * its original element (the element it was cloned from).
+	 *
+	 * @private
+	 * @param {module:engine/view/attributeelement~AttributeElement} element
+	 */
+	_unlinkBrokenAttributeElement( element ) {
+		if ( !element.is( 'element' ) ) {
+			return;
+		}
+
+		const original = element.getCustomProperty( 'originalAttributeElement' );
+
+		if ( original ) {
+			this.removeCustomProperty( 'originalAttributeElement', element );
+
+			const clones = original.getCustomProperty( 'clonedAttributeElements' );
+			clones.delete( element );
+		}
+	}
 }
 
 // Helper function for `view.writer.wrap`. Checks if given element has any children that are not ui elements.
@@ -1311,135 +1496,6 @@ function getParentContainer( position ) {
 	return parent;
 }
 
-// Function used by both public breakAttributes (without splitting text nodes) and by other methods (with
-// splitting text nodes).
-//
-// @param {module:engine/view/range~Range} range Range which `start` and `end` positions will be used to break attributes.
-// @param {Boolean} [forceSplitText = false] If set to `true`, will break text nodes even if they are directly in
-// container element. This behavior will result in incorrect view state, but is needed by other view writing methods
-// which then fixes view state. Defaults to `false`.
-// @returns {module:engine/view/range~Range} New range with located at break positions.
-function _breakAttributesRange( range, forceSplitText = false ) {
-	const rangeStart = range.start;
-	const rangeEnd = range.end;
-
-	validateRangeContainer( range );
-
-	// Break at the collapsed position. Return new collapsed range.
-	if ( range.isCollapsed ) {
-		const position = _breakAttributes( range.start, forceSplitText );
-
-		return new Range( position, position );
-	}
-
-	const breakEnd = _breakAttributes( rangeEnd, forceSplitText );
-	const count = breakEnd.parent.childCount;
-	const breakStart = _breakAttributes( rangeStart, forceSplitText );
-
-	// Calculate new break end offset.
-	breakEnd.offset += breakEnd.parent.childCount - count;
-
-	return new Range( breakStart, breakEnd );
-}
-
-// Function used by public breakAttributes (without splitting text nodes) and by other methods (with
-// splitting text nodes).
-//
-// Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-writer-cannot-break-empty-element` when break position
-// is placed inside {@link module:engine/view/emptyelement~EmptyElement EmptyElement}.
-//
-// Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-writer-cannot-break-ui-element` when break position
-// is placed inside {@link module:engine/view/uielement~UIElement UIElement}.
-//
-// @param {module:engine/view/position~Position} position Position where to break attributes.
-// @param {Boolean} [forceSplitText = false] If set to `true`, will break text nodes even if they are directly in
-// container element. This behavior will result in incorrect view state, but is needed by other view writing methods
-// which then fixes view state. Defaults to `false`.
-// @returns {module:engine/view/position~Position} New position after breaking the attributes.
-function _breakAttributes( position, forceSplitText = false ) {
-	const positionOffset = position.offset;
-	const positionParent = position.parent;
-
-	// If position is placed inside EmptyElement - throw an exception as we cannot break inside.
-	if ( position.parent.is( 'emptyElement' ) ) {
-		/**
-		 * Cannot break inside EmptyElement instance.
-		 *
-		 * @error view-writer-cannot-break-empty-element
-		 */
-		throw new CKEditorError( 'view-writer-cannot-break-empty-element' );
-	}
-
-	// If position is placed inside UIElement - throw an exception as we cannot break inside.
-	if ( position.parent.is( 'uiElement' ) ) {
-		/**
-		 * Cannot break inside UIElement instance.
-		 *
-		 * @error view-writer-cannot-break-ui-element
-		 */
-		throw new CKEditorError( 'view-writer-cannot-break-ui-element' );
-	}
-
-	// There are no attributes to break and text nodes breaking is not forced.
-	if ( !forceSplitText && positionParent.is( 'text' ) && isContainerOrFragment( positionParent.parent ) ) {
-		return Position.createFromPosition( position );
-	}
-
-	// Position's parent is container, so no attributes to break.
-	if ( isContainerOrFragment( positionParent ) ) {
-		return Position.createFromPosition( position );
-	}
-
-	// Break text and start again in new position.
-	if ( positionParent.is( 'text' ) ) {
-		return _breakAttributes( breakTextNode( position ), forceSplitText );
-	}
-
-	const length = positionParent.childCount;
-
-	// <p>foo<b><u>bar{}</u></b></p>
-	// <p>foo<b><u>bar</u>[]</b></p>
-	// <p>foo<b><u>bar</u></b>[]</p>
-	if ( positionOffset == length ) {
-		const newPosition = new Position( positionParent.parent, positionParent.index + 1 );
-
-		return _breakAttributes( newPosition, forceSplitText );
-	} else
-	// <p>foo<b><u>{}bar</u></b></p>
-	// <p>foo<b>[]<u>bar</u></b></p>
-	// <p>foo{}<b><u>bar</u></b></p>
-	if ( positionOffset === 0 ) {
-		const newPosition = new Position( positionParent.parent, positionParent.index );
-
-		return _breakAttributes( newPosition, forceSplitText );
-	}
-	// <p>foo<b><u>b{}ar</u></b></p>
-	// <p>foo<b><u>b[]ar</u></b></p>
-	// <p>foo<b><u>b</u>[]<u>ar</u></b></p>
-	// <p>foo<b><u>b</u></b>[]<b><u>ar</u></b></p>
-	else {
-		const offsetAfter = positionParent.index + 1;
-
-		// Break element.
-		const clonedNode = positionParent._clone();
-
-		// Insert cloned node to position's parent node.
-		positionParent.parent._insertChildren( offsetAfter, clonedNode );
-
-		// Get nodes to move.
-		const count = positionParent.childCount - positionOffset;
-		const nodesToMove = positionParent._removeChildren( positionOffset, count );
-
-		// Move nodes to cloned node.
-		clonedNode._appendChildren( nodesToMove );
-
-		// Create new position to work on.
-		const newPosition = new Position( positionParent.parent, offsetAfter );
-
-		return _breakAttributes( newPosition, forceSplitText );
-	}
-}
-
 // Checks if first {@link module:engine/view/attributeelement~AttributeElement AttributeElement} provided to the function
 // can be wrapped otuside second element. It is done by comparing elements'
 // {@link module:engine/view/attributeelement~AttributeElement#priority priorities}, if both have same priority
@@ -1605,3 +1661,14 @@ function validateRangeContainer( range ) {
 		throw new CKEditorError( 'view-writer-invalid-range-container' );
 	}
 }
+
+// Checks if two attribute elements can be joined together. Elements can be joined together if, and only if
+// they do not have ids specified.
+//
+// @private
+// @param {module:engine/view/element~Element} a
+// @param {module:engine/view/element~Element} b
+// @returns {Boolean}
+function canBeJoined( a, b ) {
+	return a.id === null && b.id === null;
+}
diff --git a/tests/view/writer/unwrap.js b/tests/view/writer/unwrap.js
index f70507b56..9a2fd861b 100644
--- a/tests/view/writer/unwrap.js
+++ b/tests/view/writer/unwrap.js
@@ -372,5 +372,66 @@ describe( 'Writer', () => {
 				writer.unwrap( range, attribute );
 			} ).to.throw( CKEditorError, 'view-writer-cannot-break-ui-element' );
 		} );
+
+		it( 'should unwrap if both elements have same id', () => {
+			const unwrapper = writer.createAttributeElement( 'span', null, { id: 'foo' } );
+			const attribute = writer.createAttributeElement( 'span', null, { id: 'foo' } );
+			const container = writer.createContainerElement( 'div' );
+
+			writer.insert( Position.createAt( container, 0 ), attribute );
+			writer.unwrap( Range.createOn( attribute ), unwrapper );
+
+			expect( stringify( container, null, { showType: false, showPriority: false } ) ).to.equal( '<div></div>' );
+		} );
+
+		it( 'should always unwrap whole element if both elements have same id', () => {
+			const unwrapper = writer.createAttributeElement( 'b', null, { id: 'foo' } );
+			const attribute = writer.createAttributeElement( 'span', { foo: 'foo' }, { id: 'foo' } );
+			const container = writer.createContainerElement( 'div' );
+
+			writer.insert( Position.createAt( container, 0 ), attribute );
+			writer.unwrap( Range.createOn( attribute ), unwrapper );
+
+			expect( stringify( container, null, { showType: false, showPriority: false } ) ).to.equal( '<div></div>' );
+		} );
+
+		// Below are tests for elements with different ids.
+		// Only partial unwrapping is tested because elements with different ids are never similar.
+		// This means that unwrapping of whole element is not possible in this case.
+		it( 'should not unwrap matching attributes if the element to unwrap has id', () => {
+			const unwrapper = writer.createAttributeElement( 'span', { foo: 'foo' } );
+			const attribute = writer.createAttributeElement( 'span', { foo: 'foo', bar: 'bar' }, { id: 'id' } );
+			const container = writer.createContainerElement( 'div' );
+
+			writer.insert( Position.createAt( container, 0 ), attribute );
+			writer.unwrap( Range.createOn( attribute ), unwrapper );
+
+			const view = stringify( container, null, { showType: false, showPriority: false } );
+			expect( view ).to.equal( '<div><span bar="bar" foo="foo"></span></div>' );
+		} );
+
+		it( 'should not unwrap matching attributes if the unwrapping element has id', () => {
+			const unwrapper = writer.createAttributeElement( 'span', { foo: 'foo' }, { id: 'id' } );
+			const attribute = writer.createAttributeElement( 'span', { foo: 'foo', bar: 'bar' } );
+			const container = writer.createContainerElement( 'div' );
+
+			writer.insert( Position.createAt( container, 0 ), attribute );
+			writer.unwrap( Range.createOn( attribute ), unwrapper );
+
+			const view = stringify( container, null, { showType: false, showPriority: false } );
+			expect( view ).to.equal( '<div><span bar="bar" foo="foo"></span></div>' );
+		} );
+
+		it( 'should not unwrap matching attributes if the elements have different id', () => {
+			const unwrapper = writer.createAttributeElement( 'span', { foo: 'foo' }, { id: 'a' } );
+			const attribute = writer.createAttributeElement( 'span', { foo: 'foo', bar: 'bar' }, { id: 'b' } );
+			const container = writer.createContainerElement( 'div' );
+
+			writer.insert( Position.createAt( container, 0 ), attribute );
+			writer.unwrap( Range.createOn( attribute ), unwrapper );
+
+			const view = stringify( container, null, { showType: false, showPriority: false } );
+			expect( view ).to.equal( '<div><span bar="bar" foo="foo"></span></div>' );
+		} );
 	} );
 } );
diff --git a/tests/view/writer/wrap.js b/tests/view/writer/wrap.js
index b99995b4d..e9978767a 100644
--- a/tests/view/writer/wrap.js
+++ b/tests/view/writer/wrap.js
@@ -39,7 +39,8 @@ describe( 'Writer', () => {
 				const { view, selection } = parse( input );
 				const newRange = writer.wrap( selection.getFirstRange(), parse( wrapAttribute ) );
 
-				expect( stringify( view.root, newRange, { showType: true, showPriority: true } ) ).to.equal( expected );
+				expect( stringify( view.root, newRange, { showType: true, showPriority: true, showAttributeElementId: true } ) )
+					.to.equal( expected );
 			}
 
 			it( 'wraps single text node', () => {
@@ -431,6 +432,48 @@ describe( 'Writer', () => {
 					'</container:p>'
 				);
 			} );
+
+			it( 'should not join elements if element to wrap has id', () => {
+				test(
+					'<container:p>[<attribute:span foo="foo" view-id="foo">xyz</attribute:span>]</container:p>',
+
+					'<attribute:span bar="bar"></attribute:span>',
+
+					'<container:p>' +
+						'[<attribute:span view-priority="10" bar="bar">' +
+							'<attribute:span view-priority="10" view-id="foo" foo="foo">xyz</attribute:span>' +
+						'</attribute:span>]' +
+					'</container:p>'
+				);
+			} );
+
+			it( 'should not join elements if wrapper element has id', () => {
+				test(
+					'<container:p>[<attribute:span foo="foo">xyz</attribute:span>]</container:p>',
+
+					'<attribute:span bar="bar" view-id="foo"></attribute:span>',
+
+					'<container:p>' +
+						'[<attribute:span view-priority="10" view-id="foo" bar="bar">' +
+							'<attribute:span view-priority="10" foo="foo">xyz</attribute:span>' +
+						'</attribute:span>]' +
+					'</container:p>'
+				);
+			} );
+
+			it( 'should not join elements if they have different ids', () => {
+				test(
+					'<container:p>[<attribute:span foo="foo" view-id="foo">xyz</attribute:span>]</container:p>',
+
+					'<attribute:span bar="bar" view-id="bar"></attribute:span>',
+
+					'<container:p>' +
+						'[<attribute:span view-priority="10" view-id="bar" bar="bar">' +
+							'<attribute:span view-priority="10" view-id="foo" foo="foo">xyz</attribute:span>' +
+						'</attribute:span>]' +
+					'</container:p>'
+				);
+			} );
 		} );
 
 		describe( 'collapsed range', () => {
diff --git a/tests/view/writer/writer.js b/tests/view/writer/writer.js
index 02377e444..776122444 100644
--- a/tests/view/writer/writer.js
+++ b/tests/view/writer/writer.js
@@ -7,6 +7,7 @@ import Writer from '../../../src/view/writer';
 import Document from '../../../src/view/document';
 import EditableElement from '../../../src/view/editableelement';
 import ViewPosition from '../../../src/view/position';
+import ViewRange from '../../../src/view/range';
 import createViewRoot from '../_utils/createroot';
 
 describe( 'Writer', () => {
@@ -262,6 +263,60 @@ describe( 'Writer', () => {
 		} );
 	} );
 
+	describe( 'getAllBrokenSiblings()', () => {
+		it( 'should return all clones of a broken attribute element', () => {
+			const container = writer.createContainerElement( 'div' );
+			const text = writer.createText( 'abccccde' );
+
+			writer.insert( ViewPosition.createAt( container, 0 ), text );
+
+			const span = writer.createAttributeElement( 'span' );
+			span._priority = 20;
+
+			// <div>ab<span>cccc</span>de</div>
+			writer.wrap( ViewRange.createFromParentsAndOffsets( text, 2, text, 6 ), span );
+
+			const i = writer.createAttributeElement( 'i' );
+
+			// <div>a<i>b<span>c</span></i><span>cc</span>de</div>
+			writer.wrap(
+				ViewRange.createFromParentsAndOffsets(
+					container.getChild( 0 ), 1,
+					container.getChild( 1 ).getChild( 0 ), 1
+				),
+				i
+			);
+
+			// <div>a<i>b<span>c</span></i><span>c</span><i><span>c</span>d</i>e</div>
+			writer.wrap(
+				ViewRange.createFromParentsAndOffsets(
+					container.getChild( 2 ).getChild( 0 ), 1,
+					container.getChild( 3 ), 1
+				),
+				i
+			);
+
+			// Find all spans.
+			const allSpans = Array.from( ViewRange.createIn( container ).getItems() ).filter( element => element.is( 'span' ) );
+
+			// For each of the spans created above...
+			for ( const oneOfAllSpans of allSpans ) {
+				// Find all broken siblings of that span...
+				const brokenArray = writer.getAllBrokenSiblings( oneOfAllSpans );
+
+				// Convert to set because we don't care about order.
+				const brokenSet = new Set( brokenArray );
+
+				// Check if all spans are included.
+				for ( const s of allSpans ) {
+					expect( brokenSet.has( s ) ).to.be.true;
+				}
+
+				expect( brokenArray.length ).to.equal( allSpans.length );
+			}
+		} );
+	} );
+
 	function assertElementAttributes( element, attributes ) {
 		for ( const key of Object.keys( attributes ) ) {
 			if ( element.getAttribute( key ) !== attributes[ key ] ) {

From 26bfacf99f596b170f6d8c845e945c6715107945 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Thu, 8 Mar 2018 16:08:34 +0100
Subject: [PATCH 08/13] Docs: Small tweak in docs.

---
 src/view/range.js | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/view/range.js b/src/view/range.js
index babbce2e5..3cd170765 100644
--- a/src/view/range.js
+++ b/src/view/range.js
@@ -386,9 +386,11 @@ export default class Range {
 	/**
 	 * Creates a range from given parents and offsets.
 	 *
-	 * @param {module:engine/view/element~Element} startElement Start position parent element.
+	 * @param {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment} startElement Start position
+	 * parent element.
 	 * @param {Number} startOffset Start position offset.
-	 * @param {module:engine/view/element~Element} endElement End position parent element.
+	 * @param {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment} endElement End position
+	 * parent element.
 	 * @param {Number} endOffset End position offset.
 	 * @returns {module:engine/view/range~Range} Created range.
 	 */

From 934ec28e119928382994cd45a1149d3e817ee1c7 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Thu, 8 Mar 2018 16:15:40 +0100
Subject: [PATCH 09/13] Changed: Move markers removal conversion from
 `controller.EditingController` to `conversion.DowncastDispatcher`. Changed:
 Use markers binding provided by `conversion.Mapper` in
 `conversion.downcast-converters`.

---
 src/controller/editingcontroller.js     |  82 +--------
 src/conversion/downcast-converters.js   | 142 ++++++++-------
 src/conversion/downcastdispatcher.js    |   7 +-
 tests/controller/editingcontroller.js   | 220 ------------------------
 tests/conversion/downcast-converters.js | 145 ++++++++++++----
 5 files changed, 193 insertions(+), 403 deletions(-)

diff --git a/src/controller/editingcontroller.js b/src/controller/editingcontroller.js
index 115ede901..c7030e54d 100644
--- a/src/controller/editingcontroller.js
+++ b/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 );
-}
diff --git a/src/conversion/downcast-converters.js b/src/conversion/downcast-converters.js
index d5d984254..0b3f55849 100644
--- a/src/conversion/downcast-converters.js
+++ b/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,32 +988,11 @@ 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.
  *
diff --git a/src/conversion/downcastdispatcher.js b/src/conversion/downcastdispatcher.js
index af1c92090..36046a0a4 100644
--- a/src/conversion/downcastdispatcher.js
+++ b/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 );
 		}
diff --git a/tests/controller/editingcontroller.js b/tests/controller/editingcontroller.js
index 2a6721ccc..cb9738bfa 100644
--- a/tests/controller/editingcontroller.js
+++ b/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();
diff --git a/tests/conversion/downcast-converters.js b/tests/conversion/downcast-converters.js
index 5eeed9935..322a88352 100644
--- a/tests/conversion/downcast-converters.js
+++ b/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;
-		} );
 	} );
 } );

From 7898f06c8a4b7013722021c088d09e6b796fd752 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Mon, 12 Mar 2018 14:12:09 +0100
Subject: [PATCH 10/13] Fix: Removing highlight on text crashed when marker
 range was empty-ish.

---
 src/conversion/downcast-converters.js   |  5 +++++
 tests/conversion/downcast-converters.js | 22 ++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/src/conversion/downcast-converters.js b/src/conversion/downcast-converters.js
index 0b3f55849..ab05563c3 100644
--- a/src/conversion/downcast-converters.js
+++ b/src/conversion/downcast-converters.js
@@ -916,6 +916,11 @@ export function removeHighlight( highlightDescriptor ) {
 
 		// Get all elements bound with given marker name.
 		const elements = conversionApi.mapper.markerNameToElements( data.markerName );
+
+		if ( !elements ) {
+			return;
+		}
+
 		conversionApi.mapper.unbindElementsFromMarkerName( data.markerName );
 
 		for ( const element of elements ) {
diff --git a/tests/conversion/downcast-converters.js b/tests/conversion/downcast-converters.js
index 322a88352..cb6b2c120 100644
--- a/tests/conversion/downcast-converters.js
+++ b/tests/conversion/downcast-converters.js
@@ -1392,6 +1392,28 @@ describe( 'downcast-converters', () => {
 
 				expect( viewToString( viewRoot ) ).to.equal( '<div><p>foo</p><p>bar</p></div>' );
 			} );
+
+			it( 'should do nothing if marker is applied and removed on empty-ish range', () => {
+				dispatcher.on( 'addMarker:marker', highlightText( highlightDescriptor ) );
+				dispatcher.on( 'removeMarker:marker', removeHighlight( highlightDescriptor ) );
+
+				const p1 = modelRoot.getChild( 0 );
+				const p2 = modelRoot.getChild( 1 );
+
+				const markerRange = ModelRange.createFromParentsAndOffsets( p1, 3, p2, 0 );
+
+				model.change( writer => {
+					writer.setMarker( 'marker', markerRange );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal( '<div><p>foo</p><p>bar</p></div>' );
+
+				model.change( writer => {
+					writer.removeMarker( 'marker', markerRange );
+				} );
+
+				expect( viewToString( viewRoot ) ).to.equal( '<div><p>foo</p><p>bar</p></div>' );
+			} );
 		} );
 
 		describe( 'on element', () => {

From b0cee660eb732483ac40b5ecc12d95fc60fc4a76 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Wed, 14 Mar 2018 11:54:52 +0100
Subject: [PATCH 11/13] Changed: Refactored how `view.Writer` stores cloned
 attribute elements groups and how `conversion.Mapper` uses this information.

---
 src/conversion/downcast-converters.js |  22 ++--
 src/conversion/mapper.js              |  23 ++++-
 src/view/writer.js                    | 138 ++++++++++++++++++--------
 tests/conversion/mapper.js            |   2 +-
 tests/view/writer/writer.js           |  47 +++++++--
 5 files changed, 165 insertions(+), 67 deletions(-)

diff --git a/src/conversion/downcast-converters.js b/src/conversion/downcast-converters.js
index ab05563c3..2a6d5cf31 100644
--- a/src/conversion/downcast-converters.js
+++ b/src/conversion/downcast-converters.js
@@ -810,6 +810,10 @@ export function highlightText( highlightDescriptor ) {
 			for ( const element of rangeAfterWrap.getItems() ) {
 				if ( element.is( 'attributeElement' ) && element.isSimilar( viewElement ) ) {
 					conversionApi.mapper.bindElementToMarker( element, data.markerName );
+
+					// One attribute element is enough, because all of them are bound together by the view writer.
+					// Mapper uses this binding to get all the elements no matter how many of them are registered in the mapper.
+					break;
 				}
 			}
 		}
@@ -925,23 +929,9 @@ export function removeHighlight( highlightDescriptor ) {
 
 		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 );
-					}
-				}
+				conversionApi.writer.unwrap( ViewRange.createOn( element ), viewHighlightElement );
 			} else {
-				// If the bound element is a ContainerElement, just use `removeHighlight` function on it.
+				// if element.is( 'containerElement' ).
 				element.getCustomProperty( 'removeHighlight' )( element, descriptor.id, conversionApi.writer );
 			}
 		}
diff --git a/src/conversion/mapper.js b/src/conversion/mapper.js
index 22b955d43..3c530763e 100644
--- a/src/conversion/mapper.js
+++ b/src/conversion/mapper.js
@@ -280,10 +280,29 @@ export default class Mapper {
 	 * Gets all view elements bound to the given marker name.
 	 *
 	 * @param {String} name Marker name.
-	 * @returns {Set.<module:engine/view/element~Element>} View elements bound with given marker name.
+	 * @returns {Set.<module:engine/view/element~Element>|null} View elements bound with given marker name or `null`
+	 * if no elements are bound to given marker name.
 	 */
 	markerNameToElements( name ) {
-		return this._markerNameToElements.get( name );
+		const boundElements = this._markerNameToElements.get( name );
+
+		if ( !boundElements ) {
+			return null;
+		}
+
+		const elements = new Set();
+
+		for ( const element of boundElements ) {
+			if ( element.is( 'attributeElement' ) ) {
+				for ( const clone of element.getCustomProperty( 'clonedElements' ) ) {
+					elements.add( clone );
+				}
+			} else {
+				elements.add( element );
+			}
+		}
+
+		return elements;
 	}
 
 	/**
diff --git a/src/view/writer.js b/src/view/writer.js
index 4aab8bf76..0701269b7 100644
--- a/src/view/writer.js
+++ b/src/view/writer.js
@@ -32,6 +32,15 @@ export default class Writer {
 		 * @type {module:engine/view/document~Document}
 		 */
 		this.document = document;
+
+		/**
+		 * Holds references to the attribute groups that share the same {@link module:engine/view/attributeelement~AttributeElement#id id}.
+		 * The keys are `id`s, the values are `Set`s holding {@link module:engine/view/attributeelement~AttributeElement}s.
+		 *
+		 * @private
+		 * @type {Map}
+		 */
+		this._cloneGroups = new Map();
 	}
 
 	/**
@@ -503,9 +512,9 @@ export default class Writer {
 		if ( positionParent.is( 'attributeElement' ) && positionParent.childCount === 0 ) {
 			const parent = positionParent.parent;
 			const offset = positionParent.index;
-			positionParent._remove();
 
-			this._unlinkBrokenAttributeElement( positionParent );
+			positionParent._remove();
+			this._removeFromClonedElementsGroup( positionParent );
 
 			return this.mergeAttributes( new Position( parent, offset ) );
 		}
@@ -529,7 +538,7 @@ export default class Writer {
 			nodeBefore._appendChildren( nodeAfter.getChildren() );
 
 			nodeAfter._remove();
-			this._unlinkBrokenAttributeElement( nodeAfter );
+			this._removeFromClonedElementsGroup( nodeAfter );
 
 			// New position is located inside the first node, before new nodes.
 			// Call this method recursively to merge again if needed.
@@ -617,8 +626,12 @@ export default class Writer {
 		}
 
 		const insertionPosition = this._breakAttributes( position, true );
-
 		const length = container._insertChildren( insertionPosition.offset, nodes );
+
+		for ( const node of nodes ) {
+			this._addToClonedElementsGroup( node );
+		}
+
 		const endPosition = insertionPosition.getShiftedBy( length );
 		const start = this.mergeAttributes( insertionPosition );
 
@@ -666,7 +679,7 @@ export default class Writer {
 		const removed = parentContainer._removeChildren( breakStart.offset, count );
 
 		for ( const node of removed ) {
-			this._unlinkBrokenAttributeElement( node );
+			this._removeFromClonedElementsGroup( node );
 		}
 
 		// Merge after removing.
@@ -912,21 +925,22 @@ export default class Writer {
 	}
 
 	/**
-	 * For the given attribute element, returns that element and all the elements that were cloned from the given element
-	 * when the element was broken by `Writer`.
+	 * For given {@link module:engine/view/attributeelement~AttributeElement attribute element}, returns all other
+	 * attribute elements with the same {@link module:engine/view/attributeelement~AttributeElement#id id} that
+	 * were inserted by `Writer` to the view tree.
 	 *
 	 * @param {module:engine/view/attributeelement~AttributeElement} element
-	 * @returns {Array.<module:engine/view/attributeelement~AttributeElement>}
+	 * @returns {Set.<module:engine/view/attributeelement~AttributeElement>|null} Set containing all the attribute elements
+	 * with the same `id` or `null` if given element had no `id`.
 	 */
-	getAllBrokenSiblings( element ) {
-		const original = element.getCustomProperty( 'originalAttributeElement' ) || element;
-		const clones = original.getCustomProperty( 'clonedAttributeElements' );
+	getAllClonedElements( element ) {
+		const id = element.id;
 
-		if ( clones ) {
-			return Array.from( clones ).concat( original );
-		} else {
-			return [ element ];
+		if ( !id ) {
+			return null;
 		}
+
+		return this._cloneGroups.get( id );
 	}
 
 	/**
@@ -954,6 +968,7 @@ export default class Writer {
 			if ( isText || isEmpty || isUI || ( isAttribute && shouldABeOutsideB( attribute, child ) ) ) {
 				// Clone attribute.
 				const newAttribute = attribute._clone();
+				this._addToClonedElementsGroup( newAttribute );
 
 				// Wrap current node with new attribute.
 				child._remove();
@@ -1018,7 +1033,7 @@ export default class Writer {
 
 				// Replace wrapper element with its children
 				child._remove();
-				this._unlinkBrokenAttributeElement( child );
+				this._removeFromClonedElementsGroup( child );
 
 				parent._insertChildren( i, unwrapped );
 
@@ -1314,6 +1329,15 @@ export default class Writer {
 		return true;
 	}
 
+	/**
+	 * Helper function used by other `Writer` methods. Breaks attribute elements at the boundaries of given range.
+	 *
+	 * @private
+	 * @param {module:engine/view/range~Range} range Range which `start` and `end` positions will be used to break attributes.
+	 * @param {Boolean} [forceSplitText=false] If set to `true`, will break text nodes even if they are directly in container element.
+	 * This behavior will result in incorrect view state, but is needed by other view writing methods which then fixes view state.
+	 * @returns {module:engine/view/range~Range} New range with located at break positions.
+	 */
 	_breakAttributesRange( range, forceSplitText = false ) {
 		const rangeStart = range.start;
 		const rangeEnd = range.end;
@@ -1337,6 +1361,21 @@ export default class Writer {
 		return new Range( breakStart, breakEnd );
 	}
 
+	/**
+	 * Helper function used by other `Writer` methods. Breaks attribute elements at given position.
+	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-writer-cannot-break-empty-element` when break position
+	 * is placed inside {@link module:engine/view/emptyelement~EmptyElement EmptyElement}.
+	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `view-writer-cannot-break-ui-element` when break position
+	 * is placed inside {@link module:engine/view/uielement~UIElement UIElement}.
+	 *
+	 * @private
+	 * @param {module:engine/view/position~Position} position Position where to break attributes.
+	 * @param {Boolean} [forceSplitText=false] If set to `true`, will break text nodes even if they are directly in container element.
+	 * This behavior will result in incorrect view state, but is needed by other view writing methods which then fixes view state.
+	 * @returns {module:engine/view/position~Position} New position after breaking the attributes.
+	 */
 	_breakAttributes( position, forceSplitText = false ) {
 		const positionOffset = position.offset;
 		const positionParent = position.parent;
@@ -1403,9 +1442,7 @@ export default class Writer {
 
 			// Break element.
 			const clonedNode = positionParent._clone();
-
-			// Save that the `clonedNode` was created from `positionParent`.
-			this._linkBrokenAttributeElements( clonedNode, positionParent );
+			this._addToClonedElementsGroup( clonedNode );
 
 			// Insert cloned node to position's parent node.
 			positionParent.parent._insertChildren( offsetAfter, clonedNode );
@@ -1425,42 +1462,65 @@ export default class Writer {
 	}
 
 	/**
-	 * Links `clonedNode` element to `clonedFrom` element. Saves information that `clonedNode` was created as a clone
-	 * of `clonedFrom` when `clonedFrom` was broken into two elements by the `Writer`.
+	 * Stores the information that an {@link module:engine/view/attributeelement~AttributeElement attribute element} was
+	 * added to the tree. Saves the reference to the group in the given element and updates the group, so other elements
+	 * from the group now keep a reference to the given attribute element.
+	 *
+	 * The clones group can be accessed from the element, under `clonedElements` custom property:
+	 *
+	 *		attributeElement.getCustomProperty( 'clonedElements' );
+	 *
+	 * Does nothing if added element has no {@link module:engine/view/attributeelement~AttributeElement#id id}.
 	 *
 	 * @private
-	 * @param {module:engine/view/attributeelement~AttributeElement} clonedNode
-	 * @param {module:engine/view/attributeelement~AttributeElement} clonedFrom
+	 * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to save.
 	 */
-	_linkBrokenAttributeElements( clonedNode, clonedFrom ) {
-		const original = clonedFrom.getCustomProperty( 'originalAttributeElement' ) || clonedFrom;
-		const clones = original.getCustomProperty( 'clonedAttributeElements' ) || new Set();
+	_addToClonedElementsGroup( element ) {
+		const id = element.id;
 
-		this.setCustomProperty( 'originalAttributeElement', original, clonedNode );
+		if ( !id ) {
+			return;
+		}
+
+		let group = this._cloneGroups.get( id );
+
+		if ( !group ) {
+			group = new Set();
+			this._cloneGroups.set( id, group );
+		}
 
-		clones.add( clonedNode );
-		this.setCustomProperty( 'clonedAttributeElements', clones, original );
+		group.add( element );
+		this.setCustomProperty( 'clonedElements', group, element );
 	}
 
 	/**
-	 * Unlinks `element`. Removes information about how `element` was broken by the `Writer` from `element` and from
-	 * its original element (the element it was cloned from).
+	 * Removes all the information about the given {@link module:engine/view/attributeelement~AttributeElement attribute element}
+	 * from its clones group.
+	 *
+	 * Keep in mind, that the element will still keep a reference to the group (but the group will not keep a reference to it).
+	 * This allows to reference the whole group even if the element was already removed from the tree.
+	 *
+	 * Does nothing if the element has no {@link module:engine/view/attributeelement~AttributeElement#id id}.
 	 *
 	 * @private
-	 * @param {module:engine/view/attributeelement~AttributeElement} element
+	 * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to remove.
 	 */
-	_unlinkBrokenAttributeElement( element ) {
-		if ( !element.is( 'element' ) ) {
+	_removeFromClonedElementsGroup( element ) {
+		const id = element.id;
+
+		if ( !id ) {
 			return;
 		}
 
-		const original = element.getCustomProperty( 'originalAttributeElement' );
+		const group = this._cloneGroups.get( id );
 
-		if ( original ) {
-			this.removeCustomProperty( 'originalAttributeElement', element );
+		group.delete( element );
+		// Not removing group from element on purpose!
+		// If other parts of code have reference to this element, they will be able to get references to other elements from the group.
+		// If all other elements are removed from the set, everything will be garbage collected.
 
-			const clones = original.getCustomProperty( 'clonedAttributeElements' );
-			clones.delete( element );
+		if ( group.size === 0 ) {
+			this._cloneGroups.delete( id );
 		}
 	}
 }
diff --git a/tests/conversion/mapper.js b/tests/conversion/mapper.js
index 9414bcbc2..0c35aa078 100644
--- a/tests/conversion/mapper.js
+++ b/tests/conversion/mapper.js
@@ -656,7 +656,7 @@ describe( 'Mapper', () => {
 
 			const elements = mapper.markerNameToElements( 'marker' );
 
-			expect( elements ).to.be.undefined;
+			expect( elements ).to.be.null;
 		} );
 	} );
 
diff --git a/tests/view/writer/writer.js b/tests/view/writer/writer.js
index 776122444..96c9f6f28 100644
--- a/tests/view/writer/writer.js
+++ b/tests/view/writer/writer.js
@@ -263,14 +263,14 @@ describe( 'Writer', () => {
 		} );
 	} );
 
-	describe( 'getAllBrokenSiblings()', () => {
-		it( 'should return all clones of a broken attribute element', () => {
+	describe( 'getAllClonedElements()', () => {
+		it( 'should return all clones of a broken attribute element with id', () => {
 			const container = writer.createContainerElement( 'div' );
 			const text = writer.createText( 'abccccde' );
 
 			writer.insert( ViewPosition.createAt( container, 0 ), text );
 
-			const span = writer.createAttributeElement( 'span' );
+			const span = writer.createAttributeElement( 'span', null, { id: 'foo' } );
 			span._priority = 20;
 
 			// <div>ab<span>cccc</span>de</div>
@@ -287,7 +287,7 @@ describe( 'Writer', () => {
 				i
 			);
 
-			// <div>a<i>b<span>c</span></i><span>c</span><i><span>c</span>d</i>e</div>
+			// <div>a<i>b<span>c</span></i><span>c</span><i><span>cc</span>d</i>e</div>
 			writer.wrap(
 				ViewRange.createFromParentsAndOffsets(
 					container.getChild( 2 ).getChild( 0 ), 1,
@@ -301,11 +301,8 @@ describe( 'Writer', () => {
 
 			// For each of the spans created above...
 			for ( const oneOfAllSpans of allSpans ) {
-				// Find all broken siblings of that span...
-				const brokenArray = writer.getAllBrokenSiblings( oneOfAllSpans );
-
-				// Convert to set because we don't care about order.
-				const brokenSet = new Set( brokenArray );
+				const brokenSet = writer.getAllClonedElements( oneOfAllSpans );
+				const brokenArray = Array.from( brokenSet );
 
 				// Check if all spans are included.
 				for ( const s of allSpans ) {
@@ -315,6 +312,38 @@ describe( 'Writer', () => {
 				expect( brokenArray.length ).to.equal( allSpans.length );
 			}
 		} );
+
+		it( 'should return null if an element without id is given (even if it was broken)', () => {
+			const container = writer.createContainerElement( 'div' );
+			const text = writer.createText( 'abccccde' );
+
+			writer.insert( ViewPosition.createAt( container, 0 ), text );
+
+			const span = writer.createAttributeElement( 'span' );
+			span._priority = 20;
+
+			// <div>ab<span>cccc</span>de</div>
+			writer.wrap( ViewRange.createFromParentsAndOffsets( text, 2, text, 6 ), span );
+
+			const i = writer.createAttributeElement( 'i' );
+
+			// <div>a<i>b<span>c</span></i><span>cc</span>de</div>
+			writer.wrap(
+				ViewRange.createFromParentsAndOffsets(
+					container.getChild( 0 ), 1,
+					container.getChild( 1 ).getChild( 0 ), 1
+				),
+				i
+			);
+
+			const spanFromView = container.getChild( 2 );
+
+			// Make sure a proper element was taken.
+			expect( spanFromView.is( 'span' ) ).to.be.true;
+			expect( spanFromView.id ).to.be.null;
+
+			expect( writer.getAllClonedElements( spanFromView ) ).to.be.null;
+		} );
 	} );
 
 	function assertElementAttributes( element, attributes ) {

From 8d39d367c855f7b96ef6be619a6f6fe716b309d4 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Fri, 16 Mar 2018 15:44:14 +0100
Subject: [PATCH 12/13] Changed: Moved `getAllClonedElements` from
 `view.Writer` to `view.AttributeElement`.

---
 src/conversion/mapper.js       |  2 +-
 src/view/attributeelement.js   | 50 ++++++++++++++++++++++++++++++----
 src/view/writer.js             | 25 ++---------------
 tests/view/attributeelement.js | 26 ++++++++++++++++++
 tests/view/writer/writer.js    | 36 ++----------------------
 5 files changed, 76 insertions(+), 63 deletions(-)

diff --git a/src/conversion/mapper.js b/src/conversion/mapper.js
index 3c530763e..26c179ebd 100644
--- a/src/conversion/mapper.js
+++ b/src/conversion/mapper.js
@@ -294,7 +294,7 @@ export default class Mapper {
 
 		for ( const element of boundElements ) {
 			if ( element.is( 'attributeElement' ) ) {
-				for ( const clone of element.getCustomProperty( 'clonedElements' ) ) {
+				for ( const clone of element.getElementsWithSameId() ) {
 					elements.add( clone );
 				}
 			} else {
diff --git a/src/view/attributeelement.js b/src/view/attributeelement.js
index 9deddd3b2..5e22ae7a0 100644
--- a/src/view/attributeelement.js
+++ b/src/view/attributeelement.js
@@ -8,6 +8,7 @@
  */
 
 import Element from './element';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 // Default attribute priority.
 const DEFAULT_PRIORITY = 10;
@@ -33,6 +34,14 @@ export default class AttributeElement extends Element {
 	constructor( name, attrs, children ) {
 		super( name, attrs, children );
 
+		/**
+		 * Returns block {@link module:engine/view/filler filler} offset or `null` if block filler is not needed.
+		 *
+		 * @method #getFillerOffset
+		 * @returns {Number|null} Block filler offset or `null` if block filler is not needed.
+		 */
+		this.getFillerOffset = getFillerOffset;
+
 		/**
 		 * Element priority. Decides in what order elements are wrapped by {@link module:engine/view/writer~Writer}.
 		 *
@@ -51,12 +60,15 @@ export default class AttributeElement extends Element {
 		this._id = null;
 
 		/**
-		 * Returns block {@link module:engine/view/filler filler} offset or `null` if block filler is not needed.
+		 * Keeps all the attribute elements that have the same {@link module:engine/view/attributeelement~AttributeElement#id ids}
+		 * and still exist in the view tree.
 		 *
-		 * @method #getFillerOffset
-		 * @returns {Number|null} Block filler offset or `null` if block filler is not needed.
+		 * This property is managed by {@link module:engine/view/writer~Writer}.
+		 *
+		 * @protected
+		 * @member {Set|null}
 		 */
-		this.getFillerOffset = getFillerOffset;
+		this._clonesGroup = null;
 	}
 
 	/**
@@ -71,7 +83,7 @@ export default class AttributeElement extends Element {
 
 	/**
 	 * Element identifier. If set, it is used by {@link module:engine/view/element~Element#isSimilar},
-	 * and then two elements are considered similar if, and only if they have the same `_id`.
+	 * and then two elements are considered similar if, and only if they have the same `id`.
 	 *
 	 * @readonly
 	 * @returns {String|Number}
@@ -80,6 +92,34 @@ export default class AttributeElement extends Element {
 		return this._id;
 	}
 
+	/**
+	 * Returns all {@link module:engine/view/attributeelement~AttributeElement attribute elements} that has the
+	 * same {@link module:engine/view/attributeelement~AttributeElement#id id} and are in the view tree (were not removed).
+	 *
+	 * Note: If this element has been removed from the tree, returned set will not include it.
+	 *
+	 * Throws {@link module:utils/ckeditorerror~CKEditorError attribute-element-get-elements-with-same-id-no-id}
+	 * if this element has no `id`.
+	 *
+	 * @returns {Set.<module:engine/view/attributeelement~AttributeElement>|null} Set containing all the attribute elements
+	 * with the same `id` that were added and not removed from the view tree.
+	 */
+	getElementsWithSameId() {
+		if ( this.id === null ) {
+			/**
+			 * Cannot get elements with the same id for an attribute element without id.
+			 *
+			 * @error attribute-element-get-elements-with-same-id-no-id
+			 */
+			throw new CKEditorError(
+				'attribute-element-get-elements-with-same-id-no-id: ' +
+				'Cannot get elements with the same id for an attribute element without id.'
+			);
+		}
+
+		return new Set( this._clonesGroup );
+	}
+
 	/**
 	 * @inheritDoc
 	 */
diff --git a/src/view/writer.js b/src/view/writer.js
index 0701269b7..7ddd6c775 100644
--- a/src/view/writer.js
+++ b/src/view/writer.js
@@ -924,25 +924,6 @@ export default class Writer {
 		return newElement;
 	}
 
-	/**
-	 * For given {@link module:engine/view/attributeelement~AttributeElement attribute element}, returns all other
-	 * attribute elements with the same {@link module:engine/view/attributeelement~AttributeElement#id id} that
-	 * were inserted by `Writer` to the view tree.
-	 *
-	 * @param {module:engine/view/attributeelement~AttributeElement} element
-	 * @returns {Set.<module:engine/view/attributeelement~AttributeElement>|null} Set containing all the attribute elements
-	 * with the same `id` or `null` if given element had no `id`.
-	 */
-	getAllClonedElements( element ) {
-		const id = element.id;
-
-		if ( !id ) {
-			return null;
-		}
-
-		return this._cloneGroups.get( id );
-	}
-
 	/**
 	 * Wraps children with provided `attribute`. Only children contained in `parent` element between
 	 * `startOffset` and `endOffset` will be wrapped.
@@ -1466,9 +1447,7 @@ export default class Writer {
 	 * added to the tree. Saves the reference to the group in the given element and updates the group, so other elements
 	 * from the group now keep a reference to the given attribute element.
 	 *
-	 * The clones group can be accessed from the element, under `clonedElements` custom property:
-	 *
-	 *		attributeElement.getCustomProperty( 'clonedElements' );
+	 * The clones group can be obtained using {@link module:engine/view/attributeelement~AttributeElement#getElementsWithSameId}.
 	 *
 	 * Does nothing if added element has no {@link module:engine/view/attributeelement~AttributeElement#id id}.
 	 *
@@ -1490,7 +1469,7 @@ export default class Writer {
 		}
 
 		group.add( element );
-		this.setCustomProperty( 'clonedElements', group, element );
+		element._clonesGroup = group;
 	}
 
 	/**
diff --git a/tests/view/attributeelement.js b/tests/view/attributeelement.js
index f2aff5b77..e73c219f6 100644
--- a/tests/view/attributeelement.js
+++ b/tests/view/attributeelement.js
@@ -6,6 +6,7 @@
 import AttributeElement from '../../src/view/attributeelement';
 import Element from '../../src/view/element';
 import { parse } from '../../src/dev-utils/view';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 describe( 'AttributeElement', () => {
 	describe( 'constructor()', () => {
@@ -108,6 +109,31 @@ describe( 'AttributeElement', () => {
 		} );
 	} );
 
+	// More tests are available in Writer tests.
+	describe( 'getElementsWithSameId', () => {
+		it( 'should return a copy of _clonesGroup set', () => {
+			const attributeA = new AttributeElement( 'b' );
+			const attributeB = new AttributeElement( 'b' );
+
+			attributeA._id = 'foo';
+			attributeB._id = 'foo';
+
+			attributeA._clonesGroup = attributeB._clonesGroup = new Set( [ attributeA, attributeB ] );
+
+			expect( attributeA.getElementsWithSameId() ).to.deep.equal( attributeA._clonesGroup );
+			expect( attributeA.getElementsWithSameId() ).not.to.equal( attributeA._clonesGroup );
+			expect( attributeA.getElementsWithSameId() ).to.deep.equal( attributeB.getElementsWithSameId() );
+		} );
+
+		it( 'should throw if attribute element has no id', () => {
+			const attribute = new AttributeElement( 'b' );
+
+			expect( () => {
+				attribute.getElementsWithSameId();
+			} ).to.throw( CKEditorError, /attribute-element-get-elements-with-same-id-no-id/ );
+		} );
+	} );
+
 	describe( 'getFillerOffset', () => {
 		it( 'should return position 0 if it is the only element in the container', () => {
 			const { selection } = parse( '<container:p><attribute:b>[]</attribute:b></container:p>' );
diff --git a/tests/view/writer/writer.js b/tests/view/writer/writer.js
index 96c9f6f28..200eda2f8 100644
--- a/tests/view/writer/writer.js
+++ b/tests/view/writer/writer.js
@@ -263,7 +263,7 @@ describe( 'Writer', () => {
 		} );
 	} );
 
-	describe( 'getAllClonedElements()', () => {
+	describe( 'manages AttributeElement#_clonesGroup', () => {
 		it( 'should return all clones of a broken attribute element with id', () => {
 			const container = writer.createContainerElement( 'div' );
 			const text = writer.createText( 'abccccde' );
@@ -301,7 +301,7 @@ describe( 'Writer', () => {
 
 			// For each of the spans created above...
 			for ( const oneOfAllSpans of allSpans ) {
-				const brokenSet = writer.getAllClonedElements( oneOfAllSpans );
+				const brokenSet = oneOfAllSpans.getElementsWithSameId();
 				const brokenArray = Array.from( brokenSet );
 
 				// Check if all spans are included.
@@ -312,38 +312,6 @@ describe( 'Writer', () => {
 				expect( brokenArray.length ).to.equal( allSpans.length );
 			}
 		} );
-
-		it( 'should return null if an element without id is given (even if it was broken)', () => {
-			const container = writer.createContainerElement( 'div' );
-			const text = writer.createText( 'abccccde' );
-
-			writer.insert( ViewPosition.createAt( container, 0 ), text );
-
-			const span = writer.createAttributeElement( 'span' );
-			span._priority = 20;
-
-			// <div>ab<span>cccc</span>de</div>
-			writer.wrap( ViewRange.createFromParentsAndOffsets( text, 2, text, 6 ), span );
-
-			const i = writer.createAttributeElement( 'i' );
-
-			// <div>a<i>b<span>c</span></i><span>cc</span>de</div>
-			writer.wrap(
-				ViewRange.createFromParentsAndOffsets(
-					container.getChild( 0 ), 1,
-					container.getChild( 1 ).getChild( 0 ), 1
-				),
-				i
-			);
-
-			const spanFromView = container.getChild( 2 );
-
-			// Make sure a proper element was taken.
-			expect( spanFromView.is( 'span' ) ).to.be.true;
-			expect( spanFromView.id ).to.be.null;
-
-			expect( writer.getAllClonedElements( spanFromView ) ).to.be.null;
-		} );
 	} );
 
 	function assertElementAttributes( element, attributes ) {

From a721f95601511a6fcbb7d43becb957af64f2d157 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Fri, 16 Mar 2018 15:48:02 +0100
Subject: [PATCH 13/13] Docs: Fixed mistake in docs.

---
 src/view/attributeelement.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/view/attributeelement.js b/src/view/attributeelement.js
index 5e22ae7a0..392253335 100644
--- a/src/view/attributeelement.js
+++ b/src/view/attributeelement.js
@@ -101,7 +101,7 @@ export default class AttributeElement extends Element {
 	 * Throws {@link module:utils/ckeditorerror~CKEditorError attribute-element-get-elements-with-same-id-no-id}
 	 * if this element has no `id`.
 	 *
-	 * @returns {Set.<module:engine/view/attributeelement~AttributeElement>|null} Set containing all the attribute elements
+	 * @returns {Set.<module:engine/view/attributeelement~AttributeElement>} Set containing all the attribute elements
 	 * with the same `id` that were added and not removed from the view tree.
 	 */
 	getElementsWithSameId() {