diff --git a/CHANGELOG.md b/CHANGELOG.md index 10f97f526..2324bddb7 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/package-lock.json b/package-lock.json index 0495985a8..63b54c362 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@openstax/ui-components", - "version": "1.24.3", + "version": "1.25.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@openstax/ui-components", - "version": "1.24.3", + "version": "1.25.0", "license": "MIT", "dependencies": { "@sentry/react": "^10.60.0", diff --git a/package.json b/package.json index c5434ca76..a37ec9a7f 100644 --- a/package.json +++ b/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": { diff --git a/src/components/DropdownMenu.spec.tsx b/src/components/DropdownMenu.spec.tsx index 7532902c6..bce54a69b 100644 --- a/src/components/DropdownMenu.spec.tsx +++ b/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().asFragment()).toMatchSnapshot(); expect(render().asFragment()).toMatchSnapshot(); }); + + describe('when open', () => { + const renderOpenMenu = async () => { + const user = userEvent.setup(); + render( + <> + +

Page content

+ + ); + + 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()); + }); + }); }); diff --git a/src/components/DropdownMenu.tsx b/src/components/DropdownMenu.tsx index 1b262aecc..ba4586df3 100644 --- a/src/components/DropdownMenu.tsx +++ b/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 extends MenuProps, Omit { @@ -30,11 +31,11 @@ export const DropdownMenu = ( return ( - + {children} - + ); }; diff --git a/src/components/HelpMenu/index.spec.tsx b/src/components/HelpMenu/index.spec.tsx index 1691ab68e..51de5cdd6 100644 --- a/src/components/HelpMenu/index.spec.tsx +++ b/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 () => { diff --git a/src/components/MenuPopover.tsx b/src/components/MenuPopover.tsx new file mode 100644 index 000000000..2fa130fad --- /dev/null +++ b/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( + (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 ; + }, +); +MenuPopover.displayName = "MenuPopover"; diff --git a/src/components/NavBarMenuButtons.spec.tsx b/src/components/NavBarMenuButtons.spec.tsx index 72c8ce745..c1a49fa2a 100644 --- a/src/components/NavBarMenuButtons.spec.tsx +++ b/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( + <> + + Menu item + Another menu item + +

Page content

+ , + ); + + 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( + + Menu item + , + ); + 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( + + Popover content + , + ); + + await user.click(screen.getByRole("button", { name: "Test popover" })); + + expect(await screen.findByRole("dialog")).toBeTruthy(); + }); +}); + describe("NavBarMenuItem", () => { it("composes a render-callback className", () => { render( diff --git a/src/components/NavBarMenuButtons.tsx b/src/components/NavBarMenuButtons.tsx index 44aab02f9..e0edd3641 100644 --- a/src/components/NavBarMenuButtons.tsx +++ b/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 ( - classNames("navbar-popover", resolved))} style={popoverStyle} @@ -91,7 +95,8 @@ const NavBarBaseButton = ({ return ( - + {/* Non-modal so the popover is not announced as a dialog; popoverProps can override. */} + {children} diff --git a/src/components/ProfileMenu/index.spec.tsx b/src/components/ProfileMenu/index.spec.tsx index 5f5671910..a74c6b3d0 100644 --- a/src/components/ProfileMenu/index.spec.tsx +++ b/src/components/ProfileMenu/index.spec.tsx @@ -1,4 +1,5 @@ import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { ProfileMenu, ProfileMenuItem, UserIcon } from '.'; describe('ProfileMenu', () => { @@ -32,6 +33,74 @@ describe('ProfileMenu', () => { expect(menu.getAttribute('aria-labelledby')).toBe(button.id); }); + it('does not wrap the menu in a dialog', async () => { + render( + <> + + Profile + +

Page content

+ + ); + + fireEvent.click(screen.getByTestId('profile-menu')); + await screen.findByRole('menu', { name: 'Account actions' }); + + expect(screen.queryByRole('dialog')).toBeNull(); + 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 = userEvent.setup(); + render( + + Profile + + ); + + const button = screen.getByTestId('profile-menu'); + await user.click(button); + await screen.findByRole('menu'); + + 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 () => { + const user = userEvent.setup(); + render( + <> + + Profile + +

Page content

+ + ); + + await user.click(screen.getByTestId('profile-menu')); + await screen.findByRole('menu'); + + // 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('moves focus into the menu when it opens', async () => { render( { fireEvent.click(screen.getByTestId('profile-menu')); await screen.findByRole('menu'); - const popover = screen.getByRole('dialog'); + const popover = document.querySelector('.navbar-popover') as HTMLElement; expect(popover.getAttribute('data-placement')).toBe('bottom'); }); diff --git a/src/components/ProfileMenu/index.tsx b/src/components/ProfileMenu/index.tsx index 928ac42c4..e336a4196 100644 --- a/src/components/ProfileMenu/index.tsx +++ b/src/components/ProfileMenu/index.tsx @@ -82,7 +82,7 @@ export const ProfileMenu: React.FC = ({ > {displayInitials || } - + {children} diff --git a/src/index.ts b/src/index.ts index fe2be4f4a..089816146 100644 --- a/src/index.ts +++ b/src/index.ts @@ -20,6 +20,7 @@ export * from './components/Modal'; export * from './components/NavBar'; export * from './components/NavBarButton'; export * from './components/NavBarLogo'; +export * from './components/MenuPopover'; export * from './components/NavBarMenuButtons'; export * from './components/Overlay'; export * from './components/Pagination';