BlockSettingsMenu: Ensure only one block settings menu is open at a time #54083
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +2.11 kB (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
|
Just added a couple of folks as reviewers — for context with this PR, my idea is to come up with a fix for the block settings menu so that we can auto-close menus that should not be open (for situations not covered by While it looked like the v2 of the DropDown menu was quite promising to resolve this problem (#51400), since there have been some other blockers with that work, I wondered if something like this PR could work in the shorter-term? |
andrewserong
force-pushed
the
update/block-settings-menu-to-ensure-only-one-menu-is-open
branch
from
511dacb
to
94b4025
Compare
|
Flaky tests detected in 5c51724. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6153433212
|
andrewserong
force-pushed
the
update/block-settings-menu-to-ensure-only-one-menu-is-open
branch
from
c3b5092
to
0d51d0d
Compare
|
Thanks for reviewing, Ramon!
Yes, I'd love to get some feedback from components folks to see if this is a good direction to resolve this issue in the shorter-term. I think it's probably a fairly pragmatic way for us to go, so that we can unblock the work on right-click in the list view while explorations into refactored dropdown menus continue. |
|
Hey folks, sorry for the delay in answering here — I've been catching up with the notification queue after some extended AFK I'll try have a look at this PR tomorrow and share my thoughts |
Thanks Marco, and not a worry at all. I assumed you must have a pretty big backlog of notifications! |
andrewserong
force-pushed
the
update/block-settings-menu-to-ensure-only-one-menu-is-open
branch
from
01d31d2
to
19d73b8
Compare
There was a problem hiding this comment.
So! From what I gather, this PR is proposing to add support to DropdownMenu and Dropdown so that they be used in controlled mode (while so far they could only be used in uncontrolled mode).
That sounds reasonable! But I'd propose a couple of tweaks to the implementation:
- to match general prop naming convention a bit better, let's rename the
isOpenprop toopen - let's add a third prop to both
DropdownandDropdownMenucalleddefaultOpen— it will be used to set the dropdown's initial "open" state when the components are used in uncontrolled mode - internally in
Dropdownwe can use theuseControlledValuehook which should take care automatically of the controlled/uncontrolled logic (openmaps tovalue,onTogglemaps toonChange, anddefaultOpenmaps todefaultValue)
Let me know if that makes sense and/or if you struggle to make this work!
|
Thanks for the feedback @ciampo! I've pushed the rename from The other feedback sounded really good to me, but unfortunately I did wind up struggling to make it work 😅. Please excuse the wall of text that follows, but hopefully it captures both my thinking and where I ran into some stumbling blocks 🙂
Not quite. This PR proposes that we can close the dropdown menu from the outside, but not fully control it. My objective wasn't really to use the menu in a fully controlled mode, but instead to ensure only one dropdown is open at a time, to work around the bug with DropdownMenu where Still, the controlled component idea is a compelling one! I've had a hack around, but unfortunately I haven't managed to get it working properly. I ran into a few issues:
2023-09-07.13.49.10.mp4Unfortunately, I've become stuck on that last issue, so I'm not feeling very confident about the fully controlled component approach, but I very well could be missing something obvious! To keep this PR in its current working form, I've pushed my commit trying out a controlled component to a separate throwaway PR over in #54236. The main commit I tried is in 67801dd. What do you think might be a good next step? Should we aim for a partially controlled component state as in this PR, or see if there's more we can do to debug the flickery issues in #54236? Alternately, happy to explore any other ideas! |
|
To demonstrate what I had in mind, I created this draft PR with a POC:
Hopefully this can help you to get on the right direction, but happy to discuss further in case it doesn't |
andrewserong
force-pushed
the
update/block-settings-menu-to-ensure-only-one-menu-is-open
branch
from
19f88d3
to
23d973f
Compare
Ah, perfect, that unlocks it, thanks so much @ciampo! The bit that I'd missed in my exploration was the removal of the Since this PR updates both the components package as well as the block-editor package, I'd be happy for us to use #54257 for the components updates if that helps keep things neater? |
andrewserong
force-pushed
the
update/block-settings-menu-to-ensure-only-one-menu-is-open
branch
from
615d9f9
to
5c51724
Compare
andrewserong
changed the title
|
Alrighty, I've rebased this now that #54257 is merged, and it still seems to be testing well for me, so this should be ready for review now 🙂 Since this PR does add additional state in the block-editor store, I'll just do a wider ping to @WordPress/gutenberg-core for visibility, in case anyone has any objections to using state in this way. |
andrewserong
added
[Package] Block editor
packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
Show resolved
Hide resolved
Co-authored by: Ramon <[email protected]>
|
Thanks for the approving review @ramonjd! 🙇 I'll leave this PR open overnight to see if we get any other feedback from folks, and if not, will merge this in tomorrow. |
|
I'm wondering, this seems like an issue that impacts all dropdowns (basically all of our usage of "focus outside to close"), so is there a generic solution to this? like for all dropdowns? |
|
Like: could a fix here be: when you open any dropdown, close all others? |
The solution proposed in this PR is more of a temporary solution — ideally we'd be able to achieve this in the next version of We will also want to make sure we fully understand the desired behaviour — from what I understand, it looks like we'd want all In case, for any reasons, we couldn't manage to successfully wrap up the new version of |
|
Thanks for the context @ciampo that plan works for me. There might be a case where we want to retain multiple dropdowns (but to be confirmed): "nested dropdowns" |
| // When a currentClientId is in use, treat the menu as a controlled component. | ||
| // This ensures that only one block settings menu is open at a time. |
There was a problem hiding this comment.
My only comment at this point is: wouldn't be simpler to always use DropdownMenu in a controlled way? Something like
const open = currentClientId !== undefined && openedBlockSettingsMenu === currentClientId;
I believe that the logic would just be simpler, and the behaviour of the DropdownMenu component more predictable (furthermore, React usually warns when this pattern is applied to input elements)
There was a problem hiding this comment.
It'd be simpler in this component, but due to current usage of the BlockSettingsDropdown, a currentClientId isn't always available as a block object isn't always provided. So we currently need to support both controlled and uncontrolled in this component.
In practice, at the moment, the List View provides a block object, however the block settings menu in the toolbar doesn't (it provides clientIds for the selected blocks instead, which isn't quite the same as the concept of the block when used in the list view), so if we switch to always controlling the component, we wind up breaking the dropdown in the editor canvas:
2023-09-13.10.34.46.mp4
Separately to this PR we could potentially look at refactoring other usage of the block settings dropdown to ensure a block object with clientId is always provided, and then we could have the component always operate in controlled mode.
For now, I'd prefer to try to contain the scope of this PR to the BlockSettingsMenu itself, so I think retaining this slightly complex ternary is probably a good step to go with for now, as it focuses on the main use case to solve which is with the behaviour in the list view.
There was a problem hiding this comment.
This is the block of code that I'm suggesting to add some more comments about it being a temp solution..
|
Thanks for the continued feedback @ciampo and @youknowriad (and for reviewing @aristath)!
Yes, since other efforts wound up encountering blockers, the goal here is to find a good enough temporary solution to unblock work on right click within the list view. My hope is that the change here isn't too intrusive, and with keeping the action and selector private, we can always remove this behaviour in a follow-up once there's a longer term solution in place for dropdown menus in general. For now, are there any objections to us merging this in as-is? If not, I'll look at landing this PR tomorrow. |
|
Thanks for the PR @andrewserong! I don't want to block the work here for the temp solution, but the first thing that come in mind, is what Riad also said about handling it in |
|
Thanks for taking a look @ntsekouras! Great idea about adding an extra comment. I've added some further explanation in ba4579f, along with a link to this PR. Happy to tweak the wording of course! |
| @@ -1913,6 +1913,13 @@ export function blockEditingModes( state = new Map(), action ) { | |||
| return state; | |||
| } | |||
|
|
|||
| export function openedBlockSettingsMenu( state = null, action ) { | |||
| if ( 'SET_OPENED_BLOCK_SETTINGS_MENU' === action.type ) { | |||
| return action?.clientId; | |||
There was a problem hiding this comment.
For better precision, did we mean to return the default state of null instead of undefined in that scenario?
| return action?.clientId; | |
| return action?.clientId ?? null; |
There was a problem hiding this comment.
Oh, good call, that helps give this reducer consistency with other reducers that track a client id or null 👍
| @@ -1913,6 +1913,13 @@ export function blockEditingModes( state = new Map(), action ) { | |||
| return state; | |||
| } | |||
|
|
|||
| export function openedBlockSettingsMenu( state = null, action ) { | |||
There was a problem hiding this comment.
The block editor reducer is quite thoroughly covered by unit tests. A couple of simple tests could be useful to cover this new reducer subtree.
| * Returns the client ID of the block settings menu that is currently open. | ||
| * | ||
| * @param {Object} state Global application state. | ||
| * @return {string|undefined} The client ID of the block menu that is currently open. |
There was a problem hiding this comment.
Did you mean null or undefined? The default value is null and currently we'll return undefined if a clientId is missing for some reason when dispatching the setOpenedBlockSettingsMenu() action (this feels like an inconsistent behavior on its own, so I'm commenting with it separately).
| * @param {string} clientId The block client ID. | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function setOpenedBlockSettingsMenu( clientId = '' ) { |
There was a problem hiding this comment.
Do we really mean to have the default clientId as an empty string? I can see the action is set up to handle undefined clientId and the reducer will also return a null, and those are all three different values representing "empty", which seems like an inconsistent behavior. Should we make it consistent and ultimately only go with either of them?
|
Thanks for the review @tyxla! I've updated the action, reducer, and selector for consistency so that if the passed in |
|
Latest changes LGTM. The tests cover the arg type changes 👍🏻 Also ran the branch manually - things work as per the last test. |
|
Excellent, thanks for confirming! I'll merge this in now, but happy to work on any follow-ups if there's feedback after the fact. Thanks again everyone! 🙇 |
andrewserong
deleted the
update/block-settings-menu-to-ensure-only-one-menu-is-open
branch
What?
Fixes #50988 and unblocks #50273.
Ensure only one BlockSettingsMenu is open at a time by adding state with the
clientIdof the block settings menu most recently opened. All other open block settings menus will close if they have aclientIdand it does not match the id of the menu currently open.Why?
This resolves an issue where multiple block settings menus can be open at one time if the current window does not have focus. While this sounds like an edge case, it's fairly common on Macs when users right click into a window that is not currently active, which is an issue that's revealed when we go to attempt to add in right-click support as in #50273. However, it's also an (albeit very subtle) issue for cases where a user CMD+clicks from an active window into an inactive one. This will fire a click into a window without ever shifting focus there, so the current
onFocusOutsidelogic isn't quite sufficient for the block settings menu.How?
If we introduce some state in the block editor store to keep track of the latest
clientIdof the most recently opened block settings menu, then we can auto-close any block settings menus that are already opened.onToggleandopento be passed down so that we can:openstate so that we can treat the dropdowns as controlled components.This PR updates the block editor package, the necessary updates to the
DropdownandDropdownMenucomponents were made in #54257.Testing Instructions
Note: while the issue in testing here is quite subtle, it's a good idea for us to try to fix it prior to introducing right click in #50273 as it's fairly common for folks to click between two browser windows — and (on a Mac at least) right click does not change focus between windows.
Testing Instructions for Keyboard
Screenshots or screencast
This is how it can look if a user has multiple block settings menus open (prior to this PR):