From 15194b41b435231586175f67ed53583380765dcf Mon Sep 17 00:00:00 2001 From: Ashley Harrison Date: Tue, 19 Mar 2024 10:22:17 +0000 Subject: [PATCH] Dropdown: Fix keyboard accessibility (#84683) * fix dropdown keyboard a11y * remove unnecessary css * restore tabIndex to keep linting happy * use Box in Menu * fix unit test --- .../src/components/Dropdown/Dropdown.tsx | 6 +- .../grafana-ui/src/components/Menu/Menu.tsx | 29 ++++---- .../src/components/Menu/MenuItem.tsx | 6 +- .../src/components/Menu/SubMenu.test.tsx | 4 +- .../src/components/Menu/SubMenu.tsx | 74 +++++++++---------- .../src/components/Menu/hooks.test.tsx | 7 +- .../grafana-ui/src/components/Menu/hooks.ts | 9 +-- .../contact-points/ContactPoints.test.tsx | 2 + 8 files changed, 58 insertions(+), 79 deletions(-) diff --git a/packages/grafana-ui/src/components/Dropdown/Dropdown.tsx b/packages/grafana-ui/src/components/Dropdown/Dropdown.tsx index 759c96b0dc7..e1bc28cab9b 100644 --- a/packages/grafana-ui/src/components/Dropdown/Dropdown.tsx +++ b/packages/grafana-ui/src/components/Dropdown/Dropdown.tsx @@ -1,5 +1,6 @@ import { css } from '@emotion/css'; import { + FloatingFocusManager, autoUpdate, flip, offset as floatingUIOffset, @@ -9,7 +10,6 @@ import { useFloating, useInteractions, } from '@floating-ui/react'; -import { FocusScope } from '@react-aria/focus'; import React, { useEffect, useRef, useState } from 'react'; import { CSSTransition } from 'react-transition-group'; @@ -83,7 +83,7 @@ export const Dropdown = React.memo(({ children, overlay, placement, offset, onVi })} {show && ( - + {/* this is handling bubbled events from the inner overlay see https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/docs/rules/no-static-element-interactions.md#case-the-event-handler-is-only-being-used-to-capture-bubbled-events @@ -100,7 +100,7 @@ export const Dropdown = React.memo(({ children, overlay, placement, offset, onVi
{ReactUtils.renderOrCallToRender(overlay, { ...getFloatingProps() })}
- + )} diff --git a/packages/grafana-ui/src/components/Menu/Menu.tsx b/packages/grafana-ui/src/components/Menu/Menu.tsx index 4d52692fe69..9dc03f1ee55 100644 --- a/packages/grafana-ui/src/components/Menu/Menu.tsx +++ b/packages/grafana-ui/src/components/Menu/Menu.tsx @@ -4,6 +4,7 @@ import React, { useImperativeHandle, useRef } from 'react'; import { GrafanaTheme2 } from '@grafana/data'; import { useStyles2 } from '../../themes'; +import { Box } from '../Layout/Box/Box'; import { MenuDivider } from './MenuDivider'; import { MenuGroup } from './MenuGroup'; @@ -27,17 +28,22 @@ const MenuComp = React.forwardRef( const localRef = useRef(null); useImperativeHandle(forwardedRef, () => localRef.current!); - const [handleKeys] = useMenuFocus({ localRef, onOpen, onClose, onKeyDown }); + const [handleKeys] = useMenuFocus({ isMenuOpen: true, localRef, onOpen, onClose, onKeyDown }); return ( -
{header && (
(
)} {children} -
+ ); } ); @@ -66,17 +72,10 @@ export const Menu = Object.assign(MenuComp, { const getStyles = (theme: GrafanaTheme2) => { return { header: css({ - padding: `${theme.spacing(0.5, 1, 1, 1)}`, + padding: theme.spacing(0.5, 1, 1, 1), }), headerBorder: css({ borderBottom: `1px solid ${theme.colors.border.weak}`, }), - wrapper: css({ - background: `${theme.colors.background.primary}`, - boxShadow: `${theme.shadows.z3}`, - display: `inline-block`, - borderRadius: `${theme.shape.radius.default}`, - padding: `${theme.spacing(0.5, 0)}`, - }), }; }; diff --git a/packages/grafana-ui/src/components/Menu/MenuItem.tsx b/packages/grafana-ui/src/components/Menu/MenuItem.tsx index 49f3d751f0a..b6947643064 100644 --- a/packages/grafana-ui/src/components/Menu/MenuItem.tsx +++ b/packages/grafana-ui/src/components/Menu/MenuItem.tsx @@ -79,7 +79,6 @@ export const MenuItem = React.memo( const styles = useStyles2(getStyles); const [isActive, setIsActive] = useState(active); const [isSubMenuOpen, setIsSubMenuOpen] = useState(false); - const [openedWithArrow, setOpenedWithArrow] = useState(false); const onMouseEnter = useCallback(() => { if (disabled) { return; @@ -128,7 +127,6 @@ export const MenuItem = React.memo( event.stopPropagation(); if (hasSubMenu) { setIsSubMenuOpen(true); - setOpenedWithArrow(true); setIsActive(true); } break; @@ -178,8 +176,6 @@ export const MenuItem = React.memo( @@ -219,7 +215,7 @@ const getStyles = (theme: GrafanaTheme2) => { width: '100%', position: 'relative', - '&:hover, &:focus, &:focus-visible': { + '&:hover, &:focus-visible': { background: theme.colors.action.hover, color: theme.colors.text.primary, textDecoration: 'none', diff --git a/packages/grafana-ui/src/components/Menu/SubMenu.test.tsx b/packages/grafana-ui/src/components/Menu/SubMenu.test.tsx index 67ea2daa520..b86d43e5e34 100644 --- a/packages/grafana-ui/src/components/Menu/SubMenu.test.tsx +++ b/packages/grafana-ui/src/components/Menu/SubMenu.test.tsx @@ -13,9 +13,7 @@ describe('SubMenu', () => { , ]; - render( - - ); + render(); expect(screen.getByTestId(selectors.components.Menu.SubMenu.icon)).toBeInTheDocument(); diff --git a/packages/grafana-ui/src/components/Menu/SubMenu.tsx b/packages/grafana-ui/src/components/Menu/SubMenu.tsx index 5016f72b776..50b49828874 100644 --- a/packages/grafana-ui/src/components/Menu/SubMenu.tsx +++ b/packages/grafana-ui/src/components/Menu/SubMenu.tsx @@ -17,10 +17,6 @@ export interface SubMenuProps { items?: Array>; /** Open */ isOpen: boolean; - /** Marks whether subMenu was opened with arrow */ - openedWithArrow: boolean; - /** Changes value of openedWithArrow */ - setOpenedWithArrow: (openedWithArrow: boolean) => void; /** Closes the subMenu */ close: () => void; /** Custom style */ @@ -28,46 +24,42 @@ export interface SubMenuProps { } /** @internal */ -export const SubMenu = React.memo( - ({ items, isOpen, openedWithArrow, setOpenedWithArrow, close, customStyle }: SubMenuProps) => { - const styles = useStyles2(getStyles); - const localRef = useRef(null); - const [handleKeys] = useMenuFocus({ - localRef, - isMenuOpen: isOpen, - openedWithArrow, - setOpenedWithArrow, - close, - }); +export const SubMenu = React.memo(({ items, isOpen, close, customStyle }: SubMenuProps) => { + const styles = useStyles2(getStyles); + const localRef = useRef(null); + const [handleKeys] = useMenuFocus({ + localRef, + isMenuOpen: isOpen, + close, + }); - const [pushLeft, setPushLeft] = useState(false); - useEffect(() => { - if (isOpen && localRef.current) { - setPushLeft(isElementOverflowing(localRef.current)); - } - }, [isOpen]); + const [pushLeft, setPushLeft] = useState(false); + useEffect(() => { + if (isOpen && localRef.current) { + setPushLeft(isElementOverflowing(localRef.current)); + } + }, [isOpen]); - return ( - <> -
- -
- {isOpen && ( -
-
- {items} -
+ return ( + <> +
+ +
+ {isOpen && ( +
+
+ {items}
- )} - - ); - } -); +
+ )} + + ); +}); SubMenu.displayName = 'SubMenu'; diff --git a/packages/grafana-ui/src/components/Menu/hooks.test.tsx b/packages/grafana-ui/src/components/Menu/hooks.test.tsx index 254f41c7a8a..4ca9e69045c 100644 --- a/packages/grafana-ui/src/components/Menu/hooks.test.tsx +++ b/packages/grafana-ui/src/components/Menu/hooks.test.tsx @@ -141,18 +141,15 @@ describe('useMenuFocus', () => { expect(onKeyDown).toHaveBeenCalledTimes(2); }); - it('focuses on first item when menu was opened with arrow', () => { + it('focuses on first item', () => { const ref = createRef(); render(getMenuElement(ref)); const isMenuOpen = true; - const openedWithArrow = true; - const setOpenedWithArrow = jest.fn(); - renderHook(() => useMenuFocus({ localRef: ref, isMenuOpen, openedWithArrow, setOpenedWithArrow })); + renderHook(() => useMenuFocus({ localRef: ref, isMenuOpen })); expect(screen.getByText('Item 1').tabIndex).toBe(0); - expect(setOpenedWithArrow).toHaveBeenCalledWith(false); }); it('clicks focused item when Enter key is pressed', () => { diff --git a/packages/grafana-ui/src/components/Menu/hooks.ts b/packages/grafana-ui/src/components/Menu/hooks.ts index 6713535827a..b6111f9804f 100644 --- a/packages/grafana-ui/src/components/Menu/hooks.ts +++ b/packages/grafana-ui/src/components/Menu/hooks.ts @@ -8,8 +8,6 @@ const UNFOCUSED = -1; export interface UseMenuFocusProps { localRef: RefObject; isMenuOpen?: boolean; - openedWithArrow?: boolean; - setOpenedWithArrow?: (openedWithArrow: boolean) => void; close?: () => void; onOpen?: (focusOnItem: (itemId: number) => void) => void; onClose?: () => void; @@ -23,8 +21,6 @@ export type UseMenuFocusReturn = [(event: React.KeyboardEvent) => void]; export const useMenuFocus = ({ localRef, isMenuOpen, - openedWithArrow, - setOpenedWithArrow, close, onOpen, onClose, @@ -33,11 +29,10 @@ export const useMenuFocus = ({ const [focusedItem, setFocusedItem] = useState(UNFOCUSED); useEffect(() => { - if (isMenuOpen && openedWithArrow) { + if (isMenuOpen) { setFocusedItem(0); - setOpenedWithArrow?.(false); } - }, [isMenuOpen, openedWithArrow, setOpenedWithArrow]); + }, [isMenuOpen]); useEffect(() => { const menuItems = localRef?.current?.querySelectorAll( diff --git a/public/app/features/alerting/unified/components/contact-points/ContactPoints.test.tsx b/public/app/features/alerting/unified/components/contact-points/ContactPoints.test.tsx index c6fd8c71ca7..325867253b8 100644 --- a/public/app/features/alerting/unified/components/contact-points/ContactPoints.test.tsx +++ b/public/app/features/alerting/unified/components/contact-points/ContactPoints.test.tsx @@ -126,6 +126,8 @@ describe('contact points', () => { await userEvent.click(button); const deleteButton = await screen.queryByRole('menuitem', { name: 'delete' }); expect(deleteButton).toBeDisabled(); + // click outside the menu to close it otherwise we can't interact with the rest of the page + await userEvent.click(document.body); } // check buttons in Notification Templates