feat(clickable-component): Disable interaction with disabled clickabl…

…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.

src/js/button.js

@@ -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
    *

src/js/clickable-component.js

@@ -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();
   }
 
   /**
@@ -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);
@@ -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;
   }
 
@@ -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;
   }
 

test/unit/clickable-component.test.js

@@ -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');
 
@@ -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();
+});