From 64cd089dc631018afeab5cb7d1491d234ab44108 Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Wed, 5 Nov 2025 23:17:09 +1100 Subject: [PATCH 1/9] Create a story to reproduce useFocusTrap issue --- .../src/stories/useFocusTrap.stories.tsx | 82 ++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/packages/react/src/stories/useFocusTrap.stories.tsx b/packages/react/src/stories/useFocusTrap.stories.tsx index e6b5543215e..3c6c6baa1be 100644 --- a/packages/react/src/stories/useFocusTrap.stories.tsx +++ b/packages/react/src/stories/useFocusTrap.stories.tsx @@ -3,7 +3,8 @@ import type {Meta} from '@storybook/react-vite' import {Button, Flash, Stack, Text} from '..' import {useFocusTrap} from '../hooks/useFocusTrap' -import classes from './FocusTrap.stories.module.css' +import {useOnEscapePress} from '../hooks/useOnEscapePress' +import classes from './FocusTrapStories.module.css' export default { title: 'Hooks/useFocusTrap', @@ -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 to toggle 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 d846dcdb5dcf282a020e3ee391926e136d4b850d Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Wed, 5 Nov 2025 23:18:59 +1100 Subject: [PATCH 2/9] 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 7615e854851ba8ae4ba38501d2bf7ee2b39049f4 Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Mon, 10 Nov 2025 20:03:43 +1100 Subject: [PATCH 3/9] Demonstrate useoutsideclick behavior --- .../src/stories/useFocusTrap.stories.tsx | 60 ++++++++++++++----- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/packages/react/src/stories/useFocusTrap.stories.tsx b/packages/react/src/stories/useFocusTrap.stories.tsx index 3c6c6baa1be..d1eae638e86 100644 --- a/packages/react/src/stories/useFocusTrap.stories.tsx +++ b/packages/react/src/stories/useFocusTrap.stories.tsx @@ -4,6 +4,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 {useOnOutsideClick} from '../hooks/useOnOutsideClick' import classes from './FocusTrapStories.module.css' export default { @@ -121,23 +122,44 @@ 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, - returnFocusRef: toggleButtonRef, }) + + 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() + }) + } + }, + [], + ) useOnEscapePress( React.useCallback( e => { if (!enabled) return e.preventDefault() - setEnabled(false) + disableTrap(true) }, - [enabled], + [enabled, disableTrap], ), - [enabled], + [enabled, disableTrap], ) + useOnOutsideClick({ + containerRef: containerRef as React.RefObject, + ignoreClickRefs: [toggleButtonRef], + onClickOutside: () => { + if (enabled) disableTrap(false) // explicitly skip focus restoration on outside click + }, + }) + return ( <> @@ -146,7 +168,17 @@ export const RestoreFocusMinimal = () => { Minimal focus trap example. Click to toggle focus trap 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 8f0174bb31ea22924f3f8953506fb919e986cdce Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Tue, 11 Nov 2025 21:25:38 +1100 Subject: [PATCH 4/9] 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 d1eae638e86..83db3803f67 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 79d95f424256cd34cc8db2b182caaa74957bf01e Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Tue, 11 Nov 2025 22:06:54 +1100 Subject: [PATCH 5/9] 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 83db3803f67..ba1b97b77c6 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 - + From 950d57e51c3f3f8770dd3d8a437da9d648640512 Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Tue, 25 Nov 2025 14:46:28 +1100 Subject: [PATCH 6/9] Create many-moose-talk.md --- .changeset/many-moose-talk.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/many-moose-talk.md diff --git a/.changeset/many-moose-talk.md b/.changeset/many-moose-talk.md new file mode 100644 index 00000000000..e439172a725 --- /dev/null +++ b/.changeset/many-moose-talk.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +[Bug] Fix useFocusTrap scrolling issue From d2a13a9cd082e2285accbc9c71694262febbf95f Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Tue, 25 Nov 2025 16:55:26 +1100 Subject: [PATCH 7/9] Delete .changeset/many-moose-talk.md --- .changeset/many-moose-talk.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/many-moose-talk.md diff --git a/.changeset/many-moose-talk.md b/.changeset/many-moose-talk.md deleted file mode 100644 index e439172a725..00000000000 --- a/.changeset/many-moose-talk.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@primer/react": patch ---- - -[Bug] Fix useFocusTrap scrolling issue From c57b958c40b90800e4a22a4262b7b69ff8d3154f Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Tue, 25 Nov 2025 16:56:08 +1100 Subject: [PATCH 8/9] missed import uodate --- packages/react/src/stories/useFocusTrap.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/stories/useFocusTrap.stories.tsx b/packages/react/src/stories/useFocusTrap.stories.tsx index ba1b97b77c6..5969539d81d 100644 --- a/packages/react/src/stories/useFocusTrap.stories.tsx +++ b/packages/react/src/stories/useFocusTrap.stories.tsx @@ -4,7 +4,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' +import classes from './FocusTrap.stories.module.css' export default { title: 'Hooks/useFocusTrap', From 22b2fb49ed9d884b667e8a5f59b91455843b6414 Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Mon, 8 Dec 2025 13:53:38 +1100 Subject: [PATCH 9/9] Update .changeset/unlucky-icons-speak.md Co-authored-by: Siddharth Kshetrapal --- .changeset/unlucky-icons-speak.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/unlucky-icons-speak.md b/.changeset/unlucky-icons-speak.md index b592e21f006..20030cb0440 100644 --- a/.changeset/unlucky-icons-speak.md +++ b/.changeset/unlucky-icons-speak.md @@ -2,4 +2,4 @@ "@primer/react": patch --- -useFocusTrap - Fix bug related to restoring focus on scrolling +useFocusTrap - Fix [bug related to restoring focus on scrolling](https://github.com/github/primer/issues/5200)