Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BlockSettingsMenu: Ensure only one block settings menu is open at a time #54083

Merged
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.
Comment on lines +191 to +192
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@andrewserong andrewserong Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the block of code that I'm suggesting to add some more comments about it being a temp solution..

// 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 &&
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Loading