Skip to content

CORE-2875: Stop rendering menu popovers as dialogs - #162

Draft
RoyEJohnson wants to merge 3 commits into
mainfrom
CORE-2875-menu-popover-not-dialog
Draft

RoyEJohnson wants to merge 3 commits into
mainfrom
CORE-2875-menu-popover-not-dialog

Conversation

@RoyEJohnson

@RoyEJohnson RoyEJohnson commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Jira: CORE-2875

This is the first of two PRs for this ticket. It removes the dialog wrapper, which fixes the
reported finding. Moving the Help and Resources menus to the disclosure pattern will come
in a separate PR.

The bug

An accessibility audit flagged Assignable's Help and Resources menus under WCAG 4.1.2
(Name, Role, Value) for "unnecessary use of a dialog."

Nobody wrote a dialog. react-aria-components' Popover adds one by itself:

// react-aria-components/src/Popover.tsx
let shouldBeDialog = !props.isNonModal || props.trigger === 'SubmenuTrigger';

Nothing in this repo passes isNonModal. So every MenuTrigger → Popover → Menu renders
role="dialog" around role="menu", and the trigger labels both of them. A screen reader
says "Help menu, dialog" and then "Help menu, menu". The same flag also makes the menu
modal: a full-screen underlay, a focus trap, a scroll-locked body, and aria-hidden on the
rest of the page.

The fix

isNonModal is now passed in all three places that build a menu popover:

  • NavBarMenuButton (through NavBarBaseButton). It's set ahead of the popoverProps
    spread, so a caller can still override it. This covers Help, Resources and the Activity
    Menu kebab in Assignable.
  • ProfileMenu
  • DropdownMenu, which covers the Sync/Export scores dropdown.

NavBarPopoverButton doesn't change. Its content really is a Dialog.

Outside press

isNonModal on its own leaves the menu open when you click an empty part of the page.
With it set, react-aria turns off its outside-press handler (isDismissable: false), and
its blur fallback ignores focus moving to the page body (relatedTarget is null). A
second mouse press on the trigger doesn't close the menu either: for a mouse, the menu
trigger only ever calls open(), and the underlay used to catch that second press.

The new exported MenuPopover wraps Popover with isNonModal and adds
useInteractOutside to close the menu again. NavBarPopover switches to it whenever
isNonModal is set, which covers NavBarMenuButton and ProfileMenu, and DropdownMenu
uses it directly. A tap on the trigger still toggles the menu: the tap toggles on
pointerup, before the outside handler runs on click, so that handler's close() does
nothing.

Behaviour changes while a menu is open

  • The page behind the menu stays scrollable and visible to assistive tech.
  • Focus is no longer trapped. Moving focus out of the menu closes it.
  • Escape and clicking outside still close the menu, and focus still returns to the trigger.
  • The menu now closes when the page scrolls.

Tests

The NavBarMenuButtons, ProfileMenu and DropdownMenu specs each have new open-menu tests.
They check that there's no [role=dialog] and no underlay, that the menu is named from the
trigger, and that Escape, a click outside and a second press on the trigger (mouse and
touch) all close it. The outside-press tests fire raw mouse-down/mouse-up with no focus
change, the way a real browser does. They fail without MenuPopover and pass with it. My
first version used userEvent.click, which passed through the blur path and hid the bug. Where it applies, they also
check that focus returns to the trigger and that the page behind stays exposed. Two
existing tests in the ProfileMenu and HelpMenu specs found the popover by
role="dialog"; they now use .navbar-popover. No snapshots changed, because they only
capture closed menus.

Before merging

  • Check in a real browser that clicking outside still closes each menu. With
    isNonModal, react-aria closes the menu on blur instead of through its
    outside-interaction hook. It works in jsdom, but that isn't a real browser.
  • Bump the version. package.json is still 1.24.2.

Other consumers pick this up on their next ui-components bump: rex-web (ProfileMenu,
SummaryPopup/ContextMenu), staxpass (admin/PageNav), flex-pages (BookMenu),
lti-gateway (HelpMenu) and assignments.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>

This comment was marked as resolved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Browser click-outside dismissal is not guaranteed for the affected menus, and the lockfile metadata needs synchronization.

Review effort: Lite
Findings: 1 Low severity

Open (1)
Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Ensure non-modal dropdowns close on non-focusable outside clicks

src/​components/​DropdownMenu.tsx:34

isNonModal changes RAC dismissal to blur-based behavior; a real browser click on non-focusable page content does not move focus, so no blur occurs and the dropdown can remain open. The new test clicks a <p> in jsdom and does not validate this browser behavior. Add explicit outside-interaction dismissal or a browser regression test with an implementation that guarantees the documented click-outside close.

Medium severity Add reliable browser-tested outside dismissal for navbar menus

src/​components/​NavBarMenuButtons.tsx:100

isNonModal changes RAC dismissal to blur-based behavior; a real browser click on non-focusable page content (such as the <p> used by the new test) does not move focus, so no blur occurs and the menu can remain open. The added jsdom outside-press test therefore does not validate the stated click-outside behavior. Add an explicit outside-interaction dismissal (or a real-browser regression test and equivalent implementation) before relying on this for all navbar menus.

Medium severity Ensure profile menu closes on non-focusable outside clicks

src/​components/​ProfileMenu/​index.tsx:86

isNonModal changes RAC dismissal to blur-based behavior; a real browser click on non-focusable page content does not move focus, so no blur occurs and the profile menu can remain open. The new test clicks a <p> in jsdom and does not validate this browser behavior. Add explicit outside-interaction dismissal or a browser regression test with an implementation that guarantees the documented click-outside close.

Comment thread package.json
RoyEJohnson and others added 2 commits September 29, 2026 17:00
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 <noreply@anthropic.com>

This branch was successfully deployed

1 active deployment
refs/heads/CORE-2875-menu-popover-not-dialog — 7230c398 Deployed Sep 29, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants