From f1c34368e56b584b3e34d5758f7c54e75cfda04d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 11 Apr 2017 13:42:21 +0200 Subject: [PATCH 1/6] Improved ContextualBalloon to deal with an async Views. --- src/contextualballoon.js | 21 +++-- tests/contextualballoon.js | 185 +++++++++++++++---------------------- 2 files changed, 88 insertions(+), 118 deletions(-) diff --git a/src/contextualballoon.js b/src/contextualballoon.js index 9586821c..7f4837fe 100644 --- a/src/contextualballoon.js +++ b/src/contextualballoon.js @@ -83,8 +83,9 @@ export default class ContextualBalloon extends Plugin { * Adds a new view to the stack and makes it visible. * * @param {Object} data Configuration of the view. - * @param {module:ui/view~View} view Content of the balloon. - * @param {module:utils/dom/position~Options} position Positioning options. + * @param {module:ui/view~View} [data.view] Content of the balloon. + * @param {module:utils/dom/position~Options} [data.position] Positioning options. + * @returns {Promise} A Promise resolved when the child {@link module:ui/view~View#init} is done. */ add( data ) { if ( this.hasView( data.view ) ) { @@ -105,7 +106,7 @@ export default class ContextualBalloon extends Plugin { // Add new view to the stack. this._stack.set( data.view, data ); // And display it. - this._show( data.view ); + return this._show( data.view ); } /** @@ -114,6 +115,7 @@ export default class ContextualBalloon extends Plugin { * When there is no view in the stack then balloon will hide. * * @param {module:ui/view~View} view A view to be removed from the balloon. + * @returns {Promise} A Promise resolved when the preceding view is ready. */ remove( view ) { if ( !this.hasView( view ) ) { @@ -125,6 +127,9 @@ export default class ContextualBalloon extends Plugin { throw new CKEditorError( 'contextualballoon-remove-view-not-exist: Cannot remove configuration of not existing view.' ); } + // A Promise resolved when the preceding view is ready. + let promise = Promise.resolve(); + // When visible view is being removed. if ( this.visibleView === view ) { // We need to remove it from the view content. @@ -139,7 +144,7 @@ export default class ContextualBalloon extends Plugin { // If it is some other view. if ( last ) { // Just show it. - this._show( last.view ); + promise = this._show( last.view ); } else { // Hide the balloon panel. this.view.hide(); @@ -148,6 +153,8 @@ export default class ContextualBalloon extends Plugin { // Just remove given view from the stack. this._stack.delete( view ); } + + return promise; } /** @@ -164,10 +171,12 @@ export default class ContextualBalloon extends Plugin { * * @private * @param {module:ui/view~View} view View to show in the balloon. + * @returns {Promise} A Promise resolved when the child {@link module:ui/view~View#init} is done. */ _show( view ) { - this.view.content.add( view ); - this.view.attachTo( this._getBalloonPosition() ); + return this.view.content.add( view ).then( () => { + this.view.attachTo( this._getBalloonPosition() ); + } ); } /** diff --git a/tests/contextualballoon.js b/tests/contextualballoon.js index 3b339638..cea71cb8 100644 --- a/tests/contextualballoon.js +++ b/tests/contextualballoon.js @@ -11,7 +11,7 @@ import Template from '../src/template'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; -/* global document */ +/* global document, setTimeout */ describe( 'ContextualBalloon', () => { let editor, editorElement, balloon, viewA, viewB; @@ -27,11 +27,20 @@ describe( 'ContextualBalloon', () => { editor = newEditor; balloon = editor.plugins.get( ContextualBalloon ); + // We don't need to test attachTo method of BalloonPanel it's enough to check if was called with proper data. + sinon.stub( balloon.view, 'attachTo', () => {} ); + viewA = new ViewA(); viewB = new ViewB(); - // We don't need to test attachTo method of BalloonPanel it's enough to check if was called with proper data. - sinon.stub( balloon.view, 'attachTo', () => {} ); + // Add viewA to the pane and init viewB. + return Promise.all( [ + balloon.add( { + view: viewA, + position: { target: 'fake' } + } ), + viewB.init(), + ] ); } ); } ); @@ -61,20 +70,10 @@ describe( 'ContextualBalloon', () => { describe( 'hasView()', () => { it( 'should return true when given view is in stack', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - expect( balloon.hasView( viewA ) ).to.true; } ); it( 'should return true when given view is in stack but is not visible', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - balloon.add( { view: viewB, position: { target: 'fake' } @@ -85,17 +84,26 @@ describe( 'ContextualBalloon', () => { } ); it( 'should return false when given view is not in stack', () => { - expect( balloon.hasView( viewA ) ).to.false; + expect( balloon.hasView( viewB ) ).to.false; } ); } ); describe( 'add()', () => { - it( 'should add view to the stack and display in balloon', () => { - balloon.add( { - view: viewA, + it( 'should return promise resolved when view is ready', ( done ) => { + const clock = sinon.useFakeTimers(); + const view = new LongInitView(); + + const result = balloon.add( { + view: view, position: { target: 'fake' } } ); + expect( result ).to.instanceof( Promise ); + result.then( done ); + clock.tick( 11 ); + } ); + + it( 'should add view to the stack and display in balloon attached using given position options', () => { expect( balloon.view.content.length ).to.equal( 1 ); expect( balloon.view.content.get( 0 ) ).to.deep.equal( viewA ); expect( balloon.view.attachTo.calledOnce ).to.true; @@ -103,11 +111,6 @@ describe( 'ContextualBalloon', () => { } ); it( 'should throw an error when try to add the same view more than once', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - expect( () => { balloon.add( { view: viewA, @@ -117,11 +120,6 @@ describe( 'ContextualBalloon', () => { } ); it( 'should add multiple views to he stack and display last one', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - balloon.add( { view: viewB, position: { target: 'fake' } @@ -131,47 +129,31 @@ describe( 'ContextualBalloon', () => { expect( balloon.view.content.get( 0 ) ).to.deep.equal( viewB ); } ); - it( 'should add multiple views to the stack and keep balloon in the same position', () => { - balloon.add( { - view: viewA, - position: { target: 'fake', foo: 'bar' } - } ); - - balloon.add( { + it( 'should keep balloon at the same position after adding next view', () => { + return balloon.add( { view: viewB, - position: { target: 'fake', bar: 'biz' } - } ); + position: { target: 'other' } + } ) + .then( () => { + expect( balloon.view.attachTo.calledTwice ).to.true; - expect( balloon.view.attachTo.calledTwice ).to.true; - - expect( balloon.view.attachTo.firstCall.args[ 0 ] ).to.deep.equal( { - target: 'fake', - foo: 'bar' - } ); + expect( balloon.view.attachTo.firstCall.args[ 0 ] ).to.deep.equal( { + target: 'fake' + } ); - expect( balloon.view.attachTo.secondCall.args[ 0 ] ).to.deep.equal( { - target: 'fake', - foo: 'bar' + expect( balloon.view.attachTo.secondCall.args[ 0 ] ).to.deep.equal( { + target: 'fake' + } ); } ); } ); } ); describe( 'visibleView', () => { it( 'should return data of currently visible view', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - expect( balloon.visibleView ).to.equal( viewA ); } ); it( 'should return data of currently visible view when there is more than one in the stack', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - balloon.add( { view: viewB, position: { target: 'fake' } @@ -181,28 +163,26 @@ describe( 'ContextualBalloon', () => { } ); it( 'should return `null` when the stack is empty', () => { + balloon.remove( viewA ); expect( balloon.visibleView ).to.null; } ); } ); describe( 'remove()', () => { + it( 'should return promise', () => { + expect( balloon.remove( viewA ) ).to.instanceof( Promise ); + } ); + it( 'should remove given view and hide balloon when there is no other view to display', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); + balloon.view.isVisible = true; balloon.remove( viewA ); expect( balloon.visibleView ).to.null; + expect( balloon.view.isVisible ).to.false; } ); - it( 'should remove given view and set previous in the stack as visible when removed view was visible', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - + it( 'should remove given view and set preceding in the stack as visible when removed view was visible', () => { balloon.add( { view: viewB, position: { target: 'fake' } @@ -213,12 +193,26 @@ describe( 'ContextualBalloon', () => { expect( balloon.visibleView ).to.equal( viewA ); } ); - it( 'should remove given view from the stack when view is not visible', () => { + it( 'should wait for init of preceding view when was is not ready', ( done ) => { + const clock = sinon.useFakeTimers(); + const view = new LongInitView(); + + balloon.add( { + view: view, + position: { target: 'fake' } + } ); + balloon.add( { - view: viewA, + view: viewB, position: { target: 'fake' } } ); + balloon.remove( viewB ).then( done ); + + clock.tick( 11 ); + } ); + + it( 'should remove given view from the stack when view is not visible', () => { balloon.add( { view: viewB, position: { target: 'fake' } @@ -231,18 +225,13 @@ describe( 'ContextualBalloon', () => { it( 'should throw an error when there is no given view in the stack', () => { expect( () => { - balloon.remove( viewA ); + balloon.remove( viewB ); } ).to.throw( CKEditorError, /^contextualballoon-remove-view-not-exist/ ); } ); } ); describe( 'updatePosition()', () => { it( 'should attach balloon to the target using the same position options as currently set', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - balloon.view.attachTo.reset(); balloon.updatePosition(); @@ -252,19 +241,10 @@ describe( 'ContextualBalloon', () => { } ); it( 'should attach balloon to the target using the same position options as currently set when there is more than one view', () => { - balloon.add( { - view: viewA, - position: { - target: 'fake', - foo: 'bar' - } - } ); - balloon.add( { view: viewB, position: { - target: 'fake', - bar: 'biz' + target: 'other' } } ); @@ -274,30 +254,13 @@ describe( 'ContextualBalloon', () => { expect( balloon.view.attachTo.calledOnce ); expect( balloon.view.attachTo.firstCall.args[ 0 ] ).to.deep.equal( { - target: 'fake', - foo: 'bar' - } ); - } ); - - it( 'should remove given view from the stack when view is not visible', () => { - balloon.add( { - view: viewA, - position: { target: 'fake' } - } ); - - balloon.add( { - view: viewB, - position: { target: 'fake' } + target: 'fake' } ); - - balloon.remove( viewA ); - - expect( balloon.visibleView ).to.equal( viewB ); } ); it( 'should throw an error when there is no given view in the stack', () => { expect( () => { - balloon.remove( viewA ); + balloon.remove( viewB ); } ).to.throw( CKEditorError, /^contextualballoon-remove-view-not-exist/ ); } ); } ); @@ -311,22 +274,20 @@ describe( 'ContextualBalloon', () => { } ); } ); -class ViewA extends View { +class GenericView extends View { constructor( locale ) { super( locale ); - this.template = new Template( { - tag: 'div' - } ); + this.template = new Template( { tag: 'div' } ); } } -class ViewB extends View { - constructor( locale ) { - super( locale ); +class ViewA extends GenericView {} - this.template = new Template( { - tag: 'div' - } ); +class ViewB extends GenericView {} + +class LongInitView extends GenericView { + init() { + setTimeout( () => super.init(), 10 ); } } From 9986871137bf9f9cadcfa5199631826a56093acb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 11 Apr 2017 14:01:05 +0200 Subject: [PATCH 2/6] Simplified the test. --- tests/contextualballoon.js | 51 +++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/tests/contextualballoon.js b/tests/contextualballoon.js index cea71cb8..683a9d4a 100644 --- a/tests/contextualballoon.js +++ b/tests/contextualballoon.js @@ -7,7 +7,6 @@ import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictest import ContextualBalloon from '../src/contextualballoon'; import BalloonPanelView from '../src/panel/balloon/balloonpanelview'; import View from '../src/view'; -import Template from '../src/template'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; @@ -30,8 +29,8 @@ describe( 'ContextualBalloon', () => { // We don't need to test attachTo method of BalloonPanel it's enough to check if was called with proper data. sinon.stub( balloon.view, 'attachTo', () => {} ); - viewA = new ViewA(); - viewB = new ViewB(); + viewA = new View(); + viewB = new View(); // Add viewA to the pane and init viewB. return Promise.all( [ @@ -91,7 +90,17 @@ describe( 'ContextualBalloon', () => { describe( 'add()', () => { it( 'should return promise resolved when view is ready', ( done ) => { const clock = sinon.useFakeTimers(); - const view = new LongInitView(); + + const view = { + init: () => { + return new Promise( ( resolve ) => { + setTimeout( () => { + resolve(); + }, 10 ); + } ); + }, + destroy: () => {} + }; const result = balloon.add( { view: view, @@ -99,8 +108,11 @@ describe( 'ContextualBalloon', () => { } ); expect( result ).to.instanceof( Promise ); + result.then( done ); + clock.tick( 11 ); + clock.restore(); } ); it( 'should add view to the stack and display in balloon attached using given position options', () => { @@ -195,7 +207,17 @@ describe( 'ContextualBalloon', () => { it( 'should wait for init of preceding view when was is not ready', ( done ) => { const clock = sinon.useFakeTimers(); - const view = new LongInitView(); + + const view = { + init: () => { + return new Promise( ( resolve ) => { + setTimeout( () => { + resolve(); + }, 10 ); + } ); + }, + destroy: () => {} + }; balloon.add( { view: view, @@ -210,6 +232,7 @@ describe( 'ContextualBalloon', () => { balloon.remove( viewB ).then( done ); clock.tick( 11 ); + clock.restore(); } ); it( 'should remove given view from the stack when view is not visible', () => { @@ -273,21 +296,3 @@ describe( 'ContextualBalloon', () => { } ); } ); } ); - -class GenericView extends View { - constructor( locale ) { - super( locale ); - - this.template = new Template( { tag: 'div' } ); - } -} - -class ViewA extends GenericView {} - -class ViewB extends GenericView {} - -class LongInitView extends GenericView { - init() { - setTimeout( () => super.init(), 10 ); - } -} From a9417d4b66f99d1ae07415bf865f100de4bddbbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 12 Apr 2017 12:12:28 +0200 Subject: [PATCH 3/6] Improved ContextualBalloon initialization. --- src/contextualballoon.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/contextualballoon.js b/src/contextualballoon.js index 7f4837fe..67354142 100644 --- a/src/contextualballoon.js +++ b/src/contextualballoon.js @@ -49,8 +49,8 @@ export default class ContextualBalloon extends Plugin { */ this._stack = new Map(); - // Add balloon panel view to editor `body` collection. - this.editor.ui.view.body.add( this.view ); + // Add balloon panel view to editor `body` collection and wait until view will be ready. + return this.editor.ui.view.body.add( this.view ); } static get pluginName() { From 133dbb9e967c0628c69302b96a99ad26c875f10e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 12 Apr 2017 12:15:16 +0200 Subject: [PATCH 4/6] Improved destroying of the ContextualBalloon. --- src/contextualballoon.js | 1 + tests/contextualballoon.js | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/contextualballoon.js b/src/contextualballoon.js index 67354142..ea9773e0 100644 --- a/src/contextualballoon.js +++ b/src/contextualballoon.js @@ -196,6 +196,7 @@ export default class ContextualBalloon extends Plugin { destroy() { this.editor.ui.view.body.remove( this.view ); this.view.destroy(); + this._stack.clear(); super.destroy(); } } diff --git a/tests/contextualballoon.js b/tests/contextualballoon.js index 683a9d4a..4181764b 100644 --- a/tests/contextualballoon.js +++ b/tests/contextualballoon.js @@ -289,10 +289,11 @@ describe( 'ContextualBalloon', () => { } ); describe( 'destroy()', () => { - it( 'should balloon panel remove view from editor body collection', () => { + it( 'should remove balloon panel view from editor body collection and clear stack', () => { balloon.destroy(); expect( editor.ui.view.body.getIndex( balloon.view ) ).to.equal( -1 ); + expect( balloon.visibleView ).to.null; } ); } ); } ); From 59d60ece1ebea442218f294397af4c6903aa4280 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Wed, 12 Apr 2017 12:19:45 +0200 Subject: [PATCH 5/6] Moved ContextualBalloon to the `panel/balloon` directory. --- src/{ => panel/balloon}/contextualballoon.js | 4 ++-- tests/manual/contextualballoon/contextualballoon.js | 2 +- tests/{ => panel/balloon}/contextualballoon.js | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) rename src/{ => panel/balloon}/contextualballoon.js (98%) rename tests/{ => panel/balloon}/contextualballoon.js (97%) diff --git a/src/contextualballoon.js b/src/panel/balloon/contextualballoon.js similarity index 98% rename from src/contextualballoon.js rename to src/panel/balloon/contextualballoon.js index ea9773e0..df72c155 100644 --- a/src/contextualballoon.js +++ b/src/panel/balloon/contextualballoon.js @@ -4,11 +4,11 @@ */ /** - * @module ui/contextualballoon + * @module ui/panel/balloon/contextualballoon */ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; -import BalloonPanelView from './panel/balloon/balloonpanelview'; +import BalloonPanelView from './balloonpanelview'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** diff --git a/tests/manual/contextualballoon/contextualballoon.js b/tests/manual/contextualballoon/contextualballoon.js index c3697aa8..5f16b477 100644 --- a/tests/manual/contextualballoon/contextualballoon.js +++ b/tests/manual/contextualballoon/contextualballoon.js @@ -16,7 +16,7 @@ import EssentialsPresets from '@ckeditor/ckeditor5-presets/src/essentials'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; -import ContextualBalloon from '../../../src/contextualballoon'; +import ContextualBalloon from '../../../src/panel/balloon/contextualballoon'; import ToolbarView from '../../../src/toolbar/toolbarview'; import ButtonView from '../../../src/button/buttonview'; import Template from '../../../src/template'; diff --git a/tests/contextualballoon.js b/tests/panel/balloon/contextualballoon.js similarity index 97% rename from tests/contextualballoon.js rename to tests/panel/balloon/contextualballoon.js index 4181764b..f3ae48d6 100644 --- a/tests/contextualballoon.js +++ b/tests/panel/balloon/contextualballoon.js @@ -4,9 +4,9 @@ */ import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; -import ContextualBalloon from '../src/contextualballoon'; -import BalloonPanelView from '../src/panel/balloon/balloonpanelview'; -import View from '../src/view'; +import ContextualBalloon from '../../../src/panel/balloon/contextualballoon'; +import BalloonPanelView from '../../../src/panel/balloon/balloonpanelview'; +import View from '../../../src/view'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; From 16cf7595afa327d00f14b487cd7db72770b5e401 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 12 Apr 2017 13:00:25 +0200 Subject: [PATCH 6/6] Docs: Fixed broken link. --- src/panel/balloon/contextualballoon.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/panel/balloon/contextualballoon.js b/src/panel/balloon/contextualballoon.js index df72c155..5e50cbaf 100644 --- a/src/panel/balloon/contextualballoon.js +++ b/src/panel/balloon/contextualballoon.js @@ -42,7 +42,7 @@ export default class ContextualBalloon extends Plugin { /** * Stack of the views injected into the balloon. Last one in the stack is displayed - * as a content of {@link module:ui/contextualballoon~ContextualBalloon#view}. + * as a content of {@link module:ui/panel/balloon/contextualballoon~ContextualBalloon#view}. * * @private * @member {Map} #_stack