Address a11y issues with ActionList, NavList, and ActionMenu

.changeset/chatty-owls-shave.md

@@ -0,0 +1,5 @@
+---
+'@primer/view-components': patch
+---
+
+Address NavList Axe violation

.gitignore

@@ -19,6 +19,7 @@ demo/app/assets/stylesheets/prim*
 /.yardoc
 /doc
 previews/docs/
+test/snapshots/
 test/snapshots/docs/
 demo/test/snapshots/
 previews/pages/forms/inputs/*.md.erb

app/components/primer/alpha/action_list.rb

@@ -107,7 +107,7 @@ def initialize(
           "ActionListWrap--divided" => @show_dividers
         )
 
-        @role = role || allows_selection? ? MENU_ROLE : DEFAULT_ROLE
+        @role = role || (allows_selection? ? MENU_ROLE : DEFAULT_ROLE)
         @system_arguments[:role] = @role
 
         @list_wrapper_arguments = {}

app/components/primer/alpha/action_list/item.rb

@@ -191,8 +191,6 @@ def initialize(
             "ActionListItem--disabled" => @disabled
           )
 
-          @system_arguments[:role] = :none
-
           @system_arguments[:data] ||= {}
           @system_arguments[:data][:targets] = "#{list_class.custom_element_name}.items"
 
@@ -237,6 +235,8 @@ def initialize(
                                         ActionList::DEFAULT_MENU_ITEM_ROLE
                                       end
 
+          @system_arguments[:role] = @list.acts_as_menu? ? :none : nil
+
           @description_wrapper_arguments = {
             classes: class_names(
               "ActionListItem-descriptionWrap",

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

@@ -11,7 +11,7 @@ const popoverSelector = (() => {
 
 type SelectVariant = 'single' | 'multiple' | null
 
-const menuItemSelectors = ['[role="menuitem"]', '[role="menuitemcheckbox"]', '[role="menuitemradio"]', '[role="none"]']
+const menuItemSelectors = ['[role="menuitem"]', '[role="menuitemcheckbox"]', '[role="menuitemradio"]']
 
 export class ActionMenuElement extends HTMLElement {
   #abortController: AbortController
@@ -74,17 +74,20 @@ export class ActionMenuElement extends HTMLElement {
   }
 
   handleEvent(event: Event) {
+    if (event.target === this.invokerElement && this.#isEnterKeydown(event)) {
+      if (this.#firstItem) {
+        event.preventDefault()
+        this.popoverElement?.showPopover()
+        this.#firstItem.focus()
+        return
+      }
+    }
+
     if (!this.popoverElement?.matches(popoverSelector)) return
 
     if (event.type === 'focusout' && !this.contains((event as FocusEvent).relatedTarget as Node)) {
       this.popoverElement?.hidePopover()
-    } else if (
-      (event instanceof KeyboardEvent &&
-        event.type === 'keydown' &&
-        !(event.ctrlKey || event.altKey || event.metaKey || event.shiftKey) &&
-        event.key === 'Enter') ||
-      (event instanceof MouseEvent && event.type === 'click')
-    ) {
+    } else if (this.#isEnterKeydown(event) || (event instanceof MouseEvent && event.type === 'click')) {
       const item = (event.target as Element).closest(menuItemSelectors.join(','))?.closest('li')
       if (!item) return
       const ariaChecked = item.getAttribute('aria-checked')
@@ -100,17 +103,27 @@ export class ActionMenuElement extends HTMLElement {
         }
         this.#setDynamicLabel()
       }
-      event.preventDefault()
-      this.popoverElement?.hidePopover()
+      if (event instanceof KeyboardEvent && event.target instanceof HTMLButtonElement) {
+        // 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())
+      }
     }
   }
 
   #setDynamicLabel() {
     if (!this.dynamicLabel) return
     const invoker = this.invokerElement
     if (!invoker) return
-    const selector = menuItemSelectors.map(s => `${s}[aria-checked=true]`).join(',')
-    const item = this.querySelector(selector)
+    const item = this.querySelector('[aria-checked=true]')
     if (item && this.dynamicLabel) {
       this.#originalLabel ||= invoker.textContent || ''
       const prefixSpan = document.createElement('span')
@@ -123,6 +136,19 @@ export class ActionMenuElement extends HTMLElement {
       invoker.textContent = this.#originalLabel
     }
   }
+
+  #isEnterKeydown(event: Event): boolean {
+    return (
+      event instanceof KeyboardEvent &&
+      event.type === 'keydown' &&
+      !(event.ctrlKey || event.altKey || event.metaKey || event.shiftKey) &&
+      event.key === 'Enter'
+    )
+  }
+
+  get #firstItem(): HTMLElement | null {
+    return this.querySelector(menuItemSelectors.join(','))
+  }
 }
 
 if (!window.customElements.get('action-menu')) {

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

@@ -7,9 +7,8 @@ class ActionMenu
       # This component is part of <%= link_to_component(Primer::Alpha::ActionMenu) %> and should not be
       # used as a standalone component.
       class List < Primer::Alpha::ActionList
-        DEFAULT_ITEM_TAG = :span
+        DEFAULT_ITEM_TAG = :button
         ITEM_TAG_OPTIONS = [:a, :button, :"clipboard-copy", DEFAULT_ITEM_TAG].freeze
-        ITEM_ACTION_OPTIONS = [:classes, :onclick, :href, :value].freeze
 
         # Adds a new item to the list.
         #
@@ -31,11 +30,11 @@ def with_item(**system_arguments, &block)
 
           # rubocop:disable Style/IfUnlessModifier
           if content_arguments[:tag] == :a
-            content_arguments[:href] = system_arguments.delete(:href)
+            content_arguments[:href] ||= system_arguments.delete(:href)
           end
           # rubocop:enable Style/IfUnlessModifier
 
-          system_arguments[:tabindex] = -1
+          content_arguments[:tabindex] = -1
           system_arguments[:autofocus] = "" if system_arguments[:autofocus]
 
           if system_arguments[:disabled]

app/components/primer/alpha/dropdown/menu.rb

@@ -70,7 +70,7 @@ def initialize(as:, tag: TAG_DEFAULT, divider: false, **system_arguments)
             @system_arguments[:tag] = fetch_or_fallback(TAG_OPTIONS, tag, TAG_DEFAULT)
             @system_arguments[:tag] = :li if list? && divider?
             @system_arguments[:role] ||= :menuitem
-            @system_arguments[:role] = :separator if divider
+            @system_arguments[:role] = :presentation if divider
             @system_arguments[:classes] = class_names(
               @system_arguments[:classes],
               "dropdown-item" => !divider,

app/components/primer/alpha/modal_dialog.ts

@@ -11,7 +11,14 @@ const overlayStack: ModalDialogElement[] = []
 
 function clickHandler(event: Event) {
   const target = event.target as HTMLElement
-  const button = target?.closest('button')
+  let button: HTMLButtonElement | null = null
+
+  if (target instanceof HTMLButtonElement) {
+    button = target
+  } else {
+    button = target?.querySelector('button')
+  }
+
   if (!button) return
 
   // If the user is clicking a valid dialog trigger
@@ -42,6 +49,21 @@ function clickHandler(event: Event) {
   }
 }
 
+function keydownHandler(event: Event) {
+  if (
+    !(event instanceof KeyboardEvent) ||
+    event.type !== 'keydown' ||
+    event.key !== 'Enter' ||
+    event.ctrlKey ||
+    event.altKey ||
+    event.metaKey ||
+    event.shiftKey
+  )
+    return
+
+  clickHandler(event)
+}
+
 function mousedownHandler(event: Event) {
   const target = event.target as HTMLElement
   if (target?.closest('button')) return
@@ -124,6 +146,7 @@ export class ModalDialogElement extends HTMLElement {
     if (!this.hasAttribute('role')) this.setAttribute('role', 'dialog')
 
     document.addEventListener('click', clickHandler)
+    document.addEventListener('keydown', keydownHandler)
     document.addEventListener('mousedown', mousedownHandler)
 
     this.addEventListener('keydown', e => this.#keydown(e))

demo/app/controllers/action_menu_controller.rb

@@ -4,6 +4,8 @@
 class ActionMenuController < ApplicationController
   layout false
 
+  def landing; end
+
   def deferred
     render "action_menu/deferred"
   end

demo/app/views/action_menu/landing.html.erb

@@ -0,0 +1,3 @@
+<div class="action-menu-landing-page">
+  Hello world!
+</div>

demo/config/routes.rb

@@ -20,6 +20,7 @@
     post "/example_check/error", to: "auto_check#error", as: :example_check_error
     post "/example_check/random", to: "auto_check#random", as: :example_check_random
 
+    get "/action_menu/landing_page", to: "action_menu#landing", as: :action_menu_landing
     get "/action_menu/deferred", to: "action_menu#deferred", as: :action_menu_deferred
     get "/action_menu/deferred_preload", to: "action_menu#deferred_preload", as: :action_menu_deferred_preload
   end