Skip to content

Commit

Permalink
feat(clickable-component): Disable interaction with disabled clickabl…
Browse files Browse the repository at this point in the history
…e components (#3525)

enable() and disable() on clickable components is only cosmetic. "Disabled" implies the control should not be functional.

* Remove event listeners on disable() and add back on enable().
* Move adding listeners from constructor to enable
* Remove tabindex from disabled components and add disabled attribute to disabled buttons to prevent keyboard access.
  • Loading branch information
mister-ben authored and gkatsev committed Nov 3, 2016
1 parent 9d77268 commit de1b363
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 4 deletions.
22 changes: 22 additions & 0 deletions src/js/button.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,28 @@ class Button extends ClickableComponent {
return Component.prototype.addChild.call(this, child, options);
}

/**
* Enable the button element
*
* @return {Component}
* @method enable
*/
enable() {
super.enable();
this.el_.removeAttribute('disabled');
}

/**
* Disable the button element
*
* @return {Component}
* @method disable
*/
disable() {
super.disable();
this.el_.setAttribute('disabled', 'disabled');
}

/**
* Handle KeyPress (document level) - Extend with specific functionality for button
*
Expand Down
21 changes: 17 additions & 4 deletions src/js/clickable-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@ class ClickableComponent extends Component {

this.emitTapEvents();

this.on('tap', this.handleClick);
this.on('click', this.handleClick);
this.on('focus', this.handleFocus);
this.on('blur', this.handleBlur);
this.enable();
}

/**
Expand Down Expand Up @@ -57,6 +54,8 @@ class ClickableComponent extends Component {
'aria-live': 'polite'
}, attributes);

this.tabIndex_ = props.tabIndex;

const el = super.createEl(tag, props, attributes);

this.createControlTextEl(el);
Expand Down Expand Up @@ -146,6 +145,13 @@ class ClickableComponent extends Component {
enable() {
this.removeClass('vjs-disabled');
this.el_.setAttribute('aria-disabled', 'false');
if (typeof this.tabIndex_ !== 'undefined') {
this.el_.setAttribute('tabIndex', this.tabIndex_);
}
this.on('tap', this.handleClick);
this.on('click', this.handleClick);
this.on('focus', this.handleFocus);
this.on('blur', this.handleBlur);
return this;
}

Expand All @@ -158,6 +164,13 @@ class ClickableComponent extends Component {
disable() {
this.addClass('vjs-disabled');
this.el_.setAttribute('aria-disabled', 'true');
if (typeof this.tabIndex_ !== 'undefined') {
this.el_.removeAttribute('tabIndex');
}
this.off('tap', this.handleClick);
this.off('click', this.handleClick);
this.off('focus', this.handleFocus);
this.off('blur', this.handleBlur);
return this;
}

Expand Down
32 changes: 32 additions & 0 deletions test/unit/clickable-component.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-env qunit */
import ClickableComponent from '../../src/js/clickable-component.js';
import TestHelpers from './test-helpers.js';
import * as Events from '../../src/js/utils/events.js';

QUnit.module('ClickableComponent');

Expand Down Expand Up @@ -39,3 +40,34 @@ QUnit.test('should be enabled/disabled', function(assert) {
testClickableComponent.dispose();
player.dispose();
});

QUnit.test('handleClick should not be triggered when disabled', function() {
let clicks = 0;

class TestClickableComponent extends ClickableComponent {
handleClick() {
clicks++;
}
}

const player = TestHelpers.makePlayer({});
const testClickableComponent = new TestClickableComponent(player);
const el = testClickableComponent.el();

// 1st click
Events.trigger(el, 'click');
QUnit.equal(clicks, 1, 'click on enabled ClickableComponent is handled');

testClickableComponent.disable();
// No click should happen.
Events.trigger(el, 'click');
QUnit.equal(clicks, 1, 'click on disabled ClickableComponent is not handled');

testClickableComponent.enable();
// 2nd Click
Events.trigger(el, 'click');
QUnit.equal(clicks, 2, 'click on re-enabled ClickableComponent is handled');

testClickableComponent.dispose();
player.dispose();
});

0 comments on commit de1b363

Please sign in to comment.