Merge pull request #1828 from ckeditor/i/6092

Fix: Fixed renderer bug causing editor crash in a range of scenarios involving reusing DOM elements. Closes ckeditor/ckeditor5#6092.

src/view/renderer.js

@@ -548,15 +548,30 @@ export default class Renderer {
 		let i = 0;
 		const nodesToUnbind = new Set();
 
+		// Handle deletions first.
+		// This is to prevent a situation where an element that already exists in `actualDomChildren` is inserted at a different
+		// index in `actualDomChildren`. Since `actualDomChildren` is a `NodeList`, this works like move, not like an insert,
+		// and it disrupts the whole algorithm. See https://github.com/ckeditor/ckeditor5/issues/6367.
+		//
+		// It doesn't matter in what order we remove or add nodes, as long as we remove and add correct nodes at correct indexes.
+		for ( const action of diff ) {
+			if ( action === 'delete' ) {
+				nodesToUnbind.add( actualDomChildren[ i ] );
+				remove( actualDomChildren[ i ] );
+			} else if ( action === 'equal' ) {
+				i++;
+			}
+		}
+
+		i = 0;
+
 		for ( const action of diff ) {
 			if ( action === 'insert' ) {
 				insertAt( domElement, i, expectedDomChildren[ i ] );
 				i++;
-			} else if ( action === 'delete' ) {
-				nodesToUnbind.add( actualDomChildren[ i ] );
-				remove( actualDomChildren[ i ] );
-			} else { // 'equal'
+			} else if ( action === 'equal' ) {
 				// Force updating text nodes inside elements which did not change and do not need to be re-rendered (#1125).
+				// Do it here (not in the loop above) because only after insertions the `i` index is correct.
 				this._markDescendantTextToSync( this.domConverter.domToView( expectedDomChildren[ i ] ) );
 				i++;
 			}

tests/view/renderer.js

@@ -3227,6 +3227,47 @@ describe( 'Renderer', () => {
 
 				expect( domRoot.innerHTML ).to.equal( '<p><a href="#href">Foo<i>Bar</i></a></p>' );
 			} );
+
+			// https://github.com/ckeditor/ckeditor5/issues/6367.
+			it( 'should correctly handle moving a DOM element when rendering children', () => {
+				viewRoot._insertChild( 0, parse( 'y<attribute:span>x</attribute:span>' ) );
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const viewSpan = viewRoot._removeChildren( 1, 1 )[ 0 ];
+				viewRoot._insertChild( 0, viewSpan );
+				viewRoot._insertChild( 2, parse( '<attribute:strong>z</attribute:strong>' ) );
+
+				renderer.markToSync( 'children', viewRoot );
+
+				// This would throw without a fix.
+				renderer.render();
+
+				expect( domRoot.innerHTML ).to.equal( '<span>x</span>y<strong>z</strong>' );
+			} );
+
+			// https://github.com/ckeditor/ckeditor5/issues/6367, but more complex
+			it( 'should correctly handle moving a DOM element when rendering children (more complex case)', () => {
+				viewRoot._insertChild( 0,
+					parse( '1<attribute:span>2</attribute:span><attribute:span>3</attribute:span>4<attribute:span>5</attribute:span>' )
+				);
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				const viewSpan5 = viewRoot._removeChildren( 4, 1 )[ 0 ];
+				const viewSpan2 = viewRoot._removeChildren( 1, 1 )[ 0 ];
+				viewRoot._insertChild( 0, viewSpan2 );
+				viewRoot._insertChild( 1, parse( '<attribute:strong>6</attribute:strong>' ) );
+				viewRoot._insertChild( 4, parse( '<attribute:strong>7</attribute:strong>' ) );
+				viewRoot._insertChild( 2, viewSpan5 );
+
+				renderer.markToSync( 'children', viewRoot );
+
+				// This would throw without a fix.
+				renderer.render();
+
+				expect( domRoot.innerHTML ).to.equal( '<span>2</span><strong>6</strong><span>5</span>1<span>3</span><strong>7</strong>4' );
+			} );
 		} );
 
 		describe( 'optimal (minimal) rendering – minimal children changes', () => {
@@ -3313,7 +3354,7 @@ describe( 'Renderer', () => {
 				expect( getMutationStats( observer.takeRecords() ) ).to.be.empty;
 			} );
 
-			it( 'should add and remove one', () => {
+			it( 'should remove and add one', () => {
 				viewRoot._appendChild( parse( '<container:p>1</container:p><container:p>2</container:p>' ) );
 
 				renderer.markToSync( 'children', viewRoot );
@@ -3327,8 +3368,8 @@ describe( 'Renderer', () => {
 				renderer.render();
 
 				expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [
-					'added: 1, removed: 0',
-					'added: 0, removed: 1'
+					'added: 0, removed: 1',
+					'added: 1, removed: 0'
 				] );
 			} );
 
@@ -3460,7 +3501,7 @@ describe( 'Renderer', () => {
 					expect( getMutationStats( observer.takeRecords() ) ).to.be.empty;
 				} );
 
-				it( 'should add and remove one', () => {
+				it( 'should remove and add one', () => {
 					viewRoot._appendChild( parse( makeContainers( 151 ) ) );
 
 					renderer.markToSync( 'children', viewRoot );
@@ -3474,8 +3515,8 @@ describe( 'Renderer', () => {
 					renderer.render();
 
 					expect( getMutationStats( observer.takeRecords() ) ).to.deep.equal( [
-						'added: 1, removed: 0',
-						'added: 0, removed: 1'
+						'added: 0, removed: 1',
+						'added: 1, removed: 0'
 					] );
 				} );