From f9eabee9f23861bac10cf5d3a7241fde71e7b4d6 Mon Sep 17 00:00:00 2001
From: Szymon Cofalik <s.cofalik@cksource.com>
Date: Wed, 3 Oct 2018 11:13:25 +0200
Subject: [PATCH] Changed: Remove clone groups in `view.DowncastWriter`
 manually.

---
 src/conversion/downcast-converters.js |  8 ++++---
 src/view/downcastwriter.js            | 25 ++++++++++++++++-----
 tests/view/downcastwriter/move.js     | 32 +++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/src/conversion/downcast-converters.js b/src/conversion/downcast-converters.js
index 4777b72d1..43e5a20a9 100644
--- a/src/conversion/downcast-converters.js
+++ b/src/conversion/downcast-converters.js
@@ -625,12 +625,12 @@ export function removeUIElement() {
 
 		conversionApi.mapper.unbindElementsFromMarkerName( data.markerName );
 
-		const viewWriter = conversionApi.writer;
-
 		for ( const element of elements ) {
-			viewWriter.clear( ViewRange.createOn( element ), element );
+			conversionApi.writer.clear( ViewRange.createOn( element ), element );
 		}
 
+		conversionApi.writer.clearClonedElementsGroup( data.markerName );
+
 		evt.stop();
 	};
 }
@@ -970,6 +970,8 @@ export function removeHighlight( highlightDescriptor ) {
 			}
 		}
 
+		conversionApi.writer.clearClonedElementsGroup( data.markerName );
+
 		evt.stop();
 	};
 }
diff --git a/src/view/downcastwriter.js b/src/view/downcastwriter.js
index f3651fc66..9645683e1 100644
--- a/src/view/downcastwriter.js
+++ b/src/view/downcastwriter.js
@@ -40,7 +40,7 @@ export default class DowncastWriter {
 		 * The keys are `id`s, the values are `Set`s holding {@link module:engine/view/attributeelement~AttributeElement}s.
 		 *
 		 * @private
-		 * @type {Map}
+		 * @type {Map.<String,Set>}
 		 */
 		this._cloneGroups = new Map();
 	}
@@ -921,6 +921,24 @@ export default class DowncastWriter {
 		return newElement;
 	}
 
+	/**
+	 * Cleans up memory by removing obsolete cloned elements group from the writer.
+	 *
+	 * Should be used whenever all {@link module:engine/view/attributeelement~AttributeElement attribute elements}
+	 * with the same {@link module:engine/view/attributeelement~AttributeElement#id id} are going to be removed from the view and
+	 * the group will no longer be needed.
+	 *
+	 * Cloned elements group are not removed automatically in case if the group is still needed after all its elements
+	 * were removed from the view.
+	 *
+	 * Keep in mind that group names are equal to the `id` property of the attribute element.
+	 *
+	 * @param {String} groupName Name of the group to clear.
+	 */
+	clearClonedElementsGroup( groupName ) {
+		this._cloneGroups.delete( groupName );
+	}
+
 	/**
 	 * Wraps children with provided `attribute`. Only children contained in `parent` element between
 	 * `startOffset` and `endOffset` will be wrapped.
@@ -1519,11 +1537,6 @@ export default class DowncastWriter {
 		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.
-
-		if ( group.size === 0 ) {
-			this._cloneGroups.delete( id );
-		}
 	}
 }
 
diff --git a/tests/view/downcastwriter/move.js b/tests/view/downcastwriter/move.js
index c9344fc8a..761b36ad1 100644
--- a/tests/view/downcastwriter/move.js
+++ b/tests/view/downcastwriter/move.js
@@ -7,12 +7,14 @@ import DowncastWriter from '../../../src/view/downcastwriter';
 import { stringify, parse } from '../../../src/dev-utils/view';
 import ContainerElement from '../../../src/view/containerelement';
 import AttributeElement from '../../../src/view/attributeelement';
+import RootEditableElement from '../../../src/view/rooteditableelement';
 import EmptyElement from '../../../src/view/emptyelement';
 import UIElement from '../../../src/view/uielement';
 import Range from '../../../src/view/range';
 import Position from '../../../src/view/position';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import Document from '../../../src/view/document';
+import Mapper from '../../../src/conversion/mapper';
 
 describe( 'DowncastWriter', () => {
 	describe( 'move()', () => {
@@ -180,5 +182,35 @@ describe( 'DowncastWriter', () => {
 				writer.move( srcRange, dstPosition );
 			} ).to.throw( CKEditorError, 'view-writer-cannot-break-ui-element' );
 		} );
+
+		it( 'should not break marker mappings if marker element was split and the original element was removed', () => {
+			const mapper = new Mapper();
+
+			const srcContainer = new ContainerElement( 'p' );
+			const dstContainer = new ContainerElement( 'p' );
+
+			const root = new RootEditableElement( 'div' );
+			root._appendChild( [ srcContainer, dstContainer ] );
+
+			const attrElemA = new AttributeElement( 'span' );
+			attrElemA._id = 'foo';
+
+			const attrElemB = new AttributeElement( 'span' );
+			attrElemB._id = 'foo';
+
+			writer.insert( new Position( srcContainer, 0 ), [ attrElemA, attrElemB ] );
+
+			mapper.bindElementToMarker( attrElemA, 'foo' );
+
+			expect( mapper.markerNameToElements( 'foo' ).size ).to.equal( 2 );
+
+			writer.remove( Range.createOn( attrElemA ) );
+
+			expect( mapper.markerNameToElements( 'foo' ).size ).to.equal( 1 );
+
+			writer.move( Range.createOn( attrElemB ), new Position( dstContainer, 0 ) );
+
+			expect( mapper.markerNameToElements( 'foo' ).size ).to.equal( 1 );
+		} );
 	} );
 } );