Add overflow menu (ActionMenu) to focus zone in ActionBar (#2259)

.changeset/weak-mails-teach.md

@@ -0,0 +1,9 @@
+---
+'@primer/view-components': minor
+---
+
+* Includes ActionMenu in ActionBar focus trap when present.
+* Adjusts `focus_group.ts` to set `tabindex="0"` back to invoker if it is non-focusable.
+* Prevents popover invokers from being triggered with 'left' and 'right' arrow keys.
+
+<!-- Changed components: Primer::Alpha::ActionBar, Primer::Alpha::ActionMenu -->

app/components/primer/alpha/action_bar_element.ts

@@ -82,11 +82,11 @@ class ActionBarElement extends HTMLElement {
     if (this.#focusZoneAbortController) {
       this.#focusZoneAbortController.abort()
     }
-    this.#focusZoneAbortController = focusZone(this.itemContainer, {
+    this.#focusZoneAbortController = focusZone(this, {
       bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd,
       focusOutBehavior: 'wrap',
       focusableElementFilter: element => {
-        return !element.closest('.ActionBar-item[hidden]')
+        return !element.closest('.ActionBar-item[hidden]') && !element.closest('li.ActionListItem')
       }
     })
   }

app/components/primer/focus_group.ts

@@ -8,6 +8,8 @@ const getMnemonicFor = (item: Element) => item.textContent?.trim()[0].toLowerCas
 const printable = /^\S$/
 
 export default class FocusGroupElement extends HTMLElement {
+  #retainSignal: AbortController | null = null
+
   get nowrap(): boolean {
     return this.hasAttribute('nowrap')
   }
@@ -60,8 +62,32 @@ export default class FocusGroupElement extends HTMLElement {
     const {direction, nowrap} = this
     if (event.type === 'focusin') {
       if (this.retain && event.target instanceof Element && event.target.matches(menuItemSelector)) {
+        this.#retainSignal?.abort()
+        const {signal} = (this.#retainSignal = new AbortController())
         for (const item of this.#items) {
           item.setAttribute('tabindex', item === event.target ? '0' : '-1')
+          const popover = event.target.closest<HTMLElement>('[popover]')
+          if (item === event.target && popover?.popover === 'auto' && popover.closest('focus-group') === this) {
+            popover.addEventListener(
+              'toggle',
+              (toggleEvent: Event) => {
+                if (!(toggleEvent.target instanceof Element)) return
+                if ((toggleEvent as ToggleEvent).newState === 'closed') {
+                  this.#retainSignal?.abort()
+                  item.setAttribute('tabindex', '-1')
+                  if (popover.id) {
+                    const invoker = this.querySelector(`[popovertarget="${popover.id}"]`)
+                    if (invoker) {
+                      invoker.setAttribute('tabindex', '0')
+                    } else {
+                      this.#items[0]?.setAttribute('tabindex', '0')
+                    }
+                  }
+                }
+              },
+              {signal}
+            )
+          }
         }
       }
     } else if (event instanceof KeyboardEvent) {
@@ -111,7 +137,7 @@ export default class FocusGroupElement extends HTMLElement {
         let el: HTMLElement | null = focusEl
         do {
           el = el.closest(`[popover]:not(:popover-open)`)
-          if (el?.popover === 'auto') {
+          if (el?.popover === 'auto' && !['ArrowRight', 'ArrowLeft'].includes(event.key)) {
             el.showPopover()
           }
           el = el?.parentElement || null

test/system/alpha/action_bar_test.rb

@@ -18,4 +18,67 @@ def test_resizing_hides_items
       assert_selector("[data-target=\"action-bar.moreMenu\"]")
     end
   end
+
+  def test_focus_set_on_first_item
+    visit_preview(:default)
+    page.driver.browser.keyboard.type(:tab)
+
+    assert_selector("action-bar") do
+      # focus should be set on the first item
+      assert_equal page.evaluate_script("document.activeElement.classList.contains('Button--iconOnly')"), true
+    end
+  end
+
+  def test_focus_set_within_overflow_menu
+    visit_preview(:default)
+
+    page.driver.browser.resize(width: 145, height: 350)
+    assert_selector("[data-targets=\"action-bar.items\"]", count: 2)
+
+    page.driver.browser.keyboard.type(:tab, :left)
+
+    assert_equal page.evaluate_script("document.activeElement.classList.contains('Button--iconOnly')"), true
+
+    # We want to ensure that we're within the ActionMenu assert_selector("action-bar") do
+    page.driver.browser.keyboard.type(:enter)
+    assert_equal page.evaluate_script("document.activeElement.classList.contains('ActionListContent')"), true
+  end
+
+  def test_escape_in_overflow_menu_sets_focus_back
+    visit_preview(:default)
+
+    page.driver.browser.resize(width: 145, height: 350)
+    assert_selector("[data-targets=\"action-bar.items\"]", count: 2)
+
+    page.driver.browser.keyboard.type(:tab, :left)
+
+    assert_equal page.evaluate_script("!!document.activeElement.closest('action-menu')"), true
+
+    page.driver.browser.keyboard.down(:enter)
+    assert_equal page.evaluate_script("document.activeElement.classList.contains('ActionListContent')"), true
+
+    page.driver.browser.keyboard.down(:escape)
+
+    assert_equal page.evaluate_script("document.activeElement.classList.contains('Button--iconOnly')"), true
+    assert_equal page.evaluate_script("!!document.activeElement.closest('action-menu')"), true
+  end
+
+  def test_click_outside_of_menu_sets_tabindex_back
+    visit_preview(:default)
+
+    page.driver.browser.resize(width: 145, height: 350)
+    assert_selector("[data-targets=\"action-bar.items\"]", count: 2)
+
+    page.driver.browser.keyboard.type(:tab, :left)
+
+    page.driver.browser.keyboard.down(:enter)
+    assert_equal page.evaluate_script("document.activeElement.classList.contains('ActionListContent')"), true
+
+    page.driver.browser.mouse.click(x: 0, y: 0)
+    page.driver.browser.keyboard.type(:tab)
+
+    # Ensures that ActionMenu trigger is still focusable
+    assert_equal page.evaluate_script("document.activeElement.classList.contains('Button--iconOnly')"), true
+    assert_equal page.evaluate_script("!!document.activeElement.closest('action-menu')"), true
+  end
 end