Merge pull request #671 from ckeditor/t/629b

T/629b Alternative fix infinite selection loop.

src/controller/editingcontroller.js

@@ -138,9 +138,11 @@ export default class EditingController {
 	}
 
 	/**
-	 * Removes all event listeners attached by the EditingController.
+	 * Removes all event listeners attached to the `EditingController`. Destroys all objects created
+	 * by `EditingController` that need to be destroyed.
 	 */
 	destroy() {
+		this.view.destroy();
 		this._listenter.stopListening();
 	}
 }

src/conversion/view-selection-to-model-converters.js

@@ -10,6 +10,8 @@
  * @namespace engine.conversion.viewSelectionToModel
  */
 
+import ModelSelection from '../model/selection.js';
+
 /**
  * Function factory, creates a callback function which converts a {@link engine.view.Selection view selection} taken
  * from the {@link engine.view.Document#selectionChange} event and sets in on the {@link engine.model.Document#selection model}.
@@ -26,15 +28,21 @@
  */
 export function convertSelectionChange( modelDocument, mapper ) {
 	return ( evt, data ) => {
-		modelDocument.enqueueChanges( () => {
-			const viewSelection = data.newSelection;
-			const ranges = [];
+		const viewSelection = data.newSelection;
+		const modelSelection = new ModelSelection();
+
+		const ranges = [];
+
+		for ( let viewRange of viewSelection.getRanges() ) {
+			ranges.push( mapper.toModelRange( viewRange ) );
+		}
 
-			for ( let viewRange of viewSelection.getRanges() ) {
-				ranges.push( mapper.toModelRange( viewRange ) );
-			}
+		modelSelection.setRanges( ranges, viewSelection.isBackward );
 
-			modelDocument.selection.setRanges( ranges, viewSelection.isBackward );
-		} );
+		if ( !modelSelection.isEqual( modelDocument.selection ) ) {
+			modelDocument.enqueueChanges( () => {
+				modelDocument.selection.setTo( modelSelection );
+			} );
+		}
 	};
 }

src/model/selection.js

@@ -125,25 +125,39 @@ export default class Selection {
 	}
 
 	/**
-	 * Checks whether, this selection is equal to given selection. Selections equal if they have the same ranges and directions.
+	 * Checks whether this selection is equal to given selection. Selections are equal if they have same directions,
+	 * same number of ranges and all ranges from one selection equal to a range from other selection.
 	 *
 	 * @param {engine.model.Selection} otherSelection Selection to compare with.
 	 * @returns {Boolean} `true` if selections are equal, `false` otherwise.
 	 */
 	isEqual( otherSelection ) {
-		const rangeCount = this.rangeCount;
+		if ( this.rangeCount != otherSelection.rangeCount ) {
+			return false;
+		} else if ( this.rangeCount === 0 ) {
+			return true;
+		}
 
-		if ( rangeCount != otherSelection.rangeCount ) {
+		if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) {
 			return false;
 		}
 
-		for ( let i = 0; i < this.rangeCount; i++ ) {
-			if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) {
+		for ( let thisRange of this._ranges ) {
+			let found = false;
+
+			for ( let otherRange of otherSelection._ranges ) {
+				if ( thisRange.isEqual( otherRange ) ) {
+					found = true;
+					break;
+				}
+			}
+
+			if ( !found ) {
 				return false;
 			}
 		}
 
-		return this.isBackward === otherSelection.isBackward;
+		return true;
 	}
 
 	/**

src/view/document.js

@@ -303,6 +303,15 @@ export default class Document {
 			observer.enable();
 		}
 	}
+
+	/**
+	 * Destroys all observers created by view `Document`.
+	 */
+	destroy() {
+		for ( let observer of this._observers.values() ) {
+			observer.destroy();
+		}
+	}
 }
 
 mix( Document, ObservableMixin );

src/view/observer/domeventobserver.js

@@ -56,7 +56,11 @@ export default class DomEventObserver extends Observer {
 		const types = typeof this.domEventType == 'string' ? [ this.domEventType ] : this.domEventType;
 
 		types.forEach( type => {
-			domElement.addEventListener( type, domEvent => this.isEnabled && this.onDomEvent( domEvent ) );
+			this.listenTo( domElement, type, ( eventInfo, domEvent ) => {
+				if ( this.isEnabled ) {
+					this.onDomEvent( domEvent );
+				}
+			} );
 		} );
 	}
 

src/view/observer/mutationobserver.js

@@ -112,6 +112,15 @@ export default class MutationObserver extends Observer {
 		this._mutationObserver.disconnect();
 	}
 
+	/**
+	 * @inheritDoc
+	 */
+	destroy() {
+		super.destroy();
+
+		this._mutationObserver.disconnect();
+	}
+
 	/**
 	 * Handles mutations. Deduplicates, mark view elements to sync, fire event and call render.
 	 *

src/view/observer/observer.js

@@ -3,6 +3,9 @@
  * For licensing, see LICENSE.md.
  */
 
+import DomEmitterMixin from '../../../utils/dom/emittermixin.js';
+import mix from '../../../utils/mix.js';
+
 /**
  * Abstract base observer class. Observers are classes which observe changes on DOM elements, do the preliminary
  * processing and fire events on the {@link engine.view.Document} objects. Observers can also add features to the view,
@@ -59,6 +62,14 @@ export default class Observer {
 		this.isEnabled = false;
 	}
 
+	/**
+	 * Disables and destroys the observer, among others removes event listeners created by the observer.
+	 */
+	destroy() {
+		this.disable();
+		this.stopListening();
+	}
+
 	/**
 	 * Starts observing the given root element.
 	 *
@@ -67,3 +78,5 @@ export default class Observer {
 	 * @param {String} name The name of the root element.
 	 */
 }
+
+mix( Observer, DomEmitterMixin );

src/view/observer/selectionobserver.js

@@ -3,9 +3,10 @@
  * For licensing, see LICENSE.md.
  */
 
+/* global setInterval, clearInterval */
+
 import Observer from './observer.js';
 import MutationObserver from './mutationobserver.js';
-
 import log from '../../../utils/log.js';
 
 /**
@@ -68,6 +69,8 @@ export default class SelectionObserver extends Observer {
 		 */
 		this._documents = new WeakSet();
 
+		this._clearInfiniteLoopInterval = setInterval( () => this._clearInfiniteLoop(), 2000 );
+
 		/**
 		 * Private property to store the last selection, to check if the code does not enter infinite loop.
 		 *
@@ -101,11 +104,22 @@ export default class SelectionObserver extends Observer {
 			return;
 		}
 
-		domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) );
+		this.listenTo( domDocument, 'selectionchange', () => {
+			this._handleSelectionChange( domDocument );
+		} );
 
 		this._documents.add( domDocument );
 	}
 
+	/**
+	 * @inheritDoc
+	 */
+	destroy() {
+		super.destroy();
+
+		clearInterval( this._clearInfiniteLoopInterval );
+	}
+
 	/**
 	 * Selection change listener. {@link engine.view.observer.MutationObserver#flush Flush} mutations, check if
 	 * selection changes and fires {@link engine.view.Document#selectionChange} event.
@@ -151,10 +165,6 @@ export default class SelectionObserver extends Observer {
 			newSelection: newViewSelection,
 			domSelection: domSelection
 		} );
-
-		// If nothing changes on `selectionChange` event, at this point we have "dirty DOM" (changed) and de-synched
-		// view (which has not been changed). In order to "reset DOM" we render the view again.
-		this.document.render();
 	}
 
 	/**
@@ -179,12 +189,25 @@ export default class SelectionObserver extends Observer {
 			this._loopbackCounter = 0;
 		}
 
-		if ( this._loopbackCounter > 10 ) {
+		// 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;
+	}
 }
 
 /**

src/view/selection.js

@@ -283,33 +283,47 @@ export default class Selection {
 	}
 
 	/**
-	 * Checks whether, this selection is equal to given selection. Selections equal if they have the same ranges and directions.
+	 * Checks whether, this selection is equal to given selection. Selections are equal if they have same directions,
+	 * same number of ranges and all ranges from one selection equal to a range from other selection.
 	 *
 	 * @param {engine.view.Selection} otherSelection Selection to compare with.
 	 * @returns {Boolean} `true` if selections are equal, `false` otherwise.
 	 */
 	isEqual( otherSelection ) {
-		const rangeCount = this.rangeCount;
+		if ( this.isFake != otherSelection.isFake ) {
+			return false;
+		}
 
-		if ( rangeCount != otherSelection.rangeCount ) {
+		if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) {
 			return false;
 		}
 
-		if ( this.isFake != otherSelection.isFake ) {
+		if ( this.rangeCount != otherSelection.rangeCount ) {
 			return false;
+		} else if ( this.rangeCount === 0 ) {
+			return true;
 		}
 
-		if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) {
+		if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) {
 			return false;
 		}
 
-		for ( let i = 0; i < this.rangeCount; i++ ) {
-			if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) {
+		for ( let thisRange of this._ranges ) {
+			let found = false;
+
+			for ( let otherRange of otherSelection._ranges ) {
+				if ( thisRange.isEqual( otherRange ) ) {
+					found = true;
+					break;
+				}
+			}
+
+			if ( !found ) {
 				return false;
 			}
 		}
 
-		return this._lastRangeBackward === otherSelection._lastRangeBackward;
+		return true;
 	}
 
 	/**

tests/controller/editingcontroller.js

@@ -27,18 +27,16 @@ import { getData as getViewData } from 'ckeditor5/engine/dev-utils/view.js';
 
 describe( 'EditingController', () => {
 	describe( 'constructor()', () => {
-		let model, editing;
-
-		beforeEach( () => {
-			model = new ModelDocument();
-			editing = new EditingController( model );
-		} );
-
 		it( 'should create controller with properties', () => {
+			const model = new ModelDocument();
+			const editing = new EditingController( model );
+
 			expect( editing ).to.have.property( 'model' ).that.equals( model );
 			expect( editing ).to.have.property( 'view' ).that.is.instanceof( ViewDocument );
 			expect( editing ).to.have.property( 'mapper' ).that.is.instanceof( Mapper );
 			expect( editing ).to.have.property( 'modelToView' ).that.is.instanceof( ModelConversionDispatcher );
+
+			editing.destroy();
 		} );
 	} );
 
@@ -53,6 +51,10 @@ describe( 'EditingController', () => {
 			editing = new EditingController( model );
 		} );
 
+		afterEach( () => {
+			editing.destroy();
+		} );
+
 		it( 'should create root', () => {
 			const domRoot = createElement( document, 'div', null, createElement( document, 'p' ) );
 
@@ -127,6 +129,7 @@ describe( 'EditingController', () => {
 		after( () => {
 			document.body.removeChild( domRoot );
 			listener.stopListening();
+			editing.destroy();
 		} );
 
 		beforeEach( () => {
@@ -273,6 +276,24 @@ describe( 'EditingController', () => {
 			} );
 
 			expect( spy.called ).to.be.false;
+
+			editing.destroy();
+		} );
+
+		it( 'should destroy view', () => {
+			let model, editing;
+
+			model = new ModelDocument();
+			model.createRoot();
+			model.schema.registerItem( 'paragraph', '$block' );
+
+			editing = new EditingController( model );
+
+			const spy = sinon.spy( editing.view, 'destroy' );
+
+			editing.destroy();
+
+			expect( spy.called ).to.be.true;
 		} );
 	} );
 } );

tests/conversion/buildmodelconverter.js

@@ -92,6 +92,10 @@ describe( 'Model converter builder', () => {
 		dispatcher.on( 'remove', remove() );
 	} );
 
+	afterEach( () => {
+		viewDoc.destroy();
+	} );
+
 	describe( 'model element to view element conversion', () => {
 		it( 'using passed view element name', () => {
 			buildModelConverter().for( dispatcher ).fromElement( 'paragraph' ).toElement( 'p' );