Fix issue where enter key wasn’t setting focus correctly

3 files changed, 50 insertions, 37 deletions

diff --git a/src/js/controls.js b/src/js/controls.js
index acc9d011..6265f2aa 100644
--- a/src/js/controls.js
+++ b/src/js/controls.js
@@ -381,7 +381,7 @@ const controls = {
     // We have to bind to keyup otherwise Firefox triggers a click when a keydown event handler shifts focus
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1220143
     bindMenuItemShortcuts(menuItem, type) {
-        // Handle space or -> to open menu
+        // Navigate through menus via arrow keys and space
         on(
             menuItem,
             'keydown keyup',
@@ -429,6 +429,16 @@ const controls = {
             },
             false,
         );
+
+        // Enter will fire a `click` event but we still need to manage focus
+        // So we bind to keyup which fires after and set focus here
+        on(menuItem, 'keyup', event => {
+            if (event.which !== 13) {
+                return;
+            }
+
+            controls.focusFirstMenuItem.call(this, null, true);
+        });
     },
 
     // Create a settings menu item
@@ -1074,6 +1084,23 @@ const controls = {
         toggleHidden(this.elements.settings.menu, !visible);
     },
 
+    // Focus the first menu item in a given (or visible) menu
+    focusFirstMenuItem(pane, tabFocus = false) {
+        if (this.elements.settings.popup.hidden) {
+            return;
+        }
+
+        let target = pane;
+
+        if (!is.element(target)) {
+            target = Object.values(this.elements.settings.panels).find(pane => !pane.hidden);
+        }
+
+        const firstItem = target.querySelector('[role^="menuitem"]');
+
+        setFocus.call(this, firstItem, tabFocus);
+    },
+
     // Show/hide menu
     toggleMenu(input) {
         const { popup } = this.elements.settings;
@@ -1085,7 +1112,7 @@ const controls = {
         }
 
         // True toggle by default
-        const hidden = popup.hasAttribute('hidden');
+        const { hidden } = popup;
         let show = hidden;
 
         if (is.boolean(input)) {
@@ -1094,19 +1121,13 @@ const controls = {
             show = false;
         } else if (is.event(input)) {
             const isMenuItem = popup.contains(input.target);
-            const isButton = input.target === button;
 
             // If the click was inside the menu or if the click
             // wasn't the button or menu item and we're trying to
             // show the menu (a doc click shouldn't show the menu)
-            if (isMenuItem || (!isMenuItem && !isButton && show)) {
+            if (isMenuItem || (!isMenuItem && input.target !== button && show)) {
                 return;
             }
-
-            // Prevent the toggle being caught by the doc listener
-            if (isButton) {
-                input.stopPropagation();
-            }
         }
 
         // Set button attributes
@@ -1120,9 +1141,7 @@ const controls = {
 
         // Focus the first item if key interaction
         if (show && is.keyboardEvent(input)) {
-            const pane = Object.values(this.elements.settings.panels).find(pane => !pane.hidden);
-            const firstItem = pane.querySelector('[role^="menuitem"]');
-            setFocus.call(this, firstItem, true);
+            controls.focusFirstMenuItem.call(this, null, true);
         }
         // If closing, re-focus the button
         else if (!show && !hidden) {
@@ -1205,8 +1224,7 @@ const controls = {
         toggleHidden(target, false);
 
         // Focus the first item
-        const firstItem = target.querySelector('[role^="menuitem"]');
-        setFocus.call(this, firstItem, tabFocus);
+        controls.focusFirstMenuItem.call(this, target, tabFocus);
     },
 
     // Build the default HTML
diff --git a/src/js/listeners.js b/src/js/listeners.js
index 8176e9a3..c2b772b2 100644
--- a/src/js/listeners.js
+++ b/src/js/listeners.js
@@ -553,6 +553,9 @@ class Listeners {
 
         // Settings menu - click toggle
         this.bind(elements.buttons.settings, 'click', event => {
+            // Prevent the document click listener closing the menu
+            event.stopPropagation();
+
             controls.toggleMenu.call(player, event);
         });
 
@@ -563,8 +566,16 @@ class Listeners {
             elements.buttons.settings,
             'keyup',
             event => {
+                const code = event.which;
+
                 // We only care about space and return
-                if (event.which !== 32 && event.which !== 13) {
+                if (![13, 32].includes(code)) {
+                    return;
+                }
+
+                // Because return triggers a click anyway, all we need to do is set focus
+                if (code === 13) {
+                    controls.focusFirstMenuItem.call(player, null, true);
                     return;
                 }
 
@@ -572,15 +583,13 @@ class Listeners {
                 event.preventDefault();
 
                 // Prevent playing video (Firefox)
-                if (event.which === 32) {
-                    event.stopPropagation();
-                }
+                event.stopPropagation();
 
                 // Toggle menu
                 controls.toggleMenu.call(player, event);
             },
             null,
-            false,
+            false, // Can't be passive as we're preventing default
         );
 
         // Escape closes menu
diff --git a/src/js/utils/elements.js b/src/js/utils/elements.js
index 3a3dfcfd..a6722da2 100644
--- a/src/js/utils/elements.js
+++ b/src/js/utils/elements.js
@@ -116,11 +116,7 @@ export function emptyElement(element) {
 
 // Replace element
 export function replaceElement(newChild, oldChild) {
-    if (
-        !is.element(oldChild) ||
-        !is.element(oldChild.parentNode) ||
-        !is.element(newChild)
-    ) {
+    if (!is.element(oldChild) || !is.element(oldChild.parentNode) || !is.element(newChild)) {
         return null;
     }
 
@@ -195,7 +191,7 @@ export function toggleHidden(element, hidden) {
     let hide = hidden;
 
     if (!is.boolean(hide)) {
-        hide = !element.hasAttribute('hidden');
+        hide = !element.hidden;
     }
 
     if (hide) {
@@ -263,10 +259,7 @@ export function trapFocus(element = null, toggle = false) {
         return;
     }
 
-    const focusable = getElements.call(
-        this,
-        'button:not(:disabled), input:not(:disabled), [tabindex]',
-    );
+    const focusable = getElements.call(this, 'button:not(:disabled), input:not(:disabled), [tabindex]');
     const first = focusable[0];
     const last = focusable[focusable.length - 1];
 
@@ -290,14 +283,7 @@ export function trapFocus(element = null, toggle = false) {
         }
     };
 
-    toggleListener.call(
-        this,
-        this.elements.container,
-        'keydown',
-        trap,
-        toggle,
-        false,
-    );
+    toggleListener.call(this, this.elements.container, 'keydown', trap, toggle, false);
 }
 
 // Set focus and tab focus class