Overlay: Add role="dialog", focus trap to Overlay

[TylerJDev]

Closes https://github.com/github/primer/issues/3378

Usages of Overlay tend to lean more towards a dialog across GH. This PR aims to define it as a dialog in most cases, provided with a focus trap that's toggled via the newly added focusTrap prop.

Changelog

New

• Add role="dialog" and aria-modal="true" to Overlay component

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: 5627e4f

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

[github-actions]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	97.85 KB (+0.02% 🔺)
packages/react/dist/browser.umd.js	98.16 KB (+0.1% 🔺)

[primer-integration]

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

[TylerJDev]

This change essentially turns Overlay into a "Dialog" in the sense of role="dialog" and aria-modal="true" being added. A focus trap is also added by default, and is enabled if there are focusable elements inside of the overlay. This change will not affect overlays that already have a role attribute, as long as that role attribute isn't role="dialog".

Rollout plan:

• Add role="none" to existing Overlay components that do not implicitly set role already
• Add focusTrap={false} to Overlay components that are used as Dialogs currently, but do not utilize a focus trap.

[TylerJDev]

Added a new prop in case there's an implementation that wants to be a dialog, but doesn't want the focus trap applied. This is a very specific case, and ideally won't be necessary in more than a handful of instances.

[TylerJDev]

@primer/engineer-reviewers - Hey team! Would love any additional eyes on this PR, mainly in regards to my rollout plan.

[francinelucca]

LGTM! One minor suggestion and one question:
https://github.com/github/primer/issues/3378 suggests:

allow users to switch to/from the non-modal dialog using Ctrl+F6 / Command+F6

Did we implement that? I couldn't get that to go in my tests

[francinelucca]

should this be a boolean instead?

Suggested change

	"type": "string",
	"type": "boolean",

[keithamus]

If it is a role=dialog and has focus trapping, it should be a <dialog> element with showModal() called.

We should steer away from useFocusTrap and use showModal() on <dialog> elements instead. It comes with many other small benefits such as retaining previously-focused-element.

If you need any assistance on this please reach out and I'll happily pair.

[TylerJDev]

If it is a role=dialog and has focus trapping, it should be a <dialog> element with showModal() called.

We should steer away from useFocusTrap and use showModal() on <dialog> elements instead. It comes with many other small benefits such as retaining previously-focused-element.

That makes sense! I'm curious if we'd want this to be true for components that consume Overlay or AnchoredOverlay (e.g. ActionMenu, SelectPanel), as they rely on Overlay internally. I guess it would be worth making those components utilize Dialog, as we utilize it in the PVC SelectPanel already. But I'm curious what you think? This seems like it would require more effort and testing, but could be worth it in the long run.

Right now, this PR will only affect components that utilize Overlay in a standalone context. Components that utilize Overlay like ActionMenu would retain the same semantics/roles even though it technically uses it internally to render the menu. We could see about conditionally rendering a <dialog> vs the current <div> container but only in instances where it's being used as a standalone component, but I'm wondering if that's worth it or not 🤔

[keithamus]

I think Dialog and components that use <dialog> can be discrete. I see little value in mapping our React library to individual HTML elements.

As for conditionally rendering <dialog> vs <div>, that kind of seems fine to me but I’d actually sooner see us split out ActionMenu to not use Overlay which is a case I’ve made before about the general reuse of these components; I don’t think there is enough overlap to consider reuse here. There can be multiple discrete types of floating UI.