Merge pull request #420 from ckeditor/t/418

Fix: The `BalloonToolbar` should hide when the editable is blurred. Closes #418.

Author: oleq

src/toolbar/balloon/balloontoolbar.js

@@ -11,6 +11,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import ContextualBalloon from '../../panel/balloon/contextualballoon';
 import ToolbarView from '../toolbarview';
 import BalloonPanelView from '../../panel/balloon/balloonpanelview.js';
+import FocusTracker from '@ckeditor/ckeditor5-utils/src/focustracker';
 import debounce from '@ckeditor/ckeditor5-utils/src/lib/lodash/debounce';
 import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect';
 import normalizeToolbarConfig from '../normalizetoolbarconfig';
@@ -40,31 +41,32 @@ export default class BalloonToolbar extends Plugin {
 	/**
 	 * @inheritDoc
 	 */
-	init() {
-		const editor = this.editor;
+	constructor( editor ) {
+		super( editor );
 
 		/**
 		 * The toolbar view displayed in the balloon.
 		 *
-		 * @member {module:ui/toolbar/toolbarview~ToolbarView}
+		 * @type {module:ui/toolbar/toolbarview~ToolbarView}
 		 */
-		this.toolbarView = new ToolbarView( editor.locale );
-
-		this.toolbarView.extendTemplate( {
-			attributes: {
-				class: [
-					'ck-toolbar_floating'
-				]
-			}
-		} );
+		this.toolbarView = this._createToolbarView();
 
-		this.toolbarView.render();
+		/**
+		 * Tracks the focus of the {@link module:ui/editableui/editableuiview~EditableUIView#editableElement}
+		 * and the {@link #toolbarView}. When both are blurred then the toolbar should hide.
+		 *
+		 * @readonly
+		 * @type {module:utils:focustracker~FocusTracker}
+		 */
+		this.focusTracker = new FocusTracker();
+		this.focusTracker.add( editor.ui.view.editableElement );
+		this.focusTracker.add( this.toolbarView.element );
 
 		/**
 		 * The contextual balloon plugin instance.
 		 *
 		 * @private
-		 * @member {module:ui/panel/balloon/contextualballoon~ContextualBalloon}
+		 * @type {module:ui/panel/balloon/contextualballoon~ContextualBalloon}
 		 */
 		this._balloon = editor.plugins.get( ContextualBalloon );
 
@@ -75,19 +77,52 @@ export default class BalloonToolbar extends Plugin {
 		 * trailing debounced invocation on destroy.
 		 *
 		 * @private
-		 * @member {Function}
+		 * @type {Function}
 		 */
 		this._fireSelectionChangeDebounced = debounce( () => this.fire( '_selectionChangeDebounced' ), 200 );
 
-		// Attach lifecycle actions.
-		this._handleSelectionChange();
-		this._handleFocusChange();
-
 		// The appearance of the BalloonToolbar method is event–driven.
 		// It is possible to stop the #show event and this prevent the toolbar from showing up.
 		this.decorate( 'show' );
 	}
 
+	/**
+	 * @inheritDoc
+	 */
+	init() {
+		const editor = this.editor;
+		const selection = editor.model.document.selection;
+
+		// Show/hide the toolbar on editable focus/blur.
+		this.listenTo( this.focusTracker, 'change:isFocused', ( evt, name, isFocused ) => {
+			const isToolbarVisible = this._balloon.visibleView === this.toolbarView;
+
+			if ( !isFocused && isToolbarVisible ) {
+				this.hide();
+			} else if ( isFocused ) {
+				this.show();
+			}
+		} );
+
+		// Hide the toolbar when the selection is changed by a direct change or has changed to collapsed.
+		this.listenTo( selection, 'change:range', ( evt, data ) => {
+			if ( data.directChange || selection.isCollapsed ) {
+				this.hide();
+			}
+
+			// Fire internal `_selectionChangeDebounced` event to use it for showing
+			// the toolbar after the selection stops changing.
+			this._fireSelectionChangeDebounced();
+		} );
+
+		// Show the toolbar when the selection stops changing.
+		this.listenTo( this, '_selectionChangeDebounced', () => {
+			if ( this.editor.editing.view.document.isFocused ) {
+				this.show();
+			}
+		} );
+	}
+
 	/**
 	 * Creates toolbar components based on given configuration.
 	 * This needs to be done when all plugins are ready.
@@ -102,52 +137,23 @@ export default class BalloonToolbar extends Plugin {
 	}
 
 	/**
-	 * Handles the editor focus change and hides the toolbar if it's needed.
+	 * Creates the toolbar view instance.
 	 *
 	 * @private
+	 * @returns {module:ui/toolbar/toolbarview~ToolbarView}
 	 */
-	_handleFocusChange() {
-		const editor = this.editor;
+	_createToolbarView() {
+		const toolbarView = new ToolbarView( this.editor.locale );
 
-		// Hide the panel View when editor loses focus but no the other way around.
-		this.listenTo( editor.ui.focusTracker, 'change:isFocused', ( evt, name, isFocused ) => {
-			if ( this._balloon.visibleView === this.toolbarView && !isFocused ) {
-				this.hide();
+		toolbarView.extendTemplate( {
+			attributes: {
+				class: [ 'ck-toolbar_floating' ]
 			}
 		} );
-	}
 
-	/**
-	 * Handles {@link module:engine/model/document~Document#selection} change and show or hide toolbar.
-	 *
-	 * Note that in this case it's better to listen to {@link module:engine/model/document~Document model document}
-	 * selection instead of {@link module:engine/view/document~Document view document} selection because the first one
-	 * doesn't fire `change` event after text style change (like bold or italic) and toolbar doesn't blink.
-	 *
-	 * @private
-	 */
-	_handleSelectionChange() {
-		const selection = this.editor.model.document.selection;
-		const viewDocument = this.editor.editing.view.document;
+		toolbarView.render();
 
-		this.listenTo( selection, 'change:range', ( evt, data ) => {
-			// When the selection is not changed by a collaboration and when is not collapsed.
-			if ( data.directChange || selection.isCollapsed ) {
-				// Hide the toolbar when the selection starts changing.
-				this.hide();
-			}
-
-			// Fire internal `_selectionChangeDebounced` when the selection stops changing.
-			this._fireSelectionChangeDebounced();
-		} );
-
-		// Hide the toolbar when the selection stops changing.
-		this.listenTo( this, '_selectionChangeDebounced', () => {
-			// This implementation assumes that only non–collapsed selections gets the contextual toolbar.
-			if ( viewDocument.isFocused && !viewDocument.selection.isCollapsed ) {
-				this.show();
-			}
-		} );
+		return toolbarView;
 	}
 
 	/**
@@ -156,11 +162,18 @@ export default class BalloonToolbar extends Plugin {
 	 * Fires {@link #event:show} event which can be stopped to prevent the toolbar from showing up.
 	 */
 	show() {
+		const editor = this.editor;
+
 		// Do not add the toolbar to the balloon stack twice.
 		if ( this._balloon.hasView( this.toolbarView ) ) {
 			return;
 		}
 
+		// Do not show the toolbar when the selection is collapsed.
+		if ( editor.model.document.selection.isCollapsed ) {
+			return;
+		}
+
 		// 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 ) ) {

tests/manual/tickets/418/1.html

@@ -0,0 +1,31 @@
+<div class="wrapper">
+	<div id="editor">
+		<h2>The three greatest things you learn from traveling</h2>
+		<p>
+			Like all the great things on earth traveling teaches us by example. Here are some of the most precious lessons
+			I’ve learned over the years of traveling.
+		</p>
+
+		<h3>Appreciation of diversity</h3>
+		<p>
+			Getting used to an entirely different culture can be challenging. While it’s also nice to learn about
+			cultures online or from books, nothing comes close to experiencing cultural diversity in person.
+			You learn to appreciate each and every single one of the differences while you become more culturally fluid.
+		</p>
+	</div>
+</div>
+
+<style>
+	#editor {
+		margin: 0 auto;
+		max-width: 800px;
+	}
+
+	.wrapper {
+		padding: 50px 20px;
+	}
+
+	.ck-block-toolbar-button {
+		transform: translateX( -10px );
+	}
+</style>

tests/manual/tickets/418/1.js

@@ -0,0 +1,45 @@
+/**
+ * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals window, document, console:false */
+
+import BalloonEditor from '@ckeditor/ckeditor5-editor-balloon/src/ballooneditor';
+import Essentials from '@ckeditor/ckeditor5-essentials/src/essentials';
+import List from '@ckeditor/ckeditor5-list/src/list';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import Heading from '@ckeditor/ckeditor5-heading/src/heading';
+import HeadingButtonsUI from '@ckeditor/ckeditor5-heading/src/headingbuttonsui';
+import ParagraphButtonUI from '@ckeditor/ckeditor5-paragraph/src/paragraphbuttonui';
+import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
+import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic';
+import Link from '@ckeditor/ckeditor5-link/src/link';
+
+import BlockToolbar from '../../../../src/toolbar/block/blocktoolbar';
+import BalloonToolbar from '../../../../src/toolbar/balloon/balloontoolbar';
+
+BalloonEditor
+	.create( document.querySelector( '#editor' ), {
+		plugins: [
+			Essentials,
+			List,
+			Paragraph,
+			Heading,
+			HeadingButtonsUI,
+			ParagraphButtonUI,
+			Bold,
+			Italic,
+			Link,
+			BlockToolbar,
+			BalloonToolbar
+		],
+		blockToolbar: [ 'paragraph', 'heading1', 'heading2', 'heading3', 'bulletedList', 'numberedList' ],
+		balloonToolbar: [ 'bold', 'italic', 'link' ]
+	} )
+	.then( editor => {
+		window.editor = editor;
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );

tests/manual/tickets/418/1.md

@@ -0,0 +1,5 @@
+## BlockToolbar and BalloonToolbar collision [#418](https://github.com/ckeditor/ckeditor5-ui/issues/418)
+
+- select some text, balloon toolbar should show up
+- click block toolbar button, balloon toolbar should hide
+- pres `Esc` to move focus back to editable, balloon toolbar should show up

tests/toolbar/balloon/balloontoolbar.js

@@ -8,6 +8,7 @@ import BalloonToolbar from '../../../src/toolbar/balloon/balloontoolbar';
 import ContextualBalloon from '../../../src/panel/balloon/contextualballoon';
 import BalloonPanelView from '../../../src/panel/balloon/balloonpanelview';
 import ToolbarView from '../../../src/toolbar/toolbarview';
+import FocusTracker from '@ckeditor/ckeditor5-utils/src/focustracker';
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold';
 import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic';
@@ -17,7 +18,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import { stringify as viewStringify } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 
-/* global document, setTimeout, window */
+/* global document, setTimeout, window, Event */
 
 describe( 'BalloonToolbar', () => {
 	let sandbox, editor, model, selection, editingView, balloonToolbar, balloon, editorElement;
@@ -143,6 +144,28 @@ describe( 'BalloonToolbar', () => {
 		} );
 	} );
 
+	describe( 'focusTracker', () => {
+		it( 'should be defined', () => {
+			expect( balloonToolbar.focusTracker ).to.instanceof( FocusTracker );
+		} );
+
+		it( 'it should track the focus of the #editableElement', () => {
+			expect( balloonToolbar.focusTracker.isFocused ).to.false;
+
+			editor.ui.view.editableElement.dispatchEvent( new Event( 'focus' ) );
+
+			expect( balloonToolbar.focusTracker.isFocused ).to.true;
+		} );
+
+		it( 'it should track the focus of the toolbarView#element', () => {
+			expect( balloonToolbar.focusTracker.isFocused ).to.false;
+
+			balloonToolbar.toolbarView.element.dispatchEvent( new Event( 'focus' ) );
+
+			expect( balloonToolbar.focusTracker.isFocused ).to.true;
+		} );
+	} );
+
 	describe( 'show()', () => {
 		let balloonAddSpy, backwardSelectionRect, forwardSelectionRect;
 
@@ -275,7 +298,7 @@ describe( 'BalloonToolbar', () => {
 			expect( targetRect ).to.deep.equal( backwardSelectionRect );
 		} );
 
-		it( 'should update balloon position on ui#update event while balloon is added to the #_balloon', () => {
+		it( 'should update balloon position on ui#update event when #toolbarView is already added to the #_balloon', () => {
 			setData( model, '<paragraph>b[a]r</paragraph>' );
 
 			const spy = sandbox.spy( balloon, 'updatePosition' );
@@ -297,6 +320,13 @@ describe( 'BalloonToolbar', () => {
 			sinon.assert.calledOnce( balloonAddSpy );
 		} );
 
+		it( 'should not add the #toolbarView to the #_balloon when the selection is collapsed', () => {
+			setData( model, '<paragraph>b[]ar</paragraph>' );
+
+			balloonToolbar.show();
+			sinon.assert.notCalled( balloonAddSpy );
+		} );
+
 		it( 'should not add #toolbarView to the #_balloon when all components inside #toolbarView are disabled', () => {
 			Array.from( balloonToolbar.toolbarView.items ).forEach( item => {
 				item.isEnabled = false;
@@ -319,37 +349,6 @@ describe( 'BalloonToolbar', () => {
 			balloonToolbar.show();
 			sinon.assert.calledOnce( balloonAddSpy );
 		} );
-
-		describe( 'on #_selectionChangeDebounced event', () => {
-			let showSpy;
-
-			beforeEach( () => {
-				showSpy = sandbox.spy( balloonToolbar, 'show' );
-			} );
-
-			it( 'should not be called when the editor is not focused', () => {
-				setData( model, '<paragraph>b[a]r</paragraph>' );
-				editingView.document.isFocused = false;
-
-				balloonToolbar.fire( '_selectionChangeDebounced' );
-				sinon.assert.notCalled( showSpy );
-			} );
-
-			it( 'should not be called when the selection is collapsed', () => {
-				setData( model, '<paragraph>b[]ar</paragraph>' );
-
-				balloonToolbar.fire( '_selectionChangeDebounced' );
-				sinon.assert.notCalled( showSpy );
-			} );
-
-			it( 'should be called when the selection is not collapsed and editor is focused', () => {
-				setData( model, '<paragraph>b[a]r</paragraph>' );
-				editingView.document.isFocused = true;
-
-				balloonToolbar.fire( '_selectionChangeDebounced' );
-				sinon.assert.calledOnce( showSpy );
-			} );
-		} );
 	} );
 
 	describe( 'hide()', () => {
@@ -381,7 +380,7 @@ describe( 'BalloonToolbar', () => {
 			sinon.assert.notCalled( spy );
 		} );
 
-		it( 'should not remove #ttolbarView when is not added to the #_balloon', () => {
+		it( 'should not remove #toolbarView when is not added to the #_balloon', () => {
 			balloonToolbar.hide();
 
 			sinon.assert.notCalled( removeBalloonSpy );
@@ -412,7 +411,7 @@ describe( 'BalloonToolbar', () => {
 		} );
 	} );
 
-	describe( 'showing and hiding', () => {
+	describe( 'show and hide triggers', () => {
 		let showPanelSpy, hidePanelSpy;
 
 		beforeEach( () => {
@@ -422,7 +421,7 @@ describe( 'BalloonToolbar', () => {
 			hidePanelSpy = sandbox.spy( balloonToolbar, 'hide' );
 		} );
 
-		it( 'should open when selection stops changing', () => {
+		it( 'should show when selection stops changing', () => {
 			sinon.assert.notCalled( showPanelSpy );
 			sinon.assert.notCalled( hidePanelSpy );
 
@@ -432,7 +431,18 @@ describe( 'BalloonToolbar', () => {
 			sinon.assert.notCalled( hidePanelSpy );
 		} );
 
-		it( 'should close when selection starts changing by a directChange', () => {
+		it( 'should not show when the selection stops changing when the editable is blurred', () => {
+			sinon.assert.notCalled( showPanelSpy );
+			sinon.assert.notCalled( hidePanelSpy );
+
+			editingView.document.isFocused = false;
+			balloonToolbar.fire( '_selectionChangeDebounced' );
+
+			sinon.assert.notCalled( showPanelSpy );
+			sinon.assert.notCalled( hidePanelSpy );
+		} );
+
+		it( 'should hide when selection starts changing by a direct change', () => {
 			balloonToolbar.fire( '_selectionChangeDebounced' );
 
 			sinon.assert.calledOnce( showPanelSpy );
@@ -444,7 +454,7 @@ describe( 'BalloonToolbar', () => {
 			sinon.assert.calledOnce( hidePanelSpy );
 		} );
 
-		it( 'should not close when selection starts changing by not a directChange', () => {
+		it( 'should not hide when selection starts changing by an indirect change', () => {
 			balloonToolbar.fire( '_selectionChangeDebounced' );
 
 			sinon.assert.calledOnce( showPanelSpy );
@@ -456,7 +466,7 @@ describe( 'BalloonToolbar', () => {
 			sinon.assert.notCalled( hidePanelSpy );
 		} );
 
-		it( 'should close when selection starts changing by not a directChange but will become collapsed', () => {
+		it( 'should hide when selection starts changing by an indirect change but has changed to collapsed', () => {
 			balloonToolbar.fire( '_selectionChangeDebounced' );
 
 			sinon.assert.calledOnce( showPanelSpy );
@@ -472,35 +482,43 @@ describe( 'BalloonToolbar', () => {
 			sinon.assert.calledOnce( hidePanelSpy );
 		} );
 
-		it( 'should hide if the editor loses focus', () => {
-			editor.ui.focusTracker.isFocused = true;
+		it( 'should show on #focusTracker focus', () => {
+			balloonToolbar.focusTracker.isFocused = false;
 
-			balloonToolbar.fire( '_selectionChangeDebounced' );
+			sinon.assert.notCalled( showPanelSpy );
+			sinon.assert.notCalled( hidePanelSpy );
+
+			balloonToolbar.focusTracker.isFocused = true;
+
+			sinon.assert.calledOnce( showPanelSpy );
+			sinon.assert.notCalled( hidePanelSpy );
+		} );
+
+		it( 'should hide on #focusTracker blur', () => {
+			balloonToolbar.focusTracker.isFocused = true;
 
 			const stub = sandbox.stub( balloon, 'visibleView' ).get( () => balloonToolbar.toolbarView );
 
 			sinon.assert.calledOnce( showPanelSpy );
 			sinon.assert.notCalled( hidePanelSpy );
 
-			editor.ui.focusTracker.isFocused = false;
+			balloonToolbar.focusTracker.isFocused = false;
 
 			sinon.assert.calledOnce( showPanelSpy );
 			sinon.assert.calledOnce( hidePanelSpy );
 
 			stub.restore();
 		} );
 
-		it( 'should not hide if the editor loses focus and #toolbarView is not visible', () => {
-			editor.ui.focusTracker.isFocused = true;
-
-			balloonToolbar.fire( '_selectionChangeDebounced' );
+		it( 'should not hide on #focusTracker blur when toolbar is not in the balloon stack', () => {
+			balloonToolbar.focusTracker.isFocused = true;
 
 			const stub = sandbox.stub( balloon, 'visibleView' ).get( () => null );
 
 			sinon.assert.calledOnce( showPanelSpy );
 			sinon.assert.notCalled( hidePanelSpy );
 
-			editor.ui.focusTracker.isFocused = false;
+			balloonToolbar.focusTracker.isFocused = false;
 
 			sinon.assert.calledOnce( showPanelSpy );
 			sinon.assert.notCalled( hidePanelSpy );