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 shortcut aria label for unreadable shortcuts #9582

Merged
merged 9 commits into from Sep 13, 2018
2 changes: 1 addition & 1 deletion edit-post/components/header/index.js
Expand Up @@ -61,7 +61,7 @@ function Header( {
onClick={ toggleGeneralSidebar }
isToggled={ isEditorSidebarOpened }
aria-expanded={ isEditorSidebarOpened }
shortcut={ shortcuts.toggleSidebar.display }
shortcut={ shortcuts.toggleSidebar }
/>
<DotTip id="core/editor.settings">
{ __( 'You’ll find more settings for your page and blocks in the sidebar. Click “Settings” to open it.' ) }
Expand Down
13 changes: 10 additions & 3 deletions edit-post/components/keyboard-shortcut-help-modal/config.js
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { displayShortcutList } from '@wordpress/keycodes';
import { displayShortcutList, shortcutAriaLabel } from '@wordpress/keycodes';
import { __ } from '@wordpress/i18n';

const {
Expand Down Expand Up @@ -42,18 +42,21 @@ const globalShortcuts = {
{
keyCombination: primaryShift( ',' ),
description: __( 'Show or hide the settings sidebar.' ),
ariaLabel: shortcutAriaLabel.primaryShift( ',' ),
},
{
keyCombination: ctrl( '`' ),
description: __( 'Navigate to a the next part of the editor.' ),
description: __( 'Navigate to the next part of the editor.' ),
ariaLabel: shortcutAriaLabel.ctrl( '`' ),
},
{
keyCombination: ctrlShift( '`' ),
description: __( 'Navigate to the previous part of the editor.' ),
ariaLabel: shortcutAriaLabel.ctrlShift( '`' ),
},
{
keyCombination: shiftAlt( 'n' ),
description: __( 'Navigate to a the next part of the editor (alternative).' ),
description: __( 'Navigate to the next part of the editor (alternative).' ),
},
{
keyCombination: shiftAlt( 'p' ),
Expand All @@ -76,6 +79,8 @@ const selectionShortcuts = {
{
keyCombination: 'Esc',
description: __( 'Clear selection.' ),
/* translators: The 'escape' key on a keyboard. */
ariaLabel: __( 'Escape' ),
},
],
};
Expand All @@ -102,6 +107,8 @@ const blockShortcuts = {
{
keyCombination: '/',
description: __( 'Change the block type after adding a new paragraph.' ),
/* translators: The forward-slash character. e.g. '/'. */
ariaLabel: __( 'Forward-slash' ),
},
],
};
Expand Down
4 changes: 2 additions & 2 deletions edit-post/components/keyboard-shortcut-help-modal/index.js
Expand Up @@ -42,13 +42,13 @@ const mapKeyCombination = ( keyCombination ) => keyCombination.map( ( character,

const ShortcutList = ( { shortcuts } ) => (
<dl className="edit-post-keyboard-shortcut-help__shortcut-list">
{ shortcuts.map( ( { keyCombination, description }, index ) => (
{ shortcuts.map( ( { keyCombination, description, ariaLabel }, index ) => (
<div
className="edit-post-keyboard-shortcut-help__shortcut"
key={ index }
>
<dt className="edit-post-keyboard-shortcut-help__shortcut-term">
<kbd className="edit-post-keyboard-shortcut-help__shortcut-key-combination">
<kbd className="edit-post-keyboard-shortcut-help__shortcut-key-combination" aria-label={ ariaLabel }>
{ mapKeyCombination( castArray( keyCombination ) ) }
</kbd>
</dt>
Expand Down
Expand Up @@ -63,6 +63,7 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
],
},
Object {
"ariaLabel": "Control + Shift + Comma",
"description": "Show or hide the settings sidebar.",
"keyCombination": Array [
"Ctrl",
Expand All @@ -73,14 +74,16 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
],
},
Object {
"description": "Navigate to a the next part of the editor.",
"ariaLabel": "Control + Backtick",
"description": "Navigate to the next part of the editor.",
"keyCombination": Array [
"Ctrl",
"+",
"\`",
],
},
Object {
"ariaLabel": "Control + Shift + Backtick",
"description": "Navigate to the previous part of the editor.",
"keyCombination": Array [
"Ctrl",
Expand All @@ -91,7 +94,7 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
],
},
Object {
"description": "Navigate to a the next part of the editor (alternative).",
"description": "Navigate to the next part of the editor (alternative).",
"keyCombination": Array [
"Shift",
"+",
Expand Down Expand Up @@ -139,6 +142,7 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
],
},
Object {
"ariaLabel": "Escape",
"description": "Clear selection.",
"keyCombination": "Esc",
},
Expand Down Expand Up @@ -191,6 +195,7 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
],
},
Object {
"ariaLabel": "Forward-slash",
"description": "Change the block type after adding a new paragraph.",
"keyCombination": "/",
},
Expand Down
2 changes: 1 addition & 1 deletion edit-post/components/sidebar/sidebar-header/index.js
Expand Up @@ -37,7 +37,7 @@ const SidebarHeader = ( { children, className, closeLabel, closeSidebar, title }
onClick={ closeSidebar }
icon="no-alt"
label={ closeLabel }
shortcut={ shortcuts.toggleSidebar.display }
shortcut={ shortcuts.toggleSidebar }
/>
</div>
</Fragment>
Expand Down
Expand Up @@ -40,7 +40,7 @@ export function BlockInspectorButton( {
onClick={ flow( areAdvancedSettingsOpened ? closeSidebar : openEditorSidebar, speakMessage, onClick ) }
icon="admin-generic"
label={ small ? label : undefined }
shortcut={ shortcuts.toggleSidebar.display }
shortcut={ shortcuts.toggleSidebar }
>
{ ! small && label }
</MenuItem>
Expand Down
3 changes: 2 additions & 1 deletion edit-post/keyboard-shortcuts.js
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { rawShortcut, displayShortcut } from '@wordpress/keycodes';
import { rawShortcut, displayShortcut, shortcutAriaLabel } from '@wordpress/keycodes';

export default {
toggleEditorMode: {
Expand All @@ -11,5 +11,6 @@ export default {
toggleSidebar: {
raw: rawShortcut.primaryShift( ',' ),
display: displayShortcut.primaryShift( ',' ),
ariaLabel: shortcutAriaLabel.primaryShift( ',' ),
},
};
6 changes: 3 additions & 3 deletions packages/components/src/menu-item/index.js
Expand Up @@ -13,7 +13,7 @@ import { cloneElement } from '@wordpress/element';
* Internal dependencies
*/
import Button from '../button';
import Shortcut from './shortcut';
import Shortcut from '../shortcut';
import IconButton from '../icon-button';

/**
Expand Down Expand Up @@ -45,7 +45,7 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected, r
{ ...props }
>
{ children }
<Shortcut shortcut={ shortcut } />
<Shortcut className="components-menu-item__shortcut" shortcut={ shortcut } />
</IconButton>
);
}
Expand All @@ -59,7 +59,7 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected, r
{ ...props }
>
{ children }
<Shortcut shortcut={ shortcut } />
<Shortcut className="components-menu-item__shortcut" shortcut={ shortcut } />
</Button>
);
}
Expand Down
10 changes: 0 additions & 10 deletions packages/components/src/menu-item/shortcut.js

This file was deleted.

Expand Up @@ -9,7 +9,8 @@ exports[`MenuItem should match snapshot when all props provided 1`] = `
role="menuitemcheckbox"
>
My item
<MenuItemsShortcut
<Shortcut
className="components-menu-item__shortcut"
shortcut="mod+shift+alt+w"
/>
</IconButton>
Expand All @@ -23,7 +24,8 @@ exports[`MenuItem should match snapshot when isSelected and role are optionally
role="menuitem"
>
My item
<MenuItemsShortcut
<Shortcut
className="components-menu-item__shortcut"
shortcut="mod+shift+alt+w"
/>
</IconButton>
Expand All @@ -35,6 +37,8 @@ exports[`MenuItem should match snapshot when only label provided 1`] = `
role="menuitem"
>
My item
<MenuItemsShortcut />
<Shortcut
className="components-menu-item__shortcut"
/>
</Button>
`;
30 changes: 30 additions & 0 deletions packages/components/src/shortcut/index.js
@@ -0,0 +1,30 @@
/**
* External dependencies
*/
import { isString, isObject } from 'lodash';

function Shortcut( { shortcut, className } ) {
if ( ! shortcut ) {
return null;
}

let displayText;
let ariaLabel;

if ( isString( shortcut ) ) {
displayText = shortcut;
}

if ( isObject( shortcut ) ) {
displayText = shortcut.display;
ariaLabel = shortcut.ariaLabel;
}

return (
<span className={ className } aria-label={ ariaLabel }>
{ displayText }
</span>
);
}

export default Shortcut;
41 changes: 41 additions & 0 deletions packages/components/src/shortcut/test/index.js
@@ -0,0 +1,41 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';

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

describe( 'Shortcut', () => {
it( 'does not render anything if no shortcut prop is provided', () => {
const wrapper = shallow( <Shortcut /> );

expect( wrapper.children() ).toHaveLength( 0 );
} );

it( 'renders the shortcut display text when a string is passed as the shortcut', () => {
const wrapper = shallow(
<Shortcut
shortcut="shortcut text"
/>
);

expect( wrapper.text() ).toBe( 'shortcut text' );
} );

it( 'renders the shortcut display text and aria-label when an object is passed as the shortcut with the correct properties', () => {
const wrapper = shallow(
<Shortcut
shortcut={ {
display: 'shortcut text',
ariaLabel: 'shortcut label',
} }
/>
);

expect( wrapper.text() ).toBe( 'shortcut text' );
expect( wrapper.prop( 'aria-label' ) ).toBe( 'shortcut label' );
} );
} );
3 changes: 2 additions & 1 deletion packages/components/src/tooltip/index.js
Expand Up @@ -18,6 +18,7 @@ import {
* Internal dependencies
*/
import Popover from '../popover';
import Shortcut from '../shortcut';

/**
* Time over children to wait before showing tooltip
Expand Down Expand Up @@ -178,7 +179,7 @@ class Tooltip extends Component {
aria-hidden="true"
>
{ text }
{ shortcut && <span className="components-tooltip__shortcut">{ shortcut }</span> }
<Shortcut className="components-tooltip__shortcut" shortcut={ shortcut } />
</Popover>
),
),
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/tooltip/test/index.js
Expand Up @@ -49,7 +49,7 @@ describe( 'Tooltip', () => {
expect( button.childAt( 1 ).name() ).toBe( 'Popover' );
expect( popover.prop( 'focusOnMount' ) ).toBe( false );
expect( popover.prop( 'position' ) ).toBe( 'bottom right' );
expect( popover.children().text() ).toBe( 'Help text' );
expect( popover.children().first().text() ).toBe( 'Help text' );
} );

it( 'should show popover on focus', () => {
Expand Down