Merge pull request #1572 from ckeditor/t/1571

Fix: Remove clone groups in `view.DowncastWriter` manually. Closes #1571.

Author: Piotr Jasiun

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

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

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