[Bug] Fix useFocusTrap scrolling issue #7242

pksjce · 2025-11-25T03:46:11Z

Closes #https://github.com/github/primer/issues/5200

Changelog

New

Changed

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2025-11-25T03:46:16Z

🦋 Changeset detected

Latest commit: 953e02e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-11-25T03:46:44Z

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

primer-integration · 2025-11-25T06:14:22Z

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/7563

primer-integration · 2025-12-02T11:21:54Z

🟢 ci completed with status success.

github-actions · 2025-12-04T04:40:34Z

👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

pksjce · 2025-12-04T04:40:53Z

I spoke with @ericwbailey and @JoyceZhu to validate that this is the right approach.
Basically, if new allowOutsideClick prop is present, we will ignore the returnRef and let the mouse focus wherever.

Copilot

Pull request overview

This PR addresses a scrolling-related bug in the useFocusTrap hook by adding an allowOutsideClick option that prevents focus restoration when users click outside the trap container. The change allows the browser to maintain the user's current scroll position when the trap is dismissed via outside click, rather than scrolling back to restore focus to the original element.

Key Changes:

Added allowOutsideClick boolean option to FocusTrapHookSettings interface
Integrated useOnOutsideClick hook to detect clicks outside the trap container
Modified focus restoration logic to skip restoration when exiting via outside click
Added comprehensive Storybook example (RestoreFocusMinimal) demonstrating the fix

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
`packages/react/src/hooks/useFocusTrap.ts`	Adds `allowOutsideClick` option and integrates outside click detection to conditionally prevent focus restoration
`packages/react/src/stories/useFocusTrap.stories.tsx`	Adds new story demonstrating the scrolling fix with a minimal reproducible example
`.changeset/unlucky-icons-speak.md`	Documents the patch-level bug fix for release notes

Copilot · 2025-12-04T04:45:02Z

packages/react/src/hooks/useFocusTrap.ts

+        if (settings.returnFocusRef) settings.returnFocusRef = undefined
+        settings.restoreFocusOnCleanUp = false


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 · 2025-12-04T04:45:02Z

packages/react/src/hooks/useFocusTrap.ts

+        settings.restoreFocusOnCleanUp = false
+        abortController.current?.abort()
+      }
+    },


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 · 2025-12-04T04:45:03Z

packages/react/src/hooks/useFocusTrap.ts

  settings?: FocusTrapHookSettings,
  dependencies: React.DependencyList = [],
 ): {containerRef: React.RefObject<HTMLElement | null>; initialFocusRef: React.RefObject<HTMLElement | null>} {
+  const [outsideClicked, setOutsideClicked] = React.useState(false)


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 · 2025-12-04T04:45:03Z

packages/react/src/stories/useFocusTrap.stories.tsx

+      <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


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 · 2025-12-04T04:45:03Z

packages/react/src/hooks/useFocusTrap.ts

+  /**
+   * 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


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:

Focus is not restored when clicking outside with allowOutsideClick: true

Focus is properly restored when clicking outside with allowOutsideClick: false or undefined

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 · 2025-12-04T04:45:03Z

packages/react/src/hooks/useFocusTrap.ts

+  /**
+   * 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


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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

pksjce added 5 commits November 25, 2025 14:43

Create a story to reproduce useFocusTrap issue

64cd089

Fix useFocusTrap focus restoration on scroll

d846dcd

Demonstrate useoutsideclick behavior

7615e85

Allow outside click in useFocustrap

8f0174b

FIxes

79d95f4

Create many-moose-talk.md

950d57e

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Nov 25, 2025

github-actions bot requested a deployment to storybook-preview-7242 November 25, 2025 03:50 Abandoned

pksjce had a problem deploying to github-pages November 25, 2025 03:56 — with GitHub Actions Failure

github-actions bot requested a deployment to storybook-preview-7242 November 25, 2025 03:56 Abandoned

pksjce added 2 commits November 25, 2025 16:55

Delete .changeset/many-moose-talk.md

d2a13a9

missed import uodate

c57b958

github-actions bot requested a deployment to storybook-preview-7242 November 25, 2025 06:00 Abandoned

pksjce deployed to github-pages November 25, 2025 06:08 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-7242 November 25, 2025 06:09 Inactive

Merge branch 'main' into pk/use-focus-trap

6aef135

pksjce deployed to github-pages November 28, 2025 05:01 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-7242 November 28, 2025 05:01 Inactive

github-actions bot removed the integration-tests: failing Changes in this PR cause breaking changes in gh/gh label Dec 1, 2025

Merge branch 'main' into pk/use-focus-trap

953e02e

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Dec 4, 2025

pksjce marked this pull request as ready for review December 4, 2025 04:40

pksjce requested a review from a team as a code owner December 4, 2025 04:40

pksjce requested review from TylerJDev and Copilot December 4, 2025 04:40

github-actions bot removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Dec 4, 2025

pksjce requested a review from siddharthkp December 4, 2025 04:41

Copilot started reviewing on behalf of pksjce December 4, 2025 04:41 View session

Copilot finished reviewing on behalf of pksjce December 4, 2025 04:43

Copilot AI reviewed Dec 4, 2025

View reviewed changes

pksjce deployed to github-pages December 4, 2025 04:49 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-7242 December 4, 2025 04:50 View deployment

Copilot AI reviewed Dec 4, 2025

View reviewed changes

		if (settings.returnFocusRef) settings.returnFocusRef = undefined
		settings.restoreFocusOnCleanUp = false

	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

[Bug] Fix useFocusTrap scrolling issue #7242

Are you sure you want to change the base?

[Bug] Fix useFocusTrap scrolling issue #7242

Conversation

pksjce commented Nov 25, 2025

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

Uh oh!

changeset-bot bot commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Nov 25, 2025

Uh oh!

primer-integration bot commented Nov 25, 2025

Uh oh!

primer-integration bot commented Dec 2, 2025

Uh oh!

github-actions bot commented Dec 4, 2025

Uh oh!

pksjce commented Dec 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot bot commented Nov 25, 2025 •

edited

Loading