T/629b Alternative fix infinite selection loop.

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,29 @@ 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 ( 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 ] ) ) {
-				return false;
-			}
+		if ( this.rangeCount != otherSelection.rangeCount ) {
+			return false;
 		}
 
-		return this.isBackward === otherSelection.isBackward;
+		// Every range from this selection...
+		return Array.from( this.getRanges() ).every( ( rangeA ) => {
+			// ...Has a range in other selection...
+			return Array.from( otherSelection.getRanges() ).some( ( rangeB ) => {
+				// That it is equal to.
+				return rangeA.isEqual( rangeB );
+			} );
+		} );
 	}
 
 	/**

src/view/observer/selectionobserver.js

@@ -3,6 +3,8 @@
  * For licensing, see LICENSE.md.
  */
 
+/* global setInterval */
+
 import Observer from './observer.js';
 import MutationObserver from './mutationobserver.js';
 
@@ -103,6 +105,8 @@ export default class SelectionObserver extends Observer {
 
 		domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) );
 
+		setInterval( () => this._clearInfiniteLoop(), 2000 );
+
 		this._documents.add( domDocument );
 	}
 
@@ -151,10 +155,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 +179,23 @@ export default class SelectionObserver extends Observer {
 			this._loopbackCounter = 0;
 		}
 
-		if ( this._loopbackCounter > 10 ) {
+		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,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.view.Selection} otherSelection Selection to compare with.
 	 * @returns {Boolean} `true` if selections are equal, `false` otherwise.
 	 */
 	isEqual( otherSelection ) {
-		const rangeCount = this.rangeCount;
-
-		if ( rangeCount != otherSelection.rangeCount ) {
+		if ( this.isFake != otherSelection.isFake ) {
 			return false;
 		}
 
-		if ( this.isFake != otherSelection.isFake ) {
+		if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) {
 			return false;
 		}
 
-		if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) {
+		if ( this.rangeCount != otherSelection.rangeCount ) {
 			return false;
+		} else if ( this.rangeCount === 0 ) {
+			return true;
 		}
 
-		for ( let i = 0; i < this.rangeCount; i++ ) {
-			if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) {
-				return false;
-			}
+		if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) {
+			return false;
 		}
 
-		return this._lastRangeBackward === otherSelection._lastRangeBackward;
+		// Every range from this selection...
+		return Array.from( this.getRanges() ).every( ( rangeA ) => {
+			// ...Has a range in other selection...
+			return Array.from( otherSelection.getRanges() ).some( ( rangeB ) => {
+				// That it is equal to.
+				return rangeA.isEqual( rangeB );
+			} );
+		} );
 	}
 
 	/**

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

@@ -106,4 +106,20 @@ describe( 'convertSelectionChange', () => {
 		expect( modelGetData( model ) ).to.equal( '<paragraph>f[o]o</paragraph><paragraph>b[a]r</paragraph>' );
 		expect( model.selection.isBackward ).to.true;
 	} );
+
+	it( 'should not enqueue changes if selection has not changed', () => {
+		const viewSelection = new ViewSelection();
+		viewSelection.addRange( ViewRange.createFromParentsAndOffsets(
+			viewRoot.getChild( 0 ).getChild( 0 ), 1, viewRoot.getChild( 0 ).getChild( 0 ), 1 ) );
+
+		convertSelection( null, { newSelection: viewSelection } );
+
+		const spy = sinon.spy();
+
+		model.on( 'changesDone', spy );
+
+		convertSelection( null, { newSelection: viewSelection } );
+
+		expect( spy.called ).to.be.false;
+	} );
 } );

tests/model/selection.js

@@ -671,7 +671,7 @@ describe( 'Selection', () => {
 			selection.addRange( range2 );
 
 			const otherSelection = new Selection();
-			otherSelection.addRange( range1 );
+			otherSelection.addRange( range2 );
 
 			expect( selection.isEqual( otherSelection ) ).to.be.false;
 		} );

tests/tickets/629/1.html

@@ -0,0 +1,7 @@
+<head>
+	<link rel="stylesheet" href="%APPS_DIR%ckeditor/build/modules/amd/theme/ckeditor.css">
+</head>
+
+<div id="editor">
+	<p>Lorem ipsum dolor sit amet.</p>
+</div>

tests/tickets/629/1.js

@@ -0,0 +1,23 @@
+/**
+ * @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, window, document */
+
+import ClassicEditor from '/ckeditor5/editor-classic/classic.js';
+import Enter from '/ckeditor5/enter/enter.js';
+import Typing from '/ckeditor5/typing/typing.js';
+import Paragraph from '/ckeditor5/paragraph/paragraph.js';
+import Bold from '/ckeditor5/basic-styles/bold.js';
+
+ClassicEditor.create( document.querySelector( '#editor' ), {
+	features: [ Enter, Typing, Paragraph, Bold ],
+	toolbar: [ 'bold' ]
+} )
+.then( editor => {
+	window.editor = editor;
+} )
+.catch( err => {
+	console.error( err.stack );
+} );

tests/tickets/629/1.md

@@ -0,0 +1,16 @@
+@bender-ui: collapsed
+@bender-tags: ticket, 629, iteration4
+
+### Selection infinite loop [#629](https://github.com/ckeditor/ckeditor5-engine/issues/629)
+
+Before testing open console.
+
+1. Select part of text.
+2. Click bold or press <kbd>ctrl + b</kbd>.
+3. There should be no errors or warnings in console.
+
+
+1. Put caret somewhere in text.
+2. Press <kbd>left arrow</kbd>, then <kbd>right arrow</kbd>.
+3. Repeat as fast as you can for 2-3 seconds.
+4. There should be no errors or warnings in console.

tests/view/observer/selectionobserver.js

@@ -130,7 +130,10 @@ describe( 'SelectionObserver', () => {
 	} );
 
 	it( 'should warn and not enter infinite loop', ( done ) => {
-		let counter = 30;
+		// Reset infinite loop counters so other tests won't mess up with this test.
+		selectionObserver._clearInfiniteLoop();
+
+		let counter = 100;
 
 		const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
 		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) );
@@ -161,80 +164,27 @@ describe( 'SelectionObserver', () => {
 		changeDomSelection();
 	} );
 
-	it( 'should not be treated as an infinite loop if the position is different', ( done ) => {
-		let counter = 30;
-
+	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 );
-		viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) );
 
-		listenter.listenTo( viewDocument, 'selectionChange', () => {
-			counter--;
+		// Reset infinite loop counters so other tests won't mess up with this test.
+		selectionObserver._clearInfiniteLoop();
 
-			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--;
+		const spy = testUtils.sinon.spy( log, 'warn' );
 
-			if ( counter > 0 ) {
-				setTimeout( () => changeDomSelection() );
-			} else {
-				done();
-			}
-		} );
+		for ( let i = 0; i < 10; i++ ) {
+			changeDomSelection();
+		}
 
-		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 ) );
-
-		listenter.listenTo( viewDocument, 'selectionChange', () => {
-			setTimeout( () => {
-				const domSelection = document.getSelection();
-
-				expect( domSelection.rangeCount ).to.equal( 1 );
-
-				const domRange = domSelection.getRangeAt( 0 );
-				const domBar = document.getElementById( 'main' ).childNodes[ 1 ].childNodes[ 0 ];
-
-				expect( domRange.startContainer ).to.equal( domBar );
-				expect( domRange.startOffset ).to.equal( 0 );
-				expect( domRange.endContainer ).to.equal( domBar );
-				expect( domRange.endOffset ).to.equal( 1 );
-
-				done();
-			} );
-		} );
-
-		changeDomSelection();
+		setTimeout( () => {
+			expect( spy.called ).to.be.false;
+			done();
+		}, 400 );
 	} );
 } );
 
-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();