DialogV2: re-set focusTrap when footer content changes

[TylerJDev]

Closes https://github.com/github/primer/issues/3358 https://github.com/github/primer/issues/3356 https://github.com/github/primer/issues/3357 https://github.com/github/primer/issues/2480

Adds footer as a dependency of useFocusTrap, ensures that if content is changed within either, focus trap will properly focus newly added focusable elements. Example story: Repro Multistep Dialog With Conditional Footer.

Changelog

Changed

• Adds footer to useFocusTrap dependencies
• Removes autofocus prop from stories

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]

🦋 Changeset detected

Latest commit: 4d7eba0

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	Minor

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

[TylerJDev]

I had to utilize querySelector to search for the close button. I'm wondering if there'd be any benefit to letting consumers add a ref directly to it via a prop (e.g. closeButtonRef)? This fix is only required for a story, so it's low priority.

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	96.64 KB (+0.23% 🔺)
packages/react/dist/browser.umd.js	96.9 KB (+0.14% 🔺)

[siddharthkp]

Logic is sound. Just need to test for any performance implications

[TylerJDev]

Logic is sound. Just need to test for any performance implications

That is fair! Is there a good way to go about this outside of just testing existing dialogs? Another option is making this opt-in, but not too sure if that's the best route to take if we want this functionality by default 🤔

[siddharthkp]

That is fair! Is there a good way to go about this outside of just testing existing dialogs?

Can see how if the hook is called only when it should or also on every render or on unrelated changes inside the Dialog content. Dependency on a const body is looks doubtful to me

Another option is making this opt-in, but not too sure if that's the best route to take if we want this functionality by default 🤔

I think this should be default as well

[siddharthkp]

We do this in focus-zone already, so we'd be able to take from an existing implementation. There is still some performance concern, but I don't think it'd be as bad if we're handling it correctly. This also means that on re-renders we wouldn't be initializing the focus trap once set

Ooh i really like that idea!

I wonder how eager should the observer be. We haven't received any performance concerns about focus zone yet, but it's also not a common use case where content of focus zone changes, so maybe we don't attribute it to the hook

both requiring changes to the behavior, rather than the useFocusTrap hook.

I think that's totally fine, we maintain both libraries anyways 😄

[TylerJDev]

Quick update, working on a fix in primer/behaviors: primer/behaviors#429. Basically adding a mutation observer that checks if the start or end sentinels are no longer at the start/end.

[primer-integration]

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

[TylerJDev]

@siddharthkp, tested this with the updated behaviors version & very lightly in dotcom (though I couldn't find too many dialogs). Doesn't look like there's much performance hit, as the likelihood of content being added before/after sentinels is low. In cases where elements are added after the focus trap, they will only be moved once once it's asserted that either the first child or last child in the focus trap is a .sentinel.

I'm curious if there's any additional testing we should do, or if we should ship this change (with the behaviors bump).

[siddharthkp]

Great, thanks for making sure! I think it sounds good, let's ship it