Skip to content

Commit

Permalink
Merge pull request #9535 from ckeditor/i/9534
Browse files Browse the repository at this point in the history
Fix (engine): Renderer should not crash while removing multiple DOM nodes in the same render cycle. Closes #9534.

Thanks to [bendemboski](https://github.com/bendemboski)!
  • Loading branch information
niegowski authored Apr 21, 2021
2 parents a7a37e7 + e1a9c1f commit 75ebb48
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
10 changes: 9 additions & 1 deletion packages/ckeditor5-engine/src/view/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,15 @@ export default class Renderer {
return;
}

const actualDomChildren = this.domConverter.mapViewToDom( viewElement ).childNodes;
// Removing nodes from the DOM as we iterate can cause `actualDomChildren`
// (which is a live-updating `NodeList`) to get out of sync with the
// indices that we compute as we iterate over `actions`, producing
// incorrect element mappings.
//
// Converting live list to an array to make the list static.
const actualDomChildren = Array.from(
this.domConverter.mapViewToDom( viewElement ).childNodes
);
const expectedDomChildren = Array.from(
this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, { withChildren: false } )
);
Expand Down
21 changes: 21 additions & 0 deletions packages/ckeditor5-engine/tests/view/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3435,6 +3435,27 @@ describe( 'Renderer', () => {

expect( domRoot.innerHTML ).to.equal( '<span>2</span><strong>6</strong><span>5</span>1<span>3</span><strong>7</strong>4' );
} );

it( 'should correctly handle multiple changes when using fastDiff', () => {
let str = '';

for ( let i = 0; i < 100; i++ ) {
str += `${ i }<attribute:span>${ i }</attribute:span>`;
}

viewRoot._insertChild( 0, parse( str ) );
renderer.markToSync( 'children', viewRoot );
renderer.render();

viewRoot._removeChildren( 0, 1 );
viewRoot._removeChildren( 4, 1 );
renderer.markToSync( 'children', viewRoot );
renderer.render();

expect( domRoot.innerHTML ).to.equal(
str.slice( 1 ).replaceAll( 'attribute:span', 'span' ).replace( '<span>2</span>', '' )
);
} );
} );

describe( 'optimal (minimal) rendering – minimal children changes', () => {
Expand Down

0 comments on commit 75ebb48

Please sign in to comment.