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.

Sign up for GitHub

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
merged 9 commits into from
Sep 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e60376c
BlockSettingsMenu and DropdownMenu: Ensure only one BlockSettingsMenu…
andrewserong Aug 31, 2023
17d8e0a
Update Readme files
andrewserong Aug 31, 2023
5c9e6bd
Update reducer to fall back to undefined
andrewserong Sep 5, 2023
ded5fcc
Rename isOpen to open
andrewserong Sep 7, 2023
7ccdcf2
Try switching to a controlled component
andrewserong Sep 8, 2023
5c51724
Ensure menu is closed when blocks are inserted before/after via the m…
andrewserong Sep 8, 2023
c59e974
Simplify useSelect
andrewserong Sep 12, 2023
ba4579f
Add an explanatory comment describing the current solution as a tempo…
andrewserong Sep 13, 2023
13c42ed
Update action, reducer, and selectors for consistency, add tests
andrewserong Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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 = '' ) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

return {
type: 'SET_OPENED_BLOCK_SETTINGS_MENU',
clientId,
};
}
10 changes: 10 additions & 0 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
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|undefined} The client ID of the block menu that is currently open.
Copy link
Member

Choose a reason for hiding this comment

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

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).

*/
export function getOpenedBlockSettingsMenu( state ) {
return state.openedBlockSettingsMenu;
}
8 changes: 8 additions & 0 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1913,6 +1913,13 @@ export function blockEditingModes( state = new Map(), action ) {
return 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;
Copy link
Member

Choose a reason for hiding this comment

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

For better precision, did we mean to return the default state of null instead of undefined in that scenario?

Suggested change
return action?.clientId;
return action?.clientId ?? null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good call, that helps give this reducer consistency with other reducers that track a client id or null 👍

}
return state;
}

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

function withAutomaticChangeReset( reducer ) {
Expand Down
4 changes: 4 additions & 0 deletions test/e2e/specs/editor/various/list-view.spec.js
View file
Open in desktop
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