-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changes from 4 commits
624381f
f8a52f7
7fd578b
153adb2
1e9551a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import IconButton from 'components/icon-button'; | |
class Inserter extends wp.element.Component { | ||
constructor() { | ||
super( ...arguments ); | ||
this.nodes = {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you prefer to keep this line or change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I bet you tried to remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||
|
@@ -20,6 +21,10 @@ class Inserter extends wp.element.Component { | |
} | ||
|
||
toggle() { | ||
if ( this.state.opened ) { | ||
this.toggleNode.focus(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hiding it with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What does React have to do with focus loss caused by elements being removed or hidden? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Regardless of tool, it's an undesirable complexity to programmatically manage focus as we've found to need here. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
|
||
this.setState( { | ||
opened: ! this.state.opened | ||
} ); | ||
|
@@ -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> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
* External dependencies | ||
*/ | ||
import { connect } from 'react-redux'; | ||
import { flow } from 'lodash'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we just call to |
||
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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd have to double check. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code#Browser_compatibility Even though it's now deprecated, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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 ) { | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we should drop the second part of this check There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using lodash again? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 ) { | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a fair bit of overlap between
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid duplication, should we move the special case to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we pass There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
@@ -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 } | ||
|
@@ -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> | ||
); | ||
|
There was a problem hiding this comment.
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 thisThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thebutton
element. And it's not a valid prop forbutton
. So to avoid this, we should deconstructbuttonRef
in the function props to omit it fromadditionalProps
.Something like:
function Button( { isPrimary, isLarge, isToggled, className, buttonRef, ...additionalProps } ) {
There was a problem hiding this comment.
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.