Update focusTrap to use new methodology after accessibility discussions

.changeset/twenty-readers-laugh.md

@@ -0,0 +1,5 @@
+---
+'@primer/behaviors': minor
+---
+
+Update focusTrap to use new methodology after accessibility discussions

.gitignore

@@ -1,2 +1,3 @@
 node_modules/
 dist/
+.DS_Store

src/__tests__/focus-trap.test.tsx

@@ -1,6 +1,7 @@
-import {fireEvent, render} from '@testing-library/react'
 import React from 'react'
 import {focusTrap} from '../focus-trap.js'
+import {render} from '@testing-library/react'
+import userEvent from '@testing-library/user-event'
 
 // Since we use strict `isTabbable` checks within focus trap, we need to mock these
 // properties that Jest does not populate.
@@ -22,7 +23,7 @@ beforeAll(() => {
   }
 })
 
-it('Should initially focus the container when activated', () => {
+it('Should initially focus the first element when activated', () => {
   const {container} = render(
     <div>
       <button tabIndex={0}>Bad Apple</button>
@@ -35,8 +36,9 @@ it('Should initially focus the container when activated', () => {
   )
 
   const trapContainer = container.querySelector<HTMLElement>('#trapContainer')!
+  const firstButton = trapContainer.querySelector('button')!
   const controller = focusTrap(trapContainer)
-  expect(document.activeElement).toEqual(trapContainer)
+  expect(document.activeElement).toEqual(firstButton)
 
   controller.abort()
 })
@@ -58,7 +60,7 @@ it('Should initially focus the initialFocus element when specified', () => {
   controller.abort()
 })
 
-it('Should prevent focus from exiting the trap, returns focus to previously-focused element', async () => {
+it('Should prevent focus from exiting the trap, returns focus to first element', async () => {
   const {container} = render(
     <div>
       <div id="trapContainer">
@@ -73,40 +75,25 @@ it('Should prevent focus from exiting the trap, returns focus to previously-focu
   )
 
   const trapContainer = container.querySelector<HTMLElement>('#trapContainer')!
-  const secondButton = trapContainer.querySelectorAll('button')[1]
   const durianButton = container.querySelector<HTMLElement>('#durian')!
-  const controller = focusTrap(trapContainer)
-
-  focus(durianButton)
-  expect(document.activeElement).toEqual(trapContainer)
+  const firstButton = trapContainer.querySelector('button')!
+  const lastButton = trapContainer.querySelectorAll('button')[2]
 
-  focus(secondButton)
-  expect(document.activeElement).toEqual(secondButton)
+  const controller = focusTrap(trapContainer)
 
-  focus(durianButton)
-  expect(document.activeElement).toEqual(secondButton)
+  lastButton.focus()
+  userEvent.tab()
+  expect(document.activeElement).toEqual(firstButton)
 
   controller.abort()
-})
-
-it('Should prevent focus from exiting the trap if there are no focusable children', async () => {
-  const {container} = render(
-    <div>
-      <div id="trapContainer"></div>
-      <button id="durian" tabIndex={0}>
-        Durian
-      </button>
-    </div>
-  )
 
-  const trapContainer = container.querySelector<HTMLElement>('#trapContainer')!
-  const durianButton = container.querySelector<HTMLElement>('#durian')!
-  const controller = focusTrap(trapContainer)
-
-  focus(durianButton)
-  expect(document.activeElement === durianButton).toEqual(false)
+  lastButton.focus()
+  userEvent.tab()
+  expect(document.activeElement).toEqual(durianButton)
+})
 
-  controller.abort()
+it('Should raise an error if there are no focusable children', async () => {
+  // TODO: Having a focus trap with no focusable children is not good for accessibility.
 })
 
 it('Should cycle focus from last element to first element and vice-versa', async () => {
@@ -130,10 +117,10 @@ it('Should cycle focus from last element to first element and vice-versa', async
   const controller = focusTrap(trapContainer)
 
   lastButton.focus()
-  fireEvent(lastButton, new KeyboardEvent('keydown', {bubbles: true, key: 'Tab'}))
+  userEvent.tab()
   expect(document.activeElement).toEqual(firstButton)
 
-  fireEvent(firstButton, new KeyboardEvent('keydown', {bubbles: true, key: 'Tab', shiftKey: true}))
+  userEvent.tab({shift: true})
   expect(document.activeElement).toEqual(lastButton)
 
   controller.abort()
@@ -155,15 +142,19 @@ 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 lastButton = trapContainer.querySelectorAll('button')[2]
 
   const controller = focusTrap(trapContainer)
 
-  focus(durianButton)
-  expect(document.activeElement).toEqual(trapContainer)
+  lastButton.focus()
+  userEvent.tab()
+  expect(document.activeElement).toEqual(firstButton)
 
   controller.abort()
 
-  focus(durianButton)
+  lastButton.focus()
+  userEvent.tab()
   expect(document.activeElement).toEqual(durianButton)
 })
 
@@ -185,14 +176,14 @@ it('Should should release the trap when the container is removed from the DOM',
 
   focusTrap(trapContainer)
 
-  focus(durianButton)
-  expect(document.activeElement).toEqual(trapContainer)
+  durianButton.focus()
+  expect(document.activeElement).toEqual(firstButton)
 
   // empty trap and remove it from the DOM
   trapContainer.removeChild(firstButton)
   trapContainer.parentElement?.removeChild(trapContainer)
 
-  focus(durianButton)
+  durianButton.focus()
   expect(document.activeElement).toEqual(durianButton)
 })
 
@@ -217,20 +208,11 @@ it('Should handle dynamic content', async () => {
 
   secondButton.focus()
   trapContainer.removeChild(thirdButton)
-  fireEvent(secondButton, new KeyboardEvent('keydown', {bubbles: true, key: 'Tab'}))
+  userEvent.tab()
   expect(document.activeElement).toEqual(firstButton)
 
-  fireEvent(firstButton, new KeyboardEvent('keydown', {bubbles: true, key: 'Tab', shiftKey: true}))
+  userEvent.tab({shift: true})
   expect(document.activeElement).toEqual(secondButton)
 
   controller.abort()
 })
-
-/**
- * Helper to handle firing the focusin event, which jest/JSDOM does not do for us.
- * @param element
- */
-function focus(element: HTMLElement) {
-  element.focus()
-  fireEvent(element, new FocusEvent('focusin', {bubbles: true}))
-}

src/focus-trap.ts

@@ -1,4 +1,4 @@
-import {isTabbable, iterateFocusableElements} from './utils/iterate-focusable-elements.js'
+import {getFocusableChild, isTabbable} from './utils/iterate-focusable-elements.js'
 import {polyfill as eventListenerSignalPolyfill} from './polyfills/event-listener-signal.js'
 
 eventListenerSignalPolyfill()
@@ -31,40 +31,43 @@ function followSignal(signal: AbortSignal): AbortController {
 }
 
 /**
- * Returns the first focusable child of `container`. If `lastChild` is true,
- * returns the last focusable child of `container`.
- * @param container
- * @param lastChild
- */
-function getFocusableChild(container: HTMLElement, lastChild = false) {
-  return iterateFocusableElements(container, {reverse: lastChild, strict: true, onlyTabbable: true}).next().value
-}
-
-/**
- * Traps focus within the given container.
+ * Traps focus within the given container
  * @param container The container in which to trap focus
- * @returns AbortController - call `.abort()` to disable the focus trap
+ * @param initialFocus The element to focus when the trap is enabled
+ * @param abortSignal An AbortSignal to control the focus trap
  */
-export function focusTrap(container: HTMLElement, initialFocus?: HTMLElement): AbortController
-
-/**
- * Traps focus within the given container.
- * @param container The container in which to trap focus
- * @param abortSignal An AbortSignal to control the focus trap.
- */
-export function focusTrap(container: HTMLElement, initialFocus: HTMLElement | undefined, abortSignal: AbortSignal): void
 export function focusTrap(
   container: HTMLElement,
   initialFocus?: HTMLElement,
   abortSignal?: AbortSignal
-): AbortController | void {
+): AbortController | undefined {
   // Set up an abort controller if a signal was not passed in
   const controller = new AbortController()
   const signal = abortSignal ?? controller.signal
 
   container.setAttribute('data-focus-trap', 'active')
-  let lastFocusedChild: HTMLElement | undefined = undefined
+  const sentinelStart = document.createElement('span')
+  sentinelStart.setAttribute('class', 'sentinel')
+  sentinelStart.setAttribute('tabindex', '0')
+  sentinelStart.setAttribute('aria-hidden', 'true')
+  sentinelStart.onfocus = () => {
+    const lastFocusableChild = getFocusableChild(container, true)
+    lastFocusableChild?.focus()
+  }
 
+  const sentinelEnd = document.createElement('span')
+  sentinelEnd.setAttribute('class', 'sentinel')
+  sentinelEnd.setAttribute('tabindex', '0')
+  sentinelEnd.setAttribute('aria-hidden', 'true')
+  sentinelEnd.onfocus = () => {
+    // If the end sentinel was focused, move focus to the start
+    const firstFocusableChild = getFocusableChild(container)
+    firstFocusableChild?.focus()
+  }
+  container.prepend(sentinelStart)
+  container.append(sentinelEnd)
+
+  let lastFocusedChild: HTMLElement | undefined = undefined
   // 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
@@ -83,24 +86,8 @@ export function focusTrap(
           initialFocus.focus()
           return
         } else {
-          // 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`.
-          }
+          const firstFocusableChild = getFocusableChild(container)
+          firstFocusableChild?.focus()
           return
         }
       }
@@ -109,27 +96,6 @@ export function focusTrap(
 
   const wrappingController = followSignal(signal)
 
-  container.addEventListener(
-    'keydown',
-    event => {
-      if (event.key !== 'Tab' || event.defaultPrevented) {
-        return
-      }
-
-      const {target} = event
-      const firstFocusableChild = getFocusableChild(container)
-      const lastFocusableChild = getFocusableChild(container, true)
-      if (target === firstFocusableChild && event.shiftKey) {
-        event.preventDefault()
-        lastFocusableChild?.focus()
-      } else if (target === lastFocusableChild && !event.shiftKey) {
-        event.preventDefault()
-        firstFocusableChild?.focus()
-      }
-    },
-    {signal: wrappingController.signal}
-  )
-
   if (activeTrap) {
     const suspendedTrap = activeTrap
     activeTrap.container.setAttribute('data-focus-trap', 'suspended')
@@ -145,6 +111,8 @@ export function focusTrap(
   // Only when user-canceled
   signal.addEventListener('abort', () => {
     container.removeAttribute('data-focus-trap')
+    const sentinels = container.getElementsByClassName('sentinel')
+    while (sentinels.length > 0) sentinels[0].remove()
     const suspendedTrapIndex = suspendedTrapStack.findIndex(t => t.container === container)
     if (suspendedTrapIndex >= 0) {
       suspendedTrapStack.splice(suspendedTrapIndex, 1)
@@ -173,7 +141,7 @@ export function focusTrap(
   }
 
   // If we are activating a focus trap for a container that was previously
-  // suspended, just remove it from the suspended list.
+  // suspended, just remove it from the suspended list
   const suspendedTrapIndex = suspendedTrapStack.findIndex(t => t.container === container)
   if (suspendedTrapIndex >= 0) {
     suspendedTrapStack.splice(suspendedTrapIndex, 1)

src/utils/iterate-focusable-elements.ts

@@ -67,6 +67,16 @@ export function* iterateFocusableElements(
   return undefined
 }
 
+/**
+ * Returns the first focusable child of `container`. If `lastChild` is true,
+ * returns the last focusable child of `container`.
+ * @param container
+ * @param lastChild
+ */
+export function getFocusableChild(container: HTMLElement, lastChild = false) {
+  return iterateFocusableElements(container, {reverse: lastChild, strict: true, onlyTabbable: true}).next().value
+}
+
 /**
  * Determines whether the given element is focusable. If `strict` is true, we may
  * perform additional checks that require a reflow (less performant).
@@ -80,7 +90,8 @@ export function isFocusable(elem: HTMLElement, strict = false): boolean {
     (elem as HTMLElement & {disabled: boolean}).disabled
   const hiddenInert = elem.hidden
   const hiddenInputInert = elem instanceof HTMLInputElement && elem.type === 'hidden'
-  if (disabledAttrInert || hiddenInert || hiddenInputInert) {
+  const sentinelInert = elem.classList.contains('sentinel')
+  if (disabledAttrInert || hiddenInert || hiddenInputInert || sentinelInert) {
     return false
   }