diff --git a/.changeset/mighty-pandas-pump.md b/.changeset/mighty-pandas-pump.md new file mode 100644 index 0000000000..0e582e14d6 --- /dev/null +++ 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 index 6cd21d2537..1476d30b99 100644 --- 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 + // 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 - // 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 index ca46944933..ad41d97b97 100644 --- 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 new file mode 100644 index 0000000000..35d91ce360 --- /dev/null +++ b/previews/primer/alpha/action_menu_preview/with_actions.html.erb @@ -0,0 +1,21 @@ + + +<%= 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 %>