Skip to content

Commit

Permalink
Fix dropdown menu focus loss when using arrow keys with Safari and Vo…
Browse files Browse the repository at this point in the history
…iceover (#24186)
  • Loading branch information
talldan committed Jul 27, 2020
1 parent 2f11cc7 commit d984bcc
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 10 deletions.
9 changes: 7 additions & 2 deletions packages/components/src/navigable-container/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { omit, noop, isFunction } from 'lodash';
import { Component, forwardRef } from '@wordpress/element';
import { focus } from '@wordpress/dom';

const MENU_ITEM_ROLES = [ 'menuitem', 'menuitemradio', 'menuitemcheckbox' ];

function cycleValue( value, total, offset ) {
const nextValue = value + offset;
if ( nextValue < 0 ) {
Expand Down Expand Up @@ -96,8 +98,11 @@ class NavigableContainer extends Component {
event.stopImmediatePropagation();

// When navigating a collection of items, prevent scroll containers
// from scrolling.
if ( event.target.getAttribute( 'role' ) === 'menuitem' ) {
// from scrolling. The preventDefault also prevents Voiceover from
// 'handling' the event, as voiceover will try to use arrow keys
// for highlighting text.
const targetRole = event.target.getAttribute( 'role' );
if ( MENU_ITEM_ROLES.includes( targetRole ) ) {
event.preventDefault();
}
}
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/navigable-container/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ export function NavigableMenu(
return 1;
} else if ( includes( previous, keyCode ) ) {
return -1;
} else if ( includes( [ DOWN, UP, LEFT, RIGHT ], keyCode ) ) {
// Key press should be handled, e.g. have event propagation and
// default behavior handled by NavigableContainer but not result
// in an offset.
return 0;
}
};

Expand Down
16 changes: 8 additions & 8 deletions packages/components/src/navigable-container/test/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ describe( 'NavigableMenu', () => {
assertKeyDown( UP, 2, true );
assertKeyDown( UP, 1, true );
assertKeyDown( UP, 0, true );
assertKeyDown( LEFT, 0, false );
assertKeyDown( RIGHT, 0, false );
assertKeyDown( LEFT, 0, true );
assertKeyDown( RIGHT, 0, true );
assertKeyDown( SPACE, 0, false );
} );

Expand Down Expand Up @@ -146,8 +146,8 @@ describe( 'NavigableMenu', () => {
assertKeyDown( UP, 1, true );
assertKeyDown( UP, 0, true );
assertKeyDown( UP, 0, true );
assertKeyDown( LEFT, 0, false );
assertKeyDown( RIGHT, 0, false );
assertKeyDown( LEFT, 0, true );
assertKeyDown( RIGHT, 0, true );
assertKeyDown( SPACE, 0, false );
} );

Expand Down Expand Up @@ -204,8 +204,8 @@ describe( 'NavigableMenu', () => {
assertKeyDown( LEFT, 2, true );
assertKeyDown( LEFT, 1, true );
assertKeyDown( LEFT, 0, true );
assertKeyDown( UP, 0, false );
assertKeyDown( DOWN, 0, false );
assertKeyDown( UP, 0, true );
assertKeyDown( DOWN, 0, true );
assertKeyDown( SPACE, 0, false );
} );

Expand Down Expand Up @@ -256,8 +256,8 @@ describe( 'NavigableMenu', () => {
assertKeyDown( LEFT, 1, true );
assertKeyDown( LEFT, 0, true );
assertKeyDown( LEFT, 0, true );
assertKeyDown( DOWN, 0, false );
assertKeyDown( UP, 0, false );
assertKeyDown( DOWN, 0, true );
assertKeyDown( UP, 0, true );
assertKeyDown( SPACE, 0, false );
} );

Expand Down
144 changes: 144 additions & 0 deletions packages/e2e-tests/specs/editor/various/dropdown-menu.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/**
* WordPress dependencies
*/
import { createNewPost, pressKeyTimes } from '@wordpress/e2e-test-utils';

const moreMenuButtonSelector =
'.components-button[aria-label="More tools & options"]';
const moreMenuDropdownSelector =
'.components-dropdown-menu__menu[aria-label="More tools & options"]';
const menuItemsSelector = [ 'menuitem', 'menuitemcheckbox', 'menuitemradio' ]
.map( ( role ) => `${ moreMenuDropdownSelector } [role="${ role }"]` )
.join( ',' );

describe( 'Dropdown Menu', () => {
beforeEach( async () => {
await createNewPost();
} );

it( 'allows navigation through each item using arrow keys', async () => {
await page.click( moreMenuButtonSelector );
const menuItems = await page.$$( menuItemsSelector );

// Catch any issues with the selector, which could cause a false positive test result.
expect( menuItems.length ).toBeGreaterThan( 0 );

let activeElementText = await page.evaluate(
() => document.activeElement.textContent
);
const [ firstMenuItem ] = menuItems;
const firstMenuItemText = await firstMenuItem.evaluate(
( element ) => element.textContent
);

// Expect the first menu item to be focused.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( firstMenuItemText );

// Arrow down to the last item.
await pressKeyTimes( 'ArrowDown', menuItems.length - 1 );

activeElementText = await page.evaluate(
() => document.activeElement.textContent
);

const [ lastMenuItem ] = menuItems.slice( -1 );
const lastMenuItemText = await lastMenuItem.evaluate(
( element ) => element.textContent
);

// Expect the last menu item to be focused.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( lastMenuItemText );

// Arrow back up to the first item.
await pressKeyTimes( 'ArrowUp', menuItems.length - 1 );

activeElementText = await page.evaluate(
() => document.activeElement.textContent
);

// Expect the first menu item to be focused again.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( firstMenuItemText );
} );

it( 'loops to the beginning and end when navigating past the boundaries of the menu', async () => {
await page.click( moreMenuButtonSelector );
const menuItems = await page.$$( menuItemsSelector );

// Catch any issues with the selector, which could cause a false positive test result.
expect( menuItems.length ).toBeGreaterThan( 0 );

let activeElementText = await page.evaluate(
() => document.activeElement.textContent
);
const [ firstMenuItem ] = menuItems;
const firstMenuItemText = await firstMenuItem.evaluate(
( element ) => element.textContent
);

// Expect the first menu item to be focused.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( firstMenuItemText );

// Arrow up to the last item.
await page.keyboard.press( 'ArrowUp' );

activeElementText = await page.evaluate(
() => document.activeElement.textContent
);

const [ lastMenuItem ] = menuItems.slice( -1 );
const lastMenuItemText = await lastMenuItem.evaluate(
( element ) => element.textContent
);

// Expect the last menu item to be focused.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( lastMenuItemText );

// Arrow back down to the first item.
await page.keyboard.press( 'ArrowDown' );

activeElementText = await page.evaluate(
() => document.activeElement.textContent
);

// Expect the first menu item to be focused again.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( firstMenuItemText );
} );

it( 'ignores arrow key navigation that is orthogonal to the orientation of the menu, but stays open', async () => {
await page.click( moreMenuButtonSelector );
const menuItems = await page.$$( menuItemsSelector );

// Catch any issues with the selector, which could cause a false positive test result.
expect( menuItems.length ).toBeGreaterThan( 0 );

let activeElementText = await page.evaluate(
() => document.activeElement.textContent
);
const [ firstMenuItem ] = menuItems;
const firstMenuItemText = await firstMenuItem.evaluate(
( element ) => element.textContent
);

// Expect the first menu item to be focused.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( firstMenuItemText );

// Press left and right keys an arbitrary (but > 1) number of times.
await pressKeyTimes( 'ArrowLeft', 5 );
await pressKeyTimes( 'ArrowRight', 5 );

activeElementText = await page.evaluate(
() => document.activeElement.textContent
);

// Expect the first menu item to still be focused.
expect( activeElementText ).toBeDefined();
expect( activeElementText ).toBe( firstMenuItemText );
} );
} );

0 comments on commit d984bcc

Please sign in to comment.