Merge pull request #63 from sparksuite/rethink-behavior

Rethink behavior on button clicks

Author: WesCossick

README.md

@@ -48,7 +48,8 @@ Done!
 ## Accessibility notes
 Our team carefully studied and adhered to [Web Content Accessibility Guidelines 2.1](https://www.w3.org/WAI/standards-guidelines/wcag/) and  [WAI-ARIA Authoring Practices 1.1](https://www.w3.org/TR/wai-aria-practices/) when designing this Hook. Here are some facets of accessibility that are handled automatically:
 
-- Strict adherence to the best practices for menus ([WAI-ARIA: 3.15](https://www.w3.org/TR/wai-aria-practices/#menu))
+- Careful following of the best practices for menus ([WAI-ARIA: 3.15](https://www.w3.org/TR/wai-aria-practices/#menu))
+  - The only deviation is that the first menu item is only focused when the menu is revealed via the keyboard ([see why](https://github.com/sparksuite/react-accessible-dropdown-menu-hook/pull/63))
 - Strict adherence to the best practices for menu buttons ([WAI-ARIA: 3.16](https://www.w3.org/TR/wai-aria-practices/#menubutton))
 - Full keyboard accessibility ([WCAG: 2.1](https://www.w3.org/WAI/WCAG21/quickref/#keyboard-accessible))
 - Use of ARIA properties and roles to establish relationships amongst elements ([WCAG: 1.3.1](https://www.w3.org/WAI/WCAG21/quickref/#info-and-relationships))

package.json

@@ -1,6 +1,6 @@
 {
 	"name": "react-accessible-dropdown-menu-hook",
-	"version": "1.0.0",
+	"version": "1.1.0",
 	"description": "A simple Hook for creating fully accessible dropdown menus in React",
 	"main": "dist/use-dropdown-menu.js",
 	"types": "dist/use-dropdown-menu.d.ts",
@@ -77,6 +77,7 @@
 		"collectCoverageFrom": [
 			"<rootDir>/src/**"
 		],
+		"verbose": true,
 		"projects": [
 			{
 				"displayName": "Enzyme",
@@ -86,17 +87,15 @@
 				"testMatch": [
 					"<rootDir>/test/*.test.ts",
 					"<rootDir>/test/*.test.tsx"
-				],
-				"verbose": true
+				]
 			},
 			{
 				"displayName": "Puppeteer",
 				"preset": "jest-puppeteer",
 				"testMatch": [
 					"<rootDir>/test/puppeteer/*.test.ts",
 					"<rootDir>/test/puppeteer/*.test.tsx"
-				],
-				"verbose": true
+				]
 			}
 		]
 	}

src/use-dropdown-menu.ts

@@ -7,6 +7,7 @@ export default function useDropdownMenu(itemCount: number) {
 	const [isOpen, setIsOpen] = useState<boolean>(false);
 	const currentFocusIndex = useRef<number | null>(null);
 	const firstRun = useRef(true);
+	const clickedOpen = useRef(false);
 
 	// Create refs
 	const buttonRef = useRef<HTMLButtonElement>(null);
@@ -31,8 +32,10 @@ export default function useDropdownMenu(itemCount: number) {
 		}
 
 		// If the menu is currently open focus on the first item in the menu
-		if (isOpen) {
+		if (isOpen && !clickedOpen.current) {
 			moveFocus(0);
+		} else if (!isOpen) {
+			clickedOpen.current = false;
 		}
 	}, [isOpen]);
 
@@ -53,7 +56,7 @@ export default function useDropdownMenu(itemCount: number) {
 				}
 
 				// Ignore if we're clicking inside the menu
-				if (event.target.closest('[role="menu"]')) {
+				if (event.target.closest('[role="menu"]') instanceof Element) {
 					return;
 				}
 
@@ -75,16 +78,19 @@ export default function useDropdownMenu(itemCount: number) {
 		if (isKeyboardEvent(e)) {
 			const { key } = e;
 
-			if (key !== 'Tab' && key !== 'Shift') {
-				e.preventDefault();
+			if (!['Enter', ' ', 'Tab', 'ArrowDown'].includes(key)) {
+				return;
 			}
 
-			if (key === 'Enter' || key === ' ') {
+			if ((key === 'Tab' || key === 'ArrowDown') && clickedOpen.current && isOpen) {
+				e.preventDefault();
+				moveFocus(0);
+			} else if (key !== 'Tab') {
+				e.preventDefault();
 				setIsOpen(true);
-			} else if (key === 'Tab') {
-				setIsOpen(false);
 			}
 		} else {
+			clickedOpen.current = !isOpen;
 			setIsOpen(!isOpen);
 		}
 	};

test/puppeteer/demo.test.ts

@@ -20,16 +20,16 @@ it('has the correct page title', async () => {
 	await expect(page.title()).resolves.toMatch('React Accessible Dropdown Menu Hook');
 });
 
-it('focuses on the first menu item when the enter key is pressed', async () => {
-	await page.focus('#menu-button');
-	await keyboard.down('Enter');
+it('leaves focus on the button after clicking it', async () => {
+	await page.click('#menu-button');
 	await menuOpen();
 
-	expect(await currentFocusID()).toBe('menu-item-1');
+	expect(await currentFocusID()).toBe('menu-button');
 });
 
 it('focuses on the menu button after pressing escape', async () => {
-	await page.click('#menu-button');
+	await page.focus('#menu-button');
+	await keyboard.down('Enter');
 	await menuOpen();
 
 	await keyboard.down('Escape');
@@ -39,7 +39,8 @@ it('focuses on the menu button after pressing escape', async () => {
 });
 
 it('focuses on the next item in the tab order after pressing tab', async () => {
-	await page.click('#menu-button');
+	await page.focus('#menu-button');
+	await keyboard.down('Enter');
 	await menuOpen();
 
 	await keyboard.down('Tab');
@@ -49,7 +50,8 @@ it('focuses on the next item in the tab order after pressing tab', async () => {
 });
 
 it('focuses on the previous item in the tab order after pressing shift-tab', async () => {
-	await page.click('#menu-button');
+	await page.focus('#menu-button');
+	await keyboard.down('Enter');
 	await menuOpen();
 
 	await keyboard.down('Shift');
@@ -60,17 +62,19 @@ it('focuses on the previous item in the tab order after pressing shift-tab', asy
 });
 
 it('closes the menu if you click outside of it', async () => {
-	await page.click('#menu-button');
+	await page.focus('#menu-button');
+	await keyboard.down('Enter');
 	await menuOpen();
 
-	await page.click('body');
+	await page.click('h1');
 	await menuClosed(); // times out if menu doesn't close
 
 	expect(true).toBe(true);
 });
 
 it('leaves the menu open if you click inside of it', async () => {
-	await page.click('#menu-button');
+	await page.focus('#menu-button');
+	await keyboard.down('Enter');
 	await menuOpen();
 
 	await page.click('#menu-item-1');
@@ -87,7 +91,8 @@ it('leaves the menu open if you click inside of it', async () => {
 it('reroutes enter presses on menu items as clicks', async () => {
 	let alertAppeared = false;
 
-	await page.click('#menu-button');
+	await page.focus('#menu-button');
+	await keyboard.down('Enter');
 	await menuOpen();
 
 	// eslint-disable-next-line @typescript-eslint/no-misused-promises

test/use-dropdown-menu.test.tsx

@@ -18,6 +18,36 @@ it('moves the focus to the first menu item after pressing enter while focused on
 	expect(document.activeElement?.id).toBe('menu-item-1');
 });
 
+it('moves the focus to the first menu item after pressing space while focused on the menu button', () => {
+	const component = mount(<TestComponent />);
+	const button = component.find('#menu-button');
+
+	button.getDOMNode<HTMLButtonElement>().focus();
+	button.simulate('keydown', { key: ' ' });
+
+	expect(document.activeElement?.id).toBe('menu-item-1');
+});
+
+it('moves the focus to the first menu item after clicking the menu to open it, then pressing tab while focused on the menu button', () => {
+	const component = mount(<TestComponent />);
+	const button = component.find('#menu-button');
+
+	button.simulate('click');
+	button.simulate('keydown', { key: 'Tab' });
+
+	expect(document.activeElement?.id).toBe('menu-item-1');
+});
+
+it('moves the focus to the first menu item after clicking the menu to open it, then pressing arrow down while focused on the menu button', () => {
+	const component = mount(<TestComponent />);
+	const button = component.find('#menu-button');
+
+	button.simulate('click');
+	button.simulate('keydown', { key: 'ArrowDown' });
+
+	expect(document.activeElement?.id).toBe('menu-item-1');
+});
+
 it('sets isOpen to true after pressing enter while focused on the menu button', () => {
 	const component = mount(<TestComponent />);
 	const button = component.find('#menu-button');
@@ -29,6 +59,17 @@ it('sets isOpen to true after pressing enter while focused on the menu button',
 	expect(span.text()).toBe('true');
 });
 
+it('sets isOpen to true after pressing space while focused on the menu button', () => {
+	const component = mount(<TestComponent />);
+	const button = component.find('#menu-button');
+	const span = component.find('#is-open-indicator');
+
+	button.getDOMNode<HTMLButtonElement>().focus();
+	button.simulate('keydown', { key: ' ' });
+
+	expect(span.text()).toBe('true');
+});
+
 it('moves the focus to the next element in the menu after pressing the down arrow', () => {
 	const component = mount(<TestComponent />);
 	const button = component.find('#menu-button');