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

Add new keyboard shortcuts for block settings menu #8279

Merged
merged 26 commits into from
Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ae3cfe3
Add duplicate shortcut to block settings menu
talldan Jul 25, 2018
ffe5104
Rename shortcut keycodes to raw/display to match block settings menu
talldan Jul 30, 2018
15450f0
displayShortcut removes `+` symbol between shortcuts when the provide…
talldan Jul 31, 2018
40d5eb9
displayShortcut capitalizes provided key (e.g. Del instead of DEL)
talldan Jul 31, 2018
97acd9e
Change duplicate shortcut to Cmd/Ctrl shift D
talldan Aug 1, 2018
001c693
Avoid placing a + between ⌘ and key entirely
talldan Aug 1, 2018
2fa5473
Change toggleSidebar shortcut to `cmd/ctrl + shift + ,` to avoid conf…
talldan Aug 1, 2018
0d4dfb2
When toggling the sidebar using the keyboard, open the correct tab ba…
talldan Aug 1, 2018
375bb3a
Add keyboard shortcuts to tooltips for closing and opening the settin…
talldan Aug 1, 2018
030eabf
Make use of stopImmediatePropagation less zealous
talldan Aug 6, 2018
86b4d02
Add modifier key exceptions to prevent deletion or merge of blocks
talldan Aug 6, 2018
0fbdd4e
Make block deletion shortcut Cmd+Opt+Backspace or Cmd+Opt+Delete
talldan Aug 6, 2018
625fd30
Add further clauses where event should return early
talldan Aug 7, 2018
68b9eb7
Make keyboard shortcut for delete Cmd+Alt+Delete
talldan Aug 7, 2018
2759893
Abbreviate display shortcut to prevent line wrapping in block setting…
talldan Aug 7, 2018
24fff39
Merge `withBlockSettingsActions` into `BlockSettingsMenu`
talldan Aug 7, 2018
98172cb
Remove BlockSettingsKeyboardShortcuts component, which had no logic
talldan Aug 7, 2018
06b72b5
Use preventDefault to prevent any additional browser/os shortcuts and…
talldan Aug 7, 2018
fc5a691
Use object method shorthand
talldan Aug 7, 2018
8a66776
Add !important back in, with clarifying comment
talldan Aug 7, 2018
d4f087c
Use lodash noop over noop arrow function
talldan Aug 7, 2018
8ea43bf
Destructure dispatching functions
talldan Aug 7, 2018
783821d
Refine exceptions to RichText Backspace/Delete handling
talldan Aug 7, 2018
c860d5d
Remove the Remove Block shortcut
talldan Aug 8, 2018
aad3406
Revert changes to RichText back to master version
talldan Aug 8, 2018
f59bdfb
Fix unit tests (stylelint)
youknowriad Aug 8, 2018
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
1 change: 1 addition & 0 deletions edit-post/components/header/fixed-toolbar-toggle/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ function FixedToolbarToggle( { onToggle, isActive } ) {
icon={ isActive && 'yes' }
isSelected={ isActive }
onClick={ onToggle }
role="menuitemcheckbox"
>
{ __( 'Fix Toolbar to Top' ) }
</MenuItem>
Expand Down
2 changes: 2 additions & 0 deletions edit-post/components/header/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.' ) }
Expand Down
2 changes: 1 addition & 1 deletion edit-post/components/header/mode-switcher/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
} );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
1 change: 1 addition & 0 deletions edit-post/components/header/tips-toggle/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ function TipsToggle( { onToggle, isActive } ) {
<MenuItem
icon={ isActive && 'yes' }
isSelected={ isActive }
role="menuitemcheckbox"
onClick={ onToggle }
>
{ __( 'Show Tips' ) }
Expand Down
44 changes: 31 additions & 13 deletions edit-post/components/keyboard-shortcuts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
2 changes: 2 additions & 0 deletions edit-post/components/sidebar/sidebar-header/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -36,6 +37,7 @@ const SidebarHeader = ( { children, className, closeLabel, closeSidebar, title }
onClick={ closeSidebar }
icon="no-alt"
label={ closeLabel }
shortcut={ shortcuts.toggleSidebar.display }
/>
</div>
</Fragment>
Expand Down
14 changes: 9 additions & 5 deletions edit-post/components/visual-editor/block-inspector-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Expand All @@ -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>
);
}

Expand Down
2 changes: 1 addition & 1 deletion edit-post/components/visual-editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 } } /> }
Expand Down
8 changes: 6 additions & 2 deletions edit-post/keyboard-shortcuts.js
Original file line number Diff line number Diff line change
Expand Up @@ -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( ',' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This shortcut is opening a new tab in chrome (Chrome support page)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder is this is another keyboard layout issue, as this key combination has no effect on my keyboard (cmd+shift+,).

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, I have a french Keyboard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there's a , key and you hold shift to output ? on a French keyboard. On a British keyboard layout it's , and you hold shift to output <.

However, on a British keyboard layout, when you press cmd+shift+,, the event handler returns that the , was pressed. On the French keyboard layout it returns ?.

That seems completely inconsistent, and I don't know why that's the case.

display: displayShortcut.primaryShift( ',' ),
},
};
10 changes: 7 additions & 3 deletions packages/components/src/menu-item/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I guess if we remove the role explicit prop and just use role="menuitem" in JSX, it has the exact same effect. Not sure if it's better or if we want to be explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that menuitem would usually be the right role, and at least this way there's some out of the box accessibility. Perhaps it's worth adding something to the docs explaining the preferred usage, and also commenting in the code.

className = classnames( 'components-menu-item__button', className, {
'has-icon': icon,
} );
Expand All @@ -40,7 +40,9 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected =
className={ className }
icon={ icon }
onClick={ onClick }
aria-pressed={ isSelected }
aria-checked={ isSelected }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be always aria-checked or only when the role is "menuitemcheckbox"?

Copy link
Contributor Author

@talldan talldan Aug 9, 2018

Choose a reason for hiding this comment

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

Yep - I'm relying on an undefined value preventing that attribute from being set on the outputted button element.

Both menuitemcheckbox and menuitemradiobutton are valid roles that can be used with aria-checked when it comes to menuitems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but please see my last comment. menuitemcheckbox and menuitemradio support is very scarce. It's pointless to use these roles if they're not communicated to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you say it's better to just use menuitem in that case? How about aria-checked?

role={ role }
{ ...props }
>
{ children }
<Shortcut shortcut={ shortcut } />
Expand All @@ -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 } />
Expand Down
7 changes: 3 additions & 4 deletions packages/components/src/menu-item/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -19,7 +19,7 @@
}

&.has-icon {
padding-left: 0;
padding-left: 0.5rem;
}

&:hover:not(:disabled):not([aria-disabled="true"]) {
Expand All @@ -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;
}
}
Expand Down
19 changes: 17 additions & 2 deletions packages/components/src/menu-item/test/__snapshots__/index.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 />
Expand Down
19 changes: 18 additions & 1 deletion packages/components/src/menu-item/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { shallow } from 'enzyme';
import { noop } from 'lodash';

/**
* Internal dependencies
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/menu-items-choice/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export default function MenuItemsChoice( {
return (
<MenuItem
key={ item.value }
role="menuitemradio"
icon={ isSelected && 'yes' }
isSelected={ isSelected }
shortcut={ item.shortcut }
Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 }
Copy link
Member

Choose a reason for hiding this comment

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

🤯

/>
) }
{ shouldShowBreadcrumb && (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
);
}
Loading