From 8e0443b8e67027ef583b5b19c85975f2e6f71a3b Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Tue, 17 Oct 2023 20:40:28 +0000 Subject: [PATCH 1/4] Adjust `modal_dialog.ts` and click functionality --- app/components/primer/alpha/modal_dialog.ts | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/app/components/primer/alpha/modal_dialog.ts b/app/components/primer/alpha/modal_dialog.ts index aad3b0f510..53e97edba9 100644 --- a/app/components/primer/alpha/modal_dialog.ts +++ b/app/components/primer/alpha/modal_dialog.ts @@ -30,20 +30,17 @@ function clickHandler(event: Event) { return } } - // Find the top level dialog that is open. - const topLevelDialog = overlayStack[overlayStack.length - 1] - if (!topLevelDialog) return - dialogId = button.getAttribute('data-close-dialog-id') - if (dialogId === topLevelDialog.id) { - overlayStack.pop() - topLevelDialog.close() - } + if (!overlayStack.length) return - dialogId = button.getAttribute('data-submit-dialog-id') - if (dialogId === topLevelDialog.id) { - overlayStack.pop() - topLevelDialog.close(true) + dialogId = button.getAttribute('data-close-dialog-id') || button.getAttribute('data-submit-dialog-id') + if (dialogId) { + const dialog = document.getElementById(dialogId) + if (dialog instanceof ModalDialogElement) { + const dialogIndex = overlayStack.findIndex(ele => ele.id === dialogId) + overlayStack.splice(dialogIndex, 1) + dialog.close() + } } } From dd72f4592357ca50866d4122ed0285b79715a521 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Tue, 17 Oct 2023 20:53:02 +0000 Subject: [PATCH 2/4] Add boolean param to `.close()` --- app/components/primer/alpha/modal_dialog.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/primer/alpha/modal_dialog.ts b/app/components/primer/alpha/modal_dialog.ts index 53e97edba9..62165c3810 100644 --- a/app/components/primer/alpha/modal_dialog.ts +++ b/app/components/primer/alpha/modal_dialog.ts @@ -39,7 +39,7 @@ function clickHandler(event: Event) { if (dialog instanceof ModalDialogElement) { const dialogIndex = overlayStack.findIndex(ele => ele.id === dialogId) overlayStack.splice(dialogIndex, 1) - dialog.close() + dialog.close(button.hasAttribute('data-submit-dialog-id')) } } } From c6a2e2ae230dfb6fb1ba2dc4659dcd12d45041fc Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Tue, 17 Oct 2023 21:14:40 +0000 Subject: [PATCH 3/4] Add changeset --- .changeset/eleven-steaks-press.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/eleven-steaks-press.md diff --git a/.changeset/eleven-steaks-press.md b/.changeset/eleven-steaks-press.md new file mode 100644 index 0000000000..b11f88cc19 --- /dev/null +++ b/.changeset/eleven-steaks-press.md @@ -0,0 +1,7 @@ +--- +'@primer/view-components': patch +--- + +Fixes issue where sometimes a dialog cannot be closed if another is open + + From a108bcad1981d85a89c0ac6ac1af0c2f59fdfc91 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 18 Oct 2023 00:08:36 +0000 Subject: [PATCH 4/4] Add more test cases --- test/system/alpha/dialog_test.rb | 39 ++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/system/alpha/dialog_test.rb b/test/system/alpha/dialog_test.rb index 7238074605..b16b65e12f 100644 --- a/test/system/alpha/dialog_test.rb +++ b/test/system/alpha/dialog_test.rb @@ -4,6 +4,18 @@ module Alpha class IntegrationDialogTest < System::TestCase + def click_on_initial_dialog_close_button + find("button[data-close-dialog-id='dialog-one']").trigger("click") + end + + def click_on_nested_dialog_close_button + find("button[data-close-dialog-id='dialog-two']").click + end + + def click_on_nested_dialog_button + find("#dialog-show-dialog-two").click + end + def test_modal_has_accessible_name visit_preview(:default) @@ -20,5 +32,32 @@ def test_focuses_close_button assert_equal page.evaluate_script("document.activeElement")["aria-label"], "Close" end + + def test_closes_top_level_dialog + visit_preview(:nested_dialog) + + click_button("Show Dialog") + click_on_nested_dialog_button + + assert_equal(find("modal-dialog#dialog-two")["open"], true) + + click_on_nested_dialog_close_button + + assert_selector "modal-dialog#dialog-two", visible: :hidden + assert_selector "modal-dialog#dialog-one" + end + + def test_closes_dialog_that_is_not_top_level + visit_preview(:nested_dialog) + + click_button("Show Dialog") + click_on_nested_dialog_button + + assert_equal(find("modal-dialog#dialog-two")["open"], true) + + click_on_initial_dialog_close_button + + assert_selector "modal-dialog#dialog-one", visible: :hidden + end end end