diff --git a/.changeset/lucky-apricots-move.md b/.changeset/lucky-apricots-move.md new file mode 100644 index 0000000000..db21d586b9 --- /dev/null +++ b/.changeset/lucky-apricots-move.md @@ -0,0 +1,5 @@ +--- +"@primer/view-components": patch +--- + +Fix issue with layering of nested overlays/dialogs diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark.png new file mode 100644 index 0000000000..f934092abb Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_colorblind.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_colorblind.png new file mode 100644 index 0000000000..a7deefb440 Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_colorblind.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_dimmed.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_dimmed.png new file mode 100644 index 0000000000..2fff2c180a Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_dimmed.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_high_contrast.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_high_contrast.png new file mode 100644 index 0000000000..ef5f65b432 Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/dark_high_contrast.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/default.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/default.png new file mode 100644 index 0000000000..d4ca658310 Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/default.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/focused.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/focused.png new file mode 100644 index 0000000000..5e64a34592 Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/focused.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light.png new file mode 100644 index 0000000000..157298c610 Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light_colorblind.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light_colorblind.png new file mode 100644 index 0000000000..aba771c985 Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light_colorblind.png differ diff --git a/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light_high_contrast.png b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light_high_contrast.png new file mode 100644 index 0000000000..e3091f4164 Binary files /dev/null and b/.playwright/screenshots/snapshots.test.ts-snapshots/primer/alpha/dialog/dialog_inside_overlay/light_high_contrast.png differ diff --git a/app/components/primer/alpha/action_menu/action_menu_element.ts b/app/components/primer/alpha/action_menu/action_menu_element.ts index 2600f0ead7..10829892cf 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -287,7 +287,6 @@ export class ActionMenuElement extends HTMLElement { } } // a modal 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 index 55c5ceafcd..e7582fc5bf 100644 --- a/app/components/primer/dialog_helper.ts +++ b/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 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('.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 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']}) diff --git a/previews/primer/alpha/dialog_preview.rb b/previews/primer/alpha/dialog_preview.rb index 40032b41f6..9347e07025 100644 --- a/previews/primer/alpha/dialog_preview.rb +++ b/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, diff --git a/previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb b/previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb index 198d2609b7..0e90cb5c11 100644 --- a/previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb +++ b/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 %> + + diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index ccb14cda3e..3c1edcf5dd 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/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" diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb index 2071f7172a..95392e6660 100644 --- a/test/system/alpha/dialog_test.rb +++ b/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