actionmenu: ensure focus falls back to invoking element when dialogs …

…are closed (#2983)
primer · Aug 20, 2024 · 5d68193 · 5d68193
1 parent 8f06f7a
commit 5d68193
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 7 deletions.
diff --git a/.changeset/brave-bugs-tap.md b/.changeset/brave-bugs-tap.md
@@ -0,0 +1,5 @@
+---
+'@primer/view-components': patch
+---
+
+Ensure ActionMenu restores focus on close of a dialog, if a menu item opened that dialog
diff --git a/app/components/primer/alpha/action_menu/action_menu_element.ts b/app/components/primer/alpha/action_menu/action_menu_element.ts
@@ -207,7 +207,7 @@ export class ActionMenuElement extends HTMLElement {
       if (dialogInvoker) {
         const dialog = this.ownerDocument.getElementById(dialogInvoker.getAttribute('data-show-dialog-id') || '')
 
-        if (dialog && this.contains(dialogInvoker) && this.contains(dialog)) {
+        if (dialog && this.contains(dialogInvoker)) {
           this.#handleDialogItemActivated(event, dialog)
           return
         }
@@ -243,20 +243,33 @@ export class ActionMenuElement extends HTMLElement {
   }
 
   #handleDialogItemActivated(event: Event, dialog: HTMLElement) {
-    this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = 'none'
+    if (this.contains(dialog)) {
+      this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = 'none'
+    }
     const dialog_controller = new AbortController()
     const {signal} = dialog_controller
     const handleDialogClose = () => {
       dialog_controller.abort()
-      this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = ''
-      if (this.#isOpen()) {
-        this.#hide()
+      if (this.contains(dialog)) {
+        this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = ''
+        if (this.#isOpen()) {
+          this.#hide()
+        }
       }
       const activeElement = this.ownerDocument.activeElement
       const lostFocus = this.ownerDocument.activeElement === this.ownerDocument.body
       const focusInClosedMenu = this.contains(activeElement)
-      if (lostFocus || focusInClosedMenu) {
-        setTimeout(() => this.invokerElement?.focus(), 0)
+      const focusInDialog = dialog.contains(activeElement)
+      if (lostFocus || focusInClosedMenu || focusInDialog) {
+        setTimeout(() => {
+          // if the activeElement has changed after a task, then it's likely
+          // that other JS has tried to shift focus. We should respect that
+          // focus shift as long as it's not back at the document.
+          const newActiveElement = this.ownerDocument.activeElement
+          if (newActiveElement === activeElement || newActiveElement === this.ownerDocument.body) {
+            this.invokerElement?.focus()
+          }
+        }, 0)
       }
     }
     // a modal <dialog> element will close all popovers

diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb
@@ -56,6 +56,10 @@ def click_on_fourth_item
       click_on_item(4)
     end
 
+    def close_dialog
+      find("[data-close-dialog-id][aria-label=Close]").click
+    end
+
     def focus_on_invoker_button
       page.evaluate_script(<<~JS)
         document.querySelector('action-menu button[aria-controls]').focus()
@@ -340,6 +344,19 @@ def test_opens_dialog
       refute_selector "action-menu ul li"
     end
 
+    def test_open_then_closing_dialog_restores_focus
+      visit_preview(:opens_dialog)
+
+      click_on_invoker_button
+      click_on_second_item
+
+      assert_selector "dialog[open]"
+
+      close_dialog
+
+      assert_equal page.evaluate_script("document.activeElement").text, "Menu"
+    end
+
     def test_opens_dialog_on_keydown
       visit_preview(:opens_dialog)