From b0c3de55ad4ff7c9a9024d25ebea7c864123475e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 17 Jul 2017 11:14:13 +0200 Subject: [PATCH 1/8] Changed the way of preventing ContextualToolbar from being displayed. --- src/toolbar/contextual/contextualtoolbar.js | 59 +++++++++---------- tests/toolbar/contextual/contextualtoolbar.js | 10 ++-- 2 files changed, 31 insertions(+), 38 deletions(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index 40daf823..19cd343b 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -79,6 +79,9 @@ export default class ContextualToolbar extends Plugin { // Attach lifecycle actions. this._handleSelectionChange(); this._handleFocusChange(); + + // ContextualToolbar is displayed using event to make possible to prevent displaying it by stopping this event. + this.on( 'show', () => this._show(), { priority: 'low' } ); } /** @@ -146,42 +149,15 @@ export default class ContextualToolbar extends Plugin { /** * Shows the toolbar and attaches it to the selection. * - * Fires {@link #event:beforeShow} event just before displaying the panel. + * Fires {@link #event:show} event which can be stopped that prevents toolbar from being displayed. */ show() { - const editingView = this.editor.editing.view; - let isStopped = false; - // Do not add toolbar to the balloon stack twice. if ( this._balloon.hasView( this.toolbarView ) ) { return; } - // If `beforeShow` event is not stopped by any external code then panel will be displayed. - this.once( 'beforeShow', () => { - if ( isStopped ) { - return; - } - - // Update panel position when selection changes while balloon will be opened - // (by an external document changes). - this.listenTo( editingView, 'render', () => { - this._balloon.updatePosition( this._getBalloonPositionData() ); - } ); - - // Add panel to the common editor contextual balloon. - this._balloon.add( { - view: this.toolbarView, - position: this._getBalloonPositionData(), - balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container' - } ); - } ); - - // Fire this event to inform that `ContextualToolbar` is going to be shown. - // Helper function for preventing the panel from being displayed is passed along with the event. - this.fire( 'beforeShow', () => { - isStopped = true; - } ); + this.fire( 'show' ); } /** @@ -194,6 +170,26 @@ export default class ContextualToolbar extends Plugin { } } + /** + * Executes showing toolbar. + * When {@link #event:show} is not stopped then toolbar will be displayed. + * + * @private + */ + _show() { + // Update panel position when selection changes (by external document changes) while balloon is opened. + this.listenTo( this.editor.editing.view, 'render', () => { + this._balloon.updatePosition( this._getBalloonPositionData() ); + } ); + + // Add panel to the common editor contextual balloon. + this._balloon.add( { + view: this.toolbarView, + position: this._getBalloonPositionData(), + balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container' + } ); + } + /** * Returns positioning options for the {@link #_balloon}. They control the way balloon is attached * to the selection. @@ -236,10 +232,9 @@ export default class ContextualToolbar extends Plugin { /** * This event is fired just before the toolbar shows. * Using this event, an external code can prevent ContextualToolbar - * from being displayed by calling a `stop` function which is passed along with this event. + * from being displayed by stopping it. * - * @event beforeShow - * @param {Function} stop Calling this function prevents panel from being displayed. + * @event show */ /** diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 4957be6f..18b4f2d7 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -409,24 +409,22 @@ describe( 'ContextualToolbar', () => { } ); describe( 'beforeShow event', () => { - it( 'should fire `beforeShow` event just before panel shows', () => { + it( 'should fire `show` event just before panel shows', () => { const spy = sinon.spy(); - contextualToolbar.on( 'beforeShow', spy ); + contextualToolbar.on( 'show', spy ); setData( editor.document, 'b[a]r' ); contextualToolbar.show(); sinon.assert.calledOnce( spy ); } ); - it( 'should not show the panel when `beforeShow` event is stopped', () => { + it( 'should not show the panel when `show` event is stopped', () => { const balloonAddSpy = sandbox.spy( balloon, 'add' ); setData( editor.document, 'b[a]r' ); - contextualToolbar.on( 'beforeShow', ( evt, stop ) => { - stop(); - } ); + contextualToolbar.on( 'show', evt => evt.stop() ); contextualToolbar.show(); sinon.assert.notCalled( balloonAddSpy ); From fafbc10e93359ee53ec876c470c375e0257352ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 17 Jul 2017 11:27:55 +0200 Subject: [PATCH 2/8] Imporved ContextualToolbar manual test. --- .../manual/contextualtoolbar/contextualtoolbar.html | 3 ++- tests/manual/contextualtoolbar/contextualtoolbar.js | 12 ++++++++++++ tests/manual/contextualtoolbar/contextualtoolbar.md | 1 + 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/manual/contextualtoolbar/contextualtoolbar.html b/tests/manual/contextualtoolbar/contextualtoolbar.html index 05c39012..eaeccbf3 100644 --- a/tests/manual/contextualtoolbar/contextualtoolbar.html +++ b/tests/manual/contextualtoolbar/contextualtoolbar.html @@ -1,5 +1,6 @@
-

This is a first line of text.

+

ContextualToolbar won't show for the first block element.

+

This is a first line of text.

This is a second line of text.

This is the end of text.

diff --git a/tests/manual/contextualtoolbar/contextualtoolbar.js b/tests/manual/contextualtoolbar/contextualtoolbar.js index 64f47a83..c9b9a930 100644 --- a/tests/manual/contextualtoolbar/contextualtoolbar.js +++ b/tests/manual/contextualtoolbar/contextualtoolbar.js @@ -8,6 +8,7 @@ import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; import ArticlePresets from '@ckeditor/ckeditor5-presets/src/article'; import ContextualToolbar from '../../../src/toolbar/contextual/contextualtoolbar'; +import Range from '@ckeditor/ckeditor5-engine/src/model/range'; ClassicEditor.create( document.querySelector( '#editor' ), { plugins: [ ArticlePresets, ContextualToolbar ], @@ -16,6 +17,17 @@ ClassicEditor.create( document.querySelector( '#editor' ), { } ) .then( editor => { window.editor = editor; + + const contextualToolbar = editor.plugins.get( 'ContextualToolbar' ); + + contextualToolbar.on( 'show', evt => { + const selectionRange = editor.document.selection.getFirstRange(); + const blockRange = Range.createOn( editor.document.getRoot().getChild( 0 ) ); + + if ( selectionRange.containsRange( blockRange ) || selectionRange.isIntersecting( blockRange ) ) { + evt.stop(); + } + } ); } ) .catch( err => { console.error( err.stack ); diff --git a/tests/manual/contextualtoolbar/contextualtoolbar.md b/tests/manual/contextualtoolbar/contextualtoolbar.md index 04555303..9ef00aed 100644 --- a/tests/manual/contextualtoolbar/contextualtoolbar.md +++ b/tests/manual/contextualtoolbar/contextualtoolbar.md @@ -3,3 +3,4 @@ 1. Create a non–collapsed selection. 2. Create another non–collapsed selection but in another direction. 3. For each selection, a contextual toolbar should appear and the beginning/end of the selection, duplicating main editor toolbar. +4. Toolbar should not display when selection is placed in the first block element. From 49f27887f6ef879e3c12cfdfe6455aaecccefe51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 17 Jul 2017 11:34:03 +0200 Subject: [PATCH 3/8] Changed test name. --- tests/toolbar/contextual/contextualtoolbar.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 18b4f2d7..cb8e4260 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -408,7 +408,7 @@ describe( 'ContextualToolbar', () => { } ); } ); - describe( 'beforeShow event', () => { + describe( 'show event', () => { it( 'should fire `show` event just before panel shows', () => { const spy = sinon.spy(); From cc22698c64ecc7c724ebfa0d23f0e302107b3622 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Mon, 17 Jul 2017 11:41:42 +0200 Subject: [PATCH 4/8] Prevented ContextualToolbar from being showed when all comonents inside are disabled. --- src/toolbar/contextual/contextualtoolbar.js | 6 ++++++ tests/toolbar/contextual/contextualtoolbar.js | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index 19cd343b..af3cd57a 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -177,6 +177,12 @@ export default class ContextualToolbar extends Plugin { * @private */ _show() { + // Don not show ContextualToolbar when all components inside are disabled + // see https://github.com/ckeditor/ckeditor5-ui/issues/269. + if ( Array.from( this.toolbarView.items ).every( item => !item.isEnabled ) ) { + return; + } + // Update panel position when selection changes (by external document changes) while balloon is opened. this.listenTo( this.editor.editing.view, 'render', () => { this._balloon.updatePosition( this._getBalloonPositionData() ); diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index cb8e4260..74ec4f80 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -217,6 +217,16 @@ describe( 'ContextualToolbar', () => { sinon.assert.calledOnce( balloonAddSpy ); } ); + it( 'should not add #toolbarView to the #_balloon when all components inside #toolbarView are disabled', () => { + Array.from( contextualToolbar.toolbarView.items ).forEach( item => { + item.isEnabled = false; + } ); + setData( editor.document, 'b[a]r' ); + + contextualToolbar.show(); + sinon.assert.notCalled( balloonAddSpy ); + } ); + describe( 'on #_selectionChangeDebounced event', () => { let showSpy; From 80bb1bf8e9b63ee16f8f349b55a740575fb1d30d Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Mon, 17 Jul 2017 16:09:10 +0200 Subject: [PATCH 5/8] Docs: Enhanced ContextualToolbar documentation. --- src/toolbar/contextual/contextualtoolbar.js | 23 ++++++++++----------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index af3cd57a..a6878524 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -80,7 +80,8 @@ export default class ContextualToolbar extends Plugin { this._handleSelectionChange(); this._handleFocusChange(); - // ContextualToolbar is displayed using event to make possible to prevent displaying it by stopping this event. + // The appearance of the ContextualToolbar method is event–driven. + // It is possible to stop the #show event and thus prevent the toolbar from showing up. this.on( 'show', () => this._show(), { priority: 'low' } ); } @@ -98,7 +99,7 @@ export default class ContextualToolbar extends Plugin { } /** - * Handles editor focus change and hides panel if it's needed. + * Handles the editor focus change and hides the toolbar if it's needed. * * @private */ @@ -149,10 +150,10 @@ export default class ContextualToolbar extends Plugin { /** * Shows the toolbar and attaches it to the selection. * - * Fires {@link #event:show} event which can be stopped that prevents toolbar from being displayed. + * Fires {@link #event:show} event which can be stopped, which prevents toolbar from showing up. */ show() { - // Do not add toolbar to the balloon stack twice. + // Do not add the toolbar to the balloon stack twice. if ( this._balloon.hasView( this.toolbarView ) ) { return; } @@ -171,24 +172,24 @@ export default class ContextualToolbar extends Plugin { } /** - * Executes showing toolbar. - * When {@link #event:show} is not stopped then toolbar will be displayed. + * Shows of the toolbar if the {@link #event:show} has not been stopped. * * @private */ _show() { - // Don not show ContextualToolbar when all components inside are disabled + // Don not show the toolbar when all components inside are disabled // see https://github.com/ckeditor/ckeditor5-ui/issues/269. if ( Array.from( this.toolbarView.items ).every( item => !item.isEnabled ) ) { return; } - // Update panel position when selection changes (by external document changes) while balloon is opened. + // Update the toolbar position upon #render (e.g. external document changes) + // while it's visible. this.listenTo( this.editor.editing.view, 'render', () => { this._balloon.updatePosition( this._getBalloonPositionData() ); } ); - // Add panel to the common editor contextual balloon. + // Add the toolbar to the common editor contextual balloon. this._balloon.add( { view: this.toolbarView, position: this._getBalloonPositionData(), @@ -236,9 +237,7 @@ export default class ContextualToolbar extends Plugin { } /** - * This event is fired just before the toolbar shows. - * Using this event, an external code can prevent ContextualToolbar - * from being displayed by stopping it. + * This event is fired just before the toolbar shows up. Stopping this event will prevent this. * * @event show */ From d5ed0bd7efba9ae1fec2fdacd86837f47bb300bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 18 Jul 2017 08:52:12 +0200 Subject: [PATCH 6/8] Imporved checking when components inside ContextualToolbar are disabled. --- src/toolbar/contextual/contextualtoolbar.js | 2 +- tests/toolbar/contextual/contextualtoolbar.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index af3cd57a..629bbfca 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -179,7 +179,7 @@ export default class ContextualToolbar extends Plugin { _show() { // Don not show ContextualToolbar when all components inside are disabled // see https://github.com/ckeditor/ckeditor5-ui/issues/269. - if ( Array.from( this.toolbarView.items ).every( item => !item.isEnabled ) ) { + if ( Array.from( this.toolbarView.items ).every( item => item.isEnabled !== undefined && !item.isEnabled ) ) { return; } diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 74ec4f80..7c73329e 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -227,6 +227,19 @@ describe( 'ContextualToolbar', () => { sinon.assert.notCalled( balloonAddSpy ); } ); + it( 'should add #toolbarView to the #_balloon when at least one component inside does not have #isEnabled interface', () => { + Array.from( contextualToolbar.toolbarView.items ).forEach( item => { + item.isEnabled = false; + } ); + + delete contextualToolbar.toolbarView.items.get( 0 ).isEnabled; + + setData( editor.document, 'b[a]r' ); + + contextualToolbar.show(); + sinon.assert.calledOnce( balloonAddSpy ); + } ); + describe( 'on #_selectionChangeDebounced event', () => { let showSpy; From e1feb17e2c68e1ee5a08f9c5cec0c720422a1802 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 18 Jul 2017 15:17:20 +0200 Subject: [PATCH 7/8] Docs: Minor docs improvements. --- src/toolbar/contextual/contextualtoolbar.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index a1685d07..9faa64eb 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -150,7 +150,7 @@ export default class ContextualToolbar extends Plugin { /** * Shows the toolbar and attaches it to the selection. * - * Fires {@link #event:show} event which can be stopped, which prevents toolbar from showing up. + * Fires {@link #event:show} event which can be stopped to prevent the toolbar from showing up. */ show() { // Do not add the toolbar to the balloon stack twice. From 18e1c6b899973504b51602e3e2a352b11dc731e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Tue, 18 Jul 2017 15:56:55 +0200 Subject: [PATCH 8/8] Used decorator for ContextualToolbar#show method. --- src/toolbar/contextual/contextualtoolbar.js | 33 +++++++------------ .../contextualtoolbar/contextualtoolbar.js | 2 +- tests/toolbar/contextual/contextualtoolbar.js | 2 +- 3 files changed, 14 insertions(+), 23 deletions(-) diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js index 9faa64eb..8c666ad7 100644 --- a/src/toolbar/contextual/contextualtoolbar.js +++ b/src/toolbar/contextual/contextualtoolbar.js @@ -81,8 +81,8 @@ export default class ContextualToolbar extends Plugin { this._handleFocusChange(); // The appearance of the ContextualToolbar method is event–driven. - // It is possible to stop the #show event and thus prevent the toolbar from showing up. - this.on( 'show', () => this._show(), { priority: 'low' } ); + // It is possible to stop the #show event and this prevent the toolbar from showing up. + this.decorate( 'show' ); } /** @@ -158,25 +158,6 @@ export default class ContextualToolbar extends Plugin { return; } - this.fire( 'show' ); - } - - /** - * Hides the toolbar. - */ - hide() { - if ( this._balloon.hasView( this.toolbarView ) ) { - this.stopListening( this.editor.editing.view, 'render' ); - this._balloon.remove( this.toolbarView ); - } - } - - /** - * Shows of the toolbar if the {@link #event:show} has not been stopped. - * - * @private - */ - _show() { // Don not show the toolbar when all components inside are disabled // see https://github.com/ckeditor/ckeditor5-ui/issues/269. if ( Array.from( this.toolbarView.items ).every( item => item.isEnabled !== undefined && !item.isEnabled ) ) { @@ -197,6 +178,16 @@ export default class ContextualToolbar extends Plugin { } ); } + /** + * Hides the toolbar. + */ + hide() { + if ( this._balloon.hasView( this.toolbarView ) ) { + this.stopListening( this.editor.editing.view, 'render' ); + this._balloon.remove( this.toolbarView ); + } + } + /** * Returns positioning options for the {@link #_balloon}. They control the way balloon is attached * to the selection. diff --git a/tests/manual/contextualtoolbar/contextualtoolbar.js b/tests/manual/contextualtoolbar/contextualtoolbar.js index c9b9a930..73be2ee2 100644 --- a/tests/manual/contextualtoolbar/contextualtoolbar.js +++ b/tests/manual/contextualtoolbar/contextualtoolbar.js @@ -27,7 +27,7 @@ ClassicEditor.create( document.querySelector( '#editor' ), { if ( selectionRange.containsRange( blockRange ) || selectionRange.isIntersecting( blockRange ) ) { evt.stop(); } - } ); + }, { priority: 'high' } ); } ) .catch( err => { console.error( err.stack ); diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js index 7c73329e..54e3a493 100644 --- a/tests/toolbar/contextual/contextualtoolbar.js +++ b/tests/toolbar/contextual/contextualtoolbar.js @@ -447,7 +447,7 @@ describe( 'ContextualToolbar', () => { setData( editor.document, 'b[a]r' ); - contextualToolbar.on( 'show', evt => evt.stop() ); + contextualToolbar.on( 'show', evt => evt.stop(), { priority: 'high' } ); contextualToolbar.show(); sinon.assert.notCalled( balloonAddSpy );