diff --git a/.changeset/smart-scissors-type.md b/.changeset/smart-scissors-type.md new file mode 100644 index 0000000000..7701470fe8 --- /dev/null +++ b/.changeset/smart-scissors-type.md @@ -0,0 +1,7 @@ +--- +'@primer/view-components': patch +--- + +Fix dialog invocation within deferred ActionMenus + + 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 a49e918228..218d8d9543 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -13,7 +13,8 @@ const menuItemSelectors = ['[role="menuitem"]', '[role="menuitemcheckbox"]', '[r @controller export class ActionMenuElement extends HTMLElement { - @target includeFragment: IncludeFragmentElement + @target + includeFragment: IncludeFragmentElement #abortController: AbortController #originalLabel = '' @@ -57,7 +58,9 @@ export class ActionMenuElement extends HTMLElement { const id = this.querySelector('[role=menu]')?.id if (!id) return null for (const el of this.querySelectorAll(`[aria-controls]`)) { - if (el.getAttribute('aria-controls') === id) return el as HTMLButtonElement + if (el.getAttribute('aria-controls') === id) { + return el as HTMLButtonElement + } } return null } @@ -94,7 +97,9 @@ export class ActionMenuElement extends HTMLElement { this.#updateInput() if (this.includeFragment) { - this.includeFragment.addEventListener('include-fragment-replaced', this, {signal}) + this.includeFragment.addEventListener('include-fragment-replaced', this, { + signal + }) } } @@ -103,7 +108,8 @@ export class ActionMenuElement extends HTMLElement { } handleEvent(event: Event) { - if (event.target === this.invokerElement && this.#isActivationKeydown(event)) { + const activation = this.#isActivationKeydown(event) + if (event.target === this.invokerElement && activation) { if (this.#firstItem) { event.preventDefault() this.popoverElement?.showPopover() @@ -112,11 +118,39 @@ export class ActionMenuElement extends HTMLElement { } } + // Ignore events within dialogs within menus + if ((event.target as Element)?.closest('dialog') || (event.target as Element)?.closest('modal-dialog')) { + return + } + + // If a dialog has been rendered within the menu, we do not want to hide + // the entire menu, as that will also hide the Dialog. Instead we want to + // show the Dialog while hiding just the visible part of the menu. + if ((activation || event.type === 'click') && (event.target as HTMLElement)?.closest('[data-show-dialog-id]')) { + const dialogInvoker = (event.target as HTMLElement)!.closest('[data-show-dialog-id]') + const dialog = this.ownerDocument.getElementById(dialogInvoker?.getAttribute('data-show-dialog-id') || '') + if (dialogInvoker && dialog && this.contains(dialogInvoker) && this.contains(dialog)) { + this.querySelector('.ActionListWrap')!.style.display = 'none' + const dialog_controller = new AbortController() + const {signal} = dialog_controller + const handleDialogClose = () => { + dialog_controller.abort() + this.querySelector('.ActionListWrap')!.style.display = '' + if (this.popoverElement?.matches(':popover-open')) { + this.popoverElement?.hidePopover() + } + } + dialog.addEventListener('close', handleDialogClose, {signal}) + dialog.addEventListener('cancel', handleDialogClose, {signal}) + return + } + } + if (!this.popoverElement?.matches(':popover-open')) return if (event.type === 'include-fragment-replaced') { if (this.#firstItem) this.#firstItem.focus() - } else if (this.#isActivationKeydown(event) || (event instanceof MouseEvent && event.type === 'click')) { + } else if (activation || (event instanceof MouseEvent && event.type === 'click')) { // Hide popover after current event loop to prevent changes in focus from // altering the target of the event. Not doing this specifically affects // tags. It causes the event to be sent to the currently focused element diff --git a/demo/app/views/action_menu/deferred.html.erb b/demo/app/views/action_menu/deferred.html.erb index e2daa42892..7e4793c923 100644 --- a/demo/app/views/action_menu/deferred.html.erb +++ b/demo/app/views/action_menu/deferred.html.erb @@ -2,4 +2,22 @@ <% list.with_item(label: "Copy link", value: "", autofocus: true) %> <% list.with_item(label: "Quote reply", value: "") %> <% list.with_item(label: "Reference in new issue", value: "") %> + <% list.with_item( + label: "Show dialog", + tag: :button, + content_arguments: { "data-show-dialog-id": "my-dialog" }, + value: "", + scheme: :danger + ) %> +<% end %> + + +<%= render(Primer::Alpha::Dialog.new(id: "my-dialog", title: "Confirm deletion")) do |d| %> + <%= render(Primer::Alpha::Dialog::Body.new()) do %> + Are you sure you want to delete this? + <% end %> + <%= render(Primer::Alpha::Dialog::Footer.new()) do %> + <%= render(Primer::Beta::Button.new(data: { "close-dialog-id": "my-dialog" })) { "Cancel" } %> + <%= render(Primer::Beta::Button.new(scheme: :danger)) { "Delete" } %> + <% end %> <% end %> diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 218eeb5fa5..3c25ba1fe3 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -265,6 +265,16 @@ def test_deferred_loading_on_keydown assert_equal page.evaluate_script("document.activeElement").text, "Copy link" end + def test_deferred_dialog_opens + visit_preview(:with_deferred_content) + + find("action-menu button[aria-controls]").click + + find("action-menu ul li:nth-child(4)").click + + assert_selector "modal-dialog[open]" + end + def test_opening_second_menu_closes_first_menu visit_preview(:two_menus)