From bf60d7d703752b9a6c096c8d1b1bd2fd0558a607 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Thu, 8 Jun 2023 14:24:34 -0700 Subject: [PATCH] Fix keyboard functionality with deferred loading in ActionMenu (#2059) --- .changeset/moody-pens-cry.md | 5 +++ .../alpha/action_menu/action_menu_element.ts | 14 ++++-- app/components/primer/primer.ts | 1 + previews/primer/alpha/action_menu_preview.rb | 5 +++ .../action_menu_preview/two_menus.html.erb | 13 ++++++ test/system/alpha/action_menu_test.rb | 43 +++++++++++++++++++ 6 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 .changeset/moody-pens-cry.md create mode 100644 previews/primer/alpha/action_menu_preview/two_menus.html.erb diff --git a/.changeset/moody-pens-cry.md b/.changeset/moody-pens-cry.md new file mode 100644 index 0000000000..940f9b1172 --- /dev/null +++ b/.changeset/moody-pens-cry.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Fix keyboard functionality with deferred loading in ActionMenu 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 099271544c..6cd21d2537 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -1,4 +1,5 @@ -import '@github/include-fragment-element' +import {controller, target} from '@github/catalyst' +import IncludeFragmentElement from '@github/include-fragment-element' type SelectVariant = 'none' | 'single' | 'multiple' | null type SelectedItem = { @@ -9,7 +10,10 @@ type SelectedItem = { const menuItemSelectors = ['[role="menuitem"]', '[role="menuitemcheckbox"]', '[role="menuitemradio"]'] +@controller export class ActionMenuElement extends HTMLElement { + @target includeFragment: IncludeFragmentElement + #abortController: AbortController #originalLabel = '' #inputName = '' @@ -87,6 +91,10 @@ export class ActionMenuElement extends HTMLElement { this.addEventListener('focusout', this, {signal}) this.#setDynamicLabel() this.#updateInput() + + if (this.includeFragment) { + this.includeFragment.addEventListener('include-fragment-replaced', this, {signal}) + } } disconnectedCallback() { @@ -105,8 +113,8 @@ export class ActionMenuElement extends HTMLElement { if (!this.popoverElement?.matches(':popover-open')) return - if (event.type === 'focusout' && !this.contains((event as FocusEvent).relatedTarget as Node)) { - this.popoverElement?.hidePopover() + if (event.type === 'include-fragment-replaced') { + if (this.#firstItem) this.#firstItem.focus() } else if (this.#isActivationKeydown(event) || (event instanceof MouseEvent && event.type === 'click')) { const item = (event.target as Element).closest(menuItemSelectors.join(',')) if (!item) return diff --git a/app/components/primer/primer.ts b/app/components/primer/primer.ts index 4de1f92bf6..604309ad60 100644 --- a/app/components/primer/primer.ts +++ b/app/components/primer/primer.ts @@ -1,3 +1,4 @@ +import '@github/include-fragment-element' import '@oddbird/popover-polyfill' import './alpha/dropdown' import './anchored_position' diff --git a/previews/primer/alpha/action_menu_preview.rb b/previews/primer/alpha/action_menu_preview.rb index d5849130ca..ca46944933 100644 --- a/previews/primer/alpha/action_menu_preview.rb +++ b/previews/primer/alpha/action_menu_preview.rb @@ -284,6 +284,11 @@ def leading_visual_single_select end end end + + # @label Two menus + # + def two_menus + end end end end diff --git a/previews/primer/alpha/action_menu_preview/two_menus.html.erb b/previews/primer/alpha/action_menu_preview/two_menus.html.erb new file mode 100644 index 0000000000..cbe020fad6 --- /dev/null +++ b/previews/primer/alpha/action_menu_preview/two_menus.html.erb @@ -0,0 +1,13 @@ +<%= render(Primer::Alpha::ActionMenu.new(menu_id: "menu-1")) do |menu| %> + <% menu.with_show_button { |button| button.with_trailing_action_icon(icon: :"triangle-down"); "Pac-Man" } %> + <% menu.with_item(label: "Eat a dot") %> + <% menu.with_item(label: "Avoid a ghost") %> + <% menu.with_item(label: "Eat a stunned ghost") %> +<% end %> + +<%= render(Primer::Alpha::ActionMenu.new(menu_id: "menu-2")) do |menu| %> + <% menu.with_show_button { |button| button.with_trailing_action_icon(icon: :"triangle-down"); "Mario Bros" } %> + <% menu.with_item(label: "Stomp a turtle") %> + <% menu.with_item(label: "Collect a gold coin") %> + <% menu.with_item(label: "Save the princess") %> +<% end %> diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 334f247234..546c35d06a 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -140,6 +140,9 @@ def test_opens_dialog find("action-menu ul li:nth-child(2)").click assert_selector "modal-dialog#my-dialog" + + # opening the dialog should close the menu + refute_selector "action-menu ul li" end def test_opens_dialog_on_keydown @@ -223,5 +226,45 @@ def test_individual_items_can_submit_post_requests_via_forms assert_equal page.text, 'You selected "bar"' end + + def test_deferred_loading + visit_preview(:with_deferred_content) + + find("action-menu button[aria-controls]").click + + assert_selector "action-menu ul li", text: "Copy link" + assert_selector "action-menu ul li", text: "Quote reply" + assert_selector "action-menu ul li", text: "Reference in new issue" + + assert_equal page.evaluate_script("document.activeElement").text, "Copy link" + end + + def test_deferred_loading_on_keydown + visit_preview(:with_deferred_content) + + page.evaluate_script(<<~JS) + document.querySelector('action-menu button[aria-controls]').focus() + JS + + page.driver.browser.keyboard.type(:enter) + + # wait for menu to load + assert_selector "action-menu ul li", text: "Copy link" + assert_equal page.evaluate_script("document.activeElement").text, "Copy link" + end + + def test_opening_second_menu_closes_first_menu + visit_preview(:two_menus) + + find("#menu-1-button").click + + assert_selector "action-menu ul li", text: "Eat a dot" + refute_selector "action-menu ul li", text: "Stomp a turtle" + + find("#menu-2-button").click + + refute_selector "action-menu ul li", text: "Eat a dot" + assert_selector "action-menu ul li", text: "Stomp a turtle" + end end end