Add shortcut aria label for unreadable shortcuts (WordPress#9582)

* Add new utility function that returns an aria label describing the shortcut * Add Shortcut component * Use new Shortcut component in MenuItem and Tooltip components * Add aria-label for toggleSidebar shortcut to ensure comma is read by screenreaders * On Apple systems do not separate keys in shortcut aria label by +, this matches how shortcut is rendered * Improve naming in keycodes package * Remove unused class name * Add aria labels for shortcuts in keyboard shortcut help modal * Fix copy typo
jwold · Sep 13, 2018 · 6712e28 · 6712e28
1 parent 4c579ed
commit 6712e28
Show file tree

Hide file tree

Showing 16 changed files with 197 additions and 36 deletions.
diff --git a/edit-post/components/header/index.js b/edit-post/components/header/index.js
@@ -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.' ) }

diff --git a/edit-post/components/keyboard-shortcut-help-modal/config.js b/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 {
@@ -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' ),
@@ -76,6 +79,8 @@ const selectionShortcuts = {
 		{
 			keyCombination: 'Esc',
 			description: __( 'Clear selection.' ),
+			/* translators: The 'escape' key on a keyboard. */
+			ariaLabel: __( 'Escape' ),
 		},
 	],
 };
@@ -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' ),
 		},
 	],
 };

diff --git a/edit-post/components/keyboard-shortcut-help-modal/index.js b/edit-post/components/keyboard-shortcut-help-modal/index.js
@@ -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>

diff --git a/edit-post/components/keyboard-shortcut-help-modal/test/__snapshots__/index.js.snap b/edit-post/components/keyboard-shortcut-help-modal/test/__snapshots__/index.js.snap
@@ -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",
@@ -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",
@@ -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",
               "+",
@@ -139,6 +142,7 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
             ],
           },
           Object {
+            "ariaLabel": "Escape",
             "description": "Clear selection.",
             "keyCombination": "Esc",
           },
@@ -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": "/",
           },

diff --git a/edit-post/components/sidebar/sidebar-header/index.js b/edit-post/components/sidebar/sidebar-header/index.js
@@ -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>

diff --git a/edit-post/components/visual-editor/block-inspector-button.js b/edit-post/components/visual-editor/block-inspector-button.js
@@ -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>

diff --git a/edit-post/keyboard-shortcuts.js b/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: {
@@ -11,5 +11,6 @@ export default {
 	toggleSidebar: {
 		raw: rawShortcut.primaryShift( ',' ),
 		display: displayShortcut.primaryShift( ',' ),
+		ariaLabel: shortcutAriaLabel.primaryShift( ',' ),
 	},
 };
diff --git a/packages/components/src/menu-item/index.js b/packages/components/src/menu-item/index.js
@@ -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';
 
 /**
@@ -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>
 		);
 	}
@@ -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>
 	);
 }

diff --git a/packages/components/src/menu-item/shortcut.js b/packages/components/src/menu-item/shortcut.js
diff --git a/packages/components/src/menu-item/test/__snapshots__/index.js.snap b/packages/components/src/menu-item/test/__snapshots__/index.js.snap
@@ -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>
@@ -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>
@@ -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>
 `;
diff --git a/packages/components/src/shortcut/index.js b/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;
diff --git a/packages/components/src/shortcut/test/index.js b/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' );
+	} );
+} );
diff --git a/packages/components/src/tooltip/index.js b/packages/components/src/tooltip/index.js
@@ -18,6 +18,7 @@ import {
  * Internal dependencies
  */
 import Popover from '../popover';
+import Shortcut from '../shortcut';
 
 /**
  * Time over children to wait before showing tooltip
@@ -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>
 				),
 			),

diff --git a/packages/components/src/tooltip/test/index.js b/packages/components/src/tooltip/test/index.js
@@ -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', () => {