Merge pull request #254 from ckeditor/t/248

Other: Made the UI destruction a fail–safe, repeatable process. Closes #248.

Author: Reinmar

src/panel/balloon/contextualballoon.js

@@ -205,14 +205,4 @@ export default class ContextualBalloon extends Plugin {
 	_getBalloonPosition() {
 		return this._stack.values().next().value.position;
 	}
-
-	/**
-	 * @inheritDoc
-	 */
-	destroy() {
-		this.editor.ui.view.body.remove( this.view );
-		this.view.destroy();
-		this._stack.clear();
-		super.destroy();
-	}
 }

src/view.js

@@ -309,14 +309,7 @@ export default class View {
 	destroy() {
 		this.stopListening();
 
-		return Promise.all( this._viewCollections.map( c => c.destroy() ) )
-			.then( () => {
-				this._unboundChildren.clear();
-				this._viewCollections.clear();
-
-				this.element = this.template = this.locale = this.t =
-					this._viewCollections = this._unboundChildren = null;
-			} );
+		return Promise.all( this._viewCollections.map( c => c.destroy() ) );
 	}
 
 	/**

tests/editableui/editableuiview.js

@@ -85,6 +85,16 @@ describe( 'EditableUIView', () => {
 			} );
 		} );
 
+		it( 'can be called multiple times', done => {
+			expect( () => {
+				view.destroy().then( () => {
+					return view.destroy().then( () => {
+						done();
+					} );
+				} );
+			} ).to.not.throw();
+		} );
+
 		describe( 'when #editableElement as an argument', () => {
 			it( 'reverts contentEditable property of editableElement (was false)', () => {
 				editableElement = document.createElement( 'div' );

tests/editorui/editoruiview.js

@@ -51,5 +51,15 @@ describe( 'EditorUIView', () => {
 				expect( el.parentNode ).to.be.null;
 			} );
 		} );
+
+		it( 'can be called multiple times', done => {
+			expect( () => {
+				view.destroy().then( () => {
+					return view.destroy().then( () => {
+						done();
+					} );
+				} );
+			} ).to.not.throw();
+		} );
 	} );
 } );

tests/panel/balloon/contextualballoon.js

@@ -352,11 +352,17 @@ describe( 'ContextualBalloon', () => {
 	} );
 
 	describe( 'destroy()', () => {
-		it( 'should remove balloon panel view from editor body collection and clear stack', () => {
+		it( 'can be called multiple times', () => {
+			expect( () => {
+				balloon.destroy();
+				balloon.destroy();
+			} ).to.not.throw();
+		} );
+
+		it( 'should not touch the DOM', () => {
 			balloon.destroy();
 
-			expect( editor.ui.view.body.getIndex( balloon.view ) ).to.equal( -1 );
-			expect( balloon.visibleView ).to.null;
+			expect( editor.ui.view.body.getIndex( balloon.view ) ).to.not.equal( -1 );
 		} );
 	} );
 } );

tests/toolbar/contextual/contextualtoolbar.js

@@ -291,6 +291,13 @@ describe( 'ContextualToolbar', () => {
 	} );
 
 	describe( 'destroy()', () => {
+		it( 'can be called multiple times', () => {
+			expect( () => {
+				contextualToolbar.destroy();
+				contextualToolbar.destroy();
+			} ).to.not.throw();
+		} );
+
 		it( 'should not fire `_selectionChangeDebounced` after plugin destroy', done => {
 			const spy = sandbox.spy();
 

tests/toolbar/sticky/stickytoolbarview.js

@@ -178,6 +178,16 @@ describe( 'StickyToolbarView', () => {
 			expect( view.destroy() ).to.be.instanceof( Promise );
 		} );
 
+		it( 'can be called multiple times', done => {
+			expect( () => {
+				view.destroy().then( () => {
+					return view.destroy().then( () => {
+						done();
+					} );
+				} );
+			} ).to.not.throw();
+		} );
+
 		it( 'calls destroy on parent class', () => {
 			const spy = testUtils.sinon.spy( ToolbarView.prototype, 'destroy' );
 

tests/view.js

@@ -262,41 +262,55 @@ describe( 'View', () => {
 		beforeEach( createViewWithChildren );
 
 		it( 'should return a promise', () => {
-			expect( view.destroy() ).to.be.instanceof( Promise );
+			const promise = view.destroy();
+
+			expect( promise ).to.be.instanceof( Promise );
+
+			return promise;
+		} );
+
+		it( 'can be called multiple times', done => {
+			expect( () => {
+				view.destroy().then( () => {
+					return view.destroy().then( () => {
+						done();
+					} );
+				} );
+			} ).to.not.throw();
 		} );
 
-		it( 'should set basic properties null', () => {
+		it( 'should not touch the basic properties', () => {
 			return view.destroy().then( () => {
-				expect( view.element ).to.be.null;
-				expect( view.template ).to.be.null;
-				expect( view.locale ).to.be.null;
-				expect( view.t ).to.be.null;
+				expect( view.element ).to.be.an.instanceof( HTMLElement );
+				expect( view.template ).to.be.an.instanceof( Template );
+				expect( view.locale ).to.be.an( 'object' );
+				expect( view.locale.t ).to.be.a( 'function' );
 
-				expect( view._unboundChildren ).to.be.null;
-				expect( view._viewCollections ).to.be.null;
+				expect( view._viewCollections ).to.be.instanceOf( Collection );
+				expect( view._unboundChildren ).to.be.instanceOf( ViewCollection );
 			} );
 		} );
 
-		it( 'clears #_unboundChildren', () => {
+		it( 'should not clear the #_unboundChildren', () => {
 			const cached = view._unboundChildren;
 
 			return view.addChildren( [ new View(), new View() ] )
 				.then( () => {
-					expect( cached ).to.have.length.above( 2 );
+					expect( cached ).to.have.length( 4 );
 
 					return view.destroy().then( () => {
-						expect( cached ).to.have.length( 0 );
+						expect( cached ).to.have.length( 4 );
 					} );
 				} );
 		} );
 
-		it( 'clears #_viewCollections', () => {
+		it( 'should not clear the #_viewCollections', () => {
 			const cached = view._viewCollections;
 
 			expect( cached ).to.have.length( 1 );
 
 			return view.destroy().then( () => {
-				expect( cached ).to.have.length( 0 );
+				expect( cached ).to.have.length( 1 );
 			} );
 		} );
 
@@ -326,11 +340,15 @@ describe( 'View', () => {
 		} );
 
 		it( 'destroy a template–less view', () => {
+			let promise;
+
 			view = new View();
 
 			expect( () => {
-				view.destroy();
+				promise = view.destroy();
 			} ).to.not.throw();
+
+			return promise;
 		} );
 
 		// https://github.com/ckeditor/ckeditor5-ui/issues/203
@@ -383,6 +401,8 @@ function setTestViewClass( templateDef ) {
 		constructor() {
 			super();
 
+			this.locale = { t() {} };
+
 			if ( templateDef ) {
 				this.template = new Template( templateDef );
 			}