CORE-2875: Stop rendering menu popovers as dialogs

CHANGELOG.md

@@ -6,6 +6,32 @@ All notable changes to this project will be documented in this file.
 
 ### Fixed
 
+#### Menu popovers no longer render as dialogs (CORE-2875)
+
+react-aria-components' `Popover` gives itself `role="dialog"` unless `isNonModal` is set, and
+none of our menus set it. So every `MenuTrigger` → `Popover` → `Menu` put a dialog around
+the menu, with the trigger labelling both. Screen readers announced "Help menu, dialog" and
+then "Help menu, menu". The dialog also made the menu modal: a full-screen underlay, a focus
+trap, a scroll-locked body, and `aria-hidden` on the rest of the page. An accessibility
+audit flagged this under WCAG 4.1.2 (Name, Role, Value) on the Help and Resources menus in
+Assignable.
+
+`NavBarMenuButton`, `ProfileMenu` and `DropdownMenu` now pass `isNonModal`, so the popover
+has no role and the menu is the only thing announced. `NavBarMenuButton` sets it ahead of
+`popoverProps`, so a caller can still override it. `NavBarPopoverButton` is unchanged and
+still renders a dialog.
+
+`isNonModal` on its own also stops a click elsewhere on the page from closing the menu.
+The underlay used to catch that click, and react-aria's blur fallback ignores focus
+moving to the page body. A new exported `MenuPopover` wraps `Popover` with `isNonModal`
+and closes the menu again on an outside press. `NavBarPopover` switches to it whenever
+`isNonModal` is set, and `DropdownMenu` uses it directly.
+
+What changes for users while a menu is open: the page behind it stays scrollable and
+visible to assistive tech, and focus is no longer trapped. Moving focus out of the menu
+closes it, as does Escape or a click outside. Focus still returns to the trigger on close.
+The menu also closes when the page scrolls.
+
 #### Dropped the `os` restriction that blocked Windows installs (CORE-2876)
 
 `package.json` declared `"os": [ "darwin", "linux" ]`, which npm enforces at install time in

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@openstax/ui-components",
-  "version": "1.24.3",
+  "version": "1.25.0",
   "license": "MIT",
   "repository": "https://github.com/openstax/ui-components.git",
   "publishConfig": {

src/components/DropdownMenu.spec.tsx

@@ -1,4 +1,5 @@
-import { render } from '@testing-library/react';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import { DropdownMenu, DropdownMenuItem } from './DropdownMenu';
 
 describe('DropdownMenu', () => {
@@ -19,4 +20,51 @@ describe('DropdownMenu', () => {
     expect(render(<TestMenu variant='primary'/>).asFragment()).toMatchSnapshot();
     expect(render(<TestMenu variant='light' width={'20rem'}/>).asFragment()).toMatchSnapshot();
   });
+
+  describe('when open', () => {
+    const renderOpenMenu = async () => {
+      const user = userEvent.setup();
+      render(
+        <>
+          <TestMenu variant='primary'/>
+          <p>Page content</p>
+        </>
+      );
+
+      const button = screen.getByRole('button', { name: 'Test Menu' });
+      await user.click(button);
+      const menu = await screen.findByRole('menu');
+
+      return { user, button, menu };
+    };
+
+    it('does not wrap the menu in a dialog', async () => {
+      const { button, menu } = await renderOpenMenu();
+
+      expect(document.querySelector('[role="dialog"]')).toBeNull();
+      expect(document.querySelector('[data-testid="underlay"]')).toBeNull();
+      expect(menu.getAttribute('aria-labelledby')).toBe(button.id);
+    });
+
+    it('closes on Escape and returns focus to the trigger', async () => {
+      const { user, button } = await renderOpenMenu();
+
+      await user.keyboard('{Escape}');
+
+      await waitFor(() => expect(screen.queryByRole('menu')).toBeNull());
+      // FocusScope restores focus to the trigger on an animation frame.
+      await waitFor(() => expect(document.activeElement).toBe(button));
+    });
+
+    it('closes on an outside press', async () => {
+      await renderOpenMenu();
+
+      // Press without moving focus, so only the outside-press handler can close the menu.
+      const outside = screen.getByText('Page content');
+      fireEvent.mouseDown(outside);
+      fireEvent.mouseUp(outside);
+
+      await waitFor(() => expect(screen.queryByRole('menu')).toBeNull());
+    });
+  });
 });

src/components/DropdownMenu.tsx

@@ -1,7 +1,8 @@
 import React from 'react';
-import { Button, Menu, MenuItem, MenuProps, MenuTrigger, MenuTriggerProps, Popover } from 'react-aria-components';
+import { Button, Menu, MenuItem, MenuProps, MenuTrigger, MenuTriggerProps } from 'react-aria-components';
 import { ButtonVariant, getButtonVariantStyles } from '../theme/buttons';
 import { palette } from '../theme/palette';
+import { MenuPopover } from './MenuPopover';
 import './DropdownMenu.css';
 
 interface DropdownMenuButtonProps<T> extends MenuProps<T>, Omit<MenuTriggerProps, 'children'> {
@@ -30,11 +31,11 @@ export const DropdownMenu = <T extends object>(
   return (
     <MenuTrigger {...props}>
       <Button className="dropdown-menu-button" style={buttonStyle} isDisabled={disabled}>{text}</Button>
-      <Popover>
+      <MenuPopover>
         <Menu {...props} className="dropdown-menu">
           {children}
         </Menu>
-      </Popover>
+      </MenuPopover>
     </MenuTrigger>
   );
 };

src/components/HelpMenu/index.spec.tsx

@@ -101,7 +101,7 @@ describe('HelpMenu', () => {
     fireEvent.click(await screen.findByText('Help'));
     await screen.findByRole('menu');
 
-    expect(screen.getByRole('dialog').getAttribute('data-placement')).toBe('bottom');
+    expect(document.querySelector('.navbar-popover')?.getAttribute('data-placement')).toBe('bottom');
   });
 
   it('errors if the service is unavailable', async () => {

src/components/MenuPopover.tsx

@@ -0,0 +1,24 @@
+import React from "react";
+import { useInteractOutside, useObjectRef } from "react-aria";
+import { OverlayTriggerStateContext, Popover, PopoverProps } from "react-aria-components";
+
+/**
+ * A non-modal Popover for menus, so it is not rendered as a dialog. Non-modal popovers do
+ * not close on an outside press by default; this adds that back.
+ */
+export const MenuPopover = React.forwardRef<HTMLElement, PopoverProps>(
+  (props, forwardedRef) => {
+    const ref = useObjectRef(forwardedRef);
+    const state = React.useContext(OverlayTriggerStateContext);
+
+    // A mouse press on the trigger only opens the menu, so a second press closes it here.
+    useInteractOutside({
+      ref,
+      isDisabled: !state?.isOpen,
+      onInteractOutside: () => state?.close(),
+    });
+
+    return <Popover isNonModal {...props} ref={ref} />;
+  },
+);
+MenuPopover.displayName = "MenuPopover";

src/components/NavBarMenuButtons.spec.tsx

@@ -1,4 +1,5 @@
-import { render } from "@testing-library/react";
+import { fireEvent, render, screen, waitFor } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
 import { Dialog, DialogTrigger, Menu, MenuItem } from "react-aria-components";
 import renderer from "react-test-renderer";
 import {
@@ -50,6 +51,115 @@ describe("NavBarMenuButton", () => {
   });
 });
 
+describe("NavBarMenuButton when open", () => {
+  const renderOpenMenu = async () => {
+    const user = userEvent.setup();
+    render(
+      <>
+        <NavBarMenuButton label="Test menu">
+          <NavBarMenuItem>Menu item</NavBarMenuItem>
+          <NavBarMenuItem>Another menu item</NavBarMenuItem>
+        </NavBarMenuButton>
+        <p>Page content</p>
+      </>,
+    );
+
+    const button = screen.getByRole("button", { name: "Test menu" });
+    await user.click(button);
+    const menu = await screen.findByRole("menu");
+
+    return { user, button, menu };
+  };
+
+  it("does not wrap the menu in a dialog", async () => {
+    const { menu } = await renderOpenMenu();
+
+    expect(document.querySelector('[role="dialog"]')).toBeNull();
+    expect(menu.closest(".navbar-popover")?.getAttribute("role")).toBeNull();
+  });
+
+  it("names the menu from its trigger", async () => {
+    const { button, menu } = await renderOpenMenu();
+
+    expect(screen.getByRole("menu", { name: "Test menu" })).toBe(menu);
+    expect(menu.getAttribute("aria-labelledby")).toBe(button.id);
+  });
+
+  it("leaves the rest of the page exposed to assistive tech", async () => {
+    await renderOpenMenu();
+
+    expect(document.querySelector('[data-testid="underlay"]')).toBeNull();
+    expect(screen.getByText("Page content").closest('[aria-hidden="true"]')).toBeNull();
+  });
+
+  it("closes on Escape and returns focus to the trigger", async () => {
+    const { user, button } = await renderOpenMenu();
+
+    await user.keyboard("{Escape}");
+
+    await waitFor(() => expect(screen.queryByRole("menu")).toBeNull());
+    expect(button.getAttribute("aria-expanded")).toBe("false");
+    // FocusScope restores focus to the trigger on an animation frame.
+    await waitFor(() => expect(document.activeElement).toBe(button));
+  });
+
+  it("closes on an outside press", async () => {
+    await renderOpenMenu();
+
+    // Press without moving focus, so only the outside-press handler can close the menu.
+    const outside = screen.getByText("Page content");
+    fireEvent.mouseDown(outside);
+    fireEvent.mouseUp(outside);
+
+    await waitFor(() => expect(screen.queryByRole("menu")).toBeNull());
+  });
+
+  it("closes, and stays closed, when the trigger is pressed again", async () => {
+    const { user, button } = await renderOpenMenu();
+
+    await user.click(button);
+
+    await waitFor(() => expect(screen.queryByRole("menu")).toBeNull());
+    expect(button.getAttribute("aria-expanded")).toBe("false");
+  });
+
+  it("toggles closed, and stays closed, when the trigger is tapped again", async () => {
+    render(
+      <NavBarMenuButton label="Test menu">
+        <NavBarMenuItem>Menu item</NavBarMenuItem>
+      </NavBarMenuButton>,
+    );
+    const button = screen.getByRole("button", { name: "Test menu" });
+    const tap = () => {
+      const touch = { identifier: 1, target: button, clientX: 0, clientY: 0 };
+      fireEvent.touchStart(button, { targetTouches: [touch], changedTouches: [touch] });
+      fireEvent.touchEnd(button, { targetTouches: [], changedTouches: [touch] });
+    };
+
+    tap();
+    await screen.findByRole("menu");
+    tap();
+
+    await waitFor(() => expect(screen.queryByRole("menu")).toBeNull());
+    expect(button.getAttribute("aria-expanded")).toBe("false");
+  });
+});
+
+describe("NavBarPopoverButton when open", () => {
+  it("still renders its content as a dialog", async () => {
+    const user = userEvent.setup();
+    render(
+      <NavBarPopoverButton label="Test popover">
+        Popover content
+      </NavBarPopoverButton>,
+    );
+
+    await user.click(screen.getByRole("button", { name: "Test popover" }));
+
+    expect(await screen.findByRole("dialog")).toBeTruthy();
+  });
+});
+
 describe("NavBarMenuItem", () => {
   it("composes a render-callback className", () => {
     render(

src/components/NavBarMenuButtons.tsx

@@ -13,6 +13,7 @@ import {
 import { colors } from "../theme";
 import { NavBarButton, NavBarButtonProps } from "./NavBarButton";
 import { CSSPropertiesWithVariables } from "../types";
+import { MenuPopover } from "./MenuPopover";
 import "./NavBarMenuButtons.css";
 
 export const NavBarMenuItem = React.forwardRef<
@@ -62,8 +63,11 @@ export const NavBarPopover = React.forwardRef<
     })
   );
 
+  // MenuPopover closes a non-modal popover on an outside press.
+  const PopoverComponent = props.isNonModal ? MenuPopover : Popover;
+
   return (
-    <Popover
+    <PopoverComponent
       ref={ref}
       className={composeRenderProps(className, (resolved) => classNames("navbar-popover", resolved))}
       style={popoverStyle}
@@ -91,7 +95,8 @@ const NavBarBaseButton = ({
   return (
     <Trigger>
       <NavBarButton {...props} />
-      <NavBarPopover {...popoverProps}>
+      {/* Non-modal so the popover is not announced as a dialog; popoverProps can override. */}
+      <NavBarPopover isNonModal={isMenu} {...popoverProps}>
         <Content>{children}</Content>
       </NavBarPopover>
     </Trigger>