From 1259249eaddd4b8744a3dc212fb9f8800b45daac Mon Sep 17 00:00:00 2001 From: Keith Cirkel Date: Wed, 31 Jan 2024 17:57:24 +0000 Subject: [PATCH] Fix dialog click outside early return (#2549) Co-authored-by: Cameron Dutro --- .changeset/small-flowers-refuse.md | 5 +++++ app/components/primer/dialog_helper.ts | 8 ++++++-- test/css/component_specific_selectors_test.rb | 3 ++- test/system/alpha/dialog_test.rb | 19 +++++++++++++++++++ 4 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 .changeset/small-flowers-refuse.md diff --git a/.changeset/small-flowers-refuse.md b/.changeset/small-flowers-refuse.md new file mode 100644 index 0000000000..dd0de6179a --- /dev/null +++ b/.changeset/small-flowers-refuse.md @@ -0,0 +1,5 @@ +--- +"@primer/view-components": patch +--- + +Ensure dialogs do not close when a child menu item (or similar) is clicked diff --git a/app/components/primer/dialog_helper.ts b/app/components/primer/dialog_helper.ts index 7b9fbac396..11444e0e40 100644 --- a/app/components/primer/dialog_helper.ts +++ b/app/components/primer/dialog_helper.ts @@ -15,6 +15,7 @@ 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() + event.stopPropagation() } } @@ -35,7 +36,7 @@ export class DialogHelperElement extends HTMLElement { #abortController: AbortController | null = null connectedCallback() { const {signal} = (this.#abortController = new AbortController()) - document.addEventListener('click', dialogInvokerButtonHandler) + document.addEventListener('click', dialogInvokerButtonHandler, true) document.addEventListener('click', this, {signal}) this.ownerDocument.body.style.setProperty( '--dialog-scrollgutter', @@ -58,7 +59,10 @@ export class DialogHelperElement extends HTMLElement { const target = event.target as HTMLElement const dialog = this.dialog if (!dialog?.open) return - if (target?.closest('dialog') !== dialog) return + + // if the target is inside the dialog, but is not the dialog itself, leave + // the dialog open + if (target?.closest('dialog') === dialog && target !== dialog) return const rect = dialog.getBoundingClientRect() const clickWasInsideDialog = diff --git a/test/css/component_specific_selectors_test.rb b/test/css/component_specific_selectors_test.rb index 2c86ad2278..f745b1e6b8 100644 --- a/test/css/component_specific_selectors_test.rb +++ b/test/css/component_specific_selectors_test.rb @@ -37,7 +37,8 @@ class ComponentSpecificSelectorsTest < Minitest::Test ".Banner .Banner-close" ], Primer::Alpha::Dialog => [ - ".Overlay" + ".Overlay", + ".has-modal" ], Primer::Alpha::Layout => [ ".Layout-divider", diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb index 35c756b4d4..bbaef33431 100644 --- a/test/system/alpha/dialog_test.rb +++ b/test/system/alpha/dialog_test.rb @@ -74,5 +74,24 @@ def test_with_scrollable_body visit_preview(:long_text) click_button("Show Dialog") end + + def test_outside_click_closes_dialog + visit_preview(:default) + + click_button("Show Dialog") + page.driver.browser.mouse.click(x: 0, y: 0) + + refute_selector "dialog[open]" + end + + def test_outside_menu_click_does_not_close_dialog + visit_preview(:with_auto_complete) + + click_button("Show Dialog") + fill_in "input", with: "a" + + find(".ActionListItem", text: "Avocado").click + assert_selector "dialog[open]" + end end end