Merge pull request #1403 from ckeditor/t/1401

Fix: `view.Writer` should deeply add and remove `view.AttributeElement`s to/from their clone groups. Closes #1401.

src/view/writer.js

@@ -941,12 +941,13 @@ 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();
 				newAttribute._appendChild( child );
+
 				parent._insertChild( i, newAttribute );
+				this._addToClonedElementsGroup( newAttribute );
 
 				wrapPositions.push(	new Position( parent, i ) );
 			}
@@ -1006,10 +1007,10 @@ export default class Writer {
 
 				// Replace wrapper element with its children
 				child._remove();
-				this._removeFromClonedElementsGroup( child );
-
 				parent._insertChild( i, unwrapped );
 
+				this._removeFromClonedElementsGroup( child );
+
 				// Save start and end position of moved items.
 				unwrapPositions.push(
 					new Position( parent, i ),
@@ -1415,10 +1416,10 @@ export default class Writer {
 
 			// Break element.
 			const clonedNode = positionParent._clone();
-			this._addToClonedElementsGroup( clonedNode );
 
 			// Insert cloned node to position's parent node.
 			positionParent.parent._insertChild( offsetAfter, clonedNode );
+			this._addToClonedElementsGroup( clonedNode );
 
 			// Get nodes to move.
 			const count = positionParent.childCount - positionOffset;
@@ -1447,6 +1448,19 @@ export default class Writer {
 	 * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to save.
 	 */
 	_addToClonedElementsGroup( element ) {
+		// Add only if the element is in document tree.
+		if ( !element.root.is( 'rootElement' ) ) {
+			return;
+		}
+
+		// Traverse the element's children recursively to find other attribute elements that also might got inserted.
+		// The loop is at the beginning so we can make fast returns later in the code.
+		if ( element.is( 'element' ) ) {
+			for ( const child of element.getChildren() ) {
+				this._addToClonedElementsGroup( child );
+			}
+		}
+
 		const id = element.id;
 
 		if ( !id ) {
@@ -1477,6 +1491,14 @@ export default class Writer {
 	 * @param {module:engine/view/attributeelement~AttributeElement} element Attribute element to remove.
 	 */
 	_removeFromClonedElementsGroup( element ) {
+		// Traverse the element's children recursively to find other attribute elements that also got removed.
+		// The loop is at the beginning so we can make fast returns later in the code.
+		if ( element.is( 'element' ) ) {
+			for ( const child of element.getChildren() ) {
+				this._removeFromClonedElementsGroup( child );
+			}
+		}
+
 		const id = element.id;
 
 		if ( !id ) {
@@ -1485,6 +1507,10 @@ export default class Writer {
 
 		const group = this._cloneGroups.get( id );
 
+		if ( !group ) {
+			return;
+		}
+
 		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.

tests/view/writer/writer.js

@@ -13,7 +13,7 @@ import createViewRoot from '../_utils/createroot';
 describe( 'Writer', () => {
 	let writer, attributes, root, doc;
 
-	before( () => {
+	beforeEach( () => {
 		attributes = { foo: 'bar', baz: 'quz' };
 		doc = new Document();
 		root = createViewRoot( doc );
@@ -43,6 +43,9 @@ describe( 'Writer', () => {
 
 	describe( 'setSelectionFocus()', () => {
 		it( 'should use selection._setFocus method internally', () => {
+			const position = ViewPosition.createAt( root );
+			writer.setSelection( position );
+
 			const spy = sinon.spy( writer.document.selection, '_setFocus' );
 			writer.setSelectionFocus( root, 0 );
 
@@ -255,10 +258,9 @@ describe( 'Writer', () => {
 
 	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' );
 
-			writer.insert( ViewPosition.createAt( container, 0 ), text );
+			writer.insert( ViewPosition.createAt( root, 0 ), text );
 
 			const span = writer.createAttributeElement( 'span', null, { id: 'foo' } );
 			span._priority = 20;
@@ -271,23 +273,23 @@ describe( 'Writer', () => {
 			// <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
+					root.getChild( 0 ), 1,
+					root.getChild( 1 ).getChild( 0 ), 1
 				),
 				i
 			);
 
 			// <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,
-					container.getChild( 3 ), 1
+					root.getChild( 2 ).getChild( 0 ), 1,
+					root.getChild( 3 ), 1
 				),
 				i
 			);
 
 			// Find all spans.
-			const allSpans = Array.from( ViewRange.createIn( container ).getItems() ).filter( element => element.is( 'span' ) );
+			const allSpans = Array.from( ViewRange.createIn( root ).getItems() ).filter( element => element.is( 'span' ) );
 
 			// For each of the spans created above...
 			for ( const oneOfAllSpans of allSpans ) {
@@ -302,6 +304,72 @@ describe( 'Writer', () => {
 				expect( brokenArray.length ).to.equal( allSpans.length );
 			}
 		} );
+
+		it( 'should not create groups for attribute elements that are not created in document root', () => {
+			const p = writer.createContainerElement( 'p' );
+			const foo = writer.createText( 'foo' );
+			writer.insert( ViewPosition.createAt( p, 0 ), foo );
+			// <p>foo</p>
+
+			const span = writer.createAttributeElement( 'span', null, { id: 'span' } );
+
+			// <p><span>foo</span></p>
+			writer.wrap( ViewRange.createFromParentsAndOffsets( foo, 0, foo, 3 ), span );
+
+			// Find the span.
+			const createdSpan = p.getChild( 0 );
+
+			expect( createdSpan.getElementsWithSameId().size ).to.equal( 0 );
+		} );
+
+		it( 'should add attribute elements to clone groups deeply', () => {
+			const p = writer.createContainerElement( 'p' );
+			const foo = writer.createText( 'foo' );
+			writer.insert( ViewPosition.createAt( p, 0 ), foo );
+			// <p>foo</p>
+
+			const span = writer.createAttributeElement( 'span', null, { id: 'span' } );
+
+			// <p><span>foo</span></p>
+			writer.wrap( ViewRange.createFromParentsAndOffsets( foo, 0, foo, 3 ), span );
+
+			// <div><p><span>foo</span></p>
+			writer.insert( ViewPosition.createAt( root, 0 ), p );
+
+			// Find the span.
+			const createdSpan = p.getChild( 0 );
+
+			expect( Array.from( createdSpan.getElementsWithSameId() ) ).to.deep.equal( [ createdSpan ] );
+		} );
+
+		it( 'should remove attribute elements from clone groups deeply', () => {
+			const p1 = writer.createContainerElement( 'p' );
+			const p2 = writer.createContainerElement( 'p' );
+			const foo = writer.createText( 'foo' );
+			const bar = writer.createText( 'bar' );
+
+			writer.insert( ViewPosition.createAt( root, 0 ), p1 );
+			writer.insert( ViewPosition.createAt( root, 1 ), p2 );
+			writer.insert( ViewPosition.createAt( p1, 0 ), foo );
+			writer.insert( ViewPosition.createAt( p2, 0 ), bar );
+			// <div><p>foo</p><p>bar</p></div>
+
+			const span = writer.createAttributeElement( 'span', null, { id: 'span' } );
+
+			// <div><p>fo<span>o</span></p><p>bar</p></div>
+			writer.wrap( ViewRange.createFromParentsAndOffsets( foo, 2, foo, 3 ), span );
+
+			// <div><p>fo<span>o</span></p><p><span>b</span>ar</p></div>
+			writer.wrap( ViewRange.createFromParentsAndOffsets( bar, 0, bar, 1 ), span );
+
+			// <div><p><span>b</span>ar</p></div>
+			writer.remove( ViewRange.createOn( p1 ) );
+
+			// Find the span.
+			const spanInTree = p2.getChild( 0 );
+
+			expect( Array.from( spanInTree.getElementsWithSameId() ) ).to.deep.equal( [ spanInTree ] );
+		} );
 	} );
 
 	function assertElementAttributes( element, attributes ) {