Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Always update attributes of reused elements while rendering #1568

Merged
merged 2 commits into from
Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 11 additions & 18 deletions src/view/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}

/**
Expand Down
28 changes: 28 additions & 0 deletions tests/view/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>' );
} );
} );
} );

Expand Down