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 keyboard navigation to the inserter. #578

Merged
merged 5 commits into from
May 8, 2017
Merged
Show file tree
Hide file tree
Changes from 4 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
3 changes: 2 additions & 1 deletion editor/components/button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import './style.scss';
import classnames from 'classnames';

function Button( { isPrimary, isLarge, isToggled, className, ...additionalProps } ) {
function Button( { isPrimary, isLarge, isToggled, className, buttonRef, ...additionalProps } ) {
const classes = classnames( 'editor-button', className, {
button: ( isPrimary || isLarge ),
'button-primary': isPrimary,
Expand All @@ -16,6 +16,7 @@ function Button( { isPrimary, isLarge, isToggled, className, ...additionalProps
<button
type="button"
{ ...additionalProps }
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: should we exclude buttonRef from this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean exactly.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that the buttonRef prop is passed here to the button element. And it's not a valid prop for button. So to avoid this, we should deconstruct buttonRef in the function props to omit it from additionalProps.

Something like:

function Button( { isPrimary, isLarge, isToggled, className, buttonRef, ...additionalProps } ) {

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign May 4, 2017

Choose a reason for hiding this comment

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

Okay, that is what I thought you meant, but wasn't 100% sure. Thank you for clarifying.

ref={ buttonRef }
className={ classes } />
);
}
Expand Down
8 changes: 7 additions & 1 deletion editor/components/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import IconButton from 'components/icon-button';
class Inserter extends wp.element.Component {
constructor() {
super( ...arguments );
this.nodes = {};
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 not needed anymore

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 hate working with classes lol, I am very out of my element on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, doesn't work if you remove it. Maybe I will change bindRefNode() and see if that changes things.

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 prefer to keep this line or change bindReferenceNode()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I bet you tried to remove the this.nodes from the wrong file, don't you?

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 okay 😆 . Thought this was the menu.

this.toggle = this.toggle.bind( this );
this.close = this.close.bind( this );
this.state = {
Expand All @@ -20,6 +21,10 @@ class Inserter extends wp.element.Component {
}

toggle() {
if ( this.state.opened ) {
this.toggleNode.focus();
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the need to force focus back on the button when closing the inserter menu?

My main worry here is the passing the ref callback as a prop. ref alone is undesirable but sometimes necessary escape from the virtual DOM into the "real" DOM within a component; expanding this further in allowing a parent to access its child's DOM compounds fragility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accessibility. If someone closes the menu and focus doesn't return to the button it gets confusing to navigate again.

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign May 8, 2017

Choose a reason for hiding this comment

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

I would have preferred to not write this code, but couldn't think of a better way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Chiming in to remind these kind of focus management is a basic accessibility requirement. Without that, focus would be lost because we've just removed the previously focused element from the DOM. More info of this, for example, here: https://www.w3.org/TR/wai-aria-practices/#kbd_focus_discernable_predictable

Copy link
Member

Choose a reason for hiding this comment

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

We don't necessarily need to remove the DOM element; we could apply styling conditionally to hide the element, if that helps retain focus. I've not dug too far into it, but perhaps also changing the inserter's root element to be the one that receives focus (instead of separately its button and list children) could help avoid the need for a ref prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hiding it with display: none or visibility: hidden would be the same, since the menu items would be not focusable and this would cause a focus loss. Hiding by other means (off-screen, etc.) it's not what we need because when pressing Escape we want the menu to close and focus be moved to a reasonable place, which is the control that opened the menu.
I'm sorry React is not able to handle this kind of things graciously, that's one of the reasons I say it wasn't designed with accessibility in mind.
Also noting we'll need more of this kind of interaction in other places, see for example the block switcher.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry React is not able to handle this kind of things graciously, that's one of the reasons I say it wasn't designed with accessibility in mind.

What does React have to do with focus loss caused by elements being removed or hidden?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aduth 😑 I mean React doesn't have a native, easy, way to handle things like this one. To my understanding, you all don't like the way you had to implement this focus management, no?

Copy link
Member

Choose a reason for hiding this comment

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

@aduth 😑 I mean React doesn't have a native, easy, way to handle things like this one. To my understanding, you all don't like the way you had to implement this focus management, no?

Regardless of tool, it's an undesirable complexity to programmatically manage focus as we've found to need here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't have strong opinions about the tool you'll chose to implement this. This is an accessibility requirement though, and needs to be implemented.
I'd also remind everyone that, as I've noted previously a few times, this kind of focus management will be necessary in other places too.

}

this.setState( {
opened: ! this.state.opened
} );
Expand Down Expand Up @@ -51,8 +56,9 @@ class Inserter extends wp.element.Component {
onClick={ this.toggle }
className="editor-inserter__toggle"
aria-haspopup="true"
buttonRef={ ( node ) => this.toggleNode = node }
aria-expanded={ opened ? 'true' : 'false' } />
{ opened && <InserterMenu position={ position } onSelect={ this.close } /> }
{ opened && <InserterMenu position={ position } onSelect={ this.close } closeMenu={ this.toggle } /> }
</div>
);
}
Expand Down
235 changes: 216 additions & 19 deletions editor/components/inserter/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { connect } from 'react-redux';
import { flow } from 'lodash';

/**
* Internal dependencies
Expand All @@ -12,11 +13,52 @@ import Dashicon from 'components/dashicon';
class InserterMenu extends wp.element.Component {
constructor() {
super( ...arguments );
this.blockTypes = wp.blocks.getBlocks();
this.categories = wp.blocks.getCategories();
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes the categories and blocks don't change over time. It's true for now, we can keep it this way. But worth nothing this could change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the assumption I was making. I would advise against ever having these values change, and instead use filtering to get the changed values.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just call to getBlocks and getCategories directly where they're used, specifically in the render? Even if there's a caching concern, it's usually discouraged to duplicate the source of truth because it's difficult to keep in sync (as noted by @youknowriad).

https://github.com/facebook/react/blob/8cac523/docs/tips/10-props-in-getInitialState-as-anti-pattern.md

this.nodes = {};
this.state = {
filterValue: ''
filterValue: '',
currentFocus: null
};
this.filter = this.filter.bind( this );
this.instanceId = this.constructor.instances++;
this.isShownBlock = this.isShownBlock.bind( this );
this.setSearchFocus = this.setSearchFocus.bind( this );
this.onKeyDown = this.onKeyDown.bind( this );
this.getVisibleBlocks = this.getVisibleBlocks.bind( this );
this.sortBlocksByCategory = this.sortBlocksByCategory.bind( this );
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like changeMenuSelection, bindReferenceNode, getVisibleBlocksByCategory, getCategoryIndex don't require binding?

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'd have to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Good catch.

}

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

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

isShownBlock( block ) {
return block.title.toLowerCase().indexOf( this.state.filterValue.toLowerCase() ) !== -1;
}

bindReferenceNode( nodeName ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

blockType instead of nodeName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to also bind the search field. The search field of the inserter is not a blockType so I used nodeName instead is blockType preferred?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know, what I'm proposing is not mixing the refs in the same array, the blocks nodes are in the same array, and the input in its own property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

return ( node ) => this.nodes[ nodeName ] = node;
}

isNextKeydown( keydown ) {
return keydown.code === 'ArrowDown'
Copy link
Member

Choose a reason for hiding this comment

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

The code property is only available in Chrome and Firefox:

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code#Browser_compatibility

Even though it's now deprecated, which or keyCode has best compatibility for the browser's we support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah maybe that is the source of some of the a11y problems. Un-normalized events are no fun.

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign May 8, 2017

Choose a reason for hiding this comment

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

For some reason GitHub won't let me reply to the comment above.

Why don't we just call to getBlocks and getCategories directly where they're used, specifically in the render? Even if there's a caching concern, it's usually discouraged to duplicate the source of truth because it's difficult to keep in sync (as noted by @youknowriad).

I was kind of making an assumption, maybe a bad one, that this will never change across an initialization. What are the use cases in which we would want blockTypes and categories to change on the fly? I would think we want blockTypes and categories to be set on initialization, then any shape of that data can be generated on the fly, which is what is happening here.

|| keydown.code === 'ArrowRight'
|| ( keydown.code === 'Tab' && keydown.shiftKey === false );
}

isPreviousKeydown( keydown ) {
return keydown.code === 'ArrowUp'
|| keydown.code === 'ArrowLeft'
|| ( keydown.code === 'Tab' && keydown.shiftKey === true );
}

isEscapeKey( keydown ) {
return keydown.code === 'Escape';
}

filter( event ) {
Expand All @@ -29,32 +71,183 @@ class InserterMenu extends wp.element.Component {
return () => {
this.props.onInsertBlock( slug );
this.props.onSelect();
this.setState( { filterValue: '' } );
this.setState( {
filterValue: '',
currentFocus: null
} );
};
}

render() {
const { position = 'top' } = this.props;
const blocks = wp.blocks.getBlocks();
const isShownBlock = block => block.title.toLowerCase().indexOf( this.state.filterValue.toLowerCase() ) !== -1;
const blocksByCategory = blocks.reduce( ( groups, block ) => {
if ( ! isShownBlock( block ) ) {
return groups;
}
if ( ! groups[ block.category ] ) {
groups[ block.category ] = [];
getVisibleBlocks( blockTypes ) {
return blockTypes.filter( this.isShownBlock );
}

sortBlocksByCategory( blockTypes ) {
const getCategoryIndex = ( item ) => {
return this.categories.findIndex( ( category ) => category.slug === item.slug );
};

return blockTypes.sort( ( a, b ) => {
return getCategoryIndex( a ) - getCategoryIndex( b );
} );
}

groupByCategory( blockTypes ) {
return blockTypes.reduce( ( accumulator, block ) => {
// If already an array push block on else add array of block.
Copy link
Member

Choose a reason for hiding this comment

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

We could simplify this somewhat by just creating the array if not exists and using the same push and return behavior.

return blockTypes.reduce( ( accumulator, block ) => {
	// If already an array push block on else add array of block.
	if ( ! accumulator[ block.category ] ) {
		accumulator[ block.category ] = [];
	}

	return accumulator[ block.category ].concat( block );
}, {} );

Alternatively, maybe this could be more easily achieved with Lodash's _.groupBy.

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 was considering just using lodash as well.

if ( Array.isArray( accumulator[ block.category ] ) ) {
accumulator[ block.category ].push( block );
return accumulator;
}
groups[ block.category ].push( block );
return groups;

accumulator[ block.category ] = [ block ];
return accumulator;
}, {} );
const categories = wp.blocks.getCategories();
}

getVisibleBlocksByCategory( blockTypes ) {
return flow(
this.getVisibleBlocks,
this.sortBlocksByCategory,
this.groupByCategory
)( blockTypes );
}

findNext( currentBlock, blockTypes ) {
/**
* 'search' or null are the values that will trigger iterating back to
* the top of the list of block types.
*/
if ( null === currentBlock || 'search' === currentBlock ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should drop the second part of this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup.

return blockTypes[ 0 ].slug;
}

const currentIndex = blockTypes.findIndex( ( blockType ) => currentBlock === blockType.slug );
Copy link
Member

Choose a reason for hiding this comment

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

We don't use the Babel polyfill currently, meaning we don't have access to ES2015 base type instance methods like Array#findIndex, String#startsWith, etc. Static members like Object.assign and Array.from are fair game however.

We could decide to use the polyfill, but it adds non-trivial weight to the build. The developer experience implications could make it worth including.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about using lodash again?

Copy link
Member

Choose a reason for hiding this comment

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

How about using lodash again?

Yep, that's perfectly appropriate in my book:

const nextIndex = currentIndex + 1;
const highestIndex = blockTypes.length - 1;

/**
* Default currently for going past the blocks is search, may need to be
* revised in the future as more focusable elements are added. This
* returns a null value, which currently implies that search will be set
* as the next focus.
*/
if ( nextIndex > highestIndex ) {
return null;
}

// Return the slug of the next block type.
return blockTypes[ nextIndex ].slug;
}

findPrevious( currentBlock, blockTypes ) {
/**
Copy link
Member

Choose a reason for hiding this comment

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

There's a fair bit of overlap between findNext and findPrevious implementations. I'm wondering if it could be consolidated into a single one, or at least a single base implementation with aliases. Something like:

findByIncrement( increment = 1 ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that could work, it could either be a boon or add more work in the future, but I think this is a good idea.

* null will trigger iterating back to the top of the list of block
* types.
*/
if ( null === currentBlock ) {
return blockTypes[ 0 ].slug;
}

const highestIndex = blockTypes.length - 1;

// If the search bar is focused navigate to the bottom of the block list.
if ( 'search' === currentBlock ) {
return blockTypes[ highestIndex ].slug;
}

const currentIndex = blockTypes.findIndex( ( blockType ) => currentBlock === blockType.slug );
const previousIndex = currentIndex - 1;
const lowestIndex = 0;

/**
* Default currently for going past the blocks is search, may need to be
* revised in the future as more focusable elements are added. This
* returns a null value, which currently implies that search will be set
* as the next focus.
*/
if ( previousIndex < lowestIndex ) {
return null;
}

// Return the slug of the next block type.
return blockTypes[ previousIndex ].slug;
}

focusNext( component ) {
const sortedByCategory = flow(
this.getVisibleBlocks,
this.sortBlocksByCategory,
)( component.blockTypes );
const currentBlock = component.state.currentFocus;

let nextBlock = this.findNext( currentBlock, sortedByCategory );

if ( nextBlock === null ) {
nextBlock = 'search';
}

this.changeMenuSelection( nextBlock );
}

focusPrevious( component ) {
const sortedByCategory = flow(
this.getVisibleBlocks,
this.sortBlocksByCategory,
)( component.blockTypes );
const currentBlock = component.state.currentFocus;

let nextBlock = this.findPrevious( currentBlock, sortedByCategory );

if ( nextBlock === null ) {
nextBlock = 'search';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplication, should we move the special case to changeMenuSelection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that works.


this.changeMenuSelection( nextBlock );
}

onKeyDown( keydown ) {
if ( this.isNextKeydown( keydown ) ) {
keydown.preventDefault();
this.focusNext( this );
Copy link
Member

Choose a reason for hiding this comment

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

Why do we pass this as an argument? this would still be available in the body of the function being called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a hangover from when these were separated out into another file, I can change. I get lazy I guess.

}

if ( this.isPreviousKeydown( keydown ) ) {
keydown.preventDefault();
this.focusPrevious( this );
}

if ( this.isEscapeKey( keydown ) ) {
keydown.preventDefault();
this.props.closeMenu();
}
}

changeMenuSelection( refName ) {
this.setState( {
currentFocus: refName
} );

// Focus the DOM node.
this.nodes[ refName ].focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we're leveraging the "focus" to select a node? Is this for accessibility or something?

I guess I'd prefer to avoid the imperative approach and use a className or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keyboard navigation with arrows means focus must be moved to the DOM element, this needs to happen on any "widget" that implements arrow navigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would have been a lot easier if you didn't need to actually focus any of the nodes. It is not pretty, but I don't have a better solution.

}

setSearchFocus() {
this.changeMenuSelection( 'search' );
}

render() {
const { position = 'top' } = this.props;
const blocks = this.blockTypes;
const visibleBlocksByCategory = this.getVisibleBlocksByCategory( blocks );
const categories = this.categories;

return (
<div className={ `editor-inserter__menu is-${ position }` }>
<div className={ `editor-inserter__menu is-${ position }` } tabIndex="0">
<div className="editor-inserter__arrow" />
<div className="editor-inserter__content">
<div role="menu" className="editor-inserter__content">
{ categories
.map( ( category ) => !! blocksByCategory[ category.slug ] && (
.map( ( category ) => !! visibleBlocksByCategory[ category.slug ] && (
<div key={ category.slug }>
<div
className="editor-inserter__separator"
Expand All @@ -69,12 +262,14 @@ class InserterMenu extends wp.element.Component {
tabIndex="0"
aria-labelledby={ `editor-inserter__separator-${ category.slug }-${ this.instanceId }` }
>
{ blocksByCategory[ category.slug ].map( ( { slug, title, icon } ) => (
{ visibleBlocksByCategory[ category.slug ].map( ( { slug, title, icon } ) => (
<button
role="menuitem"
key={ slug }
className="editor-inserter__block"
onClick={ this.selectBlock( slug ) }
ref={ this.bindReferenceNode( slug ) }
tabIndex="-1"
>
<Dashicon icon={ icon } />
{ title }
Expand All @@ -94,6 +289,8 @@ class InserterMenu extends wp.element.Component {
placeholder={ wp.i18n.__( 'Search…' ) }
className="editor-inserter__search"
onChange={ this.filter }
onClick={ this.setSearchFocus }
ref={ this.bindReferenceNode( 'search' ) }
/>
</div>
);
Expand Down
4 changes: 3 additions & 1 deletion editor/components/inserter/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
outline: none;
transition: color .2s ease;

&:hover {
&:hover,
&:focus {
color: $blue-medium;
}
}
Expand Down Expand Up @@ -145,6 +146,7 @@ input[type=search].editor-inserter__search {
&:focus {
border: 1px solid $dark-gray-500;
outline: none;
position: relative;
}

&:active,
Expand Down