Fix layering issues with dialog popover hack (#2556)

.changeset/lucky-apricots-move.md

@@ -0,0 +1,5 @@
+---
+"@primer/view-components": patch
+---
+
+Fix issue with layering of nested overlays/dialogs

app/components/primer/alpha/action_menu/action_menu_element.ts

@@ -287,7 +287,6 @@ export class ActionMenuElement extends HTMLElement {
       }
     }
     // a modal <dialog> element will close all popovers
-    setTimeout(() => this.#show(), 0)
     dialog.addEventListener('close', handleDialogClose, {signal})
     dialog.addEventListener('cancel', handleDialogClose, {signal})
   }

app/components/primer/dialog_helper.ts

@@ -14,6 +14,45 @@ function dialogInvokerButtonHandler(event: Event) {
       // If the behaviour is allowed through the dialog will be shown but then
       // quickly hidden- as if it were never shown. This prevents that.
       event.preventDefault()
+
+      // In some older browsers, such as Chrome 122, when a top layer element (such as a dialog)
+      // opens from within a popover, the "hide all popovers" internal algorithm runs, hiding
+      // any popover that is currently open, regardless of whether or not another top layer element,
+      // such as a <dialog> is nested inside.
+      // See https://github.com/whatwg/html/issues/9998.
+      // This is fixed by https://github.com/whatwg/html/pull/10116, but while we still support browsers
+      // that present this bug, we must undo the work they did to hide ancestral popovers of the dialog:
+      let node: HTMLElement | null | undefined = button
+      let fixed = false
+      while (node) {
+        node = node.parentElement?.closest('[popover]:not(:popover-open)')
+        if (node && node.popover === 'auto') {
+          node.classList.add('dialog-inside-popover-fix')
+          node.popover = 'manual'
+          node.showPopover()
+          fixed = true
+        }
+      }
+      if (fixed) {
+        // We need to re-open the dialog as modal, and also ensure no close event listeners
+        // are trying to act on the close
+        dialog.addEventListener('close', e => e.stopImmediatePropagation(), {once: true})
+        dialog.close()
+        dialog.showModal()
+        dialog.addEventListener(
+          'close',
+          () => {
+            for (const el of dialog.ownerDocument.querySelectorAll<HTMLElement>('.dialog-inside-popover-fix')) {
+              if (el.contains(dialog)) {
+                el.classList.remove('dialog-inside-popover-fix')
+                el.popover = 'auto'
+                el.showPopover()
+              }
+            }
+          },
+          {once: true},
+        )
+      }
     }
   }
 
@@ -44,20 +83,6 @@ export class DialogHelperElement extends HTMLElement {
       for (const record of records) {
         if (record.target === this.dialog) {
           this.ownerDocument.body.classList.toggle('has-modal', this.dialog.hasAttribute('open'))
-          // In some older browsers, such as Chrome 122, when a top layer element (such as a dialog)
-          // opens from within a popover, the "hide all popovers" internal algorithm runs, hiding
-          // any popover that is currently open, regardless of whether or not another top layer element,
-          // such as a <dialog> is nested inside.
-          // See https://github.com/whatwg/html/issues/9998.
-          // This is fixed by https://github.com/whatwg/html/pull/10116, but while we still support browsers that present this bug,
-          // we must undo the work they did to hide ancestral popovers of the dialog:
-          if (this.dialog.hasAttribute('open')) {
-            let node: HTMLElement | null = this.dialog
-            while (node) {
-              node = node.closest('[popover]:not(:popover-open)')
-              if (node) node.showPopover()
-            }
-          }
         }
       }
     }).observe(this, {subtree: true, attributeFilter: ['open']})

previews/primer/alpha/dialog_preview.rb

@@ -259,6 +259,7 @@ def scroll_container(title: "Test Dialog", subtitle: nil, position: :center, siz
       # @param visually_hide_title [Boolean] toggle
       # @param button_text [String] text
       # @param body_text [String] text
+      # @snapshot interactive
       def dialog_inside_overlay(title: "Test Dialog", subtitle: nil, position: :center, size: :medium, button_text: "Show Dialog", body_text: "Content", position_narrow: :fullscreen, visually_hide_title: false)
         render_with_template(locals: {
                                title: title,

previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb

@@ -1,9 +1,20 @@
-<%= render(Primer::Alpha::Overlay.new(title: "An overlay")) do |o| %>
+<%= render(Primer::Alpha::Overlay.new(title: "An overlay", id: "first-overlay")) do |o| %>
   <% o.with_show_button() { "Show overlay" } %>
   <% o.with_body() do %>
     <%= render(Primer::Alpha::Dialog.new(id: "dialog-one", title: title, position: position, subtitle: subtitle, visually_hide_title: false)) do |d| %>
       <% d.with_show_button { button_text } %>
-      <% d.with_body { body_text} %>
+      <% d.with_body { body_text } %>
+      <% d.with_footer(show_divider: true) do %>
+        <%= render(Primer::ButtonComponent.new(data: { "close-dialog-id": "dialog-one" })) { "Cancel" } %>
+      <% end %>
     <% end %>
   <% end %>
 <% end %>
+
+<script type="module">
+document.getElementById('overlay-show-first-overlay')?.addEventListener('click', e => {
+    setTimeout(() => {
+        document.getElementById('first-overlay').querySelector('button')?.click()
+    });
+  });
+</script>

test/system/alpha/action_menu_test.rb

@@ -492,7 +492,7 @@ def test_deferred_dialog_opens
       click_on_invoker_button
       click_on_fourth_item
 
-      assert_selector "dialog[open]", visible: :hidden
+      assert_selector "dialog[open]"
 
       # menu should close
       refute_selector "action-menu ul li"

test/system/alpha/dialog_test.rb

@@ -113,7 +113,15 @@ def test_dialog_inside_overlay_opens_when_clicked
       visit_preview(:dialog_inside_overlay)
 
       click_button("Show overlay")
+      # This preview has script that automatically opens the dialog when the overlay is clicked
+      assert_selector "dialog[open]"
+
+      click_button("Cancel")
+
+      refute_selector "dialog[open]"
+
       click_button("Show Dialog")
+
       assert_selector "dialog[open]"
     end
   end