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

Jump to bottom

[ListView] Allow deleting blocks using keyboard #50422

Merged
merged 5 commits into from
May 28, 2023
Merged

[ListView] Allow deleting blocks using keyboard #50422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e0b8130
Allow deleting blocks using keyboard from listview
kevin940726 May 4, 2023
d793d18
Update updateSelection instead
kevin940726 May 10, 2023
9431f32
Add support for kyeboard shortcut in the dropdown
kevin940726 May 11, 2023
9354a81
Only select block if there isn't any already
kevin940726 May 11, 2023
4274a5e
More things!
kevin940726 May 15, 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
122 changes: 81 additions & 41 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 @@ -16,7 +16,10 @@ import {
useRef,
} from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';
import { store as keyboardShortcutsStore } from '@wordpress/keyboard-shortcuts';
import {
store as keyboardShortcutsStore,
__unstableUseShortcutEventMatch,
} from '@wordpress/keyboard-shortcuts';
import { pipe, useCopyToClipboard } from '@wordpress/compose';

/**
Expand All @@ -30,7 +33,6 @@ import BlockSettingsMenuControls from '../block-settings-menu-controls';
import { store as blockEditorStore } from '../../store';
import { useShowMoversGestures } from '../block-toolbar/utils';

const noop = () => {};
const POPOVER_PROPS = {
className: 'block-editor-block-settings-menu__popover',
position: 'bottom right',
Expand Down Expand Up @@ -63,7 +65,6 @@ export function BlockSettingsDropdown( {
onlyBlock,
parentBlockType,
previousBlockClientId,
nextBlockClientId,
selectedBlockClientIds,
} = useSelect(
( select ) => {
Expand All @@ -72,7 +73,6 @@ export function BlockSettingsDropdown( {
getBlockName,
getBlockRootClientId,
getPreviousBlockClientId,
getNextBlockClientId,
getSelectedBlockClientIds,
getSettings,
getBlockAttributes,
Expand All @@ -98,12 +98,13 @@ export function BlockSettingsDropdown( {
getBlockType( parentBlockName ) ),
previousBlockClientId:
getPreviousBlockClientId( firstBlockClientId ),
nextBlockClientId: getNextBlockClientId( firstBlockClientId ),
selectedBlockClientIds: getSelectedBlockClientIds(),
};
},
[ firstBlockClientId ]
);
const { getBlockOrder, getSelectedBlockClientIds } =
useSelect( blockEditorStore );

const shortcuts = useSelect( ( select ) => {
const { getShortcutRepresentation } = select( keyboardShortcutsStore );
Expand All @@ -120,51 +121,47 @@ export function BlockSettingsDropdown( {
),
};
}, [] );
const isMatch = __unstableUseShortcutEventMatch();

const { selectBlock, toggleBlockHighlight } =
useDispatch( blockEditorStore );
const hasSelectedBlocks = selectedBlockClientIds.length > 0;

const updateSelectionAfterDuplicate = useCallback(
__experimentalSelectBlock
? async ( clientIdsPromise ) => {
const ids = await clientIdsPromise;
if ( ids && ids[ 0 ] ) {
__experimentalSelectBlock( ids[ 0 ] );
}
}
: noop,
async ( clientIdsPromise ) => {
if ( __experimentalSelectBlock ) {
const ids = await clientIdsPromise;
if ( ids && ids[ 0 ] ) {
__experimentalSelectBlock( ids[ 0 ], false );
}
}
},
[ __experimentalSelectBlock ]
);

const updateSelectionAfterRemove = useCallback(
__experimentalSelectBlock
? () => {
const blockToSelect =
previousBlockClientId ||
nextBlockClientId ||
firstParentClientId;
const updateSelectionAfterRemove = useCallback( () => {
if ( __experimentalSelectBlock ) {
let blockToFocus = previousBlockClientId || firstParentClientId;

if (
blockToSelect &&
// From the block options dropdown, it's possible to remove a block that is not selected,
// in this case, it's not necessary to update the selection since the selected block wasn't removed.
selectedBlockClientIds.includes( firstBlockClientId ) &&
// Don't update selection when next/prev block also is in the selection ( and gets removed ),
// In case someone selects all blocks and removes them at once.
! selectedBlockClientIds.includes( blockToSelect )
) {
__experimentalSelectBlock( blockToSelect );
}
}
: noop,
[
__experimentalSelectBlock,
previousBlockClientId,
nextBlockClientId,
firstParentClientId,
selectedBlockClientIds,
]
);
// Focus the first block if there's no previous block nor parent block.
if ( ! blockToFocus ) {
blockToFocus = getBlockOrder()[ 0 ];
}

// Only update the selection if the original selection is removed.
const shouldUpdateSelection =
hasSelectedBlocks && getSelectedBlockClientIds().length === 0;

__experimentalSelectBlock( blockToFocus, shouldUpdateSelection );
Copy link
Member Author

Choose a reason for hiding this comment

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

Still undecided if we should also rename this to __experimentalFocusAndSelectBlock. Even though it's an experimental API, for some reason it's still documented in the README 😅 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting! I didn't realise it was documented, either. Perhaps we could keep the name the same, but update the docs to mention the additional param?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking maybe just delete the docs for this prop? It's an "experimental" prop anyway? 😅 Not sure about the impact though 🤔 .

}
}, [
__experimentalSelectBlock,
previousBlockClientId,
firstParentClientId,
getBlockOrder,
hasSelectedBlocks,
getSelectedBlockClientIds,
] );

const removeBlockLabel =
count === 1 ? __( 'Delete' ) : __( 'Delete blocks' );
Expand Down Expand Up @@ -212,6 +209,49 @@ export function BlockSettingsDropdown( {
className="block-editor-block-settings-menu"
popoverProps={ POPOVER_PROPS }
noIcons
menuProps={ {
/**
* @param {KeyboardEvent} event
*/
onKeyDown( event ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the only reason for this callback to keep the focus in the list view after deleting/duplicating/inserting a block? Or is there another reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to allow pressing keyboard shortcuts when the focus is in the menu dropdown. I couldn't find any other way to do this without having to duplicate a lot of the code 😢 .

if ( event.defaultPrevented ) return;

if (
isMatch( 'core/block-editor/remove', event ) &&
canRemove
) {
event.preventDefault();
updateSelectionAfterRemove( onRemove() );
Copy link
Contributor

Choose a reason for hiding this comment

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

updateSelectionAfterRemove doesn't seem to be expecting any arguments — why do we pass onRemove() ? (same would apply for the instances of updateSelectionAfterRemove in this function)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. I think I was trying to follow the same style of pipe() used in the click handler below, and also to match the style of updateSelectionAfterDuplicate. I agree it could be confusing given that the function doesn't accept any arguments though!

} else if (
isMatch(
'core/block-editor/duplicate',
event
) &&
canDuplicate
) {
event.preventDefault();
updateSelectionAfterDuplicate( onDuplicate() );
} else if (
isMatch(
'core/block-editor/insert-after',
event
) &&
canInsertDefaultBlock
) {
event.preventDefault();
onInsertAfter();
} else if (
isMatch(
'core/block-editor/insert-before',
event
) &&
canInsertDefaultBlock
) {
event.preventDefault();
onInsertBefore();
}
},
} }
{ ...props }
>
{ ( { onClose } ) => (
Expand Down
60 changes: 59 additions & 1 deletion packages/block-editor/src/components/list-view/block-select-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import {
} from '@wordpress/components';
import { forwardRef } from '@wordpress/element';
import { Icon, lockSmall as lock } from '@wordpress/icons';
import { SPACE, ENTER } from '@wordpress/keycodes';
import { SPACE, ENTER, BACKSPACE, DELETE } from '@wordpress/keycodes';
import { useSelect, useDispatch } from '@wordpress/data';
import { __unstableUseShortcutEventMatch as useShortcutEventMatch } from '@wordpress/keyboard-shortcuts';

/**
* Internal dependencies
Expand All @@ -23,6 +25,7 @@ import useBlockDisplayInformation from '../use-block-display-information';
import useBlockDisplayTitle from '../block-title/use-block-display-title';
import ListViewExpander from './expander';
import { useBlockLock } from '../block-lock';
import { store as blockEditorStore } from '../../store';

function ListViewBlockSelectButton(
{
Expand All @@ -38,6 +41,7 @@ function ListViewBlockSelectButton(
isExpanded,
ariaLabel,
ariaDescribedBy,
updateFocusAndSelection,
},
ref
) {
Expand All @@ -47,6 +51,15 @@ function ListViewBlockSelectButton(
context: 'list-view',
} );
const { isLocked } = useBlockLock( clientId );
const {
getSelectedBlockClientIds,
getPreviousBlockClientId,
getBlockRootClientId,
getBlockOrder,
canRemoveBlocks,
} = useSelect( blockEditorStore );
const { removeBlocks } = useDispatch( blockEditorStore );
const isMatch = useShortcutEventMatch();

// The `href` attribute triggers the browser's native HTML drag operations.
// When the link is dragged, the element's outerHTML is set in DataTransfer object as text/html.
Expand All @@ -57,9 +70,54 @@ function ListViewBlockSelectButton(
onDragStart?.( event );
};

/**
* @param {KeyboardEvent} event
*/
function onKeyDownHandler( event ) {
if ( event.keyCode === ENTER || event.keyCode === SPACE ) {
onClick( event );
} else if (
event.keyCode === BACKSPACE ||
event.keyCode === DELETE ||
isMatch( 'core/block-editor/remove', event )
) {
const selectedBlockClientIds = getSelectedBlockClientIds();
const isDeletingSelectedBlocks =
selectedBlockClientIds.includes( clientId );
const firstBlockClientId = isDeletingSelectedBlocks
? selectedBlockClientIds[ 0 ]
: clientId;
const firstBlockRootClientId =
getBlockRootClientId( firstBlockClientId );

const blocksToDelete = isDeletingSelectedBlocks
? selectedBlockClientIds
: [ clientId ];

// Don't update the selection if the blocks cannot be deleted.
if ( ! canRemoveBlocks( blocksToDelete, firstBlockRootClientId ) ) {
return;
}

let blockToFocus =
getPreviousBlockClientId( firstBlockClientId ) ??
// If the previous block is not found (when the first block is deleted),
// fallback to focus the parent block.
firstBlockRootClientId;

removeBlocks( blocksToDelete, false );

// Update the selection if the original selection has been removed.
const shouldUpdateSelection =
selectedBlockClientIds.length > 0 &&
getSelectedBlockClientIds().length === 0;

// If there's no previous block nor parent block, focus the first block.
if ( ! blockToFocus ) {
blockToFocus = getBlockOrder()[ 0 ];
}

updateFocusAndSelection( blockToFocus, shouldUpdateSelection );
}
}

Expand Down
42 changes: 37 additions & 5 deletions packages/block-editor/src/components/list-view/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
} from '@wordpress/element';
import { useDispatch, useSelect } from '@wordpress/data';
import { sprintf, __ } from '@wordpress/i18n';
import { focus } from '@wordpress/dom';

/**
* Internal dependencies
Expand Down Expand Up @@ -125,6 +126,7 @@ function ListViewBlock( {
listViewInstanceId,
expandedState,
setInsertedBlock,
treeGridElementRef,
} = useListViewContext();

const hasSiblings = siblingBlockCount > 0;
Expand Down Expand Up @@ -165,11 +167,38 @@ function ListViewBlock( {
[ clientId, selectBlock ]
);

const updateSelection = useCallback(
( newClientId ) => {
selectBlock( undefined, newClientId );
const updateFocusAndSelection = useCallback(
( focusClientId, shouldSelectBlock ) => {
if ( shouldSelectBlock ) {
selectBlock( undefined, focusClientId, null, null );
}

const getFocusElement = () => {
const row = treeGridElementRef.current?.querySelector(
`[role=row][data-block="${ focusClientId }"]`
);
if ( ! row ) return null;
// Focus the first focusable in the row, which is the ListViewBlockSelectButton.
return focus.focusable.find( row )[ 0 ];
};

let focusElement = getFocusElement();
if ( focusElement ) {
focusElement.focus();
} else {
// The element hasn't been painted yet. Defer focusing on the next frame.
// This could happen when all blocks have been deleted and the default block
// hasn't been added to the editor yet.
window.requestAnimationFrame( () => {
focusElement = getFocusElement();
// Ignore if the element still doesn't exist.
if ( focusElement ) {
focusElement.focus();
}
} );
}
},
[ selectBlock ]
[ selectBlock, treeGridElementRef ]
);

const toggleExpanded = useCallback(
Expand Down Expand Up @@ -266,6 +295,7 @@ function ListViewBlock( {
selectedClientIds={ selectedClientIds }
ariaLabel={ blockAriaLabel }
ariaDescribedBy={ descriptionId }
updateFocusAndSelection={ updateFocusAndSelection }
/>
<div
className="block-editor-list-view-block-select-button__description"
Expand Down Expand Up @@ -326,10 +356,12 @@ function ListViewBlock( {
onFocus,
} }
disableOpenOnArrowDown
__experimentalSelectBlock={ updateSelection }
expand={ expand }
expandedState={ expandedState }
setInsertedBlock={ setInsertedBlock }
__experimentalSelectBlock={
updateFocusAndSelection
}
/>
) }
</TreeGridCell>
Expand Down
10 changes: 8 additions & 2 deletions packages/block-editor/src/components/list-view/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,13 @@ function ListViewComponent(
setExpandedState,
} );
const selectEditorBlock = useCallback(
( event, blockClientId ) => {
updateBlockSelection( event, blockClientId );
/**
* @param {MouseEvent | KeyboardEvent | undefined} event
* @param {string} blockClientId
* @param {null | undefined | -1 | 1} focusPosition
*/
( event, blockClientId, focusPosition ) => {
updateBlockSelection( event, blockClientId, null, focusPosition );
setSelectedTreeId( blockClientId );
if ( onSelect ) {
onSelect( getBlock( blockClientId ) );
Expand Down Expand Up @@ -222,6 +227,7 @@ function ListViewComponent(
renderAdditionalBlockUI,
insertedBlock,
setInsertedBlock,
treeGridElementRef: elementRef,
} ),
[
draggedClientIds,
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/list-view/use-block-selection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ export default function useBlockSelection() {
const { getBlockType } = useSelect( blocksStore );

const updateBlockSelection = useCallback(
async ( event, clientId, destinationClientId ) => {
async ( event, clientId, destinationClientId, focusPosition ) => {
if ( ! event?.shiftKey ) {
selectBlock( clientId );
selectBlock( clientId, focusPosition );
return;
}

Expand Down
Loading