From 6ab284289aac2bf3d6aa1d3bd4e5929a22118df4 Mon Sep 17 00:00:00 2001 From: Roy Johnson Date: Tue, 29 Sep 2026 16:18:49 -0500 Subject: [PATCH 1/4] CORE-2875: Stop rendering menu popovers as dialogs react-aria-components' Popover adds role="dialog" unless isNonModal is set, so every MenuTrigger -> Popover -> Menu wrapped the menu in a dialog labelled by the trigger. Screen readers heard the name twice, and the menu became modal: an underlay, a focus trap, a scroll-locked body and aria-hidden on the rest of the page. Flagged as a WCAG 4.1.2 failure on Assignable's Help and Resources menus. NavBarMenuButton, ProfileMenu and DropdownMenu now pass isNonModal. NavBarPopoverButton still renders a dialog. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 20 ++++++ src/components/DropdownMenu.spec.tsx | 47 +++++++++++++- src/components/DropdownMenu.tsx | 3 +- src/components/HelpMenu/index.spec.tsx | 2 +- src/components/NavBarMenuButtons.spec.tsx | 79 ++++++++++++++++++++++- src/components/NavBarMenuButtons.tsx | 8 ++- src/components/ProfileMenu/index.spec.tsx | 68 ++++++++++++++++++- src/components/ProfileMenu/index.tsx | 3 +- 8 files changed, 223 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10f97f526..0f2af174a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,26 @@ 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. + +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/src/components/DropdownMenu.spec.tsx b/src/components/DropdownMenu.spec.tsx index 7532902c6..bd2c97953 100644 --- a/src/components/DropdownMenu.spec.tsx +++ b/src/components/DropdownMenu.spec.tsx @@ -1,4 +1,5 @@ -import { render } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { DropdownMenu, DropdownMenuItem } from './DropdownMenu'; describe('DropdownMenu', () => { @@ -19,4 +20,48 @@ 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 () => { + const { user } = await renderOpenMenu(); + + await user.click(screen.getByText('Page content')); + + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + }); + }); }); diff --git a/src/components/DropdownMenu.tsx b/src/components/DropdownMenu.tsx index 1b262aecc..3ee9d61ac 100644 --- a/src/components/DropdownMenu.tsx +++ b/src/components/DropdownMenu.tsx @@ -30,7 +30,8 @@ export const DropdownMenu = ( return ( - + {/* isNonModal: a menu popover is not a dialog (see NavBarBaseButton). */} + {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/NavBarMenuButtons.spec.tsx b/src/components/NavBarMenuButtons.spec.tsx index 72c8ce745..3a6663f37 100644 --- a/src/components/NavBarMenuButtons.spec.tsx +++ b/src/components/NavBarMenuButtons.spec.tsx @@ -1,4 +1,5 @@ -import { render } from "@testing-library/react"; +import { 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,82 @@ 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 () => { + const { user } = await renderOpenMenu(); + + await user.click(screen.getByText("Page content")); + + await waitFor(() => expect(screen.queryByRole("menu")).toBeNull()); + }); +}); + +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..3a5d7e77e 100644 --- a/src/components/NavBarMenuButtons.tsx +++ b/src/components/NavBarMenuButtons.tsx @@ -91,7 +91,13 @@ const NavBarBaseButton = ({ return ( - + {/* A menu popover is not a dialog. RAC's Popover adds role="dialog" (plus a + full-screen underlay, a focus trap, body scroll-lock and aria-hidden on the + rest of the page) unless isNonModal is set. The trigger already exposes + aria-haspopup/aria-expanded and the Menu carries the accessible name, so the + extra dialog layer is announced twice and implies modality that does not hold. + isNonModal is set before the spread so callers can still override it. */} + {children} diff --git a/src/components/ProfileMenu/index.spec.tsx b/src/components/ProfileMenu/index.spec.tsx index 5f5671910..bc31cb719 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,71 @@ 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'); + + await user.click(screen.getByText('Page content')); + + 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..19d9dd3be 100644 --- a/src/components/ProfileMenu/index.tsx +++ b/src/components/ProfileMenu/index.tsx @@ -82,7 +82,8 @@ export const ProfileMenu: React.FC = ({ > {displayInitials || } - + {/* isNonModal: a menu popover is not a dialog (see NavBarBaseButton). */} + {children} From 639fb903ef6ad832a5b23c5a6b42df8c8fe48c3e Mon Sep 17 00:00:00 2001 From: Roy Johnson Date: Tue, 29 Sep 2026 16:28:08 -0500 Subject: [PATCH 2/4] Bump version --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 65232a294..aca0ac17e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@openstax/ui-components", - "version": "1.24.2", + "version": "1.25.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@openstax/ui-components", - "version": "1.24.2", + "version": "1.25.0", "license": "MIT", "dependencies": { "@sentry/react": "^10.60.0", diff --git a/package.json b/package.json index 340c29c83..8c420186a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@openstax/ui-components", - "version": "1.24.2", + "version": "1.25.0", "license": "MIT", "repository": "https://github.com/openstax/ui-components.git", "publishConfig": { From 7230c398b554dbb251f401cd62a9740af6b64190 Mon Sep 17 00:00:00 2001 From: Roy Johnson Date: Tue, 29 Sep 2026 16:54:30 -0500 Subject: [PATCH 3/4] CORE-2875: Close non-modal menus on an outside press isNonModal also turns off react-aria's outside-press dismissal, and its blur fallback ignores focus moving to the body, so in a real browser a click on empty page space left Help and kebab menus open. A second mouse press on the trigger did not close them either: a mouse press only opens the menu, and the underlay used to catch it. MenuPopover wraps Popover with isNonModal and closes the menu on an outside press. NavBarPopover uses it when isNonModal is set, and DropdownMenu uses it directly. The outside-press tests now use raw mouse events with no focus change. userEvent.click moved focus with a non-null relatedTarget, which let the old tests pass through the blur path. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++++ src/components/DropdownMenu.spec.tsx | 10 ++++-- src/components/DropdownMenu.tsx | 9 ++--- src/components/MenuPopover.tsx | 29 ++++++++++++++++ src/components/NavBarMenuButtons.spec.tsx | 40 +++++++++++++++++++++-- src/components/NavBarMenuButtons.tsx | 7 +++- src/components/ProfileMenu/index.spec.tsx | 6 +++- src/index.ts | 1 + 8 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 src/components/MenuPopover.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f2af174a..2324bddb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,12 @@ has no role and the menu is the only thing announced. `NavBarMenuButton` sets it `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. diff --git a/src/components/DropdownMenu.spec.tsx b/src/components/DropdownMenu.spec.tsx index bd2c97953..ab0d9b63f 100644 --- a/src/components/DropdownMenu.spec.tsx +++ b/src/components/DropdownMenu.spec.tsx @@ -1,4 +1,4 @@ -import { render, screen, waitFor } 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'; @@ -57,9 +57,13 @@ describe('DropdownMenu', () => { }); it('closes on an outside press', async () => { - const { user } = await renderOpenMenu(); + await renderOpenMenu(); - await user.click(screen.getByText('Page content')); + // A press on non-focusable page content, with no focus change: what a real browser + // does, and what useInteractOutside (not blur) has to catch. + 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 3ee9d61ac..1ce2bed0b 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,12 +31,12 @@ export const DropdownMenu = ( return ( - {/* isNonModal: a menu popover is not a dialog (see NavBarBaseButton). */} - + {/* A menu popover is not a dialog; MenuPopover is non-modal. */} + {children} - + ); }; diff --git a/src/components/MenuPopover.tsx b/src/components/MenuPopover.tsx new file mode 100644 index 000000000..4def60ed5 --- /dev/null +++ b/src/components/MenuPopover.tsx @@ -0,0 +1,29 @@ +import React from "react"; +import { useInteractOutside, useObjectRef } from "react-aria"; +import { OverlayTriggerStateContext, Popover, PopoverProps } from "react-aria-components"; + +/** + * A non-modal Popover for menus. RAC's Popover renders as a dialog (role="dialog", a + * full-screen underlay, a focus trap, body scroll-lock and aria-hidden on the rest of the + * page) unless isNonModal is set, which is wrong for a menu. But isNonModal also turns + * off outside-press dismissal, and the blur fallback ignores focus moving to the body, so + * clicking an empty part of the page would leave the menu open. This puts it back. + */ +export const MenuPopover = React.forwardRef( + (props, forwardedRef) => { + const ref = useObjectRef(forwardedRef); + const state = React.useContext(OverlayTriggerStateContext); + + // This also covers a second press on the trigger, which for a mouse only ever calls + // open() on the menu and relied on the underlay to close it. A touch trigger toggles + // on pointerup, before this fires on click, so close() is then a no-op. + 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 3a6663f37..d94c8c426 100644 --- a/src/components/NavBarMenuButtons.spec.tsx +++ b/src/components/NavBarMenuButtons.spec.tsx @@ -1,4 +1,4 @@ -import { render, screen, waitFor } 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"; @@ -104,11 +104,45 @@ describe("NavBarMenuButton when open", () => { }); it("closes on an outside press", async () => { - const { user } = await renderOpenMenu(); + await renderOpenMenu(); + + // A press on non-focusable page content, with no focus change: what a real browser + // does, and what useInteractOutside (not blur) has to catch. + 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 user.click(screen.getByText("Page content")); + 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"); }); }); diff --git a/src/components/NavBarMenuButtons.tsx b/src/components/NavBarMenuButtons.tsx index 3a5d7e77e..cad4eebae 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,12 @@ export const NavBarPopover = React.forwardRef< }) ); + // A non-modal popover needs MenuPopover's outside-press handling to close on a click + // elsewhere on the page. + const PopoverComponent = props.isNonModal ? MenuPopover : Popover; + return ( - classNames("navbar-popover", resolved))} style={popoverStyle} diff --git a/src/components/ProfileMenu/index.spec.tsx b/src/components/ProfileMenu/index.spec.tsx index bc31cb719..b4f24470c 100644 --- a/src/components/ProfileMenu/index.spec.tsx +++ b/src/components/ProfileMenu/index.spec.tsx @@ -93,7 +93,11 @@ describe('ProfileMenu', () => { await user.click(screen.getByTestId('profile-menu')); await screen.findByRole('menu'); - await user.click(screen.getByText('Page content')); + // A press on non-focusable page content, with no focus change: what a real browser + // does, and what useInteractOutside (not blur) has to catch. + const outside = screen.getByText('Page content'); + fireEvent.mouseDown(outside); + fireEvent.mouseUp(outside); await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); }); 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'; From 5f2a24eeaf495a2b57a01ed8d36d27b8e6bfe8fc Mon Sep 17 00:00:00 2001 From: Roy Johnson Date: Wed, 30 Sep 2026 10:48:20 -0500 Subject: [PATCH 4/4] CORE-2875: Trim code comments Co-Authored-By: Claude Sonnet 5.5 --- src/components/DropdownMenu.spec.tsx | 3 +-- src/components/DropdownMenu.tsx | 1 - src/components/MenuPopover.tsx | 11 +++-------- src/components/NavBarMenuButtons.spec.tsx | 3 +-- src/components/NavBarMenuButtons.tsx | 10 ++-------- src/components/ProfileMenu/index.spec.tsx | 3 +-- src/components/ProfileMenu/index.tsx | 1 - 7 files changed, 8 insertions(+), 24 deletions(-) diff --git a/src/components/DropdownMenu.spec.tsx b/src/components/DropdownMenu.spec.tsx index ab0d9b63f..bce54a69b 100644 --- a/src/components/DropdownMenu.spec.tsx +++ b/src/components/DropdownMenu.spec.tsx @@ -59,8 +59,7 @@ describe('DropdownMenu', () => { it('closes on an outside press', async () => { await renderOpenMenu(); - // A press on non-focusable page content, with no focus change: what a real browser - // does, and what useInteractOutside (not blur) has to catch. + // 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); diff --git a/src/components/DropdownMenu.tsx b/src/components/DropdownMenu.tsx index 1ce2bed0b..ba4586df3 100644 --- a/src/components/DropdownMenu.tsx +++ b/src/components/DropdownMenu.tsx @@ -31,7 +31,6 @@ export const DropdownMenu = ( return ( - {/* A menu popover is not a dialog; MenuPopover is non-modal. */} {children} diff --git a/src/components/MenuPopover.tsx b/src/components/MenuPopover.tsx index 4def60ed5..2fa130fad 100644 --- a/src/components/MenuPopover.tsx +++ b/src/components/MenuPopover.tsx @@ -3,20 +3,15 @@ import { useInteractOutside, useObjectRef } from "react-aria"; import { OverlayTriggerStateContext, Popover, PopoverProps } from "react-aria-components"; /** - * A non-modal Popover for menus. RAC's Popover renders as a dialog (role="dialog", a - * full-screen underlay, a focus trap, body scroll-lock and aria-hidden on the rest of the - * page) unless isNonModal is set, which is wrong for a menu. But isNonModal also turns - * off outside-press dismissal, and the blur fallback ignores focus moving to the body, so - * clicking an empty part of the page would leave the menu open. This puts it back. + * 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); - // This also covers a second press on the trigger, which for a mouse only ever calls - // open() on the menu and relied on the underlay to close it. A touch trigger toggles - // on pointerup, before this fires on click, so close() is then a no-op. + // A mouse press on the trigger only opens the menu, so a second press closes it here. useInteractOutside({ ref, isDisabled: !state?.isOpen, diff --git a/src/components/NavBarMenuButtons.spec.tsx b/src/components/NavBarMenuButtons.spec.tsx index d94c8c426..c1a49fa2a 100644 --- a/src/components/NavBarMenuButtons.spec.tsx +++ b/src/components/NavBarMenuButtons.spec.tsx @@ -106,8 +106,7 @@ describe("NavBarMenuButton when open", () => { it("closes on an outside press", async () => { await renderOpenMenu(); - // A press on non-focusable page content, with no focus change: what a real browser - // does, and what useInteractOutside (not blur) has to catch. + // 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); diff --git a/src/components/NavBarMenuButtons.tsx b/src/components/NavBarMenuButtons.tsx index cad4eebae..e0edd3641 100644 --- a/src/components/NavBarMenuButtons.tsx +++ b/src/components/NavBarMenuButtons.tsx @@ -63,8 +63,7 @@ export const NavBarPopover = React.forwardRef< }) ); - // A non-modal popover needs MenuPopover's outside-press handling to close on a click - // elsewhere on the page. + // MenuPopover closes a non-modal popover on an outside press. const PopoverComponent = props.isNonModal ? MenuPopover : Popover; return ( @@ -96,12 +95,7 @@ const NavBarBaseButton = ({ return ( - {/* A menu popover is not a dialog. RAC's Popover adds role="dialog" (plus a - full-screen underlay, a focus trap, body scroll-lock and aria-hidden on the - rest of the page) unless isNonModal is set. The trigger already exposes - aria-haspopup/aria-expanded and the Menu carries the accessible name, so the - extra dialog layer is announced twice and implies modality that does not hold. - isNonModal is set before the spread so callers can still override it. */} + {/* 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 b4f24470c..a74c6b3d0 100644 --- a/src/components/ProfileMenu/index.spec.tsx +++ b/src/components/ProfileMenu/index.spec.tsx @@ -93,8 +93,7 @@ describe('ProfileMenu', () => { await user.click(screen.getByTestId('profile-menu')); await screen.findByRole('menu'); - // A press on non-focusable page content, with no focus change: what a real browser - // does, and what useInteractOutside (not blur) has to catch. + // 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); diff --git a/src/components/ProfileMenu/index.tsx b/src/components/ProfileMenu/index.tsx index 19d9dd3be..e336a4196 100644 --- a/src/components/ProfileMenu/index.tsx +++ b/src/components/ProfileMenu/index.tsx @@ -82,7 +82,6 @@ export const ProfileMenu: React.FC = ({ > {displayInitials || } - {/* isNonModal: a menu popover is not a dialog (see NavBarBaseButton). */} {children}