updateElementMappings does not refresh attributes

[pjasiun]

This code:

https://github.com/ckeditor/ckeditor5-engine/blob/0821d9098b76c25fef577d6ccfe0c1324d0bc8ed/src/view/renderer.js#L317-L318

mark children of the remap element to be refreshed but do not map its attributes. It means that the DOM element may have old attributes after remapping.

[f1ames]

This issue is a little more tricky. The renderer works in a way that it tires not to touch DOM nodes which didn't change in a DOM and only rerenders parts that have changed (see the more in-depth description of the whole algorithm).

For this issue, the important part of the renderer is attributes refreshing. Only elements which are not the same (so classes, attributes, etc changed from the last render) are marked to have its attributes refreshed: https://github.com/ckeditor/ckeditor5-engine/blob/0821d9098b76c25fef577d6ccfe0c1324d0bc8ed/src/view/renderer.js#L309
The case we have encountered could be represented by the following test:

it( 'should rerender element if its attribute was removed before rendering', () => {
	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.
	writer.removeAttribute( 'data-placeholder', viewP );

	viewRoot._removeChildren( 0, viewRoot.childCount );

	viewRoot._appendChild( parse( '<container:p>1</container:p><container:p>2</container:p>' ) );

	renderer.markToSync( 'attributes', viewP );
	renderer.markToSync( 'children', viewRoot );
	renderer.render();

	expect( domRoot.innerHTML ).to.equal( '<p>1</p><p>2</p>' );
} );

This test fails on the 2nd expect. The problem here is that

1. After the first renderer.render() call we have HTML like <p data-placeholder="Body">1</p> and the domConverter in its mappings holds a reference to ContainerElement:p with one attribute data-placeholder: "Body".
2. The attribute and viewP element is removed. It is still hold inside domConverter mappings (has parent === null and not attributes).
3. The renderer.render() method is called and its _updateChildrenMappings() method with [ replace, insert ] actions (it means first p was replaced with another p and can be reused, while second p needs to be inserted).
4. The p replace action takes an old viewP element (it already has not attributes - check point 2 above) and new newViewP element (which is mapped via the domConverter from DOM p node).
5. Both has no attributes (in the view) so are treated as similar and not marked to have its attributes rerendered.

[f1ames]

From the renderer perspective, there are no many options to distinguish if the element has some attribute modifications done in the meantime. However, all attribute changes, causes the element to be marked to have its attributes updated by the renderer (renderer.markToSync( 'attributes', node ) method). So the solution for this issue, apart from checking if new and reused elements are not similar is to check if new element was marked to have its attributes updated - if so mark the reused element.

[f1ames]

The proposed solution from https://github.com/ckeditor/ckeditor5-engine/issues/1560#issuecomment-425950993, assumed that the attribute will be firstly removed and then the element is removed itself:

writer.removeAttribute( 'data-placeholder', viewP );

viewRoot._removeChildren( 0, viewRoot.childCount );

However, in opposite cases, when element is firstly removed and then its attributes removed:

viewRoot._removeChildren( 0, viewRoot.childCount );

writer.removeAttribute( 'data-placeholder', viewP );

the change:attributes event will not bubble to the viewRoot (because removed element has no parent to bubble the event to) and element will not be marked to have its attributes removed (so the fix above will fail in this case).

If we would like to solve this issue in the renderer, the only way is to compare attributes between view and DOM element or just mark all replaced elements to have its attributes updated.