Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
7 changes: 6 additions & 1 deletion editor/components/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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 +55,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
265 changes: 246 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,58 @@ 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 );
}

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 ) {
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 === 'Tab' && keydown.shiftKey === false );
}

isArrowRight( keydown ) {
return keydown.code === 'ArrowRight';
}

isArrowLeft( keydown ) {
return keydown.code === 'ArrowLeft';
}

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

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

filter( event ) {
Expand All @@ -29,32 +77,206 @@ 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 ) {
/**
* null is the value that will trigger iterating back to
* the top of the list of block types.
*/
if ( null === currentBlock ) {
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 );

// If the block list is empty return early.
if ( ! sortedByCategory.length ) {
return;
}

const currentBlock = component.state.currentFocus;

const nextBlock = this.findNext( currentBlock, sortedByCategory );
this.changeMenuSelection( nextBlock );
}

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

// If the block list is empty return early.
if ( ! sortedByCategory.length ) {
return;
}

const nextBlock = this.findPrevious( currentBlock, sortedByCategory );
Copy link
Member

Choose a reason for hiding this comment

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

We should treat state as a single source of truth; since findPrevious has access to this.state, there's no need to pass it as an argument.

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 );
}

/**
* Left and right arrow keys need to be handled seperately so that
* default cursor behavior can be handled in the search field.
*/
if ( this.isArrowRight( keydown ) ) {
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 not overly concerned about it, but I wonder if it would have really been any less clear to just do a switch statement here instead of extracting out to a number of additional instance methods. Like:

switch ( event.keyCode ) {
	case 39 /* ArrowRight */:
		if ( this.state.currentFocus !== 'search' ) {
			this.focusNext();
		}
	
	// ...
}

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, I like the use of a switch statement, slight optimization, and possibly more easy to read.

if ( this.state.currentFocus === 'search' ) {
return;
}
this.focusNext( this );
}

if ( this.isArrowLeft( keydown ) ) {
if ( this.state.currentFocus === 'search' ) {
return;
}
this.focusPrevious( this );
}

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

changeMenuSelection( refName ) {
if ( refName === null ) {
refName = 'search';
}

this.setState( {
currentFocus: refName
} );

// Focus the DOM node.
this.nodes[ refName ].focus();
}

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 +291,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 +318,9 @@ class InserterMenu extends wp.element.Component {
placeholder={ wp.i18n.__( 'Search…' ) }
className="editor-inserter__search"
onChange={ this.filter }
onClick={ this.setSearchFocus }
ref={ this.bindReferenceNode( 'search' ) }
tabIndex="-1"
/>
</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