Merge pull request #989 from ckeditor/t/922

Fix: Prevent unbinding elements that are reused during rendering. Closes #922.

BREAKING CHANGE: Removed `Renderer#getCorrespondingDom()` and `Renderer#getCorrespondingView()` methods.
BREAKING CHANGE: Renamed `Renderer#getCorrespondingDomText()` method to  `Renderer#findCorrespondingDomText()` and `Renderer#getCorrespondingViewText()` to  `Renderer#findCorrespondingViewText()`.
BREAKING CHANGE: Merged `Renderer#getCorrespondingDomElement()` and `Renderer#getCorrespondingDomDocumentFragment()` into one method `Renderer#mapViewToDom()`.
BREAKING CHANGE: Merged `Renderer#getCorrespondingViewElement()` and `Renderer#getCorrespondingViewDocumentFragment()` into `Renderer#mapDomToView()`.

Author: Reinmar

src/view/domconverter.js

@@ -55,7 +55,7 @@ export default class DomEventData {
 	 * @type module:engine/view/element~Element
 	 */
 	get target() {
-		return this.document.domConverter.getCorrespondingViewElement( this.domTarget );
+		return this.document.domConverter.mapDomToView( this.domTarget );
 	}
 
 	/**

src/view/observer/domeventdata.js

@@ -147,7 +147,7 @@ export default class MutationObserver extends Observer {
 		// element with changed structure anyway.
 		for ( const mutation of domMutations ) {
 			if ( mutation.type === 'childList' ) {
-				const element = domConverter.getCorrespondingViewElement( mutation.target );
+				const element = domConverter.mapDomToView( mutation.target );
 
 				// Do not collect mutations from UIElements.
 				if ( element && element.is( 'uiElement' ) ) {
@@ -162,15 +162,15 @@ export default class MutationObserver extends Observer {
 
 		// Handle `characterData` mutations later, when we have the full list of nodes which changed structure.
 		for ( const mutation of domMutations ) {
-			const element = domConverter.getCorrespondingViewElement( mutation.target );
+			const element = domConverter.mapDomToView( mutation.target );
 
 			// Do not collect mutations from UIElements.
 			if ( element && element.is( 'uiElement' ) ) {
 				continue;
 			}
 
 			if ( mutation.type === 'characterData' ) {
-				const text = domConverter.getCorrespondingViewText( mutation.target );
+				const text = domConverter.findCorrespondingViewText( mutation.target );
 
 				if ( text && !mutatedElements.has( text.parent ) ) {
 					// Use text as a key, for deduplication. If there will be another mutation on the same text element
@@ -186,7 +186,7 @@ export default class MutationObserver extends Observer {
 				// on text, but for the view, where filler text node did not existed, new text node was created, so we
 				// need to fire 'children' mutation instead of 'text'.
 				else if ( !text && startsWithFiller( mutation.target ) ) {
-					mutatedElements.add( domConverter.getCorrespondingViewElement( mutation.target.parentNode ) );
+					mutatedElements.add( domConverter.mapDomToView( mutation.target.parentNode ) );
 				}
 			}
 		}
@@ -203,7 +203,7 @@ export default class MutationObserver extends Observer {
 		}
 
 		for ( const viewElement of mutatedElements ) {
-			const domElement = domConverter.getCorrespondingDomElement( viewElement );
+			const domElement = domConverter.mapViewToDom( viewElement );
 			const viewChildren = viewElement.getChildren();
 			const newViewChildren = domConverter.domChildrenToView( domElement );
 

src/view/observer/mutationobserver.js

@@ -127,13 +127,13 @@ export default class Renderer {
 	 */
 	markToSync( type, node ) {
 		if ( type === 'text' ) {
-			if ( this.domConverter.getCorrespondingDom( node.parent ) ) {
+			if ( this.domConverter.mapViewToDom( node.parent ) ) {
 				this.markedTexts.add( node );
 			}
 		} else {
 			// If the node has no DOM element it is not rendered yet,
 			// its children/attributes do not need to be marked to be sync.
-			if ( !this.domConverter.getCorrespondingDom( node ) ) {
+			if ( !this.domConverter.mapViewToDom( node ) ) {
 				return;
 			}
 
@@ -165,9 +165,9 @@ export default class Renderer {
 	 * attributes which do not exist in the view element.
 	 *
 	 * For text nodes it updates the text string if it is different. Note that if parent element is marked as an element
-	 * which changed child list, text node update will not be done, because it may not be possible do find a
-	 * {@link module:engine/view/domconverter~DomConverter#getCorrespondingDomText corresponding DOM text}. The change will be handled
-	 * in the parent element.
+	 * which changed child list, text node update will not be done, because it may not be possible to
+	 * {@link module:engine/view/domconverter~DomConverter#findCorrespondingDomText find a corresponding DOM text}.
+	 * The change will be handled in the parent element.
 	 *
 	 * For elements, which child lists have changed, it calculates a {@link module:utils/diff~diff} and adds or removes children which have
 	 * changed.
@@ -190,7 +190,7 @@ export default class Renderer {
 		if ( this._inlineFiller ) {
 			inlineFillerPosition = this._getInlineFillerPosition();
 		}
-		// Othewise, if it's needed, create it at the selection position.
+		// Otherwise, if it's needed, create it at the selection position.
 		else if ( this._needsInlineFillerAtSelection() ) {
 			inlineFillerPosition = this.selection.getFirstPosition();
 
@@ -199,7 +199,7 @@ export default class Renderer {
 		}
 
 		for ( const node of this.markedTexts ) {
-			if ( !this.markedChildren.has( node.parent ) && this.domConverter.getCorrespondingDom( node.parent ) ) {
+			if ( !this.markedChildren.has( node.parent ) && this.domConverter.mapViewToDom( node.parent ) ) {
 				this._updateText( node, { inlineFillerPosition } );
 			}
 		}
@@ -351,7 +351,7 @@ export default class Renderer {
 		const selectionOffset = selectionPosition.offset;
 
 		// If there is no DOM root we do not care about fillers.
-		if ( !this.domConverter.getCorrespondingDomElement( selectionParent.root ) ) {
+		if ( !this.domConverter.mapViewToDom( selectionParent.root ) ) {
 			return false;
 		}
 
@@ -384,7 +384,7 @@ export default class Renderer {
 	 * filler should be rendered.
 	 */
 	_updateText( viewText, options ) {
-		const domText = this.domConverter.getCorrespondingDom( viewText );
+		const domText = this.domConverter.findCorrespondingDomText( viewText );
 		const newDomText = this.domConverter.viewToDom( viewText, domText.ownerDocument );
 
 		const actualText = domText.data;
@@ -408,7 +408,7 @@ export default class Renderer {
 	 * @param {module:engine/view/element~Element} viewElement View element to update.
 	 */
 	_updateAttrs( viewElement ) {
-		const domElement = this.domConverter.getCorrespondingDom( viewElement );
+		const domElement = this.domConverter.mapViewToDom( viewElement );
 		const domAttrKeys = Array.from( domElement.attributes ).map( attr => attr.name );
 		const viewAttrKeys = viewElement.getAttributeKeys();
 
@@ -436,7 +436,7 @@ export default class Renderer {
 	 */
 	_updateChildren( viewElement, options ) {
 		const domConverter = this.domConverter;
-		const domElement = domConverter.getCorrespondingDom( viewElement );
+		const domElement = domConverter.mapViewToDom( viewElement );
 
 		if ( !domElement ) {
 			// If there is no `domElement` it means that it was already removed from DOM.
@@ -445,9 +445,7 @@ export default class Renderer {
 		}
 
 		const domDocument = domElement.ownerDocument;
-
 		const filler = options.inlineFillerPosition;
-
 		const actualDomChildren = domElement.childNodes;
 		const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, { bind: true } ) );
 
@@ -464,20 +462,29 @@ export default class Renderer {
 		const actions = diff( actualDomChildren, expectedDomChildren, sameNodes );
 
 		let i = 0;
+		const nodesToUnbind = new Set();
 
 		for ( const action of actions ) {
 			if ( action === 'insert' ) {
 				insertAt( domElement, i, expectedDomChildren[ i ] );
 				i++;
 			} else if ( action === 'delete' ) {
-				// Whenever element is removed from DOM, unbind it and all of its children.
-				this.domConverter.unbindDomElement( actualDomChildren[ i ] );
+				nodesToUnbind.add( actualDomChildren[ i ] );
 				remove( actualDomChildren[ i ] );
 			} else { // 'equal'
 				i++;
 			}
 		}
 
+		// Unbind removed nodes. When node does not have a parent it means that it was removed from DOM tree during
+		// comparision with the expected DOM. We don't need to check child nodes, because if child node was reinserted,
+		// it was moved to DOM tree out of the removed node.
+		for ( const node of nodesToUnbind ) {
+			if ( !node.parentNode ) {
+				this.domConverter.unbindDomElement( node );
+			}
+		}
+
 		function sameNodes( actualDomChild, expectedDomChild ) {
 			// Elements.
 			if ( actualDomChild === expectedDomChild ) {
@@ -512,7 +519,7 @@ export default class Renderer {
 			return;
 		}
 
-		const domRoot = this.domConverter.getCorrespondingDomElement( this.selection.editableElement );
+		const domRoot = this.domConverter.mapViewToDom( this.selection.editableElement );
 
 		// Do nothing if there is no focus, or there is no DOM element corresponding to selection's editable element.
 		if ( !this.isFocused || !domRoot ) {
@@ -618,7 +625,7 @@ export default class Renderer {
 
 			if ( domSelection.rangeCount ) {
 				const activeDomElement = doc.activeElement;
-				const viewElement = this.domConverter.getCorrespondingViewElement( activeDomElement );
+				const viewElement = this.domConverter.mapDomToView( activeDomElement );
 
 				if ( activeDomElement && viewElement ) {
 					doc.getSelection().removeAllRanges();

src/view/renderer.js

@@ -66,7 +66,7 @@ describe( 'EditingController', () => {
 			expect( viewRoot ).to.equal( editing.view.getRoot() );
 			expect( domRoot ).to.equal( editing.view.getDomRoot() );
 
-			expect( editing.view.domConverter.getCorrespondingDom( viewRoot ) ).to.equal( domRoot );
+			expect( editing.view.domConverter.mapViewToDom( viewRoot ) ).to.equal( domRoot );
 			expect( editing.view.renderer.markedChildren.has( viewRoot ) ).to.be.true;
 
 			expect( editing.mapper.toModelElement( viewRoot ) ).to.equal( modelRoot );
@@ -81,7 +81,7 @@ describe( 'EditingController', () => {
 			expect( viewRoot ).to.equal( editing.view.getRoot( 'header' ) );
 			expect( domRoot ).to.equal( editing.view.getDomRoot( 'header' ) );
 
-			expect( editing.view.domConverter.getCorrespondingDom( viewRoot ) ).to.equal( domRoot );
+			expect( editing.view.domConverter.mapViewToDom( viewRoot ) ).to.equal( domRoot );
 			expect( editing.view.renderer.markedChildren.has( viewRoot ) ).to.be.true;
 
 			expect( editing.mapper.toModelElement( viewRoot ) ).to.equal( model.getRoot( 'header' ) );
@@ -100,7 +100,7 @@ describe( 'EditingController', () => {
 
 			expect( domRoot ).to.equal( editing.view.getDomRoot() );
 
-			expect( editing.view.domConverter.getCorrespondingDom( viewRoot ) ).to.equal( domRoot );
+			expect( editing.view.domConverter.mapViewToDom( viewRoot ) ).to.equal( domRoot );
 			expect( editing.view.renderer.markedChildren.has( viewRoot ) ).to.be.true;
 
 			expect( editing.mapper.toModelElement( viewRoot ) ).to.equal( modelRoot );

tests/controller/editingcontroller.js

@@ -103,7 +103,7 @@ describe( 'Document', () => {
 			expect( ret ).to.equal( viewRoot );
 
 			expect( domRoot ).to.equal( domDiv );
-			expect( viewDocument.domConverter.getCorrespondingDom( viewRoot ) ).to.equal( domDiv );
+			expect( viewDocument.domConverter.mapViewToDom( viewRoot ) ).to.equal( domDiv );
 
 			expect( viewRoot.name ).to.equal( 'div' );
 			expect( viewDocument.renderer.markedChildren.has( viewRoot ) ).to.be.true;
@@ -185,7 +185,7 @@ describe( 'Document', () => {
 			expect( count( viewDocument.roots ) ).to.equal( 1 );
 
 			expect( viewDocument.getDomRoot() ).to.equal( domDiv );
-			expect( viewDocument.domConverter.getCorrespondingDom( viewRoot ) ).to.equal( domDiv );
+			expect( viewDocument.domConverter.mapViewToDom( viewRoot ) ).to.equal( domDiv );
 
 			expect( viewDocument.renderer.markedChildren.has( viewRoot ) ).to.be.true;
 		} );
@@ -205,7 +205,7 @@ describe( 'Document', () => {
 			expect( count( viewDocument.roots ) ).to.equal( 2 );
 
 			expect( viewDocument.getDomRoot( 'header' ) ).to.equal( domH1 );
-			expect( viewDocument.domConverter.getCorrespondingDom( viewH1 ) ).to.equal( domH1 );
+			expect( viewDocument.domConverter.mapViewToDom( viewH1 ) ).to.equal( domH1 );
 
 			expect( viewDocument.getRoot().name ).to.equal( 'div' );
 			expect( viewDocument.renderer.markedChildren.has( viewH1 ) ).to.be.true;