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
Changes from 1 commit
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
@@ -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 can be removed from view structure and then has
// its attributes changed. In such cases it si not present in `markedAttributes` and may be the same
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

si

// (`element.isSimilar()`) as the reused element. To prevent such cases we always mark reused element
// to have its attrbiutes rerenderd (#1560).
this.markedAttributes.add( viewElement );
}

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