Merge pull request #1568 from ckeditor/t/1560-c

Other: Always update attributes of reused elements while rendering. Closes #1560.

Author: Piotr Jasiun

src/view/renderer.js

@@ -294,30 +294,23 @@ export default class Renderer {
 	 * @param {Node} domElement The DOM element representing the given view element.
 	 */
 	_updateElementMappings( viewElement, domElement ) {
-		// Because we replace new view element mapping with the existing one, the corresponding DOM element
-		// will not be rerendered. The new view element may have different attributes than the previous one.
-		// Since its corresponding DOM element will not be rerendered, new attributes will not be added
-		// to the DOM, so we need to mark it here to make sure its attributes gets updated.
-		// Such situations may happen if only new view element was added to `this.markedAttributes`
-		// or none of the elements were added (relying on 'this._updateChildren()' which by rerendering the element
-		// also rerenders its attributes). See #1427 for more detailed case study.
-		const newViewChild = this.domConverter.mapDomToView( domElement );
-
-		// It may also happen that 'newViewChild' mapping is not present since its parent mapping
-		// was already removed (the 'domConverter.unbindDomElement()' method also unbinds children
-		// mappings) so we also check for '!newViewChild'.
-		// Also check if new element ('newViewChild') was marked to have its attributes rerenderd,
-		// if so, marked reused view element too (#1560).
-		if ( !newViewChild || newViewChild && !newViewChild.isSimilar( viewElement ) || this.markedAttributes.has( newViewChild ) ) {
-			this.markedAttributes.add( viewElement );
-		}
-
 		// Remap 'DomConverter' bindings.
 		this.domConverter.unbindDomElement( domElement );
 		this.domConverter.bindElements( domElement, viewElement );
 
 		// View element may have children which needs to be updated, but are not marked, mark them to update.
 		this.markedChildren.add( viewElement );
+
+		// Because we replace new view element mapping with the existing one, the corresponding DOM element
+		// will not be rerendered. The new view element may have different attributes than the previous one.
+		// Since its corresponding DOM element will not be rerendered, new attributes will not be added
+		// to the DOM, so we need to mark it here to make sure its attributes gets updated. See #1427 for more
+		// detailed case study.
+		// Also there are cases where replaced element is removed from the view structure and then has
+		// its attributes changed or removed. In such cases the element will not be present in `markedAttributes`
+		// and also may be the same (`element.isSimilar()`) as the reused element not having its attributes updated.
+		// To prevent such situations we always mark reused element to have its attributes rerenderd (#1560).
+		this.markedAttributes.add( viewElement );
 	}
 
 	/**

tests/view/renderer.js

@@ -3100,6 +3100,34 @@ describe( 'Renderer', () => {
 
 				expect( domRoot.innerHTML ).to.equal( '<h1>h1</h1><p class="cke-test1">p</p><p>p2</p>' );
 			} );
+
+			it( 'should rerender element if it was removed and have its attributes removed after', () => {
+				const writer = new DowncastWriter();
+
+				// 1. Setup initial view/DOM.
+				viewRoot._appendChild( parse( '<container:p>1</container:p>' ) );
+
+				const viewP = viewRoot.getChild( 0 );
+
+				writer.setAttribute( 'data-placeholder', 'Body', viewP );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				expect( domRoot.innerHTML ).to.equal( '<p data-placeholder="Body">1</p>' );
+
+				// 2. Modify view.
+				viewRoot._removeChildren( 0, viewRoot.childCount );
+
+				writer.removeAttribute( 'data-placeholder', viewP );
+
+				viewRoot._appendChild( parse( '<container:p>1</container:p><container:p>2</container:p>' ) );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				expect( domRoot.innerHTML ).to.equal( '<p>1</p><p>2</p>' );
+			} );
 		} );
 	} );