ActionMenu: don't swallow events attached via addEventListener (#2071)

primer · Jun 12, 2023 · c0fa71e · c0fa71e
1 parent 9c53f8e
commit c0fa71e
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 22 deletions.
diff --git a/.changeset/mighty-pandas-pump.md b/.changeset/mighty-pandas-pump.md
@@ -0,0 +1,5 @@
+---
+'@primer/view-components': patch
+---
+
+ActionMenu: don't swallow events attached via addEventListener
diff --git a/app/components/primer/alpha/action_menu/action_menu_element.ts b/app/components/primer/alpha/action_menu/action_menu_element.ts
@@ -116,6 +116,20 @@ export class ActionMenuElement extends HTMLElement {
     if (event.type === 'include-fragment-replaced') {
       if (this.#firstItem) this.#firstItem.focus()
     } else if (this.#isActivationKeydown(event) || (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
+      // <a> tags. It causes the event to be sent to the currently focused element
+      // instead of the anchor, which effectively prevents navigation, i.e. it
+      // appears as if hitting enter does nothing. Curiously, clicking instead
+      // works fine.
+      if (this.selectVariant !== 'multiple') {
+        setTimeout(() => this.popoverElement?.hidePopover())
+      }
+
+      // The rest of the code below deals with single/multiple selection behavior, and should not
+      // interfere with events fired by menu items whose behavior is specified outside the library.
+      if (this.selectVariant !== 'multiple' && this.selectVariant !== 'single') return
+
       const item = (event.target as Element).closest(menuItemSelectors.join(','))
       if (!item) return
       const ariaChecked = item.getAttribute('aria-checked')
@@ -136,15 +150,6 @@ export class ActionMenuElement extends HTMLElement {
         // prevent buttons from being clicked twice
         event.preventDefault()
       }
-      // Hide popover after current event loop to prevent changes in focus from
-      // altering the target of the event. Not doing this specifically affects
-      // <a> tags. It causes the event to be sent to the currently focused element
-      // instead of the anchor, which effectively prevents navigation, i.e. it
-      // appears as if hitting enter does nothing. Curiously, clicking instead
-      // works fine.
-      if (this.selectVariant !== 'multiple') {
-        setTimeout(() => this.popoverElement?.hidePopover())
-      }
     }
   }
 

diff --git a/previews/primer/alpha/action_menu_preview.rb b/previews/primer/alpha/action_menu_preview.rb
@@ -180,19 +180,6 @@ def with_deferred_preloaded_content
       # @label With actions
       #
       def with_actions
-        render(Primer::Alpha::ActionMenu.new) do |component|
-          component.with_show_button { "Trigger" }
-          component.with_item(label: "Alert", tag: :button, content_arguments: { onclick: "alert('Foo')", onkeydown: "if (event.key === 'Enter') { alert(event.key) }" })
-          component.with_item(label: "Navigate", tag: :a, content_arguments: { href: UrlHelpers.action_menu_landing_path })
-          component.with_item(label: "Copy text", tag: :"clipboard-copy", content_arguments: { value: "Text to copy" })
-          component.with_item(
-            label: "Submit form",
-            href: UrlHelpers.action_menu_form_action_path,
-            form_arguments: {
-              name: "foo", value: "bar", method: :post
-            }
-          )
-        end
       end
 
       # @label Single select form

diff --git a/previews/primer/alpha/action_menu_preview/with_actions.html.erb b/previews/primer/alpha/action_menu_preview/with_actions.html.erb
@@ -0,0 +1,21 @@
+<script type="text/javascript">
+  window.addEventListener('load', function() {
+    document.querySelector('button#alert-item').addEventListener('click', (_e) => {
+      alert('Foo')
+    });
+  }, false);
+</script>
+
+<%= render(Primer::Alpha::ActionMenu.new) do |component| %>
+  <% component.with_show_button { "Trigger" } %>
+  <% component.with_item(label: "Alert", tag: :button, id: "alert-item") %>
+  <% component.with_item(label: "Navigate", tag: :a, content_arguments: { href: action_menu_landing_path }) %>
+  <% component.with_item(label: "Copy text", tag: :"clipboard-copy", content_arguments: { value: "Text to copy" }) %>
+  <% component.with_item(
+    label: "Submit form",
+    href: action_menu_form_action_path,
+    form_arguments: {
+      name: "foo", value: "bar", method: :post
+    }
+  ) %>
+<% end %>