Merge pull request #1792 from ckeditor/t/1791

Fix: Improved performance when working with fake selections. Closes #1791.

src/view/domconverter.js

@@ -106,8 +106,9 @@ export default class DomConverter {
 	}
 
 	/**
-	 * Binds given DOM element that represents fake selection to {@link module:engine/view/documentselection~DocumentSelection
-	 * document selection}. Document selection copy is stored and can be retrieved by
+	 * Binds given DOM element that represents fake selection to a **position** of a
+	 * {@link module:engine/view/documentselection~DocumentSelection document selection}.
+	 * Document selection copy is stored and can be retrieved by
 	 * {@link module:engine/view/domconverter~DomConverter#fakeSelectionToView} method.
 	 *
 	 * @param {HTMLElement} domElement

src/view/renderer.js

@@ -698,41 +698,32 @@ export default class Renderer {
 	 */
 	_updateFakeSelection( domRoot ) {
 		const domDocument = domRoot.ownerDocument;
-		let container = this._fakeSelectionContainer;
 
-		// Create fake selection container if one does not exist.
-		if ( !container ) {
-			this._fakeSelectionContainer = container = domDocument.createElement( 'div' );
+		if ( !this._fakeSelectionContainer ) {
+			this._fakeSelectionContainer = createFakeSelectionContainer( domDocument );
+		}
+
+		const container = this._fakeSelectionContainer;
 
-			Object.assign( container.style, {
-				position: 'fixed',
-				top: 0,
-				left: '-9999px',
-				// See https://github.com/ckeditor/ckeditor5/issues/752.
-				width: '42px'
-			} );
+		// Bind fake selection container with the current selection *position*.
+		this.domConverter.bindFakeSelection( container, this.selection );
 
-			// Fill it with a text node so we can update it later.
-			container.textContent = '\u00A0';
+		if ( !this._fakeSelectionNeedsUpdate( domRoot ) ) {
+			return;
 		}
 
 		if ( !container.parentElement || container.parentElement != domRoot ) {
 			domRoot.appendChild( container );
 		}
 
-		// Update contents.
 		container.textContent = this.selection.fakeSelectionLabel || '\u00A0';
 
-		// Update selection.
 		const domSelection = domDocument.getSelection();
 		const domRange = domDocument.createRange();
 
 		domSelection.removeAllRanges();
 		domRange.selectNodeContents( container );
 		domSelection.addRange( domRange );
-
-		// Bind fake selection container with current selection.
-		this.domConverter.bindFakeSelection( container, this.selection );
 	}
 
 	/**
@@ -804,6 +795,31 @@ export default class Renderer {
 		return true;
 	}
 
+	/**
+	 * Checks whether the fake selection needs to be updated.
+	 *
+	 * @private
+	 * @param {HTMLElement} domRoot A valid DOM root where a new fake selection container should be added.
+	 * @returns {Boolean}
+	 */
+	_fakeSelectionNeedsUpdate( domRoot ) {
+		const container = this._fakeSelectionContainer;
+		const domSelection = domRoot.ownerDocument.getSelection();
+
+		// Fake selection needs to be updated if there's no fake selection container, or the container currently sits
+		// in a different root.
+		if ( !container || container.parentElement !== domRoot ) {
+			return true;
+		}
+
+		// Make sure that the selection actually is within the fake selection.
+		if ( domSelection.anchorNode !== container && !container.contains( domSelection.anchorNode ) ) {
+			return true;
+		}
+
+		return container.textContent !== this.selection.fakeSelectionLabel;
+	}
+
 	/**
 	 * Removes the DOM selection.
 	 *
@@ -985,3 +1001,25 @@ function filterOutFakeSelectionContainer( domChildList, fakeSelectionContainer )
 
 	return childList;
 }
+
+// Creates a fake selection container for a given document.
+//
+// @private
+// @param {Document} domDocument
+// @returns {HTMLElement}
+function createFakeSelectionContainer( domDocument ) {
+	const container = domDocument.createElement( 'div' );
+
+	Object.assign( container.style, {
+		position: 'fixed',
+		top: 0,
+		left: '-9999px',
+		// See https://github.com/ckeditor/ckeditor5/issues/752.
+		width: '42px'
+	} );
+
+	// Fill it with a text node so we can update it later.
+	container.textContent = '\u00A0';
+
+	return container;
+}

tests/view/renderer.js

@@ -20,7 +20,7 @@ import Renderer from '../../src/view/renderer';
 import DocumentFragment from '../../src/view/documentfragment';
 import DowncastWriter from '../../src/view/downcastwriter';
 
-import { parse, setData as setViewData, getData as getViewData } from '../../src/dev-utils/view';
+import { parse, stringify, setData as setViewData, getData as getViewData } from '../../src/dev-utils/view';
 import { INLINE_FILLER, INLINE_FILLER_LENGTH, isBlockFiller, BR_FILLER } from '../../src/view/filler';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import createViewRoot from './_utils/createroot';
@@ -1804,6 +1804,71 @@ describe( 'Renderer', () => {
 				assertDomSelectionContents( domSelection, container, /^fake selection label$/ );
 			} );
 
+			describe( 'subsequent call optimization', () => {
+				// https://github.com/ckeditor/ckeditor5-engine/issues/1791
+				it( 'doesn\'t render the same selection multiple times', () => {
+					const createRangeSpy = sinon.spy( document, 'createRange' );
+					const label = 'subsequent fake selection calls';
+
+					selection._setTo( selection.getRanges(), { fake: true, label } );
+					renderer.render();
+					selection._setTo( selection.getRanges(), { fake: true, label } );
+					renderer.render();
+
+					expect( createRangeSpy.callCount ).to.be.equal( 1 );
+				} );
+
+				it( 'different subsequent fake selections sets do change native selection', () => {
+					const createRangeSpy = sinon.spy( document, 'createRange' );
+
+					selection._setTo( selection.getRanges(), { fake: true, label: 'selection 1' } );
+					renderer.render();
+					selection._setTo( selection.getRanges(), { fake: true, label: 'selection 2' } );
+					renderer.render();
+
+					expect( createRangeSpy.callCount ).to.be.equal( 2 );
+				} );
+
+				it( 'rerenders selection if disturbed externally', () => {
+					const interruptingRange = document.createRange();
+					interruptingRange.setStartBefore( domRoot.children[ 0 ] );
+					interruptingRange.setEndAfter( domRoot.children[ 0 ] );
+
+					const createRangeSpy = sinon.spy( document, 'createRange' );
+					const label = 'selection 1';
+
+					selection._setTo( selection.getRanges(), { fake: true, label } );
+					renderer.render();
+
+					document.getSelection().removeAllRanges();
+					document.getSelection().addRange( interruptingRange );
+
+					selection._setTo( selection.getRanges(), { fake: true, label } );
+					renderer.render();
+
+					expect( createRangeSpy.callCount ).to.be.equal( 2 );
+				} );
+
+				it( 'correctly maps fake selection ', () => {
+					// See https://github.com/ckeditor/ckeditor5-engine/pull/1792#issuecomment-529814641
+					const label = 'subsequent fake selection calls';
+					const { view: newParagraph, selection: newSelection } = parse( '<container:p>[baz]</container:p>' );
+
+					viewRoot._appendChild( newParagraph );
+
+					selection._setTo( selection.getRanges(), { fake: true, label } );
+					renderer.render();
+
+					selection._setTo( newSelection.getRanges(), { fake: true, label } );
+					renderer.render();
+
+					const fakeSelectionContainer = domRoot.childNodes[ 1 ];
+					const mappedSelection = renderer.domConverter.fakeSelectionToView( fakeSelectionContainer );
+
+					expect( stringify( viewRoot, mappedSelection ) ).to.be.equal( '<div><p>foo bar</p><p>[baz]</p></div>' );
+				} );
+			} );
+
 			it( 'should render &nbsp; if no selection label is provided', () => {
 				selection._setTo( selection.getRanges(), { fake: true } );
 				renderer.render();