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

Commit

Permalink
Always update attributes of reused elements while rendering.
Browse files Browse the repository at this point in the history
  • Loading branch information
f1ames committed Oct 2, 2018
1 parent 6619a1f commit 9a6b7dc
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 18 deletions.
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 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
// (`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 );
}

/**
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

0 comments on commit 9a6b7dc

Please sign in to comment.