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

Implement keyboard and focus management #3897

Closed
wants to merge 3 commits into from
Closed
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
22 changes: 22 additions & 0 deletions assets/src/edit-story/app/story/actions/useDeletePageByIndex.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* WordPress dependencies
*/
import { useCallback } from '@wordpress/element';

function useDeletePageByIndex( {
clearSelection,
setPages,
setCurrentPageIndex,
} ) {
const deletePageByIndex = useCallback( ( indexToDelete ) => {
clearSelection();
setPages( ( pages ) => [
...pages.slice( 0, indexToDelete ),
...pages.slice( indexToDelete + 1 ),
] );
setCurrentPageIndex( ( currentIndex ) => currentIndex > indexToDelete ? currentIndex - 1 : currentIndex );
barklund marked this conversation as resolved.
Show resolved Hide resolved
}, [ clearSelection, setPages, setCurrentPageIndex ] );
return deletePageByIndex;
}

export default useDeletePageByIndex;
14 changes: 6 additions & 8 deletions assets/src/edit-story/app/story/storyProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import useToggleElementIdInSelection from './actions/useToggleElementIdInSelecti
import useSelectElementById from './actions/useSelectElementById';
import useAppendElementToCurrentPage from './actions/useAppendElementToCurrentPage';
import useSetCurrentPageByIndex from './actions/useSetCurrentPageByIndex';
import useDeletePageByIndex from './actions/useDeletePageByIndex';
import useSetPropertiesOnSelectedElements from './actions/useSetPropertiesOnSelectedElements';

function StoryProvider( { storyId, children } ) {
Expand All @@ -37,17 +38,15 @@ function StoryProvider( { storyId, children } ) {
const [ currentPageIndex, setCurrentPageIndex ] = useState( null );
const [ selectedElementIds, setSelectedElementIds ] = useState( [] );

// These states are all derived from the above three variables and help keep the api easier.
// These will update based off the above in effects but should never be directly manipulated outside this component.
const [ currentPageNumber, setCurrentPageNumber ] = useState( null );
const [ currentPage, setCurrentPage ] = useState( null );
const [ selectedElements, setSelectedElements ] = useState( [] );

const hasSelection = Boolean( selectedElementIds.length );
const currentPage = pages[ currentPageIndex ] || null;
const currentPageNumber = ! currentPage ? null : currentPageIndex + 1;
const selectedElements = ! currentPage ? [] : currentPage.elements.filter( ( { id } ) => selectedElementIds.includes( id ) );
barklund marked this conversation as resolved.
Show resolved Hide resolved

const clearSelection = useClearSelection( { selectedElementIds, setSelectedElementIds } );
const deleteSelectedElements = useDeleteSelectedElements( { currentPageIndex, pages, selectedElementIds, setPages, setSelectedElementIds } );
const setCurrentPageByIndex = useSetCurrentPageByIndex( { clearSelection, setCurrentPageIndex } );
const deletePageByIndex = useDeletePageByIndex( { clearSelection, setPages, setCurrentPageIndex } );
const addBlankPage = useAddBlankPage( { pages, setPages, clearSelection } );
const deleteCurrentPage = useDeleteCurrentPage( { currentPage, pages, setPages, addBlankPage, setCurrentPageIndex, currentPageIndex } );
const selectElementById = useSelectElementById( { setSelectedElementIds } );
Expand All @@ -56,10 +55,8 @@ function StoryProvider( { storyId, children } ) {
const setPropertiesOnSelectedElements = useSetPropertiesOnSelectedElements( { currentPageIndex, pages, selectedElementIds, setPages } );

useLoadStory( { storyId, pages, setPages, setCurrentPageIndex, clearSelection } );
useCurrentPage( { currentPageIndex, pages, setCurrentPage, setCurrentPageNumber } );
useHistoryEntry( { currentPageIndex, pages, selectedElementIds } );
useHistoryReplay( { setCurrentPageIndex, setPages, setSelectedElementIds } );
useSelectedElements( { currentPageIndex, pages, selectedElementIds, setSelectedElements } );

const state = {
state: {
Expand All @@ -73,6 +70,7 @@ function StoryProvider( { storyId, children } ) {
},
actions: {
setCurrentPageByIndex,
deletePageByIndex,
addBlankPage,
clearSelection,
deleteSelectedElements,
Expand Down
59 changes: 50 additions & 9 deletions assets/src/edit-story/components/canvas/carrousel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,22 @@
* External dependencies
*/
import styled from 'styled-components';
import { rgba } from 'polished';
import PropTypes from 'prop-types';

/**
* Internal dependencies
*/
import { useStory } from '../../../app';
import { Navigable, NavigableGroup } from '../../focusable';

const List = styled.nav`
const List = styled( NavigableGroup ).attrs( {
element: 'nav',
role: 'tablist',
tabindex: -1,
direction: NavigableGroup.DIRECTION_HORIZONTAL,
hotkey: 'meta+t',
} )`
display: flex;
flex-direction: row;
align-items: flex-start;
Expand All @@ -17,27 +26,59 @@ const List = styled.nav`
padding-top: 1em;
`;

const Page = styled.a`
background-color: ${ ( { isActive, theme } ) => isActive ? theme.colors.fg.v1 : theme.colors.mg.v1 };
height: 48px;
width: 27px;
const Page = styled( Navigable ).attrs( {
element: 'button',
role: 'tab',
} )`
background-color: ${ ( { theme, isActive } ) => rgba( theme.colors.fg.v1, isActive ? 1 : 0.1 ) };
height: 90px;
width: 51px;
margin: 0 5px;
cursor: pointer;
border: 4px solid transparent;
padding: 0;

&:hover {
background-color: ${ ( { theme } ) => theme.colors.fg.v1 };
}

&:focus, &:active {
outline: none;

Choose a reason for hiding this comment

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

General believe, i think, is that outlines are good for accessibility. Is there any reason you don't like it here? I think replacing it with border is fine, but sometimes there are other factors. For instance, if UX asks to add an extra pixel to the outline, very little needs to change, vs making border thicker could cause relayout. But other than that, this is not critical.

Copy link
Contributor Author

@barklund barklund Dec 11, 2019

Choose a reason for hiding this comment

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

I'm am open to using either outline or border, no preference. However, the UX called for a specific design with a specific border and I wasn't able to replicate it using outline only. For a11y purposes I think we're safe as long as we do something else to provide highlight.

Choose a reason for hiding this comment

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

Can you point me to the precise place in figma with that style. I just want to confirm it once so we don't have to stumble on this each time.

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's shown in the section about deleting a page from the carousel:

Screen Shot 2019-12-11 at 2 51 48 PM

Choose a reason for hiding this comment

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

Ok. We'd just need to always reserve a border and only manipulate the border-color from transparent to focused color.

border-color: ${ ( { theme } ) => theme.colors.action };
}
`;

function Canvas() {
const { state: { pages, currentPageIndex }, actions: { setCurrentPageByIndex } } = useStory();
function CarouselPage( { index } ) {
const {
state: { currentPageIndex },
actions: { setCurrentPageByIndex, deletePageByIndex },
} = useStory();
const deletePage = () => deletePageByIndex( index );
const shortcuts = { backspace: deletePage, del: deletePage };
return (
<Page
shortcuts={ shortcuts }
onClick={ () => setCurrentPageByIndex( index ) }
isActive={ index === currentPageIndex }
/>
);
}

function Carousel() {
const {
state: { pages },
} = useStory();
return (
<List>
{ pages.map( ( page, index ) => (
<Page key={ index } onClick={ () => setCurrentPageByIndex( index ) } isActive={ index === currentPageIndex } />
<CarouselPage key={ index } index={ index } />
) ) }
</List>
);
}

export default Canvas;
export default Carousel;

CarouselPage.propTypes = {
index: PropTypes.number.isRequired,
};
82 changes: 82 additions & 0 deletions assets/src/edit-story/components/focusable/focusable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/**
* External dependencies
*/
import PropTypes from 'prop-types';
import MouseTrap from 'mousetrap';
import 'mousetrap/plugins/global-bind/mousetrap-global-bind';

/**
* WordPress dependencies
*/
import { useLayoutEffect, useRef } from '@wordpress/element';

/**
* Internal dependencies
*/
import useCombinedRefs from '../../utils/useCombinedRefs';
import getValidDOMAttributes from '../../utils/getValidDOMAttributes';

function Focusable( { shortcuts, hasFocus, isGlobal, element, className, role, tabindex, forwardedRef, children, ...rest } ) {
const ref = useRef();
const setRef = useCombinedRefs( ref, forwardedRef );

Choose a reason for hiding this comment

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

Why not just use React.forwardRef?

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 should also use forwardRef, yes (which allows me to use ref={} in the parent component), but I still need some way to merge the two refs, when needing the ref both in the component itself and in a parent component. It can't be done any other way, unfortunately.

Choose a reason for hiding this comment

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

A single forwarded ref usually does the trick for me, but sure, if there's need, let's keep.


useLayoutEffect( () => {
const mt = MouseTrap( ref.current );

Choose a reason for hiding this comment

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

Just to clarify: this shouldn't be new MouseTrap, should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mousetrap doesn't use new in their docs, but I do see that WP does in their implementation, that I was inspired by.

I feel that I remember seeing something about new syntax being discouraged, but I can't find it again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs say:

You can also create a new Mousetrap instance if you want to bind a bunch of things to the same element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I don't really know what the difference is here (that is, the actual effect of doing either). The current implementation works, but maybe not efficiently?


// Bind all listeners
const sequences = Object.keys( shortcuts );
sequences.forEach( ( sequence ) => {
const callback = shortcuts[ sequence ];
const bindFn = isGlobal ? 'bindGlobal' : 'bind';
mt[ bindFn ]( sequence, callback );
} );

// Clear listeners on unmount/remount
return () => mt.reset();
}, [ isGlobal, shortcuts ] );

// Force focus if relevant
useLayoutEffect( () => {
if ( hasFocus ) {
ref.current.focus();
}
}, [ hasFocus ] );

const Wrapper = element;

return (
<Wrapper ref={ setRef } className={ className } role={ role } tabIndex={ tabindex } { ...getValidDOMAttributes( rest ) }>
{ children }
</Wrapper>
);
}

Focusable.propTypes = {
children: PropTypes.oneOfType( [
PropTypes.arrayOf( PropTypes.node ),
PropTypes.node,
] ),
forwardedRef: PropTypes.object,

shortcuts: PropTypes.object,
hasFocus: PropTypes.bool,
isGlobal: PropTypes.bool,

element: PropTypes.string,
className: PropTypes.string,
role: PropTypes.string,
tabindex: PropTypes.number,
};

Focusable.defaultProps = {
shortcuts: {},
isGlobal: false,
hasFocus: false,

element: 'div',
className: '',
role: 'button',
tabindex: 0,
};

export default Focusable;
3 changes: 3 additions & 0 deletions assets/src/edit-story/components/focusable/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export { default as Focusable } from './focusable';
export { default as Navigable } from './navigable';
export { default as NavigableGroup } from './navigableGroup';
31 changes: 31 additions & 0 deletions assets/src/edit-story/components/focusable/navigable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* External dependencies
*/
import PropTypes from 'prop-types';

/**
* Internal dependencies
*/
import Focusable from './focusable';

function Navigable( { element, className, role, tabindex, children, ...rest } ) {
return (
<Focusable element={ element } className={ className } role={ role } tabindex={ tabindex } { ...rest }>
{ children }
</Focusable>
);
}

Navigable.propTypes = {
children: PropTypes.oneOfType( [
PropTypes.arrayOf( PropTypes.node ),
PropTypes.node,
] ),

element: PropTypes.string,
className: PropTypes.string,
role: PropTypes.string,
tabindex: PropTypes.number,
};

export default Navigable;
58 changes: 58 additions & 0 deletions assets/src/edit-story/components/focusable/navigableGroup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* External dependencies
*/
import PropTypes from 'prop-types';

/**
* WordPress dependencies
*/
import { useRef, useLayoutEffect, useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import getDirectionalKeysForNode from '../../utils/getDirectionalKeysForNode';
import Focusable from './focusable';

function NavigableGroup( { direction, element, className, role, tabindex, children } ) {
const ref = useRef();
const [ shortcuts, setShortcuts ] = useState( {} );

useLayoutEffect( () => {
const focus = ( node ) => node && node.focus();

if ( direction === NavigableGroup.DIRECTION_HORIZONTAL ) {
const { forward, backward } = getDirectionalKeysForNode( ref.current );

Choose a reason for hiding this comment

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

This makes sense, but to clarify: what does Tab do inside the NavigableGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pressing tab navigates between focusable elements using browser-native implementation. Just like enter clicks the currently focused element - this is also using browser-native behaviour. I believe that's excellent for a11y.

Choose a reason for hiding this comment

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

Ok. I think that's good. In some systems tab jumps between bigger blocks and arrows are used to navigate within the block. But I think for now a simple way is better. This might pop up for e.g. gallery, where there might be unlimited set of things to tab over, which could make tab harder to use to navigate to other elements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the tablist implementation at https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html and the one I built for WordPress core (click on share icon on https://wordpress.org/news/2019/12/wordpress-5-3-1-security-and-maintenance-release/embed/), pressing tab jumps to the current tab's content, not to the next tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the tablist implementation at https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html and the one I built for WordPress core (click on share icon on https://wordpress.org/news/2019/12/wordpress-5-3-1-security-and-maintenance-release/embed/), pressing tab jumps to the current tab's content, not to the next tab.

Thanks a lot for those links - very helpful!

Those definitely are different ways of implementing this. The inactive tabs don't participate in tab order at all it seems (tabindex="-1") - only the active tab and the tab content. tab and shift+tab then navigate between the active tab and the tab content in the regular way based off document order.

And they have an example of both automatic and manual tab switching. Very interesting approaches - I will read up on those!

setShortcuts( {
[ forward ]: ( evt ) => focus( evt.target.nextElementSibling ),
[ backward ]: ( evt ) => focus( evt.target.previousElementSibling ),
} );
}
}, [ direction ] );

return (
<Focusable shortcuts={ shortcuts } forwardedRef={ ref } element={ element } className={ className } role={ role } tabindex={ tabindex }>
{ children }
</Focusable>
);
}

NavigableGroup.propTypes = {
children: PropTypes.oneOfType( [
PropTypes.arrayOf( PropTypes.node ),
PropTypes.node,
] ).isRequired,

direction: PropTypes.string,
hotkey: PropTypes.string,
element: PropTypes.string,
className: PropTypes.string,
role: PropTypes.string,
tabindex: PropTypes.number,
};

export default NavigableGroup;

NavigableGroup.DIRECTION_HORIZONTAL = 'horizontal';
NavigableGroup.DIRECTION_VERTICAL = 'vertical';
NavigableGroup.DIRECTION_GRID = 'grid';
Loading