Fix Popover closing unexpectedly when a Menu inside it is closed. #72376
Conversation
tellthemachines
added
[Type] Bug
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Just checking, should the base of this PR be |
I deliberately set it against trunk that so this fix could be merged independently of the media modalPR, because it's still in early stages. |
There was a problem hiding this comment.
Can you add a minimum repro case in a unit test or a temporary Storybook story? With that, I want us to determine if this should be patched at the Popover level, rather than in Dropdown. (If the same issue happens with a "Menu inside a Modal inside a Popover", then maybe it should be fixed in Popover.)
|
@mirka I just added a couple stories testing Menu inside both Popover and Dropdown, and you're correct that the problem is with Popover 😅 so I'll update the fix. Thanks for pointing it out! |
tellthemachines
changed the title
|
That failing e2e looks like it could be related, but I've been unable to reproduce it locally so far 😕 Update: I have reproed it now, had to run the build task 🤦 |
|
Flaky tests detected in eabb81c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18702089247
|
|
I checked the new stories and added a Kapture.2025-10-20.at.11.52.28.mp4Keen to hear what @mirka thinks! |
| floatingElement?.contains( blurTarget ); | ||
| // Only proceed if the blur is actually from this popover | ||
| if ( ! isBlurFromThisPopover ) { | ||
| return; |
There was a problem hiding this comment.
Just curious, what should happen to blur events that aren't from this popover? (if anything)
I was just asking myself if the logic should wrap the callbacks. E.g.,
// Check if blur is from this popover's reference element or its floating content
const isBlurFromThisPopover =
( referenceElement &&
'contains' in referenceElement &&
referenceElement.contains( blurTarget ) ) ||
( floatingElement &&
floatingElement.contains( blurTarget ) );
// Only proceed if the blur is actually from this popover
if ( isBlurFromThisPopover ) {
if ( onFocusOutside ) {
onFocusOutside( event );
} else if ( onClose ) {
onClose();
}
}But if there's no need to, then ignore me.
There was a problem hiding this comment.
I'm working on the assumption that if a blur happens anywhere unrelated to the popover, the popover shouldn't react to it 😅
asking myself if the logic should wrap the callbacks
It should make no difference. I prefer the early return myself as it makes it easier to read.
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
I like the change here, nice work getting to the bottom of what was causing the issue!
I'm working on the assumption that if a blur happens anywhere unrelated to the popover, the popover shouldn't react to it 😅
I think that sounds like how it should work, too. The existing behaviour feels to me like the source of the bug, that is, the focus-outside was overreaching 😄
It's testing well for me in Storybook:
If I comment out the changes to popover/index.tsx then we can see the bug, where the dropdown unexpectedly closes:
2025-10-21.13.42.07.mp4
And with this PR applied, everything feels stable:
2025-10-21.13.43.18.mp4
LGTM!
|
Thanks for the reviews folks! I'll go ahead and remove the storybook stories because they were just for testing purposes. |
tellthemachines
force-pushed
the
fix/dropdown-closing-unexpectedly
branch
from
8781dd3 to
9c3c8e9
Compare
What?
Fixes issue noticed in testing #71944 . More context here.
Testing Instructions
The bug occurs when nesting a Menu inside a Modal inside a Dropdown, which is what #71944 does. You can test that PR to repro the bug:
With this patch applied, the modal should stay open when clicking outside the filters menu.
modal_closing.mp4