Fix popover never closing when clicking on the toggle button (Button, Navigation Link, Post Date) #27406
Conversation
david-szabo97
added
[Type] Bug
david-szabo97
requested review from
ntsekouras,
jorgefilipecosta and
ellatrix
|
Size Change: +876 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
This fix makes sense to me -- thanks for working on it! The code appears to mostly solve the problem. I noticed a one issue though: Chrome, Edge, Safari, and IE11 work as expected. In firefox, however, the issue doesn't seem to be resolved.
|
|
Thanks for catching that! Looking into it. |
There was a problem hiding this comment.
Tested and works well - thanks 👍 !
Regarding Firefox it seems to be Button and focus related in some browsers. This issue is related: #23886
I think we can merge this (with green CI) and iterate if a solution can be found about it.
|
@ntsekouras Just pushed a commit which seems to work consistently in all browsers on OSX. I implemented it as a hook and just to test it in more context I used the hook in the If this implementation seems good to go, then we can iterate more on it by extracting the hook into util and use it in all the places. |
|
I tested and works well in Firefox as well now! 👍
This will be great, yes! I'd love @ellatrix 's opinion on this though. She has great experience on related stuff :) |
|
Is this a problem in other popovers? If not, why not? And if yes, shouldn't we fix it generally? It seems strange to fix this on the implementation side because most popovers will have a toggle button. |
|
Moving the timeout into the popover component would be a good general fix. I'm not sure if it would make any regression. |
|
Still, I don't think it's a good idea. Toggling and the popover is two different things. I think the hook is a good way to make this happen. But we need to implement it in all the places. I think this PR should just care about the button component for now. Then we can iterate on it. What do you think about moving the hook to |
david-szabo97
force-pushed
the
fix/button-component-link-toggling
branch
from
b7368ea
to
855c38f
Compare
|
Why is |
david-szabo97
force-pushed
the
fix/button-component-link-toggling
branch
from
855c38f
to
67c1d69
Compare
|
@ellatrix Hopefully this clears it up for you, let me know!
You can see the same implementation in the gutenberg/packages/components/src/dropdown/index.js Lines 58 to 73 in 149502c I'm planning to reuse the new hook in the |
david-szabo97
requested review from
vindl,
noahtallen,
mattwiebe,
Addison-Stavlo,
Copons and
jeyip
|
It will definitely be nice to see this resolved in the dropdown component in general.
We noticed something similar when adding the Dropdown in the template part toolbar. @jeyip found that in FireFox clicking the toggle button to close the dropdown caused it to both close and re-open. It seems the change in focus would close it, but the mouse-up would force it to re-open. To keep it simple we ended up going with a temporary simple and sub-optimal solution of disabling the button while the dropdown is open, meaning it always closes by loss of focus and not the actual button toggle:
Obviously, it will be better to not have to disable the button to avoid this. 🤣 |
|
Moved this into |
david-szabo97
added
[Block] Post Date
david-szabo97
changed the title
|
|
||
| Reference to the element that's used to toggle this popover. Used to prevent `onClose` firing if focus moves to the toggle. Which usually means we clicked on the toggle. Without this, popover would reopen everytime we click on the toggle. | ||
|
|
||
| When `toggleRef` is provided, then `onFocusOutside` prop is ignored. |
There was a problem hiding this comment.
I can't remember the reason for that to be honest. Looking at it today it totally makes sense to call this callback as well. Added it
| @@ -143,6 +143,15 @@ If you need the `DOMRect` object i.e., the position of popover to be calculated | |||
| - Type: `Function` | |||
| - Required: No | |||
|
|
|||
| ### toggleRef | |||
There was a problem hiding this comment.
If we try this, it would be good to be experimental or unstable at first.
There was a problem hiding this comment.
Renamed it to experimental
| @@ -127,6 +128,7 @@ function URLPicker( { | |||
| position="bottom center" | |||
| onClose={ () => setIsURLPickerOpen( false ) } | |||
| anchorRef={ anchorRef?.current } | |||
| toggleRef={ linkToolbarButtonRef?.current } | |||
There was a problem hiding this comment.
When will linkToolbarButtonRef not be an object?
There was a problem hiding this comment.
Also, it seems better to pass the reference instead of its value?
There was a problem hiding this comment.
I followed how the anchorRef is implemented in this situation. It would be confusing that we are passing the actual value for anchorRef but we are passing the reference for toggleRef. I prefer passing the reference as well, but I wanted to avoid even more confusion.
|
I'd been looking into the issue here since some days ago at which time this PR looked a little stale. Now I've made #30397 as an alternate that fixes the toggle logic in |
Description
Fix three instances of popover toggling:
Buttoncomponent:Linktoolbar buttonNavigation Linkcomponent:Linktoolbar buttonPost Datecomponent:Edit datetoolbar buttonIntroduces a new prop for
Popovercomponent:toggleRef. WhentoggleRefis passed, thenonFocusOutsideis ignored and it is handled inside the component.How has this been tested?
Button component
ButtonsblockNavigation link component
Navigation (horizontal)blockStart EmptyPage LinkblockPost Date component
Post DateblockScreenshots
before
after
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: