fix: fix the structure of elements in menus to comply with ARIA requi…

…rements (#4034)

Fix the structure of elements in menus so that actionable elements are not children of actionable elements, as required by ARIA.
Remove unnecessary aria-labels on menus.

src/js/clickable-component.js

@@ -219,7 +219,7 @@ class ClickableComponent extends Component {
     // 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);
+      this.trigger('click');
     } else if (super.handleKeyPress) {
 
       // Pass keypress handling up for unsupported keys

src/js/control-bar/audio-track-controls/audio-track-button.js

@@ -25,8 +25,6 @@ class AudioTrackButton extends TrackButton {
     options.tracks = player.audioTracks();
 
     super(player, options);
-
-    this.el_.setAttribute('aria-label', 'Audio Menu');
   }
 
   /**

src/js/control-bar/control-bar.js

@@ -39,7 +39,8 @@ class ControlBar extends Component {
       className: 'vjs-control-bar',
       dir: 'ltr'
     }, {
-      // The control bar is a group, so it can contain menuitems
+      // The control bar is a group, but we don't aria-label it to avoid
+      //  over-announcing by JAWS
       role: 'group'
     });
   }

src/js/control-bar/text-track-controls/captions-button.js

@@ -26,7 +26,6 @@ class CaptionsButton extends TextTrackButton {
    */
   constructor(player, options, ready) {
     super(player, options, ready);
-    this.el_.setAttribute('aria-label', 'Captions Menu');
   }
 
   /**

src/js/control-bar/text-track-controls/chapters-button.js

@@ -29,7 +29,6 @@ class ChaptersButton extends TextTrackButton {
    */
   constructor(player, options, ready) {
     super(player, options, ready);
-    this.el_.setAttribute('aria-label', 'Chapters Menu');
   }
 
   /**

src/js/control-bar/text-track-controls/descriptions-button.js

@@ -26,7 +26,6 @@ class DescriptionsButton extends TextTrackButton {
    */
   constructor(player, options, ready) {
     super(player, options, ready);
-    this.el_.setAttribute('aria-label', 'Descriptions Menu');
 
     const tracks = player.textTracks();
     const changeHandler = Fn.bind(this, this.handleTracksChange);

src/js/control-bar/text-track-controls/subtitles-button.js

@@ -25,7 +25,6 @@ class SubtitlesButton extends TextTrackButton {
    */
   constructor(player, options, ready) {
     super(player, options, ready);
-    this.el_.setAttribute('aria-label', 'Subtitles Menu');
   }
 
   /**

src/js/menu/menu-button.js

@@ -1,19 +1,21 @@
 /**
  * @file menu-button.js
  */
-import ClickableComponent from '../clickable-component.js';
+import Button from '../button.js';
 import Component from '../component.js';
 import Menu from './menu.js';
 import * as Dom from '../utils/dom.js';
 import * as Fn from '../utils/fn.js';
+import * as Events from '../utils/events.js';
 import toTitleCase from '../utils/to-title-case.js';
+import document from 'global/document';
 
 /**
  * A `MenuButton` class for any popup {@link Menu}.
  *
- * @extends ClickableComponent
+ * @extends Component
  */
-class MenuButton extends ClickableComponent {
+class MenuButton extends Component {
 
   /**
    * Creates an instance of this class.
@@ -27,12 +29,27 @@ class MenuButton extends ClickableComponent {
   constructor(player, options = {}) {
     super(player, options);
 
+    this.menuButton_ = new Button(player, options);
+
+    this.menuButton_.controlText(this.controlText_);
+    this.menuButton_.el_.setAttribute('aria-haspopup', 'true');
+
+    // Add buildCSSClass values to the button, not the wrapper
+    const buttonClass = Button.prototype.buildCSSClass();
+
+    this.menuButton_.el_.className = this.buildCSSClass() + ' ' + buttonClass;
+
+    this.addChild(this.menuButton_);
+
     this.update();
 
     this.enabled_ = true;
 
-    this.el_.setAttribute('aria-haspopup', 'true');
-    this.el_.setAttribute('role', 'menuitem');
+    this.on(this.menuButton_, 'tap', this.handleClick);
+    this.on(this.menuButton_, 'click', this.handleClick);
+    this.on(this.menuButton_, 'focus', this.handleFocus);
+    this.on(this.menuButton_, 'blur', this.handleBlur);
+
     this.on('keydown', this.handleSubmenuKeyPress);
   }
 
@@ -56,7 +73,7 @@ class MenuButton extends ClickableComponent {
      * @private
      */
     this.buttonPressed_ = false;
-    this.el_.setAttribute('aria-expanded', 'false');
+    this.menuButton_.el_.setAttribute('aria-expanded', 'false');
 
     if (this.items && this.items.length === 0) {
       this.hide();
@@ -72,7 +89,7 @@ class MenuButton extends ClickableComponent {
    *         The constructed menu
    */
   createMenu() {
-    const menu = new Menu(this.player_);
+    const menu = new Menu(this.player_, { menuButton: this });
 
     // Add a title list item to the top
     if (this.options_.title) {
@@ -113,10 +130,33 @@ class MenuButton extends ClickableComponent {
    */
   createEl() {
     return super.createEl('div', {
-      className: this.buildCSSClass()
+      className: this.buildWrapperCSSClass()
+    }, {
     });
   }
 
+  /**
+   * Allow sub components to stack CSS class names for the wrapper element
+   *
+   * @return {string}
+   *         The constructed wrapper DOM `className`
+   */
+  buildWrapperCSSClass() {
+    let menuButtonClass = 'vjs-menu-button';
+
+    // If the inline option is passed, we want to use different styles altogether.
+    if (this.options_.inline === true) {
+      menuButtonClass += '-inline';
+    } else {
+      menuButtonClass += '-popup';
+    }
+
+    // TODO: Fix the CSS so that this isn't necessary
+    const buttonClass = Button.prototype.buildCSSClass();
+
+    return `vjs-menu-button ${menuButtonClass} ${buttonClass} ${super.buildCSSClass()}`;
+  }
+
   /**
    * Builds the default DOM `className`.
    *
@@ -163,6 +203,47 @@ class MenuButton extends ClickableComponent {
     }
   }
 
+  /**
+   * Set the focus to the actual button, not to this element
+   */
+  focus() {
+    this.menuButton_.focus();
+  }
+
+  /**
+   * Remove the focus from the actual button, not this element
+   */
+  blur() {
+    this.menuButton_.blur();
+  }
+
+  /**
+   * This gets called when a `MenuButton` gains focus via a `focus` event.
+   * Turns on listening for `keydown` events. When they happen it
+   * calls `this.handleKeyPress`.
+   *
+   * @param {EventTarget~Event} event
+   *        The `focus` event that caused this function to be called.
+   *
+   * @listens focus
+   */
+  handleFocus() {
+    Events.on(document, 'keydown', Fn.bind(this, this.handleKeyPress));
+  }
+
+  /**
+   * Called when a `MenuButton` loses focus. Turns off the listener for
+   * `keydown` events. Which Stops `this.handleKeyPress` from getting called.
+   *
+   * @param {EventTarget~Event} event
+   *        The `blur` event that caused this function to be called.
+   *
+   * @listens blur
+   */
+  handleBlur() {
+    Events.off(document, 'keydown', Fn.bind(this, this.handleKeyPress));
+  }
+
   /**
    * Handle tab, escape, down arrow, and up arrow keys for `MenuButton`. See
    * {@link ClickableComponent#handleKeyPress} for instances where this is called.
@@ -182,15 +263,15 @@ class MenuButton extends ClickableComponent {
       // Don't preventDefault for Tab key - we still want to lose focus
       if (event.which !== 9) {
         event.preventDefault();
+        // Set focus back to the menu button's button
+        this.menuButton_.el_.focus();
       }
     // Up (38) key or Down (40) key press the 'button'
     } else if (event.which === 38 || event.which === 40) {
       if (!this.buttonPressed_) {
         this.pressButton();
         event.preventDefault();
       }
-    } else {
-      super.handleKeyPress(event);
     }
   }
 
@@ -213,6 +294,8 @@ class MenuButton extends ClickableComponent {
       // Don't preventDefault for Tab key - we still want to lose focus
       if (event.which !== 9) {
         event.preventDefault();
+        // Set focus back to the menu button's button
+        this.menuButton_.el_.focus();
       }
     }
   }
@@ -224,7 +307,7 @@ class MenuButton extends ClickableComponent {
     if (this.enabled_) {
       this.buttonPressed_ = true;
       this.menu.lockShowing();
-      this.el_.setAttribute('aria-expanded', 'true');
+      this.menuButton_.el_.setAttribute('aria-expanded', 'true');
       // set the focus into the submenu
       this.menu.focus();
     }
@@ -237,32 +320,30 @@ class MenuButton extends ClickableComponent {
     if (this.enabled_) {
       this.buttonPressed_ = false;
       this.menu.unlockShowing();
-      this.el_.setAttribute('aria-expanded', 'false');
-      // Set focus back to this menu button
-      this.el_.focus();
+      this.menuButton_.el_.setAttribute('aria-expanded', 'false');
     }
   }
 
   /**
    * Disable the `MenuButton`. Don't allow it to be clicked.
    */
   disable() {
-    // Unpress, but don't force focus on this button
-    this.buttonPressed_ = false;
-    this.menu.unlockShowing();
-    this.el_.setAttribute('aria-expanded', 'false');
+    this.unpressButton();
 
     this.enabled_ = false;
+    this.addClass('vjs-disabled');
 
-    super.disable();
+    this.menuButton_.disable();
   }
 
   /**
    * Enable the `MenuButton`. Allow it to be clicked.
    */
   enable() {
     this.enabled_ = true;
-    super.enable();
+    this.removeClass('vjs-disabled');
+
+    this.menuButton_.enable();
   }
 }
 

src/js/menu/menu.js

@@ -27,6 +27,10 @@ class Menu extends Component {
   constructor(player, options) {
     super(player, options);
 
+    if (options) {
+      this.menuButton_ = options.menuButton;
+    }
+
     this.focusedChild_ = -1;
 
     this.on('keydown', this.handleKeyPress);
@@ -42,8 +46,11 @@ class Menu extends Component {
   addItem(component) {
     this.addChild(component);
     component.on('click', Fn.bind(this, function(event) {
-      this.unlockShowing();
-      // TODO: Need to set keyboard focus back to the menuButton
+      // Unpress the associated MenuButton, and move focus back to it
+      if (this.menuButton_) {
+        this.menuButton_.unpressButton();
+        this.menuButton_.focus();
+      }
     }));
   }
 
@@ -67,7 +74,6 @@ class Menu extends Component {
       className: 'vjs-menu'
     });
 
-    el.setAttribute('role', 'presentation');
     el.appendChild(this.contentEl_);
 
     // Prevent clicks from bubbling up. Needed for Menu Buttons,

test/unit/menu.test.js

@@ -45,7 +45,7 @@ QUnit.test('clicking should display the menu', function(assert) {
   const menuButton = new MenuButton(player, {
     title: 'testTitle'
   });
-  const el = menuButton.el();
+  const el = menuButton.menuButton_.el();
 
   assert.ok(menuButton.menu !== undefined, 'menu is created');