From bbe45ab0554820ded9f1b9b6c2f1224444303d57 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 18 Aug 2026 20:58:55 +0100 Subject: [PATCH 01/10] [DREAM-775] Extract move menu items Every sortable-lists consumer offers the same four directional moves, but each surface hand-maintains its own labels, icons and direction values. Introduces one descriptor so the vocabulary has a single home and Backlogs renders from it. The vocabulary carries the acts_as_list `move_to` value alongside the relative direction, so the surfaces that still persist server-side can adopt the table before they move onto the `prev_id` wire. https://community.openproject.org/wp/DREAM-775 --- .../sortable_lists/move_menu_items.rb | 80 +++++++++++++++++ .../work_package_card_menu_component.html.erb | 2 +- .../work_package_card_menu_component.rb | 26 +----- .../sortable_lists/move_menu_items_spec.rb | 86 +++++++++++++++++++ 4 files changed, 169 insertions(+), 25 deletions(-) create mode 100644 app/components/sortable_lists/move_menu_items.rb create mode 100644 spec/components/sortable_lists/move_menu_items_spec.rb diff --git a/app/components/sortable_lists/move_menu_items.rb b/app/components/sortable_lists/move_menu_items.rb new file mode 100644 index 000000000000..33a88871c580 --- /dev/null +++ b/app/components/sortable_lists/move_menu_items.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module SortableLists + # Shared vocabulary and rendering for the four-direction move items every + # sortable-lists consumer offers (#DREAM-775). + # + # `move_to:` carries the acts_as_list value surfaces that still persist + # server-side build their hrefs from; each drops it on moving to `prev_id`. + class MoveMenuItems + include ActionView::RecordIdentifier + + DIRECTIONS = [ + { direction: :top, label: :label_sort_highest, icon: :"move-to-top", move_to: :highest }.freeze, + { direction: :up, label: :label_sort_higher, icon: :"chevron-up", move_to: :higher }.freeze, + { direction: :down, label: :label_sort_lower, icon: :"chevron-down", move_to: :lower }.freeze, + { direction: :bottom, label: :label_sort_lowest, icon: :"move-to-bottom", move_to: :lowest }.freeze + ].freeze + + def initialize(dom_key:) + @dom_key = dom_key + end + + def render_into(menu) + DIRECTIONS.each { |entry| add_item(menu, entry) } + end + + private + + attr_reader :dom_key + + def add_item(menu, entry) + direction = entry[:direction] + + # The `data:` hash must live at item level so Primer renders it on the + # ActionList `
  • `: the controller's targets and the action-menu API's + # `disableItem`/`enableItem` both address that element, and the click + # action rides it via the bubbled button click. + menu.with_item( + id: dom_target(dom_key, :menu, direction), + label: I18n.t(entry[:label]), + tag: :button, + data: { + sortable_lists__item_target: "moveItem", + sortable_lists__item_direction_param: direction, + action: "click->sortable-lists--item#move" + } + ) do |item| + item.with_leading_visual_icon(icon: entry[:icon]) + end + end + end +end diff --git a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb index b339a5285b40..e394d43009cc 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb +++ b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb @@ -131,7 +131,7 @@ See COPYRIGHT and LICENSE files for more details. ) do |move_menu| move_menu.with_leading_visual_icon(icon: :"op-arrow-in") - build_move_menu(move_menu) + move_menu_items.render_into(move_menu) end end end diff --git a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb index 56e2bf1b916d..e272a87263d8 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb +++ b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb @@ -77,30 +77,8 @@ def show_move_submenu? show_move_items? || show_move_to_sprint? || show_move_to_inbox? || show_move_to_backlog_bucket? end - def build_move_menu(menu) - build_move_item(menu, label: :label_sort_highest, direction: "top", icon: :"move-to-top") - build_move_item(menu, label: :label_sort_higher, direction: "up", icon: :"chevron-up") - build_move_item(menu, label: :label_sort_lower, direction: "down", icon: :"chevron-down") - build_move_item(menu, label: :label_sort_lowest, direction: "bottom", icon: :"move-to-bottom") - end - - def build_move_item(menu, label:, icon:, direction:) - # The `data:` hash must live on the item level so Primer renders it on - # the ActionList `
  • `: the controller's `
  • ` targets and the - # action-menu API's `disableItem`/`enableItem` both address that element, - # and the click action rides it via the bubbled button click. - menu.with_item( - id: dom_target(work_package, :menu, label), - label: I18n.t(label), - tag: :button, - data: { - sortable_lists__item_target: "moveItem", - sortable_lists__item_direction_param: direction, - action: "click->sortable-lists--item#move" - } - ) do |item| - item.with_leading_visual_icon(icon:) - end + def move_menu_items + SortableLists::MoveMenuItems.new(dom_key: work_package) end def inbox_list_type diff --git a/spec/components/sortable_lists/move_menu_items_spec.rb b/spec/components/sortable_lists/move_menu_items_spec.rb new file mode 100644 index 000000000000..7c2d0cc24909 --- /dev/null +++ b/spec/components/sortable_lists/move_menu_items_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "rails_helper" + +RSpec.describe SortableLists::MoveMenuItems, type: :component do + describe "DIRECTIONS" do + it "carries direction, label, icon and legacy move_to for every entry, in menu order" do + expect(described_class::DIRECTIONS).to eq( + [ + { direction: :top, label: :label_sort_highest, icon: :"move-to-top", move_to: :highest }, + { direction: :up, label: :label_sort_higher, icon: :"chevron-up", move_to: :higher }, + { direction: :down, label: :label_sort_lower, icon: :"chevron-down", move_to: :lower }, + { direction: :bottom, label: :label_sort_lowest, icon: :"move-to-bottom", move_to: :lowest } + ] + ) + end + + it "is frozen as an array and entry by entry" do + expect(described_class::DIRECTIONS).to be_frozen + expect(described_class::DIRECTIONS).to all(be_frozen) + end + end + + describe "#render_into" do + before do + render_inline(Primer::Alpha::ActionMenu::List.new(menu_id: "test-menu")) do |list| + described_class.new(dom_key: :story).render_into(list) + end + end + + it "renders one item per direction, in menu order" do + expect(page).to have_css("li", count: 4) + expect(page.all("li").map { it.text.strip }) + .to eq(["Move to top", "Move up", "Move down", "Move to bottom"]) + end + + # The wiring must sit on the `
  • `, not the button: the controller's targets + # and the action-menu API's disableItem/enableItem both address that element. + it "renders every item with its dom_key id, label, icon and Stimulus wiring", + :aggregate_failures do + [ + ["top", "story_menu_top", "Move to top", "octicon-move-to-top"], + ["up", "story_menu_up", "Move up", "octicon-chevron-up"], + ["down", "story_menu_down", "Move down", "octicon-chevron-down"], + ["bottom", "story_menu_bottom", "Move to bottom", "octicon-move-to-bottom"] + ].each do |direction, id, label, icon| + item = page.find( + :element, :li, + "data-sortable-lists--item-direction-param": direction, + "data-sortable-lists--item-target": "moveItem", + "data-action": "click->sortable-lists--item#move" + ) + expect(item).to have_element(:button, id:, text: label) + expect(item).to have_css(".#{icon}") + end + end + end +end From f3614f20be1ca56db35d328c09fe2702ed1bdfb6 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 05:04:32 +0100 Subject: [PATCH 02/10] [AGILE-364] Unify menu action scope Resolves each action menu scope before presentation so contextual and More-button invocation share one synchronous selection policy. The item projects availability from that settled scope while retaining non-mutating late refreshes for deferred menu content. https://community.openproject.org/wp/AGILE-364 --- .../contextual-action-menu.controller.spec.ts | 57 ++++++ .../dynamic/sortable-lists.controller.spec.ts | 79 +++++++++ .../sortable-lists/item.controller.spec.ts | 164 +++++++++++++++++- .../dynamic/sortable-lists/item.controller.ts | 34 +++- 4 files changed, 330 insertions(+), 4 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.spec.ts index c137af3a9040..9cc1fed2a5d4 100644 --- a/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.spec.ts @@ -462,6 +462,63 @@ describe('Contextual action menu controller', () => { expect(origins).toEqual(['pointer', 'keyboard']); }); + it('dispatches a cancelable pointer beforeOpen before the presenter opens', async () => { + const { card } = renderCard(); + await nextFrame(); + const sequence:string[] = []; + const events:CustomEvent<{ origin:string }>[] = []; + card.addEventListener('contextual-action-menu:beforeOpen', (event) => { + sequence.push('beforeOpen'); + events.push(event as CustomEvent<{ origin:string }>); + }); + vi.spyOn(ContextualActionMenu.prototype, 'openAtPoint').mockImplementation(() => { + sequence.push('present'); + }); + + contextMenu(card); + + expect(sequence).toEqual(['beforeOpen', 'present']); + expect(events).toHaveLength(1); + expect(events[0]).toMatchObject({ cancelable: true, detail: { origin: 'pointer' } }); + }); + + it.each([ + ['Context Menu key', 'ContextMenu', {}], + ['Shift+F10', 'F10', { shiftKey: true }], + ])('dispatches a cancelable keyboard beforeOpen before the presenter opens for %s', async (_label, key, init) => { + const { card } = renderCard(); + await nextFrame(); + const sequence:string[] = []; + const events:CustomEvent<{ origin:string }>[] = []; + card.addEventListener('contextual-action-menu:beforeOpen', (event) => { + sequence.push('beforeOpen'); + events.push(event as CustomEvent<{ origin:string }>); + }); + vi.spyOn(ContextualActionMenu.prototype, 'openAtInvoker').mockImplementation(() => { + sequence.push('present'); + }); + + keydown(card, key, init); + + expect(sequence).toEqual(['beforeOpen', 'present']); + expect(events).toHaveLength(1); + expect(events[0]).toMatchObject({ cancelable: true, detail: { origin: 'keyboard' } }); + }); + + it('cancellation prevents both pointer and keyboard presentation', async () => { + const { card } = renderCard(); + await nextFrame(); + const openAtPoint = vi.spyOn(ContextualActionMenu.prototype, 'openAtPoint'); + const openAtInvoker = vi.spyOn(ContextualActionMenu.prototype, 'openAtInvoker'); + card.addEventListener('contextual-action-menu:beforeOpen', (event) => event.preventDefault()); + + contextMenu(card); + keydown(card, 'ContextMenu'); + + expect(openAtPoint).not.toHaveBeenCalled(); + expect(openAtInvoker).not.toHaveBeenCalled(); + }); + it('leaves the native context menu alone when the card has no action menu', async () => { const { card } = renderCardWithoutMenu(); await nextFrame(); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 4cbb4a39aff5..f34d5f490b85 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1575,6 +1575,85 @@ describe('Sortable lists controller', () => { ])).toEqual([{ type: 'sprint', id: '1' }]); }); + describe('action menu invocation scope', () => { + const selectItem = (item:HTMLElement, init:MouseEventInit = {}):void => { + item.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ...init })); + }; + + it('preserves a selected eligible invoker and returns the live ordered batch', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + selectItem(items[1]); + selectItem(items[0], { ctrlKey: true }); + + expect(controller.selectForAction(items[1])).toMatchObject({ + kind: 'batch' + }); + expect(controller.selectedItems().map((item) => item.id)).toEqual(['1', '2']); + }); + + it('replaces an unrelated selection when an unselected eligible invoker opens its menu', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + selectItem(items[0]); + selectItem(items[1], { ctrlKey: true }); + + expect(controller.selectForAction(items[2])).toMatchObject({ + kind: 'batch' + }); + expect(controller.selectedItems().map((item) => item.id)).toEqual(['3']); + }); + + it('preserves the current selection when a fixed invoker opens its singular menu', async () => { + const { root, items } = renderSelectableRoot(); + items[2].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + selectItem(items[0]); + selectItem(items[1], { ctrlKey: true }); + announceSpy.mockClear(); + + expect(controller.selectForAction(items[2])).toMatchObject({ kind: 'refused', items: [] }); + expect(controller.selectedItems().map((item) => item.id)).toEqual(['1', '2']); + expect(announcedMessages()).toEqual([]); + }); + + it('returns a prospective one-card scope without changing visible selection while busy', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + selectItem(items[0]); + selectItem(items[1], { ctrlKey: true }); + root.setAttribute('data-sortable-lists-busy', 'true'); + + expect(controller.selectForAction(items[2])).toMatchObject({ + kind: 'batch' + }); + expect(controller.selectedItems().map((item) => item.id)).toEqual(['1', '2']); + }); + + it('settles duplicate pre-open delivery once without repeating its selection announcement', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + selectItem(items[0]); + selectItem(items[1], { ctrlKey: true }); + announceSpy.mockClear(); + + const contextualScope = controller.selectForAction(items[2]); + const popoverScope = controller.selectForAction(items[2]); + + expect(contextualScope).toMatchObject({ kind: 'batch' }); + expect(popoverScope).toMatchObject({ kind: 'batch' }); + expect(controller.selectedItems().map((item) => item.id)).toEqual(['3']); + expect(announcedMessages()).toEqual([ + ['[selected:1]', { politeness: 'polite' }], + ]); + }); + }); + describe('direct destination moves', () => { const destination = { type: 'inbox', id: null }; diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index 72da4217e743..ac9c122eb1ac 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -58,7 +58,7 @@ import type { ActionEvent } from '@hotwired/stimulus'; import type ItemControllerType from './item.controller'; import type { SortableListsRoot } from './drag-and-drop'; import type { DestinationIdentity } from './list-dom'; -import type { ActionScope } from './action-scope'; +import { scopeIds, type ActionScope } from './action-scope'; describe('Sortable lists item controller', () => { let draggable:typeof draggableFn; @@ -1493,6 +1493,168 @@ describe('Sortable lists item controller', () => { expect(actionScopeFor).not.toHaveBeenCalled(); }); + async function mountActionMenuInvocationFixture() { + const { el } = renderItemWithMenu(1); + const card = document.createElement('article'); + card.setAttribute('data-sortable-lists--item-target', 'focus'); + const actionMenu = el.querySelector('action-menu')!; + const popover = actionMenu.querySelector('anchored-position')!; + popover.setAttribute('popover', ''); + card.append(actionMenu); + el.append(card); + destinationFor(el, [{ type: 'inbox', id: null }]); + document.body.appendChild(el); + + const controller = await mountItemController(el); + const scope:ActionScope = { kind: 'batch', items: [el] }; + const sequence:string[] = []; + const selectForAction = vi.fn(() => { + sequence.push('prepare'); + return scope; + }); + const availableDestinations = vi.fn((projectedScope:ActionScope, candidates:DestinationIdentity[]) => { + sequence.push(`project:${scopeIds(projectedScope).join(',')}`); + return candidates; + }); + const root = { + ...stubRoot(el, { isFirst: true, isLast: true }), + selectForAction, + availableDestinations, + }; + controller.connectRoot(root); + await menuCtx!.nextFrame(); + sequence.length = 0; + selectForAction.mockClear(); + availableDestinations.mockClear(); + + return { + el, + card, + popover, + controller, + root, + scope, + sequence, + selectForAction, + availableDestinations, + }; + } + + it.each([ + ['pointer context menu', 'pointer'], + ['Context Menu key', 'keyboard'], + ['Shift+F10', 'keyboard'], + ])('settles action scope before projecting a %s invocation', async (_label, origin) => { + const { + el, card, scope, sequence, selectForAction, availableDestinations, + } = await mountActionMenuInvocationFixture(); + + card.dispatchEvent(new CustomEvent('contextual-action-menu:beforeOpen', { + bubbles: true, + cancelable: true, + detail: { origin }, + })); + + expect(selectForAction).toHaveBeenCalledWith(el); + expect(availableDestinations).toHaveBeenCalledWith(scope, [{ type: 'inbox', id: null }]); + expect(sequence).toEqual(['prepare', 'project:1']); + }); + + it.each([ + 'More button pointer invocation', + 'More button keyboard invocation', + ])('settles action scope before projecting a %s', async () => { + const { + el, popover, scope, sequence, selectForAction, availableDestinations, + } = await mountActionMenuInvocationFixture(); + + popover.dispatchEvent(new ToggleEvent('beforetoggle', { + oldState: 'closed', + newState: 'open', + })); + + expect(selectForAction).toHaveBeenCalledWith(el); + expect(availableDestinations).toHaveBeenCalledWith(scope, [{ type: 'inbox', id: null }]); + expect(sequence).toEqual(['prepare', 'project:1']); + }); + + it('ignores closing and unrelated popover pre-toggle events', async () => { + const { card, popover, selectForAction } = await mountActionMenuInvocationFixture(); + const tooltip = document.createElement('div'); + tooltip.setAttribute('popover', ''); + card.append(tooltip); + + popover.dispatchEvent(new ToggleEvent('beforetoggle', { + oldState: 'open', + newState: 'closed', + })); + tooltip.dispatchEvent(new ToggleEvent('beforetoggle', { + oldState: 'closed', + newState: 'open', + })); + + expect(selectForAction).not.toHaveBeenCalled(); + }); + + it('does nothing when a pre-open event arrives after its root disconnects', async () => { + const { + controller, card, popover, selectForAction, + } = await mountActionMenuInvocationFixture(); + controller.disconnectRoot(); + + card.dispatchEvent(new CustomEvent('contextual-action-menu:beforeOpen', { + bubbles: true, + cancelable: true, + detail: { origin: 'pointer' }, + })); + popover.dispatchEvent(new ToggleEvent('beforetoggle', { + oldState: 'closed', + newState: 'open', + })); + + expect(selectForAction).not.toHaveBeenCalled(); + }); + + it('projects one stable scope when contextual and popover pre-open events both arrive', async () => { + const { + card, popover, scope, selectForAction, availableDestinations, + } = await mountActionMenuInvocationFixture(); + + card.dispatchEvent(new CustomEvent('contextual-action-menu:beforeOpen', { + bubbles: true, + cancelable: true, + detail: { origin: 'pointer' }, + })); + popover.dispatchEvent(new ToggleEvent('beforetoggle', { + oldState: 'closed', + newState: 'open', + })); + + expect(selectForAction).toHaveBeenCalledTimes(2); + expect(availableDestinations).toHaveBeenCalledTimes(2); + expect(availableDestinations.mock.calls.map(([projectedScope]) => projectedScope)).toEqual([scope, scope]); + }); + + it('handles a contextual pre-open from the item itself when no focus target exists', async () => { + const { el } = renderItemWithMenu(1); + document.body.appendChild(el); + const controller = await mountItemController(el); + const scope:ActionScope = { kind: 'batch', items: [el] }; + const selectForAction = vi.fn(() => scope); + controller.connectRoot({ + ...stubRoot(el, { isFirst: true, isLast: true }), + selectForAction, + }); + + el.dispatchEvent(new CustomEvent('contextual-action-menu:beforeOpen', { + bubbles: true, + cancelable: true, + detail: { origin: 'pointer' }, + })); + + expect(selectForAction).toHaveBeenCalledWith(el); + }); + it('hides up/top for a first item and shows the rest', async () => { const { el, menu } = renderItemWithMenu(1); document.body.appendChild(el); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index e31e165b7cf3..a5e7a2a4df09 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -62,7 +62,7 @@ import { } from './list-dom'; import { webLinkHref } from './external-data'; import { renderDragPreview } from './preview'; -import { scopeIds } from './action-scope'; +import { scopeIds, type ActionScope } from './action-scope'; import { refreshMenuAvailability } from './menu-availability'; type CleanupFn = () => void; @@ -126,9 +126,28 @@ export default class ItemController extends Controller implements R } }; + private readonly onContextualBeforeOpen = (event:Event):void => { + const focusTarget = this.hasFocusTarget ? this.focusTarget : null; + if (event.target === this.element || event.target === focusTarget) { + this.prepareActionMenu(); + } + }; + + private readonly onMenuBeforeToggle = (event:Event):void => { + if ( + (event as ToggleEvent).newState === 'open' + && this.hasMenuElement + && event.target === this.menuElement.popoverElement + ) { + this.prepareActionMenu(); + } + }; + connect():void { this.warnOnMissingValues(); this.register(); + this.element.addEventListener('contextual-action-menu:beforeOpen', this.onContextualBeforeOpen); + this.element.addEventListener('beforetoggle', this.onMenuBeforeToggle, true); this.element.addEventListener('toggle', this.onMenuToggle, true); } @@ -140,6 +159,8 @@ export default class ItemController extends Controller implements R this.clearDropIndicator(); this.cleanupFn?.(); this.cleanupFn = undefined; + this.element.removeEventListener('contextual-action-menu:beforeOpen', this.onContextualBeforeOpen); + this.element.removeEventListener('beforetoggle', this.onMenuBeforeToggle, true); this.element.removeEventListener('toggle', this.onMenuToggle, true); this.disconnectRoot(); } @@ -515,7 +536,14 @@ export default class ItemController extends Controller implements R this.dropIndicatorElement = undefined; } - private refreshActionAvailability():void { + private prepareActionMenu():void { + const scope = this.root?.selectForAction(this.element); + if (scope) { + this.refreshActionAvailability(scope); + } + } + + private refreshActionAvailability(preparedScope?:ActionScope):void { this.refreshToken = undefined; const root = this.root; if (!root || !this.hasMenuElement || (this.destinationItemTargets.length === 0 && this.moveItemTargets.length === 0)) { @@ -524,7 +552,7 @@ export default class ItemController extends Controller implements R refreshMenuAvailability({ menu: this.menuElement, - scope: root.actionScopeFor(this.element), + scope: preparedScope ?? root.actionScopeFor(this.element), destinationItems: this.destinationItemTargets, moveItems: this.moveItemTargets, moveMenu: this.hasMoveMenuTarget ? this.moveMenuTarget : null, From 734ec124eeeca9002e2e0120d80d1c8477194a30 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 05:42:44 +0100 Subject: [PATCH 03/10] [AGILE-364] Cover menu invocation policy Locks all card-menu entry points to one observable action-scope contract so presenter changes cannot silently collapse or retarget selected batches. https://community.openproject.org/wp/AGILE-364 --- .../work_packages/batch_action_menu_spec.rb | 183 ++++++++++++++++++ .../work_packages/card_context_menu_spec.rb | 4 +- .../work_packages/move_via_menu_spec.rb | 8 +- .../backlogs/spec/support/pages/backlog.rb | 102 ++++++++++ 4 files changed, 293 insertions(+), 4 deletions(-) create mode 100644 modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb diff --git a/modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb b/modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb new file mode 100644 index 000000000000..12ef5f3f09c7 --- /dev/null +++ b/modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb @@ -0,0 +1,183 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" +require_relative "../../support/pages/backlog" + +RSpec.describe "Backlogs batch action menus", :js, :selenium, :settings_reset do + let!(:project) do + create(:project, types: [type], enabled_module_names: %w(work_package_tracking backlogs)) + end + let(:type) { create(:type) } + let(:permissions) { %i[view_sprints manage_sprint_items view_work_packages edit_work_packages] } + let(:role) { create(:project_role, permissions:) } + let!(:sprint) { create(:sprint, project:, name: "Sprint") } + let!(:stories) do + Array.new(4) do |index| + create(:work_package, project:, type:, sprint:, position: index + 1) + end + end + let(:backlogs_page) { Pages::Backlog.new(project) } + + current_user do + create(:user, member_with_roles: { project => role }) + end + + before do + backlogs_page.visit! + end + + %i[more right_click context_menu_key shift_f10].each do |entry_point| + context "via #{entry_point.to_s.tr('_', ' ')}" do + it "preserves a selected batch across its menu lifecycle" do + backlogs_page.select_cards(stories[1], stories[0]) + + menu = backlogs_page.open_card_menu(stories[1], via: entry_point) + + backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) + expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details"), focused: true) + backlogs_page.expect_card_menu_anchor(stories[1], via: entry_point) + backlogs_page.expect_card_menu_loaded(stories[1]) + + backlogs_page.dismiss_card_menu(stories[1], via: entry_point) + backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) + end + + it "replaces an old batch when an unselected card invokes the one-card menu" do + backlogs_page.select_cards(stories[0], stories[1]) + + backlogs_page.open_card_menu(stories[2], via: entry_point) + + backlogs_page.expect_selected_cards_in_order(stories[2]) + backlogs_page.expect_card_menu_loaded(stories[2]) + + backlogs_page.dismiss_card_menu(stories[2], via: entry_point) + backlogs_page.expect_selected_cards_in_order(stories[2]) + end + end + end + + it "restores card focus when a contextual batch menu is light-dismissed" do + backlogs_page.select_cards(stories[0], stories[1]) + backlogs_page.open_card_menu(stories[1], via: :right_click) + + backlogs_page.light_dismiss_card_menu(stories[1]) + + backlogs_page.expect_work_package_card_focused(stories[1]) + backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) + end + + it "keeps every singular action bound to its invoking card without collapsing the batch", :aggregate_failures do + backlogs_page.select_cards(stories[0], stories[1]) + + stories.first(2).each do |invoker| + menu = backlogs_page.open_card_menu(invoker, via: :more) + + expect(menu.find(:menuitem, text: I18n.t(:"js.button_open_details"))[:href]) + .to end_with("/backlogs/backlog/details/#{invoker.id}") + expect(menu.find(:menuitem, text: I18n.t(:"js.button_open_fullscreen"))[:href]) + .to end_with("/work_packages/#{invoker.id}") + expect(menu.find(:menuitem, text: "Copy URL to clipboard")[:value]) + .to end_with("/work_packages/#{invoker.id}") + expect(menu.find(:menuitem, text: "Copy work package ID")[:value]).to eq(invoker.id.to_s) + + backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) + backlogs_page.dismiss_card_menu(invoker, via: :more) + end + end + + it "passes the ordered batch to a destination dialog and a positional move" do + create(:sprint, project:, name: "Destination") + backlogs_page.visit! + backlogs_page.select_contiguous_cards(stories[1], stories[2]) + + backlogs_page.activate_batch_menu_action( + invoker: stories[2], + action: "Move to sprint", + via: :context_menu_key + ) + backlogs_page.expect_destination_dialog("Move to sprint", work_packages: [stories[1], stories[2]]) + backlogs_page.cancel_destination_dialog("Move to sprint") + + backlogs_page.activate_batch_menu_action( + invoker: stories[1], + action: "Move down", + via: :shift_f10 + ) + + moved_order = [stories[0], stories[3], stories[1], stories[2]] + backlogs_page.expect_sprint_items_in_order(sprint, items: moved_order) + + backlogs_page.visit! + backlogs_page.expect_sprint_items_in_order(sprint, items: moved_order) + end + + it "keeps positional actions when the destination intersection is empty" do + sprint.update_columns(status: "completed") + inbox_stories = Array.new(3) do |index| + create(:work_package, project:, type:, position: index + 1) + end + backlogs_page.visit! + backlogs_page.select_contiguous_cards(inbox_stories[0], inbox_stories[1]) + + backlogs_page.open_card_menu(inbox_stories[0], via: :right_click) + + backlogs_page.expect_open_menu_actions( + invoker: inbox_stories[0], + present: ["Move to position"], + absent: ["Move to sprint", "Move to backlog bucket", "Move to backlog inbox"] + ) + end + + it "keeps destination actions when a sparse selection has no valid position" do + create(:sprint, project:, name: "Destination") + backlogs_page.visit! + backlogs_page.select_cards(stories[0], stories[2]) + + backlogs_page.open_card_menu(stories[0], via: :more) + + backlogs_page.expect_open_menu_actions( + invoker: stories[0], + present: ["Move to sprint"], + absent: ["Move to position"] + ) + end + + context "when every card is fixed" do + let(:permissions) { %i[view_sprints view_work_packages] } + + it "shows the unchanged singular menu for a fixed card" do + backlogs_page.open_card_menu(stories[0], via: :more) + + backlogs_page.expect_fixed_action_menu(invoker: stories[0]) + backlogs_page.expect_no_selected_cards + end + end +end diff --git a/modules/backlogs/spec/features/work_packages/card_context_menu_spec.rb b/modules/backlogs/spec/features/work_packages/card_context_menu_spec.rb index 547c74bdfd6f..0e6f3a557c79 100644 --- a/modules/backlogs/spec/features/work_packages/card_context_menu_spec.rb +++ b/modules/backlogs/spec/features/work_packages/card_context_menu_spec.rb @@ -70,7 +70,7 @@ end it "opens the lazily loaded menu on right-click and focuses its first action" do - backlogs_page.right_click_work_package_card(sprint_wp) + backlogs_page.open_card_menu(sprint_wp, via: :right_click) backlogs_page.within_work_package_context_menu(sprint_wp) do |menu| expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details"), focused: true) @@ -124,7 +124,7 @@ # position of its own: same menu, same card, so the two ways of asking for it # have no reason to disagree. Only the focus return stays contextual. it "opens the same menu from the keyboard with Shift+F10" do - backlogs_page.send_work_package_card_keys(sprint_wp, %i[shift f10]) + backlogs_page.open_card_menu(sprint_wp, via: :shift_f10) backlogs_page.within_work_package_context_menu(sprint_wp) do |menu| expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details"), focused: true) diff --git a/modules/backlogs/spec/features/work_packages/move_via_menu_spec.rb b/modules/backlogs/spec/features/work_packages/move_via_menu_spec.rb index 06d5512802c2..56349314f44a 100644 --- a/modules/backlogs/spec/features/work_packages/move_via_menu_spec.rb +++ b/modules/backlogs/spec/features/work_packages/move_via_menu_spec.rb @@ -84,8 +84,12 @@ # helper: a same-list menu move now settles via a background fetch, not a # frame reload, so the assertion right below has to observe the DOM before # that request has any chance to complete. - backlogs_page - .click_in_sprint_story_move_menu(sprint_wp2, "Move up", wait: false) + backlogs_page.activate_batch_menu_action( + invoker: sprint_wp2, + action: "Move up", + via: :more, + wait: false + ) backlogs_page .expect_sprint_items_in_order(sprint, diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index 7109c93bb1b5..85edc6c78e90 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -339,6 +339,104 @@ def within_work_package_menu(work_package, &) dismiss_menu(work_package) end + # Opens one card's menu through the same four user entry points supported + # by Backlogs. Keeping the native key/pointer synthesis and deferred-menu + # wait here lets feature specs talk in product language and prevents each + # caller from encoding Primer's overlay structure. + def open_card_menu(work_package, via:) + case via + when :more + within_work_package(work_package) do + find(:button, accessible_name: "Work package actions").click + end + when :right_click + right_click_work_package_card(work_package) + when :context_menu_key + # Selenium's W3C key table does not expose the dedicated Context Menu + # key. Dispatch its browser-level KeyboardEvent against a focused card + # so this path still differs from Shift+F10 at the controller boundary. + work_package_card(work_package).execute_script(<<~JS) + this.focus({ focusVisible: true, preventScroll: true }); + this.dispatchEvent(new KeyboardEvent('keydown', { + key: 'ContextMenu', + bubbles: true, + cancelable: true + })); + JS + when :shift_f10 + send_work_package_card_keys(work_package, %i[shift f10]) + else + raise ArgumentError, "Unknown card-menu entry point: #{via.inspect}" + end + + work_package_action_menu(work_package) + end + + def activate_singular_menu_action(invoker:, action:, via: :more) + open_card_menu(invoker, via:).find(:menuitem, text: action, exact_text: true).click + end + + def activate_batch_menu_action(invoker:, action:, via: :more, wait: true) + menu = open_card_menu(invoker, via:) + + if %w[Move\ to\ top Move\ up Move\ down Move\ to\ bottom].include?(action) + wait_for_backlogs_turbo_stream(wait:) do + open_move_submenu(menu).find(:menuitem, text: action, exact_text: true, visible: :all).click + end + else + menu.find(:menuitem, text: action, exact_text: true).click + end + end + + def expect_card_menu_anchor(work_package, via:) + if via == :right_click + expect_menu_anchored_contextually(work_package) + else + expect_menu_anchored_at_button(work_package) + end + end + + def expect_card_menu_loaded(work_package) + expect(page.find(menu_owner_overlay_selector(work_package))) + .to have_no_css("include-fragment[src]", visible: :all) + end + + def dismiss_card_menu(work_package, via:) + send_keys_to_focused_element(:escape) + expect_no_work_package_context_menu(work_package) + + if via == :more + within_work_package(work_package) do + expect(page).to have_button(accessible_name: "Work package actions", focused: true) + end + else + expect_work_package_card_focused(work_package) + end + end + + def light_dismiss_card_menu(work_package) + find(:heading, I18n.t(:label_backlog_and_sprints), level: 2).click + expect_no_work_package_context_menu(work_package) + end + + def expect_open_menu_actions(invoker:, present:, absent:) + menu = work_package_action_menu(invoker) + present.each do |label| + expect(menu).to have_selector(:menuitem, text: label, exact_text: true) + end + absent.each do |label| + expect(menu).to have_no_selector(:menuitem, text: label, exact_text: true) + end + end + + def expect_fixed_action_menu(invoker:) + menu = work_package_action_menu(invoker) + + expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details")) + expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_fullscreen")) + expect(menu).to have_no_selector(:menuitem, text: "Move to position") + end + def work_package_card(work_package) find(work_package_card_selector(work_package)) end @@ -1314,6 +1412,10 @@ def menu_owner_overlay_selector(menu_owner) "##{ActionView::RecordIdentifier.dom_target(menu_owner, :menu)}-overlay" end + def work_package_action_menu(work_package) + page.find(menu_owner_overlay_selector(work_package)).find(:menu) + end + # Located by the lock's accessible name so the expectation fails if the # icon ever loses the text that explains it. def readonly_lock_selector From daf00288229ae4df053a525f5eb7b334de289b3d Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sun, 16 Aug 2026 06:17:01 +0100 Subject: [PATCH 04/10] [AGILE-364] Hide fixed-card batch actions Projects the settled action scope before exposing deferred batch destinations and position controls, so a fixed invoker cannot present stale batch actions the server would refuse. https://community.openproject.org/wp/AGILE-364 --- .../dynamic/sortable-lists.controller.spec.ts | 2 +- .../dynamic/sortable-lists/drag-and-drop.ts | 3 + .../sortable-lists/item.controller.spec.ts | 59 ++++++++++++++++++- .../dynamic/sortable-lists/item.controller.ts | 1 + .../sortable-lists/menu-availability.spec.ts | 1 + .../sortable-lists/menu-availability.ts | 8 ++- .../selection-orchestrator.spec.ts | 13 ++++ 7 files changed, 81 insertions(+), 6 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index f34d5f490b85..4574b46628e3 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1615,7 +1615,7 @@ describe('Sortable lists controller', () => { selectItem(items[1], { ctrlKey: true }); announceSpy.mockClear(); - expect(controller.selectForAction(items[2])).toMatchObject({ kind: 'refused', items: [] }); + expect(controller.selectForAction(items[2])).toEqual({ kind: 'refused', items: [] }); expect(controller.selectedItems().map((item) => item.id)).toEqual(['1', '2']); expect(announcedMessages()).toEqual([]); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts index 38591b802c66..b0ef22eb5b7f 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts @@ -100,6 +100,9 @@ export interface SortableListData extends Record { // Implemented by the sortable-lists root controller and handed to list/item // controllers via outlet callbacks, so children read shared state through a // typed reference instead of walking the DOM. +// +// Membership stays inside SelectionOrchestrator: children get action scopes, +// availability and the frozen drag batch, never the model itself. export interface SortableListsRoot { readonly element:HTMLElement; readonly busy:boolean; diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index ac9c122eb1ac..2debbead2d33 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -1445,10 +1445,26 @@ describe('Sortable lists item controller', () => { function stubMenuRoot(el:HTMLElement, position:{ isFirst:boolean; isLast:boolean }) { const actionScopeFor = vi.fn(():ActionScope => ({ kind: 'refused', items: [] })); + const selectForAction = vi.fn(():ActionScope => ({ kind: 'refused', items: [] })); const availableDestinations = vi.fn((_scope:ActionScope, _candidates:DestinationIdentity[]):DestinationIdentity[] => []); - const root = { ...stubRoot(el, position), actionScopeFor, availableDestinations }; + const moveToDestination = vi.fn(); + const moveInDirection = vi.fn(); + const root = { + ...stubRoot(el, position, moveInDirection), + actionScopeFor, + selectForAction, + availableDestinations, + moveToDestination, + }; - return { root, actionScopeFor, availableDestinations }; + return { + root, + actionScopeFor, + selectForAction, + availableDestinations, + moveToDestination, + moveInDirection, + }; } it('ignores opening tooltips and submenu overlays', async () => { @@ -1866,10 +1882,47 @@ describe('Sortable lists item controller', () => { const moveToSprint = destinationFor(el, [{ type: 'sprint', id: '12' }]); await menuCtx!.nextFrame(); - expect(availableDestinations).toHaveBeenCalledWith(scope, [{ type: 'sprint', id: '12' }]); + expect(availableDestinations).not.toHaveBeenCalled(); expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); }); + it('hides a stale batch action skeleton for a fixed singular invoker', async () => { + const { el } = renderItemWithMenu(1, true); + el.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { + root, + actionScopeFor, + selectForAction, + availableDestinations, + moveToDestination, + moveInDirection, + } = stubMenuRoot(el, { isFirst: false, isLast: false }); + const scope:ActionScope = { kind: 'refused', items: [] }; + actionScopeFor.mockReturnValue(scope); + // Keep the collaborators deliberately permissive: the item controller + // must project the already-settled singular scope, not let stale batch + // markup look actionable while the root's defences would reject it. + availableDestinations.mockImplementation((_scope, candidates) => candidates); + controller.connectRoot(root); + + const destination = destinationFor(el, [{ type: 'sprint', id: '1' }]); + await menuCtx!.nextFrame(); + + expect(destination).toHaveAttribute('hidden'); + expect(el.querySelector('[data-sortable-lists--item-target="moveMenu"]')).toHaveAttribute('hidden'); + + liFor(el, 'down').click(); + const destinationEvent = new Event('click') as ActionEvent; + Object.defineProperty(destinationEvent, 'currentTarget', { value: destination }); + controller.moveToDestination(destinationEvent); + + expect(moveInDirection).not.toHaveBeenCalled(); + expect(moveToDestination).not.toHaveBeenCalled(); + expect(selectForAction).not.toHaveBeenCalled(); + }); + it.each([ ['invalid JSON', '{invalid'], ['an empty destination type', '[{"type":"","id":null}]'], diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index a5e7a2a4df09..93d1eb54f280 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -553,6 +553,7 @@ export default class ItemController extends Controller implements R refreshMenuAvailability({ menu: this.menuElement, scope: preparedScope ?? root.actionScopeFor(this.element), + itemOrderable: isOrderableItem(this.element), destinationItems: this.destinationItemTargets, moveItems: this.moveItemTargets, moveMenu: this.hasMoveMenuTarget ? this.moveMenuTarget : null, diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.spec.ts index 6f712618c002..6eb8406ee747 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.spec.ts @@ -44,6 +44,7 @@ function fixture(hideUnavailable = true) { const input:Parameters[0] = { menu, scope: { kind: 'batch', items: [document.createElement('div'), document.createElement('div')] }, + itemOrderable: true, destinationItems: [destination], moveItems: [moveItem], moveMenu, diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts index f4e72ef23e61..fd56f9ad83d3 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts @@ -34,6 +34,7 @@ import { isMoveDirection, parseDestinationCandidates, type DestinationIdentity } interface MenuAvailabilityInput { menu:Pick; scope:ActionScope; + itemOrderable:boolean; destinationItems:HTMLElement[]; moveItems:HTMLElement[]; moveMenu:HTMLElement|null; @@ -53,12 +54,15 @@ export function refreshMenuAvailability(input:MenuAvailabilityInput):void { } }; + const refused = input.scope.kind === 'refused' && !input.itemOrderable; for (const item of input.destinationItems) { const candidates = parseDestinationCandidates(item.dataset.sortableListsDestinations); - setAvailability(item, candidates.length > 0 && input.availableDestinations(input.scope, candidates).length > 0); + setAvailability(item, !refused && candidates.length > 0 && input.availableDestinations(input.scope, candidates).length > 0); } - const availability = input.moveAvailability(); + const availability = refused + ? { top: false, up: false, down: false, bottom: false } + : input.moveAvailability(); if (availability) { let available = 0; for (const item of input.moveItems) { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index 53557e83f409..5914431a0fe0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -809,6 +809,19 @@ describe('SelectionOrchestrator', () => { expect(selectedIds()).toEqual(['2']); }); + // A refused invoker must not re-anchor the selection: the following + // Shift range still extends from the card the user actually picked. + it('reports a fixed action invoker without changing the selection anchor', () => { + item('3').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + + orchestrator.selectForAction(item('3')); + orchestrator.handleClick(clickOn(item('2'), { shiftKey: true })); + + expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2']); + }); + it('keeps the selected batch for an action on a member', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); orchestrator.handleClick(clickOn(item('1'))); From ab334d543339d86dcf575aa84917205de6cfe8f4 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 18 Aug 2026 22:03:43 +0100 Subject: [PATCH 05/10] [AGILE-364] Separate the batch and singular menus Presents either the invoker's own actions or the batch actions, never both: UI/UX review rejected the sectioned menu that showed headings over both groups at once. A group toggle replaces the headings, keyed to how many actions the menu presents rather than how many are executable, so disable-mode consumers keep their items on screen. The batch context moves into the invoker's tooltip, whose text is the button's accessible name through Primer's aria-labelledby wiring. The rename applies only while the menu is open and reverts on close; the singular name arrives as a server-rendered value, since the tooltip's own text is no restore source once a Turbo snapshot captures it mid-rename, and connecting restores it over such a snapshot. Reopening an already-loaded menu needs its own focus correction: Primer's open-time pick checks [hidden] one level deep, so it still matches the first singular item under a hidden group and focusing it no-ops. Focus also parks on a presented batch item before its former group hides, because Primer dismisses the menu the moment focus leaves it. The shared dismiss helper closes menus with Escape: the compact batch menu can leave a live menuitem under the overlay's centre point, so the old outside click activated an action instead of dismissing. https://community.openproject.org/wp/AGILE-364 --- .../sortable-lists/item.controller.spec.ts | 422 ++++++++++++++++-- .../dynamic/sortable-lists/item.controller.ts | 168 ++++++- .../sortable-lists/menu-availability.ts | 12 +- .../work_package_card_list_item_component.rb | 6 + .../work_package_card_menu_component.html.erb | 191 ++++---- .../work_package_card_menu_component.rb | 3 - modules/backlogs/config/locales/js-en.yml | 6 + ...k_package_card_list_item_component_spec.rb | 4 +- .../work_package_card_menu_component_spec.rb | 24 +- .../work_packages/batch_action_menu_spec.rb | 23 +- .../backlogs/spec/support/pages/backlog.rb | 67 ++- 11 files changed, 757 insertions(+), 169 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index 2debbead2d33..66ae20f0b41b 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -1370,23 +1370,30 @@ describe('Sortable lists item controller', () => { isItemHidden:ReturnType; } - function renderItemWithMenu(idNumber:number, withDivider = false):{ el:HTMLElement; menu:FakeActionMenu } { + function renderItemWithMenu(idNumber:number):{ el:HTMLElement; menu:FakeActionMenu } { const el = document.createElement('div'); el.dataset.controller = 'sortable-lists--item'; el.setAttribute('data-sortable-lists--item-id-value', String(idNumber)); el.setAttribute('data-sortable-lists--item-type-value', 'work_package'); const menuElement = document.createElement('action-menu'); - // The divider opens the move group, so everything below it is what - // decides whether it still separates anything. - menuElement.innerHTML = withDivider ? '
  • ' : ''; + const invokerGroup = document.createElement('ul'); + invokerGroup.setAttribute('role', 'group'); + invokerGroup.setAttribute('data-sortable-lists--item-target', 'invokerGroup'); + invokerGroup.innerHTML = '
    • '; + const divider = document.createElement('li'); + divider.setAttribute('data-sortable-lists--item-target', 'groupDivider'); + const batchGroup = document.createElement('ul'); + batchGroup.setAttribute('role', 'group'); + batchGroup.setAttribute('data-sortable-lists--item-target', 'batchGroup'); const parent = document.createElement('li'); parent.setAttribute('data-sortable-lists--item-target', 'moveMenu'); parent.innerHTML = ['top', 'up', 'down', 'bottom'].map((direction) => ( `
    • ' )).join(''); - menuElement.appendChild(parent); + batchGroup.appendChild(parent); + menuElement.append(invokerGroup, divider, batchGroup); el.appendChild(menuElement); const menu:FakeActionMenu = { @@ -1408,11 +1415,12 @@ describe('Sortable lists item controller', () => { } const liFor = (el:HTMLElement, direction:string) => el.querySelector(`li[data-sortable-lists--item-direction-param="${direction}"]`)!; + const groupFor = (el:HTMLElement, target:string) => el.querySelector(`[data-sortable-lists--item-target="${target}"]`)!; const destinationWithMetadata = (el:HTMLElement, metadata:string) => { const item = document.createElement('li'); item.setAttribute('data-sortable-lists--item-target', 'destinationItem'); item.dataset.sortableListsDestinations = metadata; - el.querySelector('action-menu')!.append(item); + el.querySelector('[data-sortable-lists--item-target="batchGroup"]')!.append(item); return item; }; const destinationFor = (el:HTMLElement, candidates:{ type:string; id:string|null }[]) => ( @@ -1509,13 +1517,19 @@ describe('Sortable lists item controller', () => { expect(actionScopeFor).not.toHaveBeenCalled(); }); + const attachMenuPopover = (el:HTMLElement):HTMLElement => { + const actionMenu = el.querySelector('action-menu')!; + const popover = actionMenu.querySelector('anchored-position')!; + popover.setAttribute('popover', ''); + return popover; + }; + async function mountActionMenuInvocationFixture() { const { el } = renderItemWithMenu(1); const card = document.createElement('article'); card.setAttribute('data-sortable-lists--item-target', 'focus'); const actionMenu = el.querySelector('action-menu')!; - const popover = actionMenu.querySelector('anchored-position')!; - popover.setAttribute('popover', ''); + const popover = attachMenuPopover(el); card.append(actionMenu); el.append(card); destinationFor(el, [{ type: 'inbox', id: null }]); @@ -1524,6 +1538,7 @@ describe('Sortable lists item controller', () => { const controller = await mountItemController(el); const scope:ActionScope = { kind: 'batch', items: [el] }; const sequence:string[] = []; + const actionScopeFor = vi.fn(() => scope); const selectForAction = vi.fn(() => { sequence.push('prepare'); return scope; @@ -1534,12 +1549,14 @@ describe('Sortable lists item controller', () => { }); const root = { ...stubRoot(el, { isFirst: true, isLast: true }), + actionScopeFor, selectForAction, availableDestinations, }; controller.connectRoot(root); await menuCtx!.nextFrame(); sequence.length = 0; + actionScopeFor.mockClear(); selectForAction.mockClear(); availableDestinations.mockClear(); @@ -1551,6 +1568,7 @@ describe('Sortable lists item controller', () => { root, scope, sequence, + actionScopeFor, selectForAction, availableDestinations, }; @@ -1651,6 +1669,33 @@ describe('Sortable lists item controller', () => { expect(availableDestinations.mock.calls.map(([projectedScope]) => projectedScope)).toEqual([scope, scope]); }); + it('late-refreshes only for the card menu popover, not a nested submenu', async () => { + const { + popover, actionScopeFor, availableDestinations, + } = await mountActionMenuInvocationFixture(); + const nestedPopover = document.createElement('anchored-position'); + nestedPopover.setAttribute('popover', ''); + popover.append(nestedPopover); + + popover.dispatchEvent(new ToggleEvent('toggle', { + oldState: 'closed', + newState: 'open', + })); + + expect(actionScopeFor).toHaveBeenCalledTimes(1); + expect(availableDestinations).toHaveBeenCalledTimes(1); + actionScopeFor.mockClear(); + availableDestinations.mockClear(); + + nestedPopover.dispatchEvent(new ToggleEvent('toggle', { + oldState: 'closed', + newState: 'open', + })); + + expect(actionScopeFor).not.toHaveBeenCalled(); + expect(availableDestinations).not.toHaveBeenCalled(); + }); + it('handles a contextual pre-open from the item itself when no focus target exists', async () => { const { el } = renderItemWithMenu(1); document.body.appendChild(el); @@ -1730,7 +1775,7 @@ describe('Sortable lists item controller', () => { }); it('projects deferred destination items for the selected invoker', async () => { - const { el, menu } = renderItemWithMenu(1, true); + const { el, menu } = renderItemWithMenu(1); document.body.appendChild(el); const controller = await mountItemController(el); const scope:ActionScope = { kind: 'batch', items: [el]}; @@ -1779,9 +1824,9 @@ describe('Sortable lists item controller', () => { menu.showItem.mockClear(); activeScope = prospectiveScope; - const menuElement = el.querySelector('action-menu')!; + const popover = attachMenuPopover(el); const toggle = new ToggleEvent('toggle', { newState: 'open', oldState: 'closed' }); - menuElement.querySelector('anchored-position')!.dispatchEvent(toggle); + popover.dispatchEvent(toggle); expect(actionScopeFor).toHaveBeenLastCalledWith(el); expect(availableDestinations).toHaveBeenCalledWith(prospectiveScope, [{ type: 'sprint', id: '1' }]); @@ -1791,7 +1836,7 @@ describe('Sortable lists item controller', () => { }); it('shows available batch position directions when every destination action is hidden', async () => { - const { el, menu } = renderItemWithMenu(1, true); + const { el, menu } = renderItemWithMenu(1); document.body.appendChild(el); const controller = await mountItemController(el); // Batch size is the member count: two elements, not one element with @@ -1808,7 +1853,6 @@ describe('Sortable lists item controller', () => { await menuCtx!.nextFrame(); const moveMenu = el.querySelector('li[data-sortable-lists--item-target="moveMenu"]')!; - const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); expect(menu.hideItem).toHaveBeenCalledWith(moveToInbox); expect(menu.showItem).toHaveBeenCalledWith(moveMenu); @@ -1816,14 +1860,38 @@ describe('Sortable lists item controller', () => { expect(menu.showItem).toHaveBeenCalledWith(liFor(el, 'up')); expect(menu.showItem).toHaveBeenCalledWith(liFor(el, 'down')); expect(menu.hideItem).toHaveBeenCalledWith(liFor(el, 'bottom')); - expect(divider.hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(true); + expect(groupFor(el, 'batchGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(true); + }); + + // Primer's focus zone stops managing exactly the element the `hidden` + // attribute lands on, so a group hidden on its own leaves its items in + // the zone and the arrows step onto rows that cannot take focus. + it('hides the items of a hidden group, not just the group', async () => { + const { el } = renderItemWithMenu(1); + document.body.appendChild(el); + const controller = await mountItemController(el); + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); + actionScopeFor.mockReturnValue(scope); + availableDestinations.mockReturnValue([]); + root.moveAvailability = () => ({ top: false, up: true, down: true, bottom: false }); + const singularItem = groupFor(el, 'invokerGroup').querySelector('button')!; + singularItem.setAttribute('role', 'menuitem'); + controller.connectRoot(root); + controller.moveItemTargetConnected(); + await Promise.resolve(); + + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(true); + expect(singularItem.hasAttribute('hidden')).toBe(true); }); it('hides an all-unavailable batch position submenu and its directions', async () => { - const { el, menu } = renderItemWithMenu(1, true); + const { el, menu } = renderItemWithMenu(1); document.body.appendChild(el); const controller = await mountItemController(el); - const scope:ActionScope = { kind: 'batch', items: [el] }; + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; const { root, actionScopeFor, availableDestinations } = stubMenuRoot(el, { isFirst: false, isLast: false }); actionScopeFor.mockReturnValue(scope); availableDestinations.mockReturnValue([]); @@ -1834,20 +1902,21 @@ describe('Sortable lists item controller', () => { await menuCtx!.nextFrame(); const moveMenu = el.querySelector('li[data-sortable-lists--item-target="moveMenu"]')!; - const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; expect(menu.hideItem).toHaveBeenCalledWith(moveToSprint); expect(menu.hideItem).toHaveBeenCalledWith(moveMenu); for (const direction of ['top', 'up', 'down', 'bottom']) { expect(menu.hideItem).toHaveBeenCalledWith(liFor(el, direction)); } - expect(divider.hasAttribute('hidden')).toBe(true); + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'batchGroup').hasAttribute('hidden')).toBe(true); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(true); actionScopeFor.mockReturnValue({ kind: 'batch', items: [el] }); root.moveAvailability = () => ({ top: true, up: true, down: false, bottom: false }); el.querySelector('action-menu > anchored-position')!.dispatchEvent(new ToggleEvent('toggle', { newState: 'open' })); expect(moveMenu.hasAttribute('hidden')).toBe(false); expect(liFor(el, 'top').hasAttribute('hidden')).toBe(false); - expect(divider.hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(false); }); it('keeps only the current owner destination for a confined batch scope', async () => { @@ -1887,7 +1956,7 @@ describe('Sortable lists item controller', () => { }); it('hides a stale batch action skeleton for a fixed singular invoker', async () => { - const { el } = renderItemWithMenu(1, true); + const { el } = renderItemWithMenu(1); el.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); document.body.appendChild(el); const controller = await mountItemController(el); @@ -1942,11 +2011,11 @@ describe('Sortable lists item controller', () => { expect(availableDestinations).not.toHaveBeenCalled(); }); - // Regression: the divider is rendered server-side from a permission check - // alone, so an item with nowhere to move used to be left with a separator - // and nothing below it. - it('hides the divider when nothing below it is left visible', async () => { - const { el } = renderItemWithMenu(1, true); + // Regression: the batch group and its divider are rendered server-side + // from a permission check alone, so an item with nowhere to move used to + // be left with a separator and nothing below it. + it('hides the batch group and divider when it presents no action', async () => { + const { el } = renderItemWithMenu(1); const overlay = document.createElement('anchored-position'); overlay.setAttribute('popover', 'auto'); el.querySelector('action-menu')!.append(overlay); @@ -1956,27 +2025,29 @@ describe('Sortable lists item controller', () => { controller.moveItemTargetConnected(); await Promise.resolve(); - const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; - expect(divider.hasAttribute('hidden')).toBe(true); + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'batchGroup').hasAttribute('hidden')).toBe(true); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(true); }); - it('keeps the divider while something below it is still visible', async () => { - const { el } = renderItemWithMenu(1, true); + it('keeps the batch group and divider while an action is presented', async () => { + const { el } = renderItemWithMenu(1); document.body.appendChild(el); const controller = await mountItemController(el); controller.connectRoot(stubRoot(el, { isFirst: true, isLast: false })); controller.moveItemTargetConnected(); await Promise.resolve(); - const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; - expect(divider.hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'batchGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(false); }); - // A divider has no `.ActionListContent`, so routing it through - // `disableItem` would throw — and in this mode the group stays visible - // anyway, only disabled. - it('leaves the divider alone when hideUnavailable is off', async () => { - const { el } = renderItemWithMenu(1, true); + // With hideUnavailable off the consumer asked for unavailable items to + // stay on screen, only disabled — so a group whose items are all + // unavailable still presents them and must not disappear. + it('keeps the batch group when unavailable items stay presented', async () => { + const { el } = renderItemWithMenu(1); el.setAttribute('data-sortable-lists--item-hide-unavailable-value', 'false'); document.body.appendChild(el); const controller = await mountItemController(el); @@ -1984,8 +2055,283 @@ describe('Sortable lists item controller', () => { controller.moveItemTargetConnected(); await Promise.resolve(); - const divider = el.querySelector('li[data-sortable-lists--item-target="moveDivider"]')!; - expect(divider.hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'batchGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(false); + }); + + it('hides the singular group for a disabled-mode batch with no available action', async () => { + const { el, menu } = renderItemWithMenu(1); + el.setAttribute('data-sortable-lists--item-hide-unavailable-value', 'false'); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor } = stubMenuRoot(el, { isFirst: true, isLast: true }); + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(scope); + root.moveAvailability = () => ({ top: false, up: false, down: false, bottom: false }); + controller.connectRoot(root); + controller.moveItemTargetConnected(); + await Promise.resolve(); + + expect(menu.disableItem).toHaveBeenCalledWith(liFor(el, 'top')); + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(true); + expect(groupFor(el, 'batchGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(true); + }); + + it('keeps the singular group for a one-card batch scope', async () => { + const { el } = renderItemWithMenu(1); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor } = stubMenuRoot(el, { isFirst: false, isLast: false }); + const scope:ActionScope = { kind: 'batch', items: [el] }; + actionScopeFor.mockReturnValue(scope); + controller.connectRoot(root); + controller.moveItemTargetConnected(); + await Promise.resolve(); + + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'batchGroup').hasAttribute('hidden')).toBe(false); + expect(groupFor(el, 'groupDivider').hasAttribute('hidden')).toBe(false); + }); + + // Hiding the group that holds focus would fling focus to the body, and + // Primer closes the whole menu when focus leaves it. + it('parks focus on a presented batch item before hiding its group', async () => { + const { el } = renderItemWithMenu(1); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor } = stubMenuRoot(el, { isFirst: false, isLast: false }); + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(scope); + const singularButton = groupFor(el, 'invokerGroup').querySelector('button')!; + singularButton.setAttribute('role', 'menuitem'); + const batchButton = document.createElement('button'); + batchButton.setAttribute('role', 'menuitem'); + groupFor(el, 'batchGroup').append(batchButton); + singularButton.focus(); + expect(document.activeElement).toBe(singularButton); + + controller.connectRoot(root); + controller.moveItemTargetConnected(); + await Promise.resolve(); + + expect(groupFor(el, 'invokerGroup').hasAttribute('hidden')).toBe(true); + expect(document.activeElement).toBe(batchButton); + }); + + // Primer's open-time focus matches `:not([hidden]) > [role=menuitem]`, + // a check one level deep: the first singular item still matches when only + // its group
      is hidden, and focusing it no-ops, so reopening an + // already-loaded batch menu would strand focus on the invoker. + it('settles reopen focus on the first visible batch item', async () => { + const { el } = renderItemWithMenu(1); + const popover = attachMenuPopover(el); + const invokerButton = document.createElement('button'); + el.querySelector('action-menu')!.append(invokerButton); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor } = stubMenuRoot(el, { isFirst: false, isLast: false }); + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(scope); + const singularItem = groupFor(el, 'invokerGroup').querySelector('button')!; + singularItem.setAttribute('role', 'menuitem'); + const batchButton = document.createElement('button'); + batchButton.setAttribute('role', 'menuitem'); + groupFor(el, 'batchGroup').append(batchButton); + controller.connectRoot(root); + + popover.dispatchEvent(new ToggleEvent('toggle', { newState: 'open', oldState: 'closed' })); + await menuCtx!.nextFrame(); + await menuCtx!.nextFrame(); + popover.dispatchEvent(new ToggleEvent('toggle', { newState: 'closed', oldState: 'open' })); + invokerButton.focus(); + + popover.dispatchEvent(new ToggleEvent('toggle', { newState: 'open', oldState: 'closed' })); + await menuCtx!.nextFrame(); + await menuCtx!.nextFrame(); + + expect(document.activeElement).toBe(batchButton); + }); + + describe('menu renaming', () => { + beforeEach(() => { + window.I18n.store({ + en: { + js: { + backlogs: { + action_menu: { + batch_menu_label: { + one: 'Actions for 1 selected work package', + other: 'Actions for %{count} selected work packages', + }, + }, + }, + }, + }, + }); + }); + + // A Primer IconButton renders a label-type tooltip: the button carries + // NO aria-label — icon_button.rb deletes it and points aria-labelledby + // at the , whose text is both the visible tooltip and the + // accessible name. A fixture with an aria-label would conceal a + // controller that wrote the inert attribute instead. + const attachMenuInvoker = (el:HTMLElement, label = 'Work package actions'):{ invoker:HTMLElement; tooltip:HTMLElement } => { + const actionMenu = el.querySelector('action-menu')!; + const invoker = document.createElement('button'); + const tooltip = document.createElement('tool-tip'); + tooltip.id = `${el.getAttribute('data-sortable-lists--item-id-value')}-menu-tooltip`; + tooltip.textContent = label; + invoker.setAttribute('aria-labelledby', tooltip.id); + actionMenu.append(invoker, tooltip); + Object.assign(actionMenu, { invokerElement: invoker }); + return { invoker, tooltip }; + }; + + async function mountRenamableMenu({ + withKey = true, + withSingularLabel = true, + tooltipText = 'Work package actions', + } = {}) { + const { el } = renderItemWithMenu(1); + if (withKey) { + el.setAttribute( + 'data-sortable-lists--item-batch-menu-label-key-value', + 'js.backlogs.action_menu.batch_menu_label', + ); + } + if (withSingularLabel) { + el.setAttribute( + 'data-sortable-lists--item-singular-menu-label-value', + 'Work package actions', + ); + } + const { invoker, tooltip } = attachMenuInvoker(el, tooltipText); + const popover = attachMenuPopover(el); + document.body.appendChild(el); + const controller = await mountItemController(el); + const { root, actionScopeFor } = stubMenuRoot(el, { isFirst: false, isLast: false }); + + return { + el, invoker, tooltip, popover, controller, root, actionScopeFor, + }; + } + + const openMenu = (popover:HTMLElement) => { + popover.dispatchEvent(new ToggleEvent('beforetoggle', { newState: 'open', oldState: 'closed' })); + popover.dispatchEvent(new ToggleEvent('toggle', { newState: 'open', oldState: 'closed' })); + }; + + it('renames the invoker tooltip while an open menu presents a batch and reverts for singular', async () => { + const { + el, invoker, tooltip, popover, controller, root, actionScopeFor, + } = await mountRenamableMenu(); + const batchScope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(batchScope); + controller.connectRoot(root); + openMenu(popover); + + expect(tooltip.textContent).toBe('Actions for 2 selected work packages'); + expect(invoker.getAttribute('aria-label')).toBeNull(); + + const singularScope:ActionScope = { kind: 'refused', items: [] }; + actionScopeFor.mockReturnValue(singularScope); + controller.moveItemTargetConnected(); + await Promise.resolve(); + + expect(tooltip.textContent).toBe('Work package actions'); + }); + + it('keeps the singular name for a one-card batch scope', async () => { + const { + el, tooltip, popover, controller, root, actionScopeFor, + } = await mountRenamableMenu(); + const scope:ActionScope = { kind: 'batch', items: [el] }; + actionScopeFor.mockReturnValue(scope); + controller.connectRoot(root); + openMenu(popover); + + expect(tooltip.textContent).toBe('Work package actions'); + }); + + it('keeps the singular name for a batch that presents no action', async () => { + const { + el, tooltip, popover, controller, root, actionScopeFor, + } = await mountRenamableMenu(); + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(scope); + root.moveAvailability = () => ({ top: false, up: false, down: false, bottom: false }); + controller.connectRoot(root); + openMenu(popover); + + expect(tooltip.textContent).toBe('Work package actions'); + }); + + it('restores the singular name when the menu closes', async () => { + const { + el, tooltip, popover, controller, root, actionScopeFor, + } = await mountRenamableMenu(); + const batchScope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(batchScope); + controller.connectRoot(root); + openMenu(popover); + expect(tooltip.textContent).toBe('Actions for 2 selected work packages'); + + popover.dispatchEvent(new ToggleEvent('toggle', { newState: 'closed', oldState: 'open' })); + + expect(tooltip.textContent).toBe('Work package actions'); + }); + + // A morph can reconnect targets while the popover is closed; the closed + // menu must keep presenting the singular name no matter the scope. + it('does not apply the batch name while the menu is closed', async () => { + const { + el, tooltip, controller, root, actionScopeFor, + } = await mountRenamableMenu(); + const batchScope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(batchScope); + controller.connectRoot(root); + controller.moveItemTargetConnected(); + await Promise.resolve(); + + expect(tooltip.textContent).toBe('Work package actions'); + }); + + // A Turbo back/forward restore resurrects the DOM as it was snapshotted, + // batch tooltip text included; the fresh controller must present the + // server-rendered singular name, not adopt the snapshot's text. + it('recovers the singular name from a snapshot carrying the batch one', async () => { + const { tooltip } = await mountRenamableMenu({ + tooltipText: 'Actions for 2 selected work packages', + }); + + expect(tooltip.textContent).toBe('Work package actions'); + }); + + it('leaves the name alone when no label key is configured', async () => { + const { + el, tooltip, popover, controller, root, actionScopeFor, + } = await mountRenamableMenu({ withKey: false }); + const scope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(scope); + controller.connectRoot(root); + openMenu(popover); + + expect(tooltip.textContent).toBe('Work package actions'); + }); + + it('leaves the name alone when no singular name is provided', async () => { + const { + el, tooltip, popover, controller, root, actionScopeFor, + } = await mountRenamableMenu({ withSingularLabel: false }); + const scope:ActionScope = { kind: 'batch', items: [el] }; + actionScopeFor.mockReturnValue(scope); + controller.connectRoot(root); + openMenu(popover); + + expect(tooltip.textContent).toBe('Work package actions'); + }); }); it('delegates an enabled click to the root and no-ops a disabled one', async () => { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index 93d1eb54f280..2e3df08e9671 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -68,7 +68,18 @@ import { refreshMenuAvailability } from './menu-availability'; type CleanupFn = () => void; export default class ItemController extends Controller implements RootAwareChild { - static targets = ['handle', 'preview', 'destinationItem', 'moveItem', 'moveMenu', 'moveDivider', 'focus']; + static targets = [ + 'handle', + 'preview', + 'destinationItem', + 'moveItem', + 'moveMenu', + 'invokerGroup', + 'batchGroup', + 'groupDivider', + 'focus', + ]; + static elements = { menu: 'action-menu' }; static values = { @@ -76,6 +87,13 @@ export default class ItemController extends Controller implements R type: String, externalUrl: String, hideUnavailable: { type: Boolean, default: true }, + label: String, + // I18n key (with count plurals) naming the menu while it is batch-scoped. + batchMenuLabelKey: String, + // The menu's singular name, server-rendered: the invoker tooltip's own + // text is no restore source, since a Turbo snapshot can capture it + // mid-rename. Renaming only engages when both label values are present. + singularMenuLabel: String, // See ItemMobility in list-dom. A `confined` item is still a full drag // source; only the lists the batch's permitted set names accept it. mobility: { type: String, default: 'free' }, @@ -88,6 +106,12 @@ export default class ItemController extends Controller implements R declare readonly externalUrlValue:string; declare readonly hasExternalUrlValue:boolean; declare readonly hideUnavailableValue:boolean; + declare readonly labelValue:string; + declare readonly hasLabelValue:boolean; + declare readonly batchMenuLabelKeyValue:string; + declare readonly hasBatchMenuLabelKeyValue:boolean; + declare readonly singularMenuLabelValue:string; + declare readonly hasSingularMenuLabelValue:boolean; declare readonly handleTarget:HTMLElement; declare readonly hasHandleTarget:boolean; @@ -97,8 +121,12 @@ export default class ItemController extends Controller implements R declare readonly moveItemTargets:HTMLElement[]; declare readonly moveMenuTarget:HTMLElement; declare readonly hasMoveMenuTarget:boolean; - declare readonly moveDividerTarget:HTMLElement; - declare readonly hasMoveDividerTarget:boolean; + declare readonly invokerGroupTarget:HTMLElement; + declare readonly hasInvokerGroupTarget:boolean; + declare readonly batchGroupTarget:HTMLElement; + declare readonly hasBatchGroupTarget:boolean; + declare readonly groupDividerTarget:HTMLElement; + declare readonly hasGroupDividerTarget:boolean; declare readonly focusTarget:HTMLElement; declare readonly hasFocusTarget:boolean; @@ -112,6 +140,7 @@ export default class ItemController extends Controller implements R private dropIndicatorElement?:HTMLElement; private root?:SortableListsRoot; private refreshToken?:object; + private menuOpen = false; private readonly onMenuToggle = (event:Event):void => { // The toggle event does not bubble, so listen in capture phase; recompute @@ -119,10 +148,18 @@ export default class ItemController extends Controller implements R // shifted siblings meanwhile. Read newState by duck typing rather than // `instanceof ToggleEvent` so a browser without the ToggleEvent global // cannot throw. - if (this.hasMenuElement - && event.target === this.menuElement.popoverElement - && (event as ToggleEvent).newState === 'open') { + if (!this.hasMenuElement || event.target !== this.menuElement.popoverElement) { + return; + } + + const { newState } = event as ToggleEvent; + if (newState === 'open') { + this.menuOpen = true; this.refreshActionAvailability(); + this.rescueOpenFocus(); + } else if (newState === 'closed') { + this.menuOpen = false; + this.refreshMenuLabel(false, 0); } }; @@ -139,6 +176,7 @@ export default class ItemController extends Controller implements R && this.hasMenuElement && event.target === this.menuElement.popoverElement ) { + this.menuOpen = true; this.prepareActionMenu(); } }; @@ -149,6 +187,9 @@ export default class ItemController extends Controller implements R this.element.addEventListener('contextual-action-menu:beforeOpen', this.onContextualBeforeOpen); this.element.addEventListener('beforetoggle', this.onMenuBeforeToggle, true); this.element.addEventListener('toggle', this.onMenuToggle, true); + // A Turbo restore resurrects the tooltip as snapshotted, batch name and + // all; a closed menu always presents the singular one. + this.refreshMenuLabel(false, 0); } disconnect():void { @@ -550,19 +591,128 @@ export default class ItemController extends Controller implements R return; } - refreshMenuAvailability({ + const scope = preparedScope ?? root.actionScopeFor(this.element); + const presented = refreshMenuAvailability({ menu: this.menuElement, - scope: preparedScope ?? root.actionScopeFor(this.element), + scope, itemOrderable: isOrderableItem(this.element), destinationItems: this.destinationItemTargets, moveItems: this.moveItemTargets, moveMenu: this.hasMoveMenuTarget ? this.moveMenuTarget : null, - divider: this.hasMoveDividerTarget ? this.moveDividerTarget : null, + divider: null, hideUnavailable: this.hideUnavailableValue, identifier: this.identifier, availableDestinations: (scope, candidates) => root.availableDestinations(scope, candidates), moveAvailability: () => root.moveAvailability(this.element), }); + this.refreshActionGroups(scope, presented); + } + + private refreshActionGroups(scope:ActionScope, presentedBatchActionCount:number):void { + const batch = scope.kind === 'batch' && scope.items.length > 1; + const batchActionsPresented = presentedBatchActionCount > 0; + + if (this.hasInvokerGroupTarget) { + if (batch && batchActionsPresented) { + this.rescueFocusFrom(this.invokerGroupTarget); + } + this.hideGroup(this.invokerGroupTarget, batch && batchActionsPresented); + } + + if (this.hasBatchGroupTarget) { + if (!batchActionsPresented) { + this.rescueFocusFrom(this.batchGroupTarget); + } + this.hideGroup(this.batchGroupTarget, !batchActionsPresented); + } + + // The divider only separates anything while both groups show. It never + // goes through setAvailability: `disableItem` writes to the item's + // `.ActionListContent`, which a divider does not have. + if (this.hasGroupDividerTarget) { + this.groupDividerTarget.toggleAttribute( + 'hidden', + (batch && batchActionsPresented) || !batchActionsPresented, + ); + } + + this.refreshMenuLabel(batch && batchActionsPresented, scope.items.length); + } + + // Primer's focus zone stops managing exactly the element `hidden` lands on, + // so hiding a group alone leaves its items in the zone for the arrows to + // step onto. A group that comes back is repopulated by the availability + // pass, which sets each item's own hidden state before this runs. + private hideGroup(group:HTMLElement, hidden:boolean):void { + group.toggleAttribute('hidden', hidden); + + if (!hidden) { + return; + } + + for (const item of group.querySelectorAll('[role="menuitem"]')) { + item.toggleAttribute('hidden', true); + } + } + + // Hiding the group that holds focus would fling focus to the body, and + // Primer closes the whole menu when focus leaves it. Park focus on the + // first menu item that stays presented before the group disappears. + private rescueFocusFrom(group:HTMLElement):void { + const active = this.element.ownerDocument.activeElement; + if (!(active instanceof HTMLElement) || !group.contains(active) || !this.hasMenuElement) { + return; + } + + const fallback = Array.from(this.menuElement.querySelectorAll('[role="menuitem"]')) + .find((item) => !group.contains(item) && !item.closest('[hidden]')); + fallback?.focus(); + } + + // The invoker is a Primer IconButton whose label-type tooltip owns the + // accessible name — icon_button.rb drops the button's aria-label and points + // aria-labelledby at the — so writing aria-label would be inert. + // The batch name shows only while the menu is open; any projection with the + // popover closed settles back on the server-rendered singular name. + private refreshMenuLabel(batch:boolean, count:number):void { + const invoker = this.hasMenuElement ? this.menuElement.invokerElement : null; + const labelId = invoker?.getAttribute('aria-labelledby'); + const tooltip = labelId ? this.element.ownerDocument.getElementById(labelId) : null; + if (!tooltip || !this.hasBatchMenuLabelKeyValue || !this.hasSingularMenuLabelValue) { + return; + } + + tooltip.textContent = batch && this.menuOpen + ? I18n.t(this.batchMenuLabelKeyValue, { count }) + : this.singularMenuLabelValue; + } + + // Primer's open-time focus matches `:not([hidden]) > [role=menuitem]`, one + // level deep: a singular item whose group alone is hidden still matches, + // and focusing it no-ops. Primer schedules that focus a frame after the + // toggle, so the correction runs a frame later still. + private rescueOpenFocus():void { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + if (!this.menuOpen || !this.hasMenuElement) { + return; + } + + const active = this.element.ownerDocument.activeElement; + if ( + active instanceof HTMLElement + && this.menuElement.contains(active) + && active.getAttribute('role') === 'menuitem' + && !active.closest('[hidden]') + ) { + return; + } + + const fallback = Array.from(this.menuElement.querySelectorAll('[role="menuitem"]')) + .find((item) => !item.closest('[hidden]')); + fallback?.focus(); + }); + }); } private menuItemActionable(item:EventTarget|null):item is HTMLElement { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts index fd56f9ad83d3..7f9f2d95de55 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts @@ -45,7 +45,7 @@ interface MenuAvailabilityInput { moveAvailability:() => ReturnType; } -export function refreshMenuAvailability(input:MenuAvailabilityInput):void { +export function refreshMenuAvailability(input:MenuAvailabilityInput):number { const setAvailability = (item:HTMLElement, available:boolean):void => { if (input.hideUnavailable) { input.menu[available ? 'showItem' : 'hideItem'](item); @@ -74,15 +74,23 @@ export function refreshMenuAvailability(input:MenuAvailabilityInput):void { if (input.moveMenu) setAvailability(input.moveMenu, available > 0); } + const presented = (items:HTMLElement[]):number => input.hideUnavailable + ? items.filter((item) => !item.hasAttribute('hidden')).length + : items.length; + const presentedActionCount = presented(input.destinationItems) + + presented(input.moveMenu ? [input.moveMenu] : input.moveItems); + if (input.divider && input.hideUnavailable) { let sibling = input.divider.nextElementSibling; while (sibling) { if (sibling instanceof HTMLLIElement && !sibling.hasAttribute('hidden')) { input.divider.removeAttribute('hidden'); - return; + return presentedActionCount; } sibling = sibling.nextElementSibling; } input.divider.setAttribute('hidden', 'hidden'); } + + return presentedActionCount; } diff --git a/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb b/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb index 02e53d4c5e3f..2d9ea1bf5143 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb +++ b/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb @@ -110,6 +110,12 @@ def row_data sortable_lists__item_label_value: work_package.to_fs(:caption), sortable_lists__item_type_value: "work_package", sortable_lists__item_mobility_value: mobility, + sortable_lists__item_batch_menu_label_key_value: "js.backlogs.action_menu.batch_menu_label", + # The same string the card menu's invoker tooltip renders; the + # controller restores it from here rather than trusting the tooltip + # text, which a Turbo snapshot can capture mid-rename. + sortable_lists__item_singular_menu_label_value: + I18n.t(:"open_project.common.work_package_card_component.menu.label_actions"), # Native drag payload for external consumers; the same absolute URL # as the card menu's "Copy URL to clipboard" item. The label above # doubles as the link text of the text/html flavour. diff --git a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb index e394d43009cc..66051aad8c22 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb +++ b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.html.erb @@ -29,109 +29,118 @@ See COPYRIGHT and LICENSE files for more details. <%= render(Primer::Alpha::ActionMenu::List.new(menu_id:)) do |list| - list.with_item( - id: dom_target(work_package, :menu, :open_details), - tag: :a, - label: t(:"js.button_open_details"), - href: project_backlogs_backlog_details_path(project, work_package), - content_arguments: { data: { action: "backlogs--work-package#openSplitPane:prevent" } } - ) do |item| - item.with_leading_visual_icon(icon: :"op-view-split") - end + list.with_group(data: { sortable_lists__item_target: "invokerGroup" }) do |group| + group.with_item( + id: dom_target(work_package, :menu, :open_details), + label: t(:"js.button_open_details"), + content_arguments: { + tag: :a, + href: project_backlogs_backlog_details_path(project, work_package), + tabindex: -1, + data: { action: "backlogs--work-package#openSplitPane:prevent" } + } + ) do |item| + item.with_leading_visual_icon(icon: :"op-view-split") + end - list.with_item( - id: dom_target(work_package, :menu, :open_fullscreen), - tag: :a, - label: t(:"js.button_open_fullscreen"), - href: work_package_path(work_package), - content_arguments: { data: { turbo_frame: "_top" } } - ) do |item| - item.with_leading_visual_icon(icon: :"screen-full") - end + group.with_item( + id: dom_target(work_package, :menu, :open_fullscreen), + label: t(:"js.button_open_fullscreen"), + content_arguments: { + tag: :a, + href: work_package_path(work_package), + tabindex: -1, + data: { turbo_frame: "_top" } + } + ) do |item| + item.with_leading_visual_icon(icon: :"screen-full") + end - list.with_item( - id: dom_target(work_package, :menu, :copy_url_to_clipboard), - tag: :"clipboard-copy", - label: t(".action_menu.copy_url_to_clipboard"), - content_arguments: { value: work_package_url(work_package) } - ) do |item| - item.with_leading_visual_icon(icon: :copy) - end + group.with_item( + id: dom_target(work_package, :menu, :copy_url_to_clipboard), + label: t(".action_menu.copy_url_to_clipboard"), + content_arguments: { tag: :"clipboard-copy", value: work_package_url(work_package), tabindex: -1 } + ) do |item| + item.with_leading_visual_icon(icon: :copy) + end - list.with_item( - id: dom_target(work_package, :menu, :copy_work_package_id), - tag: :"clipboard-copy", - label: t(".action_menu.copy_work_package_id"), - content_arguments: { value: work_package.display_id } - ) do |item| - item.with_leading_visual_icon(icon: :hash) + group.with_item( + id: dom_target(work_package, :menu, :copy_work_package_id), + label: t(".action_menu.copy_work_package_id"), + content_arguments: { tag: :"clipboard-copy", value: work_package.display_id, tabindex: -1 } + ) do |item| + item.with_leading_visual_icon(icon: :hash) + end end if show_move_submenu? - list.with_divider(data: { sortable_lists__item_target: "moveDivider" }) - - if show_move_to_inbox? - list.with_item( - id: dom_target(work_package, :menu, :move_to_inbox), - label: t(".action_menu.move_to_inbox"), - tag: :button, - data: destination_data(inbox_list_type, []).merge( - action: "click->sortable-lists--item#moveToDestination:prevent" - ) - ) do |item| - item.with_leading_visual_icon(icon: :inbox) + list.with_divider(data: { sortable_lists__item_target: "groupDivider" }) + + list.with_group(data: { sortable_lists__item_target: "batchGroup" }) do |group| + if show_move_to_inbox? + group.with_item( + id: dom_target(work_package, :menu, :move_to_inbox), + label: t(".action_menu.move_to_inbox"), + data: destination_data(inbox_list_type, []).merge( + action: "click->sortable-lists--item#moveToDestination:prevent" + ), + content_arguments: { tabindex: -1 } + ) do |item| + item.with_leading_visual_icon(icon: :inbox) + end end - end - if show_move_to_backlog_bucket? - list.with_item( - id: dom_target(work_package, :menu, :move_to_backlog_bucket), - label: t(".action_menu.move_to_backlog_bucket"), - tag: :button, - href: move_to_bucket_dialog_project_backlogs_work_packages_path(project), - form_arguments: { method: :post }, - content_arguments: { - data: { - controller: "async-dialog", - action: "async-dialog:beforeLoad->sortable-lists--item#prepareDialog" - } - }, - data: destination_data(bucket_list_type, bucket_ids) - ) do |item| - item.with_leading_visual_icon(icon: :package) + if show_move_to_backlog_bucket? + group.with_item( + id: dom_target(work_package, :menu, :move_to_backlog_bucket), + label: t(".action_menu.move_to_backlog_bucket"), + href: move_to_bucket_dialog_project_backlogs_work_packages_path(project), + form_arguments: { method: :post }, + content_arguments: { + tabindex: -1, + data: { + controller: "async-dialog", + action: "async-dialog:beforeLoad->sortable-lists--item#prepareDialog" + } + }, + data: destination_data(bucket_list_type, bucket_ids) + ) do |item| + item.with_leading_visual_icon(icon: :package) + end end - end - if show_move_to_sprint? - list.with_item( - id: dom_target(work_package, :menu, :move_to_sprint), - label: t(".action_menu.move_to_sprint"), - tag: :button, - href: move_to_sprint_dialog_project_backlogs_work_packages_path(project), - form_arguments: { method: :post }, - content_arguments: { - data: { - controller: "async-dialog", - action: "async-dialog:beforeLoad->sortable-lists--item#prepareDialog" - } - }, - data: destination_data(sprint_list_type, sprint_ids) - ) do |item| - item.with_leading_visual_icon(icon: :zap) + if show_move_to_sprint? + group.with_item( + id: dom_target(work_package, :menu, :move_to_sprint), + label: t(".action_menu.move_to_sprint"), + href: move_to_sprint_dialog_project_backlogs_work_packages_path(project), + form_arguments: { method: :post }, + content_arguments: { + tabindex: -1, + data: { + controller: "async-dialog", + action: "async-dialog:beforeLoad->sortable-lists--item#prepareDialog" + } + }, + data: destination_data(sprint_list_type, sprint_ids) + ) do |item| + item.with_leading_visual_icon(icon: :zap) + end end - end - if show_move_items? - list.with_item( - component_klass: Primer::Alpha::ActionMenu::SubMenuItem, - label: t(".action_menu.move_menu"), - select_variant: :none, - form_arguments: {}, - data: { sortable_lists__item_target: "moveMenu" } - ) do |move_menu| - move_menu.with_leading_visual_icon(icon: :"op-arrow-in") - - move_menu_items.render_into(move_menu) + if show_move_items? + group.with_item( + component_klass: Primer::Alpha::ActionMenu::SubMenuItem, + label: t(".action_menu.move_menu"), + select_variant: :none, + form_arguments: {}, + content_arguments: { tabindex: -1 }, + data: { sortable_lists__item_target: "moveMenu" } + ) do |move_menu| + move_menu.with_leading_visual_icon(icon: :"op-arrow-in") + + move_menu_items.render_into(move_menu) + end end end end diff --git a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb index e272a87263d8..10f90339be43 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb +++ b/modules/backlogs/app/components/backlogs/work_package_card_menu_component.rb @@ -54,9 +54,6 @@ def menu_id private - # Positional moves reorder within the card's own list, which the server - # allows even for a read-only work package, so they gate on the page-level - # permission alone. Only the cross-container moves below require movable?. def show_move_items? sortable? end diff --git a/modules/backlogs/config/locales/js-en.yml b/modules/backlogs/config/locales/js-en.yml index 48973394f3b6..f25f113c4e1c 100644 --- a/modules/backlogs/config/locales/js-en.yml +++ b/modules/backlogs/config/locales/js-en.yml @@ -30,6 +30,12 @@ en: js: backlogs: + action_menu: + # A complete standalone string per plural form, never the singular + # name plus a suffix. + batch_menu_label: + one: "Actions for 1 selected work package" + other: "Actions for %{count} selected work packages" announcements: batch_too_large: "Cannot move %{count} work packages at once. Select no more than %{max}." fallback_item_label: "Work package" diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb index 6c97cf845e0f..497910247717 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb @@ -82,7 +82,9 @@ sortable_lists__item_id_value: work_package.id, sortable_lists__item_type_value: "work_package", sortable_lists__item_label_value: work_package.to_fs(:caption), - sortable_lists__item_mobility_value: "free" + sortable_lists__item_mobility_value: "free", + sortable_lists__item_batch_menu_label_key_value: "js.backlogs.action_menu.batch_menu_label", + sortable_lists__item_singular_menu_label_value: "Work package actions" ) expect(item.row_args[:draggable]).to be(true) expect(item.row_args).not_to include(:tabindex) diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb index 9836ab41cbc8..6900f1dbf9ad 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_menu_component_spec.rb @@ -169,16 +169,32 @@ def render_component(work_package: self.work_package, end end - it "shows a divider before the Move submenu" do + it "shows a divider between the singular and batch-capable actions" do render_component - expect(page).to have_css(".ActionList-sectionDivider") + expect(page).to have_css(".ActionList-sectionDivider[data-sortable-lists--item-target='groupDivider']") end - it "wires the divider up to the item controller, so it can be hidden with the group" do + it "separates the invoker's own actions from the batch-capable ones", :aggregate_failures do render_component - expect(page).to have_css(".ActionList-sectionDivider[data-sortable-lists--item-target='moveDivider']") + invoker_group = page.find("ul[role='group'][data-sortable-lists--item-target='invokerGroup']") + expect(invoker_group).to have_css("a[id$='_menu_open_details']") + expect(invoker_group).to have_css("a[id$='_menu_open_fullscreen']") + expect(invoker_group).to have_css("clipboard-copy[id$='_menu_copy_url_to_clipboard']") + expect(invoker_group).to have_css("clipboard-copy[id$='_menu_copy_work_package_id']") + + batch_group = page.find("ul[role='group'][data-sortable-lists--item-target='batchGroup']") + expect(batch_group).to have_css("li[data-sortable-lists--item-target~='destinationItem']") + expect(batch_group).to have_css("li[data-sortable-lists--item-target='moveMenu']") + expect(batch_group).to have_no_css("a[id$='_menu_open_details']") + end + + it "renders both groups without headings", :aggregate_failures do + render_component + + expect(page).to have_no_css("ul[role='group'][aria-labelledby]") + expect(page).to have_no_css("ul[role='group'] h3") end it "shows the Move to position submenu with incoming-arrow icon" do diff --git a/modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb b/modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb index 12ef5f3f09c7..8640ad57ac8e 100644 --- a/modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb +++ b/modules/backlogs/spec/features/work_packages/batch_action_menu_spec.rb @@ -56,18 +56,27 @@ %i[more right_click context_menu_key shift_f10].each do |entry_point| context "via #{entry_point.to_s.tr('_', ' ')}" do - it "preserves a selected batch across its menu lifecycle" do + it "preserves a selected batch and presents only its batch menu" do backlogs_page.select_cards(stories[1], stories[0]) menu = backlogs_page.open_card_menu(stories[1], via: entry_point) backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) - expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details"), focused: true) - backlogs_page.expect_card_menu_anchor(stories[1], via: entry_point) backlogs_page.expect_card_menu_loaded(stories[1]) + backlogs_page.expect_action_menu_groups(invoker: stories[1], selected_count: 2) + expect(menu).to have_no_selector(:menuitem, text: I18n.t(:"js.button_open_details")) + expect(menu).to have_selector(:menuitem, text: "Move to backlog inbox", focused: true) + backlogs_page.expect_card_menu_anchor(stories[1], via: entry_point) backlogs_page.dismiss_card_menu(stories[1], via: entry_point) backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) + backlogs_page.expect_menu_invoker_name(stories[1], "Work package actions") + + # Reopening skips the deferred-fragment load, so focus must be + # corrected past Primer's open-time pick of the hidden singular item. + reopened_menu = backlogs_page.open_card_menu(stories[1], via: entry_point) + expect(reopened_menu).to have_selector(:menuitem, text: "Move to backlog inbox", focused: true) + backlogs_page.dismiss_card_menu(stories[1], via: entry_point) end it "replaces an old batch when an unselected card invokes the one-card menu" do @@ -77,6 +86,7 @@ backlogs_page.expect_selected_cards_in_order(stories[2]) backlogs_page.expect_card_menu_loaded(stories[2]) + backlogs_page.expect_action_menu_groups(invoker: stories[2], selected_count: 1) backlogs_page.dismiss_card_menu(stories[2], via: entry_point) backlogs_page.expect_selected_cards_in_order(stories[2]) @@ -94,9 +104,7 @@ backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) end - it "keeps every singular action bound to its invoking card without collapsing the batch", :aggregate_failures do - backlogs_page.select_cards(stories[0], stories[1]) - + it "keeps every singular action bound to its invoking card", :aggregate_failures do stories.first(2).each do |invoker| menu = backlogs_page.open_card_menu(invoker, via: :more) @@ -108,7 +116,6 @@ .to end_with("/work_packages/#{invoker.id}") expect(menu.find(:menuitem, text: "Copy work package ID")[:value]).to eq(invoker.id.to_s) - backlogs_page.expect_selected_cards_in_order(stories[0], stories[1]) backlogs_page.dismiss_card_menu(invoker, via: :more) end end @@ -149,6 +156,7 @@ backlogs_page.open_card_menu(inbox_stories[0], via: :right_click) + backlogs_page.expect_action_menu_groups(invoker: inbox_stories[0], selected_count: 2) backlogs_page.expect_open_menu_actions( invoker: inbox_stories[0], present: ["Move to position"], @@ -163,6 +171,7 @@ backlogs_page.open_card_menu(stories[0], via: :more) + backlogs_page.expect_action_menu_groups(invoker: stories[0], selected_count: 2) backlogs_page.expect_open_menu_actions( invoker: stories[0], present: ["Move to sprint"], diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index 85edc6c78e90..755714b6c20a 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -339,10 +339,8 @@ def within_work_package_menu(work_package, &) dismiss_menu(work_package) end - # Opens one card's menu through the same four user entry points supported - # by Backlogs. Keeping the native key/pointer synthesis and deferred-menu - # wait here lets feature specs talk in product language and prevents each - # caller from encoding Primer's overlay structure. + # The four user entry points Backlogs supports, with the native key and + # pointer synthesis and the deferred-menu wait kept out of the callers. def open_card_menu(work_package, via:) case via when :more @@ -372,6 +370,34 @@ def open_card_menu(work_package, via:) work_package_action_menu(work_package) end + def expect_action_menu_groups(invoker:, selected_count:) + # The menu list arrives via a deferred fragment; a node captured before + # the swap goes stale, so settle the load before scoping into the menu. + expect_card_menu_loaded(invoker) + menu = work_package_action_menu(invoker) + singular_group = "ul[role='group'][data-sortable-lists--item-target='invokerGroup']" + batch_group = "ul[role='group'][data-sortable-lists--item-target='batchGroup']" + + if selected_count > 1 + expect(menu).to have_css("#{singular_group}[hidden]", visible: :all) + expect(menu).to have_css(batch_group) + expect_menu_invoker_name( + invoker, + I18n.t("js.backlogs.action_menu.batch_menu_label", count: selected_count) + ) + else + expect(menu).to have_css(singular_group) + expect(menu).to have_css(batch_group) + expect_menu_invoker_name(invoker, "Work package actions") + end + end + + def expect_menu_invoker_name(work_package, name) + within_work_package(work_package) do + expect(page).to have_button(accessible_name: name) + end + end + def activate_singular_menu_action(invoker:, action:, via: :more) open_card_menu(invoker, via:).find(:menuitem, text: action, exact_text: true).click end @@ -432,6 +458,8 @@ def expect_open_menu_actions(invoker:, present:, absent:) def expect_fixed_action_menu(invoker:) menu = work_package_action_menu(invoker) + expect(menu).to have_css("ul[role='group'][data-sortable-lists--item-target='invokerGroup']") + expect(menu).to have_no_css("[data-sortable-lists--item-target='batchGroup']") expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details")) expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_fullscreen")) expect(menu).to have_no_selector(:menuitem, text: "Move to position") @@ -1824,22 +1852,33 @@ def open_move_submenu(menu) page.find(:menu, id: move_item["aria-controls"]) end + # Dismisses with Escape rather than an outside click: the compact batch + # menu can leave a live menuitem under the overlay's centre point, so a + # click there activates an action instead of closing the menu. def dismiss_menu(menu_owner) selector = menu_owner_overlay_selector(menu_owner) return unless page.has_css?(selector, visible: true, wait: 0) return if page.has_selector?(:modal, wait: 0) - find(selector, wait: 0).click - rescue Capybara::ElementNotFound, - Ferrum::CoordinatesNotFoundError, - Selenium::WebDriver::Error::ElementNotInteractableError - # Menu actions can close the menu or open a modal between the checks - # above and the click; once the menu is gone or a modal owns focus, - # the overlay is unclickable (Cuprite raises CoordinatesNotFoundError, - # Selenium ElementNotInteractableError) and dismissal is moot. - raise unless page.has_selector?(:modal, wait: 0) || - page.has_no_css?(selector, visible: true, wait: 0) + press_escape + # Escape closes only the top-most popover; an open submenu absorbs the + # first press and the menu itself needs a second one. + press_escape if page.has_css?(selector, visible: true, wait: 0) + + expect(page).to have_no_css(selector, visible: :visible) + end + + # Neither driver may deliver the key through the focused element: + # Selenium's Session#send_keys refuses once a closing submenu parked focus + # on a hidden item, and Cuprite's clicks its receiver first. Both paths + # below dispatch the key without touching any element. + def press_escape + if using_cuprite? + page.driver.browser.keyboard.type(:escape) + else + send_keys_to_focused_element(:escape) + end end def headed_section_titles(id_prefix:) From 14b7b040e881186087a761f9d4fdce1943406c60 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Mon, 17 Aug 2026 01:31:56 +0100 Subject: [PATCH 06/10] [AGILE-364] Preserve direct-navigation selection Keeps the inherited selection-persistence scenario on direct card navigation now that opening an unselected action menu intentionally settles a new one-card action scope. https://community.openproject.org/wp/AGILE-364 --- .../spec/features/work_packages/batch_selection_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb index b03959102b89..c58245942443 100644 --- a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb +++ b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb @@ -218,7 +218,8 @@ expect(page).to have_css("[data-batch-selected]", count: 2) - backlogs_page.open_work_package_details(story3) + backlogs_page.send_work_package_card_keys(story3, [:enter]) + backlogs_page.expect_details_view(story3) expect(page).to have_css("[data-batch-selected]", count: 2) expect(backlogs_page.selected_card_ids).to contain_exactly(story1.id.to_s, story2.id.to_s) From 26e3d0b64b8c422f84a6b4dc4602159ba218b3c1 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 19 Aug 2026 17:56:34 +0100 Subject: [PATCH 07/10] [AGILE-364] Extract the action menu projection Primer's toggle lifecycle, group and divider visibility, menu naming and open-time focus repair had grown into a second state machine inside the controller that owns sortable behaviour. A plain collaborator wrapping the ActionMenu element keeps each of them answerable on its own. Naming moves with it, and gains the menu itself: Primer labels the list by its invoker button, whose own name comes from a further aria-labelledby, and those references do not chain. https://community.openproject.org/wp/AGILE-364 --- ...ailability.spec.ts => action-menu.spec.ts} | 56 ++-- .../dynamic/sortable-lists/action-menu.ts | 313 ++++++++++++++++++ .../sortable-lists/item.controller.spec.ts | 52 ++- .../dynamic/sortable-lists/item.controller.ts | 211 +++++------- .../sortable-lists/menu-availability.ts | 96 ------ 5 files changed, 474 insertions(+), 254 deletions(-) rename frontend/src/stimulus/controllers/dynamic/sortable-lists/{menu-availability.spec.ts => action-menu.spec.ts} (57%) create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts delete mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.spec.ts similarity index 57% rename from frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.spec.ts rename to frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.spec.ts index 6eb8406ee747..df4b07d3889a 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.spec.ts @@ -26,62 +26,58 @@ // See COPYRIGHT and LICENSE files for more details. //++ -import { refreshMenuAvailability } from './menu-availability'; +import type { ActionMenuElement } from '@openproject/primer-view-components/app/components/primer/alpha/action_menu/action_menu_element'; +import { SortableActionMenu } from './action-menu'; function fixture(hideUnavailable = true) { const list = document.createElement('ul'); - list.innerHTML = '
    • '; - const [divider, destination, moveMenu] = Array.from(list.querySelectorAll('li')); + list.innerHTML = '
    • '; + const [groupDivider, destination, moveSubmenu] = Array.from(list.querySelectorAll('li')); const moveItem = document.createElement('li'); - moveItem.setAttribute('data-sortable-lists-item-direction-param', 'top'); - moveMenu.append(moveItem); + moveSubmenu.append(moveItem); const menu = { showItem: (item:HTMLElement) => item.removeAttribute('hidden'), hideItem: (item:HTMLElement) => item.setAttribute('hidden', ''), enableItem: (item:HTMLElement) => item.removeAttribute('aria-disabled'), disableItem: (item:HTMLElement) => item.setAttribute('aria-disabled', 'true'), - }; - const input:Parameters[0] = { - menu, - scope: { kind: 'batch', items: [document.createElement('div'), document.createElement('div')] }, - itemOrderable: true, + } as ActionMenuElement; + const projection = new SortableActionMenu(menu, hideUnavailable, null); + const elements = { destinationItems: [destination], moveItems: [moveItem], - moveMenu, - divider, - hideUnavailable, - identifier: 'sortable-lists-item', - availableDestinations: vi.fn(() => []), - moveAvailability: vi.fn(() => ({ top: false, up: false, down: false, bottom: false })), + moveSubmenu, + groupDivider, + invokerGroup: null, + batchGroup: null, }; - return { input, divider, destination, moveMenu, moveItem }; + const scope = { batch: false, count: 1 }; + const availability = { destinationItem: () => false, moveItem: () => false }; + return { projection, elements, scope, availability, destination, moveSubmenu, moveItem, groupDivider }; } describe('menu availability projection', () => { it('ignores the trailing overlay when every real action is hidden', () => { - const { input, divider, destination, moveMenu, moveItem } = fixture(); - refreshMenuAvailability(input); - for (const element of [divider, destination, moveMenu, moveItem]) { + const { projection, elements, scope, availability, groupDivider, destination, moveSubmenu, moveItem } = fixture(); + projection.project(elements, scope, availability); + for (const element of [groupDivider, destination, moveSubmenu, moveItem]) { expect(element.hasAttribute('hidden')).toBe(true); } - expect(input.moveAvailability).toHaveBeenCalled(); }); it('restores the position group when the batch becomes positionable', () => { - const { input, divider, moveMenu, moveItem } = fixture(); - refreshMenuAvailability(input); - input.moveAvailability = vi.fn(() => ({ top: true, up: true, down: false, bottom: false })); - refreshMenuAvailability(input); - for (const element of [divider, moveMenu, moveItem]) { + const { projection, elements, scope, availability, groupDivider, moveSubmenu, moveItem } = fixture(); + projection.project(elements, scope, availability); + projection.project(elements, scope, { ...availability, moveItem: () => true }); + for (const element of [groupDivider, moveSubmenu, moveItem]) { expect(element.hasAttribute('hidden')).toBe(false); } }); it('disables unavailable actions without hiding the group separator', () => { - const { input, divider, destination, moveMenu, moveItem } = fixture(false); - refreshMenuAvailability(input); - expect(divider.hasAttribute('hidden')).toBe(false); - for (const element of [destination, moveMenu, moveItem]) { + const { projection, elements, scope, availability, groupDivider, destination, moveSubmenu, moveItem } = fixture(false); + projection.project(elements, scope, availability); + expect(groupDivider.hasAttribute('hidden')).toBe(false); + for (const element of [destination, moveSubmenu, moveItem]) { expect(element.getAttribute('aria-disabled')).toBe('true'); expect(element.hasAttribute('hidden')).toBe(false); } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts new file mode 100644 index 000000000000..1d28d4a71a54 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts @@ -0,0 +1,313 @@ +//-- copyright +// OpenProject is an open source project management software. +// Copyright (C) the OpenProject GmbH +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License version 3. +// +// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +// Copyright (C) 2006-2013 Jean-Philippe Lang +// Copyright (C) 2010-2013 the ChiliProject Team +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License +// as published by the Free Software Foundation; either version 2 +// of the License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program; if not, write to the Free Software +// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import type { ActionMenuElement } from '@openproject/primer-view-components/app/components/primer/alpha/action_menu/action_menu_element'; + +export interface ActionMenuLabels { + singular:string; + batchKey:string; +} + +// The menu's own elements, re-read by the caller on every projection so a +// morph that replaces the subtree is picked up without rebuilding anything. +export interface ActionMenuElements { + destinationItems:HTMLElement[]; + moveItems:HTMLElement[]; + moveSubmenu:HTMLElement|null; + invokerGroup:HTMLElement|null; + batchGroup:HTMLElement|null; + groupDivider:HTMLElement|null; +} + +export interface ActionMenuScope { + batch:boolean; + count:number; +} + +// Answers, for one menu item, whether the action it invokes can run in the +// current scope. A null `moveItem` leaves the move actions untouched. +export interface ActionAvailability { + destinationItem:(item:HTMLElement) => boolean; + moveItem:((item:HTMLElement) => boolean)|null; +} + +/** + * Projects a sortable item's action scope onto its Primer ActionMenu. + * + * It owns only presentation: which actions the menu presents, which of its two + * groups shows, and what the menu is called. It never resolves a scope, never + * decides whether an action is permitted, and holds no selection state — the + * caller answers those through {@link ActionAvailability}. + */ +export class SortableActionMenu { + private open = false; + + constructor( + private readonly menu:ActionMenuElement, + private readonly hideUnavailable:boolean, + private readonly labels:ActionMenuLabels|null, + ) {} + + get isOpen():boolean { + return this.open; + } + + ownsPopoverEvent(event:Event):boolean { + return event.target === this.menu.popoverElement; + } + + isItemActionable(item:HTMLElement):boolean { + return !this.menu.isItemDisabled(item) && !this.menu.isItemHidden(item); + } + + opening():void { + this.open = true; + } + + opened():void { + this.rescueOpenFocus(); + } + + closed():void { + this.open = false; + this.rename(false, 0); + } + + // A Turbo restore resurrects the tooltip as snapshotted, batch name and + // all; a closed menu always presents the singular one. + settleName():void { + this.rename(false, 0); + } + + project(elements:ActionMenuElements, scope:ActionMenuScope, availability:ActionAvailability):void { + const presented = this.projectDestinations(elements, availability.destinationItem) + + this.projectMoves(elements, availability.moveItem); + + this.projectGroups(elements, scope, presented); + } + + // Both projections return how many actions the menu is presenting, not how + // many are executable: with hideUnavailable off, unavailable items stay on + // screen disabled, and the group toggle must count them as presented. + private projectDestinations( + { destinationItems }:ActionMenuElements, + allowed:(item:HTMLElement) => boolean, + ):number { + let available = 0; + + for (const item of destinationItems) { + const enabled = allowed(item); + this.setAvailability(item, enabled); + if (enabled) { + available += 1; + } + } + + return this.hideUnavailable ? available : destinationItems.length; + } + + private projectMoves( + { moveItems, moveSubmenu }:ActionMenuElements, + allowed:((item:HTMLElement) => boolean)|null, + ):number { + // A null policy means the item is not in a list yet; leave the menu alone + // until the outlet wiring settles. + if (!allowed) { + return this.presentedMoveActionCount(moveItems, moveSubmenu); + } + + let available = 0; + for (const item of moveItems) { + const enabled = allowed(item); + this.setAvailability(item, enabled); + if (enabled) { + available += 1; + } + } + + if (moveSubmenu) { + this.setAvailability(moveSubmenu, available > 0); + return this.hideUnavailable && available === 0 ? 0 : 1; + } + + return this.hideUnavailable ? available : moveItems.length; + } + + // A nested submenu presents as one action; flat move items count each. + private presentedMoveActionCount(moveItems:HTMLElement[], moveSubmenu:HTMLElement|null):number { + if (moveSubmenu) { + return moveSubmenu.hasAttribute('hidden') ? 0 : 1; + } + + return moveItems.filter((item) => !item.hasAttribute('hidden')).length; + } + + private projectGroups( + { invokerGroup, batchGroup, groupDivider }:ActionMenuElements, + scope:ActionMenuScope, + presentedBatchActionCount:number, + ):void { + const batchActionsPresented = presentedBatchActionCount > 0; + const batch = scope.batch && batchActionsPresented; + + if (invokerGroup) { + if (batch) { + this.rescueFocusFrom(invokerGroup); + } + this.hideGroup(invokerGroup, batch); + } + + if (batchGroup) { + if (!batchActionsPresented) { + this.rescueFocusFrom(batchGroup); + } + this.hideGroup(batchGroup, !batchActionsPresented); + } + + // The divider only separates anything while both groups show. It never + // goes through setAvailability: `disableItem` writes to the item's + // `.ActionListContent`, which a divider does not have. + groupDivider?.toggleAttribute('hidden', batch || !batchActionsPresented); + + this.rename(batch, scope.count); + } + + // Primer's focus zone stops managing exactly the element `hidden` lands on, + // so hiding a group alone leaves its items in the zone for the arrows to + // step onto. A group that comes back is repopulated by the availability + // pass, which sets each item's own hidden state before this runs. + private hideGroup(group:HTMLElement, hidden:boolean):void { + group.toggleAttribute('hidden', hidden); + + if (!hidden) { + return; + } + + for (const item of group.querySelectorAll('[role="menuitem"]')) { + item.toggleAttribute('hidden', true); + } + } + + // Hiding the group that holds focus would fling focus to the body, and + // Primer closes the whole menu when focus leaves it. Park focus on the + // first menu item that stays presented before the group disappears. + private rescueFocusFrom(group:HTMLElement):void { + const active = this.menu.ownerDocument.activeElement; + if (!(active instanceof HTMLElement) || !group.contains(active)) { + return; + } + + this.presentedItems().find((item) => !group.contains(item))?.focus(); + } + + // Primer's open-time focus matches `:not([hidden]) > [role=menuitem]`, one + // level deep: a singular item whose group alone is hidden still matches, + // and focusing it no-ops. Primer schedules that focus a frame after the + // toggle, so the correction runs a frame later still. + private rescueOpenFocus():void { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + if (!this.open) { + return; + } + + const active = this.menu.ownerDocument.activeElement; + if ( + active instanceof HTMLElement + && this.menu.contains(active) + && active.getAttribute('role') === 'menuitem' + && !active.closest('[hidden]') + ) { + return; + } + + this.presentedItems()[0]?.focus(); + }); + }); + } + + private presentedItems():HTMLElement[] { + return Array.from(this.menu.querySelectorAll('[role="menuitem"]')) + .filter((item) => !item.closest('[hidden]')); + } + + // The invoker is a Primer IconButton whose label-type tooltip owns the + // accessible name — icon_button.rb drops the button's aria-label and points + // aria-labelledby at the — so writing aria-label would be inert. + // The batch name shows only while the menu is open; any projection with the + // popover closed settles back on the server-rendered singular name. + private rename(batch:boolean, count:number):void { + const tooltip = this.nameSource(); + if (!tooltip || !this.labels) { + return; + } + + tooltip.textContent = batch && this.open + ? I18n.t(this.labels.batchKey, { count }) + : this.labels.singular; + } + + // Primer labels the list by the invoker button, and accessible-name + // references do not chain: the button's own name comes from a further + // aria-labelledby at the tooltip, which leaves the menu itself unnamed. + // Point the list at the tooltip directly so both carry one name and the + // rename above moves them together. + private nameSource():HTMLElement|null { + const labelId = this.menu.invokerElement?.getAttribute('aria-labelledby'); + const tooltip = labelId ? this.menu.ownerDocument.getElementById(labelId) : null; + if (!labelId || !tooltip) { + return null; + } + + const list = this.menu.querySelector('[role="menu"]'); + if (list && list.getAttribute('aria-labelledby') !== labelId) { + list.setAttribute('aria-labelledby', labelId); + } + + return tooltip; + } + + // Availability goes through the action-menu element's API: disableItem sets + // the ActionListItem--disabled class plus aria-disabled on the item's + // content, and hideItem toggles hidden. It operates on any descendant li, + // including the ones in the nested move submenu. Default is hide; + // hideUnavailable=false switches to disable. + private setAvailability(item:HTMLElement, available:boolean):void { + if (this.hideUnavailable) { + if (available) { + this.menu.showItem(item); + } else { + this.menu.hideItem(item); + } + } else if (available) { + this.menu.enableItem(item); + } else { + this.menu.disableItem(item); + } + } +} diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index 66ae20f0b41b..36389b2cdc9b 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -1393,7 +1393,14 @@ describe('Sortable lists item controller', () => { + ' data-action="click->sortable-lists--item#move">' )).join(''); batchGroup.appendChild(parent); - menuElement.append(invokerGroup, divider, batchGroup); + // Primer's list wrapper: role=menu labelled by the invoker button, which + // is where the unnamed-menu bug lives — the button's own name comes from + // a further aria-labelledby, and those do not chain. + const list = document.createElement('ul'); + list.setAttribute('role', 'menu'); + list.setAttribute('aria-labelledby', `${idNumber}-menu-button`); + list.append(invokerGroup, divider, batchGroup); + menuElement.append(list); el.appendChild(menuElement); const menu:FakeActionMenu = { @@ -2332,6 +2339,49 @@ describe('Sortable lists item controller', () => { expect(tooltip.textContent).toBe('Work package actions'); }); + + // Server-side the menu is labelled by its invoker button, whose own name + // comes from a further aria-labelledby at the tooltip — and accessible + // name references do not chain, so the menu resolves to nothing. Follow + // the reference the assistive technology would, not the tooltip text. + const menuName = (el:HTMLElement):string|null => { + const labelId = el.querySelector('[role="menu"]')?.getAttribute('aria-labelledby'); + return labelId ? el.ownerDocument.getElementById(labelId)?.textContent ?? null : null; + }; + + it('names the menu itself, not only its invoker', async () => { + const { + el, popover, controller, root, actionScopeFor, + } = await mountRenamableMenu(); + const batchScope:ActionScope = { kind: 'batch', items: [el, document.createElement('li')] }; + actionScopeFor.mockReturnValue(batchScope); + controller.connectRoot(root); + openMenu(popover); + + expect(menuName(el)).toBe('Actions for 2 selected work packages'); + + popover.dispatchEvent(new ToggleEvent('toggle', { newState: 'closed', oldState: 'open' })); + + expect(menuName(el)).toBe('Work package actions'); + }); + + it('names the menu for a singular scope', async () => { + const { + el, popover, controller, root, + } = await mountRenamableMenu(); + controller.connectRoot(root); + openMenu(popover); + + expect(menuName(el)).toBe('Work package actions'); + }); + + // A consumer that names no batch scope still gets a named menu: the + // reference is repointed whatever the labels say. + it('names the menu when no batch label key is configured', async () => { + const { el } = await mountRenamableMenu({ withKey: false }); + + expect(menuName(el)).toBe('Work package actions'); + }); }); it('delegates an enabled click to the root and no-ops a disabled one', async () => { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index 2e3df08e9671..f345a1b22917 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -61,9 +61,14 @@ import { type DestinationIdentity, } from './list-dom'; import { webLinkHref } from './external-data'; +import { + SortableActionMenu, + type ActionAvailability, + type ActionMenuElements, + type ActionMenuLabels, +} from './action-menu'; import { renderDragPreview } from './preview'; import { scopeIds, type ActionScope } from './action-scope'; -import { refreshMenuAvailability } from './menu-availability'; type CleanupFn = () => void; @@ -140,7 +145,8 @@ export default class ItemController extends Controller implements R private dropIndicatorElement?:HTMLElement; private root?:SortableListsRoot; private refreshToken?:object; - private menuOpen = false; + private menu?:SortableActionMenu; + private projectedMenuElement?:ActionMenuElement; private readonly onMenuToggle = (event:Event):void => { // The toggle event does not bubble, so listen in capture phase; recompute @@ -148,18 +154,18 @@ export default class ItemController extends Controller implements R // shifted siblings meanwhile. Read newState by duck typing rather than // `instanceof ToggleEvent` so a browser without the ToggleEvent global // cannot throw. - if (!this.hasMenuElement || event.target !== this.menuElement.popoverElement) { + const menu = this.actionMenu; + if (!menu?.ownsPopoverEvent(event)) { return; } const { newState } = event as ToggleEvent; if (newState === 'open') { - this.menuOpen = true; + menu.opening(); this.refreshActionAvailability(); - this.rescueOpenFocus(); + menu.opened(); } else if (newState === 'closed') { - this.menuOpen = false; - this.refreshMenuLabel(false, 0); + menu.closed(); } }; @@ -171,14 +177,13 @@ export default class ItemController extends Controller implements R }; private readonly onMenuBeforeToggle = (event:Event):void => { - if ( - (event as ToggleEvent).newState === 'open' - && this.hasMenuElement - && event.target === this.menuElement.popoverElement - ) { - this.menuOpen = true; - this.prepareActionMenu(); + const menu = this.actionMenu; + if ((event as ToggleEvent).newState !== 'open' || !menu?.ownsPopoverEvent(event)) { + return; } + + menu.opening(); + this.prepareActionMenu(); }; connect():void { @@ -187,9 +192,7 @@ export default class ItemController extends Controller implements R this.element.addEventListener('contextual-action-menu:beforeOpen', this.onContextualBeforeOpen); this.element.addEventListener('beforetoggle', this.onMenuBeforeToggle, true); this.element.addEventListener('toggle', this.onMenuToggle, true); - // A Turbo restore resurrects the tooltip as snapshotted, batch name and - // all; a closed menu always presents the singular one. - this.refreshMenuLabel(false, 0); + this.actionMenu?.settleName(); } disconnect():void { @@ -229,7 +232,11 @@ export default class ItemController extends Controller implements R move(event:ActionEvent):void { const item = event.currentTarget; - if (!isOrderableItem(this.element) || !this.menuItemActionable(item)) { + if (!isOrderableItem(this.element) || !this.hasMenuElement || !(item instanceof HTMLElement)) { + return; + } + + if (!this.actionMenu?.isItemActionable(item)) { return; } @@ -266,7 +273,11 @@ export default class ItemController extends Controller implements R moveToDestination(event:ActionEvent):void { const item = event.currentTarget; - if (!this.menuItemActionable(item)) { + if (!this.hasMenuElement || !(item instanceof HTMLElement)) { + return; + } + + if (!this.actionMenu?.isItemActionable(item)) { return; } @@ -587,137 +598,83 @@ export default class ItemController extends Controller implements R private refreshActionAvailability(preparedScope?:ActionScope):void { this.refreshToken = undefined; const root = this.root; - if (!root || !this.hasMenuElement || (this.destinationItemTargets.length === 0 && this.moveItemTargets.length === 0)) { + const menu = this.actionMenu; + if (!root || !menu || (this.destinationItemTargets.length === 0 && this.moveItemTargets.length === 0)) { return; } const scope = preparedScope ?? root.actionScopeFor(this.element); - const presented = refreshMenuAvailability({ - menu: this.menuElement, - scope, - itemOrderable: isOrderableItem(this.element), - destinationItems: this.destinationItemTargets, - moveItems: this.moveItemTargets, - moveMenu: this.hasMoveMenuTarget ? this.moveMenuTarget : null, - divider: null, - hideUnavailable: this.hideUnavailableValue, - identifier: this.identifier, - availableDestinations: (scope, candidates) => root.availableDestinations(scope, candidates), - moveAvailability: () => root.moveAvailability(this.element), - }); - this.refreshActionGroups(scope, presented); - } - private refreshActionGroups(scope:ActionScope, presentedBatchActionCount:number):void { - const batch = scope.kind === 'batch' && scope.items.length > 1; - const batchActionsPresented = presentedBatchActionCount > 0; + menu.project( + this.menuElements(), + { batch: scope.kind === 'batch' && scope.items.length > 1, count: scope.items.length }, + this.actionAvailability(root, scope), + ); + } - if (this.hasInvokerGroupTarget) { - if (batch && batchActionsPresented) { - this.rescueFocusFrom(this.invokerGroupTarget); - } - this.hideGroup(this.invokerGroupTarget, batch && batchActionsPresented); + // Rebuilt whenever a morph hands the blessing a new element, carrying the + // open state across so a morph mid-popover does not read as a fresh close. + private get actionMenu():SortableActionMenu|null { + if (!this.hasMenuElement) { + return null; } - if (this.hasBatchGroupTarget) { - if (!batchActionsPresented) { - this.rescueFocusFrom(this.batchGroupTarget); + if (this.projectedMenuElement !== this.menuElement) { + const wasOpen = this.menu?.isOpen ?? false; + this.projectedMenuElement = this.menuElement; + this.menu = new SortableActionMenu(this.menuElement, this.hideUnavailableValue, this.menuLabels()); + if (wasOpen) { + this.menu.opening(); } - this.hideGroup(this.batchGroupTarget, !batchActionsPresented); - } - - // The divider only separates anything while both groups show. It never - // goes through setAvailability: `disableItem` writes to the item's - // `.ActionListContent`, which a divider does not have. - if (this.hasGroupDividerTarget) { - this.groupDividerTarget.toggleAttribute( - 'hidden', - (batch && batchActionsPresented) || !batchActionsPresented, - ); } - this.refreshMenuLabel(batch && batchActionsPresented, scope.items.length); + return this.menu ?? null; } - // Primer's focus zone stops managing exactly the element `hidden` lands on, - // so hiding a group alone leaves its items in the zone for the arrows to - // step onto. A group that comes back is repopulated by the availability - // pass, which sets each item's own hidden state before this runs. - private hideGroup(group:HTMLElement, hidden:boolean):void { - group.toggleAttribute('hidden', hidden); - - if (!hidden) { - return; + // A consumer that names neither scope keeps the server-rendered name in both. + private menuLabels():ActionMenuLabels|null { + if (!this.hasBatchMenuLabelKeyValue || !this.hasSingularMenuLabelValue) { + return null; } - for (const item of group.querySelectorAll('[role="menuitem"]')) { - item.toggleAttribute('hidden', true); - } + return { batchKey: this.batchMenuLabelKeyValue, singular: this.singularMenuLabelValue }; } - // Hiding the group that holds focus would fling focus to the body, and - // Primer closes the whole menu when focus leaves it. Park focus on the - // first menu item that stays presented before the group disappears. - private rescueFocusFrom(group:HTMLElement):void { - const active = this.element.ownerDocument.activeElement; - if (!(active instanceof HTMLElement) || !group.contains(active) || !this.hasMenuElement) { - return; - } - - const fallback = Array.from(this.menuElement.querySelectorAll('[role="menuitem"]')) - .find((item) => !group.contains(item) && !item.closest('[hidden]')); - fallback?.focus(); + private menuElements():ActionMenuElements { + return { + destinationItems: this.destinationItemTargets, + moveItems: this.moveItemTargets, + moveSubmenu: this.hasMoveMenuTarget ? this.moveMenuTarget : null, + invokerGroup: this.hasInvokerGroupTarget ? this.invokerGroupTarget : null, + batchGroup: this.hasBatchGroupTarget ? this.batchGroupTarget : null, + groupDivider: this.hasGroupDividerTarget ? this.groupDividerTarget : null, + }; } - // The invoker is a Primer IconButton whose label-type tooltip owns the - // accessible name — icon_button.rb drops the button's aria-label and points - // aria-labelledby at the — so writing aria-label would be inert. - // The batch name shows only while the menu is open; any projection with the - // popover closed settles back on the server-rendered singular name. - private refreshMenuLabel(batch:boolean, count:number):void { - const invoker = this.hasMenuElement ? this.menuElement.invokerElement : null; - const labelId = invoker?.getAttribute('aria-labelledby'); - const tooltip = labelId ? this.element.ownerDocument.getElementById(labelId) : null; - if (!tooltip || !this.hasBatchMenuLabelKeyValue || !this.hasSingularMenuLabelValue) { - return; + // A card that is not itself orderable offers no action in a singular scope: + // it is an addressable position, not something a menu can move. + private actionAvailability(root:SortableListsRoot, scope:ActionScope):ActionAvailability { + if (scope.kind === 'refused' && !isOrderableItem(this.element)) { + return { destinationItem: () => false, moveItem: () => false }; } - tooltip.textContent = batch && this.menuOpen - ? I18n.t(this.batchMenuLabelKeyValue, { count }) - : this.singularMenuLabelValue; - } + // Null availability means the item is not in a list yet. + const moves = root.moveAvailability(this.element); - // Primer's open-time focus matches `:not([hidden]) > [role=menuitem]`, one - // level deep: a singular item whose group alone is hidden still matches, - // and focusing it no-ops. Primer schedules that focus a frame after the - // toggle, so the correction runs a frame later still. - private rescueOpenFocus():void { - requestAnimationFrame(() => { - requestAnimationFrame(() => { - if (!this.menuOpen || !this.hasMenuElement) { - return; - } - - const active = this.element.ownerDocument.activeElement; - if ( - active instanceof HTMLElement - && this.menuElement.contains(active) - && active.getAttribute('role') === 'menuitem' - && !active.closest('[hidden]') - ) { - return; + return { + destinationItem: (item) => { + const candidates = this.destinationCandidates(item); + return candidates.length > 0 && root.availableDestinations(scope, candidates).length > 0; + }, + moveItem: moves + // Outside a Stimulus action there is no event.params, so read the + // param's backing attribute directly. + ? (item) => { + const direction = item.getAttribute(`data-${this.identifier}-direction-param`); + return isMoveDirection(direction) && moves[direction]; } - - const fallback = Array.from(this.menuElement.querySelectorAll('[role="menuitem"]')) - .find((item) => !item.closest('[hidden]')); - fallback?.focus(); - }); - }); - } - - private menuItemActionable(item:EventTarget|null):item is HTMLElement { - return this.hasMenuElement && item instanceof HTMLElement - && !this.menuElement.isItemDisabled(item) && !this.menuElement.isItemHidden(item); + : null, + }; } private destinationCandidates(item:HTMLElement):DestinationIdentity[] { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts deleted file mode 100644 index 7f9f2d95de55..000000000000 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/menu-availability.ts +++ /dev/null @@ -1,96 +0,0 @@ -//-- copyright -// OpenProject is an open source project management software. -// Copyright (C) the OpenProject GmbH -// -// This program is free software; you can redistribute it and/or -// modify it under the terms of the GNU General Public License version 3. -// -// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -// Copyright (C) 2006-2013 Jean-Philippe Lang -// Copyright (C) 2010-2013 the ChiliProject Team -// -// This program is free software; you can redistribute it and/or -// modify it under the terms of the GNU General Public License -// as published by the Free Software Foundation; either version 2 -// of the License, or (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with this program; if not, write to the Free Software -// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -// -// See COPYRIGHT and LICENSE files for more details. -//++ - -import type { ActionMenuElement } from '@openproject/primer-view-components/app/components/primer/alpha/action_menu/action_menu_element'; -import type { ActionScope } from './action-scope'; -import type { SortableListsRoot } from './drag-and-drop'; -import { isMoveDirection, parseDestinationCandidates, type DestinationIdentity } from './list-dom'; - -interface MenuAvailabilityInput { - menu:Pick; - scope:ActionScope; - itemOrderable:boolean; - destinationItems:HTMLElement[]; - moveItems:HTMLElement[]; - moveMenu:HTMLElement|null; - divider:HTMLElement|null; - hideUnavailable:boolean; - identifier:string; - availableDestinations:(scope:ActionScope, candidates:DestinationIdentity[]) => DestinationIdentity[]; - moveAvailability:() => ReturnType; -} - -export function refreshMenuAvailability(input:MenuAvailabilityInput):number { - const setAvailability = (item:HTMLElement, available:boolean):void => { - if (input.hideUnavailable) { - input.menu[available ? 'showItem' : 'hideItem'](item); - } else { - input.menu[available ? 'enableItem' : 'disableItem'](item); - } - }; - - const refused = input.scope.kind === 'refused' && !input.itemOrderable; - for (const item of input.destinationItems) { - const candidates = parseDestinationCandidates(item.dataset.sortableListsDestinations); - setAvailability(item, !refused && candidates.length > 0 && input.availableDestinations(input.scope, candidates).length > 0); - } - - const availability = refused - ? { top: false, up: false, down: false, bottom: false } - : input.moveAvailability(); - if (availability) { - let available = 0; - for (const item of input.moveItems) { - const direction = item.getAttribute(`data-${input.identifier}-direction-param`); - const enabled = isMoveDirection(direction) && availability[direction]; - setAvailability(item, enabled); - if (enabled) available += 1; - } - if (input.moveMenu) setAvailability(input.moveMenu, available > 0); - } - - const presented = (items:HTMLElement[]):number => input.hideUnavailable - ? items.filter((item) => !item.hasAttribute('hidden')).length - : items.length; - const presentedActionCount = presented(input.destinationItems) - + presented(input.moveMenu ? [input.moveMenu] : input.moveItems); - - if (input.divider && input.hideUnavailable) { - let sibling = input.divider.nextElementSibling; - while (sibling) { - if (sibling instanceof HTMLLIElement && !sibling.hasAttribute('hidden')) { - input.divider.removeAttribute('hidden'); - return presentedActionCount; - } - sibling = sibling.nextElementSibling; - } - input.divider.setAttribute('hidden', 'hidden'); - } - - return presentedActionCount; -} From cbcd0bfd9322fd5476d1ca2650e88e6c846f4552 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 19 Aug 2026 17:57:07 +0100 Subject: [PATCH 08/10] [AGILE-364] Name the menu from one plural key The menu took its batch name from an I18n key and its singular name from a literal string the component read out of another key, so the two scopes were named by different mechanisms and could drift apart. One key with plural forms names both: `one` is the menu of a single card, which is also what a batch of one presents. https://community.openproject.org/wp/AGILE-364 --- .../dynamic/sortable-lists/action-menu.ts | 25 +++++++--------- .../sortable-lists/item.controller.spec.ts | 29 ++++--------------- .../dynamic/sortable-lists/item.controller.ts | 29 +++++++------------ .../work_package_card_list_item_component.rb | 7 +---- modules/backlogs/config/locales/js-en.yml | 7 +++-- ...k_package_card_list_item_component_spec.rb | 10 +++++-- .../backlogs/spec/support/pages/backlog.rb | 2 +- 7 files changed, 40 insertions(+), 69 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts index 1d28d4a71a54..b749162fd014 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/action-menu.ts @@ -28,10 +28,9 @@ import type { ActionMenuElement } from '@openproject/primer-view-components/app/components/primer/alpha/action_menu/action_menu_element'; -export interface ActionMenuLabels { - singular:string; - batchKey:string; -} +// The plural form that names one card's own menu. A batch of one is that same +// menu, so it never counts a selection down to "1 selected work package". +const SINGULAR_COUNT = 1; // The menu's own elements, re-read by the caller on every projection so a // morph that replaces the subtree is picked up without rebuilding anything. @@ -70,7 +69,7 @@ export class SortableActionMenu { constructor( private readonly menu:ActionMenuElement, private readonly hideUnavailable:boolean, - private readonly labels:ActionMenuLabels|null, + private readonly labelKey:string|null, ) {} get isOpen():boolean { @@ -95,13 +94,13 @@ export class SortableActionMenu { closed():void { this.open = false; - this.rename(false, 0); + this.rename(SINGULAR_COUNT); } // A Turbo restore resurrects the tooltip as snapshotted, batch name and // all; a closed menu always presents the singular one. settleName():void { - this.rename(false, 0); + this.rename(SINGULAR_COUNT); } project(elements:ActionMenuElements, scope:ActionMenuScope, availability:ActionAvailability):void { @@ -194,7 +193,7 @@ export class SortableActionMenu { // `.ActionListContent`, which a divider does not have. groupDivider?.toggleAttribute('hidden', batch || !batchActionsPresented); - this.rename(batch, scope.count); + this.rename(batch ? scope.count : SINGULAR_COUNT); } // Primer's focus zone stops managing exactly the element `hidden` lands on, @@ -260,16 +259,14 @@ export class SortableActionMenu { // accessible name — icon_button.rb drops the button's aria-label and points // aria-labelledby at the — so writing aria-label would be inert. // The batch name shows only while the menu is open; any projection with the - // popover closed settles back on the server-rendered singular name. - private rename(batch:boolean, count:number):void { + // popover closed settles back on the singular form. + private rename(count:number):void { const tooltip = this.nameSource(); - if (!tooltip || !this.labels) { + if (!tooltip || !this.labelKey) { return; } - tooltip.textContent = batch && this.open - ? I18n.t(this.labels.batchKey, { count }) - : this.labels.singular; + tooltip.textContent = I18n.t(this.labelKey, { count: this.open ? count : SINGULAR_COUNT }); } // Primer labels the list by the invoker button, and accessible-name diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index 36389b2cdc9b..276943e9a844 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -2168,8 +2168,8 @@ describe('Sortable lists item controller', () => { js: { backlogs: { action_menu: { - batch_menu_label: { - one: 'Actions for 1 selected work package', + menu_label: { + one: 'Work package actions', other: 'Actions for %{count} selected work packages', }, }, @@ -2198,20 +2198,13 @@ describe('Sortable lists item controller', () => { async function mountRenamableMenu({ withKey = true, - withSingularLabel = true, tooltipText = 'Work package actions', } = {}) { const { el } = renderItemWithMenu(1); if (withKey) { el.setAttribute( - 'data-sortable-lists--item-batch-menu-label-key-value', - 'js.backlogs.action_menu.batch_menu_label', - ); - } - if (withSingularLabel) { - el.setAttribute( - 'data-sortable-lists--item-singular-menu-label-value', - 'Work package actions', + 'data-sortable-lists--item-menu-label-key-value', + 'js.backlogs.action_menu.menu_label', ); } const { invoker, tooltip } = attachMenuInvoker(el, tooltipText); @@ -2328,18 +2321,6 @@ describe('Sortable lists item controller', () => { expect(tooltip.textContent).toBe('Work package actions'); }); - it('leaves the name alone when no singular name is provided', async () => { - const { - el, tooltip, popover, controller, root, actionScopeFor, - } = await mountRenamableMenu({ withSingularLabel: false }); - const scope:ActionScope = { kind: 'batch', items: [el] }; - actionScopeFor.mockReturnValue(scope); - controller.connectRoot(root); - openMenu(popover); - - expect(tooltip.textContent).toBe('Work package actions'); - }); - // Server-side the menu is labelled by its invoker button, whose own name // comes from a further aria-labelledby at the tooltip — and accessible // name references do not chain, so the menu resolves to nothing. Follow @@ -2377,7 +2358,7 @@ describe('Sortable lists item controller', () => { // A consumer that names no batch scope still gets a named menu: the // reference is repointed whatever the labels say. - it('names the menu when no batch label key is configured', async () => { + it('names the menu when no label key is configured', async () => { const { el } = await mountRenamableMenu({ withKey: false }); expect(menuName(el)).toBe('Work package actions'); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index f345a1b22917..fad066a917d0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -65,7 +65,6 @@ import { SortableActionMenu, type ActionAvailability, type ActionMenuElements, - type ActionMenuLabels, } from './action-menu'; import { renderDragPreview } from './preview'; import { scopeIds, type ActionScope } from './action-scope'; @@ -93,12 +92,11 @@ export default class ItemController extends Controller implements R externalUrl: String, hideUnavailable: { type: Boolean, default: true }, label: String, - // I18n key (with count plurals) naming the menu while it is batch-scoped. - batchMenuLabelKey: String, - // The menu's singular name, server-rendered: the invoker tooltip's own - // text is no restore source, since a Turbo snapshot can capture it - // mid-rename. Renaming only engages when both label values are present. - singularMenuLabel: String, + // I18n key naming the menu, pluralized on the size of the scope it acts + // on. The invoker tooltip's own text is no restore source, since a Turbo + // snapshot can capture it mid-rename; a consumer that sets no key keeps + // the server-rendered name in every scope. + menuLabelKey: String, // See ItemMobility in list-dom. A `confined` item is still a full drag // source; only the lists the batch's permitted set names accept it. mobility: { type: String, default: 'free' }, @@ -113,10 +111,8 @@ export default class ItemController extends Controller implements R declare readonly hideUnavailableValue:boolean; declare readonly labelValue:string; declare readonly hasLabelValue:boolean; - declare readonly batchMenuLabelKeyValue:string; - declare readonly hasBatchMenuLabelKeyValue:boolean; - declare readonly singularMenuLabelValue:string; - declare readonly hasSingularMenuLabelValue:boolean; + declare readonly menuLabelKeyValue:string; + declare readonly hasMenuLabelKeyValue:boolean; declare readonly handleTarget:HTMLElement; declare readonly hasHandleTarget:boolean; @@ -622,7 +618,7 @@ export default class ItemController extends Controller implements R if (this.projectedMenuElement !== this.menuElement) { const wasOpen = this.menu?.isOpen ?? false; this.projectedMenuElement = this.menuElement; - this.menu = new SortableActionMenu(this.menuElement, this.hideUnavailableValue, this.menuLabels()); + this.menu = new SortableActionMenu(this.menuElement, this.hideUnavailableValue, this.menuLabelKey()); if (wasOpen) { this.menu.opening(); } @@ -631,13 +627,8 @@ export default class ItemController extends Controller implements R return this.menu ?? null; } - // A consumer that names neither scope keeps the server-rendered name in both. - private menuLabels():ActionMenuLabels|null { - if (!this.hasBatchMenuLabelKeyValue || !this.hasSingularMenuLabelValue) { - return null; - } - - return { batchKey: this.batchMenuLabelKeyValue, singular: this.singularMenuLabelValue }; + private menuLabelKey():string|null { + return this.hasMenuLabelKeyValue ? this.menuLabelKeyValue : null; } private menuElements():ActionMenuElements { diff --git a/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb b/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb index 2d9ea1bf5143..c56e90a121a7 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb +++ b/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb @@ -110,12 +110,7 @@ def row_data sortable_lists__item_label_value: work_package.to_fs(:caption), sortable_lists__item_type_value: "work_package", sortable_lists__item_mobility_value: mobility, - sortable_lists__item_batch_menu_label_key_value: "js.backlogs.action_menu.batch_menu_label", - # The same string the card menu's invoker tooltip renders; the - # controller restores it from here rather than trusting the tooltip - # text, which a Turbo snapshot can capture mid-rename. - sortable_lists__item_singular_menu_label_value: - I18n.t(:"open_project.common.work_package_card_component.menu.label_actions"), + sortable_lists__item_menu_label_key_value: "js.backlogs.action_menu.menu_label", # Native drag payload for external consumers; the same absolute URL # as the card menu's "Copy URL to clipboard" item. The label above # doubles as the link text of the text/html flavour. diff --git a/modules/backlogs/config/locales/js-en.yml b/modules/backlogs/config/locales/js-en.yml index f25f113c4e1c..bf8b2564818f 100644 --- a/modules/backlogs/config/locales/js-en.yml +++ b/modules/backlogs/config/locales/js-en.yml @@ -32,9 +32,10 @@ en: backlogs: action_menu: # A complete standalone string per plural form, never the singular - # name plus a suffix. - batch_menu_label: - one: "Actions for 1 selected work package" + # name plus a suffix. `one` names the menu of a single card, so it + # says what that menu does rather than counting a selection of one. + menu_label: + one: "Work package actions" other: "Actions for %{count} selected work packages" announcements: batch_too_large: "Cannot move %{count} work packages at once. Select no more than %{max}." diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb index 497910247717..045a62ac1c18 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb @@ -83,13 +83,19 @@ sortable_lists__item_type_value: "work_package", sortable_lists__item_label_value: work_package.to_fs(:caption), sortable_lists__item_mobility_value: "free", - sortable_lists__item_batch_menu_label_key_value: "js.backlogs.action_menu.batch_menu_label", - sortable_lists__item_singular_menu_label_value: "Work package actions" + sortable_lists__item_menu_label_key_value: "js.backlogs.action_menu.menu_label" ) expect(item.row_args[:draggable]).to be(true) expect(item.row_args).not_to include(:tabindex) end + # The client renames the menu from this one key, so its singular form has + # to be the very name the card's invoker tooltip already renders. + it "names a singular menu exactly as the card's own tooltip does" do + expect(I18n.t("js.backlogs.action_menu.menu_label", count: 1)) + .to eq(I18n.t(:"open_project.common.work_package_card_component.menu.label_actions")) + end + context "with classic mode", with_settings: { work_packages_identifier: "classic" } do it "exposes the absolute ID-based work package URL for external drag consumers" do diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index 755714b6c20a..b2c7f1d77f3f 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -383,7 +383,7 @@ def expect_action_menu_groups(invoker:, selected_count:) expect(menu).to have_css(batch_group) expect_menu_invoker_name( invoker, - I18n.t("js.backlogs.action_menu.batch_menu_label", count: selected_count) + I18n.t("js.backlogs.action_menu.menu_label", count: selected_count) ) else expect(menu).to have_css(singular_group) From 6a58fece51b8655c22f3a98c4d328ba2d8a6f303 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Thu, 20 Aug 2026 17:39:21 +0100 Subject: [PATCH 09/10] [AGILE-364] Stop dismissal closing a loading dialog Escape dismissal races the dialog a menu item is still loading. The modal guard reads the DOM before showModal, so the key arrives after it and closes the dialog, which the dialog stream action then removes from the DOM. The spec sees a menu action that opened nothing. Primer already hides the menu when an item is activated, so the click path only waits for that instead of forcing the key. Menu inspection keeps the Escape dismissal, and so does the one invocation whose response morphs the card list out from under the open menu. Reproduced by holding the dismissal until the dialog opens, which fails every run before this change and passes after. https://community.openproject.org/wp/AGILE-364 --- modules/backlogs/spec/support/pages/backlog.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index b2c7f1d77f3f..c897f6d3f5c0 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -330,13 +330,13 @@ def click_in_inbox_menu(item_name) end end - def within_work_package_menu(work_package, &) + def within_work_package_menu(work_package, activated: false, &) within_work_package(work_package) do button = find(:button, accessible_name: "Work package actions") within(open_controlled_menu(button), &) end - dismiss_menu(work_package) + dismiss_menu(work_package, activated:) end # The four user entry points Backlogs supports, with the native key and @@ -634,7 +634,7 @@ def scroll_past_work_package_card(work_package, distance:) end def click_in_work_package_menu(work_package, item_name, wait: true) - within_work_package_menu(work_package) do |submenu| + within_work_package_menu(work_package, activated: true) do |submenu| wait_for_turbo_stream(wait:) do submenu.find(:menuitem, text: item_name).click end @@ -1855,9 +1855,15 @@ def open_move_submenu(menu) # Dismisses with Escape rather than an outside click: the compact batch # menu can leave a live menuitem under the overlay's centre point, so a # click there activates an action instead of closing the menu. - def dismiss_menu(menu_owner) + # Activating an item closes the menu through Primer itself, so the caller + # only waits for that to happen. Pressing Escape on top of it races the + # dialog the same activation is still loading: the modal guard below reads + # the DOM before `showModal`, and the key then arrives after it and closes + # the dialog, which `StreamActions.dialog` removes from the DOM on close. + def dismiss_menu(menu_owner, activated: false) selector = menu_owner_overlay_selector(menu_owner) + return expect(page).to have_no_css(selector, visible: :visible) if activated return unless page.has_css?(selector, visible: true, wait: 0) return if page.has_selector?(:modal, wait: 0) From f9a2616073bbca3cb6f3d986f7435a718157aa84 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 15 Sep 2026 17:52:39 +0100 Subject: [PATCH 10/10] Assert rendered selection in action menu specs Adapt AGILE-364 tests to the selection API removed during AGILE-278 review. --- .../dynamic/sortable-lists.controller.spec.ts | 10 +++++----- .../sortable-lists/selection-orchestrator.spec.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 4574b46628e3..3afb99c4ceb2 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -1590,7 +1590,7 @@ describe('Sortable lists controller', () => { expect(controller.selectForAction(items[1])).toMatchObject({ kind: 'batch' }); - expect(controller.selectedItems().map((item) => item.id)).toEqual(['1', '2']); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0], items[1]]); }); it('replaces an unrelated selection when an unselected eligible invoker opens its menu', async () => { @@ -1603,7 +1603,7 @@ describe('Sortable lists controller', () => { expect(controller.selectForAction(items[2])).toMatchObject({ kind: 'batch' }); - expect(controller.selectedItems().map((item) => item.id)).toEqual(['3']); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[2]]); }); it('preserves the current selection when a fixed invoker opens its singular menu', async () => { @@ -1616,7 +1616,7 @@ describe('Sortable lists controller', () => { announceSpy.mockClear(); expect(controller.selectForAction(items[2])).toEqual({ kind: 'refused', items: [] }); - expect(controller.selectedItems().map((item) => item.id)).toEqual(['1', '2']); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0], items[1]]); expect(announcedMessages()).toEqual([]); }); @@ -1631,7 +1631,7 @@ describe('Sortable lists controller', () => { expect(controller.selectForAction(items[2])).toMatchObject({ kind: 'batch' }); - expect(controller.selectedItems().map((item) => item.id)).toEqual(['1', '2']); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0], items[1]]); }); it('settles duplicate pre-open delivery once without repeating its selection announcement', async () => { @@ -1647,7 +1647,7 @@ describe('Sortable lists controller', () => { expect(contextualScope).toMatchObject({ kind: 'batch' }); expect(popoverScope).toMatchObject({ kind: 'batch' }); - expect(controller.selectedItems().map((item) => item.id)).toEqual(['3']); + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[2]]); expect(announcedMessages()).toEqual([ ['[selected:1]', { politeness: 'polite' }], ]); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index 5914431a0fe0..999d2031f243 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -819,7 +819,7 @@ describe('SelectionOrchestrator', () => { orchestrator.selectForAction(item('3')); orchestrator.handleClick(clickOn(item('2'), { shiftKey: true })); - expect(orchestrator.selectedItems().map((i) => i.id)).toEqual(['1', '2']); + expect(selectedIds()).toEqual(['1', '2']); }); it('keeps the selected batch for an action on a member', () => {