Skip to content

[Bug] Fix useFocusTrap scrolling issue #7242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pksjce merged 11 commits into from
Dec 8, 2025
Merged

[Bug] Fix useFocusTrap scrolling issue #7242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
64cd089
Create a story to reproduce useFocusTrap issue
pksjce Nov 5, 2025
d846dcd
Fix useFocusTrap focus restoration on scroll
pksjce Nov 5, 2025
7615e85
Demonstrate useoutsideclick behavior
pksjce Nov 10, 2025
8f0174b
Allow outside click in useFocustrap
pksjce Nov 11, 2025
79d95f4
FIxes
pksjce Nov 11, 2025
950d57e
Create many-moose-talk.md
pksjce Nov 25, 2025
d2a13a9
Delete .changeset/many-moose-talk.md
pksjce Nov 25, 2025
c57b958
missed import uodate
pksjce Nov 25, 2025
6aef135
Merge branch 'main' into pk/use-focus-trap
pksjce Nov 28, 2025
953e02e
Merge branch 'main' into pk/use-focus-trap
pksjce Dec 4, 2025
22b2fb4
Update .changeset/unlucky-icons-speak.md
pksjce Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/unlucky-icons-speak.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

useFocusTrap - Fix [bug related to restoring focus on scrolling](https://github.com/github/primer/issues/5200)
24 changes: 23 additions & 1 deletion packages/react/src/hooks/useFocusTrap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react'
import {focusTrap} from '@primer/behaviors'
import {useProvidedRefOrCreate} from './useProvidedRefOrCreate'
import {useOnOutsideClick} from './useOnOutsideClick'

export interface FocusTrapHookSettings {
/**
Expand Down Expand Up @@ -34,6 +35,12 @@ export interface FocusTrapHookSettings {
* Overrides restoreFocusOnCleanUp
*/
returnFocusRef?: React.RefObject<HTMLElement | null>
/**
* 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
Comment on lines +38 to +43
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new allowOutsideClick functionality lacks test coverage. Since there are no existing tests for useFocusTrap in the __tests__ directory, and this introduces significant new behavior that could affect focus restoration, tests should be added to verify:

  1. Focus is not restored when clicking outside with allowOutsideClick: true
  2. Focus is properly restored when clicking outside with allowOutsideClick: false or undefined
  3. The interaction between allowOutsideClick and returnFocusRef/restoreFocusOnCleanUp

Consider adding comprehensive tests for this hook to ensure the focus restoration logic works correctly in all scenarios.

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +43
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation states that allowOutsideClick "should allow focus to escape the trap when clicking outside," but the implementation doesn't actually allow focus to escape - it only prevents focus restoration. The actual trap is aborted (line 106), but the wording "allow focus to escape" is misleading since it suggests the focus trap will remain active but allow outside clicks, when in reality it's being disabled.

Consider clarifying the documentation to say: "If true, clicking outside the trap container will disable the trap without restoring focus to the previously focused element."

Copilot uses AI. Check for mistakes.
}

/**
Expand All @@ -45,6 +52,7 @@ export function useFocusTrap(
settings?: FocusTrapHookSettings,
dependencies: React.DependencyList = [],
): {containerRef: React.RefObject<HTMLElement | null>; initialFocusRef: React.RefObject<HTMLElement | null>} {
const [outsideClicked, setOutsideClicked] = React.useState(false)
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outsideClicked state is never reset, which can cause issues when the focus trap is re-enabled. If a user clicks outside once, sets outsideClicked to true, then disables and re-enables the trap, the flag remains true. This means that subsequent cleanups will incorrectly skip focus restoration even if the trap wasn't exited via an outside click the second time.

The state should be reset when the trap is disabled or when disabled changes from false to true.

Copilot uses AI. Check for mistakes.
const containerRef = useProvidedRefOrCreate(settings?.containerRef)
const initialFocusRef = useProvidedRefOrCreate(settings?.initialFocusRef)
const disabled = settings?.disabled
Expand All @@ -53,14 +61,17 @@ 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
}

// This function removes the event listeners that enable the focus trap and restores focus
// 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) {
Expand All @@ -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<HTMLDivElement>,
onClickOutside: () => {
setOutsideClicked(true)
if (settings?.allowOutsideClick) {
if (settings.returnFocusRef) settings.returnFocusRef = undefined
settings.restoreFocusOnCleanUp = false
Comment on lines +104 to +105
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutating the settings object properties is problematic. This violates React's principles of immutability and can lead to unexpected behavior. The settings object is passed from the parent component and should not be modified directly. Additionally, setting returnFocusRef to undefined doesn't actually remove the property from the object, and mutating restoreFocusOnCleanUp will affect the original settings object in the parent component.

Instead, consider using internal state or flags to control the focus restoration behavior without mutating the incoming props.

Copilot uses AI. Check for mistakes.
abortController.current?.abort()
}
},
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useOnOutsideClick hook is called unconditionally on every render, even when allowOutsideClick is not enabled. This adds unnecessary event listeners and performance overhead when the feature isn't being used.

Consider conditionally calling the hook only when settings?.allowOutsideClick is true, or pass a disabled flag to control the hook's behavior. However, note that hooks cannot be called conditionally, so you may need to refactor useOnOutsideClick to accept a disabled parameter or handle the conditional logic internally.

Suggested change
},
},
disabled: !settings?.allowOutsideClick,

Copilot uses AI. Check for mistakes.
})

return {containerRef, initialFocusRef}
}
90 changes: 90 additions & 0 deletions packages/react/src/stories/useFocusTrap.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 './FocusTrap.stories.module.css'

export default {
Expand Down Expand Up @@ -118,6 +119,95 @@ export const RestoreFocus = () => {
)
}

export const RestoreFocusMinimal = () => {
const [enabled, setEnabled] = React.useState(false)
const toggleButtonRef = React.useRef<HTMLButtonElement>(null)
const {containerRef} = useFocusTrap({
disabled: !enabled,
restoreFocusOnCleanUp: true,
returnFocusRef: toggleButtonRef,
allowOutsideClick: true,
})

useOnEscapePress(
React.useCallback(
e => {
if (!enabled) return
e.preventDefault()
setEnabled(false)
},
[enabled, setEnabled],
),
[enabled, setEnabled],
)

return (
<>
<HelperGlobalStyling />
<Stack direction="vertical" gap="normal">
<Flash style={{marginBottom: 'var(--base-size-12)'}}>
Minimal focus trap example. Click to toggle focus trap to toggle. While enabled, focus stays inside the green
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in text: "Click to toggle focus trap to toggle" should be "Click the button to toggle the focus trap" or similar. The phrase "to toggle" is duplicated unnecessarily.

Suggested change
Minimal focus trap example. Click to toggle focus trap to toggle. While enabled, focus stays inside the green
Minimal focus trap example. Click the button to toggle the focus trap. While enabled, focus stays inside the green

Copilot uses AI. Check for mistakes.
zone. Disabling restores focus to the toggle button.
</Flash>
<Button
ref={toggleButtonRef}
onClick={() => {
if (enabled) {
setEnabled(false)
} else {
setEnabled(true)
}
}}
>
{enabled ? 'Disable' : 'Enable'} focus trap
</Button>
<div
style={{
height: '900px',
overflow: 'auto',
border: '1px dashed var(--borderColor-default)',
padding: 'var(--base-size-16)',
background: 'var(--bgColor-muted)',
}}
aria-hidden="true"
>
<Text
as="p"
style={{
fontSize: '12px',
lineHeight: '1.25',
margin: 0,
}}
>
Scroll down to reach the trap zone. This spacer exists so that when the trap zone becomes active you can
scroll such that the original toggle button is no longer visible. When you press Escape or the Close trap
button, focus will still restore to the toggle button and the browser will scroll it back into view.
</Text>
<Text
as="p"
style={{
fontSize: '12px',
lineHeight: '1.25',
margin: 0,
}}
>
(Content intentionally verbose to create vertical space.)
</Text>
</div>
<div className={classes.TrapZone} ref={containerRef as React.RefObject<HTMLDivElement>}>
<Stack direction="vertical" gap="normal">
<MarginButton>First</MarginButton>
<MarginButton>Second</MarginButton>
<MarginButton>Third</MarginButton>
<Button onClick={() => setEnabled(false)}>Close trap</Button>
</Stack>
</div>
<Button>Click here to escape trap</Button>
</Stack>
</>
)
}

export const CustomInitialFocus = () => {
const [trapEnabled, setTrapEnabled] = React.useState(false)
const {containerRef, initialFocusRef} = useFocusTrap({disabled: !trapEnabled})
Expand Down
Loading