From e4f643b28f141eb72f08574991ca76dd661987ac Mon Sep 17 00:00:00 2001 From: Owen Edwards Date: Fri, 22 Jan 2016 01:11:05 -0800 Subject: [PATCH 1/2] Separate button functionality into clickable-component and pure button component. --- src/js/button.js | 102 ++++--------- src/js/clickable-component.js | 174 +++++++++++++++++++++++ src/js/control-bar/volume-menu-button.js | 1 - src/js/menu/menu-button.js | 5 +- src/js/menu/menu-item.js | 4 +- src/js/poster-image.js | 4 +- test/unit/button.test.js | 3 +- test/unit/clickable-component.test.js | 19 +++ 8 files changed, 230 insertions(+), 82 deletions(-) create mode 100644 src/js/clickable-component.js create mode 100644 test/unit/clickable-component.test.js diff --git a/src/js/button.js b/src/js/button.js index 1651c86664..a0fb2cf40f 100644 --- a/src/js/button.js +++ b/src/js/button.js @@ -1,10 +1,11 @@ /** * @file button.js */ +import ClickableComponent from './clickable-component.js'; import Component from './component'; -import * as Dom from './utils/dom.js'; import * as Events from './utils/events.js'; import * as Fn from './utils/fn.js'; +import log from './utils/log.js'; import document from 'global/document'; import assign from 'object.assign'; @@ -13,122 +14,77 @@ import assign from 'object.assign'; * * @param {Object} player Main Player * @param {Object=} options Object of option names and values - * @extends Component + * @extends ClickableComponent * @class Button */ -class Button extends Component { +class Button extends ClickableComponent { constructor(player, options) { super(player, options); - - this.emitTapEvents(); - - this.on('tap', this.handleClick); - this.on('click', this.handleClick); - this.on('focus', this.handleFocus); - this.on('blur', this.handleBlur); } /** * Create the component's DOM element * * @param {String=} type Element's node type. e.g. 'div' - * @param {Object=} props An object of element attributes that should be set on the element Tag name + * @param {Object=} props An object of properties that should be set on the element + * @param {Object=} attributes An object of attributes that should be set on the element * @return {Element} * @method createEl */ createEl(tag='button', props={}, attributes={}) { props = assign({ - className: this.buildCSSClass(), - tabIndex: 0 + className: this.buildCSSClass() }, props); - // Add standard Aria info + if (tag !== 'button') { + log.warn(`Creating a Button with an HTML element of ${tag} is deprecated; use ClickableComponent instead.`); + } + + // Add attributes for button element attributes = assign({ - role: 'button', type: 'button', // Necessary since the default button type is "submit" 'aria-live': 'polite' // let the screen reader user know that the text of the button may change }, attributes); - let el = super.createEl(tag, props, attributes); + let el = Component.prototype.createEl.call(this, tag, props, attributes); - this.controlTextEl_ = Dom.createEl('span', { - className: 'vjs-control-text' - }); - - el.appendChild(this.controlTextEl_); - - this.controlText(this.controlText_); + this.createControlTextEl(el); return el; } /** - * Controls text - both request and localize + * Adds a child component inside this button * - * @param {String} text Text for button - * @return {String} - * @method controlText + * @param {String|Component} child The class name or instance of a child to add + * @param {Object=} options Options, including options to be passed to children of the child. + * @return {Component} The child component (created by this process if a string was used) + * @deprecated + * @method addChild */ - controlText(text) { - if (!text) return this.controlText_ || 'Need Text'; - - this.controlText_ = text; - this.controlTextEl_.innerHTML = this.localize(this.controlText_); + addChild(child, options={}) { + let className = this.constructor.name; + log.warn(`Adding an actionable (user controllable) child to a Button (${className}) is not supported; use a ClickableComponent instead.`); - return this; + // Avoid the error message generated by ClickableComponent's addChild method + return Component.prototype.addChild.call(this, child, options); } /** - * Allows sub components to stack CSS class names - * - * @return {String} - * @method buildCSSClass - */ - buildCSSClass() { - return `vjs-control vjs-button ${super.buildCSSClass()}`; - } - - /** - * Handle Click - Override with specific functionality for button - * - * @method handleClick - */ - handleClick() {} - - /** - * Handle Focus - Add keyboard functionality to element - * - * @method handleFocus - */ - handleFocus() { - Events.on(document, 'keydown', Fn.bind(this, this.handleKeyPress)); - } - - /** - * Handle KeyPress (document level) - Trigger click when keys are pressed + * Handle KeyPress (document level) - Extend with specific functionality for button * * @method handleKeyPress */ handleKeyPress(event) { - // Check for space bar (32) or enter (13) keys + // Ignore Space (32) or Enter (13) key operation, which is handled by the browser for a button. if (event.which === 32 || event.which === 13) { - event.preventDefault(); - this.handleClick(event); + } else { + super.handleKeyPress(event); // Pass keypress handling up for unsupported keys } } - /** - * Handle Blur - Remove keyboard triggers - * - * @method handleBlur - */ - handleBlur() { - Events.off(document, 'keydown', Fn.bind(this, this.handleKeyPress)); - } - } - Component.registerComponent('Button', Button); export default Button; diff --git a/src/js/clickable-component.js b/src/js/clickable-component.js new file mode 100644 index 0000000000..8e4d53e0a6 --- /dev/null +++ b/src/js/clickable-component.js @@ -0,0 +1,174 @@ +/** + * @file button.js + */ +import Component from './component'; +import * as Dom from './utils/dom.js'; +import * as Events from './utils/events.js'; +import * as Fn from './utils/fn.js'; +import log from './utils/log.js'; +import document from 'global/document'; +import assign from 'object.assign'; + +/** + * Clickable Component which is clickable or keyboard actionable, but is not a native HTML button + * + * @param {Object} player Main Player + * @param {Object=} options Object of option names and values + * @extends Component + * @class ClickableComponent + */ +class ClickableComponent extends Component { + + constructor(player, options) { + super(player, options); + + this.emitTapEvents(); + + this.on('tap', this.handleClick); + this.on('click', this.handleClick); + this.on('focus', this.handleFocus); + this.on('blur', this.handleBlur); + } + + /** + * Create the component's DOM element + * + * @param {String=} type Element's node type. e.g. 'div' + * @param {Object=} props An object of properties that should be set on the element + * @param {Object=} attributes An object of attributes that should be set on the element + * @return {Element} + * @method createEl + */ + createEl(tag='div', props={}, attributes={}) { + props = assign({ + className: this.buildCSSClass(), + tabIndex: 0 + }, props); + + if (tag === 'button') { + log.error(`Creating a ClickableComponent with an HTML element of ${tag} is not supported; use a Button instead.`); + } + + // Add ARIA attributes for clickable element which is not a native HTML button + attributes = assign({ + role: 'button', + 'aria-live': 'polite' // let the screen reader user know that the text of the element may change + }, attributes); + + let el = super.createEl(tag, props, attributes); + + this.createControlTextEl(el); + + return el; + } + + /** + * create control text + * + * @param {Element} el Parent element for the control text + * @return {Element} + * @method controlText + */ + createControlTextEl(el) { + this.controlTextEl_ = Dom.createEl('span', { + className: 'vjs-control-text' + }); + + if (el) { + el.appendChild(this.controlTextEl_); + } + + this.controlText(this.controlText_); + + return this.controlTextEl_; + } + + /** + * Controls text - both request and localize + * + * @param {String} text Text for element + * @return {String} + * @method controlText + */ + controlText(text) { + if (!text) return this.controlText_ || 'Need Text'; + + this.controlText_ = text; + this.controlTextEl_.innerHTML = this.localize(this.controlText_); + + return this; + } + + /** + * Allows sub components to stack CSS class names + * + * @return {String} + * @method buildCSSClass + */ + buildCSSClass() { + return `vjs-control vjs-button ${super.buildCSSClass()}`; + } + + /** + * Adds a child component inside this clickable-component + * + * @param {String|Component} child The class name or instance of a child to add + * @param {Object=} options Options, including options to be passed to children of the child. + * @return {Component} The child component (created by this process if a string was used) + * @method addChild + */ + addChild(child, options={}) { + // TODO: Fix adding an actionable child to a ClickableComponent; currently + // it will cause issues with assistive technology (e.g. screen readers) + // which support ARIA, since an element with role="button" cannot have + // actionable child elements. + + //let className = this.constructor.name; + //log.warn(`Adding a child to a ClickableComponent (${className}) can cause issues with assistive technology which supports ARIA, since an element with role="button" cannot have actionable child elements.`); + + return super.addChild(child, options); + } + + /** + * Handle Click - Override with specific functionality for component + * + * @method handleClick + */ + handleClick() {} + + /** + * Handle Focus - Add keyboard functionality to element + * + * @method handleFocus + */ + handleFocus() { + Events.on(document, 'keydown', Fn.bind(this, this.handleKeyPress)); + } + + /** + * Handle KeyPress (document level) - Trigger click when Space or Enter key is pressed + * + * @method handleKeyPress + */ + handleKeyPress(event) { + // Support Space (32) or Enter (13) key operation to fire a click event + if (event.which === 32 || event.which === 13) { + event.preventDefault(); + this.handleClick(event); + } else { + super.handleKeyPress(event); // Pass keypress handling up for unsupported keys + } + } + + /** + * Handle Blur - Remove keyboard triggers + * + * @method handleBlur + */ + handleBlur() { + Events.off(document, 'keydown', Fn.bind(this, this.handleKeyPress)); + } +} + +Component.registerComponent('ClickableComponent', ClickableComponent); +export default ClickableComponent; diff --git a/src/js/control-bar/volume-menu-button.js b/src/js/control-bar/volume-menu-button.js index 7791415111..d510b3738e 100644 --- a/src/js/control-bar/volume-menu-button.js +++ b/src/js/control-bar/volume-menu-button.js @@ -1,7 +1,6 @@ /** * @file volume-menu-button.js */ -import Button from '../button.js'; import * as Fn from '../utils/fn.js'; import Component from '../component.js'; import Menu from '../menu/menu.js'; diff --git a/src/js/menu/menu-button.js b/src/js/menu/menu-button.js index c13143169a..dc31af0e2d 100644 --- a/src/js/menu/menu-button.js +++ b/src/js/menu/menu-button.js @@ -1,7 +1,7 @@ /** * @file menu-button.js */ -import Button from '../button.js'; +import ClickableComponent from '../clickable-component.js'; import Component from '../component.js'; import Menu from './menu.js'; import * as Dom from '../utils/dom.js'; @@ -16,7 +16,7 @@ import toTitleCase from '../utils/to-title-case.js'; * @extends Button * @class MenuButton */ -class MenuButton extends Button { +class MenuButton extends ClickableComponent { constructor(player, options={}){ super(player, options); @@ -25,7 +25,6 @@ class MenuButton extends Button { this.on('keydown', this.handleKeyPress); this.el_.setAttribute('aria-haspopup', true); - this.el_.setAttribute('role', 'button'); } /** diff --git a/src/js/menu/menu-item.js b/src/js/menu/menu-item.js index 15c6e71a9d..aed29678b2 100644 --- a/src/js/menu/menu-item.js +++ b/src/js/menu/menu-item.js @@ -1,7 +1,7 @@ /** * @file menu-item.js */ -import Button from '../button.js'; +import ClickableComponent from '../clickable-component.js'; import Component from '../component.js'; import assign from 'object.assign'; @@ -13,7 +13,7 @@ import assign from 'object.assign'; * @extends Button * @class MenuItem */ -class MenuItem extends Button { +class MenuItem extends ClickableComponent { constructor(player, options) { super(player, options); diff --git a/src/js/poster-image.js b/src/js/poster-image.js index c0bccc44d8..704d9b1bf3 100644 --- a/src/js/poster-image.js +++ b/src/js/poster-image.js @@ -1,7 +1,7 @@ /** * @file poster-image.js */ -import Button from './button.js'; +import ClickableComponent from './clickable-component.js'; import Component from './component.js'; import * as Fn from './utils/fn.js'; import * as Dom from './utils/dom.js'; @@ -15,7 +15,7 @@ import * as browser from './utils/browser.js'; * @extends Button * @class PosterImage */ -class PosterImage extends Button { +class PosterImage extends ClickableComponent { constructor(player, options){ super(player, options); diff --git a/test/unit/button.test.js b/test/unit/button.test.js index 66515b8fbb..9070738606 100644 --- a/test/unit/button.test.js +++ b/test/unit/button.test.js @@ -4,7 +4,7 @@ import TestHelpers from './test-helpers.js'; q.module('Button'); test('should localize its text', function(){ - expect(1); + expect(2); var player, testButton, el; @@ -21,5 +21,6 @@ test('should localize its text', function(){ testButton.controlText_ = 'Play'; el = testButton.createEl(); + ok(el.nodeName.toLowerCase().match('button')); ok(el.innerHTML.match('Juego')); }); diff --git a/test/unit/clickable-component.test.js b/test/unit/clickable-component.test.js new file mode 100644 index 0000000000..eeba22a42e --- /dev/null +++ b/test/unit/clickable-component.test.js @@ -0,0 +1,19 @@ +import ClickableComponent from '../../src/js/clickable-component.js'; +import TestHelpers from './test-helpers.js'; + +q.module('ClickableComponent'); + +test('should create a div with role="button"', function(){ + expect(2); + + var player, testClickableComponent, el; + + player = TestHelpers.makePlayer({ + }); + + testClickableComponent = new ClickableComponent(player); + el = testClickableComponent.createEl(); + + ok(el.nodeName.toLowerCase().match('div')); + ok(el.getAttribute('role').toLowerCase().match('button')); +}); From 9768e02faab3f980064c67ba806f3ddd6b2b3c3c Mon Sep 17 00:00:00 2001 From: Owen Edwards Date: Mon, 25 Jan 2016 14:51:24 -0800 Subject: [PATCH 2/2] Couple of small changes based on gkatsev's feedback. --- src/js/clickable-component.js | 2 +- test/unit/clickable-component.test.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/js/clickable-component.js b/src/js/clickable-component.js index 8e4d53e0a6..cdb0012fda 100644 --- a/src/js/clickable-component.js +++ b/src/js/clickable-component.js @@ -155,7 +155,7 @@ class ClickableComponent extends Component { if (event.which === 32 || event.which === 13) { event.preventDefault(); this.handleClick(event); - } else { + } else if (super.handleKeyPress) { super.handleKeyPress(event); // Pass keypress handling up for unsupported keys } } diff --git a/test/unit/clickable-component.test.js b/test/unit/clickable-component.test.js index eeba22a42e..1498b43a3a 100644 --- a/test/unit/clickable-component.test.js +++ b/test/unit/clickable-component.test.js @@ -14,6 +14,6 @@ test('should create a div with role="button"', function(){ testClickableComponent = new ClickableComponent(player); el = testClickableComponent.createEl(); - ok(el.nodeName.toLowerCase().match('div')); - ok(el.getAttribute('role').toLowerCase().match('button')); + equal(el.nodeName.toLowerCase(), 'div', 'the name of the element is "div"'); + equal(el.getAttribute('role').toLowerCase(), 'button', 'the role of the element is "button"'); });