diff --git a/src/controller/editingcontroller.js b/src/controller/editingcontroller.js index 64f87211e..5feddd862 100644 --- a/src/controller/editingcontroller.js +++ b/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(); } } diff --git a/src/conversion/view-selection-to-model-converters.js b/src/conversion/view-selection-to-model-converters.js index 0e1e14d85..6b5fc4094 100644 --- a/src/conversion/view-selection-to-model-converters.js +++ b/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 ); + } ); + } }; } diff --git a/src/model/selection.js b/src/model/selection.js index 5268f89d3..3e1ed2bf4 100644 --- a/src/model/selection.js +++ b/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; } /** diff --git a/src/view/document.js b/src/view/document.js index 3eed92f9a..cca039f51 100644 --- a/src/view/document.js +++ b/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 ); diff --git a/src/view/observer/domeventobserver.js b/src/view/observer/domeventobserver.js index 0d720531f..799471669 100644 --- a/src/view/observer/domeventobserver.js +++ b/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 ); + } + } ); } ); } diff --git a/src/view/observer/mutationobserver.js b/src/view/observer/mutationobserver.js index 277a451a7..a10d51934 100644 --- a/src/view/observer/mutationobserver.js +++ b/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. * diff --git a/src/view/observer/observer.js b/src/view/observer/observer.js index 61f1b7995..cb3e42df0 100644 --- a/src/view/observer/observer.js +++ b/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 ); diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index 215289a0f..34103bb98 100644 --- a/src/view/observer/selectionobserver.js +++ b/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; + } } /** diff --git a/src/view/selection.js b/src/view/selection.js index 52d2460c1..e5a0b3799 100644 --- a/src/view/selection.js +++ b/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; } /** diff --git a/tests/controller/editingcontroller.js b/tests/controller/editingcontroller.js index 6691780a0..3271177a7 100644 --- a/tests/controller/editingcontroller.js +++ b/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; } ); } ); } ); diff --git a/tests/conversion/buildmodelconverter.js b/tests/conversion/buildmodelconverter.js index 7d9aacf89..3fe92b099 100644 --- a/tests/conversion/buildmodelconverter.js +++ b/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' ); diff --git a/tests/conversion/model-selection-to-view-converters.js b/tests/conversion/model-selection-to-view-converters.js index 181adbbcc..485a861aa 100644 --- a/tests/conversion/model-selection-to-view-converters.js +++ b/tests/conversion/model-selection-to-view-converters.js @@ -63,6 +63,10 @@ beforeEach( () => { dispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } ); } ); +afterEach( () => { + viewDoc.destroy(); +} ); + describe( 'default converters', () => { beforeEach( () => { // Selection converters for selection attributes. diff --git a/tests/conversion/view-selection-to-model-converters.js b/tests/conversion/view-selection-to-model-converters.js index 3c7c237e5..f2992c6f1 100644 --- a/tests/conversion/view-selection-to-model-converters.js +++ b/tests/conversion/view-selection-to-model-converters.js @@ -40,6 +40,10 @@ describe( 'convertSelectionChange', () => { convertSelection = convertSelectionChange( model, mapper ); } ); + afterEach( () => { + view.destroy(); + } ); + it( 'should convert collapsed selection', () => { const viewSelection = new ViewSelection(); viewSelection.addRange( ViewRange.createFromParentsAndOffsets( @@ -106,4 +110,20 @@ describe( 'convertSelectionChange', () => { expect( modelGetData( model ) ).to.equal( 'f[o]ob[a]r' ); 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; + } ); } ); diff --git a/tests/dev-utils/view.js b/tests/dev-utils/view.js index b762ab488..bf987d02f 100644 --- a/tests/dev-utils/view.js +++ b/tests/dev-utils/view.js @@ -48,6 +48,8 @@ describe( 'view test utils', () => { expect( stringifyOptions ).to.have.property( 'showType' ).that.equals( false ); expect( stringifyOptions ).to.have.property( 'showPriority' ).that.equals( false ); expect( stringifyOptions ).to.have.property( 'ignoreRoot' ).that.equals( true ); + + viewDocument.destroy(); } ); it( 'should use stringify method with selection', () => { @@ -68,6 +70,8 @@ describe( 'view test utils', () => { expect( stringifyOptions ).to.have.property( 'showType' ).that.equals( false ); expect( stringifyOptions ).to.have.property( 'showPriority' ).that.equals( false ); expect( stringifyOptions ).to.have.property( 'ignoreRoot' ).that.equals( true ); + + viewDocument.destroy(); } ); it( 'should throw an error when passing invalid document', () => { @@ -92,6 +96,8 @@ describe( 'view test utils', () => { expect( args[ 0 ] ).to.equal( data ); expect( args[ 1 ] ).to.be.an( 'object' ); expect( args[ 1 ].rootElement ).to.equal( viewDocument.getRoot() ); + + viewDocument.destroy(); } ); it( 'should use parse method with selection', () => { @@ -107,6 +113,8 @@ describe( 'view test utils', () => { expect( args[ 0 ] ).to.equal( data ); expect( args[ 1 ] ).to.be.an( 'object' ); expect( args[ 1 ].rootElement ).to.equal( viewDocument.getRoot() ); + + viewDocument.destroy(); } ); it( 'should throw an error when passing invalid document', () => { diff --git a/tests/manual/tickets/629/1.html b/tests/manual/tickets/629/1.html new file mode 100644 index 000000000..d7f095aa3 --- /dev/null +++ b/tests/manual/tickets/629/1.html @@ -0,0 +1,7 @@ + + + + +
+

Lorem ipsum dolor sit amet.

+
diff --git a/tests/manual/tickets/629/1.js b/tests/manual/tickets/629/1.js new file mode 100644 index 000000000..41043e8ff --- /dev/null +++ b/tests/manual/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 ); +} ); diff --git a/tests/manual/tickets/629/1.md b/tests/manual/tickets/629/1.md new file mode 100644 index 000000000..a4643b0c1 --- /dev/null +++ b/tests/manual/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 ctrl + b. +3. There should be no errors or warnings in console. + + +1. Put caret somewhere in text. +2. Press left arrow, then right arrow. +3. Repeat as fast as you can for 2-3 seconds. +4. There should be no errors or warnings in console. diff --git a/tests/model/liveselection.js b/tests/model/liveselection.js index d03deb17e..e076556ae 100644 --- a/tests/model/liveselection.js +++ b/tests/model/liveselection.js @@ -21,6 +21,8 @@ import count from 'ckeditor5/utils/count.js'; import testUtils from 'tests/core/_utils/utils.js'; import { wrapInDelta } from 'tests/engine/model/_utils/utils.js'; +import log from 'ckeditor5/utils/log.js'; + testUtils.createSinonSandbox(); describe( 'LiveSelection', () => { @@ -136,13 +138,16 @@ describe( 'LiveSelection', () => { } ); it( 'should not add a range that is in graveyard', () => { + const spy = testUtils.sinon.stub( log, 'warn' ); + selection.addRange( Range.createIn( doc.graveyard ) ); expect( selection._ranges.length ).to.equal( 0 ); + expect( spy.calledOnce ).to.be.true; } ); it( 'should refresh attributes', () => { - const spy = sinon.spy( selection, '_updateAttributes' ); + const spy = testUtils.sinon.spy( selection, '_updateAttributes' ); selection.addRange( range ); diff --git a/tests/model/selection.js b/tests/model/selection.js index d0812fcb1..2f12b0cdc 100644 --- a/tests/model/selection.js +++ b/tests/model/selection.js @@ -666,21 +666,29 @@ describe( 'Selection', () => { expect( selection.isEqual( otherSelection ) ).to.be.true; } ); + it( 'should return true if both selections have no ranges', () => { + const otherSelection = new Selection(); + + expect( selection.isEqual( otherSelection ) ).to.be.true; + } ); + it( 'should return false if ranges count does not equal', () => { selection.addRange( range1 ); selection.addRange( range2 ); const otherSelection = new Selection(); - otherSelection.addRange( range1 ); + otherSelection.addRange( range2 ); expect( selection.isEqual( otherSelection ) ).to.be.false; } ); - it( 'should return false if ranges do not equal', () => { + it( 'should return false if ranges (other than the last added range) do not equal', () => { selection.addRange( range1 ); + selection.addRange( range3 ); const otherSelection = new Selection(); otherSelection.addRange( range2 ); + otherSelection.addRange( range3 ); expect( selection.isEqual( otherSelection ) ).to.be.false; } ); diff --git a/tests/view/document/document.js b/tests/view/document/document.js index fe0a3cece..ca429ea85 100644 --- a/tests/view/document/document.js +++ b/tests/view/document/document.js @@ -25,7 +25,7 @@ testUtils.createSinonSandbox(); describe( 'Document', () => { const DEFAULT_OBSERVERS_COUNT = 5; - let ObserverMock, ObserverMockGlobalCount, instantiated, enabled, domRoot; + let ObserverMock, ObserverMockGlobalCount, instantiated, enabled, domRoot, viewDocument; before( () => { domRoot = createElement( document, 'div', { @@ -65,12 +65,16 @@ describe( 'Document', () => { enabled++; } }; + + viewDocument = new Document(); + } ); + + afterEach( () => { + viewDocument.destroy(); } ); describe( 'constructor()', () => { it( 'should create Document with all properties', () => { - const viewDocument = new Document(); - expect( count( viewDocument.domRoots ) ).to.equal( 0 ); expect( count( viewDocument.roots ) ).to.equal( 0 ); expect( viewDocument ).to.have.property( 'renderer' ).that.is.instanceOf( Renderer ); @@ -79,8 +83,6 @@ describe( 'Document', () => { } ); it( 'should add default observers', () => { - const viewDocument = new Document(); - expect( count( viewDocument._observers ) ).to.equal( DEFAULT_OBSERVERS_COUNT ); expect( viewDocument.getObserver( MutationObserver ) ).to.be.instanceof( MutationObserver ); expect( viewDocument.getObserver( SelectionObserver ) ).to.be.instanceof( SelectionObserver ); @@ -96,7 +98,6 @@ describe( 'Document', () => { const domDiv = document.createElement( 'div' ); domDiv.appendChild( domP ); - const viewDocument = new Document(); const ret = viewDocument.createRoot( domDiv ); expect( count( viewDocument.domRoots ) ).to.equal( 1 ); @@ -115,7 +116,10 @@ describe( 'Document', () => { } ); it( 'should call observe on each observer', () => { - const viewDocument = new Document( document.createElement( 'div' ) ); + // The variable will be overwritten. + viewDocument.destroy(); + + viewDocument = new Document( document.createElement( 'div' ) ); viewDocument.renderer.render = sinon.spy(); const domDiv1 = document.createElement( 'div' ); @@ -135,8 +139,6 @@ describe( 'Document', () => { it( 'should create "main" root by default', () => { const domDiv = document.createElement( 'div' ); - - const viewDocument = new Document(); const ret = viewDocument.createRoot( domDiv ); expect( count( viewDocument.domRoots ) ).to.equal( 1 ); @@ -152,8 +154,6 @@ describe( 'Document', () => { it( 'should create root with given name', () => { const domDiv = document.createElement( 'div' ); - - const viewDocument = new Document(); const ret = viewDocument.createRoot( domDiv, 'header' ); expect( count( viewDocument.domRoots ) ).to.equal( 1 ); @@ -168,7 +168,6 @@ describe( 'Document', () => { } ); it( 'should create root without attaching DOM element', () => { - const viewDocument = new Document(); const ret = viewDocument.createRoot( 'div' ); expect( count( viewDocument.domRoots ) ).to.equal( 0 ); @@ -180,8 +179,6 @@ describe( 'Document', () => { describe( 'attachDomRoot', () => { it( 'should create root without attach DOM element to the view element', () => { const domDiv = document.createElement( 'div' ); - - const viewDocument = new Document(); const viewRoot = viewDocument.createRoot( 'div' ); expect( count( viewDocument.domRoots ) ).to.equal( 0 ); @@ -201,8 +198,6 @@ describe( 'Document', () => { it( 'should create root without attach DOM element to the view element with given name', () => { const domH1 = document.createElement( 'h1' ); - - const viewDocument = new Document(); viewDocument.createRoot( 'DIV' ); const viewH1 = viewDocument.createRoot( 'h1', 'header' ); @@ -225,7 +220,6 @@ describe( 'Document', () => { describe( 'getRoot', () => { it( 'should return "main" root', () => { - const viewDocument = new Document(); viewDocument.createRoot( document.createElement( 'div' ) ); expect( count( viewDocument.roots ) ).to.equal( 1 ); @@ -234,7 +228,6 @@ describe( 'Document', () => { } ); it( 'should return named root', () => { - const viewDocument = new Document(); viewDocument.createRoot( document.createElement( 'h1' ), 'header' ); expect( count( viewDocument.roots ) ).to.equal( 1 ); @@ -244,13 +237,18 @@ describe( 'Document', () => { } ); describe( 'addObserver', () => { - let viewDocument; - beforeEach( () => { + // The variable will be overwritten. + viewDocument.destroy(); + viewDocument = new Document( document.createElement( 'div' ) ); viewDocument.renderer.render = sinon.spy(); } ); + afterEach( () => { + viewDocument.destroy(); + } ); + it( 'should be instantiated and enabled on adding', () => { const observerMock = viewDocument.addObserver( ObserverMock ); @@ -316,8 +314,6 @@ describe( 'Document', () => { describe( 'getObserver', () => { it( 'should return observer it it is added', () => { - const viewDocument = new Document(); - const addedObserverMock = viewDocument.addObserver( ObserverMock ); const getObserverMock = viewDocument.getObserver( ObserverMock ); @@ -326,7 +322,6 @@ describe( 'Document', () => { } ); it( 'should return undefined if observer is not added', () => { - const viewDocument = new Document(); const getObserverMock = viewDocument.getObserver( ObserverMock ); expect( getObserverMock ).to.be.undefined; @@ -335,8 +330,6 @@ describe( 'Document', () => { describe( 'disableObservers', () => { it( 'should disable observers', () => { - const viewDocument = new Document(); - const addedObserverMock = viewDocument.addObserver( ObserverMock ); expect( addedObserverMock.enable.calledOnce ).to.be.true; @@ -351,8 +344,6 @@ describe( 'Document', () => { describe( 'enableObservers', () => { it( 'should enable observers', () => { - const viewDocument = new Document(); - const addedObserverMock = viewDocument.addObserver( ObserverMock ); viewDocument.disableObservers(); @@ -369,8 +360,6 @@ describe( 'Document', () => { describe( 'isFocused', () => { it( 'should change renderer.isFocused too', () => { - const viewDocument = new Document(); - expect( viewDocument.isFocused ).to.equal( false ); expect( viewDocument.renderer.isFocused ).to.equal( false ); @@ -382,10 +371,9 @@ describe( 'Document', () => { } ); describe( 'focus', () => { - let viewDocument, domEditable, viewEditable; + let domEditable, viewEditable; beforeEach( () => { - viewDocument = new Document(); domEditable = document.createElement( 'div' ); domEditable.setAttribute( 'contenteditable', 'true' ); document.body.appendChild( domEditable ); diff --git a/tests/view/document/integration.js b/tests/view/document/integration.js index 43784566d..6e56d866c 100644 --- a/tests/view/document/integration.js +++ b/tests/view/document/integration.js @@ -25,6 +25,8 @@ describe( 'Document integration', () => { expect( domDiv.childNodes.length ).to.equal( 1 ); expect( isBlockFiller( domDiv.childNodes[ 0 ], BR_FILLER ) ).to.be.true; + + viewDocument.destroy(); } ); it( 'should render changes in the Document', () => { @@ -38,6 +40,8 @@ describe( 'Document integration', () => { expect( domDiv.childNodes.length ).to.equal( 1 ); expect( domDiv.childNodes[ 0 ].tagName ).to.equal( 'P' ); + + viewDocument.destroy(); } ); it( 'should render attribute changes', () => { @@ -58,5 +62,7 @@ describe( 'Document integration', () => { expect( domRoot.childNodes.length ).to.equal( 1 ); expect( domRoot.childNodes[ 0 ].getAttribute( 'class' ) ).to.equal( 'bar' ); + + viewDocument.destroy(); } ); } ); diff --git a/tests/view/document/jumpoverinlinefiller.js b/tests/view/document/jumpoverinlinefiller.js index ea899c176..0cfabe9d3 100644 --- a/tests/view/document/jumpoverinlinefiller.js +++ b/tests/view/document/jumpoverinlinefiller.js @@ -33,8 +33,7 @@ describe( 'Document', () => { } ); afterEach( () => { - // There's no destroy() method yet, so let's at least kill the observers. - viewDocument.disableObservers(); + viewDocument.destroy(); domRoot.parentElement.removeChild( domRoot ); } ); diff --git a/tests/view/domconverter/domconverter.js b/tests/view/domconverter/domconverter.js index 5d3df984f..cc3a4bda1 100644 --- a/tests/view/domconverter/domconverter.js +++ b/tests/view/domconverter/domconverter.js @@ -33,10 +33,11 @@ describe( 'DomConverter', () => { } ); describe( 'focus', () => { - let viewEditable, domEditable; + let viewEditable, domEditable, viewDocument; beforeEach( () => { - viewEditable = new ViewEditable( new ViewDocument(), 'div' ); + viewDocument = new ViewDocument(); + viewEditable = new ViewEditable( viewDocument, 'div' ); domEditable = document.createElement( 'div' ); converter.bindElements( domEditable, viewEditable ); domEditable.setAttribute( 'contenteditable', 'true' ); @@ -45,6 +46,7 @@ describe( 'DomConverter', () => { afterEach( () => { document.body.removeChild( domEditable ); + viewDocument.destroy(); } ); it( 'should call focus on corresponding DOM editable', () => { diff --git a/tests/view/observer/clickobserver.js b/tests/view/observer/clickobserver.js index 5d8064761..576a34817 100644 --- a/tests/view/observer/clickobserver.js +++ b/tests/view/observer/clickobserver.js @@ -17,6 +17,10 @@ describe( 'ClickObserver', () => { observer = viewDocument.addObserver( ClickObserver ); } ); + afterEach( () => { + viewDocument.destroy(); + } ); + it( 'should define domEventType', () => { expect( observer.domEventType ).to.equal( 'click' ); } ); diff --git a/tests/view/observer/domeventdata.js b/tests/view/observer/domeventdata.js index 728ce77f8..8809298fc 100644 --- a/tests/view/observer/domeventdata.js +++ b/tests/view/observer/domeventdata.js @@ -35,6 +35,7 @@ describe( 'DomEventData', () => { afterEach( () => { domRoot.parentElement.removeChild( domRoot ); + viewDocument.destroy(); } ); describe( 'constructor()', () => { diff --git a/tests/view/observer/domeventobserver.js b/tests/view/observer/domeventobserver.js index b4d71d907..f64984309 100644 --- a/tests/view/observer/domeventobserver.js +++ b/tests/view/observer/domeventobserver.js @@ -41,6 +41,10 @@ describe( 'DomEventObserver', () => { viewDocument = new ViewDocument(); } ); + afterEach( () => { + viewDocument.destroy(); + } ); + describe( 'constructor()', () => { it( 'should create Observer with properties', () => { const observer = new DomEventObserver( viewDocument ); @@ -107,7 +111,6 @@ describe( 'DomEventObserver', () => { it( 'should fire event if observer is disabled and re-enabled', () => { const domElement = document.createElement( 'p' ); const domEvent = new MouseEvent( 'click' ); - const viewDocument = new ViewDocument(); const evtSpy = sinon.spy(); viewDocument.createRoot( domElement, 'root' ); diff --git a/tests/view/observer/fakeselectionobserver.js b/tests/view/observer/fakeselectionobserver.js index f1ff60a4c..c8cda135d 100644 --- a/tests/view/observer/fakeselectionobserver.js +++ b/tests/view/observer/fakeselectionobserver.js @@ -36,6 +36,10 @@ describe( 'FakeSelectionObserver', () => { viewDocument.selection.setFake(); } ); + afterEach( () => { + viewDocument.destroy(); + } ); + it( 'should do nothing if selection is not fake', () => { viewDocument.selection.setFake( false ); diff --git a/tests/view/observer/focusobserver.js b/tests/view/observer/focusobserver.js index 0f6e4c10d..006350a38 100644 --- a/tests/view/observer/focusobserver.js +++ b/tests/view/observer/focusobserver.js @@ -18,6 +18,10 @@ describe( 'FocusObserver', () => { observer = viewDocument.getObserver( FocusObserver ); } ); + afterEach( () => { + viewDocument.destroy(); + } ); + it( 'should define domEventType', () => { expect( observer.domEventType ).to.deep.equal( [ 'focus', 'blur' ] ); } ); diff --git a/tests/view/observer/keyobserver.js b/tests/view/observer/keyobserver.js index d2f434c2d..0dce8a3ab 100644 --- a/tests/view/observer/keyobserver.js +++ b/tests/view/observer/keyobserver.js @@ -18,6 +18,10 @@ describe( 'KeyObserver', () => { observer = viewDocument.getObserver( KeyObserver ); } ); + afterEach( () => { + viewDocument.destroy(); + } ); + it( 'should define domEventType', () => { expect( observer.domEventType ).to.equal( 'keydown' ); } ); diff --git a/tests/view/observer/mutationobserver.js b/tests/view/observer/mutationobserver.js index fcf297c7d..62b662444 100644 --- a/tests/view/observer/mutationobserver.js +++ b/tests/view/observer/mutationobserver.js @@ -41,6 +41,7 @@ describe( 'MutationObserver', () => { mutationObserver.disable(); domRoot.parentElement.removeChild( domRoot ); + viewDocument.destroy(); } ); it( 'should handle typing', () => { diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index 0bd45d2e7..1c8d560f7 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -/* globals setTimeout, Range, document */ +/* globals setTimeout, document */ /* bender-tags: view, browser-only */ import ViewRange from 'ckeditor5/engine/view/range.js'; @@ -13,7 +13,6 @@ 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 'ckeditor5/engine/dev-utils/view.js'; @@ -21,15 +20,13 @@ import { parse } from 'ckeditor5/engine/dev-utils/view.js'; testUtils.createSinonSandbox(); describe( 'SelectionObserver', () => { - let viewDocument, viewRoot, mutationObserver, selectionObserver, listenter, domRoot; + let viewDocument, viewRoot, mutationObserver, selectionObserver, domRoot; - before( () => { + beforeEach( ( done ) => { domRoot = document.createElement( 'div' ); domRoot.innerHTML = `
`; document.body.appendChild( domRoot ); - listenter = Object.create( EmitterMixin ); - viewDocument = new ViewDocument(); viewDocument.createRoot( document.getElementById( 'main' ) ); @@ -43,30 +40,26 @@ describe( 'SelectionObserver', () => { 'yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy' ) ); viewDocument.render(); - } ); - - after( () => { - domRoot.parentElement.removeChild( domRoot ); - } ); - beforeEach( ( done ) => { viewDocument.selection.removeAllRanges(); document.getSelection().removeAllRanges(); viewDocument.isFocused = true; - viewDocument.getObserver( SelectionObserver ).enable(); + selectionObserver.enable(); // Ensure selectionchange will not be fired. setTimeout( () => done(), 100 ); } ); afterEach( () => { - listenter.stopListening(); + domRoot.parentElement.removeChild( domRoot ); + + viewDocument.destroy(); } ); it( 'should fire selectionChange when it is the only change', ( done ) => { - listenter.listenTo( viewDocument, 'selectionChange', ( evt, data ) => { + viewDocument.on( 'selectionChange', ( evt, data ) => { expect( data ).to.have.property( 'domSelection' ).that.equals( document.getSelection() ); expect( data ).to.have.property( 'oldSelection' ).that.is.instanceof( ViewSelection ); @@ -79,7 +72,7 @@ describe( 'SelectionObserver', () => { const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); expect( newViewRange.start.parent ).to.equal( viewFoo ); - expect( newViewRange.start.offset ).to.equal( 1 ); + expect( newViewRange.start.offset ).to.equal( 2 ); expect( newViewRange.end.parent ).to.equal( viewFoo ); expect( newViewRange.end.offset ).to.equal( 2 ); @@ -93,7 +86,7 @@ describe( 'SelectionObserver', () => { // Add second roots to ensure that listener is added once. viewDocument.createRoot( document.getElementById( 'additional' ), 'additional' ); - listenter.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { done(); } ); @@ -101,11 +94,11 @@ describe( 'SelectionObserver', () => { } ); it( 'should not fire selectionChange on render', ( done ) => { - listenter.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { throw 'selectionChange on render'; } ); - setTimeout( () => done(), 70 ); + setTimeout( done, 70 ); const viewBar = viewDocument.getRoot().getChild( 1 ).getChild( 0 ); viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewBar, 1, viewBar, 2 ) ); @@ -115,11 +108,11 @@ describe( 'SelectionObserver', () => { it( 'should not fired if observer is disabled', ( done ) => { viewDocument.getObserver( SelectionObserver ).disable(); - listenter.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { throw 'selectionChange on render'; } ); - setTimeout( () => done(), 70 ); + setTimeout( done, 70 ); changeDomSelection(); } ); @@ -127,28 +120,38 @@ describe( 'SelectionObserver', () => { it( 'should not fired if there is no focus', ( done ) => { viewDocument.isFocused = false; - listenter.listenTo( viewDocument, 'selectionChange', () => { + // changeDomSelection() may focus the editable element (happens on Chrome) + // so cancel this because it sets the isFocused flag. + viewDocument.on( 'focus', ( evt ) => evt.stop(), { priority: 'highest' } ); + + viewDocument.on( 'selectionChange', () => { + // Validate the correctness of the test. May help tracking issue with this test. + expect( viewDocument.isFocused ).to.be.false; + throw 'selectionChange on render'; } ); - setTimeout( () => done(), 70 ); + setTimeout( done, 70 ); changeDomSelection(); } ); 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 ) ); - listenter.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { counter--; if ( counter > 0 ) { setTimeout( changeDomSelection ); } else { - throw( 'Infinite loop!' ); + throw 'Infinite loop!'; } } ); @@ -168,86 +171,69 @@ 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--; - - 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; + // Reset infinite loop counters so other tests won't mess up with this test. + selectionObserver._clearInfiniteLoop(); - 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(); + setTimeout( () => { + expect( spy.called ).to.be.false; + done(); + }, 400 ); } ); - 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 ) ); + it( 'should not be treated as an infinite loop if changes are not often', ( done ) => { + const clock = testUtils.sinon.useFakeTimers( 'setInterval', 'clearInterval' ); + const spy = testUtils.sinon.spy( log, 'warn' ); - listenter.listenTo( viewDocument, 'selectionChange', () => { - setTimeout( () => { - const domSelection = document.getSelection(); + // 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; - expect( domSelection.rangeCount ).to.equal( 1 ); + for ( let i = 0; i < changeCount; i++ ) { + setTimeout( () => { + changeDomSelection(); + }, i * 20 ); + } - const domRange = domSelection.getRangeAt( 0 ); - const domBar = document.getElementById( 'main' ).childNodes[ 1 ].childNodes[ 0 ]; + setTimeout( () => { + // Move the clock by 2100ms which will trigger callback added to `setInterval` and reset the inf-loop counter. + clock.tick( 2100 ); - expect( domRange.startContainer ).to.equal( domBar ); - expect( domRange.startOffset ).to.equal( 0 ); - expect( domRange.endContainer ).to.equal( domBar ); - expect( domRange.endOffset ).to.equal( 1 ); + for ( let i = 0; i < changeCount; i++ ) { + changeDomSelection(); + } + setTimeout( () => { + expect( spy.called ).to.be.false; + clock.restore(); done(); - } ); - } ); - - changeDomSelection(); + }, 200 ); + }, 400 ); } ); } ); -function changeCollapsedDomSelection( pos = 1 ) { +function changeDomSelection() { 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 ); -} + const offset = domSelection.anchorOffset; -function changeDomSelection() { - const domSelection = document.getSelection(); domSelection.removeAllRanges(); - const domFoo = document.getElementById( 'main' ).childNodes[ 0 ].childNodes[ 0 ]; - const domRange = new Range(); - domRange.setStart( domFoo, 1 ); - domRange.setEnd( domFoo, 2 ); - domSelection.addRange( domRange ); + domSelection.collapse( domFoo, offset == 2 ? 3 : 2 ); } diff --git a/tests/view/position.js b/tests/view/position.js index f9ae053ee..5f081abe2 100644 --- a/tests/view/position.js +++ b/tests/view/position.js @@ -463,6 +463,8 @@ describe( 'Position', () => { const position = new Position( p, 0 ); expect( position.editableElement ).to.equal( editable ); + + document.destroy(); } ); } ); } ); diff --git a/tests/view/selection.js b/tests/view/selection.js index 5e8d1fed2..1c2874870 100644 --- a/tests/view/selection.js +++ b/tests/view/selection.js @@ -443,11 +443,13 @@ describe( 'Selection', () => { expect( selection.isEqual( otherSelection ) ).to.be.false; } ); - it( 'should return false if ranges do not equal', () => { + it( 'should return false if ranges (other than the last added one) do not equal', () => { selection.addRange( range1 ); + selection.addRange( range3 ); const otherSelection = new Selection(); otherSelection.addRange( range2 ); + otherSelection.addRange( range3 ); expect( selection.isEqual( otherSelection ) ).to.be.false; } ); @@ -706,6 +708,8 @@ describe( 'Selection', () => { selection.addRange( Range.createFromParentsAndOffsets( element, 0, element, 0 ) ); expect( selection.editableElement ).to.equal( root ); + + viewDocument.destroy(); } ); } ); diff --git a/tests/view/treewalker.js b/tests/view/treewalker.js index d8dc30cb6..4ce8ac0ed 100644 --- a/tests/view/treewalker.js +++ b/tests/view/treewalker.js @@ -53,6 +53,10 @@ describe( 'TreeWalker', () => { rootEnding = new Position( root, 2 ); } ); + afterEach( () => { + doc.destroy(); + } ); + describe( 'constructor()', () => { it( 'should throw if neither boundaries nor starting position is set', () => { expect( () => {