浏览代码

Merge pull request #487 from ckeditor/t/400

t/400: Resolve infinite loop because of selection change.
Piotrek Koszuliński 9 年之前
父节点
当前提交
6adb486d39

+ 68 - 1
packages/ckeditor5-engine/src/view/observer/selectionobserver.js

@@ -8,6 +8,8 @@
 import Observer from './observer.js';
 import MutationObserver from './mutationobserver.js';
 
+import log from '../../../utils/log.js';
+
 /**
  * Selection observer class observes selection changes in the document. If selection changes on the document this
  * observer checks if there are any mutations and if DOM selection is different than the
@@ -47,7 +49,7 @@ export default class SelectionObserver extends Observer {
 		 * Reference to the view {@link engine.view.Selection} object used to compare new selection with it.
 		 *
 		 * @readonly
-		 * @member {engine.view.Document} engine.view.observer.SelectionObserver#selection
+		 * @member {engine.view.Selection} engine.view.observer.SelectionObserver#selection
 		 */
 		this.selection = document.selection;
 
@@ -67,6 +69,27 @@ export default class SelectionObserver extends Observer {
 		 * @member {WeakSet.<Document>} engine.view.observer.SelectionObserver#_documents
 		 */
 		this._documents = new WeakSet();
+
+		/**
+		 * Private property to store the last selection, to check if the code does not enter infinite loop.
+		 *
+		 * @private
+		 * @member {engine.view.Selection} engine.view.observer.SelectionObserver#_lastSelection
+		 */
+
+		/**
+		 * Private property to store the last but one selection, to check if the code does not enter infinite loop.
+		 *
+		 * @private
+		 * @member {engine.view.Selection} engine.view.observer.SelectionObserver#_lastButOneSelection
+		 */
+
+		/**
+		 * Private property to check if the code does not enter infinite loop.
+		 *
+		 * @private
+		 * @member {Number} engine.view.observer.SelectionObserver#_loopbackCounter
+		 */
 	}
 
 	/**
@@ -114,6 +137,21 @@ export default class SelectionObserver extends Observer {
 			return;
 		}
 
+		// Ensure we are not in the infinite loop (#400).
+		if ( this._isInfiniteLoop( newViewSelection ) ) {
+			/**
+			 * Selection change observer detected an infinite rendering loop.
+			 * Most probably you try to put the selection in the position which is not allowed
+			 * by the browser and browser fixes it automatically what causes `selectionchange` event on
+			 * which a loopback through a model tries to re-render the wrong selection and again.
+			 *
+			 * @error selectionchange-infinite-loop
+			 */
+			log.warn( 'selectionchange-infinite-loop: Selection change observer detected an infinite rendering loop.' );
+
+			return;
+		}
+
 		// Should be fired only when selection change was the only document change.
 		this.document.fire( 'selectionChange', {
 			oldSelection: this.selection,
@@ -123,6 +161,35 @@ export default class SelectionObserver extends Observer {
 
 		this.document.render();
 	}
+
+	/**
+	 * Checks if selection rendering entered an infinite loop.
+	 *
+	 * See https://github.com/ckeditor/ckeditor5-engine/issues/400.
+	 *
+	 * @private
+	 * @param {engine.view.Selection} newSelection DOM selection converted to view.
+	 * @returns {Boolean} True is the same selection repeat more then 10 times.
+	 */
+	_isInfiniteLoop( newSelection ) {
+		// If the position is the same a the last one or the last but one we increment the counter.
+		// We need to check last two selections because the browser will first fire a selectionchange event
+		// for an incorrect selection and then for a corrected one.
+		if ( this._lastSelection && this._lastButOneSelection &&
+			( newSelection.isEqual( this._lastSelection ) || newSelection.isEqual( this._lastButOneSelection ) ) ) {
+			this._loopbackCounter++;
+		} else {
+			this._lastButOneSelection = this._lastSelection;
+			this._lastSelection = newSelection;
+			this._loopbackCounter = 0;
+		}
+
+		if ( this._loopbackCounter > 10 ) {
+			return true;
+		}
+
+		return false;
+	}
 }
 
 /**

+ 87 - 1
packages/ckeditor5-engine/tests/view/observer/selectionobserver.js

@@ -8,15 +8,19 @@
 'use strict';
 
 import ViewRange from '/ckeditor5/engine/view/range.js';
+import testUtils from '/tests/ckeditor5/_utils/utils.js';
 import ViewSelection from '/ckeditor5/engine/view/selection.js';
 import ViewDocument from '/ckeditor5/engine/view/document.js';
 import SelectionObserver from '/ckeditor5/engine/view/observer/selectionobserver.js';
 import MutationObserver from '/ckeditor5/engine/view/observer/mutationobserver.js';
 
 import EmitterMixin from '/ckeditor5/utils/emittermixin.js';
+import log from '/ckeditor5/utils/log.js';
 
 import { parse } from '/tests/engine/_utils/view.js';
 
+testUtils.createSinonSandbox();
+
 describe( 'SelectionObserver', () => {
 	let viewDocument, viewRoot, mutationObserver, selectionObserver, listenter;
 
@@ -32,7 +36,9 @@ describe( 'SelectionObserver', () => {
 
 		viewRoot = viewDocument.getRoot();
 
-		viewRoot.appendChildren( parse( '<container:p>foo</container:p><container:p>bar</container:p>' ) );
+		viewRoot.appendChildren( parse(
+			'<container:p>xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx</container:p>' +
+			'<container:p>yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy</container:p>' ) );
 
 		viewDocument.render();
 	} );
@@ -124,6 +130,76 @@ describe( 'SelectionObserver', () => {
 		changeDomSelection();
 	} );
 
+	it( 'should warn and not enter infinite loop', ( done ) => {
+		let counter = 30;
+
+		const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
+		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) );
+
+		listenter.listenTo( viewDocument, 'selectionChange', () => {
+			counter--;
+
+			if ( counter > 0 ) {
+				setTimeout( changeDomSelection );
+			} else {
+				throw( 'Infinite loop!' );
+			}
+		} );
+
+		let warnedOnce = false;
+
+		testUtils.sinon.stub( log, 'warn', ( msg ) => {
+			if ( !warnedOnce ) {
+				warnedOnce = true;
+
+				setTimeout( () => {
+					expect( msg ).to.match( /^selectionchange-infinite-loop/ );
+					done();
+				}, 200 );
+			}
+		} );
+
+		changeDomSelection();
+	} );
+
+	it( 'should not be treated as an infinite loop if the position is different', ( done ) => {
+		let counter = 30;
+
+		const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
+		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) );
+
+		listenter.listenTo( viewDocument, 'selectionChange', () => {
+			counter--;
+
+			if ( counter > 0 ) {
+				setTimeout( () => changeCollapsedDomSelection( counter ) );
+			} else {
+				done();
+			}
+		} );
+
+		changeCollapsedDomSelection( counter );
+	} );
+
+	it( 'should not be treated as an infinite loop if it is less then 3 times', ( done ) => {
+		let counter = 3;
+
+		const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
+		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) );
+
+		listenter.listenTo( viewDocument, 'selectionChange', () => {
+			counter--;
+
+			if ( counter > 0 ) {
+				setTimeout( () => changeDomSelection() );
+			} else {
+				done();
+			}
+		} );
+
+		changeDomSelection();
+	} );
+
 	it( 'should call render after selection change which reset selection if it was not changed', ( done ) => {
 		const viewBar = viewDocument.getRoot().getChild( 1 ).getChild( 0 );
 		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewBar, 0, viewBar, 1 ) );
@@ -150,6 +226,16 @@ describe( 'SelectionObserver', () => {
 	} );
 } );
 
+function changeCollapsedDomSelection( pos = 1 ) {
+	const domSelection = document.getSelection();
+	domSelection.removeAllRanges();
+	const domFoo = document.getElementById( 'main' ).childNodes[ 0 ].childNodes[ 0 ];
+	const domRange = new Range();
+	domRange.setStart( domFoo, pos );
+	domRange.setEnd( domFoo, pos );
+	domSelection.addRange( domRange );
+}
+
 function changeDomSelection() {
 	const domSelection = document.getSelection();
 	domSelection.removeAllRanges();