Several ActionMenu a11y fixes (primer#2260)

.changeset/sour-colts-prove.md

@@ -0,0 +1,7 @@
+---
+'@primer/view-components': patch
+---
+
+ActionMenu: hide the menu when focus leaves the component; focus the first list item when the menu is activated with the mouse; allow disabling list items while still permitting them to be focused with the keyboard
+
+<!-- Changed components: Primer::Alpha::ActionMenu -->

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

@@ -20,6 +20,7 @@ export class ActionMenuElement extends HTMLElement {
   #abortController: AbortController
   #originalLabel = ''
   #inputName = ''
+  #invokerBeingClicked = false
 
   get selectVariant(): SelectVariant {
     return this.getAttribute('data-select-variant') as SelectVariant
@@ -52,7 +53,7 @@ export class ActionMenuElement extends HTMLElement {
   }
 
   get popoverElement(): HTMLElement | null {
-    return this.invokerElement?.popoverTargetElement || null
+    return (this.invokerElement?.popoverTargetElement as HTMLElement) || null
   }
 
   get invokerElement(): HTMLButtonElement | null {
@@ -94,8 +95,10 @@ export class ActionMenuElement extends HTMLElement {
     this.addEventListener('click', this, {signal})
     this.addEventListener('mouseover', this, {signal})
     this.addEventListener('focusout', this, {signal})
+    this.addEventListener('mousedown', this, {signal})
     this.#setDynamicLabel()
     this.#updateInput()
+    this.#softDisableItems()
 
     if (this.includeFragment) {
       this.includeFragment.addEventListener('include-fragment-replaced', this, {
@@ -104,104 +107,231 @@ export class ActionMenuElement extends HTMLElement {
     }
   }
 
+  #softDisableItems() {
+    const {signal} = this.#abortController
+
+    for (const item of this.#items) {
+      item.addEventListener('click', this.#potentiallyDisallowActivation.bind(this), {signal})
+      item.addEventListener('keydown', this.#potentiallyDisallowActivation.bind(this), {signal})
+    }
+  }
+
+  #potentiallyDisallowActivation(event: Event) {
+    if (!this.#isActivation(event)) return
+
+    const item = (event.target as HTMLElement).closest(menuItemSelectors.join(','))
+    if (!item) return
+
+    if (item.getAttribute('aria-disabled')) {
+      event.preventDefault()
+      event.stopPropagation()
+      event.stopImmediatePropagation()
+    }
+  }
+
   disconnectedCallback() {
     this.#abortController.abort()
   }
 
+  #isKeyboardActivation(event: Event): boolean {
+    return (
+      event instanceof KeyboardEvent &&
+      event.type === 'keydown' &&
+      !(event.ctrlKey || event.altKey || event.metaKey || event.shiftKey) &&
+      (event.key === 'Enter' || event.key === ' ')
+    )
+  }
+
+  #isMouseActivation(event: Event): boolean {
+    return event instanceof MouseEvent && event.type === 'click'
+  }
+
+  #isActivation(event: Event): boolean {
+    return this.#isMouseActivation(event) || this.#isKeyboardActivation(event)
+  }
+
   handleEvent(event: Event) {
-    const activation = this.#isActivationKeydown(event)
-    if (event.target === this.invokerElement && activation) {
-      if (this.#firstItem) {
-        event.preventDefault()
-        this.popoverElement?.showPopover()
-        this.#firstItem.focus()
-        return
-      }
+    const targetIsInvoker = this.invokerElement?.contains(event.target as HTMLElement)
+    const eventIsActivation = this.#isActivation(event)
+
+    if (targetIsInvoker && event.type === 'mousedown') {
+      this.#invokerBeingClicked = true
+      return
+    }
+
+    // Prevent safari bug that dismisses menu on mousedown instead of allowing
+    // the click event to propagate to the button
+    if (event.type === 'mousedown') {
+      event.preventDefault()
+      return
+    }
+
+    if (targetIsInvoker && eventIsActivation) {
+      this.#handleInvokerActivated(event)
+      this.#invokerBeingClicked = false
+      return
     }
 
     // Ignore events within dialogs within menus
     if ((event.target as Element)?.closest('dialog') || (event.target as Element)?.closest('modal-dialog')) {
       return
     }
 
-    // If a dialog has been rendered within the menu, we do not want to hide
-    // the entire menu, as that will also hide the Dialog. Instead we want to
-    // show the Dialog while hiding just the visible part of the menu.
-    if ((activation || event.type === 'click') && (event.target as HTMLElement)?.closest('[data-show-dialog-id]')) {
-      const dialogInvoker = (event.target as HTMLElement)!.closest('[data-show-dialog-id]')
-      const dialog = this.ownerDocument.getElementById(dialogInvoker?.getAttribute('data-show-dialog-id') || '')
-      if (dialogInvoker && dialog && this.contains(dialogInvoker) && this.contains(dialog)) {
-        this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = 'none'
-        const dialog_controller = new AbortController()
-        const {signal} = dialog_controller
-        const handleDialogClose = () => {
-          dialog_controller.abort()
-          this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = ''
-          if (this.popoverElement?.matches(':popover-open')) {
-            this.popoverElement?.hidePopover()
-          }
+    if (event.type === 'focusout') {
+      if (this.#invokerBeingClicked) return
+
+      // Give the browser time to focus the next element
+      requestAnimationFrame(() => {
+        if (!this.contains(document.activeElement) || document.activeElement === this.invokerElement) {
+          this.#handleFocusOut()
         }
-        dialog.addEventListener('close', handleDialogClose, {signal})
-        dialog.addEventListener('cancel', handleDialogClose, {signal})
-        return
-      }
+      })
+
+      return
     }
 
-    if (!this.popoverElement?.matches(':popover-open')) return
+    const item = (event.target as Element).closest(menuItemSelectors.join(','))
+    const targetIsItem = item !== null
 
-    if (event.type === 'include-fragment-replaced') {
-      if (this.#firstItem) this.#firstItem.focus()
-    } else if (activation || (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(() => {
-          if (this.popoverElement?.matches(':popover-open')) {
-            this.popoverElement?.hidePopover()
-          }
-        })
+    if (targetIsItem && eventIsActivation) {
+      const dialogInvoker = item.closest('[data-show-dialog-id]')
+
+      if (dialogInvoker) {
+        const dialog = this.ownerDocument.getElementById(dialogInvoker.getAttribute('data-show-dialog-id') || '')
+
+        if (dialog && this.contains(dialogInvoker) && this.contains(dialog)) {
+          this.#handleDialogItemActivated(event, dialog)
+          return
+        }
       }
 
-      // 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
+      this.#activateItem(event, item)
+      this.#handleItemActivated(event, item)
+      return
+    }
 
-      const item = (event.target as Element).closest(menuItemSelectors.join(','))
-      if (!item) return
-      const ariaChecked = item.getAttribute('aria-checked')
-      const checked = ariaChecked !== 'true'
+    if (event.type === 'include-fragment-replaced') {
+      this.#handleIncludeFragmentReplaced()
+    }
+  }
 
-      if (this.selectVariant === 'single') {
-        // Only check, never uncheck here. Single-select mode does not allow unchecking a checked item.
-        if (checked) {
-          item.setAttribute('aria-checked', 'true')
-        }
+  #handleInvokerActivated(event: Event) {
+    event.preventDefault()
+    event.stopPropagation()
 
-        for (const checkedItem of this.querySelectorAll('[aria-checked]')) {
-          if (checkedItem !== item) {
-            checkedItem.setAttribute('aria-checked', 'false')
-          }
-        }
+    if (this.#isOpen()) {
+      this.#hide()
+    } else {
+      this.#show()
+      this.#firstItem?.focus()
+    }
+  }
 
-        this.#setDynamicLabel()
-      } else {
-        // multi-select mode allows unchecking a checked item
-        item.setAttribute('aria-checked', `${checked}`)
+  #handleDialogItemActivated(event: Event, dialog: HTMLElement) {
+    this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = 'none'
+    const dialog_controller = new AbortController()
+    const {signal} = dialog_controller
+    const handleDialogClose = () => {
+      dialog_controller.abort()
+      this.querySelector<HTMLElement>('.ActionListWrap')!.style.display = ''
+      if (this.#isOpen()) {
+        this.#hide()
       }
+    }
+    dialog.addEventListener('close', handleDialogClose, {signal})
+    dialog.addEventListener('cancel', handleDialogClose, {signal})
+  }
+
+  #handleItemActivated(event: Event, item: Element) {
+    // 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(() => {
+        if (this.#isOpen()) {
+          this.#hide()
+        }
+      })
+    }
+
+    // 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
 
-      this.#updateInput()
+    const ariaChecked = item.getAttribute('aria-checked')
+    const checked = ariaChecked !== 'true'
 
-      if (event instanceof KeyboardEvent && event.target instanceof HTMLButtonElement) {
-        // prevent buttons from being clicked twice
-        event.preventDefault()
+    if (this.selectVariant === 'single') {
+      // Only check, never uncheck here. Single-select mode does not allow unchecking a checked item.
+      if (checked) {
+        item.setAttribute('aria-checked', 'true')
+      }
+
+      for (const checkedItem of this.querySelectorAll('[aria-checked]')) {
+        if (checkedItem !== item) {
+          checkedItem.setAttribute('aria-checked', 'false')
+        }
       }
+
+      this.#setDynamicLabel()
+    } else {
+      // multi-select mode allows unchecking a checked item
+      item.setAttribute('aria-checked', `${checked}`)
+    }
+
+    this.#updateInput()
+
+    if (event instanceof KeyboardEvent && event.target instanceof HTMLButtonElement) {
+      // prevent buttons from being clicked twice
+      event.preventDefault()
+      return
     }
   }
 
+  #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
+    event.stopPropagation()
+    const elem = item as HTMLElement
+    elem.click()
+  }
+
+  #handleIncludeFragmentReplaced() {
+    if (this.#firstItem) this.#firstItem.focus()
+    this.#softDisableItems()
+  }
+
+  // Close when focus leaves menu
+  #handleFocusOut() {
+    this.#hide()
+  }
+
+  #show() {
+    this.popoverElement?.showPopover()
+  }
+
+  #hide() {
+    this.popoverElement?.hidePopover()
+  }
+
+  #isOpen() {
+    return this.popoverElement?.matches(':popover-open')
+  }
+
   #setDynamicLabel() {
     if (!this.dynamicLabel) return
     const invokerLabel = this.invokerLabel
@@ -261,18 +391,13 @@ export class ActionMenuElement extends HTMLElement {
     }
   }
 
-  #isActivationKeydown(event: Event): boolean {
-    return (
-      event instanceof KeyboardEvent &&
-      event.type === 'keydown' &&
-      !(event.ctrlKey || event.altKey || event.metaKey || event.shiftKey) &&
-      (event.key === 'Enter' || event.key === ' ')
-    )
-  }
-
   get #firstItem(): HTMLElement | null {
     return this.querySelector(menuItemSelectors.join(','))
   }
+
+  get #items(): HTMLElement[] {
+    return Array.from(this.querySelectorAll(menuItemSelectors.join(',')))
+  }
 }
 
 if (!window.customElements.get('action-menu')) {

app/components/primer/alpha/action_menu/list.rb

@@ -111,8 +111,6 @@ def organize_arguments(data: {}, **system_arguments)
               system_arguments,
               { aria: { disabled: true } }
             )
-
-            content_arguments[:disabled] = "" if content_arguments[:tag] == :button
           end
 
           { data: data, **system_arguments, content_arguments: content_arguments }

package.json

@@ -45,7 +45,7 @@
     "@github/auto-check-element": "^5.2.0",
     "@github/auto-complete-element": "^3.3.4",
     "@github/catalyst": "^1.6.0",
-    "@github/clipboard-copy-element": "^1.1.2",
+    "@github/clipboard-copy-element": "^1.3.0",
     "@github/details-menu-element": "^1.0.12",
     "@github/image-crop-element": "^5.0.0",
     "@github/include-fragment-element": "^6.1.1",

previews/primer/alpha/action_menu_preview.rb

@@ -216,7 +216,10 @@ def with_deferred_preloaded_content
 
       # @label With actions
       #
-      def with_actions; end
+      # @param disable_items toggle
+      def with_actions(disable_items: false)
+        render_with_template(locals: { disable_items: disable_items })
+      end
 
       # @label Single select form
       #

previews/primer/alpha/action_menu_preview/with_actions.html.erb

@@ -8,14 +8,15 @@
 
 <%= 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: "Alert", tag: :button, id: "alert-item", disabled: disable_items) %>
+  <% component.with_item(label: "Navigate", tag: :a, content_arguments: { href: action_menu_landing_path }, disabled: disable_items) %>
+  <% component.with_item(label: "Copy text", tag: :"clipboard-copy", content_arguments: { value: "Text to copy" }, disabled: disable_items) %>
   <% component.with_item(
     label: "Submit form",
     href: action_menu_form_action_path,
     form_arguments: {
       name: "foo", value: "bar", method: :post
-    }
+    },
+    disabled: disable_items
   ) %>
 <% end %>

test/components/alpha/action_menu_test.rb

@@ -115,7 +115,7 @@ def test_disabled
         render_preview(:with_disabled_items)
 
         assert_selector("li[aria-disabled=true]") do
-          assert_selector("button.ActionListContent[aria-disabled=true][disabled=disabled]", text: "Does something")
+          assert_selector("button.ActionListContent[aria-disabled=true]", text: "Does something")
           assert_selector("a.ActionListContent[aria-disabled=true]", text: "Site")
         end
       end