Overlay: only prevent focus on open when component is open #5636

ktravers · 2025-01-31T18:03:06Z

Depends on https://github.com/github/github/pull/361151
Related to https://github.com/github/github/pull/360074

This PR updates the useOpenAndCloseFocus hook to apply focus to the given return ref element when the preventFocusOnOpen prop is also given.

Previously, if preventFocusOnOpen argument was given, then focus was not applied as expected to the given return ref element, resulting in accessibility issues. With these changes, focus will be applied to the given return ref in all cases, regardless of whether preventFocusOnOpen argument is included, allowing these arguments to be used together without any unexpected conflicts.

Currently, these change affect the Overlay component only, as the sole consumer of the useOpenAndCloseFocus hook.

Note

The preventFocusOnOpen prop is currently undocumented in the Overlay component. It's unclear to me whether this is intentional. I opted not to document it for now, but would appreciate reviewers' thoughts on that.

Changelog

New

Additional tests for useOpenAndCloseFocus hook
Additional tests for Overlay component (which depends on useOpenAndCloseFocus hook)
PreventFocusOnOpen story in Overlay.dev.stories.tsx

Changed

Logic within useOpenAndCloseFocus hook

Removed

N/A

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

Run npm test Overlay to run component tests
Run npm test hooks/useOpenAndCloseFocus to run hook tests
View Overlay component in Storybook (Overlay > dev > PreventFocusOnOpen)

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)

Otherwise, prevents focus from returning to given returnFocusRef.

changeset-bot · 2025-01-31T18:03:09Z

🦋 Changeset detected

Latest commit: 868ece3

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-01-31T18:03:37Z

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

github-actions · 2025-01-31T18:06:40Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	105.33 KB (-0.09% 🔽)
packages/react/dist/browser.umd.js	105.81 KB (-0.02% 🔽)

ktravers · 2025-02-05T03:38:18Z

packages/react/src/hooks/useOpenAndCloseFocus.ts

-    if (preventFocusOnOpen) {
-      return
-    }


These lines are the most critical change. By returning early here, we prevent the hook from ever applying focus to the given returnFocusRef. This results in unexpected behavior in components like Overlay when preventFocusOnOpen and returnFocusRef props are used together -- specifically, return focus is not applied as expected.

Also worth noting -- preventFocusOnOpen is currently undocumented. However, this has not prevented it from being used in production (for example, in the case that inspired me to open this PR).

Should this prop be documented? 🤔

On the one hand, I suspect we don't want to encourage its use. However, by not documenting it, users are more susceptible to "misusing" the prop, resulting in unexpected behavior. Maybe that's not a concern if these changes are merged, but I wanted to ask anyway since it seems worth considering.

By returning early here, we prevent the hook from ever applying focus to the given returnFocusRef

I see! So that meant that returnRef wasn't defined if preventFocusOnOpen === true?

Should this prop be documented?

I think we have some pretty old and mostly hidden docs for this hook in primer.style. It doesn't really explain too much, so I think it's worth updating! Ideally, we'll need to rely on this hook even less once we utilize popover in Overlay.

So that meant that returnRef wasn't defined if preventFocusOnOpen === true?

returnRef was defined, but returnRef.focus() was never called if preventFocusOnOpen === true. It just sort of failed silently 😞

I think it's worth updating! Ideally, we'll need to rely on this hook even less once we utilize popover in Overlay.

Cool, makes sense! Also thanks for linking to the hooks docs. I never checked outside the component docs 🤦‍♀️ I'll make sure preventFocusOnOpen is documented in both places 👍

hook docs update: primer/design#926

component docs updated here: 868ece3

ktravers · 2025-02-06T02:43:52Z

Taking back to draft while I work on integration tests.

ktravers · 2025-02-06T03:26:31Z

Did some debugging on the integration test branch, and turns out there's a component in github/github that relies on the current (and I would argue, incorrect) behavior, where no focus is applied even on close if preventFocusOnOpen prop is given. See this commit for full details: https://github.com/github/github/pull/361072/commits/b2d5902cdcf7a6d082dcf3de18b491d2f3f3abc3

I'm going to test whether the change above works with the current Overlay behavior. If yes, then I'll ship the gh/gh PR first, then come back to this one. If no, then I'll need to chat with maintainers about options for moving forward (if any).

[update] Good news: the changes are backwards compatible. See https://github.com/github/github/pull/361151.

primer-integration · 2025-02-06T04:05:47Z

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

primer-integration · 2025-02-06T05:07:28Z

🟢 golden-jobs completed with status success.

TylerJDev · 2025-02-06T19:31:36Z

packages/react/src/hooks/useOpenAndCloseFocus.ts

@@ -16,18 +16,21 @@ export function useOpenAndCloseFocus({
  preventFocusOnOpen,
 }: UseOpenAndCloseFocusSettings): void {
  useEffect(() => {
-    if (preventFocusOnOpen) {


I'm wondering what exactly preventFocusOnOpen was being used for 🤔 In your use case, were you utilizing this prop? Seems counter intuitive, but I might not be seeing the full picture 😅

The two cases I'm aware of were both used to keep focus on the "entrypoint" element when an overlay was opened:

Copilot chat panel. I think it was used by mistake here, because there was also code to apply focus to an input inside the overlay after it was opened 🤦‍♀️ I recently removed it.

Autocomplete. Here it was used very intentionally to keep focus on the textarea, where the user is typing, while the autocomplete dialog is open.

Related: primer/react#5636

Based on hooks docs: * https://primer.style/guides/react/hooks/use-open-and-close-focus Related: * primer/design#926

github-actions · 2025-02-06T20:55:49Z

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

Related: primer/react#5636

…ay hooks (#926) * document preventFocusOnOpen prop Related: primer/react#5636 * document prop in useOverlay hook

TylerJDev

Looks good to me! Thank you for all the changes and updates to the docs, truly appreciate it ✨

only prevent focus when component is open

fb33fed

Otherwise, prevents focus from returning to given returnFocusRef.

ktravers self-assigned this Jan 31, 2025

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 Jan 31, 2025

ktravers temporarily deployed to github-pages January 31, 2025 18:18 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5636 January 31, 2025 18:18 Inactive

ktravers temporarily deployed to github-pages February 4, 2025 03:57 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5636 February 4, 2025 03:58 Inactive

ktravers force-pushed the ktravers-patch-1 branch from 6acf793 to 9e6abee Compare February 5, 2025 02:16

github-actions bot requested a deployment to storybook-preview-5636 February 5, 2025 02:24 Abandoned

ktravers temporarily deployed to github-pages February 5, 2025 02:38 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5636 February 5, 2025 02:38 Inactive

ktravers force-pushed the ktravers-patch-1 branch from 9e6abee to 3e9b2f9 Compare February 5, 2025 02:42

fixup callback

d4253a8

ktravers force-pushed the ktravers-patch-1 branch from 3e9b2f9 to 3a3ea3d Compare February 5, 2025 02:47

github-actions bot requested a deployment to storybook-preview-5636 February 5, 2025 02:51 Abandoned

add component tests asserting expected behavior

8b89620

ktravers force-pushed the ktravers-patch-1 branch from 3a3ea3d to 8b89620 Compare February 5, 2025 03:03

github-actions bot temporarily deployed to storybook-preview-5636 February 5, 2025 03:05 Inactive

ktravers temporarily deployed to github-pages February 5, 2025 03:20 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5636 February 5, 2025 03:21 Inactive

update hook tests

9b745ba

ktravers force-pushed the ktravers-patch-1 branch from 494bc84 to 9b745ba Compare February 5, 2025 03:35

ktravers commented Feb 5, 2025

View reviewed changes

github-actions bot requested a deployment to storybook-preview-5636 February 5, 2025 03:38 Abandoned

ktravers temporarily deployed to github-pages February 5, 2025 03:52 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5636 February 5, 2025 03:52 Inactive

Merge branch 'main' into ktravers-patch-1

b386f7c

ktravers requested a review from TylerJDev February 5, 2025 05:19

ktravers temporarily deployed to github-pages February 5, 2025 05:20 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5636 February 5, 2025 05:21 Inactive

github-actions bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Feb 5, 2025

ktravers marked this pull request as draft February 6, 2025 02:43

github-actions bot added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: failing Changes in this PR cause breaking changes in gh/gh labels Feb 6, 2025

ktravers marked this pull request as ready for review February 6, 2025 04:04

TylerJDev reviewed Feb 6, 2025

View reviewed changes

ktravers added a commit to primer/design that referenced this pull request Feb 6, 2025

document preventFocusOnOpen prop

9d4229b

Related: primer/react#5636

ktravers mentioned this pull request Feb 6, 2025

document preventFocusOnOpen prop in useOpenAndCloseFocus and useOverlay hooks primer/design#926

Merged

document preventFocusOnOpen prop

868ece3

Based on hooks docs: * https://primer.style/guides/react/hooks/use-open-and-close-focus Related: * primer/design#926

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 Feb 6, 2025

ktravers added a commit to primer/design that referenced this pull request Feb 6, 2025

document preventFocusOnOpen prop

7326474

Related: primer/react#5636

ktravers deployed to github-pages February 6, 2025 21:07 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-5636 February 6, 2025 21:08 View deployment

camertron pushed a commit to primer/design that referenced this pull request Feb 6, 2025

document preventFocusOnOpen prop in useOpenAndCloseFocus and useOverl…

e845189

…ay hooks (#926) * document preventFocusOnOpen prop Related: primer/react#5636 * document prop in useOverlay hook

TylerJDev approved these changes Feb 6, 2025

View reviewed changes

ktravers removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 7, 2025

ktravers added this pull request to the merge queue Feb 8, 2025

Merged via the queue into main with commit c2165af Feb 8, 2025
47 checks passed

ktravers deleted the ktravers-patch-1 branch February 8, 2025 01:47

primer bot mentioned this pull request Feb 8, 2025

Release tracking #5665

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overlay: only prevent focus on open when component is open #5636

Overlay: only prevent focus on open when component is open #5636

ktravers commented Jan 31, 2025 •

edited

Loading

changeset-bot bot commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025

github-actions bot commented Jan 31, 2025 •

edited

Loading

ktravers Feb 5, 2025

ktravers Feb 5, 2025

TylerJDev Feb 6, 2025

ktravers Feb 6, 2025

ktravers Feb 6, 2025

ktravers Feb 6, 2025

ktravers commented Feb 6, 2025

ktravers commented Feb 6, 2025 •

edited

Loading

primer-integration bot commented Feb 6, 2025

primer-integration bot commented Feb 6, 2025

TylerJDev Feb 6, 2025

ktravers Feb 6, 2025

github-actions bot commented Feb 6, 2025

TylerJDev left a comment

Overlay: only prevent focus on open when component is open #5636

Overlay: only prevent focus on open when component is open #5636

Conversation

ktravers commented Jan 31, 2025 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Jan 31, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Jan 31, 2025

github-actions bot commented Jan 31, 2025 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktravers commented Feb 6, 2025

ktravers commented Feb 6, 2025 • edited Loading

primer-integration bot commented Feb 6, 2025

primer-integration bot commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2025

TylerJDev left a comment

Choose a reason for hiding this comment

ktravers commented Jan 31, 2025 •

edited

Loading

changeset-bot bot commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

ktravers commented Feb 6, 2025 •

edited

Loading