[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 <[email protected]>

Author: gitstart-sourcegraph

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<HTMLButtonElement>('[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))
     })
 })

client/web/src/nav/NavBar/NavDropdown.tsx

@@ -184,8 +184,8 @@ export const NavDropdown: React.FunctionComponent<React.PropsWithChildren<NavDro
                                 >
                                     {mobileHomeItem.content}
                                 </MenuLink>
-                                {items.map(item => (
-                                    <MenuLink as={Link} key={item.path} to={item.path}>
+                                {items.map((item, index) => (
+                                    <MenuLink as={Link} key={item.path} to={item.path} index={index}>
                                         {item.content}
                                     </MenuLink>
                                 ))}

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>

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).

client/wildcard/src/components/Popover/components/floating-panel/FloatingPanel.tsx

@@ -22,6 +22,8 @@ export interface FloatingPanelProps extends Omit<Tether, 'target' | 'element'>,
      * 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) {

client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx

@@ -17,6 +17,7 @@ export interface PopoverContentProps extends Omit<FloatingPanelProps, 'target' |
     isOpen?: boolean
     focusLocked?: boolean
     autoFocus?: boolean
+    keepInDOM?: boolean
 }
 
 export const PopoverContent = forwardRef((props, reference) => {
@@ -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 ? (
                 <FocusLock disabled={!focusLock} returnFocus={true}>

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)
 

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