Fix: FocusCycler should ignore invisible views. Closes #209.

T/209: FocusCycler should not consider invisible views.

Author: oskarwrobel

src/button/buttonview.js

@@ -84,6 +84,14 @@ export default class ButtonView extends View {
 		 */
 		this.set( 'isEnabled', true );
 
+		/**
+		 * Controls whether the button view is visible.
+		 *
+		 * @observable
+		 * @member {Boolean} #isVisible
+		 */
+		this.set( 'isVisible', true );
+
 		/**
 		 * (Optional) Whether the label of the button is hidden (e.g. button with icon only).
 		 *
@@ -142,6 +150,7 @@ export default class ButtonView extends View {
 					'ck-button',
 					bind.if( '_tooltipString', 'ck-tooltip_s' ),
 					bind.to( 'isEnabled', value => value ? 'ck-enabled' : 'ck-disabled' ),
+					bind.if( 'isVisible', 'ck-hidden', value => !value ),
 					bind.to( 'isOn', value => value ? 'ck-on' : 'ck-off' ),
 					bind.if( 'withText', 'ck-button_with-text' )
 				],

src/focuscycler.js

@@ -7,6 +7,8 @@
  * @module ui/focuscycler
  */
 
+import global from '@ckeditor/ckeditor5-utils/src/dom/global';
+
 /**
  * Helps cycling over focusable views in a {@link module:ui/viewcollection~ViewCollection}
  * when the focus is tracked by {@link module:utils/focustracker~FocusTracker} instance.
@@ -271,5 +273,5 @@ export default class FocusCycler {
 // @param {module:ui/view~View} view A view to be checked.
 // @returns {Boolean}
 function isFocusable( view ) {
-	return view.focus;
+	return !!( view.focus && global.window.getComputedStyle( view.element ).display != 'none' );
 }

tests/button/buttonview.js

@@ -25,6 +25,7 @@ describe( 'ButtonView', () => {
 			it( 'is set initially', () => {
 				expect( view.element.classList ).to.have.length( 3 );
 				expect( view.element.classList.contains( 'ck-button' ) ).to.true;
+				expect( view.element.classList.contains( 'ck-enabled' ) ).to.true;
 				expect( view.element.classList.contains( 'ck-off' ) ).to.true;
 			} );
 
@@ -44,6 +45,14 @@ describe( 'ButtonView', () => {
 				expect( view.element.classList.contains( 'ck-on' ) ).to.false;
 			} );
 
+			it( 'reacts on view#isVisible', () => {
+				view.isVisible = true;
+				expect( view.element.classList.contains( 'ck-hidden' ) ).to.be.false;
+
+				view.isVisible = false;
+				expect( view.element.classList.contains( 'ck-hidden' ) ).to.be.true;
+			} );
+
 			it( 'reacts on view#withText', () => {
 				view.withText = true;
 				expect( view.element.classList.contains( 'ck-button_with-text' ) ).to.true;

tests/focuscycler.js

@@ -8,6 +8,10 @@ import View from '../src/view';
 import FocusCycler from '../src/focuscycler';
 import KeystrokeHandler from '@ckeditor/ckeditor5-utils/src/keystrokehandler';
 import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
+import global from '@ckeditor/ckeditor5-utils/src/dom/global';
+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
+
+testUtils.createSinonSandbox();
 
 describe( 'FocusCycler', () => {
 	let focusables, focusTracker, cycler;
@@ -22,6 +26,8 @@ describe( 'FocusCycler', () => {
 			focusTracker: focusTracker
 		} );
 
+		testUtils.sinon.stub( global.window, 'getComputedStyle' );
+
 		focusables.add( nonFocusable() );
 		focusables.add( focusable() );
 		focusables.add( focusable() );
@@ -196,11 +202,9 @@ describe( 'FocusCycler', () => {
 
 	describe( 'focusFirst()', () => {
 		it( 'focuses first focusable view', () => {
-			const spy = sinon.spy( focusables.get( 1 ), 'focus' );
-
 			cycler.focusFirst();
 
-			sinon.assert.calledOnce( spy );
+			sinon.assert.calledOnce( focusables.get( 1 ).focus );
 		} );
 
 		it( 'does not throw when no focusable items', () => {
@@ -223,15 +227,27 @@ describe( 'FocusCycler', () => {
 				cycler.focusFirst();
 			} ).to.not.throw();
 		} );
+
+		it( 'ignores invisible items', () => {
+			const item = focusable();
+
+			focusables = new ViewCollection();
+			focusables.add( nonFocusable() );
+			focusables.add( focusable( true ) );
+			focusables.add( item );
+
+			cycler = new FocusCycler( { focusables, focusTracker } );
+
+			cycler.focusFirst();
+			sinon.assert.calledOnce( item.focus );
+		} );
 	} );
 
 	describe( 'focusLast()', () => {
 		it( 'focuses last focusable view', () => {
-			const spy = sinon.spy( focusables.get( 3 ), 'focus' );
-
 			cycler.focusLast();
 
-			sinon.assert.calledOnce( spy );
+			sinon.assert.calledOnce( focusables.get( 3 ).focus );
 		} );
 
 		it( 'does not throw when no focusable items', () => {
@@ -258,12 +274,10 @@ describe( 'FocusCycler', () => {
 
 	describe( 'focusNext()', () => {
 		it( 'focuses next focusable view', () => {
-			const spy = sinon.spy( focusables.get( 3 ), 'focus' );
-
 			focusTracker.focusedElement = focusables.get( 2 ).element;
 			cycler.focusNext();
 
-			sinon.assert.calledOnce( spy );
+			sinon.assert.calledOnce( focusables.get( 3 ).focus );
 		} );
 
 		it( 'does not throw when no focusable items', () => {
@@ -290,12 +304,10 @@ describe( 'FocusCycler', () => {
 
 	describe( 'focusPrevious()', () => {
 		it( 'focuses previous focusable view', () => {
-			const spy = sinon.spy( focusables.get( 3 ), 'focus' );
-
 			focusTracker.focusedElement = focusables.get( 1 ).element;
 			cycler.focusPrevious();
 
-			sinon.assert.calledOnce( spy );
+			sinon.assert.calledOnce( focusables.get( 3 ).focus );
 		} );
 
 		it( 'does not throw when no focusable items', () => {
@@ -385,15 +397,23 @@ describe( 'FocusCycler', () => {
 	} );
 } );
 
-function focusable() {
+function nonFocusable( isHidden ) {
 	const view = new View();
+	view.element = Math.random();
 
-	view.focus = () => {};
-	view.element = {};
+	global.window.getComputedStyle
+		.withArgs( view.element )
+		.returns( {
+			display: isHidden ? 'none' : 'block'
+		} );
 
 	return view;
 }
 
-function nonFocusable() {
-	return new View();
+function focusable( isHidden ) {
+	const view = nonFocusable( isHidden );
+
+	view.focus = sinon.spy();
+
+	return view;
 }

theme/helpers/_states.scss

@@ -14,5 +14,7 @@
  * A class which hides an element in DOM.
  */
 .ck-hidden {
-	display: none;
+	// Override selector specificity. Otherwise, all elements with some display
+	// style defined will override this one, which is not a desired result.
+	display: none !important;
 }