From f4c0d25398bf9a85376e2f24f32c26cff188e16e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 2 Nov 2016 09:34:38 +0100 Subject: [PATCH 01/21] Fixed: infinite selection fixing loop. --- .../view-selection-to-model-converters.js | 24 ++++++++++++------- src/model/selection.js | 22 ++++++++++------- src/view/observer/selectionobserver.js | 4 ---- 3 files changed, 29 insertions(+), 21 deletions(-) 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..bd79946c9 100644 --- a/src/model/selection.js +++ b/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 ); + } ); + } ); } /** diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index 215289a0f..efb1ed453 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -151,10 +151,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(); } /** From b9d6bc262ffb601025aeca0ab90ba064bdda24d1 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 2 Nov 2016 10:51:09 +0100 Subject: [PATCH 02/21] Fixed: clear infinite loop counter after user interaction. --- src/view/observer/selectionobserver.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index efb1ed453..072a0424f 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -102,6 +102,9 @@ export default class SelectionObserver extends Observer { } domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) ); + domDocument.addEventListener( 'keydown', () => this._clearInfiniteLoop() ); + domDocument.addEventListener( 'mousemove', () => this._clearInfiniteLoop() ); + domDocument.addEventListener( 'mousedown', () => this._clearInfiniteLoop() ); this._documents.add( domDocument ); } @@ -181,6 +184,17 @@ export default class SelectionObserver extends Observer { return false; } + + /** + * Clears `SelectionObserver` internal properties connected with preventing infinite loop. + * + * @private + */ + _clearInfiniteLoop() { + this._lastSelection = null; + this._lastButOneSelection = null; + this._loopbackCounter = 0; + } } /** From 1b39bfbc550a406d366bce912b99ef8854965a3c Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 2 Nov 2016 10:53:47 +0100 Subject: [PATCH 03/21] Fixed: view.Selection#isEqual better algorithm. --- src/view/selection.js | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/view/selection.js b/src/view/selection.js index 52d2460c1..5c6554c46 100644 --- a/src/view/selection.js +++ b/src/view/selection.js @@ -283,15 +283,20 @@ 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.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; } @@ -303,13 +308,14 @@ export default class Selection { return false; } - for ( let i = 0; i < this.rangeCount; i++ ) { - if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) { - 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 ); + } ); + } ); } /** From d2d1e73f199fb3be322864848c0ff5b88ff92a99 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 2 Nov 2016 12:59:18 +0100 Subject: [PATCH 04/21] Fixed: view.Selection#isEqual for fake selections. --- src/view/selection.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/view/selection.js b/src/view/selection.js index 5c6554c46..b196fac91 100644 --- a/src/view/selection.js +++ b/src/view/selection.js @@ -290,21 +290,21 @@ export default class Selection { * @returns {Boolean} `true` if selections are equal, `false` otherwise. */ isEqual( otherSelection ) { - if ( this.rangeCount != otherSelection.rangeCount ) { + if ( this.isFake != otherSelection.isFake ) { return false; - } else if ( this.rangeCount === 0 ) { - return true; } - if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) { + 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; } From a65682997b1b5dece29a2a1fe28649aa2cfb06cb Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 2 Nov 2016 14:31:14 +0100 Subject: [PATCH 05/21] Tests: fixed failed tests and 100% CC. --- src/model/selection.js | 2 +- .../view-selection-to-model-converters.js | 16 +++ tests/model/selection.js | 2 +- tests/view/observer/selectionobserver.js | 97 +++++++++++-------- 4 files changed, 76 insertions(+), 41 deletions(-) diff --git a/src/model/selection.js b/src/model/selection.js index bd79946c9..d4cec4904 100644 --- a/src/model/selection.js +++ b/src/model/selection.js @@ -125,7 +125,7 @@ export default class Selection { } /** - * Checks whether, this selection is equal to given selection. Selections are equal if they have same 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. diff --git a/tests/conversion/view-selection-to-model-converters.js b/tests/conversion/view-selection-to-model-converters.js index b6c3a5278..71fe05248 100644 --- a/tests/conversion/view-selection-to-model-converters.js +++ b/tests/conversion/view-selection-to-model-converters.js @@ -106,4 +106,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/model/selection.js b/tests/model/selection.js index 1b7676f10..cc91e06ee 100644 --- a/tests/model/selection.js +++ b/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; } ); diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index 08a704c66..f69bbb3b6 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, Range, document, KeyboardEvent, MouseEvent */ /* bender-tags: view, browser-only */ import ViewRange from '/ckeditor5/engine/view/range.js'; @@ -162,71 +162,90 @@ describe( 'SelectionObserver', () => { } ); 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 ) ); + let counter = 0; + + const spy = testUtils.sinon.spy( log, 'warn' ); + listenter.listenTo( viewDocument, 'selectionChange', () => { - counter--; + counter++; - if ( counter > 0 ) { - setTimeout( () => changeCollapsedDomSelection( counter ) ); - } else { - done(); + if ( counter < 15 ) { + setTimeout( changeCollapsedDomSelection, 100 ); } } ); - changeCollapsedDomSelection( counter ); - } ); + changeCollapsedDomSelection(); - it( 'should not be treated as an infinite loop if it is less then 3 times', ( done ) => { - let counter = 3; + setTimeout( () => { + expect( spy.called ).to.be.false; + done(); + }, 1500 ); + } ); + 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--; + const spy = testUtils.sinon.spy( log, 'warn' ); - if ( counter > 0 ) { - setTimeout( () => changeDomSelection() ); - } else { - done(); - } - } ); + for ( let i = 0; i < 4; i++ ) { + changeDomSelection(); + } - changeDomSelection(); + setTimeout( () => { + expect( spy.called ).to.be.false; + done(); + }, 1500 ); } ); - 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 ) ); + const events = { + keydown: KeyboardEvent, + mousedown: MouseEvent, + mousemove: MouseEvent + }; - listenter.listenTo( viewDocument, 'selectionChange', () => { - setTimeout( () => { - const domSelection = document.getSelection(); + for ( let event in events ) { + it( 'should not be treated as an infinite loop if change is triggered by ' + event + ' event', ( done ) => { + let counter = 0; - expect( domSelection.rangeCount ).to.equal( 1 ); + const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); + viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); - const domRange = domSelection.getRangeAt( 0 ); - const domBar = document.getElementById( 'main' ).childNodes[ 1 ].childNodes[ 0 ]; + const spy = testUtils.sinon.spy( log, 'warn' ); - expect( domRange.startContainer ).to.equal( domBar ); - expect( domRange.startOffset ).to.equal( 0 ); - expect( domRange.endContainer ).to.equal( domBar ); - expect( domRange.endOffset ).to.equal( 1 ); + listenter.listenTo( viewDocument, 'selectionChange', () => { + counter++; - done(); + if ( counter < 15 ) { + setTimeout( () => { + document.dispatchEvent( new events[ event ]( event ) ); + changeDomSelection(); + }, 100 ); + } } ); - } ); - changeDomSelection(); - } ); + setTimeout( () => { + expect( spy.called ).to.be.false; + done(); + }, 1000 ); + + document.dispatchEvent( new events[ event ]( event ) ); + changeDomSelection(); + } ); + } } ); -function changeCollapsedDomSelection( pos = 1 ) { +function changeCollapsedDomSelection() { const domSelection = document.getSelection(); + const pos = domSelection.anchorOffset + 1; + + if ( pos > 20 ) { + return; + } + domSelection.removeAllRanges(); const domFoo = document.getElementById( 'main' ).childNodes[ 0 ].childNodes[ 0 ]; const domRange = new Range(); From d80b36740b044d75f7852dbf7b5b20f311a4474c Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 9 Nov 2016 15:34:45 +0100 Subject: [PATCH 06/21] Changed: SelectionObserver clearing infinite loop counter in time intervals. --- src/view/observer/selectionobserver.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index 072a0424f..024433c92 100644 --- a/src/view/observer/selectionobserver.js +++ b/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'; @@ -102,9 +104,8 @@ export default class SelectionObserver extends Observer { } domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) ); - domDocument.addEventListener( 'keydown', () => this._clearInfiniteLoop() ); - domDocument.addEventListener( 'mousemove', () => this._clearInfiniteLoop() ); - domDocument.addEventListener( 'mousedown', () => this._clearInfiniteLoop() ); + + setInterval( () => this._clearInfiniteLoop(), 2000 ); this._documents.add( domDocument ); } @@ -178,7 +179,7 @@ export default class SelectionObserver extends Observer { this._loopbackCounter = 0; } - if ( this._loopbackCounter > 10 ) { + if ( this._loopbackCounter > 50 ) { return true; } @@ -188,7 +189,7 @@ export default class SelectionObserver extends Observer { /** * Clears `SelectionObserver` internal properties connected with preventing infinite loop. * - * @private + * @protected */ _clearInfiniteLoop() { this._lastSelection = null; From 317d3dc3fce329c62558b605fcda735f0cc2a332 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 9 Nov 2016 15:35:59 +0100 Subject: [PATCH 07/21] Tests: SelectionObserver infinite loop removed unstable unit tests and added manual test. --- tests/tickets/629/1.html | 7 ++ tests/tickets/629/1.js | 23 ++++++ tests/tickets/629/1.md | 16 +++++ tests/view/observer/selectionobserver.js | 89 +++--------------------- 4 files changed, 56 insertions(+), 79 deletions(-) create mode 100644 tests/tickets/629/1.html create mode 100644 tests/tickets/629/1.js create mode 100644 tests/tickets/629/1.md diff --git a/tests/tickets/629/1.html b/tests/tickets/629/1.html new file mode 100644 index 000000000..d7f095aa3 --- /dev/null +++ b/tests/tickets/629/1.html @@ -0,0 +1,7 @@ + + + + +
+

Lorem ipsum dolor sit amet.

+
diff --git a/tests/tickets/629/1.js b/tests/tickets/629/1.js new file mode 100644 index 000000000..41043e8ff --- /dev/null +++ b/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 ); +} ); diff --git a/tests/tickets/629/1.md b/tests/tickets/629/1.md new file mode 100644 index 000000000..20a284b85 --- /dev/null +++ b/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 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 fast more than 20 times. +4. There should be no errors or warnings in console. diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index f69bbb3b6..860b6a6d2 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, KeyboardEvent, MouseEvent */ +/* globals setTimeout, Range, document */ /* bender-tags: view, browser-only */ import ViewRange from '/ckeditor5/engine/view/range.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,99 +164,27 @@ describe( 'SelectionObserver', () => { changeDomSelection(); } ); - it( 'should not be treated as an infinite loop if the position is different', ( done ) => { + 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 ) ); - - let counter = 0; - - const spy = testUtils.sinon.spy( log, 'warn' ); - - listenter.listenTo( viewDocument, 'selectionChange', () => { - counter++; - if ( counter < 15 ) { - setTimeout( changeCollapsedDomSelection, 100 ); - } - } ); + // Reset infinite loop counters so other tests won't mess up with this test. + selectionObserver._clearInfiniteLoop(); - changeCollapsedDomSelection(); - - setTimeout( () => { - expect( spy.called ).to.be.false; - done(); - }, 1500 ); - } ); - - 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 ) ); const spy = testUtils.sinon.spy( log, 'warn' ); - for ( let i = 0; i < 4; i++ ) { + for ( let i = 0; i < 10; i++ ) { changeDomSelection(); } setTimeout( () => { expect( spy.called ).to.be.false; done(); - }, 1500 ); + }, 400 ); } ); - - const events = { - keydown: KeyboardEvent, - mousedown: MouseEvent, - mousemove: MouseEvent - }; - - for ( let event in events ) { - it( 'should not be treated as an infinite loop if change is triggered by ' + event + ' event', ( done ) => { - let counter = 0; - - const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); - viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); - - const spy = testUtils.sinon.spy( log, 'warn' ); - - listenter.listenTo( viewDocument, 'selectionChange', () => { - counter++; - - if ( counter < 15 ) { - setTimeout( () => { - document.dispatchEvent( new events[ event ]( event ) ); - changeDomSelection(); - }, 100 ); - } - } ); - - setTimeout( () => { - expect( spy.called ).to.be.false; - done(); - }, 1000 ); - - document.dispatchEvent( new events[ event ]( event ) ); - changeDomSelection(); - } ); - } } ); -function changeCollapsedDomSelection() { - const domSelection = document.getSelection(); - const pos = domSelection.anchorOffset + 1; - - if ( pos > 20 ) { - return; - } - - 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(); From f2201097f9db996c5d77c23e984d153664fd11d0 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 9 Nov 2016 15:39:08 +0100 Subject: [PATCH 08/21] Tests: updated manual test description. --- tests/tickets/629/1.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tickets/629/1.md b/tests/tickets/629/1.md index 20a284b85..a4643b0c1 100644 --- a/tests/tickets/629/1.md +++ b/tests/tickets/629/1.md @@ -12,5 +12,5 @@ Before testing open console. 1. Put caret somewhere in text. 2. Press left arrow, then right arrow. -3. Repeat fast more than 20 times. +3. Repeat as fast as you can for 2-3 seconds. 4. There should be no errors or warnings in console. From 6b6da0c5c735710f35923b9471092e88cdd5da2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 9 Nov 2016 17:35:23 +0100 Subject: [PATCH 09/21] Manual ticket tests should also be inside manual/ directory. --- tests/{ => manual}/tickets/629/1.html | 0 tests/{ => manual}/tickets/629/1.js | 0 tests/{ => manual}/tickets/629/1.md | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename tests/{ => manual}/tickets/629/1.html (100%) rename tests/{ => manual}/tickets/629/1.js (100%) rename tests/{ => manual}/tickets/629/1.md (100%) diff --git a/tests/tickets/629/1.html b/tests/manual/tickets/629/1.html similarity index 100% rename from tests/tickets/629/1.html rename to tests/manual/tickets/629/1.html diff --git a/tests/tickets/629/1.js b/tests/manual/tickets/629/1.js similarity index 100% rename from tests/tickets/629/1.js rename to tests/manual/tickets/629/1.js diff --git a/tests/tickets/629/1.md b/tests/manual/tickets/629/1.md similarity index 100% rename from tests/tickets/629/1.md rename to tests/manual/tickets/629/1.md From 3768f047870b0dea2013cb4a28fc940814e48e90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 10 Nov 2016 14:45:20 +0100 Subject: [PATCH 10/21] Tests: Improved cleanup. --- tests/view/observer/selectionobserver.js | 39 +++++++++++------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index b8b40274c..52a034ccf 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.js @@ -21,14 +21,14 @@ 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, listener, domRoot; - before( () => { + beforeEach( ( done ) => { domRoot = document.createElement( 'div' ); domRoot.innerHTML = `
`; document.body.appendChild( domRoot ); - listenter = Object.create( EmitterMixin ); + listener = Object.create( EmitterMixin ); viewDocument = new ViewDocument(); viewDocument.createRoot( document.getElementById( 'main' ) ); @@ -43,30 +43,27 @@ 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 ); + + listener.stopListening(); + selectionObserver.disable(); } ); it( 'should fire selectionChange when it is the only change', ( done ) => { - listenter.listenTo( viewDocument, 'selectionChange', ( evt, data ) => { + listener.listenTo( viewDocument, 'selectionChange', ( evt, data ) => { expect( data ).to.have.property( 'domSelection' ).that.equals( document.getSelection() ); expect( data ).to.have.property( 'oldSelection' ).that.is.instanceof( ViewSelection ); @@ -93,7 +90,7 @@ describe( 'SelectionObserver', () => { // Add second roots to ensure that listener is added once. viewDocument.createRoot( document.getElementById( 'additional' ), 'additional' ); - listenter.listenTo( viewDocument, 'selectionChange', () => { + listener.listenTo( viewDocument, 'selectionChange', () => { done(); } ); @@ -101,11 +98,11 @@ describe( 'SelectionObserver', () => { } ); it( 'should not fire selectionChange on render', ( done ) => { - listenter.listenTo( viewDocument, 'selectionChange', () => { + listener.listenTo( viewDocument, '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 +112,11 @@ describe( 'SelectionObserver', () => { it( 'should not fired if observer is disabled', ( done ) => { viewDocument.getObserver( SelectionObserver ).disable(); - listenter.listenTo( viewDocument, 'selectionChange', () => { + listener.listenTo( viewDocument, 'selectionChange', () => { throw 'selectionChange on render'; } ); - setTimeout( () => done(), 70 ); + setTimeout( done, 70 ); changeDomSelection(); } ); @@ -127,11 +124,11 @@ describe( 'SelectionObserver', () => { it( 'should not fired if there is no focus', ( done ) => { viewDocument.isFocused = false; - listenter.listenTo( viewDocument, 'selectionChange', () => { + listener.listenTo( viewDocument, 'selectionChange', () => { throw 'selectionChange on render'; } ); - setTimeout( () => done(), 70 ); + setTimeout( done, 70 ); changeDomSelection(); } ); @@ -145,13 +142,13 @@ describe( 'SelectionObserver', () => { const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); - listenter.listenTo( viewDocument, 'selectionChange', () => { + listener.listenTo( viewDocument, 'selectionChange', () => { counter--; if ( counter > 0 ) { setTimeout( changeDomSelection ); } else { - throw( 'Infinite loop!' ); + throw 'Infinite loop!'; } } ); From 3dc704c86d08f6b549d5c258a2ea5947fe7c4a75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 10 Nov 2016 14:57:47 +0100 Subject: [PATCH 11/21] Tests: Cleaning listeners should not be necessary since we're disabling the observer (so view#selChange won't be fired). --- tests/view/observer/selectionobserver.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index 52a034ccf..bb1928e99 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.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, listener, domRoot; + let viewDocument, viewRoot, mutationObserver, selectionObserver, domRoot; beforeEach( ( done ) => { domRoot = document.createElement( 'div' ); domRoot.innerHTML = `
`; document.body.appendChild( domRoot ); - listener = Object.create( EmitterMixin ); - viewDocument = new ViewDocument(); viewDocument.createRoot( document.getElementById( 'main' ) ); @@ -58,12 +55,11 @@ describe( 'SelectionObserver', () => { afterEach( () => { domRoot.parentElement.removeChild( domRoot ); - listener.stopListening(); selectionObserver.disable(); } ); it( 'should fire selectionChange when it is the only change', ( done ) => { - listener.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 ); @@ -90,7 +86,7 @@ describe( 'SelectionObserver', () => { // Add second roots to ensure that listener is added once. viewDocument.createRoot( document.getElementById( 'additional' ), 'additional' ); - listener.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { done(); } ); @@ -98,7 +94,7 @@ describe( 'SelectionObserver', () => { } ); it( 'should not fire selectionChange on render', ( done ) => { - listener.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { throw 'selectionChange on render'; } ); @@ -112,7 +108,7 @@ describe( 'SelectionObserver', () => { it( 'should not fired if observer is disabled', ( done ) => { viewDocument.getObserver( SelectionObserver ).disable(); - listener.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { throw 'selectionChange on render'; } ); @@ -124,7 +120,7 @@ describe( 'SelectionObserver', () => { it( 'should not fired if there is no focus', ( done ) => { viewDocument.isFocused = false; - listener.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { throw 'selectionChange on render'; } ); @@ -142,7 +138,7 @@ describe( 'SelectionObserver', () => { const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); - listener.listenTo( viewDocument, 'selectionChange', () => { + viewDocument.on( 'selectionChange', () => { counter--; if ( counter > 0 ) { From a8e53cadeb375562d17d90f6824c54df3d6406be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 10 Nov 2016 15:06:25 +0100 Subject: [PATCH 12/21] Tests: Fixed a selection observer test which could never work, but was hard to find because it depended on how you executed the tests. --- tests/view/observer/selectionobserver.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index bb1928e99..b41827475 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.js @@ -120,7 +120,14 @@ describe( 'SelectionObserver', () => { it( 'should not fired if there is no focus', ( done ) => { viewDocument.isFocused = false; + // 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'; } ); From dcf65b801b7b072df07c737f48b9ac0ffdae05bb Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 15 Nov 2016 12:59:36 +0100 Subject: [PATCH 13/21] Added: Implemented destroy chain for EditingController->observers->dom events. --- src/controller/editingcontroller.js | 1 + src/view/document.js | 6 +++ src/view/observer/domeventobserver.js | 6 ++- src/view/observer/mutationobserver.js | 6 +++ src/view/observer/observer.js | 10 ++++ src/view/observer/selectionobserver.js | 17 +++++-- tests/controller/editingcontroller.js | 19 ++++--- tests/conversion/buildmodelconverter.js | 4 ++ .../model-selection-to-view-converters.js | 4 ++ .../view-selection-to-model-converters.js | 4 ++ tests/dev-utils/view.js | 8 +++ tests/view/document/document.js | 50 +++++++------------ tests/view/document/integration.js | 6 +++ tests/view/document/jumpoverinlinefiller.js | 3 +- tests/view/domconverter/domconverter.js | 6 ++- tests/view/observer/clickobserver.js | 4 ++ tests/view/observer/domeventdata.js | 1 + tests/view/observer/domeventobserver.js | 5 +- tests/view/observer/fakeselectionobserver.js | 4 ++ tests/view/observer/focusobserver.js | 4 ++ tests/view/observer/keyobserver.js | 4 ++ tests/view/observer/mutationobserver.js | 1 + tests/view/observer/selectionobserver.js | 1 + tests/view/position.js | 2 + tests/view/selection.js | 2 + tests/view/treewalker.js | 4 ++ 26 files changed, 133 insertions(+), 49 deletions(-) diff --git a/src/controller/editingcontroller.js b/src/controller/editingcontroller.js index 64f87211e..9ab2412b4 100644 --- a/src/controller/editingcontroller.js +++ b/src/controller/editingcontroller.js @@ -141,6 +141,7 @@ export default class EditingController { * Removes all event listeners attached by the EditingController. */ destroy() { + this.view.destroy(); this._listenter.stopListening(); } } diff --git a/src/view/document.js b/src/view/document.js index 3eed92f9a..7fad2f77b 100644 --- a/src/view/document.js +++ b/src/view/document.js @@ -303,6 +303,12 @@ export default class Document { observer.enable(); } } + + 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..be3fceec5 100644 --- a/src/view/observer/mutationobserver.js +++ b/src/view/observer/mutationobserver.js @@ -112,6 +112,12 @@ export default class MutationObserver extends Observer { this._mutationObserver.disconnect(); } + 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..136be2a32 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,11 @@ export default class Observer { this.isEnabled = false; } + destroy() { + this.disable(); + this.stopListening(); + } + /** * Starts observing the given root element. * @@ -67,3 +75,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 024433c92..61f8f4a06 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -3,11 +3,10 @@ * For licensing, see LICENSE.md. */ -/* global setInterval */ +/* global setInterval, clearInterval */ import Observer from './observer.js'; import MutationObserver from './mutationobserver.js'; - import log from '../../../utils/log.js'; /** @@ -70,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. * @@ -103,13 +104,19 @@ export default class SelectionObserver extends Observer { return; } - domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) ); - - setInterval( () => this._clearInfiniteLoop(), 2000 ); + this.listenTo( domDocument, 'selectionchange', () => { + this._handleSelectionChange( domDocument ); + } ); this._documents.add( domDocument ); } + 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. diff --git a/tests/controller/editingcontroller.js b/tests/controller/editingcontroller.js index 47e800730..89f5a6740 100644 --- a/tests/controller/editingcontroller.js +++ b/tests/controller/editingcontroller.js @@ -28,18 +28,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(); } ); } ); @@ -54,6 +52,10 @@ describe( 'EditingController', () => { editing = new EditingController( model ); } ); + afterEach( () => { + editing.destroy(); + } ); + it( 'should create root', () => { const domRoot = createElement( document, 'div', null, createElement( document, 'p' ) ); @@ -128,6 +130,7 @@ describe( 'EditingController', () => { after( () => { document.body.removeChild( domRoot ); listener.stopListening(); + editing.destroy(); } ); beforeEach( () => { @@ -274,6 +277,8 @@ describe( 'EditingController', () => { } ); expect( spy.called ).to.be.false; + + editing.destroy(); } ); } ); } ); 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 1f52ea5f1..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( diff --git a/tests/dev-utils/view.js b/tests/dev-utils/view.js index 1384ab7e4..2772f4e01 100644 --- a/tests/dev-utils/view.js +++ b/tests/dev-utils/view.js @@ -47,6 +47,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', () => { @@ -67,6 +69,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', () => { @@ -91,6 +95,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', () => { @@ -106,6 +112,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/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 b41827475..8290f067a 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.js @@ -56,6 +56,7 @@ describe( 'SelectionObserver', () => { domRoot.parentElement.removeChild( domRoot ); selectionObserver.disable(); + viewDocument.destroy(); } ); it( 'should fire selectionChange when it is the only change', ( done ) => { 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..bae763608 100644 --- a/tests/view/selection.js +++ b/tests/view/selection.js @@ -706,6 +706,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( () => { From 0de2bf70911c6ad35b3c0e48e053346eef98c07a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 15 Nov 2016 13:00:29 +0100 Subject: [PATCH 14/21] Fixed: Selection#isEqual was throwing in the selection had no ranges. --- src/model/selection.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/model/selection.js b/src/model/selection.js index d4cec4904..be498cc7f 100644 --- a/src/model/selection.js +++ b/src/model/selection.js @@ -132,11 +132,13 @@ export default class Selection { * @returns {Boolean} `true` if selections are equal, `false` otherwise. */ isEqual( otherSelection ) { - if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) { + if ( this.rangeCount != otherSelection.rangeCount ) { return false; + } else if ( this.rangeCount === 0 ) { + return true; } - if ( this.rangeCount != otherSelection.rangeCount ) { + if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) { return false; } From 25b8ba2eb6ef54a211a672299cf32c6bf63dd97e Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 15 Nov 2016 16:03:30 +0100 Subject: [PATCH 15/21] Docs: added/fixed code comments. --- src/model/selection.js | 4 ++-- src/view/observer/selectionobserver.js | 2 ++ src/view/selection.js | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/model/selection.js b/src/model/selection.js index be498cc7f..8517f966c 100644 --- a/src/model/selection.js +++ b/src/model/selection.js @@ -144,9 +144,9 @@ export default class Selection { // Every range from this selection... return Array.from( this.getRanges() ).every( ( rangeA ) => { - // ...Has a range in other selection... + // ... has a range in other selection... return Array.from( otherSelection.getRanges() ).some( ( rangeB ) => { - // That it is equal to. + // ... which it is equal to. return rangeA.isEqual( rangeB ); } ); } ); diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index 61f8f4a06..99f81318c 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -186,6 +186,8 @@ export default class SelectionObserver extends Observer { 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; } diff --git a/src/view/selection.js b/src/view/selection.js index b196fac91..9187fa66c 100644 --- a/src/view/selection.js +++ b/src/view/selection.js @@ -310,9 +310,9 @@ export default class Selection { // Every range from this selection... return Array.from( this.getRanges() ).every( ( rangeA ) => { - // ...Has a range in other selection... + // ... has a range in other selection... return Array.from( otherSelection.getRanges() ).some( ( rangeB ) => { - // That it is equal to. + // ... which it is equal to. return rangeA.isEqual( rangeB ); } ); } ); From a3690d5d3cff728a624e72eba12f746118607078 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 15 Nov 2016 16:04:33 +0100 Subject: [PATCH 16/21] Tests: Additional tests for model.Selection and view.SelectionObserver. --- tests/model/selection.js | 6 +++ tests/view/observer/selectionobserver.js | 51 ++++++++++++++++++++---- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/tests/model/selection.js b/tests/model/selection.js index 509450fc9..5734aa3c1 100644 --- a/tests/model/selection.js +++ b/tests/model/selection.js @@ -666,6 +666,12 @@ 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 ); diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index 8290f067a..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'; @@ -55,7 +55,6 @@ describe( 'SelectionObserver', () => { afterEach( () => { domRoot.parentElement.removeChild( domRoot ); - selectionObserver.disable(); viewDocument.destroy(); } ); @@ -73,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 ); @@ -191,14 +190,50 @@ describe( 'SelectionObserver', () => { done(); }, 400 ); } ); + + 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' ); + + // 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; + clock.restore(); + done(); + }, 200 ); + }, 400 ); + } ); } ); 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 ); + const offset = domSelection.anchorOffset; + + domSelection.removeAllRanges(); + domSelection.collapse( domFoo, offset == 2 ? 3 : 2 ); } From adcdbdcbaad704a35a6cf0ec730f5d581e515102 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 16 Nov 2016 10:10:35 +0100 Subject: [PATCH 17/21] Changed: simplified implementation of model/view.Selection#isEqual. --- src/model/selection.js | 24 ++++++++++++++++-------- src/view/selection.js | 24 ++++++++++++++++-------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/model/selection.js b/src/model/selection.js index 8517f966c..3e1ed2bf4 100644 --- a/src/model/selection.js +++ b/src/model/selection.js @@ -142,14 +142,22 @@ export default class Selection { return false; } - // 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 ) => { - // ... which it is equal to. - return rangeA.isEqual( rangeB ); - } ); - } ); + 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 true; } /** diff --git a/src/view/selection.js b/src/view/selection.js index 9187fa66c..e5a0b3799 100644 --- a/src/view/selection.js +++ b/src/view/selection.js @@ -308,14 +308,22 @@ export default class Selection { return false; } - // 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 ) => { - // ... which it is equal to. - return rangeA.isEqual( rangeB ); - } ); - } ); + 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 true; } /** From 6a2418bfe85ac9d05cd4fa59711ab642fc1636a4 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 16 Nov 2016 10:10:51 +0100 Subject: [PATCH 18/21] Tests: added additional test for EditingController#destroy. --- tests/controller/editingcontroller.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/controller/editingcontroller.js b/tests/controller/editingcontroller.js index 89f5a6740..81dcce364 100644 --- a/tests/controller/editingcontroller.js +++ b/tests/controller/editingcontroller.js @@ -280,5 +280,21 @@ describe( 'EditingController', () => { 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; + } ); } ); } ); From 7ccaa133a0015aa8e6da74f91c3915e5f5c957e2 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 16 Nov 2016 11:21:18 +0100 Subject: [PATCH 19/21] Docs: added missing documentation. --- src/controller/editingcontroller.js | 3 ++- src/view/document.js | 3 +++ src/view/observer/mutationobserver.js | 3 +++ src/view/observer/observer.js | 3 +++ src/view/observer/selectionobserver.js | 3 +++ 5 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/controller/editingcontroller.js b/src/controller/editingcontroller.js index 9ab2412b4..5feddd862 100644 --- a/src/controller/editingcontroller.js +++ b/src/controller/editingcontroller.js @@ -138,7 +138,8 @@ 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(); diff --git a/src/view/document.js b/src/view/document.js index 7fad2f77b..cca039f51 100644 --- a/src/view/document.js +++ b/src/view/document.js @@ -304,6 +304,9 @@ export default class Document { } } + /** + * Destroys all observers created by view `Document`. + */ destroy() { for ( let observer of this._observers.values() ) { observer.destroy(); diff --git a/src/view/observer/mutationobserver.js b/src/view/observer/mutationobserver.js index be3fceec5..a10d51934 100644 --- a/src/view/observer/mutationobserver.js +++ b/src/view/observer/mutationobserver.js @@ -112,6 +112,9 @@ export default class MutationObserver extends Observer { this._mutationObserver.disconnect(); } + /** + * @inheritDoc + */ destroy() { super.destroy(); diff --git a/src/view/observer/observer.js b/src/view/observer/observer.js index 136be2a32..cb3e42df0 100644 --- a/src/view/observer/observer.js +++ b/src/view/observer/observer.js @@ -62,6 +62,9 @@ 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(); diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index 99f81318c..34103bb98 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -111,6 +111,9 @@ export default class SelectionObserver extends Observer { this._documents.add( domDocument ); } + /** + * @inheritDoc + */ destroy() { super.destroy(); From bf9fc9fe2528a8c90497870a641ff361eb358bfc Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 16 Nov 2016 11:29:07 +0100 Subject: [PATCH 20/21] Tests: expanded tests for model/view.Selection to cover more cases. --- tests/model/selection.js | 4 +++- tests/view/selection.js | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/model/selection.js b/tests/model/selection.js index 5734aa3c1..2f12b0cdc 100644 --- a/tests/model/selection.js +++ b/tests/model/selection.js @@ -682,11 +682,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 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/selection.js b/tests/view/selection.js index bae763608..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; } ); From bf87840c42758e8f91b68ead319d8d08bd578967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 16 Nov 2016 11:42:20 +0100 Subject: [PATCH 21/21] Tests: Should not log a warning which is expected to be logged. --- tests/model/liveselection.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 );