Dialog: Fix issue where focus would be lost if multiple 'popovers' were active

[TylerJDev]

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

Fixes an issue where focus would be lost (escape the dialog's focus trap) when a <dialog> was opened via showModal() from inside a popover with popover="auto". When tabbing past the last focusable element in the dialog, focus would escape the dialog instead of being trapped within it.

This happened because the popover="auto" behavior on ancestor popovers interfered with the native <dialog> focus trap.

Closes https://github.com/github/accessibility-audits/issues/14739

Screenshots

List the issues that this change affects.

Closes https://github.com/github/accessibility-audits/issues/14739

Risk Assessment

• Low risk the change is small, highly observable, and easily rolled back.

This change modifies a single file (dialog_helper.ts) with a minimal, targeted adjustment to the existing popover workaround logic. The behavioral change is limited to how ancestor popovers are handled when a dialog opens from within them.

What approach did you choose and why?

The existing code in dialog_helper.ts already had a workaround for an older browser bug where opening a <dialog> from inside a popover would cause the "hide all popovers" algorithm to run, closing the ancestor popover. The fix converted those closed popovers from auto to manual and re-showed them.

However, the existing workaround only targeted popovers that were not currently open (:not(:popover-open)), which meant it didn't address a second, related issue in newer browsers: even when the popover stays open, popover="auto" still interferes with the native <dialog> focus trap, causing focus to escape the dialog.

The fix broadens the selector from [popover]:not(:popover-open) to [popover], so that all ancestor auto popovers are converted to manual, regardless of whether they are currently open. Since changing a popover's type from auto to manual closes it, we also re-show it immediately after conversion. This addresses both the old browser bug and the newer focus trap interference in a single, unified approach.

Anything you want to highlight for special attention from reviewers?

• The key change is removing :not(:popover-open) from the closest() selector, which means the workaround now applies to all ancestor auto popovers, not just those that were closed by the browser's "hide all popovers" algorithm.
• Since switching a popover from auto to manual causes it to close, the showPopover() call is now necessary in all cases (not just the older browser scenario). The comment has been updated to reflect this.
• Worth verifying that existing popover + dialog interactions (e.g., action menus inside dialogs) are not affected by this broader conversion.

Accessibility

• Fixes axe scan violation - This change fixes an existing axe scan violation.
• No new axe scan violation - This change does not introduce any new axe scan violations.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Lookbook)
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 0daf4a0

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

This PR includes changesets to release 1 package

Name	Type
@primer/view-components	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

[Copilot]

Pull request overview

This PR fixes an accessibility issue where focus could escape a dialog's focus trap when the dialog was opened from within a popover with popover="auto". The fix broadens the existing popover workaround to convert all ancestor auto popovers to manual, not just those that were closed by older browser behaviors. This prevents the popover="auto" behavior from interfering with the native <dialog> focus trap.

Changes:

• Modified the popover selector in dialog_helper.ts from [popover]:not(:popover-open) to [popover] to handle both open and closed ancestor popovers
• Updated comments to explain both the old browser bug and the new focus trap interference issue
• Added clarifying comment about why showPopover() is always needed after converting to manual

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
app/components/primer/dialog_helper.ts	Broadened the popover workaround to convert all ancestor auto popovers to manual, preventing focus trap interference
.changeset/metal-dots-attend.md	Added changeset entry documenting the bug fix

Comments suppressed due to low confidence (1)

app/components/primer/dialog_helper.ts:44

• The fix for the focus trap issue looks correct, but there's no test coverage for this specific behavior. The existing tests in test/system/alpha/action_menu_test.rb verify that dialogs open from action menus, but don't test the focus trap behavior when tabbing through elements.

Consider adding a system test that:

1. Opens a dialog from within a popover (e.g., from an ActionMenu)
2. Tabs through all focusable elements in the dialog
3. Verifies that focus remains trapped within the dialog and doesn't escape to the popover or document body

This would ensure the fix works as intended and prevent regression.

      // When a <dialog> is opened with showModal() from inside a popover with popover="auto",
      // there are two related issues:
      //
      // 1. In older browsers (e.g. Chrome 122), the "hide all popovers" algorithm runs when a
      //    top layer element opens, closing any ancestor popover. We must re-open those popovers.
      //    See https://github.com/whatwg/html/issues/9998,
      //    fixed by https://github.com/whatwg/html/pull/10116.
      //
      // 2. In newer browsers where the popover stays open, the popover="auto" behavior still
      //    interferes with the native <dialog> focus trap, causing focus to escape the dialog
      //    when tabbing past the last focusable element. Converting the popover to "manual"
      //    prevents this interference.
      //
      // In both cases, the fix is the same: convert ancestor auto popovers to manual.
      let node: HTMLElement | null | undefined = button
      let fixed = false
      while (node) {
        node = node.parentElement?.closest('[popover]')
        if (node && node.popover === 'auto') {
          node.classList.add('dialog-inside-popover-fix')
          node.popover = 'manual'
          // Changing popover from "auto" to "manual" closes the popover,
          // so we need to re-show it regardless of whether it was previously open.
          node.showPopover()
          fixed = true
        }
      }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.