From e60376c94e7200e01df6717be6733215c41e1022 Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Thu, 31 Aug 2023 15:17:08 +1000 Subject: [PATCH 1/9] BlockSettingsMenu and DropdownMenu: Ensure only one BlockSettingsMenu is open at a time --- .../block-settings-dropdown.js | 41 +++++++++++++++++++ .../block-editor/src/store/private-actions.js | 13 ++++++ .../src/store/private-selectors.js | 10 +++++ packages/block-editor/src/store/reducer.js | 9 ++++ 4 files changed, 73 insertions(+) diff --git a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js index ce059d7720a9ff..355869a423d716 100644 --- a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js +++ b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js @@ -31,6 +31,7 @@ import BlockHTMLConvertButton from './block-html-convert-button'; import __unstableBlockSettingsMenuFirstItem from './block-settings-menu-first-item'; import BlockSettingsMenuControls from '../block-settings-menu-controls'; import { store as blockEditorStore } from '../../store'; +import { unlock } from '../../lock-unlock'; import { useShowHoveredOrFocusedGestures } from '../block-toolbar/utils'; const POPOVER_PROPS = { @@ -47,12 +48,15 @@ function CopyMenuItem( { blocks, onCopy, label } ) { } export function BlockSettingsDropdown( { + block, clientIds, __experimentalSelectBlock, children, __unstableDisplayLocation, ...props } ) { + // Get the client id of the current block for this menu, if one is set. + const currentClientId = block?.clientId; const blockClientIds = Array.isArray( clientIds ) ? clientIds : [ clientIds ]; @@ -102,6 +106,19 @@ export function BlockSettingsDropdown( { const { getBlockOrder, getSelectedBlockClientIds } = useSelect( blockEditorStore ); + const { openedBlockSettingsMenu } = useSelect( ( select ) => { + const { getOpenedBlockSettingsMenu } = unlock( + select( blockEditorStore ) + ); + return { + openedBlockSettingsMenu: getOpenedBlockSettingsMenu(), + }; + }, [] ); + + const { setOpenedBlockSettingsMenu } = unlock( + useDispatch( blockEditorStore ) + ); + const shortcuts = useSelect( ( select ) => { const { getShortcutRepresentation } = select( keyboardShortcutsStore ); return { @@ -174,6 +191,28 @@ export function BlockSettingsDropdown( { const parentBlockIsSelected = selectedBlockClientIds?.includes( firstParentClientId ); + // Only override the isOpen prop if the current block is not the one that + // opened the menu. The logic here is to ensure that non-current + // block menus are automatically closed when a new block menu is opened. + // This is useful for cases where focus is not present in the current window. + // All other behavior of the drop down menu should be otherwise unaffected. + const isOpen = + ! currentClientId || openedBlockSettingsMenu === currentClientId + ? undefined + : false; + + const onToggle = useCallback( + ( localIsOpen ) => { + // When the current menu is opened, update the state to reflect + // the new current menu. This allows all other instances of the + // menu to close if they already open. + if ( localIsOpen ) { + setOpenedBlockSettingsMenu( currentClientId ); + } + }, + [ currentClientId, setOpenedBlockSettingsMenu ] + ); + return ( Date: Thu, 31 Aug 2023 16:41:55 +1000 Subject: [PATCH 2/9] Update Readme files --- packages/components/src/dropdown-menu/README.md | 14 ++++++++++++++ packages/components/src/dropdown/README.md | 6 ++++++ 2 files changed, 20 insertions(+) diff --git a/packages/components/src/dropdown-menu/README.md b/packages/components/src/dropdown-menu/README.md index dcdb30997038eb..e36910533802af 100644 --- a/packages/components/src/dropdown-menu/README.md +++ b/packages/components/src/dropdown-menu/README.md @@ -171,6 +171,20 @@ A class name to apply to the dropdown menu's toggle element wrapper. - Required: No +### `isOpen`: `boolean` + +Control whether the dropdown is open or not. + +- Required: No + +### `onToggle`: `( willOpen: boolean ) => void` + +A callback invoked when the state of the popover changes from open to closed and vice versa. + +The callback receives a boolean as a parameter. If `true`, the popover will open. If `false`, the popover will close. + +- Required: No + #### `popoverProps`: `DropdownProps[ 'popoverProps' ]` Properties of `popoverProps` object will be passed as props to the nested `Popover` component. diff --git a/packages/components/src/dropdown/README.md b/packages/components/src/dropdown/README.md index 5bb3ec2c3b6e0e..fddb79a8ba9d55 100644 --- a/packages/components/src/dropdown/README.md +++ b/packages/components/src/dropdown/README.md @@ -74,6 +74,12 @@ Set this to customize the text that is shown in the dropdown's header when it is - Required: No +### `isOpen`: `boolean` + +Control whether the dropdown is open or not. + +- Required: No + ### `onClose`: `() => void` A callback invoked when the popover should be closed. From 5c9e6bd549f9ebfafb2c85f90fecf5c4a9910c6c Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Tue, 5 Sep 2023 14:32:49 +1000 Subject: [PATCH 3/9] Update reducer to fall back to undefined Co-authored-by: Ramon --- packages/block-editor/src/store/reducer.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index cdf966badf17d5..57344d4be74d2a 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -1914,9 +1914,8 @@ export function blockEditingModes( state = new Map(), action ) { } export function openedBlockSettingsMenu( state = null, action ) { - switch ( action.type ) { - case 'SET_OPENED_BLOCK_SETTINGS_MENU': - return action.clientId || null; + if ( 'SET_OPENED_BLOCK_SETTINGS_MENU' === action.type ) { + return action?.clientId; } return state; } From ded5fcc1ef74b581ab8694db9c96b1ba7476dec2 Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Thu, 7 Sep 2023 10:38:19 +1000 Subject: [PATCH 4/9] Rename isOpen to open --- .../block-settings-menu/block-settings-dropdown.js | 10 +++++----- packages/components/src/dropdown-menu/README.md | 14 -------------- packages/components/src/dropdown/README.md | 6 ------ 3 files changed, 5 insertions(+), 25 deletions(-) diff --git a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js index 355869a423d716..1bd6ce620b1ace 100644 --- a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js +++ b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js @@ -191,22 +191,22 @@ export function BlockSettingsDropdown( { const parentBlockIsSelected = selectedBlockClientIds?.includes( firstParentClientId ); - // Only override the isOpen prop if the current block is not the one that + // Only override the `open` prop if the current block is not the one that // opened the menu. The logic here is to ensure that non-current // block menus are automatically closed when a new block menu is opened. // This is useful for cases where focus is not present in the current window. // All other behavior of the drop down menu should be otherwise unaffected. - const isOpen = + const open = ! currentClientId || openedBlockSettingsMenu === currentClientId ? undefined : false; const onToggle = useCallback( - ( localIsOpen ) => { + ( localOpen ) => { // When the current menu is opened, update the state to reflect // the new current menu. This allows all other instances of the // menu to close if they already open. - if ( localIsOpen ) { + if ( localOpen ) { setOpenedBlockSettingsMenu( currentClientId ); } }, @@ -238,7 +238,7 @@ export function BlockSettingsDropdown( { label={ __( 'Options' ) } className="block-editor-block-settings-menu" popoverProps={ POPOVER_PROPS } - isOpen={ isOpen } + open={ open } onToggle={ onToggle } noIcons menuProps={ { diff --git a/packages/components/src/dropdown-menu/README.md b/packages/components/src/dropdown-menu/README.md index e36910533802af..dcdb30997038eb 100644 --- a/packages/components/src/dropdown-menu/README.md +++ b/packages/components/src/dropdown-menu/README.md @@ -171,20 +171,6 @@ A class name to apply to the dropdown menu's toggle element wrapper. - Required: No -### `isOpen`: `boolean` - -Control whether the dropdown is open or not. - -- Required: No - -### `onToggle`: `( willOpen: boolean ) => void` - -A callback invoked when the state of the popover changes from open to closed and vice versa. - -The callback receives a boolean as a parameter. If `true`, the popover will open. If `false`, the popover will close. - -- Required: No - #### `popoverProps`: `DropdownProps[ 'popoverProps' ]` Properties of `popoverProps` object will be passed as props to the nested `Popover` component. diff --git a/packages/components/src/dropdown/README.md b/packages/components/src/dropdown/README.md index fddb79a8ba9d55..5bb3ec2c3b6e0e 100644 --- a/packages/components/src/dropdown/README.md +++ b/packages/components/src/dropdown/README.md @@ -74,12 +74,6 @@ Set this to customize the text that is shown in the dropdown's header when it is - Required: No -### `isOpen`: `boolean` - -Control whether the dropdown is open or not. - -- Required: No - ### `onClose`: `() => void` A callback invoked when the popover should be closed. From 7ccdcf2ce8328d82d7c3b49e085bfa14ddb80195 Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Fri, 8 Sep 2023 10:57:34 +1000 Subject: [PATCH 5/9] Try switching to a controlled component --- .../block-settings-dropdown.js | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js index 1bd6ce620b1ace..01d78c49814c8c 100644 --- a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js +++ b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js @@ -191,26 +191,25 @@ export function BlockSettingsDropdown( { const parentBlockIsSelected = selectedBlockClientIds?.includes( firstParentClientId ); - // Only override the `open` prop if the current block is not the one that - // opened the menu. The logic here is to ensure that non-current - // block menus are automatically closed when a new block menu is opened. - // This is useful for cases where focus is not present in the current window. - // All other behavior of the drop down menu should be otherwise unaffected. - const open = - ! currentClientId || openedBlockSettingsMenu === currentClientId - ? undefined - : false; + // 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. + const open = ! currentClientId + ? undefined + : openedBlockSettingsMenu === currentClientId || false; const onToggle = useCallback( ( localOpen ) => { - // When the current menu is opened, update the state to reflect - // the new current menu. This allows all other instances of the - // menu to close if they already open. - if ( localOpen ) { + if ( localOpen && openedBlockSettingsMenu !== currentClientId ) { setOpenedBlockSettingsMenu( currentClientId ); + } else if ( + ! localOpen && + openedBlockSettingsMenu && + openedBlockSettingsMenu === currentClientId + ) { + setOpenedBlockSettingsMenu( undefined ); } }, - [ currentClientId, setOpenedBlockSettingsMenu ] + [ currentClientId, openedBlockSettingsMenu, setOpenedBlockSettingsMenu ] ); return ( From 5c51724eb5102650cbfbdb61082c79f03c8360fc Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Fri, 8 Sep 2023 14:38:51 +1000 Subject: [PATCH 6/9] Ensure menu is closed when blocks are inserted before/after via the menu, help stabilise test --- .../components/block-settings-menu/block-settings-dropdown.js | 2 ++ test/e2e/specs/editor/various/list-view.spec.js | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js index 01d78c49814c8c..ac72c55836f748 100644 --- a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js +++ b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js @@ -270,6 +270,7 @@ export function BlockSettingsDropdown( { canInsertDefaultBlock ) { event.preventDefault(); + setOpenedBlockSettingsMenu( undefined ); onInsertAfter(); } else if ( isMatch( @@ -279,6 +280,7 @@ export function BlockSettingsDropdown( { canInsertDefaultBlock ) { event.preventDefault(); + setOpenedBlockSettingsMenu( undefined ); onInsertBefore(); } }, diff --git a/test/e2e/specs/editor/various/list-view.spec.js b/test/e2e/specs/editor/various/list-view.spec.js index 130bd9b7952fde..e0ac030d1fd24b 100644 --- a/test/e2e/specs/editor/various/list-view.spec.js +++ b/test/e2e/specs/editor/various/list-view.spec.js @@ -784,6 +784,10 @@ test.describe( 'List View', () => { ).toBeHidden(); await optionsForFileToggle.click(); + await expect( + optionsForFileMenu, + 'Pressing Space should also open the menu dropdown' + ).toBeVisible(); await pageUtils.pressKeys( 'access+z' ); // Keyboard shortcut for Delete. await expect .poll( From c59e97405aaecdaa7ca1a195c63bfeca1c776a8f Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Tue, 12 Sep 2023 14:36:43 +1000 Subject: [PATCH 7/9] Simplify useSelect Co-authored by: Ramon --- .../block-settings-menu/block-settings-dropdown.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js index ac72c55836f748..9a50b69250afff 100644 --- a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js +++ b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js @@ -106,14 +106,11 @@ export function BlockSettingsDropdown( { const { getBlockOrder, getSelectedBlockClientIds } = useSelect( blockEditorStore ); - const { openedBlockSettingsMenu } = useSelect( ( select ) => { - const { getOpenedBlockSettingsMenu } = unlock( - select( blockEditorStore ) - ); - return { - openedBlockSettingsMenu: getOpenedBlockSettingsMenu(), - }; - }, [] ); + const openedBlockSettingsMenu = useSelect( + ( select ) => + unlock( select( blockEditorStore ) ).getOpenedBlockSettingsMenu(), + [] + ); const { setOpenedBlockSettingsMenu } = unlock( useDispatch( blockEditorStore ) From ba4579fa93b72c5583c6e4e5ecfd5b828cacd869 Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Wed, 13 Sep 2023 16:29:54 +1000 Subject: [PATCH 8/9] Add an explanatory comment describing the current solution as a temporary solution --- .../block-settings-menu/block-settings-dropdown.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js index 9a50b69250afff..bb8e00d0c11db9 100644 --- a/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js +++ b/packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js @@ -190,6 +190,11 @@ export function BlockSettingsDropdown( { // 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. + // This is a temporary solution to work around an issue with `onFocusOutside` + // where it does not allow a dropdown to be closed if focus was never within + // the dropdown to begin with. Examples include a user either CMD+Clicking or + // right clicking into an inactive window. + // See: https://github.com/WordPress/gutenberg/pull/54083 const open = ! currentClientId ? undefined : openedBlockSettingsMenu === currentClientId || false; From 13c42eda4882d78d3ac15ccf3783c65f15a231af Mon Sep 17 00:00:00 2001 From: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Date: Thu, 14 Sep 2023 09:52:56 +1000 Subject: [PATCH 9/9] Update action, reducer, and selectors for consistency, add tests --- .../block-editor/src/store/private-actions.js | 4 +-- .../src/store/private-selectors.js | 2 +- packages/block-editor/src/store/reducer.js | 10 +++++++- .../src/store/test/private-actions.js | 17 +++++++++++++ .../block-editor/src/store/test/reducer.js | 25 +++++++++++++++++++ 5 files changed, 54 insertions(+), 4 deletions(-) diff --git a/packages/block-editor/src/store/private-actions.js b/packages/block-editor/src/store/private-actions.js index e2e75768ae63b4..cc3ac41983c8cc 100644 --- a/packages/block-editor/src/store/private-actions.js +++ b/packages/block-editor/src/store/private-actions.js @@ -263,10 +263,10 @@ export function setBlockRemovalRules( rules = false ) { /** * Sets the client ID of the block settings menu that is currently open. * - * @param {string} clientId The block client ID. + * @param {?string} clientId The block client ID. * @return {Object} Action object. */ -export function setOpenedBlockSettingsMenu( clientId = '' ) { +export function setOpenedBlockSettingsMenu( clientId ) { return { type: 'SET_OPENED_BLOCK_SETTINGS_MENU', clientId, diff --git a/packages/block-editor/src/store/private-selectors.js b/packages/block-editor/src/store/private-selectors.js index 5377033ccaf6a5..2dc5ad8360fa33 100644 --- a/packages/block-editor/src/store/private-selectors.js +++ b/packages/block-editor/src/store/private-selectors.js @@ -144,7 +144,7 @@ export function getBlockRemovalRules( state ) { * 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. + * @return {string|null} The client ID of the block menu that is currently open. */ export function getOpenedBlockSettingsMenu( state ) { return state.openedBlockSettingsMenu; diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 57344d4be74d2a..4d1713ebcd0e0f 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -1913,9 +1913,17 @@ export function blockEditingModes( state = new Map(), action ) { return state; } +/** + * Reducer returning the clientId of the block settings menu that is currently open. + * + * @param {string|null} state Current state. + * @param {Object} action Dispatched action. + * + * @return {string|null} Updated state. + */ export function openedBlockSettingsMenu( state = null, action ) { if ( 'SET_OPENED_BLOCK_SETTINGS_MENU' === action.type ) { - return action?.clientId; + return action?.clientId ?? null; } return state; } diff --git a/packages/block-editor/src/store/test/private-actions.js b/packages/block-editor/src/store/test/private-actions.js index 6a9593493fbc4d..5763cb382937b2 100644 --- a/packages/block-editor/src/store/test/private-actions.js +++ b/packages/block-editor/src/store/test/private-actions.js @@ -5,6 +5,7 @@ import { hideBlockInterface, showBlockInterface, __experimentalUpdateSettings, + setOpenedBlockSettingsMenu, } from '../private-actions'; describe( 'private actions', () => { @@ -78,4 +79,20 @@ describe( 'private actions', () => { } ); } ); } ); + + describe( 'setOpenedBlockSettingsMenu', () => { + it( 'should return the SET_OPENED_BLOCK_SETTINGS_MENU action', () => { + expect( setOpenedBlockSettingsMenu() ).toEqual( { + clientId: undefined, + type: 'SET_OPENED_BLOCK_SETTINGS_MENU', + } ); + } ); + + it( 'should return the SET_OPENED_BLOCK_SETTINGS_MENU action with client id if provided', () => { + expect( setOpenedBlockSettingsMenu( 'abcd' ) ).toEqual( { + clientId: 'abcd', + type: 'SET_OPENED_BLOCK_SETTINGS_MENU', + } ); + } ); + } ); } ); diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index 9d886f9aa37bbc..8c82d1c3092b16 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -33,6 +33,7 @@ import { lastBlockAttributesChange, lastBlockInserted, blockEditingModes, + openedBlockSettingsMenu, } from '../reducer'; const noop = () => {}; @@ -3415,4 +3416,28 @@ describe( 'state', () => { ); } ); } ); + + describe( 'openedBlockSettingsMenu', () => { + it( 'should return null by default', () => { + expect( openedBlockSettingsMenu( undefined, {} ) ).toBe( null ); + } ); + + it( 'should set client id for opened block settings menu', () => { + const state = openedBlockSettingsMenu( null, { + type: 'SET_OPENED_BLOCK_SETTINGS_MENU', + clientId: '14501cc2-90a6-4f52-aa36-ab6e896135d1', + } ); + expect( state ).toBe( '14501cc2-90a6-4f52-aa36-ab6e896135d1' ); + } ); + + it( 'should clear the state when no client id is passed', () => { + const state = openedBlockSettingsMenu( + '14501cc2-90a6-4f52-aa36-ab6e896135d1', + { + type: 'SET_OPENED_BLOCK_SETTINGS_MENU', + } + ); + expect( state ).toBe( null ); + } ); + } ); } );