Browse Source

Merge branch 'master' into t/390

Oskar Wrobel 9 years ago
parent
commit
02f912477c

+ 1 - 1
packages/ckeditor5-engine/src/model/batch.js

@@ -71,7 +71,7 @@ export default class Batch {
 	/**
 	 * Gets an iterable collection of operations.
 	 *
-	 * @returns {Iterable.<engine.model.operation.Operation}
+	 * @returns {Iterable.<engine.model.operation.Operation>}
 	 */
 	*getOperations() {
 		for ( let delta of this.deltas ) {

+ 0 - 2
packages/ckeditor5-engine/src/view/observer/mutationobserver.js

@@ -75,8 +75,6 @@ export default class MutationObserver extends Observer {
 	/**
 	 * Synchronously fires {@link engine.view.Document#mutations} event with all mutations in record queue.
 	 * At the same time empties the queue so mutations will not be fired twice.
-	 *
-	 * @returns {[type]} [description]
 	 */
 	flush() {
 		this._onMutations( this._mutationObserver.takeRecords() );

+ 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;
+	}
 }
 
 /**

+ 12 - 0
packages/ckeditor5-engine/tests/_utils-tests/model.js

@@ -47,6 +47,12 @@ describe( 'model test utils', () => {
 			sinon.assert.calledOnce( stringifySpy );
 			sinon.assert.calledWithExactly( stringifySpy, root, document.selection );
 		} );
+
+		it( 'should throw an error when passing invalid document', () => {
+			expect( () => {
+				getData( { invalid: 'document' } );
+			} ).to.throw( TypeError, 'Document needs to be an instance of engine.model.Document.' );
+		} );
 	} );
 
 	describe( 'setData', () => {
@@ -75,6 +81,12 @@ describe( 'model test utils', () => {
 			const args = parseSpy.firstCall.args;
 			expect( args[ 0 ] ).to.equal( data );
 		} );
+
+		it( 'should throw an error when passing invalid document', () => {
+			expect( () => {
+				setData( { invalid: 'document' } );
+			} ).to.throw( TypeError, 'Document needs to be an instance of engine.model.Document.' );
+		} );
 	} );
 
 	describe( 'stringify', () => {

+ 12 - 0
packages/ckeditor5-engine/tests/_utils-tests/view.js

@@ -68,6 +68,12 @@ describe( 'view test utils', () => {
 				expect( stringifyOptions ).to.have.property( 'showPriority' ).that.equals( false );
 				expect( stringifyOptions ).to.have.property( 'ignoreRoot' ).that.equals( true );
 			} );
+
+			it( 'should throw an error when passing invalid document', () => {
+				expect( () => {
+					getData( { invalid: 'document' } );
+				} ).to.throw( TypeError, 'Document needs to be an instance of engine.view.Document.' );
+			} );
 		} );
 
 		describe( 'setData', () => {
@@ -101,6 +107,12 @@ describe( 'view test utils', () => {
 				expect( args[ 1 ] ).to.be.an( 'object' );
 				expect( args[ 1 ].rootElement ).to.equal( viewDocument.getRoot() );
 			} );
+
+			it( 'should throw an error when passing invalid document', () => {
+				expect( () => {
+					setData( { invalid: 'document' } );
+				} ).to.throw( TypeError, 'Document needs to be an instance of engine.view.Document.' );
+			} );
 		} );
 	} );
 

+ 8 - 0
packages/ckeditor5-engine/tests/_utils/model.js

@@ -27,6 +27,10 @@ import Document from '/ckeditor5/engine/model/document.js';
  * @returns {String} The stringified data.
  */
 export function getData( document, options = {} ) {
+	if ( !( document instanceof Document ) ) {
+		throw new TypeError( 'Document needs to be an instance of engine.model.Document.' );
+	}
+
 	const withoutSelection = !!options.withoutSelection;
 	const rootName = options.rootName || 'main';
 	const root = document.getRoot( rootName );
@@ -47,6 +51,10 @@ getData._stringify = stringify;
  * used.
  */
 export function setData( document, data, options = {} ) {
+	if ( !( document instanceof Document ) ) {
+		throw new TypeError( 'Document needs to be an instance of engine.model.Document.' );
+	}
+
 	setData._parse( data, {
 		document: document,
 		rootName: options.rootName

+ 10 - 1
packages/ckeditor5-engine/tests/_utils/view.js

@@ -5,6 +5,7 @@
 
 'use strict';
 
+import Document from '/ckeditor5/engine/view/document.js';
 import ViewDocumentFragment from '/ckeditor5/engine/view/documentfragment.js';
 import HtmlDataProcessor from '/ckeditor5/engine/dataprocessor/htmldataprocessor.js';
 import ViewElement from '/ckeditor5/engine/view/element.js';
@@ -36,6 +37,10 @@ const TEXT_RANGE_END_TOKEN = '}';
  * @returns {String} The stringified data.
  */
 export function getData( document, options = {} ) {
+	if ( !( document instanceof Document ) ) {
+		throw new TypeError( 'Document needs to be an instance of engine.view.Document.' );
+	}
+
 	const withoutSelection = !!options.withoutSelection;
 	const rootName = options.rootName || 'main';
 	const root = document.getRoot( rootName );
@@ -63,6 +68,10 @@ getData._stringify = stringify;
  * used.
  */
 export function setData( document, data, options = {} ) {
+	if ( !( document instanceof Document ) ) {
+		throw new TypeError( 'Document needs to be an instance of engine.view.Document.' );
+	}
+
 	const rootName = options.rootName || 'main';
 	const root = document.getRoot( rootName );
 	const result = setData._parse( data, { rootElement: root } );
@@ -421,7 +430,7 @@ class RangeParser {
 	 * Uses all found bracket positions to create ranges from them.
 	 *
 	 * @private
-	 * @returns {Array.<engine.view.Range}
+	 * @returns {Array.<engine.view.Range>}
 	 */
 	_createRanges() {
 		const ranges = [];

+ 1 - 1
packages/ckeditor5-engine/tests/dataprocessor/htmldataprocessor.js

@@ -8,7 +8,7 @@
 'use strict';
 
 import HtmlDataProcessor from '/ckeditor5/engine/dataprocessor/htmldataprocessor.js';
-import xssTemplates from './_utils/xsstemplates.js';
+import xssTemplates from '/tests/engine/dataprocessor/_utils/xsstemplates.js';
 
 describe( 'HtmlDataProcessor', () => {
 	const dataProcessor = new HtmlDataProcessor();

+ 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();