ActionMenu: Replace typeahead with mnemonics

.changeset/gold-falcons-shake.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+ActionMenu: Replace typeahead behavior with single key mnemonics

src/ActionMenu.tsx

@@ -3,7 +3,7 @@ import {useSSRSafeId} from '@react-aria/ssr'
 import {TriangleDownIcon} from '@primer/octicons-react'
 import {AnchoredOverlay, AnchoredOverlayProps} from './AnchoredOverlay'
 import {OverlayProps} from './Overlay'
-import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuInitialFocus, useTypeaheadFocus} from './hooks'
+import {useProvidedRefOrCreate, useProvidedStateOrCreate, useMenuInitialFocus, useMnemonics} from './hooks'
 import {Divider} from './ActionList/Divider'
 import {ActionListContainerContext} from './ActionList/ActionListContainerContext'
 import {Button, ButtonProps} from './Button'
@@ -112,7 +112,7 @@ const Overlay: React.FC<MenuOverlayProps> = ({children, align = 'start', ...over
 
   const containerRef = React.createRef<HTMLDivElement>()
   const {openWithFocus} = useMenuInitialFocus(open, onOpen, containerRef)
-  useTypeaheadFocus(open, containerRef)
+  useMnemonics(open, containerRef)
 
   return (
     <AnchoredOverlay

src/__tests__/hooks/useMenuTypeaheadFocus.test.tsx → src/__tests__/hooks/useMnemonics.test.tsx

@@ -1,9 +1,8 @@
 import React from 'react'
 import {render, fireEvent, cleanup} from '@testing-library/react'
-import {useTypeaheadFocus} from '../../hooks'
-import {TYPEAHEAD_TIMEOUT} from '../../hooks/useTypeaheadFocus'
+import {useMnemonics} from '../../hooks'
 
-const Component = ({
+const Fixture = ({
   onSelect = () => null,
   hasInput = false,
   refNotAttached = false
@@ -13,7 +12,7 @@ const Component = ({
   refNotAttached?: boolean
 }) => {
   const containerRef = React.createRef<HTMLDivElement>()
-  useTypeaheadFocus(true, containerRef) // hard coding open=true for test
+  useMnemonics(true, containerRef) // hard coding open=true for test
 
   return (
     <>
@@ -22,9 +21,12 @@ const Component = ({
         {hasInput && <input autoFocus type="text" placeholder="Filter options" />}
         <button onKeyDown={onSelect}>button 1</button>
         <button onKeyDown={onSelect}>Button 2</button>
-        <button disabled>button 3 is disabled</button>
-        <button onKeyDown={onSelect}>button 4</button>
         <button onKeyDown={onSelect}>third button</button>
+        <button disabled>fourth button is disabled</button>
+        <button onKeyDown={onSelect}>button 5</button>
+        <button onKeyDown={onSelect} aria-keyshortcuts="6 E">
+          button 6
+        </button>
         <span>not focusable</span>
       </div>
     </>
@@ -35,31 +37,31 @@ describe('useTypeaheadFocus', () => {
   afterEach(cleanup)
 
   it('First element: when b is pressed, it should move focus the "b"utton 1', () => {
-    const {getByTestId, getByText} = render(<Component />)
+    const {getByTestId, getByText} = render(<Fixture />)
     const container = getByTestId('container')
 
     fireEvent.keyDown(container, {key: 'b', code: 'b'})
     expect(getByText('button 1')).toEqual(document.activeElement)
   })
 
   it('Not first element: when t is pressed, it should move focus the "t"hird button', () => {
-    const {getByTestId, getByText} = render(<Component />)
+    const {getByTestId, getByText} = render(<Fixture />)
     const container = getByTestId('container')
 
     fireEvent.keyDown(container, {key: 't', code: 't'})
     expect(getByText('third button')).toEqual(document.activeElement)
   })
 
   it('Case insensitive: when B is pressed, it should move focus the "b"utton 1', () => {
-    const {getByTestId, getByText} = render(<Component />)
+    const {getByTestId, getByText} = render(<Fixture />)
     const container = getByTestId('container')
 
     fireEvent.keyDown(container, {key: 'B', code: 'B'})
     expect(getByText('button 1')).toEqual(document.activeElement)
   })
 
   it('Repeating letter: when b is pressed repeatedly, it should wrap through the buttons starting with "b", skipping the disabled button', () => {
-    const {getByTestId, getByText} = render(<Component />)
+    const {getByTestId, getByText} = render(<Fixture />)
     const container = getByTestId('container')
 
     fireEvent.keyDown(container, {key: 'b', code: 'b'})
@@ -69,65 +71,61 @@ describe('useTypeaheadFocus', () => {
     expect(getByText('Button 2')).toEqual(document.activeElement)
 
     fireEvent.keyDown(container, {key: 'b', code: 'b'})
-    expect(getByText('button 4')).toEqual(document.activeElement)
+    expect(getByText('button 5')).toEqual(document.activeElement)
 
+    // should cycle back to start of the list
     fireEvent.keyDown(container, {key: 'b', code: 'b'})
     expect(getByText('button 1')).toEqual(document.activeElement)
   })
 
-  it('Timeout: when presses b, waits and presses t, it should move focus the "t"hird button', async () => {
-    jest.useFakeTimers()
-    const {getByTestId, getByText} = render(<Component />)
+  it('User defined aria-keyshortcuts: Should catch all shortcuts defined by user', () => {
+    const {getByTestId, getByText} = render(<Fixture />)
     const container = getByTestId('container')
 
-    fireEvent.keyDown(container, {key: 'b', code: 'b'})
-    expect(getByText('button 1')).toEqual(document.activeElement)
-
-    // if we press t now, the query would be "bt",
-    // and focus will stay wherever it is
-    fireEvent.keyDown(container, {key: 't', code: 't'})
-    expect(getByText('button 1')).toEqual(document.activeElement)
+    fireEvent.keyDown(container, {key: '6', code: '6'})
+    expect(getByText('button 6')).toEqual(document.activeElement)
 
-    // but, if we simulate typeahead timeout, then type t
-    // it should jump to the third button
-    jest.advanceTimersByTime(TYPEAHEAD_TIMEOUT)
+    // send focus elsewhere
     fireEvent.keyDown(container, {key: 't', code: 't'})
     expect(getByText('third button')).toEqual(document.activeElement)
+
+    fireEvent.keyDown(container, {key: 'e', code: 'e'})
+    expect(getByText('button 6')).toEqual(document.activeElement)
   })
 
-  it('Space: when user is typing and presses Space, it should not select the option', () => {
-    const mockFunction = jest.fn()
-    const {getByTestId, getByText} = render(<Component onSelect={mockFunction} />)
+  it('aria-keyshortcuts: it should add aria-keyshortcuts to focusable items', () => {
+    const {getByText} = render(<Fixture />)
 
-    const container = getByTestId('container')
-    fireEvent.keyDown(container, {key: 't', code: 't'})
+    expect(getByText('button 1')).toHaveAttribute('aria-keyshortcuts', 'b')
+    expect(getByText('Button 2')).toHaveAttribute('aria-keyshortcuts', 'b')
+    expect(getByText('third button')).toHaveAttribute('aria-keyshortcuts', 't')
+    expect(getByText('button 5')).toHaveAttribute('aria-keyshortcuts', 'b')
 
-    const thirdButton = getByText('third button')
-    expect(thirdButton).toEqual(document.activeElement)
-    fireEvent.keyDown(thirdButton, {key: ' ', code: 'Space'})
-    expect(mockFunction).not.toHaveBeenCalled()
+    // don't overwrite aria-keyshortcuts if it's already defined
+    expect(getByText('button 6')).toHaveAttribute('aria-keyshortcuts', '6 E')
+
+    // not focusable items should not have aria-keyshortcuts
+    expect(getByText('fourth button is disabled')).not.toHaveAttribute('aria-keyshortcuts')
+    expect(getByText('not focusable')).not.toHaveAttribute('aria-keyshortcuts')
   })
 
-  it('Space after timeout: when user is presses Space after waiting, it should select the option', () => {
-    jest.useFakeTimers()
+  it('Space: when user presses Space, it should select the option', () => {
     const mockFunction = jest.fn()
-    const {getByTestId, getByText} = render(<Component onSelect={mockFunction} />)
+    const {getByTestId, getByText} = render(<Fixture onSelect={mockFunction} />)
 
     const container = getByTestId('container')
     fireEvent.keyDown(container, {key: 't', code: 't'})
 
     const thirdButton = getByText('third button')
     expect(thirdButton).toEqual(document.activeElement)
-
-    jest.advanceTimersByTime(TYPEAHEAD_TIMEOUT)
     fireEvent.keyDown(thirdButton, {key: ' ', code: 'Space'})
     expect(mockFunction).toHaveBeenCalled()
   })
 
-  it('Enter: when user is presses Enter, it should instantly select the option', () => {
+  it('Enter: when user is presses Enter, it should select the option', () => {
     jest.useFakeTimers()
     const mockFunction = jest.fn()
-    const {getByTestId, getByText} = render(<Component onSelect={mockFunction} />)
+    const {getByTestId, getByText} = render(<Fixture onSelect={mockFunction} />)
 
     const container = getByTestId('container')
     fireEvent.keyDown(container, {key: 't', code: 't'})
@@ -139,44 +137,16 @@ describe('useTypeaheadFocus', () => {
     expect(mockFunction).toHaveBeenCalled()
   })
 
-  it('Long string: when user starts typing a longer string "button 4", focus should jump to closest match', async () => {
-    jest.useFakeTimers()
-    const mockFunction = jest.fn()
-    const {getByTestId, getByText} = render(<Component onSelect={mockFunction} />)
-    const container = getByTestId('container')
-
-    fireEvent.keyDown(container, {key: 'b', code: 'b'})
-    expect(getByText('button 1')).toEqual(document.activeElement)
-
-    fireEvent.keyDown(container, {key: 'u', code: 'u'})
-    expect(getByText('button 1')).toEqual(document.activeElement)
-
-    fireEvent.keyDown(container, {key: 't', code: 't'})
-    fireEvent.keyDown(container, {key: 't', code: 't'})
-    fireEvent.keyDown(container, {key: 'o', code: 'o'})
-    fireEvent.keyDown(container, {key: 'n', code: 'n'})
-
-    // pressing Space should be treated as part of query
-    // and shouldn't select the option
-    fireEvent.keyDown(container, {key: ' ', code: 'Space'})
-    expect(mockFunction).not.toHaveBeenCalled()
-    expect(getByText('button 1')).toEqual(document.activeElement)
-
-    fireEvent.keyDown(container, {key: '4', code: '4'})
-    // the query is now "button 4" and should move focus
-    expect(getByText('button 4')).toEqual(document.activeElement)
-  })
-
   it('Shortcuts: when a modifier is used, typeahead should not do anything', () => {
-    const {getByTestId, getByText} = render(<Component />)
+    const {getByTestId, getByText} = render(<Fixture />)
     const container = getByTestId('container')
 
     fireEvent.keyDown(container, {metaKey: true, key: 'b', code: 'b'})
     expect(getByText('button 1')).not.toEqual(document.activeElement)
   })
 
   it('TextInput: when an textinput has focus, typeahead should not do anything', async () => {
-    const {getByTestId, getByPlaceholderText} = render(<Component hasInput={true} />)
+    const {getByTestId, getByPlaceholderText} = render(<Fixture hasInput={true} />)
     const container = getByTestId('container')
 
     const input = getByPlaceholderText('Filter options')
@@ -187,7 +157,7 @@ describe('useTypeaheadFocus', () => {
   })
 
   it('Missing ref: when a ref is not attached, typeahead should break the component', async () => {
-    const {getByTestId, getByText} = render(<Component refNotAttached={true} />)
+    const {getByTestId, getByText} = render(<Fixture refNotAttached={true} />)
     const container = getByTestId('container')
 
     fireEvent.keyDown(container, {key: 'b', code: 'b'})

src/hooks/index.ts

@@ -11,4 +11,4 @@ export type {UseOverlaySettings} from './useOverlay'
 export {useRenderForcingRef} from './useRenderForcingRef'
 export {useProvidedStateOrCreate} from './useProvidedStateOrCreate'
 export {useMenuInitialFocus} from './useMenuInitialFocus'
-export {useTypeaheadFocus} from './useTypeaheadFocus'
+export {useMnemonics} from './useMnemonics'

src/hooks/useMnemonics.ts

@@ -0,0 +1,90 @@
+import React from 'react'
+import {iterateFocusableElements} from '@primer/behaviors/utils'
+import {useProvidedRefOrCreate} from './useProvidedRefOrCreate'
+
+/*
+ * A mnemonic indicates to the user which key to press (single)
+ * to activate a command or navigate to a component
+ * typically appearing in a menu title, menu item, or the text of a button.
+ */
+
+export const useMnemonics = (open: boolean, providedRef?: React.RefObject<HTMLElement>) => {
+  const containerRef = useProvidedRefOrCreate(providedRef)
+
+  React.useEffect(
+    function addAriaKeyshortcuts() {
+      if (!open || !containerRef.current) return
+      const container = containerRef.current
+
+      const focusableItems = [...iterateFocusableElements(container)]
+
+      focusableItems.map(item => {
+        // if item already has aria-keyshortcuts defined by user, skip
+        if (item.getAttribute('aria-keyshortcuts')) return
+
+        const firstLetter = item.textContent?.toLowerCase()[0]
+        if (firstLetter) item.setAttribute('aria-keyshortcuts', firstLetter)
+      })
+    },
+    [open, containerRef]
+  )
+
+  React.useEffect(
+    function handleKeyDown() {
+      if (!open || !containerRef.current) return
+      const container = containerRef.current
+
+      const handler = (event: KeyboardEvent) => {
+        // skip if a TextInput has focus
+        const activeElement = document.activeElement as HTMLElement
+        if (activeElement.tagName === 'INPUT') return
+
+        // skip if used with modifier to preserve shortcuts like ⌘ + F
+        const hasModifier = event.ctrlKey || event.altKey || event.metaKey
+        if (hasModifier) return
+
+        // skip if it's not a alphabet key
+        if (!isAlphabetKey(event)) return
+
+        // if this is a typeahead event, don't propagate outside of menu
+        event.stopPropagation()
+
+        const query = event.key.toLowerCase()
+
+        let elementToFocus: HTMLElement | undefined
+
+        const focusableItems = [...iterateFocusableElements(container)]
+
+        const itemsMatchingKey = focusableItems.filter(item => {
+          const keyshortcuts = item
+            .getAttribute('aria-keyshortcuts')
+            ?.split(' ')
+            .map(shortcut => shortcut.toLowerCase())
+          return keyshortcuts && keyshortcuts.includes(query)
+        })
+
+        const currentActiveIndex = itemsMatchingKey.indexOf(activeElement)
+
+        // If the last element is already selected, cycle through the list
+        if (currentActiveIndex === itemsMatchingKey.length - 1) {
+          elementToFocus = itemsMatchingKey[0]
+        } else {
+          elementToFocus = itemsMatchingKey.find((item, index) => {
+            return index > currentActiveIndex
+          })
+        }
+        elementToFocus?.focus()
+      }
+
+      container.addEventListener('keydown', handler)
+      return () => container.removeEventListener('keydown', handler)
+    },
+    [open, containerRef]
+  )
+
+  const isAlphabetKey = (event: KeyboardEvent) => {
+    return event.key.length === 1 && /[a-z\d]/i.test(event.key)
+  }
+
+  return {containerRef}
+}