Popover Component: Add nested popover detection with context management#74179
Popover Component: Add nested popover detection with context management#74179SirLouen wants to merge 4 commits intoWordPress:trunkfrom
Conversation
|
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. |
ramonjd
left a comment
There was a problem hiding this comment.
Nice catch @SirLouen and thanks for getting the PR up.
I did some manual testing and it fixes the custom color popover bug, and preserves the parent behaviour from #72376 🎉
I mostly left questions, which might not help, mainly to help my own understanding 😄
I'd lean on @tellthemachines and @mirka for a more learned opinion!
|
|
||
| let current: Element | null = closestPopover; | ||
| while ( current ) { | ||
| const parentId = current.getAttribute( PARENT_POPOVER_ID_ATTR ); |
There was a problem hiding this comment.
Just checking: here we look whether the reference element is inside the parent's floating element. Is that right?
Might be a dumb question, but could a nested popover's trigger ever be outside a parent's floating element, in the DOM at least?
There was a problem hiding this comment.
Yep, I feel it's quite complex to catch all popover scenarios. I'm not sure if we should be now hunting for each of them, or just wait for them to appear (just the one referenced in the report to this PR). The thing is that I don't have a massive knowledge of all the popover cases in GB, so I'm not sure if there could be additional patterns that could trigger this (and looking at #72376, I doubt that anyone has)
1375000 to
2f2f1a8
Compare
What?
Closes: #74154
When there is a nested popover that should be closed along with its parent on blur, it's not happening.
The simplest example is when clicking on the Color Palette (select colors on a paragraph block, for example).
Why?
The culprit is here:
Commit: 8501118
PR: #72376
Because of this PR, it's generating a side effect. When there is a parent/child connected relationship, both should close. In the case of PR #72376 the filter popover should be disconnected, hence, it should not close the popover.
Testing Instructions
There are two scenarios:
And at the same time
Screenshots or screencast
74154.mp4