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

Block Toolbar: Refactor using KeyboardShorcuts component #3031

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
10 changes: 8 additions & 2 deletions components/keyboard-shortcuts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class SelectAllDetection extends Component {
super( ...arguments );

this.setAllSelected = this.setAllSelected.bind( this );
this.save = this.save.bind( this );

this.state = { isAllSelected: false };
}
Expand All @@ -23,11 +24,16 @@ class SelectAllDetection extends Component {
this.setState( { isAllSelected: true } );
}

save() {
this.props.onSave();
}

render() {
return (
<div>
<KeyboardShorcuts shortcuts={ {
'mod+a': this.setAllSelected,
'mod+s': [ this.save, true ],
} } />
Combination pressed? { isAllSelected ? 'Yes' : 'No' }
</div>
Expand All @@ -38,15 +44,15 @@ class SelectAllDetection extends Component {

__Note:__ The value of each shortcut should be a consistent function reference, not an anonymous function. Otherwise, the callback will not be correctly unbound when the component unmounts.

__Note:__ The callback will not be invoked if the key combination occurs in an editable field.
__Note:__ The callback will not be invoked if the key combination occurs in an editable field, unless you pass the callback as an array with the callback and `true` as entries of the array respectively. Refer to the example above, and see the `shortcuts` prop documentation for more information.

## Props

The component accepts the following props:

### shortcuts

An object of shortcut bindings, where each key is a keyboard combination, the value of which is the callback to be invoked when the key combination is pressed.
An object of shortcut bindings, where each key is a keyboard combination, the value of which is the callback to be invoked when the key combination is pressed. To capture a key event globally (including within input fields), assign as the value an array with the function callback as the first argument and `true` as the second argument.

- Type: `Object`
- Required: No
Expand Down
15 changes: 14 additions & 1 deletion components/keyboard-shortcuts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import Mousetrap from 'mousetrap';
import 'mousetrap/plugins/global-bind/mousetrap-global-bind';
import { forEach } from 'lodash';

/**
Expand All @@ -13,7 +14,19 @@ class KeyboardShortcuts extends Component {
componentWillMount() {
this.mousetrap = new Mousetrap;
forEach( this.props.shortcuts, ( callback, key ) => {
this.mousetrap.bind( key, callback );
// Normalize callback, which can be passed as either a function or
// an array of [ callback, isGlobal ]
let isGlobal = false;
if ( Array.isArray( callback ) ) {
isGlobal = callback[ 1 ];
callback = callback[ 0 ];
}

if ( isGlobal ) {
this.mousetrap.bindGlobal( key, callback );
} else {
this.mousetrap.bind( key, callback );
}
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think the shortcuts prop could change over time? Should we implement componentWillReceivePorps or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think the shortcuts prop could change over time? Should we implement componentWillReceivePorps or not?

Potentially, though the effect could be imitated by rendering separate KeyboardShortcuts components (or changing the key of an existing one). Doesn't seem like a need we currently have, but then again, could be a potential source of confusion for future usage.

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 started looking at this, and realized that there may be some non-trivial checks to consider whether keyboard bindings need to be updated.

https://gist.github.com/aduth/52f5c4ff561723c2d71b7e9d7fc142e7

Instead, and since there's currently no need, I improved documentation to note that shortcuts changes won't take effect and a suggested workaround (2601680).

}

Expand Down
61 changes: 61 additions & 0 deletions components/keyboard-shortcuts/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* External dependencies
*/
import { mount } from 'enzyme';

/**
* Internal dependencies
*/
import KeyboardShortcuts from '../';

describe( 'KeyboardShortcuts', () => {
afterEach( () => {
while ( document.body.firstChild ) {
document.body.removeChild( document.body.firstChild );
}
} );

function keyPress( which, target ) {
[ 'keydown', 'keypress', 'keyup' ].forEach( ( eventName ) => {
const event = new window.Event( eventName, { bubbles: true } );
event.keyCode = which;
event.which = which;
target.dispatchEvent( event );
} );
}

it( 'should capture key events', () => {
const spy = jest.fn();
mount(
<KeyboardShortcuts
shortcuts={ {
d: spy,
} } />
);

keyPress( 68, document );

expect( spy ).toHaveBeenCalled();
} );

it( 'should capture key events globally', () => {
const spy = jest.fn();
const attachNode = document.createElement( 'div' );
document.body.appendChild( attachNode );

const wrapper = mount(
<div>
<KeyboardShortcuts
shortcuts={ {
d: [ spy, true ],
} } />
<textarea></textarea>
</div>,
{ attachTo: attachNode }
);

keyPress( 68, wrapper.find( 'textarea' ).getDOMNode() );

expect( spy ).toHaveBeenCalled();
} );
} );
51 changes: 13 additions & 38 deletions editor/block-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
/**
* WordPress Dependencies
*/
import { IconButton, Toolbar, NavigableMenu, Slot } from '@wordpress/components';
import { IconButton, Toolbar, NavigableMenu, Slot, KeyboardShortcuts } from '@wordpress/components';
import { Component, Children, findDOMNode } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { focus, keycodes } from '@wordpress/utils';
Expand All @@ -19,47 +19,29 @@ import './style.scss';
import BlockSwitcher from '../block-switcher';
import BlockMover from '../block-mover';
import BlockRightMenu from '../block-settings-menu';
import { isMac } from '../utils/dom';

/**
* Module Constants
*/
const { ESCAPE, F10 } = keycodes;
const { ESCAPE } = keycodes;

function FirstChild( { children } ) {
const childrenArray = Children.toArray( children );
return childrenArray[ 0 ] || null;
}

function metaKeyPressed( event ) {
return isMac() ? event.metaKey : ( event.ctrlKey && ! event.altKey );
}

class BlockToolbar extends Component {
constructor() {
super( ...arguments );

this.toggleMobileControls = this.toggleMobileControls.bind( this );
this.bindNode = this.bindNode.bind( this );
this.onKeyUp = this.onKeyUp.bind( this );
this.onKeyDown = this.onKeyDown.bind( this );
this.focusToolbar = this.focusToolbar.bind( this );
this.onToolbarKeyDown = this.onToolbarKeyDown.bind( this );

this.state = {
showMobileControls: false,
};

// it's not easy to know if the user only clicked on a "meta" key without simultaneously clicking on another key
// We keep track of the key counts to ensure it's reliable
this.metaCount = 0;
}

componentDidMount() {
document.addEventListener( 'keyup', this.onKeyUp );
document.addEventListener( 'keydown', this.onKeyDown );
}

componentWillUnmount() {
document.removeEventListener( 'keyup', this.onKeyUp );
document.removeEventListener( 'keydown', this.onKeyDown );
}

bindNode( ref ) {
Expand All @@ -72,21 +54,10 @@ class BlockToolbar extends Component {
} ) );
}

onKeyDown( event ) {
if ( metaKeyPressed( event ) ) {
this.metaCount++;
}
}

onKeyUp( event ) {
const shouldFocusToolbar = this.metaCount === 1 || ( event.keyCode === F10 && event.altKey );
this.metaCount = 0;

if ( shouldFocusToolbar ) {
const tabbables = focus.tabbable.find( this.toolbar );
if ( tabbables.length ) {
tabbables[ 0 ].focus();
}
focusToolbar() {
const tabbables = focus.tabbable.find( this.toolbar );
if ( tabbables.length ) {
tabbables[ 0 ].focus();
}
}

Expand Down Expand Up @@ -128,6 +99,10 @@ class BlockToolbar extends Component {
role="toolbar"
deep
>
<KeyboardShortcuts shortcuts={ {
mod: [ this.focusToolbar, true ],
'alt+f10': [ this.focusToolbar, true ],
} } />
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice refactoring 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we handle the "escape" key as well onToolbarKeyDown?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we handle the "escape" key as well onToolbarKeyDown?

We can, but to recreate the handling of these keys only taking effect while within the toolbar, we'd want to introduce child scoping for KeyboardShortcuts. I mentioned this in #1944, and should probably be done as a separate refactoring step.

<div className="editor-block-toolbar__group" onKeyDown={ this.onToolbarKeyDown }>
{ ! showMobileControls && [
<BlockSwitcher key="switcher" uid={ uid } />,
Expand Down
9 changes: 0 additions & 9 deletions editor/utils/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,3 @@ export function placeCaretAtEdge( container, start = false ) {
sel.addRange( range );
container.focus();
}

/**
* Checks whether the user is on MacOS or not
*
* @return {Boolean} Is Mac or Not
*/
export function isMac() {
return window.navigator.platform.toLowerCase().indexOf( 'mac' ) !== -1;
}