Fix dialogs rendered inside menu items

primer · Dec 14, 2023 · 1e17b70 · 1e17b70
1 parent da60c73
commit 1e17b70
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 25 deletions.
diff --git a/app/components/primer/alpha/action_menu/action_menu_element.ts b/app/components/primer/alpha/action_menu/action_menu_element.ts
@@ -32,6 +32,7 @@ export class ActionMenuElement extends HTMLElement {
   #originalLabel = ''
   #inputName = ''
   #invokerBeingClicked = false
+  #dialogOpen = false
 
   get selectVariant(): SelectVariant {
     return this.getAttribute('data-select-variant') as SelectVariant
@@ -116,6 +117,10 @@ export class ActionMenuElement extends HTMLElement {
         signal,
       })
     }
+
+    if (this.popoverElement) {
+      this.popoverElement.addEventListener('toggle', this, {signal})
+    }
   }
 
   disconnectedCallback() {
@@ -179,6 +184,18 @@ export class ActionMenuElement extends HTMLElement {
   }
 
   handleEvent(event: Event) {
+    if (event.type === 'toggle') {
+      // If the menu's popover is being closed, prevent it from actually closing so as to
+      // allow any dialogs contained within the menu to appear as expected. The menu should
+      // still appear to have closed, which is handled by #handleDialogItemActivated.
+      if ((event as ToggleEvent).newState === 'closed' && this.#dialogOpen) {
+        if (this.popoverElement) {
+          requestAnimationFrame(() => this.popoverElement?.showPopover())
+        }
+        return
+      }
+    }
+
     const targetIsInvoker = this.invokerElement?.contains(event.target as HTMLElement)
     const eventIsActivation = this.#isActivation(event)
 
@@ -225,7 +242,13 @@ export class ActionMenuElement extends HTMLElement {
         const dialog = this.ownerDocument.getElementById(dialogInvoker.getAttribute('data-show-dialog-id') || '')
 
         if (dialog && this.contains(dialogInvoker) && this.contains(dialog)) {
-          this.#handleDialogItemActivated(event, dialog)
+          this.#handleDialogItemActivated(event, dialog as HTMLDialogElement)
+
+          const open = this.#isOpen()
+          this.handleEvent(
+            new ToggleEvent('toggle', {newState: open ? 'closed' : 'open', oldState: open ? 'open' : 'closed'}),
+          )
+
           return
         }
       }
@@ -265,25 +288,21 @@ export class ActionMenuElement extends HTMLElement {
     }
   }
 
-  #handleDialogItemActivated(event: Event, dialog: HTMLElement) {
+  #handleDialogItemActivated(event: Event, dialog: HTMLDialogElement) {
     this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = 'none'
+    this.#dialogOpen = true
     const dialog_controller = new AbortController()
     const {signal} = dialog_controller
     const handleDialogClose = () => {
       dialog_controller.abort()
+      this.#dialogOpen = false
       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)
-      }
+      dialog.close()
+
+      // Actually close the menu now, which will have appeared to re-open
+      // since the dialog is now closed (see handleEvent).
+      this.#hide()
     }
-    // a modal <dialog> element will close all popovers
-    setTimeout(() => this.#show(), 0)
     dialog.addEventListener('close', handleDialogClose, {signal})
     dialog.addEventListener('cancel', handleDialogClose, {signal})
   }

diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts
@@ -37,27 +37,35 @@ export class DialogHelperElement extends HTMLElement {
     const {signal} = (this.#abortController = new AbortController())
     document.addEventListener('click', dialogInvokerButtonHandler)
     document.addEventListener('click', this, {signal})
+    document.addEventListener('keyup', this, {signal})
   }
 
   disconnectedCallback() {
     this.#abortController?.abort()
   }
 
-  handleEvent(event: MouseEvent) {
-    const target = event.target as HTMLElement
+  handleEvent(event: Event) {
     const dialog = this.dialog
-    if (!dialog?.open) return
-    if (target?.closest('dialog') !== dialog) return
 
-    const rect = dialog.getBoundingClientRect()
-    const clickWasInsideDialog =
-      rect.top <= event.clientY &&
-      event.clientY <= rect.top + rect.height &&
-      rect.left <= event.clientX &&
-      event.clientX <= rect.left + rect.width
+    if (event.type === 'keyup') {
+      if ((event as KeyboardEvent).key.toLowerCase() === 'escape') {
+        dialog?.dispatchEvent(new Event('cancel'))
+      }
+    } else if (event instanceof MouseEvent) {
+      const target = event.target as HTMLElement
+      if (!dialog?.open) return
+      if (target?.closest('dialog') !== dialog) return
 
-    if (!clickWasInsideDialog) {
-      dialog.close()
+      const rect = dialog.getBoundingClientRect()
+      const clickWasInsideDialog =
+        rect.top <= event.clientY &&
+        event.clientY <= rect.top + rect.height &&
+        rect.left <= event.clientX &&
+        event.clientX <= rect.left + rect.width
+
+      if (!clickWasInsideDialog) {
+        dialog.close()
+      }
     }
   }
 }