Fix ActionMenu item activation

.changeset/orange-bats-arrive.md

@@ -0,0 +1,5 @@
+---
+'@primer/view-components': patch
+---
+
+Fix ActionMenu item activation by using PointerEvent instead of MouseEvent and KeyboardEvent.

app/components/primer/alpha/action_menu/action_menu_element.ts

@@ -151,34 +151,21 @@ export class ActionMenuElement extends HTMLElement {
     return false
   }
 
-  #isKeyboardActivation(event: Event): boolean {
-    return this.#isKeyboardActivationViaEnter(event) || this.#isKeyboardActivationViaSpace(event)
-  }
-
-  #isKeyboardActivationViaEnter(event: Event): boolean {
-    return (
-      event instanceof KeyboardEvent &&
-      event.type === 'keydown' &&
-      !(event.ctrlKey || event.altKey || event.metaKey || event.shiftKey) &&
-      event.key === 'Enter'
-    )
-  }
-
-  #isKeyboardActivationViaSpace(event: Event): boolean {
+  #isAnchorActivationViaSpace(event: Event): boolean {
     return (
+      event.target instanceof HTMLAnchorElement &&
       event instanceof KeyboardEvent &&
       event.type === 'keydown' &&
       !(event.ctrlKey || event.altKey || event.metaKey || event.shiftKey) &&
       event.key === ' '
     )
   }
 
-  #isMouseActivation(event: Event): boolean {
-    return event instanceof MouseEvent && event.type === 'click'
-  }
-
   #isActivation(event: Event): boolean {
-    return this.#isMouseActivation(event) || this.#isKeyboardActivation(event)
+    // Some browsers fire MouseEvents (Firefox) and others fire PointerEvents (Chrome). Activating an item via
+    // enter or space counterintuitively fires one of these rather than a KeyboardEvent. Since PointerEvent
+    // inherits from MouseEvent, it is enough to check for MouseEvent here.
+    return (event instanceof MouseEvent && event.type === 'click') || this.#isAnchorActivationViaSpace(event)
   }
 
   handleEvent(event: Event) {
@@ -237,21 +224,15 @@ export class ActionMenuElement extends HTMLElement {
         }
       }
 
-      this.#activateItem(event, item)
-      this.#handleItemActivated(item)
-
-      // Pressing the space key on a button or link will cause the page to scroll unless preventDefault()
-      // is called. While calling preventDefault() appears to have no effect on link navigation, it skips
-      // form submission. The code below therefore only calls preventDefault() if the button has been
-      // activated by the space key, and manually submits the form if the button is a submit button.
-      if (this.#isKeyboardActivationViaSpace(event)) {
+      // Pressing the space key on a link will cause the page to scroll unless preventDefault() is called.
+      // We then click it manually to navigate.
+      if (this.#isAnchorActivationViaSpace(event)) {
         event.preventDefault()
-
-        if (item.getAttribute('type') === 'submit') {
-          item.closest('form')?.submit()
-        }
+        ;(item as HTMLElement).click()
       }
 
+      this.#handleItemActivated(item)
+
       return
     }
 
@@ -335,33 +316,14 @@ export class ActionMenuElement extends HTMLElement {
     }
 
     this.#updateInput()
+
     this.dispatchEvent(
       new CustomEvent('itemActivated', {
         detail: {item: item.parentElement, checked: this.isItemChecked(item.parentElement)},
       }),
     )
   }
 
-  #activateItem(event: Event, item: Element) {
-    const eventWillActivateByDefault =
-      (event instanceof MouseEvent && event.type === 'click') ||
-      (event instanceof KeyboardEvent &&
-        event.type === 'keydown' &&
-        !(event.ctrlKey || event.altKey || event.metaKey || event.shiftKey) &&
-        event.key === 'Enter')
-
-    // if the event will result in activating the current item by default, i.e. is a
-    // mouse click or keyboard enter, bail out
-    if (eventWillActivateByDefault) return
-
-    // otherwise, event will not result in activation by default, so we stop it and
-    // simulate a click
-    /* eslint-disable-next-line no-restricted-syntax */
-    event.stopPropagation()
-    const elem = item as HTMLElement
-    elem.click()
-  }
-
   #handleIncludeFragmentReplaced() {
     if (this.#firstItem) this.#firstItem.focus()
     this.#softDisableItems()

test/system/alpha/action_menu_test.rb

@@ -508,6 +508,8 @@ def test_deferred_dialog_opens
       visit_preview(:with_deferred_content)
 
       click_on_invoker_button
+      # wait for items to load
+      assert_selector "action-menu ul li"
       click_on_fourth_item
 
       assert_selector "dialog[open]"
@@ -540,6 +542,40 @@ def test_single_select_item_checked
       assert_selector "[aria-checked=true]", text: "Recursive", visible: :hidden
     end
 
+    def test_single_select_item_checked_via_keyboard_enter
+      visit_preview(:single_select)
+
+      focus_on_invoker_button
+
+      # open menu, "click" on first item
+      keyboard.type(:enter, :enter)
+
+      # activating item closes menu, so checked item is hidden
+      assert_selector "[aria-checked=true]", text: "Fast forward", visible: :hidden
+
+      focus_on_invoker_button
+
+      keyboard.type(:enter, :down, :enter)
+      assert_selector "[aria-checked=true]", text: "Recursive", visible: :hidden
+    end
+
+    def test_single_select_item_checked_via_keyboard_space
+      visit_preview(:single_select)
+
+      focus_on_invoker_button
+
+      # open menu, "click" on first item
+      keyboard.type(:enter, :space)
+
+      # activating item closes menu, so checked item is hidden
+      assert_selector "[aria-checked=true]", text: "Fast forward", visible: :hidden
+
+      focus_on_invoker_button
+
+      keyboard.type(:enter, :down, :space)
+      assert_selector "[aria-checked=true]", text: "Recursive", visible: :hidden
+    end
+
     def test_single_select_item_unchecks_previously_checked_item
       visit_preview(:single_select)
 
@@ -581,14 +617,47 @@ def test_multi_select_items_checked
       assert_selector "[aria-checked=true]", text: "broccolinisoup"
     end
 
+    def test_multi_select_items_checked_via_keyboard_enter
+      visit_preview(:multiple_select)
+
+      focus_on_invoker_button
+
+      # open menu, select first item
+      keyboard.type(:enter, :enter)
+
+      assert_selector "[aria-checked=true]", text: "langermank"
+
+      # select second item
+      keyboard.type(:down, :enter)
+
+      assert_selector "[aria-checked=true]", text: "langermank"
+      assert_selector "[aria-checked=true]", text: "jonrohan"
+    end
+
+    def test_multi_select_items_checked_via_keyboard_space
+      visit_preview(:multiple_select)
+
+      focus_on_invoker_button
+
+      # open menu, select first item
+      keyboard.type(:enter, :space)
+
+      assert_selector "[aria-checked=true]", text: "langermank"
+
+      # select second item
+      keyboard.type(:down, :space)
+
+      assert_selector "[aria-checked=true]", text: "langermank"
+      assert_selector "[aria-checked=true]", text: "jonrohan"
+    end
+
     def test_multi_select_items_can_be_unchecked
       visit_preview(:multiple_select)
 
       click_on_invoker_button
       click_on_second_item
       click_on_third_item
 
-      # clicking item closes menu, so checked item is hidden
       assert_selector "[aria-checked=true]", text: "jonrohan"
       assert_selector "[aria-checked=true]", text: "broccolinisoup"