fix: Restore "fix: Don’t focus first 'Item' of 'DropdownMenu' and 'Se…

…lectMenu' on open" (#1293)

* Revert "Revert "fix: Don’t focus first 'Item' of 'DropdownMenu' and 'SelectMenu' on open (#1270)" (#1291)"

This reverts commit 2a0e8fb.

* fix: Don’t blur to body

* fix: Replace 'AbortController' with 'once: true'

* Create good-grapes-collect.md

.changeset/good-grapes-collect.md

@@ -0,0 +1,5 @@
+---
+"@primer/components": patch
+---
+
+Restore "fix: Don’t focus first 'Item' of 'DropdownMenu' and 'SelectMenu' on open" by deferring the removal of a temporary `tabIndex` until focus moves within the container.

src/AnchoredOverlay/AnchoredOverlay.tsx

@@ -1,6 +1,6 @@
 import React, {useCallback, useMemo} from 'react'
 import Overlay, {OverlayProps} from '../Overlay'
-import {useFocusTrap} from '../hooks/useFocusTrap'
+import {FocusTrapHookSettings, useFocusTrap} from '../hooks/useFocusTrap'
 import {FocusZoneHookSettings, useFocusZone} from '../hooks/useFocusZone'
 import {useAnchoredPosition, useProvidedRefOrCreate, useRenderForcingRef} from '../hooks'
 import {uniqueId} from '../utils/uniqueId'
@@ -53,6 +53,11 @@ interface AnchoredOverlayBaseProps extends Pick<OverlayProps, 'height' | 'width'
    */
   overlayProps?: Partial<OverlayProps>
 
+  /**
+   * Settings to apply to the Focus Zone on the internal `Overlay` component.
+   */
+  focusTrapSettings?: Partial<FocusTrapHookSettings>
+
   /**
    * Settings to apply to the Focus Zone on the internal `Overlay` component.
    */
@@ -76,6 +81,7 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({
   height,
   width,
   overlayProps,
+  focusTrapSettings,
   focusZoneSettings
 }) => {
   const anchorRef = useProvidedRefOrCreate(externalAnchorRef)
@@ -121,7 +127,7 @@ export const AnchoredOverlay: React.FC<AnchoredOverlayProps> = ({
     disabled: !open || !position,
     ...focusZoneSettings
   })
-  useFocusTrap({containerRef: overlayRef, disabled: !open || !position})
+  useFocusTrap({containerRef: overlayRef, disabled: !open || !position, ...focusTrapSettings})
 
   return (
     <>

src/FilteredActionList/FilteredActionList.tsx

@@ -11,6 +11,7 @@ import {itemActiveDescendantClass} from '../ActionList/Item'
 import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate'
 import styled from 'styled-components'
 import {get} from '../constants'
+import {useProvidedRefOrCreate} from '../hooks/useProvidedRefOrCreate'
 import useScrollFlash from '../hooks/useScrollFlash'
 
 export interface FilteredActionListProps extends Partial<Omit<GroupedListProps, keyof ListPropsBase>>, ListPropsBase {
@@ -19,6 +20,7 @@ export interface FilteredActionListProps extends Partial<Omit<GroupedListProps,
   filterValue?: string
   onFilterChange: (value: string, e: React.ChangeEvent<HTMLInputElement>) => void
   textInputProps?: Partial<Omit<TextInputProps, 'onChange'>>
+  inputRef?: React.RefObject<HTMLInputElement>
 }
 
 function scrollIntoViewingArea(
@@ -56,6 +58,7 @@ export function FilteredActionList({
   onFilterChange,
   items,
   textInputProps,
+  inputRef: providedInputRef,
   ...listProps
 }: FilteredActionListProps): JSX.Element {
   const [filterValue, setInternalFilterValue] = useProvidedStateOrCreate(externalFilterValue, undefined, '')
@@ -70,7 +73,7 @@ export function FilteredActionList({
 
   const containerRef = useRef<HTMLInputElement>(null)
   const scrollContainerRef = useRef<HTMLDivElement>(null)
-  const inputRef = useRef<HTMLInputElement>(null)
+  const inputRef = useProvidedRefOrCreate<HTMLInputElement>(providedInputRef)
   const activeDescendantRef = useRef<HTMLElement>()
   const listId = useMemo(uniqueId, [])
   const onInputKeyPress: KeyboardEventHandler = useCallback(

src/Overlay.tsx

@@ -57,6 +57,9 @@ const StyledOverlay = styled.div<StyledOverlayProps & SystemCommonProps & System
     }
   }
   visibility: ${props => props.visibility || 'visible'};
+  :focus {
+    outline: none;
+  }
   ${COMMON};
   ${POSITION};
   ${sx};

src/SelectPanel/SelectPanel.tsx

@@ -127,13 +127,19 @@ export function SelectPanel({
     })
   }, [onClose, onSelectedChange, items, selected])
 
+  const inputRef = React.useRef<HTMLInputElement>(null)
+  const focusTrapSettings = {
+    initialFocusRef: inputRef
+  }
+
   return (
     <AnchoredOverlay
       renderAnchor={renderMenuAnchor}
       open={open}
       onOpen={onOpen}
       onClose={onClose}
       overlayProps={overlayProps}
+      focusTrapSettings={focusTrapSettings}
       focusZoneSettings={focusZoneSettings}
     >
       <Flex flexDirection="column" width="100%" height="100%">
@@ -145,6 +151,7 @@ export function SelectPanel({
           items={itemsToRender}
           selectionVariant={isMultiSelectVariant(selected) ? 'multiple' : 'single'}
           textInputProps={textInputProps}
+          inputRef={inputRef}
         />
       </Flex>
     </AnchoredOverlay>

src/__tests__/behaviors/focusTrap.tsx

@@ -22,7 +22,7 @@ beforeAll(() => {
   }
 })
 
-it('Should initially focus the first focusable element when activated', () => {
+it('Should initially focus the container when activated', () => {
   const {container} = render(
     <div>
       <button tabIndex={0}>Bad Apple</button>
@@ -35,9 +35,8 @@ it('Should initially focus the first focusable element when activated', () => {
   )
 
   const trapContainer = container.querySelector<HTMLElement>('#trapContainer')!
-  const firstButton = trapContainer.querySelector('button')!
   const controller = focusTrap(trapContainer)
-  expect(document.activeElement).toEqual(firstButton)
+  expect(document.activeElement).toEqual(trapContainer)
 
   controller.abort()
 })
@@ -74,13 +73,12 @@ it('Should prevent focus from exiting the trap, returns focus to previously-focu
   )
 
   const trapContainer = container.querySelector<HTMLElement>('#trapContainer')!
-  const firstButton = trapContainer.querySelector('button')!
   const secondButton = trapContainer.querySelectorAll('button')[1]!
   const durianButton = container.querySelector<HTMLElement>('#durian')!
   const controller = focusTrap(trapContainer)
 
   focus(durianButton)
-  expect(document.activeElement).toEqual(firstButton)
+  expect(document.activeElement).toEqual(trapContainer)
 
   focus(secondButton)
   expect(document.activeElement).toEqual(secondButton)
@@ -157,12 +155,11 @@ it('Should should release the trap when the signal is aborted', async () => {
 
   const trapContainer = container.querySelector<HTMLElement>('#trapContainer')!
   const durianButton = container.querySelector<HTMLElement>('#durian')!
-  const firstButton = trapContainer.querySelector('button')!
 
   const controller = focusTrap(trapContainer)
 
   focus(durianButton)
-  expect(document.activeElement).toEqual(firstButton)
+  expect(document.activeElement).toEqual(trapContainer)
 
   controller.abort()
 
@@ -189,7 +186,7 @@ it('Should should release the trap when the container is removed from the DOM',
   focusTrap(trapContainer)
 
   focus(durianButton)
-  expect(document.activeElement).toEqual(firstButton)
+  expect(document.activeElement).toEqual(trapContainer)
 
   // empty trap and remove it from the DOM
   trapContainer.removeChild(firstButton)

src/behaviors/focusTrap.ts

@@ -68,8 +68,7 @@ export function focusTrap(
   // Ensure focus remains in the trap zone by checking that a given recently-focused
   // element is inside the trap zone. If it isn't, redirect focus to a suitable
   // element within the trap zone. If need to redirect focus and a suitable element
-  // is not found, blur the recently-focused element so that focus doesn't leave the
-  // trap zone.
+  // is not found, focus the container.
   function ensureTrapZoneHasFocus(focusedElement: EventTarget | null) {
     if (focusedElement instanceof HTMLElement && document.contains(container)) {
       if (container.contains(focusedElement)) {
@@ -80,16 +79,29 @@ export function focusTrap(
         if (lastFocusedChild && isTabbable(lastFocusedChild) && container.contains(lastFocusedChild)) {
           lastFocusedChild.focus()
           return
+        } else if (initialFocus && container.contains(initialFocus)) {
+          initialFocus.focus()
+          return
         } else {
-          const toFocus = initialFocus && container.contains(initialFocus) ? initialFocus : getFocusableChild(container)
-          if (toFocus) {
-            toFocus.focus()
-            return
-          } else {
-            // no element focusable within trap, blur the external element instead
-            // eslint-disable-next-line github/no-blur
-            focusedElement.blur()
+          // Ensure the container is focusable:
+          // - Either the container already has a `tabIndex`
+          // - Or provide a temporary `tabIndex`
+          const containerNeedsTemporaryTabIndex = container.getAttribute('tabindex') === null
+          if (containerNeedsTemporaryTabIndex) {
+            container.setAttribute('tabindex', '-1')
+          }
+          // Focus the container.
+          container.focus()
+          // If a temporary `tabIndex` was provided, remove it.
+          if (containerNeedsTemporaryTabIndex) {
+            // Once focus has moved from the container to a child within the FocusTrap,
+            // the container can be made un-refocusable by removing `tabIndex`.
+            container.addEventListener('blur', () => container.removeAttribute('tabindex'), {once: true})
+            // NB: If `tabIndex` was removed *before* `blur`, then certain browsers (e.g. Chrome)
+            // would consider `body` the `activeElement`, and as a result, keyboard navigation
+            // between children would break, since `body` is outside the `FocusTrap`.
           }
+          return
         }
       }
     }

src/behaviors/focusZone.ts

@@ -588,7 +588,9 @@ export function focusZone(container: HTMLElement, settings?: FocusZoneSettings):
     }
 
     const focusedIndex = focusableElements.indexOf(currentFocusedElement)
-    return focusedIndex === -1 ? 0 : focusedIndex
+    const fallbackIndex = currentFocusedElement === container ? -1 : 0
+
+    return focusedIndex !== -1 ? focusedIndex : fallbackIndex
   }
 
   // "keydown" is the event that triggers DOM focus change, so that is what we use here