Skip to content

Commit

Permalink
Fix keyboard functionality with deferred loading in ActionMenu (#2059)
Browse files Browse the repository at this point in the history
  • Loading branch information
camertron authored Jun 8, 2023
1 parent 3e50e06 commit bf60d7d
Show file tree
Hide file tree
Showing 6 changed files with 78 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/moody-pens-cry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/view-components': patch
---

Fix keyboard functionality with deferred loading in ActionMenu
14 changes: 11 additions & 3 deletions app/components/primer/alpha/action_menu/action_menu_element.ts
Original file line number Diff line number Diff line change
@@ -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 = {
Expand All @@ -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 = ''
Expand Down Expand Up @@ -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() {
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions app/components/primer/primer.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import '@github/include-fragment-element'
import '@oddbird/popover-polyfill'
import './alpha/dropdown'
import './anchored_position'
Expand Down
5 changes: 5 additions & 0 deletions previews/primer/alpha/action_menu_preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,11 @@ def leading_visual_single_select
end
end
end

# @label Two menus
#
def two_menus
end
end
end
end
13 changes: 13 additions & 0 deletions previews/primer/alpha/action_menu_preview/two_menus.html.erb
Original file line number Diff line number Diff line change
@@ -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 %>
43 changes: 43 additions & 0 deletions test/system/alpha/action_menu_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

0 comments on commit bf60d7d

Please sign in to comment.