Merge pull request #893 from ckeditor/t/889

Other: Simplified `SelectionObserver`'s infinite loop check which should improve its stability. Closes #889.

Author: Reinmar

src/view/observer/selectionobserver.js

@@ -83,28 +83,15 @@ export default class SelectionObserver extends Observer {
 		 */
 		this._fireSelectionChangeDoneDebounced = debounce( data => this.document.fire( 'selectionChangeDone', data ), 200 );
 
-		this._clearInfiniteLoopInterval = setInterval( () => this._clearInfiniteLoop(), 2000 );
-
-		/**
-		 * Private property to store the last selection, to check if the code does not enter infinite loop.
-		 *
-		 * @private
-		 * @member {module:engine/view/selection~Selection} module:engine/view/observer/selectionobserver~SelectionObserver#_lastSelection
-		 */
-
-		/**
-		 * Private property to store the last but one selection, to check if the code does not enter infinite loop.
-		 *
-		 * @private
-		 * @member {module:engine/view/selection~Selection} module:engine/view/observer/selectionobserver~SelectionObserver#_lastButOneSelection
-		 */
+		this._clearInfiniteLoopInterval = setInterval( () => this._clearInfiniteLoop(), 1000 );
 
 		/**
 		 * Private property to check if the code does not enter infinite loop.
 		 *
 		 * @private
 		 * @member {Number} module:engine/view/observer/selectionobserver~SelectionObserver#_loopbackCounter
 		 */
+		this._loopbackCounter = 0;
 	}
 
 	/**
@@ -161,7 +148,9 @@ export default class SelectionObserver extends Observer {
 		}
 
 		// Ensure we are not in the infinite loop (#400).
-		if ( this._isInfiniteLoop( newViewSelection ) ) {
+		// This counter is reset each second. 60 selection changes in 1 second is enough high number
+		// to be very difficult (impossible) to achieve using just keyboard keys (during normal editor use).
+		if ( ++this._loopbackCounter > 60 ) {
 			/**
 			 * Selection change observer detected an infinite rendering loop.
 			 * Most probably you try to put the selection in the position which is not allowed
@@ -191,45 +180,12 @@ export default class SelectionObserver extends Observer {
 		this._fireSelectionChangeDoneDebounced( data );
 	}
 
-	/**
-	 * Checks if selection rendering entered an infinite loop.
-	 *
-	 * See https://github.com/ckeditor/ckeditor5-engine/issues/400.
-	 *
-	 * @private
-	 * @param {module:engine/view/selection~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;
-		}
-
-		// This counter is reset every 2 seconds. 50 selection changes in 2 seconds is enough high number
-		// to be very difficult (impossible) to achieve using just keyboard keys (during normal editor use).
-		if ( this._loopbackCounter > 50 ) {
-			return true;
-		}
-
-		return false;
-	}
-
 	/**
 	 * Clears `SelectionObserver` internal properties connected with preventing infinite loop.
 	 *
 	 * @protected
 	 */
 	_clearInfiniteLoop() {
-		this._lastSelection = null;
-		this._lastButOneSelection = null;
 		this._loopbackCounter = 0;
 	}
 }

tests/view/observer/selectionobserver.js

@@ -3,7 +3,7 @@
  * For licensing, see LICENSE.md.
  */
 
-/* globals setTimeout, setInterval, clearInterval, document */
+/* globals setTimeout, document */
 
 import ViewRange from '../../../src/view/range';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
@@ -136,98 +136,79 @@ describe( 'SelectionObserver', () => {
 		changeDomSelection();
 	} );
 
-	it( 'should warn and not enter infinite loop', ( done ) => {
-		// Reset infinite loop counters so other tests won't mess up with this test.
-		selectionObserver._clearInfiniteLoop();
-		clearInterval( selectionObserver._clearInfiniteLoopInterval );
-		selectionObserver._clearInfiniteLoopInterval = setInterval( () => selectionObserver._clearInfiniteLoop(), 2000 );
-
-		let counter = 100;
+	it( 'should warn and not enter infinite loop', () => {
+		// Selectionchange event is called twice per `changeDomSelection()` execution.
+		let counter = 35;
 
 		const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
 		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) );
 
-		viewDocument.on( 'selectionChange', () => {
-			counter--;
-
-			if ( counter > 0 ) {
-				setTimeout( changeDomSelection );
-			} else {
-				throw 'Infinite loop!';
-			}
-		} );
+		return new Promise( ( resolve, reject ) => {
+			testUtils.sinon.stub( log, 'warn', ( msg ) => {
+				expect( msg ).to.match( /^selectionchange-infinite-loop/ );
 
-		let warnedOnce = false;
+				resolve();
+			} );
 
-		testUtils.sinon.stub( log, 'warn', ( msg ) => {
-			if ( !warnedOnce ) {
-				warnedOnce = true;
+			viewDocument.on( 'selectionChangeDone', () => {
+				if ( !counter ) {
+					reject( new Error( 'Infinite loop warning was not logged.' ) );
+				}
+			} );
 
-				setTimeout( () => {
-					expect( msg ).to.match( /^selectionchange-infinite-loop/ );
-					done();
-				}, 200 );
+			while ( counter > 0 ) {
+				changeDomSelection();
+				counter--;
 			}
 		} );
-
-		changeDomSelection();
 	} );
 
 	it( 'should not be treated as an infinite loop if selection is changed only few times', ( done ) => {
 		const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
-
-		// Reset infinite loop counters so other tests won't mess up with this test.
-		selectionObserver._clearInfiniteLoop();
-
 		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) );
-
 		const spy = testUtils.sinon.spy( log, 'warn' );
 
+		viewDocument.on( 'selectionChangeDone', () => {
+			expect( spy.called ).to.be.false;
+			done();
+		} );
+
 		for ( let i = 0; i < 10; i++ ) {
 			changeDomSelection();
 		}
-
-		setTimeout( () => {
-			expect( spy.called ).to.be.false;
-			done();
-		}, 400 );
 	} );
 
-	it( 'should not be treated as an infinite loop if changes are not often', ( done ) => {
+	it( 'should not be treated as an infinite loop if changes are not often', () => {
 		const clock = testUtils.sinon.useFakeTimers( 'setInterval', 'clearInterval' );
-		const spy = testUtils.sinon.spy( log, 'warn' );
+		const stub = testUtils.sinon.stub( log, 'warn' );
 
 		// We need to recreate SelectionObserver, so it will use mocked setInterval.
 		selectionObserver.disable();
 		selectionObserver.destroy();
 		viewDocument._observers.delete( SelectionObserver );
 		viewDocument.addObserver( SelectionObserver );
 
-		// Inf-loop kicks in after 50th time the selection is changed in 2s.
-		// We will test 30 times, tick sinon clock to clean counter and then test 30 times again.
-		// Note that `changeDomSelection` fires two events.
-		let changeCount = 15;
-
-		for ( let i = 0; i < changeCount; i++ ) {
-			setTimeout( () => {
-				changeDomSelection();
-			}, i * 20 );
-		}
-
-		setTimeout( () => {
-			// Move the clock by 2100ms which will trigger callback added to `setInterval` and reset the inf-loop counter.
-			clock.tick( 2100 );
-
-			for ( let i = 0; i < changeCount; i++ ) {
-				changeDomSelection();
-			}
-
-			setTimeout( () => {
-				expect( spy.called ).to.be.false;
+		return doChanges()
+			.then( doChanges )
+			.then( () => {
+				sinon.assert.notCalled( stub );
 				clock.restore();
-				done();
-			}, 200 );
-		}, 400 );
+			} );
+
+		// Selectionchange event is called twice per `changeDomSelection()` execution. We call it 25 times to get
+		// 50 events. Infinite loop counter is reset, so calling this method twice should not show any warning.
+		function doChanges() {
+			return new Promise( resolve => {
+				viewDocument.once( 'selectionChangeDone', () => {
+					clock.tick( 1100 );
+					resolve();
+				} );
+
+				for ( let i = 0; i < 30; i++ ) {
+					changeDomSelection();
+				}
+			} );
+		}
 	} );
 
 	it( 'should fire `selectionChangeDone` event after selection stop changing', ( done ) => {