Add new keyboard shortcuts for block settings menu (#8279)

Adds new keyboard shortcuts for the block settings menu:

Cmd+Shift+D / Ctrl+Shift+D - duplicate block
Cmd+shift+, / Ctrl+shift+, - show/hide sidebar

* Add duplicate shortcut to block settings menu

- Move onDuplicate dispatch from BlockDuplicateButton into a new HOC called
`withBlockSettingsActions`
- Add `shortcuts` as a prop in `withBlockSettingsActions` that is passed to
its wrapped component
- Pass onDuplicate and shortcut through to BlockDuplicateButton
- Add new BlockSettingsKeyboardShortcuts component for handling shortcut
functionality

Make BlockDuplicateButton a dumb component - move HOC logic to withBlockSettingsActions

Add keyboard shortcut for block removal (Cmd/Special/Windows + DEL)

- Move block remove action dispatch from BlockRemoveButton up to withBlockSettingsActions
- Make BlockRemoveButton a dumb component
- Add keyboard shortcut for block removal to BlockSettingsKeyboardShortcuts
- Pass onRemove into KeyboardShortcuts and BlockRemoveButton

Add shortcut for toggling the sidebar

- Add shortcut keys to list of keyboard shortcuts for edit-post
- Add open/close dispatchers to EditorModeKeyboardShortcuts and hook up
- Make BlockInspectorButton display the keyboard shortcut

MenuItem should use aria-checked attribute instead of aria-pressed

See example implementation of a MenuBar here for details:
https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-2/menubar-2.html

Provide a default role for MenuItem component of menuitem

Still allows the role to be overriden for other menuitem types
(i.e. menuitemcheckbox, menuitemradio), but defaults to menuitem.

Also makes the provision of isSelected prop optional since the resulting
aria-checked role should not be used with a menu-item.

Update usages of MenuItem that should not use the default role

Switch menu items in block settings menu that have a shortcut to use MenuItem

Use the MenuItem component which correctly renders the shortcut. The need for
wrapping components BlockDuplicateButton and BlockRemoveButton is minimal, so
remove them.

Remove role props, since MenuItem implements a default

Make all block settings menu items use MenuItem component instead of IconButton

Spread additional props for MenuItem onto rendered component

This allows MenuItem to support additional props like `disabled`

Remove stroke being applied to MenuItem icons

Most icons don't have stroke - this style rule was adding an additional stroke
to SVG icons, making them appear chunky and blurry.

Remove use of !important

Fix lack of left margin on menu items with icons

Fix potential access of undefined property

When removing the block, there's momentarily a null item in the blocks array

Render block settings when typing, but make it hidden

This ensures BlockSettingsKeyboardShortcuts is rendered and shortcuts
work while actively typing

* Rename shortcut keycodes to raw/display to match block settings menu

* displayShortcut removes `+` symbol between shortcuts when the provided key is any single character

* displayShortcut capitalizes provided key (e.g. Del instead of DEL)

* Avoid placing a + between ⌘ and key entirely

* Change duplicate shortcut to Cmd/Ctrl shift D

* Change toggleSidebar shortcut to `cmd/ctrl + shift + ,` to avoid conflict with Chrome settings shortcut

* When toggling the sidebar using the keyboard, open the correct tab based on whether block(s) are selected

* Add keyboard shortcuts to tooltips for closing and opening the settings sidebar

* Make use of stopImmediatePropagation less zealous

Code is supposed to prevent further handling of keyboard events when a block is
deleted, but doesn't consider cases where a block is not deleted:

- If the user hits delete and the block is kept, but content from another block
merged in
- If the user hits delete, but the block has content and is thus not deleted

* Add modifier key exceptions to prevent deletion or merge of blocks

* Make block deletion shortcut Cmd+Opt+Backspace or Cmd+Opt+Delete

- For Windows that's Ctrl+Alt+Backspace or Ctrl+Alt+Delete

* Add further clauses where event should return early

Iron out inconsistent behaviour with use of backspace/delete key when it can
result in deletion or merging of blocks. When specific modifiers are pressed
the event should not have any effect. Here, the use of preventDefault and
stopImmediatePropagation were blocking further binding of keyboard shortcuts
and preventing browser/system level keyboard shortcuts.

* Make keyboard shortcut for delete Cmd+Alt+Delete

* Abbreviate display shortcut to prevent line wrapping in block settings menu

* Merge `withBlockSettingsActions` into `BlockSettingsMenu`

- BlockSettingsMenu was the only consumer of withBlockSettings
- shortcuts object is now not injected into props

* Remove BlockSettingsKeyboardShortcuts component, which had no logic

* Use preventDefault to prevent any additional browser/os shortcuts and add comments

* Use object method shorthand

* Add !important back in, with clarifying comment

* Use lodash noop over noop arrow function

* Destructure dispatching functions

* Refine exceptions to RichText Backspace/Delete handling

* Remove the Remove Block shortcut

* Revert changes to RichText back to master version

* Fix unit tests (stylelint)

edit-post/components/header/fixed-toolbar-toggle/index.js

@@ -17,6 +17,7 @@ function FixedToolbarToggle( { onToggle, isActive } ) {
 			icon={ isActive && 'yes' }
 			isSelected={ isActive }
 			onClick={ onToggle }
+			role="menuitemcheckbox"
 		>
 			{ __( 'Fix Toolbar to Top' ) }
 		</MenuItem>

edit-post/components/header/index.js

@@ -19,6 +19,7 @@ import './style.scss';
 import MoreMenu from './more-menu';
 import HeaderToolbar from './header-toolbar';
 import PinnedPlugins from './pinned-plugins';
+import shortcuts from '../../keyboard-shortcuts';
 
 function Header( {
 	isEditorSidebarOpened,
@@ -59,6 +60,7 @@ function Header( {
 						onClick={ toggleGeneralSidebar }
 						isToggled={ isEditorSidebarOpened }
 						aria-expanded={ isEditorSidebarOpened }
+						shortcut={ shortcuts.toggleSidebar.display }
 					>
 						<DotTip id="core/editor.settings">
 							{ __( 'You’ll find more settings for your page and blocks in the sidebar. Click ‘Settings’ to open it.' ) }

edit-post/components/header/mode-switcher/index.js

@@ -29,7 +29,7 @@ const MODES = [
 function ModeSwitcher( { onSwitch, mode } ) {
 	const choices = MODES.map( ( choice ) => {
 		if ( choice.value !== mode ) {
-			return { ...choice, shortcut: shortcuts.toggleEditorMode.label };
+			return { ...choice, shortcut: shortcuts.toggleEditorMode.display };
 		}
 		return choice;
 	} );

edit-post/components/header/plugin-sidebar-more-menu-item/index.js

@@ -17,6 +17,7 @@ const PluginSidebarMoreMenuItem = ( { children, icon, isSelected, onClick } ) =>
 			<MenuItem
 				icon={ isSelected ? 'yes' : icon }
 				isSelected={ isSelected }
+				role="menuitemcheckbox"
 				onClick={ compose( onClick, fillProps.onClose ) }
 			>
 				{ children }

edit-post/components/header/tips-toggle/index.js

@@ -15,6 +15,7 @@ function TipsToggle( { onToggle, isActive } ) {
 		<MenuItem
 			icon={ isActive && 'yes' }
 			isSelected={ isActive }
+			role="menuitemcheckbox"
 			onClick={ onToggle }
 		>
 			{ __( 'Show Tips' ) }

edit-post/components/keyboard-shortcuts/index.js

@@ -16,36 +16,54 @@ class EditorModeKeyboardShortcuts extends Component {
 		super( ...arguments );
 
 		this.toggleMode = this.toggleMode.bind( this );
+		this.toggleSidebar = this.toggleSidebar.bind( this );
 	}
 
 	toggleMode() {
 		const { mode, switchMode } = this.props;
 		switchMode( mode === 'visual' ? 'text' : 'visual' );
 	}
 
+	toggleSidebar( event ) {
+		// This shortcut has no known clashes, but use preventDefault to prevent any
+		// obscure shortcuts from triggering.
+		event.preventDefault();
+		const { isEditorSidebarOpen, closeSidebar, openSidebar } = this.props;
+
+		if ( isEditorSidebarOpen ) {
+			closeSidebar();
+		} else {
+			openSidebar();
+		}
+	}
+
 	render() {
 		return (
 			<KeyboardShortcuts
 				bindGlobal
 				shortcuts={ {
-					[ shortcuts.toggleEditorMode.value ]: this.toggleMode,
+					[ shortcuts.toggleEditorMode.raw ]: this.toggleMode,
+					[ shortcuts.toggleSidebar.raw ]: this.toggleSidebar,
 				} }
 			/>
 		);
 	}
 }
 
 export default compose( [
-	withSelect( ( select ) => {
-		return {
-			mode: select( 'core/edit-post' ).getEditorMode(),
-		};
-	} ),
-	withDispatch( ( dispatch ) => {
-		return {
-			switchMode: ( mode ) => {
-				dispatch( 'core/edit-post' ).switchEditorMode( mode );
-			},
-		};
-	} ),
+	withSelect( ( select ) => ( {
+		mode: select( 'core/edit-post' ).getEditorMode(),
+		isEditorSidebarOpen: select( 'core/edit-post' ).isEditorSidebarOpened(),
+		hasBlockSelection: !! select( 'core/editor' ).getBlockSelectionStart(),
+	} ) ),
+	withDispatch( ( dispatch, { hasBlockSelection } ) => ( {
+		switchMode( mode ) {
+			dispatch( 'core/edit-post' ).switchEditorMode( mode );
+		},
+		openSidebar() {
+			const sidebarToOpen = hasBlockSelection ? 'edit-post/block' : 'edit-post/document';
+			dispatch( 'core/edit-post' ).openGeneralSidebar( sidebarToOpen );
+		},
+		closeSidebar: dispatch( 'core/edit-post' ).closeGeneralSidebar,
+	} ) ),
 ] )( EditorModeKeyboardShortcuts );

edit-post/components/sidebar/sidebar-header/index.js

@@ -16,6 +16,7 @@ import { withDispatch, withSelect } from '@wordpress/data';
  * Internal dependencies
  */
 import './style.scss';
+import shortcuts from '../../../keyboard-shortcuts';
 
 const SidebarHeader = ( { children, className, closeLabel, closeSidebar, title } ) => {
 	return (
@@ -36,6 +37,7 @@ const SidebarHeader = ( { children, className, closeLabel, closeSidebar, title }
 					onClick={ closeSidebar }
 					icon="no-alt"
 					label={ closeLabel }
+					shortcut={ shortcuts.toggleSidebar.display }
 				/>
 			</div>
 		</Fragment>

edit-post/components/visual-editor/block-inspector-button.js

@@ -7,18 +7,22 @@ import { flow, noop } from 'lodash';
  * WordPress dependencies
  */
 import { __ } from '@wordpress/i18n';
-import { IconButton, withSpokenMessages } from '@wordpress/components';
+import { MenuItem, withSpokenMessages } from '@wordpress/components';
 import { withSelect, withDispatch } from '@wordpress/data';
 import { compose } from '@wordpress/compose';
 
+/**
+ * Internal dependencies
+ */
+import shortcuts from '../../keyboard-shortcuts';
+
 export function BlockInspectorButton( {
 	areAdvancedSettingsOpened,
 	closeSidebar,
 	openEditorSidebar,
 	onClick = noop,
 	small = false,
 	speak,
-	role,
 } ) {
 	const speakMessage = () => {
 		if ( areAdvancedSettingsOpened ) {
@@ -31,15 +35,15 @@ export function BlockInspectorButton( {
 	const label = areAdvancedSettingsOpened ? __( 'Hide Block Settings' ) : __( 'Show Block Settings' );
 
 	return (
-		<IconButton
+		<MenuItem
 			className="editor-block-settings-menu__control"
 			onClick={ flow( areAdvancedSettingsOpened ? closeSidebar : openEditorSidebar, speakMessage, onClick ) }
 			icon="admin-generic"
 			label={ small ? label : undefined }
-			role={ role }
+			shortcut={ shortcuts.toggleSidebar.display }
 		>
 			{ ! small && label }
-		</IconButton>
+		</MenuItem>
 	);
 }
 

edit-post/components/visual-editor/index.js

@@ -34,7 +34,7 @@ function VisualEditor() {
 				</ObserveTyping>
 			</WritingFlow>
 			<_BlockSettingsMenuFirstItem>
-				{ ( { onClose } ) => <BlockInspectorButton onClick={ onClose } role="menuitem" /> }
+				{ ( { onClose } ) => <BlockInspectorButton onClick={ onClose } /> }
 			</_BlockSettingsMenuFirstItem>
 			<_BlockSettingsMenuPluginsExtension>
 				{ ( { clientIds, onClose } ) => <PluginBlockSettingsMenuGroup.Slot fillProps={ { clientIds, onClose } } /> }

edit-post/keyboard-shortcuts.js

@@ -5,7 +5,11 @@ import { rawShortcut, displayShortcut } from '@wordpress/keycodes';
 
 export default {
 	toggleEditorMode: {
-		value: rawShortcut.secondary( 'm' ),
-		label: displayShortcut.secondary( 'm' ),
+		raw: rawShortcut.secondary( 'm' ),
+		display: displayShortcut.secondary( 'm' ),
+	},
+	toggleSidebar: {
+		raw: rawShortcut.primaryShift( ',' ),
+		display: displayShortcut.primaryShift( ',' ),
 	},
 };

packages/components/src/menu-item/index.js

@@ -21,7 +21,7 @@ import IconButton from '../icon-button';
  *
  * @return {WPElement} More menu item.
  */
-function MenuItem( { children, className, icon, onClick, shortcut, isSelected = false } ) {
+function MenuItem( { children, className, icon, onClick, shortcut, isSelected, role = 'menuitem', ...props } ) {
 	className = classnames( 'components-menu-item__button', className, {
 		'has-icon': icon,
 	} );
@@ -40,7 +40,9 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected =
 				className={ className }
 				icon={ icon }
 				onClick={ onClick }
-				aria-pressed={ isSelected }
+				aria-checked={ isSelected }
+				role={ role }
+				{ ...props }
 			>
 				{ children }
 				<Shortcut shortcut={ shortcut } />
@@ -52,7 +54,9 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected =
 		<Button
 			className={ className }
 			onClick={ onClick }
-			aria-pressed={ isSelected }
+			aria-checked={ isSelected }
+			role={ role }
+			{ ...props }
 		>
 			{ children }
 			<Shortcut shortcut={ shortcut } />

packages/components/src/menu-item/style.scss

@@ -3,7 +3,7 @@
 	width: 100%;
 	padding: 8px;
 	text-align: left;
-	padding-left: 25px;
+	padding-left: 2rem;
 	color: $dark-gray-500;
 
 	// Target plugin icons that can have arbitrary classes by using an aggressive selector.
@@ -19,7 +19,7 @@
 	}
 
 	&.has-icon {
-		padding-left: 0;
+		padding-left: 0.5rem;
 	}
 
 	&:hover:not(:disabled):not([aria-disabled="true"]) {
@@ -33,13 +33,12 @@
 	// Colorize plugin icons to ensure contrast and cohesion, but allow plugin developers to override.
 	svg,
 	svg * {
-		stroke: $dark-gray-500;
 		fill: $dark-gray-500;
 	}
 
 	&:hover svg,
 	&:hover svg * {
-		stroke: $dark-gray-900 !important;
+		// !important allows icons from plugins to be overriden and given a dark-gray fill
 		fill: $dark-gray-900 !important;
 	}
 }

packages/components/src/menu-item/test/__snapshots__/index.js.snap

@@ -2,10 +2,25 @@
 
 exports[`MenuItem should match snapshot when all props provided 1`] = `
 <IconButton
-  aria-pressed={true}
+  aria-checked={true}
   className="components-menu-item__button my-class has-icon"
   icon="wordpress"
   onClick={[Function]}
+  role="menuitemcheckbox"
+>
+  My item
+  <MenuItemsShortcut
+    shortcut="mod+shift+alt+w"
+  />
+</IconButton>
+`;
+
+exports[`MenuItem should match snapshot when isSelected and role are optionally provided 1`] = `
+<IconButton
+  className="components-menu-item__button my-class has-icon"
+  icon="wordpress"
+  onClick={[Function]}
+  role="menuitem"
 >
   My item
   <MenuItemsShortcut
@@ -16,8 +31,8 @@ exports[`MenuItem should match snapshot when all props provided 1`] = `
 
 exports[`MenuItem should match snapshot when only label provided 1`] = `
 <Button
-  aria-pressed={false}
   className="components-menu-item__button"
+  role="menuitem"
 >
   My item
   <MenuItemsShortcut />

packages/components/src/menu-item/test/index.js

@@ -2,6 +2,7 @@
  * External dependencies
  */
 import { shallow } from 'enzyme';
+import { noop } from 'lodash';
 
 /**
  * Internal dependencies
@@ -27,7 +28,23 @@ describe( 'MenuItem', () => {
 				className="my-class"
 				icon="wordpress"
 				isSelected={ true }
-				onClick={ () => {} }
+				role="menuitemcheckbox"
+				onClick={ noop }
+				shortcut="mod+shift+alt+w"
+			>
+				My item
+			</MenuItem>
+		);
+
+		expect( wrapper ).toMatchSnapshot();
+	} );
+
+	test( 'should match snapshot when isSelected and role are optionally provided', () => {
+		const wrapper = shallow(
+			<MenuItem
+				className="my-class"
+				icon="wordpress"
+				onClick={ noop }
 				shortcut="mod+shift+alt+w"
 			>
 				My item

packages/components/src/menu-items-choice/index.js

@@ -13,6 +13,7 @@ export default function MenuItemsChoice( {
 		return (
 			<MenuItem
 				key={ item.value }
+				role="menuitemradio"
 				icon={ isSelected && 'yes' }
 				isSelected={ isSelected }
 				shortcut={ item.shortcut }

packages/editor/src/components/block-list/block.js

@@ -393,7 +393,7 @@ export class BlockListBlock extends Component {
 		const shouldAppearSelectedParent = ! showSideInserter && hasSelectedInnerBlock && ! isTypingWithinBlock;
 		// We render block movers and block settings to keep them tabbale even if hidden
 		const shouldRenderMovers = ( isSelected || hoverArea === 'left' ) && ! showEmptyBlockSideInserter && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock;
-		const shouldRenderBlockSettings = ( isSelected || hoverArea === 'right' ) && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock;
+		const shouldRenderBlockSettings = ( isSelected || hoverArea === 'right' ) && ! isMultiSelecting && ! isPartOfMultiSelection;
 		const shouldShowBreadcrumb = isHovered && ! isEmptyDefaultBlock;
 		const shouldShowContextualToolbar = ! showSideInserter && ( ( isSelected && ! isTypingWithinBlock && isValid ) || isFirstMultiSelected ) && ( ! hasFixedToolbar || ! isLargeViewport );
 		const shouldShowMobileToolbar = shouldAppearSelected;
@@ -499,7 +499,7 @@ export class BlockListBlock extends Component {
 					<BlockSettingsMenu
 						clientIds={ clientId }
 						rootClientId={ rootClientId }
-						isHidden={ ! ( isHovered || isSelected ) || hoverArea !== 'right' }
+						isHidden={ ! ( isHovered || isSelected ) || hoverArea !== 'right' || isTypingWithinBlock }
 					/>
 				) }
 				{ shouldShowBreadcrumb && (

packages/editor/src/components/block-settings-menu/block-convert-button.js

@@ -2,23 +2,22 @@
  * WordPress dependencies
  */
 import { __ } from '@wordpress/i18n';
-import { IconButton } from '@wordpress/components';
+import { MenuItem } from '@wordpress/components';
 
-export default function BlockConverter( { shouldRender, onClick, small, role } ) {
+export default function BlockConvertButton( { shouldRender, onClick, small } ) {
 	if ( ! shouldRender ) {
 		return null;
 	}
 
 	const label = __( 'Convert to Blocks' );
 	return (
-		<IconButton
+		<MenuItem
 			className="editor-block-settings-menu__control"
 			onClick={ onClick }
 			icon="screenoptions"
 			label={ small ? label : undefined }
-			role={ role }
 		>
 			{ ! small && label }
-		</IconButton>
+		</MenuItem>
 	);
 }