Skip to content

Commit

Permalink
BlockSettingsMenu: Ensure only one block settings menu is open at a t…
Browse files Browse the repository at this point in the history
…ime (#54083)

* BlockSettingsMenu and DropdownMenu: Ensure only one BlockSettingsMenu is open at a time

* Update Readme files

* Update reducer to fall back to undefined

Co-authored-by: Ramon <ramonjd@users.noreply.github.com>

* Rename isOpen to open

* Try switching to a controlled component

* Ensure menu is closed when blocks are inserted before/after via the menu, help stabilise test

* Simplify useSelect

Co-authored by: Ramon <ramonjd@users.noreply.github.com>

* Add an explanatory comment describing the current solution as a temporary solution

* Update action, reducer, and selectors for consistency, add tests

---------

Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
  • Loading branch information
3 people authored Sep 14, 2023
1 parent fec5ffc commit 21b6a37
Show file tree
Hide file tree
Showing 7 changed files with 129 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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 ];
Expand Down Expand Up @@ -102,6 +106,16 @@ export function BlockSettingsDropdown( {
const { getBlockOrder, getSelectedBlockClientIds } =
useSelect( blockEditorStore );

const openedBlockSettingsMenu = useSelect(
( select ) =>
unlock( select( blockEditorStore ) ).getOpenedBlockSettingsMenu(),
[]
);

const { setOpenedBlockSettingsMenu } = unlock(
useDispatch( blockEditorStore )
);

const shortcuts = useSelect( ( select ) => {
const { getShortcutRepresentation } = select( keyboardShortcutsStore );
return {
Expand Down Expand Up @@ -174,6 +188,32 @@ export function BlockSettingsDropdown( {
const parentBlockIsSelected =
selectedBlockClientIds?.includes( firstParentClientId );

// 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;

const onToggle = useCallback(
( localOpen ) => {
if ( localOpen && openedBlockSettingsMenu !== currentClientId ) {
setOpenedBlockSettingsMenu( currentClientId );
} else if (
! localOpen &&
openedBlockSettingsMenu &&
openedBlockSettingsMenu === currentClientId
) {
setOpenedBlockSettingsMenu( undefined );
}
},
[ currentClientId, openedBlockSettingsMenu, setOpenedBlockSettingsMenu ]
);

return (
<BlockActions
clientIds={ clientIds }
Expand All @@ -199,6 +239,8 @@ export function BlockSettingsDropdown( {
label={ __( 'Options' ) }
className="block-editor-block-settings-menu"
popoverProps={ POPOVER_PROPS }
open={ open }
onToggle={ onToggle }
noIcons
menuProps={ {
/**
Expand Down Expand Up @@ -230,6 +272,7 @@ export function BlockSettingsDropdown( {
canInsertDefaultBlock
) {
event.preventDefault();
setOpenedBlockSettingsMenu( undefined );
onInsertAfter();
} else if (
isMatch(
Expand All @@ -239,6 +282,7 @@ export function BlockSettingsDropdown( {
canInsertDefaultBlock
) {
event.preventDefault();
setOpenedBlockSettingsMenu( undefined );
onInsertBefore();
}
},
Expand Down
13 changes: 13 additions & 0 deletions packages/block-editor/src/store/private-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,16 @@ export function setBlockRemovalRules( rules = false ) {
rules,
};
}

/**
* Sets the client ID of the block settings menu that is currently open.
*
* @param {?string} clientId The block client ID.
* @return {Object} Action object.
*/
export function setOpenedBlockSettingsMenu( clientId ) {
return {
type: 'SET_OPENED_BLOCK_SETTINGS_MENU',
clientId,
};
}
10 changes: 10 additions & 0 deletions packages/block-editor/src/store/private-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,13 @@ export function getRemovalPromptData( state ) {
export function getBlockRemovalRules( state ) {
return state.blockRemovalRules;
}

/**
* Returns the client ID of the block settings menu that is currently open.
*
* @param {Object} state Global application state.
* @return {string|null} The client ID of the block menu that is currently open.
*/
export function getOpenedBlockSettingsMenu( state ) {
return state.openedBlockSettingsMenu;
}
16 changes: 16 additions & 0 deletions packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1913,6 +1913,21 @@ 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 ?? null;
}
return state;
}

const combinedReducers = combineReducers( {
blocks,
isTyping,
Expand All @@ -1938,6 +1953,7 @@ const combinedReducers = combineReducers( {
blockEditingModes,
removalPromptData,
blockRemovalRules,
openedBlockSettingsMenu,
} );

function withAutomaticChangeReset( reducer ) {
Expand Down
17 changes: 17 additions & 0 deletions packages/block-editor/src/store/test/private-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
hideBlockInterface,
showBlockInterface,
__experimentalUpdateSettings,
setOpenedBlockSettingsMenu,
} from '../private-actions';

describe( 'private actions', () => {
Expand Down Expand Up @@ -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',
} );
} );
} );
} );
25 changes: 25 additions & 0 deletions packages/block-editor/src/store/test/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
lastBlockAttributesChange,
lastBlockInserted,
blockEditingModes,
openedBlockSettingsMenu,
} from '../reducer';

const noop = () => {};
Expand Down Expand Up @@ -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 );
} );
} );
} );
4 changes: 4 additions & 0 deletions test/e2e/specs/editor/various/list-view.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 21b6a37

Please sign in to comment.