Skip to content

Menu: Return focus to the trigger button after closing a Modal opened from menu item - #80735

Open
yashjawale wants to merge 11 commits into
WordPress:trunkfrom
yashjawale:fix/menu-focus-80734
Open

Menu: Return focus to the trigger button after closing a Modal opened from menu item#80735
yashjawale wants to merge 11 commits into
WordPress:trunkfrom
yashjawale:fix/menu-focus-80734

Conversation

@yashjawale

@yashjawale yashjawale commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What?

Closes #80734

This PR fixes focus issue after closing any popovers that appear from triggers in menu.

Why?

This PR restructures the Menu popover so Ariakit renders MenuMotionRoot directly, making it aware of the CSS transition duration and fixing focus restoration timing.

How?

In popover.tsx Spread Ariakit's htmlProps (except children) onto MenuMotionRoot instead of MenuSurface and then place children inside MenuSurface explicitly.
In style.ts, change MenuMotionRoot selectors from :has(> MenuSurface[data-enter]) to &[data-enter] etc., since data-* attributes now live on MenuMotionRoot.

Testing Instructions

  1. Open a post in the editor.
  2. Open the Post actions menu (top‑right "…" button).
  3. Click any item that opens a modal (e.g. Duplicate, Rename, Trash).
  4. Close the modal (Esc or click "Cancel").
  5. Verify focus returns to the Post actions trigger button (Tab after closing to confirm).
  6. Repeat with keyboard only: Tab to the trigger, Space to open, arrow‑keys to an item, Enter to select, Esc to close the modal — focus should land back on the trigger. (Same applicable with MacOS Voiceover keys control + option + Left/Right arrows & Space for selecting the options)

Screenshots or screencast

Screen.Recording.2026-07-27.at.5.29.04.PM.mov

Use of AI Tools

Assisted by DeepSeek V4 Flash via Opencode
All changes reviewed & tested manually.

@github-actions github-actions Bot added the [Package] Components /packages/components label Jul 27, 2026
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yashjawale <yashjawale@git.wordpress.org>
Co-authored-by: aduth <aduth@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@aduth

aduth commented Jul 27, 2026

Copy link
Copy Markdown
Member

Is it possible to add regression test coverage for this? Something in packages/components/src/menu/test/index.tsx that fails in trunk and passes on this branch.

@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended Backport to WP 7.1 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jul 28, 2026
@yashjawale

Copy link
Copy Markdown
Contributor Author

Updated to include a test case for checking focused element after modal is closed when opened from menu.

Checked by running the case on trunk & current branch.

On trunk:

  ● Menu › pointer and keyboard interactions › should return focus to the trigger button after closing a modal opened from a menu item

    expect(element).toHaveFocus()

    Expected element with focus:
      <button aria-expanded="false" aria-haspopup="menu" class="components-button is-next-40px-default-size" id=":r1l:" type="button">Open dropdown</button>
    Received element with focus:
      <body class="" style=""><div style=""><button aria-expanded="false" aria-haspopup="menu" class="components-button is-next-40px-default-size" id=":r1l:" type="button">Open dropdown</button></div></body>

On current branch:

 PASS  packages/components/src/menu/test/index.tsx (5.38 s)
  Menu
    ✓ should follow the WAI-ARIA spec (481 ms)
    pointer and keyboard interactions
      ✓ should open and focus the menu when clicking the trigger (134 ms)
      ✓ should open and focus the first item when pressing the arrow down key on the trigger (76 ms)
      ✓ should open when pressing the space key on the trigger (80 ms)
      ✓ should close when pressing the escape key (103 ms)
      ✓ should return focus to the trigger button after closing a modal opened from a menu item (163 ms)

With the test I noticed another thing, since the transition-* properties were inside @media not (prefers-reduced-motion), the fix only worked for users with animations enabled since Ariakit relied on CSS transition duration to detect animation & delay unmounting & the test runs were also flaky.

I've moved them outside so Ariakit detects non-zero duration regardless of motion preference, which delays the unmounting of popover in time, so useFocusReturn captures the correct focused element before the popover is removed from the DOM. This also solved the flaky test issue & now the trigger is correctly focused even when prefers-reduced-motion is active.

For users with reduced motion, there still isn't any visual animation for this component nor do they see the popover showing up longer than it did before.

@aduth

aduth commented Jul 28, 2026

Copy link
Copy Markdown
Member

With the test I noticed another thing, since the transition-* properties were inside @media not (prefers-reduced-motion), the fix only worked for users with animations enabled since Ariakit relied on CSS transition duration to detect animation & delay unmounting & the test runs were also flaky.

That's great that you were able to find an additional issue through the tests! 🎉

I'll be review this one shortly.

@aduth

aduth commented Jul 28, 2026

Copy link
Copy Markdown
Member

One thing I'd worry about is that this seems to effectively undo what @ciampo described as an intentional decision in #77460 , which may bring its own separate issues?

Related context: mui/base-ui#4640 — describes a scroll-lock handoff bug when opening another modal overlay while an Ariakit Menu is still closing; one mitigation is moving panel motion off the Ariakit root so scroll-lock cleanup is not delayed by leave transitions on that element.

(emphasis mine for the change that's being undone here)

@aduth

aduth commented Jul 28, 2026

Copy link
Copy Markdown
Member

It'd be good to understand what the practical difference would be between this and just reverting #77460.

@ciampo has limited availability until next week but it'd be good to get his feedback, or also @mirka as a reviewer of #77460.

Functionally, this seems fine, and I confirmed that it does fix the bug. The added regression coverage will be nice to have. But since it seems like this largely backtracks what #77460 was aimed at implementing, I'd like to better understand how these two pull requests interact. If I had to choose between functioning focus return and a brief delay in scroll-lock clean-up (if that's the actual issue, unclear), then I'd opt for the focus return for WordPress 7.1.

@ciampo

ciampo commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

I confirm that this PR essentially un-does #77460 , which was a deliberate fix.

It would be interesting to understand if we can achieve a similar fix to that of #77460 without its drawback. I don't have much time on my hands, but we may want to look at the Ariakit update PR, too, in the chance that it allows un-doing #77460 without re-introducing the regression.

Alternatively, we could add some imperative code in Menu to handle the focus return manually (potentially focus store.disclosureElement (the trigger) as the menu changes to closed, before the modal’s mount ref captures document.activeElement).

@yashjawale

yashjawale commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Got to learn a lot while digging into this...

I checked out to commit 0f0ca8f which was before #77460 was merged. And can confirm that the focus bug doesn't appear there.

However, it returns back when using reduced motion settings. I think we'll still need the change of moving transition-* properties out of @media not (prefers-reduced-motion) for the focus to work correctly on reduced motion settings & in the unit test case.

Will continue to check out other approaches & I'll post an update as soon as I find something.

@yashjawale

Copy link
Copy Markdown
Contributor Author

Updated with alternative approach of handling focus return manually.
From my testing it appears to be working nicely too.

@ciampo

ciampo commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I'm still not convinced about this approach, which would cause a new regression when returning focus on non-modal popovers.

I'm proposing an alternative approach in #81164, I'll soon mark it as ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backport to WP 7.1 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Projects

Status: 🔎 Needs Review

Development

Successfully merging this pull request may close these issues.

Menu: Focus is not returned to the trigger button after closing a Modal opened from a menu item

4 participants