From d35c78742174535e9aa196f54a4e424ab80694ec Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Wed, 5 Nov 2025 23:17:09 +1100 Subject: [PATCH 1/6] Create a story to reproduce useFocusTrap issue --- .../src/stories/useFocusTrap.stories.tsx | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/packages/react/src/stories/useFocusTrap.stories.tsx b/packages/react/src/stories/useFocusTrap.stories.tsx index 6df61bdf5e6..6e0c74f0d82 100644 --- a/packages/react/src/stories/useFocusTrap.stories.tsx +++ b/packages/react/src/stories/useFocusTrap.stories.tsx @@ -3,6 +3,7 @@ import type {Meta} from '@storybook/react-vite' import {Button, Flash, Stack, Text} from '..' import {useFocusTrap} from '../hooks/useFocusTrap' +import {useOnEscapePress} from '../hooks/useOnEscapePress' import classes from './FocusTrapStories.module.css' export default { @@ -118,6 +119,85 @@ export const RestoreFocus = () => { ) } +export const RestoreFocusMinimal = () => { + const [enabled, setEnabled] = React.useState(false) + const toggleButtonRef = React.useRef(null) + const {containerRef} = useFocusTrap({ + disabled: !enabled, + returnFocusRef: toggleButtonRef, + }) + useOnEscapePress( + React.useCallback( + e => { + if (!enabled) return + e.preventDefault() + setEnabled(false) + }, + [enabled], + ), + [enabled], + ) + + return ( + <> + + + + Minimal focus trap example. Click "{enabled ? 'Disable' : 'Enable'} focus trap" to toggle. While enabled, + focus stays inside the green zone. Disabling restores focus to the toggle button. + + + + {enabled && ( +
}> + + First + Second + Third + + +
+ )} + +
+ + ) +} + export const CustomInitialFocus = () => { const [trapEnabled, setTrapEnabled] = React.useState(false) const {containerRef, initialFocusRef} = useFocusTrap({disabled: !trapEnabled}) From 30c9e52edf10a9f3f8060638d9b66379055218cb Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Wed, 5 Nov 2025 23:18:59 +1100 Subject: [PATCH 2/6] Fix useFocusTrap focus restoration on scroll --- .changeset/unlucky-icons-speak.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/unlucky-icons-speak.md diff --git a/.changeset/unlucky-icons-speak.md b/.changeset/unlucky-icons-speak.md new file mode 100644 index 00000000000..b592e21f006 --- /dev/null +++ b/.changeset/unlucky-icons-speak.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +useFocusTrap - Fix bug related to restoring focus on scrolling From b25bccece1fa11e97d6ac47395cefb67d3c174f3 Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Wed, 5 Nov 2025 23:26:54 +1100 Subject: [PATCH 3/6] fix lint error --- packages/react/src/stories/useFocusTrap.stories.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/stories/useFocusTrap.stories.tsx b/packages/react/src/stories/useFocusTrap.stories.tsx index 6e0c74f0d82..66113e0f6e9 100644 --- a/packages/react/src/stories/useFocusTrap.stories.tsx +++ b/packages/react/src/stories/useFocusTrap.stories.tsx @@ -143,8 +143,8 @@ export const RestoreFocusMinimal = () => { - Minimal focus trap example. Click "{enabled ? 'Disable' : 'Enable'} focus trap" to toggle. While enabled, - focus stays inside the green zone. Disabling restores focus to the toggle button. + Minimal focus trap example. Click to toggle. While enabled, focus stays inside the green zone. Disabling + restores focus to the toggle button.
{ (Content intentionally verbose to create vertical space.)
- {enabled && ( -
}> - - First - Second - Third - - -
- )} +
}> + + First + Second + Third + + +
From af15edb60f749c75899e66c840be3aa9a2ad719d Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Tue, 11 Nov 2025 21:25:38 +1100 Subject: [PATCH 5/6] Allow outside click in useFocustrap --- packages/react/src/hooks/useFocusTrap.ts | 24 ++++++++++++++- .../src/stories/useFocusTrap.stories.tsx | 30 +++++-------------- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/packages/react/src/hooks/useFocusTrap.ts b/packages/react/src/hooks/useFocusTrap.ts index 62fbc68e636..4e010e6123e 100644 --- a/packages/react/src/hooks/useFocusTrap.ts +++ b/packages/react/src/hooks/useFocusTrap.ts @@ -1,6 +1,7 @@ import React from 'react' import {focusTrap} from '@primer/behaviors' import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' +import {useOnOutsideClick} from './useOnOutsideClick' export interface FocusTrapHookSettings { /** @@ -34,6 +35,12 @@ export interface FocusTrapHookSettings { * Overrides restoreFocusOnCleanUp */ returnFocusRef?: React.RefObject + /** + * If true, it should allow focus to escape the trap when clicking outside of the trap container and mark it as disabled. + * + * Overrides restoreFocusOnCleanUp and returnFocusRef + */ + allowOutsideClick?: boolean } /** @@ -45,6 +52,7 @@ export function useFocusTrap( settings?: FocusTrapHookSettings, dependencies: React.DependencyList = [], ): {containerRef: React.RefObject; initialFocusRef: React.RefObject} { + const [outsideClicked, setOutsideClicked] = React.useState(false) const containerRef = useProvidedRefOrCreate(settings?.containerRef) const initialFocusRef = useProvidedRefOrCreate(settings?.initialFocusRef) const disabled = settings?.disabled @@ -53,7 +61,7 @@ export function useFocusTrap( // If we are enabling a focus trap and haven't already stored the previously focused element // go ahead an do that so we can restore later when the trap is disabled. - if (!previousFocusedElement.current && !settings?.disabled) { + if (!previousFocusedElement.current && !disabled) { previousFocusedElement.current = document.activeElement } @@ -61,6 +69,9 @@ export function useFocusTrap( // to the previously-focused element (if necessary). function disableTrap() { abortController.current?.abort() + if (settings?.allowOutsideClick && outsideClicked) { + return + } if (settings?.returnFocusRef && settings.returnFocusRef.current instanceof HTMLElement) { settings.returnFocusRef.current.focus() } else if (settings?.restoreFocusOnCleanUp && previousFocusedElement.current instanceof HTMLElement) { @@ -85,6 +96,17 @@ export function useFocusTrap( // eslint-disable-next-line react-hooks/exhaustive-deps [containerRef, initialFocusRef, disabled, ...dependencies], ) + useOnOutsideClick({ + containerRef: containerRef as React.RefObject, + onClickOutside: () => { + setOutsideClicked(true) + if (settings?.allowOutsideClick) { + if (settings?.returnFocusRef) settings.returnFocusRef = undefined + settings.restoreFocusOnCleanUp = false + abortController.current?.abort() + } + }, + }) return {containerRef, initialFocusRef} } diff --git a/packages/react/src/stories/useFocusTrap.stories.tsx b/packages/react/src/stories/useFocusTrap.stories.tsx index 8f2cb30085c..ebca6c73066 100644 --- a/packages/react/src/stories/useFocusTrap.stories.tsx +++ b/packages/react/src/stories/useFocusTrap.stories.tsx @@ -122,24 +122,17 @@ export const RestoreFocus = () => { export const RestoreFocusMinimal = () => { const [enabled, setEnabled] = React.useState(false) - // We manage focus restoration manually so we can skip restoring when outside click disables the trap. const toggleButtonRef = React.useRef(null) const {containerRef} = useFocusTrap({ disabled: !enabled, + restoreFocusOnCleanUp: true, + returnFocusRef: toggleButtonRef, + allowOutsideClick: true, }) - const disableTrap = React.useCallback( - (restoreFocus: boolean) => { - setEnabled(false) - if (restoreFocus) { - // Wait a frame to allow trap cleanup to finish before moving focus. - requestAnimationFrame(() => { - toggleButtonRef.current?.focus() - }) - } - }, - [], - ) + const disableTrap = React.useCallback((restoreFocus: boolean) => { + setEnabled(false) + }, []) useOnEscapePress( React.useCallback( e => { @@ -152,14 +145,6 @@ export const RestoreFocusMinimal = () => { [enabled, disableTrap], ) - useOnOutsideClick({ - containerRef: containerRef as React.RefObject, - ignoreClickRefs: [toggleButtonRef], - onClickOutside: () => { - if (enabled) disableTrap(false) // explicitly skip focus restoration on outside click - }, - }) - return ( <> @@ -175,7 +160,6 @@ export const RestoreFocusMinimal = () => { disableTrap(true) } else { setEnabled(true) - // Button already has focus when enabling; no action needed. } }} > @@ -222,7 +206,7 @@ export const RestoreFocusMinimal = () => { - + ) From bc1fcea2ca8a714b50a7580b965ab97897f30cb9 Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Tue, 11 Nov 2025 22:06:54 +1100 Subject: [PATCH 6/6] FIxes --- packages/react/src/hooks/useFocusTrap.ts | 2 +- .../react/src/stories/useFocusTrap.stories.tsx | 14 +++++--------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/react/src/hooks/useFocusTrap.ts b/packages/react/src/hooks/useFocusTrap.ts index 4e010e6123e..6ad1721bb53 100644 --- a/packages/react/src/hooks/useFocusTrap.ts +++ b/packages/react/src/hooks/useFocusTrap.ts @@ -101,7 +101,7 @@ export function useFocusTrap( onClickOutside: () => { setOutsideClicked(true) if (settings?.allowOutsideClick) { - if (settings?.returnFocusRef) settings.returnFocusRef = undefined + if (settings.returnFocusRef) settings.returnFocusRef = undefined settings.restoreFocusOnCleanUp = false abortController.current?.abort() } diff --git a/packages/react/src/stories/useFocusTrap.stories.tsx b/packages/react/src/stories/useFocusTrap.stories.tsx index ebca6c73066..4b49b66badb 100644 --- a/packages/react/src/stories/useFocusTrap.stories.tsx +++ b/packages/react/src/stories/useFocusTrap.stories.tsx @@ -4,7 +4,6 @@ import type {Meta} from '@storybook/react-vite' import {Button, Flash, Stack, Text} from '..' import {useFocusTrap} from '../hooks/useFocusTrap' import {useOnEscapePress} from '../hooks/useOnEscapePress' -import {useOnOutsideClick} from '../hooks/useOnOutsideClick' import classes from './FocusTrapStories.module.css' export default { @@ -130,19 +129,16 @@ export const RestoreFocusMinimal = () => { allowOutsideClick: true, }) - const disableTrap = React.useCallback((restoreFocus: boolean) => { - setEnabled(false) - }, []) useOnEscapePress( React.useCallback( e => { if (!enabled) return e.preventDefault() - disableTrap(true) + setEnabled(false) }, - [enabled, disableTrap], + [enabled, setEnabled], ), - [enabled, disableTrap], + [enabled, setEnabled], ) return ( @@ -157,7 +153,7 @@ export const RestoreFocusMinimal = () => { ref={toggleButtonRef} onClick={() => { if (enabled) { - disableTrap(true) + setEnabled(false) } else { setEnabled(true) } @@ -203,7 +199,7 @@ export const RestoreFocusMinimal = () => { First Second Third - +