From d30b22a9751c61bb7731cb6eb69bfcdf270ccf0c Mon Sep 17 00:00:00 2001 From: GitStart-SourceGraph <89894075+gitstart-sourcegraph@users.noreply.github.com> Date: Mon, 4 Jul 2022 17:43:15 +0200 Subject: [PATCH] [SG-36350] Accessibility: Global `Menu`: Focus isn't correctly captured when menus are opened (#36915) * feat: keep MenuList in DOM when Menu is hidden Co-authored-by: gitstart-sourcegraph --- .../DashboardsContentPage.test.tsx | 20 +++++++++---------- client/web/src/nav/NavBar/NavDropdown.tsx | 4 ++-- .../wildcard/src/components/Menu/MenuList.tsx | 1 + .../wildcard/src/components/Popover/README.md | 2 ++ .../floating-panel/FloatingPanel.tsx | 5 +++++ .../popover-content/PopoverContent.tsx | 8 ++++++-- .../Popover/tether/services/tether-render.ts | 2 +- .../Popover/tether/services/types.ts | 2 ++ 8 files changed, 28 insertions(+), 16 deletions(-) diff --git a/client/web/src/enterprise/insights/pages/dashboards/dashboard-page/DashboardsContentPage.test.tsx b/client/web/src/enterprise/insights/pages/dashboards/dashboard-page/DashboardsContentPage.test.tsx index 1f44196a2b917..f3ca7391fade4 100644 --- a/client/web/src/enterprise/insights/pages/dashboards/dashboard-page/DashboardsContentPage.test.tsx +++ b/client/web/src/enterprise/insights/pages/dashboards/dashboard-page/DashboardsContentPage.test.tsx @@ -165,15 +165,14 @@ const renderDashboardsContent = ( const triggerDashboardMenuItem = async (screen: RenderWithBrandedContextResult & { user: UserEvent }, name: RegExp) => { const { user } = screen - const dashboardMenu = await waitFor(() => screen.getByRole('button', { name: /dashboard context menu/ })) - user.click(dashboardMenu) - const dashboardMenuItem = screen.getByRole('menuitem', { name }) + const dashboardMenu = await screen.findByRole('button', { name: /dashboard context menu/ }) + userEvent.click(dashboardMenu) // We're simulating keyboard navigation here to circumvent a bug in ReachUI // does not respond to programmatic click events on menu items - dashboardMenuItem.focus() - user.keyboard(' ') + screen.getByText(name).closest('[role="menuitem"]')?.focus() + user.keyboard('{enter}') } beforeEach(() => { @@ -218,7 +217,7 @@ describe('DashboardsContent', () => { const { history } = screen - await triggerDashboardMenuItem(screen, /configure dashboard/) + await triggerDashboardMenuItem(screen, /configure dashboard/i) expect(history.location.pathname).toEqual('/insights/dashboards/foo/edit') }) @@ -238,18 +237,17 @@ describe('DashboardsContent', () => { it('opens delete dashboard modal', async () => { const screen = renderDashboardsContent() - await triggerDashboardMenuItem(screen, /Delete/) + await triggerDashboardMenuItem(screen, /delete/i) - const addInsightHeader = await waitFor(() => screen.getByRole('heading', { name: /Delete/ })) - expect(addInsightHeader).toBeInTheDocument() + await waitFor(() => expect(screen.getByRole('heading', { name: /Delete/ })).toBeInTheDocument()) }) // copies dashboard url it('copies dashboard url', async () => { const screen = renderDashboardsContent() - await triggerDashboardMenuItem(screen, /Copy link/) + await triggerDashboardMenuItem(screen, /copy link/i) - sinon.assert.calledOnce(mockCopyURL) + await waitFor(() => sinon.assert.calledOnce(mockCopyURL)) }) }) diff --git a/client/web/src/nav/NavBar/NavDropdown.tsx b/client/web/src/nav/NavBar/NavDropdown.tsx index f0a26dd51736f..a65564014e55a 100644 --- a/client/web/src/nav/NavBar/NavDropdown.tsx +++ b/client/web/src/nav/NavBar/NavDropdown.tsx @@ -184,8 +184,8 @@ export const NavDropdown: React.FunctionComponent {mobileHomeItem.content} - {items.map(item => ( - + {items.map((item, index) => ( + {item.content} ))} diff --git a/client/wildcard/src/components/Menu/MenuList.tsx b/client/wildcard/src/components/Menu/MenuList.tsx index f0e026f523ddd..1eee4800e6606 100644 --- a/client/wildcard/src/components/Menu/MenuList.tsx +++ b/client/wildcard/src/components/Menu/MenuList.tsx @@ -46,5 +46,6 @@ const Popover = React.forwardRef(({ popoverPosition, ...props }, reference) => ( position={popoverPosition} focusLocked={false} className={classNames('py-1', props.className)} + keepInDOM={true} /> )) as ForwardReferenceComponent<'div', PopoverProps> diff --git a/client/wildcard/src/components/Popover/README.md b/client/wildcard/src/components/Popover/README.md index edf32af4a1079..bfb596a219f64 100644 --- a/client/wildcard/src/components/Popover/README.md +++ b/client/wildcard/src/components/Popover/README.md @@ -226,3 +226,5 @@ enum Strategy { ``` - **_targetPadding_** (optional) - Adds space/padding between target and popover elements + +- **_keepInDOM_** (optional) - By default `PopoverContent` element is removed from DOM when tooltip is hidden. If it's `true`, `PopoverContent` element will be kept in DOM but is hidden with CSS rule `visibility=hidden`. This prop is useful when `PopoverContent` children need this behavior to work. Ex: `@sourcegraph/wildcard` `MenuList` component need this to fix [focus state issue](https://github.com/sourcegraph/sourcegraph/issues/36350). diff --git a/client/wildcard/src/components/Popover/components/floating-panel/FloatingPanel.tsx b/client/wildcard/src/components/Popover/components/floating-panel/FloatingPanel.tsx index bac6ec7c49fad..12ecaa3281768 100644 --- a/client/wildcard/src/components/Popover/components/floating-panel/FloatingPanel.tsx +++ b/client/wildcard/src/components/Popover/components/floating-panel/FloatingPanel.tsx @@ -22,6 +22,8 @@ export interface FloatingPanelProps extends Omit, * outside the dom tree. */ rootRender?: HTMLElement | null + + forceHidden?: boolean } /** @@ -45,6 +47,7 @@ export const FloatingPanel = forwardRef((props, reference) => { targetPadding, constraint, rootRender, + forceHidden, ...otherProps } = props @@ -72,6 +75,7 @@ export const FloatingPanel = forwardRef((props, reference) => { constrainToScrollParents, overflowToScrollParents, flipping, + forceHidden, }) return unsubscribe @@ -90,6 +94,7 @@ export const FloatingPanel = forwardRef((props, reference) => { constrainToScrollParents, overflowToScrollParents, flipping, + forceHidden, ]) if (strategy === Strategy.Absolute) { diff --git a/client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx b/client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx index 290581102eed8..f9fcc889384f5 100644 --- a/client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx +++ b/client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx @@ -17,6 +17,7 @@ export interface PopoverContentProps extends Omit { @@ -28,9 +29,11 @@ export const PopoverContent = forwardRef((props, reference) => { as: Component = 'div', role = 'dialog', 'aria-modal': ariaModel = true, + keepInDOM = false, + // we should let FloatingPanel to control its `hidden` attribute + hidden, ...otherProps } = props - const { isOpen: isOpenContext, targetElement, tailElement, anchor, setOpen } = useContext(PopoverContext) const { renderRoot } = useContext(PopoverRoot) @@ -68,7 +71,7 @@ export const PopoverContent = forwardRef((props, reference) => { return () => setFocusLock(false) }, [autoFocus, focusLocked, tooltipElement]) - if (!isOpenContext && !isOpen) { + if (!keepInDOM && !isOpenContext && !isOpen) { return null } @@ -83,6 +86,7 @@ export const PopoverContent = forwardRef((props, reference) => { aria-modal={ariaModel} rootRender={renderRoot} className={classNames(styles.popover, otherProps.className)} + forceHidden={keepInDOM && !isOpenContext && !isOpen} > {focusLocked ? ( diff --git a/client/wildcard/src/components/Popover/tether/services/tether-render.ts b/client/wildcard/src/components/Popover/tether/services/tether-render.ts index a7a1ce863c460..bfeffd57a03cf 100644 --- a/client/wildcard/src/components/Popover/tether/services/tether-render.ts +++ b/client/wildcard/src/components/Popover/tether/services/tether-render.ts @@ -36,7 +36,7 @@ export function render(tether: Tether, eventTarget: HTMLElement | null, preserve const layout = getLayout(tether) const state = getState(layout) - if (state === null || !isVisible(tether.target)) { + if (state === null || !isVisible(tether.target) || tether.forceHidden) { setVisibility(tether.element, false) setVisibility((tether.marker as HTMLElement) ?? null, false) diff --git a/client/wildcard/src/components/Popover/tether/services/types.ts b/client/wildcard/src/components/Popover/tether/services/types.ts index 9ea82509d2742..1b7fb6aedef5b 100644 --- a/client/wildcard/src/components/Popover/tether/services/types.ts +++ b/client/wildcard/src/components/Popover/tether/services/types.ts @@ -55,6 +55,8 @@ export interface Tether { overflowToScrollParents?: boolean constrainToScrollParents?: boolean + + forceHidden?: boolean } export type MarkerElement = HTMLElement | SVGElement