From 12785754c8629a159e006daee582fdfe94e944fe Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Wed, 20 May 2026 18:30:25 +0200 Subject: [PATCH] feat(design-system): split DS::Menu into strict action-list + new DS::Popover (#1850) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit Co-authored-by: Juan José Mata --- app/components/DS/menu.html.erb | 14 +- app/components/DS/menu.rb | 35 +++-- app/components/DS/menu_controller.js | 63 ++++++-- app/components/DS/menu_item.html.erb | 6 +- app/components/DS/menu_item.rb | 24 ++- app/components/DS/popover.html.erb | 32 ++++ app/components/DS/popover.rb | 62 ++++++++ app/components/DS/popover_controller.js | 140 ++++++++++++++++++ .../UI/account/activity_feed.html.erb | 6 +- app/views/budgets/_budget_header.html.erb | 6 +- app/views/categories/_menu.html.erb | 6 +- app/views/holdings/_cost_basis_cell.html.erb | 8 +- app/views/reports/index.html.erb | 6 +- .../transactions/searches/_form.html.erb | 6 +- .../transactions/searches/_menu.html.erb | 2 +- app/views/users/_user_menu.html.erb | 55 ++++--- config/locales/views/components/en.yml | 2 + config/locales/views/users/en.yml | 1 + .../previews/menu_component_preview.rb | 21 +-- test/system/settings_test.rb | 2 +- 20 files changed, 391 insertions(+), 106 deletions(-) create mode 100644 app/components/DS/popover.html.erb create mode 100644 app/components/DS/popover.rb create mode 100644 app/components/DS/popover_controller.js diff --git a/app/components/DS/menu.html.erb b/app/components/DS/menu.html.erb index ed6490184..27c262a59 100644 --- a/app/components/DS/menu.html.erb +++ b/app/components/DS/menu.html.erb @@ -1,26 +1,16 @@ <%= tag.div data: { controller: "DS--menu", DS__menu_placement_value: placement, DS__menu_offset_value: offset, DS__menu_mobile_fullwidth_value: mobile_fullwidth, testid: testid } do %> <% if variant == :icon %> - <%= render DS::Button.new(variant: "icon", icon: icon_vertical ? "more-vertical" : "more-horizontal", data: { DS__menu_target: "button" }) %> + <%= render DS::Button.new(variant: "icon", icon: icon_vertical ? "more-vertical" : "more-horizontal", aria: { haspopup: "menu", expanded: "false", controls: menu_id }, data: { DS__menu_target: "button" }) %> <% elsif variant == :button %> <%= button %> - <% elsif variant == :avatar %> - <% end %> - - <%= render DS::Menu.new(variant: "button", no_padding: true) do |menu| %> - <% menu.with_button( + <%= render DS::Popover.new(variant: "button", no_padding: true) do |popover| %> + <% popover.with_button( id: "transaction-filters-button", type: "button", text: t(".filter"), @@ -26,7 +26,7 @@ icon: "list-filter" ) %> - <% menu.with_custom_content do %> + <% popover.with_custom_content do %> <%= render "transactions/searches/menu", form: form %> <% end %> <% end %> diff --git a/app/views/transactions/searches/_menu.html.erb b/app/views/transactions/searches/_menu.html.erb index db7370e30..c424b20ed 100644 --- a/app/views/transactions/searches/_menu.html.erb +++ b/app/views/transactions/searches/_menu.html.erb @@ -37,7 +37,7 @@
- <%= render DS::Button.new(text: t(".cancel"), type: "button", variant: "ghost", data: { action: "DS--menu#close" }) %> + <%= render DS::Button.new(text: t(".cancel"), type: "button", variant: "ghost", data: { action: "DS--popover#close" }) %> <%= render DS::Button.new(text: t(".apply"), type: :submit) %>
diff --git a/app/views/users/_user_menu.html.erb b/app/views/users/_user_menu.html.erb index ddb2bc561..6268defb9 100644 --- a/app/views/users/_user_menu.html.erb +++ b/app/views/users/_user_menu.html.erb @@ -3,19 +3,20 @@ <% intro_mode = local_assigns.fetch(:intro_mode, false) %>
- <%= render DS::Menu.new( - variant: "avatar", + <%# `DS::Popover` (not `DS::Menu`) because the panel includes a + decorative profile header alongside the action items — `role="menu"` + would restrict AT users to menuitem-only navigation and hide the + user-info block. %> + <%= render DS::Popover.new( + variant: intro_mode ? "icon" : "avatar", avatar_url: user.profile_image&.variant(:small)&.url, initials: user.initials, placement: placement, - offset: offset - ) do |menu| %> - <% if intro_mode %> - <% menu.with_button do %> - <%= render DS::Button.new(variant: "icon", icon: "settings", data: { DS__menu_target: "button" }) %> - <% end %> - <% end %> - <%= menu.with_header do %> + offset: offset, + icon: "settings", + aria_label: t(".aria_label", default: "Open account menu") + ) do |popover| %> + <%= popover.with_header do %>
<%= render "settings/user_avatar", avatar_url: user.profile_image&.variant(:small)&.url, initials: user.initials, lazy: true %> @@ -43,21 +44,27 @@ <% end %> <% end %> - <% menu.with_item( - variant: "link", - text: t(".settings"), - icon: "settings", - href: intro_mode ? settings_profile_path : accounts_path(return_to: request.fullpath) - ) %> - <% menu.with_item(variant: "link", text: t(".changelog"), icon: "box", href: changelog_path) %> + <% popover.with_custom_content do %> + <%# `roving: false` keeps these items in the normal Tab order. The parent + `DS::Popover` has no arrow-key roving handler and is not a `role="menu"` + container, so the default `tabindex="-1"` would skip every item. %> + <%= render DS::MenuItem.new( + variant: "link", + roving: false, + text: t(".settings"), + icon: "settings", + href: intro_mode ? settings_profile_path : accounts_path(return_to: request.fullpath) + ) %> + <%= render DS::MenuItem.new(variant: "link", roving: false, text: t(".changelog"), icon: "box", href: changelog_path) %> - <% if self_hosted? && !intro_mode %> - <% menu.with_item(variant: "link", text: t(".feedback"), icon: "megaphone", href: feedback_path) %> + <% if self_hosted? && !intro_mode %> + <%= render DS::MenuItem.new(variant: "link", roving: false, text: t(".feedback"), icon: "megaphone", href: feedback_path) %> + <% end %> + <%= render DS::MenuItem.new(variant: "link", roving: false, text: t(".contact"), icon: "message-square-more", href: "https://discord.gg/36ZGBsxYEK", target: "_blank", rel: "noopener noreferrer") %> + + <%= render DS::MenuItem.new(variant: "divider") %> + + <%= render DS::MenuItem.new(variant: "button", roving: false, text: t(".log_out"), icon: "log-out", href: session_path(Current.session), method: :delete) %> <% end %> - <% menu.with_item(variant: "link", text: t(".contact"), icon: "message-square-more", href: "https://discord.gg/36ZGBsxYEK", target: "_blank", rel: "noopener noreferrer") %> - - <% menu.with_item(variant: "divider") %> - - <% menu.with_item(variant: "button", text: t(".log_out"), icon: "log-out", href: session_path(Current.session), method: :delete) %> <% end %>
diff --git a/config/locales/views/components/en.yml b/config/locales/views/components/en.yml index 11d896222..e0dfca1ab 100644 --- a/config/locales/views/components/en.yml +++ b/config/locales/views/components/en.yml @@ -89,6 +89,8 @@ en: default_label: Preview dialog: close: Close + popover: + avatar_default_label: Open menu tooltip: trigger_label: More info link: diff --git a/config/locales/views/users/en.yml b/config/locales/views/users/en.yml index 56bb4992d..9537d57ba 100644 --- a/config/locales/views/users/en.yml +++ b/config/locales/views/users/en.yml @@ -21,6 +21,7 @@ en: member: Member super_admin: Super admin user_menu: + aria_label: Open account menu version: Version settings: Settings changelog: Changelog diff --git a/test/components/previews/menu_component_preview.rb b/test/components/previews/menu_component_preview.rb index 78049537b..cf5e1b8e9 100644 --- a/test/components/previews/menu_component_preview.rb +++ b/test/components/previews/menu_component_preview.rb @@ -12,33 +12,14 @@ class MenuComponentPreview < ViewComponent::Preview end end - def avatar - render DS::Menu.new(variant: "avatar") do |menu| - menu_contents(menu) - end - end - private def menu_contents(menu) - menu.with_header do - content_tag(:div, class: "p-3") do - content_tag(:h3, "Menu header", class: "font-medium text-primary") - end - end - menu.with_item(variant: "link", text: "Link", href: "#", icon: "plus") menu.with_item(variant: "button", text: "Action", href: "#", method: :post, icon: "circle") menu.with_item(variant: "button", text: "Action destructive", href: "#", method: :delete, icon: "circle") menu.with_item(variant: "divider") - menu.with_custom_content do - content_tag(:div, class: "p-4") do - safe_join([ - content_tag(:h3, "Custom content header", class: "font-medium text-primary"), - content_tag(:p, "Some custom content", class: "text-sm text-secondary") - ]) - end - end + menu.with_item(variant: "link", text: "Another link", href: "#", icon: "external-link") end end diff --git a/test/system/settings_test.rb b/test/system/settings_test.rb index 1107c2aed..96ec24dcc 100644 --- a/test/system/settings_test.rb +++ b/test/system/settings_test.rb @@ -104,7 +104,7 @@ class SettingsTest < ApplicationSystemTestCase def open_settings_from_sidebar user_menu = find("div[data-testid=user-menu]", match: :first, visible: :visible) within user_menu do - find("[data-DS--menu-target='button']", match: :first).click + find("[data-DS--popover-target='button']", match: :first).click click_link "Settings", match: :first end end