Merge pull request #284 from ckeditor/t/283

Fix: The `TooltipView` should hide when the `TooltipView#text` is empty. The `ButtonView's` ability to get a tooltip should not be restricted after `View` initialization. Closes #283.

Author: oleq

src/button/buttonview.js

@@ -147,17 +147,18 @@ export default class ButtonView extends View {
 		);
 
 		/**
-		 * Icon of the button view.
+		 * Tooltip of the button view.
 		 *
 		 * @readonly
-		 * @member {module:ui/icon/iconview~IconView} #iconView
+		 * @member {module:ui/tooltip/tooltipview~TooltipView} #tooltipView
 		 */
+		this.tooltipView = this._createTooltipView();
 
 		/**
-		 * Tooltip of the button view.
+		 * Icon of the button view.
 		 *
 		 * @readonly
-		 * @member {module:ui/tooltip/tooltipview~TooltipView} #tooltipView
+		 * @member {module:ui/icon/iconview~IconView} #iconView
 		 */
 
 		const bind = this.bindTemplate;
@@ -190,7 +191,8 @@ export default class ButtonView extends View {
 							text: bind.to( 'label' )
 						}
 					]
-				}
+				},
+				this.tooltipView
 			],
 
 			on: {
@@ -233,17 +235,6 @@ export default class ButtonView extends View {
 			this.addChildren( iconView );
 		}
 
-		if ( this.tooltip ) {
-			const tooltipView = this.tooltipView = new TooltipView();
-
-			tooltipView.bind( 'text' ).to( this, '_tooltipString' );
-			tooltipView.bind( 'position' ).to( this, 'tooltipPosition' );
-			this.element.appendChild( tooltipView.element );
-
-			// Make sure the tooltip will be destroyed along with the button.
-			this.addChildren( tooltipView );
-		}
-
 		super.init();
 	}
 
@@ -254,6 +245,21 @@ export default class ButtonView extends View {
 		this.element.focus();
 	}
 
+	/**
+	 * Creates TooltipView instance and bind with button properties.
+	 *
+	 * @private
+	 * @returns {module:ui/tooltip/tooltipview~TooltipView}
+	 */
+	_createTooltipView() {
+		const tooltipView = new TooltipView();
+
+		tooltipView.bind( 'text' ).to( this, '_tooltipString' );
+		tooltipView.bind( 'position' ).to( this, 'tooltipPosition' );
+
+		return tooltipView;
+	}
+
 	/**
 	 * Gets the text for the {@link #tooltipView} from the combination of
 	 * {@link #tooltip}, {@link #label} and {@link #keystroke} attributes.
@@ -277,12 +283,12 @@ export default class ButtonView extends View {
 
 				if ( tooltip instanceof Function ) {
 					return tooltip( label, keystroke );
-				} else if ( tooltip === true ) {
+				} else {
 					return `${ label }${ keystroke ? ` (${ keystroke })` : '' }`;
 				}
 			}
 		}
 
-		return false;
+		return '';
 	}
 }

src/tooltip/tooltipview.js

@@ -28,7 +28,7 @@ export default class TooltipView extends View {
 		 * @observable
 		 * @member {String} #text
 		 */
-		this.set( 'text' );
+		this.set( 'text', '' );
 
 		/**
 		 * The position of the tooltip (south or north).
@@ -58,7 +58,8 @@ export default class TooltipView extends View {
 			attributes: {
 				class: [
 					'ck-tooltip',
-					bind.to( 'position', position => 'ck-tooltip_' + position )
+					bind.to( 'position', position => 'ck-tooltip_' + position ),
+					bind.if( 'text', 'ck-hidden', value => !value.trim() )
 				]
 			},
 			children: [

tests/button/buttonview.js

@@ -80,56 +80,12 @@ describe( 'ButtonView', () => {
 				view = new ButtonView( locale );
 			} );
 
-			it( 'is not initially set', () => {
-				expect( view.tooltipView ).to.be.undefined;
-				expect( view.element.childNodes ).to.have.length( 1 );
+			it( 'is initially set', () => {
+				expect( view.tooltipView ).to.be.instanceof( TooltipView );
+				expect( Array.from( view.template.children ) ).to.include( view.tooltipView );
 			} );
 
-			it( 'is not initially set (despite #label and #keystroke)', () => {
-				view.label = 'foo';
-				view.keystroke = 'A';
-				view.init();
-
-				expect( view.tooltipView ).to.be.undefined;
-			} );
-
-			it( 'is not set if neither `true`, String or Function', () => {
-				view.label = 'foo';
-				view.keystroke = 'A';
-				view.tooltip = false;
-				view.init();
-
-				expect( view.tooltipView ).to.be.undefined;
-
-				view.tooltip = 3;
-				expect( view.tooltipView ).to.be.undefined;
-
-				view.tooltip = new Date();
-				expect( view.tooltipView ).to.be.undefined;
-			} );
-
-			it( 'when set, is added to the DOM', () => {
-				view.tooltip = 'foo';
-				view.icon = 'bar';
-				view.init();
-
-				expect( view.element.childNodes ).to.have.length( 3 );
-				expect( view.element.childNodes[ 2 ] ).to.equal( view.tooltipView.element );
-				expect( view.tooltipView ).to.instanceOf( TooltipView );
-				expect( view.tooltipView.position ).to.equal( 's' );
-			} );
-
-			it( 'when set, is destroyed along with the view', () => {
-				view.tooltip = 'foo';
-				view.init();
-
-				const spy = sinon.spy( view.tooltipView, 'destroy' );
-
-				view.destroy();
-				sinon.assert.calledOnce( spy );
-			} );
-
-			it( 'when set, reacts to #tooltipPosition attribute', () => {
+			it( 'it reacts to #tooltipPosition attribute', () => {
 				view.tooltip = 'foo';
 				view.icon = 'bar';
 				view.init();
@@ -151,6 +107,15 @@ describe( 'ButtonView', () => {
 					expect( view.tooltipView.text ).to.equal( 'bar (A)' );
 				} );
 
+				it( 'not render tooltip text when #tooltip value is false', () => {
+					view.tooltip = false;
+					view.label = 'bar';
+					view.keystroke = 'A';
+					view.init();
+
+					expect( view.tooltipView.text ).to.equal( '' );
+				} );
+
 				it( 'reacts to changes in #label and #keystroke', () => {
 					view.tooltip = true;
 					view.label = 'foo';
@@ -265,7 +230,7 @@ describe( 'ButtonView', () => {
 
 	describe( 'icon', () => {
 		it( 'is not initially set', () => {
-			expect( view.element.childNodes ).to.have.length( 1 );
+			expect( view.element.childNodes ).to.have.length( 2 );
 			expect( view.iconView ).to.be.undefined;
 		} );
 
@@ -274,7 +239,7 @@ describe( 'ButtonView', () => {
 			view.icon = 'foo';
 
 			view.init();
-			expect( view.element.childNodes ).to.have.length( 2 );
+			expect( view.element.childNodes ).to.have.length( 3 );
 			expect( view.element.childNodes[ 0 ] ).to.equal( view.iconView.element );
 
 			expect( view.iconView ).to.instanceOf( IconView );

tests/tooltip/tooltipview.js

@@ -44,6 +44,14 @@ describe( 'TooltipView', () => {
 		} );
 
 		describe( 'class', () => {
+			it( 'should react on view#text', () => {
+				expect( view.element.classList.contains( 'ck-hidden' ) ).to.be.false;
+
+				view.text = '';
+
+				expect( view.element.classList.contains( 'ck-hidden' ) ).to.be.true;
+			} );
+
 			it( 'should react on view#position', () => {
 				expect( view.element.classList.contains( 'ck-tooltip_n' ) ).to.be.false;
 				expect( view.element.classList.contains( 'ck-tooltip_s' ) ).to.be.true;