From 31e29de024736f39a999f0bf31237d5f313cefed Mon Sep 17 00:00:00 2001 From: Chris Bedwell Date: Fri, 25 Aug 2023 16:00:58 +0100 Subject: [PATCH] A11y: Fix toggletip predictable focus for keyboard users (#72100) * Updated toggletip to use strategy fixed, added tests and temporary story so it can easily be checked * Added FocusScope restoreFocus and appropriate tests to toggletip * Open toggletip in test for making sure focus remains when using escape * Add aria-expanded to toggletip toggle child * Added to temp story for Toggletip * Remove focusScope for toggletip and handle focus restoration manually * Remove toggletip temp story and add toggletip long content story --------- Co-authored-by: joshhunt --- .../src/components/Toggletip/Toggletip.mdx | 4 +- .../components/Toggletip/Toggletip.story.tsx | 51 ++++++++ .../components/Toggletip/Toggletip.test.tsx | 115 +++++++++++++++++- .../src/components/Toggletip/Toggletip.tsx | 97 ++++++++------- 4 files changed, 217 insertions(+), 50 deletions(-) diff --git a/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx b/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx index 111569e56f7..53b75f5b5cb 100644 --- a/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx +++ b/packages/grafana-ui/src/components/Toggletip/Toggletip.mdx @@ -64,7 +64,7 @@ function onClose() { } return ( - - + ); ``` diff --git a/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx b/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx index 5c853261ab9..8a720c02ae3 100644 --- a/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx +++ b/packages/grafana-ui/src/components/Toggletip/Toggletip.story.tsx @@ -5,6 +5,7 @@ import { SelectableValue } from '@grafana/data'; import { withCenteredStory } from '../../utils/storybook/withCenteredStory'; import { Button } from '../Button'; +import { CustomScrollbar } from '../CustomScrollbar/CustomScrollbar'; import { ButtonSelect } from '../Dropdown/ButtonSelect'; import { InlineField } from '../Forms/InlineField'; import { Icon } from '../Icon/Icon'; @@ -175,4 +176,54 @@ HostingMultiElements.args = { theme: 'info', }; +export const LongContent: StoryFn = ({ + title, + content, + footer, + theme, + closeButton, + placement, + ...args +}) => { + return ( + Toggletip with scrollable content and no interactive controls} + content={ + + {/* one of the few documented cases we can turn this rule off */} + {/* https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/docs/rules/no-noninteractive-tabindex.md#case-shouldnt-i-add-a-tabindex-so-that-users-can-navigate-to-this-item */} + {/* eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex */} +
+

+ If for any reason you have to use a Toggletip with a lot of content with no interactive controls, set a{' '} + tabIndex=0 attribute to the container so keyboard users are able to focus the content and + able to scroll up and down it. +

+ {new Array(15).fill(undefined).map((_, i) => ( +

This is some content repeated over and over again to ensure it is scrollable.

+ ))} +
+ + } + footer={footer} + theme={theme} + placement={placement} + {...args} + > + + + ); +}; +LongContent.args = { + placement: 'auto', + theme: 'info', +}; + +LongContent.parameters = { + controls: { + hideNoControlsWarning: true, + exclude: ['title', 'content', 'children'], + }, +}; + export default meta; diff --git a/packages/grafana-ui/src/components/Toggletip/Toggletip.test.tsx b/packages/grafana-ui/src/components/Toggletip/Toggletip.test.tsx index 8e40d7a600a..358477e9a3b 100644 --- a/packages/grafana-ui/src/components/Toggletip/Toggletip.test.tsx +++ b/packages/grafana-ui/src/components/Toggletip/Toggletip.test.tsx @@ -1,4 +1,4 @@ -import { render, screen } from '@testing-library/react'; +import { act, render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; @@ -7,7 +7,7 @@ import { Button } from '../Button'; import { Toggletip } from './Toggletip'; describe('Toggletip', () => { - it('should display toogletip after click on "Click me!" button', async () => { + it('should display toggletip after click on "Click me!" button', async () => { render( + + + + ); + + expect(screen.queryByTestId('toggletip-content')).not.toBeInTheDocument(); + + const button = screen.getByTestId('myButton'); + const afterButton = screen.getByText(afterInDom); + await userEvent.click(button); + await userEvent.tab(); + const closeButton = screen.getByTestId('toggletip-header-close'); + expect(closeButton).toHaveFocus(); + + // focus after + await userEvent.tab(); + expect(afterButton).toHaveFocus(); + + // focus backwards + await userEvent.tab({ shift: true }); + expect(closeButton).toHaveFocus(); + + // focus back to togglebutton + await userEvent.tab({ shift: true }); + expect(button).toHaveFocus(); + }); + + describe('Focus state', () => { + let user: ReturnType; + + beforeEach(() => { + jest.useFakeTimers(); + // Need to use delay: null here to work with fakeTimers + // see https://github.com/testing-library/user-event/issues/833 + user = userEvent.setup({ delay: null }); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('should restore focus to the button that opened the toggletip when closed from within the toggletip', async () => { + const closeSpy = jest.fn(); + render( + + + + ); + + const button = screen.getByTestId('myButton'); + await user.click(button); + const closeButton = await screen.findByTestId('toggletip-header-close'); + expect(closeButton).toBeInTheDocument(); + await user.click(closeButton); + act(() => { + jest.runAllTimers(); + }); + + expect(button).toHaveFocus(); + }); + + it('should NOT restore focus to the button that opened the toggletip when closed from outside the toggletip', async () => { + const closeSpy = jest.fn(); + const afterInDom = 'Outside of toggletip'; + + render( + <> + + + + + + ); + + const button = screen.getByTestId('myButton'); + await user.click(button); + const closeButton = await screen.findByTestId('toggletip-header-close'); + + expect(closeButton).toBeInTheDocument(); + const afterButton = screen.getByText(afterInDom); + afterButton.focus(); + + await user.keyboard('{escape}'); + act(() => { + jest.runAllTimers(); + }); + + expect(afterButton).toHaveFocus(); + }); + }); }); diff --git a/packages/grafana-ui/src/components/Toggletip/Toggletip.tsx b/packages/grafana-ui/src/components/Toggletip/Toggletip.tsx index 21afa268c89..35699b9c6bd 100644 --- a/packages/grafana-ui/src/components/Toggletip/Toggletip.tsx +++ b/packages/grafana-ui/src/components/Toggletip/Toggletip.tsx @@ -8,7 +8,6 @@ import { GrafanaTheme2 } from '@grafana/data'; import { useStyles2 } from '../../themes/ThemeContext'; import { buildTooltipTheme } from '../../utils/tooltipUtils'; import { IconButton } from '../IconButton/IconButton'; -import { Portal } from '../Portal/Portal'; import { ToggletipContent } from './types'; @@ -50,16 +49,43 @@ export const Toggletip = React.memo( const contentRef = useRef(null); const [controlledVisible, setControlledVisible] = React.useState(false); - const closeToggletip = useCallback(() => { - setControlledVisible(false); - onClose?.(); - }, [onClose]); + const { getArrowProps, getTooltipProps, setTooltipRef, setTriggerRef, visible, update, tooltipRef, triggerRef } = + usePopperTooltip( + { + visible: controlledVisible, + placement: placement, + interactive: true, + offset: [0, 8], + trigger: 'click', + onVisibleChange: (value: boolean) => { + setControlledVisible(value); + if (!value) { + onClose?.(); + } + }, + }, + { + strategy: 'fixed', + } + ); + + const closeToggletip = useCallback( + (event: KeyboardEvent | React.MouseEvent) => { + setControlledVisible(false); + onClose?.(); + + if (event.target instanceof Node && tooltipRef?.contains(event.target)) { + triggerRef?.focus(); + } + }, + [onClose, tooltipRef, triggerRef] + ); useEffect(() => { if (controlledVisible) { const handleKeyDown = (enterKey: KeyboardEvent) => { if (enterKey.key === 'Escape') { - closeToggletip(); + closeToggletip(enterKey); } }; document.addEventListener('keydown', handleKeyDown); @@ -70,52 +96,37 @@ export const Toggletip = React.memo( return; }, [controlledVisible, closeToggletip]); - const { getArrowProps, getTooltipProps, setTooltipRef, setTriggerRef, visible, update } = usePopperTooltip({ - visible: controlledVisible, - placement: placement, - interactive: true, - offset: [0, 8], - trigger: 'click', - onVisibleChange: (value: boolean) => { - setControlledVisible(value); - if (!value) { - onClose?.(); - } - }, - }); - return ( <> {React.cloneElement(children, { ref: setTriggerRef, tabIndex: 0, + 'aria-expanded': visible, })} {visible && ( - -
- {Boolean(title) &&
{title}
} - {closeButton && ( -
- -
- )} -
-
- {(typeof content === 'string' || React.isValidElement(content)) && content} - {typeof content === 'function' && update && content({ update })} +
+ {Boolean(title) &&
{title}
} + {closeButton && ( +
+
- {Boolean(footer) &&
{footer}
} + )} +
+
+ {(typeof content === 'string' || React.isValidElement(content)) && content} + {typeof content === 'function' && update && content({ update })}
- + {Boolean(footer) &&
{footer}
} +
)} );