From 652e7957c59ddc5d05af9c9bc797a917d01cf453 Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Thu, 1 Feb 2024 18:28:38 +0000 Subject: [PATCH] Ensure popovers that open dialogs do not close when the dialog opens (#2554) --- .changeset/cool-meals-appear.md | 5 ++++ app/components/primer/dialog_helper.ts | 14 +++++++++++ previews/primer/alpha/dialog_preview.rb | 23 +++++++++++++++++++ .../dialog_inside_overlay.html.erb | 9 ++++++++ test/system/alpha/dialog_test.rb | 8 +++++++ 5 files changed, 59 insertions(+) create mode 100644 .changeset/cool-meals-appear.md create mode 100644 previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb diff --git a/.changeset/cool-meals-appear.md b/.changeset/cool-meals-appear.md new file mode 100644 index 0000000000..42c7a7a5ee --- /dev/null +++ b/.changeset/cool-meals-appear.md @@ -0,0 +1,5 @@ +--- +"@primer/view-components": patch +--- + +Ensure Overlays that open dialogs do not close when the Dialog opens diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts index 11444e0e40..b39ad5dac4 100644 --- a/app/components/primer/dialog_helper.ts +++ b/app/components/primer/dialog_helper.ts @@ -46,6 +46,20 @@ 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 c92bedcd7b..40032b41f6 100644 --- a/previews/primer/alpha/dialog_preview.rb +++ b/previews/primer/alpha/dialog_preview.rb @@ -248,6 +248,29 @@ def scroll_container(title: "Test Dialog", subtitle: nil, position: :center, siz visually_hide_title: visually_hide_title }) end + + # @label Dialog inside Overlay + # + # @param title [String] text + # @param subtitle [String] text + # @param size [Symbol] select [small, medium, medium_portrait, large, xlarge] + # @param position [Symbol] select [center, right, left] + # @param position_narrow [Symbol] select [inherit, bottom, fullscreen, left, right] + # @param visually_hide_title [Boolean] toggle + # @param button_text [String] text + # @param body_text [String] text + 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, + subtitle: subtitle, + position: position, + size: size, + button_text: button_text, + body_text: body_text, + position_narrow: position_narrow, + visually_hide_title: visually_hide_title + }) + end end end end diff --git a/previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb b/previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb new file mode 100644 index 0000000000..198d2609b7 --- /dev/null +++ b/previews/primer/alpha/dialog_preview/dialog_inside_overlay.html.erb @@ -0,0 +1,9 @@ +<%= render(Primer::Alpha::Overlay.new(title: "An 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} %> + <% end %> + <% end %> +<% end %> diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb index bbaef33431..95e7f1389a 100644 --- a/test/system/alpha/dialog_test.rb +++ b/test/system/alpha/dialog_test.rb @@ -93,5 +93,13 @@ def test_outside_menu_click_does_not_close_dialog find(".ActionListItem", text: "Avocado").click assert_selector "dialog[open]" end + + def test_dialog_inside_overlay_opens_when_clicked + visit_preview(:dialog_inside_overlay) + + click_button("Show overlay") + click_button("Show Dialog") + assert_selector "dialog[open]" + end end end