feat(design-system): split DS::Menu into strict action-list + new DS::Popover (#1850)

* feat(design-system): split DS::Menu into strict action-list + new DS::Popover for mixed content

Closes #1743.

DS::Menu used to absorb both action-list dropdowns (row context menus,
"more actions") AND mixed-content panels (user-account dropdown,
filter forms, picker pop-ups). The two shapes carry incompatible a11y
contracts:

- **Action list**: `role="menu"` container, `role="menuitem"` children,
  Up/Down arrow nav per WAI-ARIA APG.
- **Mixed content**: NO menu role — `role="menu"` restricts AT users
  to menuitem-only navigation and breaks any panel with forms,
  headings, or generic groupings.

This PR splits the component:

## DS::Menu (tightened)

Strict action-list primitive. Variants reduced to `:icon` and
`:button` (no `:avatar`). `custom_content` slot removed. Bakes in:

- `role="menu"` on the panel, `aria-haspopup="menu"` +
  `aria-expanded` + `aria-controls` on the trigger.
- `role="menuitem"` + `tabindex="-1"` on every DS::MenuItem; the
  controller installs roving tabindex (first item gets `tabindex="0"`
  when the menu opens) and handles ArrowUp/Down/Home/End +
  Escape + Enter/Space activation.
- `role="separator"` on the divider variant.
- Stable per-instance `menu-<8-char hex>` id so the trigger's
  `aria-controls` resolves correctly.

`DS::Menu.new(variant: :avatar, ...)` now raises ArgumentError
pointing at DS::Popover.

## DS::Popover (new)

Positioned panel for **mixed**, **non-action-list** content: account
menus, picker forms, filter forms, embedded controls. Slots: `button`,
`header`, `custom_content`. Variants: `:icon`, `:button`, `:avatar`.
NO `role="menu"` — the panel announces as a generic dialog-popup
(`aria-haspopup="dialog"`, `aria-expanded`, `aria-controls`).
Mirrors DS::Menu's floating-ui positioning + Escape/outside-click
lifecycle in its own Stimulus controller (`DS--popover`). Avatar
variant ships a focus ring + bumped touch target (44×44 via `w-11
h-11` per #1738).

## Migrated callsites (7 → DS::Popover)

- `app/views/users/_user_menu.html.erb` — avatar trigger + profile
  header + nav links (items kept as DS::MenuItem inside
  `custom_content` for visual parity)
- `app/views/categories/_menu.html.erb` — turbo-framed category picker
- `app/views/budgets/_budget_header.html.erb` — budget picker
- `app/views/reports/index.html.erb` — period picker
- `app/views/holdings/_cost_basis_cell.html.erb` — cost-basis edit form
- `app/views/transactions/searches/_form.html.erb` — filter form
- `app/components/UI/account/activity_feed.html.erb:70` — status
  checkboxes (the row-level "new" menu on line 9 stays as DS::Menu)

The other 33 DS::Menu callsites stay as-is — pure action lists.

Locale: `ds.popover.avatar_default_label` + `users.user_menu.aria_label`
keys added (en only; other locales handled in a separate i18n pass).

* fix(test): update sidebar user-menu selector for Menu→Popover migration

The user-menu now renders as `DS::Popover` (variant: :avatar) instead
of `DS::Menu` after the menu split, so its trigger carries
`data-DS--popover-target="button"` rather than the old
`data-DS--menu-target`. Update the sidebar-driven settings test helper
to match — every system test that drives Settings via the sidebar
gates on this selector.

* fix(review): DS::Popover/Menu trigger a11y + caller-attr preservation

- popover.rb / menu.rb: button slot now merges (not overwrites) caller-
  provided data and aria hashes, sets aria-haspopup/expanded/controls on
  the :button variant, defaults type="button" on block-rendered buttons.
- menu.rb / menu.html.erb: drop renders_one :header (strict-menu API
  shouldn't expose an arbitrary-markup escape hatch); preview updated.
- menu_controller.js: handle Enter/Space activation on focused menuitem
  so keyboard navigation matches the ARIA menu pattern.
- cost_basis_cell / transactions/searches/_menu: retarget cancel button
  data-action from DS--menu#close to DS--popover#close (host controller
  changed in the migration).

* fix: apply CodeRabbit auto-fixes

Fixed 1 file(s) based on 1 unresolved review comment.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>

* fix(review): MenuItem roving: false for DS::Popover usage

Codex P1 on #1850: \`DS::MenuItem\` hard-codes \`tabindex=\"-1\"\` and
\`role=\"menuitem\"\` for both link and button variants — correct
inside \`DS::Menu\` (which provides arrow-key roving and announces
\`role=\"menu\"\`), but breaks every \`DS::MenuItem\` rendered inside
\`DS::Popover\` (\`app/views/users/_user_menu.html.erb\`). Popover has
no roving handler, so Tab skips every item — Settings, Changelog,
Feedback, Contact, Log out become keyboard-unreachable.

Add a \`roving:\` keyword (default \`true\`) to \`DS::MenuItem\` that
gates both \`tabindex=\"-1\"\` and \`role=\"menuitem\"\`. \`DS::Menu\`
callers keep the default (roving menu semantics intact). Pass
\`roving: false\` from \`_user_menu.html.erb\` so user-menu items land
in the normal Tab order. Existing \`menu.with_item(...)\` callers in
the design system still default to \`true\`, so no behavior change for
\`DS::Menu\` consumers.

* fix(review): make menuitem_attrs authoritative on roving

CodeRabbit Major on #1850: \`merged_opts\` was splatted AFTER
\`menuitem_attrs\` in \`DS::MenuItem#wrapper\`, so a stray
\`role: :button\` or \`tabindex: 0\` from a \`menu.with_item(..., role: …)\`
caller could silently downgrade the \`DS::Menu\` ARIA contract that
\`menuitem_attrs\` enforces.

Strip \`:role\` and \`:tabindex\` from \`merged_opts\` whenever
\`roving\` is enabled, then splat \`menuitem_attrs\` last. When
\`roving: false\` (popover usage in \`_user_menu.html.erb\`) callers
keep full control — Tab order and explicit ARIA stay tunable by the
caller.

---------

Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
This commit is contained in:
Guillem Arias Fauste
2026-05-20 18:30:25 +02:00
committed by GitHub
parent 355648c4a6
commit 12785754c8
20 changed files with 391 additions and 106 deletions

View File

@@ -8,7 +8,10 @@ import {
import { Controller } from "@hotwired/stimulus";
/**
* A "menu" can contain arbitrary content including non-clickable items, links, buttons, and forms.
* Strict action-list menu. Container is `role="menu"`, items are
* `role="menuitem"`. Arrow Up/Down moves focus between items, Home/End
* jumps to first/last, Escape closes the menu and returns focus to the
* trigger. Use DS::Popover for mixed-content panels (forms, pickers).
*/
export default class extends Controller {
static targets = ["button", "content"];
@@ -59,31 +62,71 @@ export default class extends Controller {
if (event.key === "Escape") {
this.close();
this.buttonTarget.focus();
return;
}
if (!this.show) return;
const items = this.#menuItems();
if (items.length === 0) return;
const currentIndex = items.indexOf(event.target);
// Activate the focused item on Enter / Space (ARIA menu pattern).
// Without this, link-based menuitems can't be activated by keyboard
// once focus has moved off the native default.
if (event.key === "Enter" || event.key === " ") {
if (currentIndex < 0) return;
event.preventDefault();
items[currentIndex].click();
return;
}
let nextIndex = null;
switch (event.key) {
case "ArrowDown":
nextIndex = currentIndex < 0 ? 0 : (currentIndex + 1) % items.length;
break;
case "ArrowUp":
nextIndex = currentIndex < 0 ? items.length - 1 : (currentIndex - 1 + items.length) % items.length;
break;
case "Home":
nextIndex = 0;
break;
case "End":
nextIndex = items.length - 1;
break;
default:
return;
}
event.preventDefault();
items.forEach((item, i) => item.setAttribute("tabindex", i === nextIndex ? "0" : "-1"));
items[nextIndex].focus();
};
toggle = () => {
this.show = !this.show;
this.contentTarget.classList.toggle("hidden", !this.show);
this.buttonTarget.setAttribute("aria-expanded", this.show.toString());
if (this.show) {
this.update();
this.focusFirstElement();
this.#focusFirstMenuItem();
}
};
close() {
this.show = false;
this.contentTarget.classList.add("hidden");
this.buttonTarget.setAttribute("aria-expanded", "false");
}
focusFirstElement() {
const focusableElements =
'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])';
const firstFocusableElement =
this.contentTarget.querySelectorAll(focusableElements)[0];
if (firstFocusableElement) {
firstFocusableElement.focus({ preventScroll: true });
}
#menuItems() {
return Array.from(this.contentTarget.querySelectorAll('[role="menuitem"]'));
}
#focusFirstMenuItem() {
const items = this.#menuItems();
if (items.length === 0) return;
items.forEach((item, i) => item.setAttribute("tabindex", i === 0 ? "0" : "-1"));
items[0].focus({ preventScroll: true });
}
startAutoUpdate() {