Files
sure/test/system/settings_test.rb
Guillem Arias Fauste 12785754c8 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>
2026-05-20 18:30:25 +02:00

112 lines
4.3 KiB
Ruby

require "application_system_test_case"
class SettingsTest < ApplicationSystemTestCase
setup do
sign_in @user = users(:family_admin)
# Base settings available to all users
@settings_links = [
[ "Accounts", accounts_path ]
]
@settings_links << [ "Bank sync", settings_providers_path ] if @user.admin?
@settings_links += [
[ "Preferences", settings_preferences_path ],
[ "Profile Info", settings_profile_path ],
[ "Security", settings_security_path ],
[ "Categories", categories_path ],
[ "Tags", tags_path ],
[ "Rules", rules_path ],
[ "Merchants", family_merchants_path ],
[ "Guides", settings_guides_path ],
[ "What's new", changelog_path ],
[ "Feedback", feedback_path ]
]
# Add admin settings if user is admin
if @user.admin?
merchants_index = @settings_links.index([ "Merchants", family_merchants_path ])
@settings_links.insert(merchants_index + 1, [ "Statement Vault", account_statements_path ])
@settings_links += [
[ "AI Prompts", settings_ai_prompts_path ],
[ "API Key", settings_api_key_path ]
]
end
end
test "can access settings from sidebar" do
VCR.use_cassette("git_repository_provider/fetch_latest_release_notes") do
open_settings_from_sidebar
assert_selector "h1", text: "Accounts"
assert_current_path accounts_path, ignore_query: true
@settings_links.each do |name, path|
click_link name, match: :first
assert_selector "h1", text: name
assert_current_path path
end
end
end
test "can update self hosting settings" do
sign_in users(:sure_support_staff)
Rails.application.config.app_mode.stubs(:self_hosted?).returns(true)
Provider::Registry.stubs(:get_provider).with(:twelve_data).returns(nil)
Provider::Registry.stubs(:get_provider).with(:yahoo_finance).returns(nil)
Provider::Registry.stubs(:get_provider).with(:github).returns(stub(fetch_latest_release_notes: nil))
open_settings_from_sidebar
assert_selector "li", text: "Self-Hosting"
click_link "Self-Hosting", match: :first
assert_current_path settings_hosting_path
assert_selector "h1", text: "Self-Hosting"
find("select#setting_onboarding_state").select("Invite-only")
within("select#setting_onboarding_state") do
assert_selector "option[selected]", text: "Invite-only"
end
click_button "Generate new code"
assert_selector 'span[data-clipboard-target="source"]', visible: true, count: 1 # invite code copy widget
copy_button = find('button[data-action="clipboard#copy"]', match: :first) # Find the first copy button (adjust if needed)
page.execute_script("Object.defineProperty(navigator, 'clipboard', { value: { writeText: () => Promise.resolve() }, writable: true, configurable: true })") # Mock clipboard API due to browser security restrictions in tests
copy_button.click
assert_selector 'span[data-clipboard-target="iconSuccess"]', visible: true, count: 1 # text copied and icon changed to checkmark
end
test "does not show payment link if self hosting" do
Rails.application.config.app_mode.stubs(:self_hosted?).returns(true)
open_settings_from_sidebar
assert_no_selector "li", text: I18n.t("settings.settings_nav.payment_label")
end
test "does not show admin settings to non-admin users" do
VCR.use_cassette("git_repository_provider/fetch_latest_release_notes") do
# Visit accounts path directly as non-admin user to avoid user menu issues
visit new_session_path
within %(form[action='#{sessions_path}']) do
fill_in "Email", with: users(:family_member).email
fill_in "Password", with: user_password_test
click_on "Log in"
end
# Go directly to accounts (settings) page
visit accounts_path
# Assert that admin-only settings are not present in the navigation
assert_no_selector "li", text: "AI Prompts"
assert_no_selector "li", text: "API Key"
assert_no_selector "li", text: "Bank sync"
assert_no_selector "li", text: "Statement Vault"
end
end
private
def open_settings_from_sidebar
user_menu = find("div[data-testid=user-menu]", match: :first, visible: :visible)
within user_menu do
find("[data-DS--popover-target='button']", match: :first).click
click_link "Settings", match: :first
end
end
end