From 00bf0583d7613ea573a4cd529ca3c328b5af787b Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 29 Jul 2026 00:02:10 +0200 Subject: [PATCH 1/3] [AGILE-348] Add contextual action menu presenter Primer's ActionMenu opens at its trigger button; presenting one at a pointer position means re-anchoring the overlay and restoring it after. It lands in `src/common/` because the Angular card lists will want the same behaviour. https://community.openproject.org/wp/AGILE-348 --- frontend/AGENTS.md | 1 + .../src/common/contextual-action-menu.spec.ts | 414 ++++++++++++++++++ frontend/src/common/contextual-action-menu.ts | 314 +++++++++++++ 3 files changed, 729 insertions(+) create mode 100644 frontend/src/common/contextual-action-menu.spec.ts create mode 100644 frontend/src/common/contextual-action-menu.ts diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index dcd23462d442..6fccc2fec73c 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -4,6 +4,7 @@ - `./src/` - Frontend code - `./src/app/` - Legacy Angular modules/components + - `./src/common/` - Framework-agnostic modules (the `core-common` alias), importable from both Angular and Stimulus. Code belongs here when it depends on neither framework and both sides need it; a helper only Stimulus controllers use belongs in `./src/stimulus/helpers/` instead. - `./src/stimulus/` - Stimulus controllers - `./src/turbo/` - Turbo integration diff --git a/frontend/src/common/contextual-action-menu.spec.ts b/frontend/src/common/contextual-action-menu.spec.ts new file mode 100644 index 000000000000..503e00a91c6f --- /dev/null +++ b/frontend/src/common/contextual-action-menu.spec.ts @@ -0,0 +1,414 @@ +//-- 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 { getAnchoredPosition } from '@primer/behaviors'; +import type { MockInstance } from 'vitest'; +import { CONTEXTUAL_ALIGN, CONTEXTUAL_SIDE, ContextualActionMenu } from './contextual-action-menu'; + +// Mirrors the members of Primer's `AnchoredPositionElement` that the presenter +// drives — including the quirk that gives this spec its teeth: `anchorOffset` +// reads its attribute as an enum (only `spacious`/`8` mean 8), so an arbitrary +// pixel offset can only reach `getAnchoredPosition` as a property. +// +// The accessors deliberately live on a prototype. The presenter carries the +// vertical offset as an *own* property shadowing `anchorOffset`, and a fake +// that defined its accessors per instance would be overwritten rather than +// shadowed, leaving the restore with nothing to undo and nothing to prove. +// +// `update()` runs the real `getAnchoredPosition`, with the element itself as +// the settings object, exactly as the production element does. The offsets +// this spec asserts on are therefore read the way Primer reads them, rather +// than the way this spec wishes they were read. +class FakeAnchoredPositionElement extends HTMLElement { + private ownAnchorElement:HTMLElement|null = null; + + get align():'start'|'center'|'end' { + const value = this.getAttribute('align'); + return value === 'center' || value === 'end' ? value : 'start'; + } + + get side():'outside-bottom'|'outside-top' { + return this.getAttribute('side') === 'outside-top' ? 'outside-top' : 'outside-bottom'; + } + + get anchorOffset():number { + const alias = this.getAttribute('anchor-offset'); + return alias === 'spacious' || alias === '8' ? 8 : 4; + } + + get alignmentOffset():number { + return Number(this.getAttribute('alignment-offset')); + } + + get allowOutOfBounds():boolean { + return this.hasAttribute('allow-out-of-bounds'); + } + + get anchorElement():HTMLElement|null { + if (this.ownAnchorElement) { + return this.ownAnchorElement; + } + + const idRef = this.getAttribute('anchor'); + return idRef ? this.ownerDocument.getElementById(idRef) : null; + } + + set anchorElement(value:HTMLElement|null) { + this.ownAnchorElement = value; + if (!value) { + this.removeAttribute('anchor'); + } + } + + update():void { + const anchor = this.anchorElement; + if (!anchor) { + return; + } + + const { top, left } = getAnchoredPosition(this, anchor, this); + this.style.top = `${top}px`; + this.style.left = `${left}px`; + this.style.bottom = 'auto'; + this.style.right = 'auto'; + } +} + +if (!customElements.get('op-fake-anchored-position')) { + customElements.define('op-fake-anchored-position', FakeAnchoredPositionElement); +} + +// The real custom element is registered by the Primer bundle, +// which the Vitest browser environment does not load. The presenter only +// touches four public members, so a structural stand-in exercises the exact +// contract without pulling in the whole component. +interface FakeMenu { + overlay:FakeAnchoredPositionElement; + popoverElement:HTMLElement; + invokerElement:HTMLButtonElement; + ownerDocument:Document; + contains(node:Node|null):boolean; +} + +describe('ContextualActionMenu', () => { + const nextFrame = () => new Promise((resolve) => requestAnimationFrame(() => resolve())); + + // The card sits at a known place well inside the viewport, so the pointer + // offsets below are unambiguous and no viewport clamping muddies them. + const CARD_LEFT = 100; + const CARD_TOP = 200; + const CARD_BOTTOM = 280; + const POINTER_X = 150; + const POINTER_Y = 240; + + let fixture:HTMLElement; + let menu:FakeMenu; + let card:HTMLElement; + let overlay:FakeAnchoredPositionElement; + let popover:HTMLElement; + let invoker:HTMLButtonElement; + let update:MockInstance<() => void>; + let presenter:ContextualActionMenu; + + // Where Primer would put the overlay for the anchoring currently in force. + const anchoredPosition = () => getAnchoredPosition(overlay, overlay.anchorElement!, overlay); + + // Where Primer would put the overlay if it were anchored on a zero-size + // point at the pointer, with no offsets at all — the position a contextual + // invocation is supposed to produce. Both sides run the same library code, + // so the comparison is immune to the positioned parent and to clamping. + const positionAtPointer = (clientX:number, clientY:number) => getAnchoredPosition( + overlay, + new DOMRect(clientX, clientY, 0, 0), + { + side: CONTEXTUAL_SIDE, align: CONTEXTUAL_ALIGN, anchorOffset: 0, alignmentOffset: 0, + }, + ); + + // `hidePopover()` does not fire `toggle` synchronously — it queues a task — + // and nothing orders that task against `requestAnimationFrame`. Waiting a + // fixed number of frames therefore only holds where the browser happens to + // drain the task before the frame callback, which Chrome does and Firefox + // does not reliably do. Waiting for the event the presenter listens to is + // both exact and one frame cheaper; the presenter registered its listener at + // open time, so it has already run by the time this resolves. + const closeMenu = () => new Promise((resolve) => { + const listener = (event:Event):void => { + if ((event as ToggleEvent).newState !== 'closed') { + return; + } + + overlay.removeEventListener('toggle', listener); + resolve(); + }; + + overlay.addEventListener('toggle', listener); + popover.hidePopover(); + }); + + beforeEach(() => { + fixture = document.createElement('div'); + fixture.innerHTML = ` +
+ + + + +
+ `; + document.body.appendChild(fixture); + + card = fixture.querySelector('.card')!; + overlay = fixture.querySelector('op-fake-anchored-position')!; + popover = overlay; + invoker = fixture.querySelector('#wp-1-menu-button')!; + update = vi.spyOn(overlay, 'update'); + + menu = { + overlay, + popoverElement: popover, + invokerElement: invoker, + ownerDocument: document, + contains: (node:Node|null) => (node ? card.contains(node) : false), + }; + + presenter = new ContextualActionMenu(menu as never); + }); + + afterEach(() => { + presenter.destroy(); + fixture.remove(); + vi.restoreAllMocks(); + }); + + it('anchors the overlay on the invoking element and opens the popover', () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + + // Anchoring on the card, rather than on a point in the viewport, is what + // makes every reposition Primer runs recompute against a rect that moves + // with the page. See the scroll-tracking spec below. + expect(overlay.anchorElement).toBe(card); + // The More button's idref must be gone while the card anchors the menu: + // the property alone would position it correctly but leave the DOM + // claiming an anchoring that is no longer in force. + expect(overlay.hasAttribute('anchor')).toBe(false); + expect(overlay.getAttribute('align')).toBe(CONTEXTUAL_ALIGN); + expect(overlay.getAttribute('side')).toBe(CONTEXTUAL_SIDE); + expect(popover.matches(':popover-open')).toBe(true); + expect(update).toHaveBeenCalled(); + }); + + // The offsets are the whole mechanism: they are what turns an anchoring on + // the card into a menu at the pointer. Asserting the resulting position + // through the real `getAnchoredPosition` — rather than asserting the two + // numbers this spec would have to derive itself — is what makes this fail if + // either offset lands on the wrong axis or with the wrong sign. + it('positions the menu at the pointer', () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + + const wanted = positionAtPointer(POINTER_X, POINTER_Y); + const actual = anchoredPosition(); + + expect(actual.top).toBeCloseTo(wanted.top, 0); + expect(actual.left).toBeCloseTo(wanted.left, 0); + }); + + // Regression (AGILE-348 QA): the menu used to be anchored on a synthetic + // `position: fixed` element placed at the pointer, which never moves in + // viewport space, so every reposition recomputed the same viewport point + // while the card scrolled out from under it. Offsets from the card's live + // rect are re-read on each update, so the menu goes where the card goes — + // whether the window scrolls or a scrolling ancestor does. + it('follows the invoking element when it moves', () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + const before = anchoredPosition(); + + card.style.top = `${CARD_TOP - 50}px`; + overlay.update(); + + const after = anchoredPosition(); + expect(after.top).toBeCloseTo(before.top - 50, 0); + expect(after.left).toBeCloseTo(before.left, 0); + }); + + it('re-anchors an already open menu instead of toggling it shut', () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + presenter.openAtPoint(POINTER_X + 60, POINTER_Y + 20, card); + + const wanted = positionAtPointer(POINTER_X + 60, POINTER_Y + 20); + const actual = anchoredPosition(); + + expect(actual.top).toBeCloseTo(wanted.top, 0); + expect(actual.left).toBeCloseTo(wanted.left, 0); + expect(popover.matches(':popover-open')).toBe(true); + }); + + it('restores the More button anchoring when the menu closes', async () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + + // The open really did take the anchoring away, so the assertion below is + // a restore rather than an attribute that was never touched. + expect(overlay.hasAttribute('anchor')).toBe(false); + + await closeMenu(); + + expect(overlay.getAttribute('anchor')).toBe('wp-1-menu-button'); + expect(overlay.getAttribute('align')).toBe('end'); + expect(overlay.hasAttribute('side')).toBe(false); + expect(overlay.anchorElement).toBe(invoker); + }); + + // Regression (AGILE-348 QA): the offsets are as much a part of the borrowed + // anchoring as the idref is. Left behind, they would shove the More button's + // own menu to wherever the last right-click happened to land — which is the + // exact class of bug the restore exists to prevent. + it('restores the pointer offsets when the menu closes', async () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + + expect(overlay.getAttribute('alignment-offset')).toBe(`${POINTER_X - CARD_LEFT}`); + expect(overlay.anchorOffset).toBe(POINTER_Y - CARD_BOTTOM); + + await closeMenu(); + + expect(overlay.getAttribute('alignment-offset')).toBe('12'); + // Back to the enum the attribute actually spells, which is the proof the + // shadowing property was dropped rather than merely overwritten. + expect(overlay.anchorOffset).toBe(8); + }); + + it('returns focus to the invoking element when the menu closes', async () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + invoker.focus(); + await closeMenu(); + await nextFrame(); + + expect(document.activeElement).toBe(card); + }); + + it('leaves focus alone when something outside the menu claimed it', async () => { + const elsewhere = document.createElement('input'); + document.body.appendChild(elsewhere); + + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + await closeMenu(); + // The restore is armed but deferred a frame, so this claims focus in the + // window the restore exists to keep out of. + elsewhere.focus(); + await nextFrame(); + + expect(document.activeElement).toBe(elsewhere); + elsewhere.remove(); + }); + + // The restore is scheduled by the close and runs a frame later, so anything + // that closes and reopens the popover within that frame leaves the menu + // showing with focus legitimately on the invoker — pulling focus back to the + // card there would empty a menu the user just opened. + it('leaves focus alone when the menu is open again by the time the restore runs', async () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + invoker.focus(); + + // Awaiting the close is what lets the close listener run — and arm its + // restore — before the reopen. + await closeMenu(); + + popover.showPopover(); + await nextFrame(); + + expect(document.activeElement).toBe(invoker); + }); + + it('restores overlay state on destroy', () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + presenter.destroy(); + + expect(overlay.getAttribute('anchor')).toBe('wp-1-menu-button'); + expect(overlay.getAttribute('alignment-offset')).toBe('12'); + expect(overlay.anchorOffset).toBe(8); + }); + + describe('opening at the menu invoker', () => { + it('opens the popover without touching the anchoring', () => { + presenter.openAtInvoker(card); + + // Primer's own anchoring is left in force, so this opening lands exactly + // where the More button lands it. + expect(overlay.getAttribute('anchor')).toBe('wp-1-menu-button'); + expect(overlay.anchorElement).toBe(invoker); + expect(overlay.getAttribute('align')).toBe('end'); + expect(overlay.hasAttribute('side')).toBe(false); + expect(overlay.getAttribute('alignment-offset')).toBe('12'); + expect(overlay.anchorOffset).toBe(8); + expect(popover.matches(':popover-open')).toBe(true); + // Nothing moved, so nothing needs repositioning either. + expect(update).not.toHaveBeenCalled(); + }); + + it('still returns focus to the invoking element when the menu closes', async () => { + presenter.openAtInvoker(card); + invoker.focus(); + await closeMenu(); + await nextFrame(); + + expect(document.activeElement).toBe(card); + }); + + // Nothing was overridden, so the close has nothing to put back. A restore + // running here would re-apply state captured by some earlier invocation + // and quietly overwrite whatever the page has set since. + it('leaves the anchoring alone when the menu closes', async () => { + presenter.openAtInvoker(card); + + // Set *after* the open: state captured at open time would be re-applied + // over this on close, quietly reverting a change the page made while the + // menu was up. + overlay.setAttribute('align', 'center'); + + await closeMenu(); + + expect(overlay.getAttribute('align')).toBe('center'); + }); + + // Taking over a menu that a right-click left open: its borrowed anchoring + // has to go before this opening claims the invoker's position, or the + // pointer offsets would silently drag the menu away from the button. + it('undoes a contextual invocation it takes over from', () => { + presenter.openAtPoint(POINTER_X, POINTER_Y, card); + presenter.openAtInvoker(card); + + expect(overlay.getAttribute('anchor')).toBe('wp-1-menu-button'); + expect(overlay.anchorElement).toBe(invoker); + expect(overlay.getAttribute('align')).toBe('end'); + expect(overlay.getAttribute('alignment-offset')).toBe('12'); + expect(overlay.anchorOffset).toBe(8); + expect(popover.matches(':popover-open')).toBe(true); + }); + }); +}); diff --git a/frontend/src/common/contextual-action-menu.ts b/frontend/src/common/contextual-action-menu.ts new file mode 100644 index 000000000000..ca90b5486f69 --- /dev/null +++ b/frontend/src/common/contextual-action-menu.ts @@ -0,0 +1,314 @@ +//-- 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 AnchoredPositionElement from '@openproject/primer-view-components/app/components/primer/anchored_position'; + +// A context menu conventionally grows down and to the right of its invocation +// point. `anchored-position` clamps into the viewport unless +// `allow-out-of-bounds` is set, so viewport safety comes from leaving that +// attribute alone. Both values are single constants because Design QA on +// AGILE-348 may retune them. +// +// The pair is also what makes the pointer offsets below mean what they say: +// `getAnchoredPosition` computes `top = anchorRect.bottom + anchorOffset` and +// `left = anchorRect.left + alignmentOffset` for exactly this side/align +// combination. Change either constant and the two offsets change axis. +export const CONTEXTUAL_ALIGN = 'start'; +export const CONTEXTUAL_SIDE = 'outside-bottom'; + +interface OverlayState { + anchor:string|null; + align:string|null; + side:string|null; + alignmentOffset:string|null; +} + +function restoreAttribute(element:Element, name:string, value:string|null):void { + if (value === null) { + element.removeAttribute(name); + } else { + element.setAttribute(name, value); + } +} + +/** + * Carries the vertical pointer offset as an own property on the overlay. + * + * `alignment-offset` is a plain `Number(getAttribute(...))` on + * `AnchoredPositionElement`, so the horizontal offset travels as an attribute. + * `anchorOffset` is not: its getter reads the attribute as an enum — only + * `spacious`/`8` mean 8, everything else means 4 — so the attribute cannot + * carry an arbitrary pixel offset, and its setter only writes that attribute. + * `AnchoredPositionElement.update()` passes the element itself to + * `getAnchoredPosition` as the settings object, which reads `anchorOffset` off + * it like any other property, so an own property shadowing the prototype + * accessor is what gets a real number through — and deleting it hands the + * accessor, and the More button's ordinary 4px gap, straight back. + */ +function setAnchorOffset(overlay:AnchoredPositionElement, value:number):void { + Object.defineProperty(overlay, 'anchorOffset', { configurable: true, value }); +} + +function clearAnchorOffset(overlay:AnchoredPositionElement):void { + Reflect.deleteProperty(overlay, 'anchorOffset'); +} + +/** + * Presents an existing Primer ActionMenu contextually: at a pointer position + * on the invoking element, or at the menu's own show button but with the + * invoking element owning focus afterwards. + * + * It owns only presentation. It never decides which actions the menu contains, + * never mutates menu items, and holds no selection state, so a later slice can + * layer batch-selection policy on top of the same presenter. + */ +export class ContextualActionMenu { + private savedState?:OverlayState; + private returnFocusTo?:HTMLElement; + private closeListener?:(event:Event) => void; + + constructor(private readonly menu:ActionMenuElement) {} + + /** + * Opens the menu at a viewport point (pointer invocation), anchored on the + * invoking element so that it keeps tracking that element while the page + * scrolls. + * + * `invoker` both anchors the overlay and receives focus back when the menu + * closes; it is the element the gesture happened on — the card. + */ + openAtPoint(clientX:number, clientY:number, invoker:HTMLElement):void { + const overlay = this.overlay; + const popover = this.menu.popoverElement; + if (!overlay || !popover) { + return; + } + + this.saveOverlayState(overlay); + this.anchorAtPoint(overlay, invoker, clientX, clientY); + this.show(overlay, popover, invoker); + } + + /** + * Opens the menu where its own show button would open it, leaving Primer's + * anchoring untouched, and only takes over where focus goes afterwards. + * + * This is the keyboard invocation and the right-click on the menu's own + * trigger: both want the menu's ordinary position, not a pointer-relative + * one. Nothing is overridden here, so nothing is saved — but a contextual + * invocation that is still open has to be undone, or its pointer offsets + * would silently reposition this opening. + */ + openAtInvoker(returnFocusTo:HTMLElement):void { + const overlay = this.overlay; + const popover = this.menu.popoverElement; + if (!overlay || !popover) { + return; + } + + // Only if there was something to undo: an untouched overlay is already + // positioned by Primer, and a gratuitous reposition here would be a claim + // that this path does something to the anchoring, which it does not. + if (this.restoreOverlayState(overlay)) { + overlay.update(); + } + + this.show(overlay, popover, returnFocusTo); + } + + /** + * Drops every contextual mutation. Called when the host controller + * disconnects, so a morph that removes the card mid-invocation cannot leave + * a rewritten overlay behind. + */ + destroy():void { + const overlay = this.overlay; + if (overlay && this.closeListener) { + overlay.removeEventListener('toggle', this.closeListener); + } + this.closeListener = undefined; + this.returnFocusTo = undefined; + if (overlay) { + this.restoreOverlayState(overlay); + } + } + + private get overlay():AnchoredPositionElement|undefined { + return this.menu.overlay ?? undefined; + } + + private show( + overlay:AnchoredPositionElement, + popover:HTMLElement, + returnFocusTo:HTMLElement, + ):void { + this.returnFocusTo = returnFocusTo; + this.listenForClose(overlay); + + // Repeated invocation on an already open menu only moves it. Toggling it + // shut and open again would restart the deferred fragment focus dance. + if (!popover.matches(':popover-open')) { + popover.showPopover(); + } + } + + /** + * Expresses the pointer position as offsets from the invoking element rather + * than anchoring to a synthetic element placed at the pointer. + * + * A `position: fixed` element pinned at viewport coordinates never moves in + * viewport space, so every reposition Primer runs — on scroll, on resize — + * recomputed the same viewport point while the card scrolled away underneath + * it. Offsets from the card's live rect are recomputed on every `update()`, + * so the menu follows the card through window *and* container scrolling. + */ + private anchorAtPoint( + overlay:AnchoredPositionElement, + anchorElement:HTMLElement, + clientX:number, + clientY:number, + ):void { + const anchorRect = anchorElement.getBoundingClientRect(); + + overlay.anchorElement = anchorElement; + // The property wins over the `anchor` idref for positioning, but only + // clearing it to null strips the attribute, so taking over the anchoring + // has to strip it here. Leaving the More button's idref in place would + // have the DOM advertise an anchoring that is no longer in force — and it + // is the one attribute a test can read to tell a contextual invocation + // from an ordinary one. `saveOverlayState` captured it for restore. + overlay.removeAttribute('anchor'); + overlay.setAttribute('align', CONTEXTUAL_ALIGN); + overlay.setAttribute('side', CONTEXTUAL_SIDE); + overlay.setAttribute('alignment-offset', `${clientX - anchorRect.left}`); + setAnchorOffset(overlay, clientY - anchorRect.bottom); + // Setting anchorElement is a property write, so it does not run the + // element's attributeChangedCallback; ask for a reposition explicitly. + overlay.update(); + } + + private saveOverlayState(overlay:AnchoredPositionElement):void { + if (this.savedState) { + return; + } + + this.savedState = { + anchor: overlay.getAttribute('anchor'), + align: overlay.getAttribute('align'), + side: overlay.getAttribute('side'), + alignmentOffset: overlay.getAttribute('alignment-offset'), + }; + } + + /** @returns whether there was any contextual state left to undo. */ + private restoreOverlayState(overlay:AnchoredPositionElement):boolean { + const state = this.savedState; + this.savedState = undefined; + if (!state) { + return false; + } + + // The property has to go first: it wins over the attribute, so a stale + // anchor would otherwise survive the re-applied idref. Both the open path + // and this null assignment strip `anchor`, which is why the saved + // attributes are re-applied afterwards — that is what puts the More button + // back in charge on the menu's next ordinary invocation. The two offsets + // are part of that: left behind, they would shove the More button's own + // menu to wherever the last right-click happened to land. + overlay.anchorElement = null; + clearAnchorOffset(overlay); + restoreAttribute(overlay, 'anchor', state.anchor); + restoreAttribute(overlay, 'align', state.align); + restoreAttribute(overlay, 'side', state.side); + restoreAttribute(overlay, 'alignment-offset', state.alignmentOffset); + + return true; + } + + private listenForClose(overlay:AnchoredPositionElement):void { + if (this.closeListener) { + return; + } + + const listener = (event:Event):void => { + if (event.target !== overlay || (event as ToggleEvent).newState !== 'closed') { + return; + } + + overlay.removeEventListener('toggle', listener); + this.closeListener = undefined; + + const returnFocusTo = this.returnFocusTo; + this.returnFocusTo = undefined; + + this.restoreOverlayState(overlay); + this.restoreFocus(returnFocusTo); + }; + + this.closeListener = listener; + overlay.addEventListener('toggle', listener); + } + + // Primer sends focus back to the show button when the menu closes, which is + // wrong for a contextual invocation: the card, not the button, was the + // invoker. Only reclaim focus when nothing else has taken it — an activated + // item may have opened a dialog or moved focus deliberately. + private restoreFocus(returnFocusTo?:HTMLElement):void { + if (!returnFocusTo) { + return; + } + + const doc = this.menu.ownerDocument; + requestAnimationFrame(() => { + // The restore is deferred a frame, so the menu may be showing again by + // the time it runs — anything that closes and reopens the popover inside + // one gesture leaves it that way. Focus then legitimately sits on the + // invoker, and treating it as loose, as the check below does for the + // Escape path, would yank focus out of a menu the user just opened. + // + // (Clicking the More button is *not* such a gesture, despite the + // symmetry: measured on Chrome 150, light-dismiss never fires for a + // popover's own invoker, so the click simply closes the menu. See the + // AGILE-348 QA report.) + if (this.menu.popoverElement?.matches(':popover-open')) { + return; + } + + const active = doc.activeElement; + const focusIsLoose = active === null + || active === doc.body + || active === this.menu.invokerElement + || this.menu.contains(active); + + if (focusIsLoose) { + returnFocusTo.focus(); + } + }); + } +} From 9a88f50d56e5c3ceee0cbe0e4afe17f5b793dd4a Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 29 Jul 2026 00:02:16 +0200 Subject: [PATCH 2/3] [AGILE-348] Add contextual-action-menu controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attaches contextual entry points — right-click, Shift+F10, the Context Menu key — to any element that already owns an action menu, without touching that element's own behaviour. Its cancelable `beforeOpen` event is the seam a later batch-selection slice needs. https://community.openproject.org/wp/AGILE-348 --- .../contextual-action-menu.controller.spec.ts | 722 ++++++++++++++++++ .../contextual-action-menu.controller.ts | 384 ++++++++++ 2 files changed, 1106 insertions(+) create mode 100644 frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.spec.ts create mode 100644 frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.ts 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 new file mode 100644 index 000000000000..9e9d9882fc37 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.spec.ts @@ -0,0 +1,722 @@ +//-- 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 { Application } from '@hotwired/stimulus'; +import { installElements } from '@openproject/stimulus-elements'; +import { ContextualActionMenu } from 'core-common/contextual-action-menu'; +import type ContextualActionMenuControllerType from './contextual-action-menu.controller'; + +describe('Contextual action menu controller', () => { + const nextFrame = () => new Promise((resolve) => requestAnimationFrame(() => resolve())); + + let application:Application; + let fixture:HTMLElement; + let ContextualActionMenuController:typeof ContextualActionMenuControllerType; + + beforeAll(async () => { + installElements(); + ({ default: ContextualActionMenuController } = await import('./contextual-action-menu.controller')); + }); + + beforeEach(() => { + fixture = document.createElement('div'); + document.body.appendChild(fixture); + + application = Application.start(); + application.register('contextual-action-menu', ContextualActionMenuController); + }); + + afterEach(async () => { + fixture.remove(); + await nextFrame(); + application.stop(); + vi.restoreAllMocks(); + }); + + // The real element is not registered in this environment, so + // the fixture provides an element of that tag name carrying the two members + // the presenter reads. That keeps the controller under test honest about + // resolving the menu through the elements blessing. + // + // `.menu-chrome` is a plain, non-interactive descendant of the menu itself + // (unlike the menuitem button, which `closestInteractiveElement` already + // excludes on its own) — it exists so a right-click on the menu's own + // decoration, rather than on one of its items, has something to target. + function renderCard() { + fixture.innerHTML = ` +
+ Subject + Story points + + + + + + + +
+ `; + + const card = fixture.querySelector('article')!; + const menu = fixture.querySelector('action-menu')!; + const overlay = fixture.querySelector('anchored-position')!; + const invoker = fixture.querySelector('#wp-1-menu-button')!; + + Object.assign(menu, { + overlay, + popoverElement: overlay, + invokerElement: invoker, + }); + Object.assign(overlay, { anchorElement: null, update: vi.fn() }); + + return { + card, menu, overlay, invoker, + }; + } + + // The presenter takes the overlay's `anchor` idref away for a pointer + // invocation and expresses the pointer as an offset from the card, so these + // two are what tell the three openings apart without measuring pixels: + // at the pointer, at the menu's own trigger, or not retargeted at all. + function pointerOffsetOf(overlay:HTMLElement) { + return overlay.hasAttribute('anchor') + ? null + : Number(overlay.getAttribute('alignment-offset')); + } + + function expectAnchoredAtInvoker(overlay:HTMLElement) { + expect(overlay.getAttribute('anchor')).toBe('wp-1-menu-button'); + expect(overlay.hasAttribute('alignment-offset')).toBe(false); + } + + // A card whose ordinary controller (backlogs-card, work-package-card, ...) + // never rendered an action menu at all: nothing to attach a contextual + // presentation to, so every invocation must fall through untouched. + function renderCardWithoutMenu() { + fixture.innerHTML = ` + + `; + + return { card: fixture.querySelector('article')! }; + } + + // Builds a detached subtree carrying its own overlay and + // invoker, wired the same way renderCard's is. Used to simulate a morph or + // turbo-stream update that replaces the action-menu subtree in place while + // the card and its controller stay connected. + function buildActionMenu(idSuffix:string) { + const template = document.createElement('template'); + template.innerHTML = ` + + + + + + + `; + + const menu = template.content.firstElementChild as HTMLElement; + const overlay = menu.querySelector('anchored-position')!; + const invoker = menu.querySelector('button')!; + + Object.assign(menu, { + overlay, + popoverElement: overlay, + invokerElement: invoker, + }); + Object.assign(overlay, { anchorElement: null, update: vi.fn() }); + + return { menu, overlay }; + } + + // A bare contextmenu event with no button held: the ordering Windows and + // Linux produce, where the event arrives at mouseup time and the release + // has already happened. + function contextMenu(target:Element, init:MouseEventInit = {}) { + const event = new MouseEvent('contextmenu', { + bubbles: true, cancelable: true, clientX: 50, clientY: 60, ...init, + }); + target.dispatchEvent(event); + return event; + } + + // The macOS/Chrome ordering: the contextmenu event arrives at mousedown + // time, while the button is still held, and the release follows separately. + function rightClickPress(target:Element, init:MouseEventInit = {}) { + target.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, button: 2, pointerType: 'mouse' })); + target.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, button: 2 })); + return contextMenu(target, init); + } + + // A touch long-press: Chrome emits no `mousedown` at all before the + // contextmenu it produces, so the gesture is a bare pointerdown followed by + // the event while the finger is still down. + function touchLongPress(target:Element, init:MouseEventInit = {}) { + target.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, pointerType: 'touch' })); + return contextMenu(target, init); + } + + function pointerRelease(target:Element = document.body) { + target.dispatchEvent(new PointerEvent('pointerup', { bubbles: true, button: 2 })); + target.dispatchEvent(new MouseEvent('mouseup', { bubbles: true, button: 2 })); + } + + function keydown(target:Element, key:string, init:KeyboardEventInit = {}) { + const event = new KeyboardEvent('keydown', { + key, bubbles: true, cancelable: true, ...init, + }); + target.dispatchEvent(event); + return event; + } + + // The pointer path opens from the release, never from the contextmenu event + // itself: a popover opened mid-gesture is light-dismissed by the trailing + // pointerup in a real browser. This spec cannot reproduce that dismissal — + // a synthetic contextmenu carries no pointer sequence for Chrome's + // light-dismiss to act on — so what it pins down is the ordering the fix + // depends on. The Selenium feature spec is the guard for the dismissal. + it('opens the menu at the pointer on the release, not before it', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + const event = rightClickPress(card, { clientX: 120, clientY: 240 }); + + expect(event.defaultPrevented).toBe(true); + expect(overlay.matches(':popover-open')).toBe(false); + + pointerRelease(card); + + expect(overlay.matches(':popover-open')).toBe(true); + expect(pointerOffsetOf(overlay)).toBeCloseTo(120 - card.getBoundingClientRect().left, 0); + }); + + // Windows and Linux deliver the contextmenu event at mouseup time, so no + // further release is coming and waiting for one would never open anything. + it('opens synchronously when the contextmenu arrives after the release', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + card.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, button: 2, pointerType: 'mouse' })); + card.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, button: 2 })); + pointerRelease(card); + + const event = contextMenu(card, { clientX: 120, clientY: 240 }); + + expect(event.defaultPrevented).toBe(true); + expect(overlay.matches(':popover-open')).toBe(true); + }); + + // Hardening: the release that clears the held-pointer flag is listened for + // on the document in capture phase, so a release that never reaches this + // element — the pointer left the card while held, or a descendant stopped + // propagation — still leaves the flag honest. Were it stale-true, the next + // right-click would take the asynchronous branch and wait for a release + // that had already happened. + it('clears the held-pointer flag from a release outside the element', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + card.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, button: 2, pointerType: 'mouse' })); + card.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, button: 2 })); + // The release lands elsewhere on the page and never travels through the + // card, so only a document-level listener can see it. + pointerRelease(document.body); + + const event = contextMenu(card, { clientX: 15, clientY: 25 }); + + expect(event.defaultPrevented).toBe(true); + expect(overlay.matches(':popover-open')).toBe(true); + }); + + // Regression: a long press must keep the browser's own menu. Suppressing it + // and then opening a popover mid-gesture leaves a tablet user with nothing + // at all — the trailing pointerup light-dismisses the popover, and the + // native menu has already been prevented. AGILE-348 covers mouse and + // keyboard; touch keeps today's behaviour untouched. + it('leaves the native long-press menu alone for touch', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + const event = touchLongPress(card, { clientX: 30, clientY: 40 }); + + expect(event.defaultPrevented).toBe(false); + expect(overlay.matches(':popover-open')).toBe(false); + + // Nor may the release that ends the gesture open anything after the fact. + pointerRelease(card); + + expect(overlay.matches(':popover-open')).toBe(false); + expectAnchoredAtInvoker(overlay); + }); + + it('does not announce an invocation it declines to make for touch', async () => { + const { card } = renderCard(); + await nextFrame(); + + const origins:string[] = []; + card.addEventListener('contextual-action-menu:beforeOpen', (event) => { + origins.push((event as CustomEvent<{ origin:string }>).detail.origin); + }); + + touchLongPress(card); + pointerRelease(card); + + expect(origins).toEqual([]); + }); + + // Regression: declining touch must not leave the controller stuck declining + // — a hybrid device's next mouse right-click has to work normally. + it('still opens for a mouse right-click after a touch long press', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + touchLongPress(card); + pointerRelease(card); + + const event = rightClickPress(card, { clientX: 300, clientY: 400 }); + pointerRelease(card); + + expect(event.defaultPrevented).toBe(true); + expect(overlay.matches(':popover-open')).toBe(true); + }); + + it('leaves the native context menu to interactive descendants', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + const event = contextMenu(card.querySelector('a')!); + + expect(event.defaultPrevented).toBe(false); + expect(overlay.matches(':popover-open')).toBe(false); + }); + + // AGILE-348 QA: the menu's own trigger is the one interactive descendant + // that must not fall through to the browser's menu — right-clicking the + // control whose whole job is to open this menu should open this menu. It + // opens where the trigger itself opens it, which is why the overlay keeps + // Primer's own anchoring here. + it('opens the menu at its trigger when the trigger is right-clicked', async () => { + const { overlay, invoker } = renderCard(); + await nextFrame(); + + const event = rightClickPress(invoker, { clientX: 300, clientY: 400 }); + pointerRelease(invoker); + + expect(event.defaultPrevented).toBe(true); + expect(overlay.matches(':popover-open')).toBe(true); + expectAnchoredAtInvoker(overlay); + }); + + // Following a pointer invocation, the trigger has to take its anchoring + // back: the offsets left by the right-click on the card would otherwise drag + // this opening away from the button it was aimed at. + it('moves an open contextual menu back to the trigger when the trigger is right-clicked', async () => { + const { card, overlay, invoker } = renderCard(); + await nextFrame(); + + rightClickPress(card, { clientX: 120, clientY: 240 }); + pointerRelease(card); + expect(pointerOffsetOf(overlay)).not.toBeNull(); + + rightClickPress(invoker, { clientX: 300, clientY: 400 }); + pointerRelease(invoker); + + expect(overlay.matches(':popover-open')).toBe(true); + expectAnchoredAtInvoker(overlay); + }); + + it('opens on a non-interactive descendant of the card', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + const plain = card.querySelector('.plain')!; + const event = rightClickPress(plain); + pointerRelease(plain); + + expect(event.defaultPrevented).toBe(true); + expect(overlay.matches(':popover-open')).toBe(true); + }); + + // A keyboard invocation opens the menu exactly where its own show button + // opens it (AGILE-348 QA): same menu, same card, so a second, card-relative + // position for it would be a difference with nothing behind it. Only the + // focus return stays contextual, which the presenter spec covers. + it('opens at the menu trigger for Shift+F10', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + const event = keydown(card, 'F10', { shiftKey: true }); + + expect(event.defaultPrevented).toBe(true); + expect(overlay.matches(':popover-open')).toBe(true); + expectAnchoredAtInvoker(overlay); + }); + + it('opens at the menu trigger for the Context Menu key', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + keydown(card, 'ContextMenu'); + + expect(overlay.matches(':popover-open')).toBe(true); + expectAnchoredAtInvoker(overlay); + }); + + it('swallows the contextmenu event a keyboard invocation may still emit', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + keydown(card, 'F10', { shiftKey: true }); + const event = contextMenu(card, { clientX: 500, clientY: 500 }); + + expect(event.defaultPrevented).toBe(true); + // Not re-anchored at the echo's coordinates: nothing reopened. + expectAnchoredAtInvoker(overlay); + }); + + // Regression: suppressing the echo must not eat the next genuine + // right-click, and must never eat one aimed at an interactive descendant + // whose native menu the user actually wants. + it('still opens for a real right-click after a keyboard invocation', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + keydown(card, 'F10', { shiftKey: true }); + rightClickPress(card, { clientX: 300, clientY: 400 }); + pointerRelease(card); + + expect(pointerOffsetOf(overlay)).toBeCloseTo(300 - card.getBoundingClientRect().left, 0); + expect((overlay as unknown as { anchorElement:HTMLElement|null }).anchorElement).toBe(card); + }); + + it('never swallows a right-click aimed at an interactive descendant', async () => { + const { card } = renderCard(); + await nextFrame(); + + keydown(card, 'F10', { shiftKey: true }); + const event = contextMenu(card.querySelector('a')!); + + expect(event.defaultPrevented).toBe(false); + }); + + it('ignores keyboard invocation from a focused descendant', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + const event = keydown(card.querySelector('a')!, 'F10', { shiftKey: true }); + + expect(event.defaultPrevented).toBe(false); + expect(overlay.matches(':popover-open')).toBe(false); + }); + + it('does not present when a beforeOpen listener cancels the invocation', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + card.addEventListener('contextual-action-menu:beforeOpen', (event) => event.preventDefault()); + const event = contextMenu(card); + + expect(event.defaultPrevented).toBe(false); + expect(overlay.matches(':popover-open')).toBe(false); + }); + + it('reports the invocation origin on the beforeOpen event', async () => { + const { card } = renderCard(); + await nextFrame(); + + const origins:string[] = []; + card.addEventListener('contextual-action-menu:beforeOpen', (event) => { + origins.push((event as CustomEvent<{ origin:string }>).detail.origin); + }); + + contextMenu(card); + keydown(card, 'ContextMenu'); + + expect(origins).toEqual(['pointer', 'keyboard']); + }); + + it('leaves the native context menu alone when the card has no action menu', async () => { + const { card } = renderCardWithoutMenu(); + await nextFrame(); + + // A thrown exception inside a DOM event listener is reported to `window` + // rather than re-thrown at the dispatchEvent() call site, so a crash here + // (menuElement is null without an action-menu) would otherwise leave + // defaultPrevented untouched and this assertion none the wiser. + const uncaughtErrors:unknown[] = []; + const onError = (event:ErrorEvent) => uncaughtErrors.push(event.error ?? event.message); + window.addEventListener('error', onError); + + const event = contextMenu(card); + await nextFrame(); + + window.removeEventListener('error', onError); + + expect(event.defaultPrevented).toBe(false); + expect(uncaughtErrors).toEqual([]); + }); + + it('leaves the native context menu alone on a right-click on the menu chrome itself', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + const event = contextMenu(card.querySelector('.menu-chrome')!); + + expect(event.defaultPrevented).toBe(false); + expect(overlay.matches(':popover-open')).toBe(false); + }); + + // Regression: a morph or turbo-stream update can replace the action-menu + // subtree in place while the card and its controller stay connected (no + // disconnect/connect cycle). `menuElement` is a live querySelector, so it + // already reports the new element on the very next access; the presenter + // must not keep driving the detached one. + it('rebuilds the presenter against a replaced action-menu element', async () => { + const { card, overlay: staleOverlay } = renderCard(); + await nextFrame(); + + contextMenu(card, { clientX: 10, clientY: 10 }); + expect(staleOverlay.matches(':popover-open')).toBe(true); + + const { menu: freshMenu, overlay: freshOverlay } = buildActionMenu('2'); + card.querySelector('action-menu')!.replaceWith(freshMenu); + + const event = contextMenu(card, { clientX: 20, clientY: 20 }); + + expect(event.defaultPrevented).toBe(true); + expect(freshOverlay.matches(':popover-open')).toBe(true); + + // The stale presenter was destroy()ed, not merely dropped: it restored + // the detached overlay's attributes and gave up its pointer anchor, + // rather than leaving both behind. + expect(staleOverlay.getAttribute('align')).toBe('end'); + expect(staleOverlay.getAttribute('anchor')).toBe('wp-1-menu-button'); + expect(staleOverlay.hasAttribute('alignment-offset')).toBe(false); + expect((staleOverlay as unknown as { anchorElement:HTMLElement|null }).anchorElement).toBeNull(); + + expect(pointerOffsetOf(freshOverlay)).toBeCloseTo(20 - card.getBoundingClientRect().left, 0); + }); + + // Regression: `contextualMenu` must be read when the release opens the + // menu, not captured back when the button went down — otherwise a morph + // landing while the button is held would be invisible to it. Swapping the + // element in exactly that gap is what makes the distinction observable. + it('reads the presenter afresh at open time, after a swap while the button is held', async () => { + const { card } = renderCard(); + await nextFrame(); + + rightClickPress(card, { clientX: 10, clientY: 10 }); + + const { menu: freshMenu, overlay: freshOverlay } = buildActionMenu('3'); + card.querySelector('action-menu')!.replaceWith(freshMenu); + + pointerRelease(card); + + expect(freshOverlay.matches(':popover-open')).toBe(true); + }); + + // Regression: a controller torn down while the button is still held must + // leave nothing armed. `card.remove()` runs before the release, so this is + // a real disconnect ahead of the pending open, not one that merely happens + // to win a scheduling race in this environment. + it('cancels a pending open if the controller disconnects before the release', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + rightClickPress(card, { clientX: 10, clientY: 10 }); + card.remove(); + await nextFrame(); + + // The release still happens somewhere; nothing may act on it. + pointerRelease(); + + expect(overlay.matches(':popover-open')).toBe(false); + expectAnchoredAtInvoker(overlay); + }); + + it('stops presenting and cleans up once the controller disconnects', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + contextMenu(card, { clientX: 10, clientY: 10 }); + expect(overlay.matches(':popover-open')).toBe(true); + + card.remove(); + await nextFrame(); + + const event = contextMenu(card, { clientX: 20, clientY: 20 }); + + expect(event.defaultPrevented).toBe(false); + // restoreOverlayState only runs from the presenter's destroy(), so a + // reverted `align` attribute is evidence disconnect() actually called it. + expect(overlay.getAttribute('align')).toBe('end'); + expectAnchoredAtInvoker(overlay); + }); + + // Regression: `canPresent()` and the `contextualMenu` getter are not + // evaluated atomically. `announceInvocation` dispatches `beforeOpen` + // synchronously, so a listener that removes the action menu without + // cancelling reaches the getter with `hasMenuElement` already false for + // this same invocation, even though `canPresent()` passed moments earlier. + it('does not throw when a beforeOpen listener removes the action menu without cancelling', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + // A thrown exception inside a DOM event listener is reported to `window` + // rather than re-thrown at the call site, so this is what makes a crash + // observable. + const uncaughtErrors:unknown[] = []; + const onError = (event:ErrorEvent) => uncaughtErrors.push(event.error ?? event.message); + window.addEventListener('error', onError); + + card.addEventListener('contextual-action-menu:beforeOpen', () => { + card.querySelector('action-menu')?.remove(); + }); + + contextMenu(card); + + window.removeEventListener('error', onError); + + expect(uncaughtErrors).toEqual([]); + expect(overlay.matches(':popover-open')).toBe(false); + expectAnchoredAtInvoker(overlay); + }); + + // Regression: once the action menu is gone for good, `canPresent()`'s own + // `!hasMenuElement` guard stops every later invocation from ever reaching + // the `contextualMenu` getter again — so the invocation in which the menu + // disappears is the last chance to retire a presenter left over from an + // earlier, successful invocation. + it('destroys an already-open presenter when a later beforeOpen listener removes the menu', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + // First invocation: opens for real, leaving a presenter bound to this + // action-menu element with an open pointer anchor. + contextMenu(card, { clientX: 10, clientY: 10 }); + expect(overlay.matches(':popover-open')).toBe(true); + + // Second invocation: canPresent() passes (the menu is still there), then + // this listener removes that same element before the getter runs. + card.addEventListener('contextual-action-menu:beforeOpen', () => { + card.querySelector('action-menu')?.remove(); + }); + + contextMenu(card, { clientX: 20, clientY: 20 }); + + expect(overlay.getAttribute('align')).toBe('end'); + expectAnchoredAtInvoker(overlay); + }); + + // Regression: only one open may be pending at a time. A second right-click + // arriving before the first one's release has to retire it, so the release + // opens the menu once, at the newer position — not twice, and not at the + // abandoned one. Counting the presenter's own calls is what distinguishes + // "the newer open ran last" from "only the newer open ran at all". + it('supersedes a pending open with a second right-click, keeping the later one', async () => { + const { card } = renderCard(); + await nextFrame(); + + const openAtPoint = vi.spyOn(ContextualActionMenu.prototype, 'openAtPoint'); + + rightClickPress(card, { clientX: 10, clientY: 10 }); + // Still held: a second contextmenu for the same press, as a re-dispatch + // or a browser repeating the event would produce. + contextMenu(card, { clientX: 20, clientY: 20 }); + pointerRelease(card); + + expect(openAtPoint).toHaveBeenCalledTimes(1); + expect(openAtPoint).toHaveBeenCalledWith(20, 20, card); + }); + + // Regression: a keyboard invocation arriving while a right-click waits for + // its release supersedes it. The menu belongs on the card, and the + // abandoned pointer position must not claim it back when the button comes + // up a moment later. + it('supersedes a pending open with a keyboard invocation', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + rightClickPress(card, { clientX: 10, clientY: 10 }); + keydown(card, 'F10', { shiftKey: true }); + pointerRelease(card); + + expect(overlay.matches(':popover-open')).toBe(true); + expectAnchoredAtInvoker(overlay); + }); + + // Not every release is reported as a pointer event: a gesture synthesised + // by a driver or by other page code may only produce the mouse one, and + // the menu still has to open from it. + it('opens on a mouseup when the release brings no pointerup', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + rightClickPress(card, { clientX: 10, clientY: 10 }); + card.dispatchEvent(new MouseEvent('mouseup', { bubbles: true, button: 2 })); + + expect(overlay.matches(':popover-open')).toBe(true); + }); + + // Regression: a cancelled gesture (a touch or pen taken over by a scroll, + // a drag starting) never produces the release the open is waiting for, and + // must not be left armed for whatever release comes next. + it('drops a pending open when the gesture is cancelled', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + rightClickPress(card, { clientX: 10, clientY: 10 }); + card.dispatchEvent(new PointerEvent('pointercancel', { bubbles: true })); + pointerRelease(card); + + expect(overlay.matches(':popover-open')).toBe(false); + expectAnchoredAtInvoker(overlay); + }); + + // Regression: a release that never arrives — the pointer left the window, + // the gesture was cancelled — leaves an open armed against a gesture that + // is over. It must not open a menu on the next, unrelated click somewhere + // else on the page. + it('does not open on a later unrelated click when the release never arrives', async () => { + const { card, overlay } = renderCard(); + await nextFrame(); + + rightClickPress(card, { clientX: 10, clientY: 10 }); + + // A whole later gesture, elsewhere: this one's own press is what has to + // retire the abandoned open before its release lands. + document.body.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true })); + pointerRelease(); + + expect(overlay.matches(':popover-open')).toBe(false); + expectAnchoredAtInvoker(overlay); + }); +}); diff --git a/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.ts b/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.ts new file mode 100644 index 000000000000..5f122e29c92f --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/contextual-action-menu.controller.ts @@ -0,0 +1,384 @@ +//-- 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 { Controller } from '@hotwired/stimulus'; +import type { ActionMenuElement } from '@openproject/primer-view-components/app/components/primer/alpha/action_menu/action_menu_element'; +import { ContextualActionMenu } from 'core-common/contextual-action-menu'; +// Keep using the strict variant — a drag-gating variant treats far more of the +// card as interactive and would hand most right-clicks back to the browser. +import { closestInteractiveElement } from 'core-common/interactive-element-helper'; + +type InvocationOrigin = 'pointer'|'keyboard'; + +function isContextMenuKey(event:KeyboardEvent):boolean { + return event.key === 'ContextMenu' || (event.shiftKey && event.key === 'F10'); +} + +/** + * Presents the action menu of the element it is attached to as a context menu. + * + * Attach it alongside whatever controller owns the element's ordinary + * behaviour; this controller adds only the contextual entry points and never + * touches navigation, selection, or the menu's contents. Listeners of the + * cancelable `contextual-action-menu:beforeOpen` event can suppress or prepare + * an invocation — the seam a later batch-selection slice uses. + * + * `beforeOpen` means "an invocation was attempted", not "a menu is about to + * appear". It fires at gesture time, because whether to call preventDefault() + * on the browser's own context menu has to be decided there and then, while + * the pointer path opens the menu later, at the release. So a menu need never + * follow the event: the gesture may be cancelled, the pointer may leave the + * window, or a second invocation may supersede the first — and a right-click + * repeated before its release fires `beforeOpen` twice for the one menu that + * eventually opens. A listener applying selection or other preparatory state + * must be idempotent and must not assume a matching "opened" ever arrives. + */ +export default class ContextualActionMenuController extends Controller { + static elements = { menu: 'action-menu' }; + + // Provided by the stimulus-elements blessing; absent when the host element + // has no action menu, in which case every invocation falls through to the + // browser's own context menu. + declare readonly menuElement:ActionMenuElement; + declare readonly hasMenuElement:boolean; + + private abortController?:AbortController; + private presenter?:ContextualActionMenu; + // The element the current presenter was built for. `menuElement` is a live + // getter (a fresh querySelector on every access, see the stimulus-elements + // blessing), not a stable reference, so this is what lets the getter below + // notice a morph or turbo-stream update that swapped the action-menu + // subtree while this controller's host element stayed connected. + private presenterMenu?:ActionMenuElement; + // Set by a handled keyboard invocation, cleared by the echo it may produce + // or by any pointer press. A real right-click is always preceded by a + // mousedown; the browser's keyboard-generated contextmenu event never is, + // so this needs no timing window and cannot swallow a genuine right-click. + private expectKeyboardEcho = false; + // Whether a pointer is currently held down on this element. Read by + // onContextMenu to tell the two platform orderings apart — see the comment + // there. Set from `pointerdown` as well as `mousedown` because Chrome emits + // no `mousedown` at all before a long-press contextmenu; the release + // listeners sit on the document in capture phase so the flag does not depend + // on the release bubbling back to this element (a native HTML5 drag ends in + // `dragend`, not `mouseup`) nor on descendants leaving propagation alone. + private pointerIsDown = false; + // The kind of pointer that last pressed this element, as reported by + // `pointerdown`. Read by onContextMenu to keep the browser's own long-press + // menu for touch and pen — see the comment there. + private pointerType = ''; + // The pointer path's open, waiting for the release that ends the gesture; + // aborting it removes the release listeners. At most one is ever pending. + private pendingOpen?:AbortController; + + connect():void { + this.abortController = new AbortController(); + const { signal } = this.abortController; + + this.element.addEventListener('contextmenu', this.onContextMenu, { signal }); + this.element.addEventListener('keydown', this.onKeydown, { signal }); + this.element.addEventListener('pointerdown', this.onPointerDown, { signal }); + this.element.addEventListener('mousedown', this.onPointerPress, { signal }); + + const doc = this.element.ownerDocument; + doc.addEventListener('pointerup', this.onPointerRelease, { capture: true, signal }); + doc.addEventListener('mouseup', this.onPointerRelease, { capture: true, signal }); + } + + disconnect():void { + this.cancelPendingOpen(); + this.pointerIsDown = false; + this.pointerType = ''; + + this.abortController?.abort(); + this.abortController = undefined; + this.presenter?.destroy(); + this.presenter = undefined; + this.presenterMenu = undefined; + } + + private readonly onPointerDown = (event:PointerEvent):void => { + this.pointerType = event.pointerType; + this.onPointerPress(); + }; + + // Any pointer press means the next contextmenu event, if one comes, is a + // real right-click rather than the echo of a keyboard invocation. Both + // `pointerdown` and `mousedown` route here: a mouse fires both, a long press + // fires only the former, and a driver-synthesised gesture may fire only the + // latter. + private readonly onPointerPress = ():void => { + this.expectKeyboardEcho = false; + this.pointerIsDown = true; + }; + + private readonly onPointerRelease = ():void => { + this.pointerIsDown = false; + }; + + private readonly onContextMenu = (event:MouseEvent):void => { + if (this.expectKeyboardEcho) { + this.expectKeyboardEcho = false; + + // Browsers that ignore the keydown's preventDefault still emit this + // event against the focused card. The menu is already open where its + // own show button opens it, so suppress the browser menu without + // reopening anything. A right-click on a descendant is never this echo. + if (event.target === this.element) { + event.preventDefault(); + return; + } + } + + // Touch and pen reach this event through a long press, and AGILE-348 + // covers the mouse and keyboard invocations only: there is no touch-sized + // design for the menu, no way to reach it other than the gesture the + // browser already spends on its own menu, and no coverage for either. + // Bailing here — before preventDefault(), and before announcing an + // invocation that is not going to happen — leaves the native long-press + // menu exactly as it is today. Replacing it is a deliberate slice of its + // own, not a side effect of this one. An empty pointerType means no + // pointerdown was seen and the gesture is treated as a mouse. + if (this.pointerType !== '' && this.pointerType !== 'mouse') { + return; + } + + if (!this.canPresent(event.target)) { + return; + } + + // Decided from the DOM as the gesture found it, before `beforeOpen` gets + // to run: a listener may replace the action-menu subtree, and where this + // invocation belongs was settled by where the user right-clicked. + const atInvoker = this.invokerTargeted(event.target); + + if (!this.announceInvocation('pointer')) { + return; + } + + event.preventDefault(); + + // Chrome light-dismisses a `popover="auto"` that was opened while the + // right-click gesture is still in flight: the trailing pointerup lands + // outside the freshly opened popover, counts as an outside click, and + // closes it again within the same gesture (`toggle open` followed by a + // non-cancelable `toggle closed`). The keyboard path has no trailing + // pointer event, which is why only this one is affected. A timer does not + // fix it — Chrome 150 runs a setTimeout(0) scheduled here several + // milliseconds *before* it dispatches the gesture's own pointerup — so + // the open is keyed off the real release instead. + // + // The two platform orderings need opposite treatment, and `pointerIsDown` + // is what tells them apart without sniffing the platform: + // * macOS/Chrome fires contextmenu at mousedown time. The release is + // still pending, so the open has to wait for it. + // * Windows/Linux fire contextmenu at mouseup time. The release has + // already landed and no further one is coming, so opening now is both + // correct and necessary — waiting would hang forever. + // Do not "simplify" either branch into a single inline call or a timer. + // + // Read the presenter inside the closure, never here: a morph that swaps or + // removes the action-menu while the button is held has to be visible to + // the getter's invalidation at open time. + // + // A right-click on the show button itself opens the menu where the button + // opens it. The pointer is already on the control the menu belongs to, so + // there is nowhere better to put it, and the position stays the one that + // button has always produced. + const { clientX, clientY } = event; + const open = atInvoker + ? ():void => this.contextualMenu?.openAtInvoker(this.element) + : ():void => this.contextualMenu?.openAtPoint(clientX, clientY, this.element); + + if (this.pointerIsDown) { + this.openOnRelease(open); + } else { + open(); + } + }; + + private readonly onKeydown = (event:KeyboardEvent):void => { + if (!isContextMenuKey(event) || event.target !== this.element) { + return; + } + + if (!this.canPresent(event.target) || !this.announceInvocation('keyboard')) { + return; + } + + // A keyboard invocation supersedes a right-click still waiting for its + // release: the menu belongs where the show button puts it, not at the + // abandoned pointer position, and only one of the two may open. + this.cancelPendingOpen(); + + this.expectKeyboardEcho = true; + event.preventDefault(); + // Opened exactly where the More button opens it. A keyboard user reaching + // the menu through Shift+F10 and one reaching it through the button are + // looking at the same card, and a second, card-relative position for the + // same menu is a difference with nothing behind it. Only the focus return + // stays contextual: the card invoked the menu, so the card gets focus back. + // + // Deliberately synchronous: nothing is racing this one, and callers + // (including this controller's own tests) observe the menu immediately. + this.contextualMenu?.openAtInvoker(this.element); + }; + + /** + * Runs the given open from the pointer release that ends the current + * gesture, rather than from the contextmenu event itself. See onContextMenu + * for why the release, and not a timer, is what this waits on. + */ + private openOnRelease(openMenu:() => void):void { + // At most one open is ever pending: a second right-click arriving while + // this one waits retires it, so the release opens the menu once, at the + // latest invocation's position. + this.cancelPendingOpen(); + + const controllerSignal = this.abortController?.signal; + if (!controllerSignal || controllerSignal.aborted) { + return; + } + + const pending = new AbortController(); + this.pendingOpen = pending; + const { signal } = pending; + const doc = this.element.ownerDocument; + + const open = ():void => { + this.cancelPendingOpen(); + + // disconnect() cancels this explicitly; the controller's own signal is + // the backstop for any path where that tracking slips. Never open + // against a disconnected controller. + if (controllerSignal.aborted) { + return; + } + + openMenu(); + }; + + const cancel = ():void => this.cancelPendingOpen(); + + // On the document, capture phase: the release opens the menu wherever it + // lands (the pointer may have left the card while the button was held) + // and before anything else reacts to it. + doc.addEventListener('pointerup', open, { capture: true, signal }); + doc.addEventListener('mouseup', open, { capture: true, signal }); + // If the release never arrives at all — the pointer left the window, the + // gesture was cancelled — the pending open must not survive into some + // later, unrelated click. The next gesture's own press retires it. + doc.addEventListener('pointerdown', cancel, { capture: true, signal }); + doc.addEventListener('pointercancel', cancel, { capture: true, signal }); + } + + private cancelPendingOpen():void { + this.pendingOpen?.abort(); + this.pendingOpen = undefined; + } + + private canPresent(target:EventTarget|null):boolean { + if (!this.hasMenuElement || !(target instanceof Element)) { + return false; + } + + // The menu's own trigger is the one interactive descendant that does not + // keep the browser's menu: a right-click on the control whose whole job is + // to open this menu should open this menu. It has to be tested before the + // guards below, both of which would otherwise claim it. + if (this.invokerTargeted(target)) { + return true; + } + + // Right-clicking inside the open menu, or on a link, button, field, or + // other meaningful control on the card, keeps the browser's own menu. + // `closestInteractiveElement` stops at the host element, so the host's own + // tab stop does not count as an interactive descendant. + if (this.menuElement.contains(target)) { + return false; + } + + return closestInteractiveElement(target, this.element) === null; + } + + // Whether the gesture landed on the menu's show button (or something inside + // it, such as the icon). Such an invocation opens the menu where the button + // itself would, rather than at the pointer. + private invokerTargeted(target:EventTarget|null):boolean { + if (!this.hasMenuElement || !(target instanceof Node)) { + return false; + } + + return this.menuElement.invokerElement?.contains(target) ?? false; + } + + // Announces an *attempted* invocation, not an imminent menu: on the pointer + // path this fires at gesture time while the open waits for the release, so + // it may fire without a menu ever appearing, and twice for one that does. + // See the class docstring for what a listener may rely on. + private announceInvocation(origin:InvocationOrigin):boolean { + const event = this.dispatch('beforeOpen', { + detail: { origin }, + cancelable: true, + }); + + return !event.defaultPrevented; + } + + private get contextualMenu():ContextualActionMenu|undefined { + const menu = this.hasMenuElement ? this.menuElement : undefined; + + // A morph or turbo-stream update can replace the action-menu subtree, or + // remove it outright, without this controller ever disconnecting — and + // `canPresent()` having passed for this invocation before that happened + // is exactly what lets this getter still run with no menu left at all + // (a synchronous `beforeOpen` listener that drops the element without + // cancelling is one way that happens). This comparison runs before the + // "no menu" return below on purpose: a vanished menu must retire the + // stale presenter (restoring the anchoring it took over) + // just as much as a swapped one does, and once the menu is gone, + // `canPresent()`'s own guard stops every later call from ever reaching + // this getter again — so this is the last chance to clean up. + if (this.presenter && this.presenterMenu !== menu) { + this.presenter.destroy(); + this.presenter = undefined; + this.presenterMenu = undefined; + } + + if (!menu) { + return undefined; + } + + if (!this.presenter) { + this.presenter = new ContextualActionMenu(menu); + this.presenterMenu = menu; + } + + return this.presenter; + } +} From 007f09f7220d4dbe9927f86b7b021caf0b086039 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 29 Jul 2026 00:02:16 +0200 Subject: [PATCH 3/3] [AGILE-348] Open Backlogs card menu contextually Right-clicking a backlog card is the gesture people reach for, and the card already carries the menu its More button opens. Non-movable cards opt in too: their one action is still worth reaching contextually. https://community.openproject.org/wp/AGILE-348 --- .../work_package_card_list_item_component.rb | 15 +- .../backlogs/bucket_component_spec.rb | 2 +- .../backlogs/sprint_component_spec.rb | 2 +- .../work_package_card_list_component_spec.rb | 4 +- ...k_package_card_list_item_component_spec.rb | 11 +- .../work_packages/card_context_menu_spec.rb | 219 ++++++++++++++++++ .../backlogs/spec/support/pages/backlog.rb | 146 +++++++++++- 7 files changed, 385 insertions(+), 14 deletions(-) create mode 100644 modules/backlogs/spec/features/work_packages/card_context_menu_spec.rb 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 3f3ade7d2b32..0bd8bfedb7c4 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 @@ -74,7 +74,9 @@ def card_arguments def card_data data = { story: true, - controller: "backlogs--work-package", + # Non-movable cards opt in too: they have no move actions, but their + # singular menu is still worth reaching contextually. + controller: "backlogs--work-package contextual-action-menu", backlogs__work_package_id_value: work_package.id, backlogs__work_package_display_id_value: work_package.display_id, backlogs__work_package_split_url_value: split_url, @@ -86,13 +88,14 @@ def card_data data.merge(sortable_lists__item_target: "preview handle") end - # @return [Hash] ARIA wiring announcing the card's Enter activation without - # claiming button or draggable semantics. Lives in the subclass because - # only here is the `backlogs--work-package` Enter handler attached; the - # base card is focusable for styling alone and must not claim a shortcut. + # @return [Hash] ARIA wiring announcing the card's Enter activation and its + # context-menu shortcut without claiming button or draggable semantics. + # Shift+F10 is the conventional context-menu command in the WAI-ARIA APG + # and is worth announcing; the dedicated Context Menu key is left out + # because it needs no discovery — pressing it is its own affordance. def card_aria { - keyshortcuts: "Enter", + keyshortcuts: "Enter Shift+F10", label: work_package.to_fs(:caption) } end diff --git a/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb b/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb index 83f6af538be2..e64244994229 100644 --- a/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb @@ -158,7 +158,7 @@ def render_component end expect(rendered_component).to have_css(".op-work-package-card") do |card| - expect(card["data-controller"]).to eq("backlogs--work-package") + expect(card["data-controller"].split).to include("backlogs--work-package") expect(card["data-sortable-lists--item-target"]).to eq("preview handle") expect(card["data-backlogs--work-package-split-url-value"]) .to end_with(project_backlogs_backlog_details_path(project, work_package)) diff --git a/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb b/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb index a908e5b917d9..1efc830bd4c8 100644 --- a/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb @@ -125,7 +125,7 @@ def menu_items end expect(rendered_component).to have_css(".Box-row#work_package_#{work_package1.id} .op-work-package-card") do |card| - expect(card["data-controller"]).to eq("backlogs--work-package") + expect(card["data-controller"].split).to include("backlogs--work-package") expect(card["data-sortable-lists--item-target"]).to eq("preview handle") expect(card["data-backlogs--work-package-display-id-value"]).to eq(work_package1.display_id.to_s) end diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb index 78ce082985fc..5c521c164fba 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb @@ -105,8 +105,8 @@ def render_component(work_packages:, container:, drag_and_drop:) work_package = work_packages.first expect(rendered_component).to have_css( - ".Box-row#work_package_#{work_package.id}[data-controller='sortable-lists--item'] " \ - ".op-work-package-card[data-controller='backlogs--work-package']" + ".Box-row#work_package_#{work_package.id}[data-controller~='sortable-lists--item'] " \ + ".op-work-package-card[data-controller~='backlogs--work-package']" ) end 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 b7743adb93a6..3e16fc243b6a 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 @@ -125,6 +125,12 @@ expect(render_inline(item.card)).to have_css(".op-work-package-card.Box-card--clickable") expect(page).to have_no_css(".Box-card--draggable") end + + it "still wires the contextual-action-menu controller" do + expect(render_inline(item.card)).to have_css( + ".op-work-package-card[data-controller~='contextual-action-menu']" + ) + end end end @@ -172,6 +178,7 @@ it "wires the card as a Backlogs work package" do expect(rendered_card).to have_css( ".op-work-package-card[data-controller~='backlogs--work-package']" \ + "[data-controller~='contextual-action-menu']" \ "[data-backlogs--work-package-id-value='#{work_package.id}']" \ "[data-backlogs--work-package-display-id-value='#{work_package.display_id}']" \ "[data-backlogs--work-package-full-url-value='#{work_package_path(work_package)}']" \ @@ -180,12 +187,12 @@ ) end - it "announces Enter activation to assistive tech without a button or drag role" do + it "announces Enter and Shift+F10 to assistive tech without a button or drag role" do expect(rendered_card).to have_css( ".op-work-package-card", role: "article", aria: { - keyshortcuts: "Enter", + keyshortcuts: "Enter Shift+F10", label: work_package.to_fs(:caption), roledescription: nil } 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 new file mode 100644 index 000000000000..547c74bdfd6f --- /dev/null +++ b/modules/backlogs/spec/features/work_packages/card_context_menu_spec.rb @@ -0,0 +1,219 @@ +# 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" + +# Selenium, not Cuprite: the presenter's behaviour hangs on a trusted +# `contextmenu` and on a real pointer sequence for the browser's light-dismiss +# to act on, neither of which a synthesised gesture reproduces. +RSpec.describe "Open a backlog card's action menu contextually", + :js, :selenium do + let!(:project) do + create(:project, + types: [type], + enabled_module_names: %w(work_package_tracking backlogs)) + end + let(:manage_sprint_items_role) do + create(:project_role, + permissions: %i(view_sprints + manage_sprint_items + view_work_packages + edit_work_packages)) + end + + let(:type) { create(:type) } + + let!(:sprint) { create(:sprint, project:) } + let!(:sprint_wp) { create(:work_package, sprint:, type:, project:) } + # Declared here, ahead of the visit below, because RSpec runs an outer + # group's `before` hooks before an inner group's `let!`: cards created by a + # nested group would not exist yet when the page renders. Groups that need a + # scrollable list override this. + let!(:extra_sprint_wps) { [] } + + let(:backlogs_page) { Pages::Backlog.new(project) } + + current_user do + create(:user, member_with_roles: { project => manage_sprint_items_role }) + end + + before do + backlogs_page.visit! + 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.within_work_package_context_menu(sprint_wp) do |menu| + expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details"), focused: true) + end + + backlogs_page.expect_menu_anchored_contextually(sprint_wp) + end + + # Escape has to travel through whatever currently holds focus. Sending it to + # the card element would make the driver focus the card first, closing the + # menu by focus-out and leaving focus on the card no matter what the + # presenter does — the assertion would pass against a broken implementation. + it "returns focus to the card when the menu closes" do + backlogs_page.right_click_work_package_card(sprint_wp) + + 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) + end + + backlogs_page.send_keys_to_focused_element(:escape) + + backlogs_page.expect_no_work_package_context_menu(sprint_wp) + backlogs_page.expect_work_package_card_focused(sprint_wp) + end + + # The first opening exercises the deferred fetch; the second exercises the + # already-loaded menu, whose first-item focus runs through a different + # Primer code path (the toggle handler rather than include-fragment-replaced). + it "focuses the first action again when an already loaded menu reopens" do + backlogs_page.right_click_work_package_card(sprint_wp) + + 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) + end + + backlogs_page.send_keys_to_focused_element(:escape) + backlogs_page.expect_no_work_package_context_menu(sprint_wp) + + backlogs_page.right_click_work_package_card(sprint_wp) + + 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) + end + end + + # Selenium's Element Send Keys focuses its receiver, so the card needs no + # separate focus step. The menu has to take focus here just as it does for a + # right-click: that is what the card's aria-keyshortcuts promise is worth. + # + # It opens where the More button opens it, not at a second, card-relative + # 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.within_work_package_context_menu(sprint_wp) do |menu| + expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details"), focused: true) + end + + backlogs_page.expect_menu_anchored_at_button(sprint_wp) + end + + # The card's own actions button keeps no native context menu: right-clicking + # the control that exists to open this menu opens this menu, at the position + # that button has always produced. Every other interactive descendant — the + # subject link, fields, clipboard controls — still falls through to the + # browser, which the unit specs pin down. + it "opens the menu at its trigger when the trigger is right-clicked" do + backlogs_page.right_click_work_package_menu_button(sprint_wp) + + 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) + end + + backlogs_page.expect_menu_anchored_at_button(sprint_wp) + end + + # The presenter rewrites the overlay's anchoring for one invocation; if it + # failed to restore it, the More button would reopen the menu at the last + # pointer position instead of beside the button. + it "leaves the More button menu working after a contextual invocation" do + backlogs_page.right_click_work_package_card(sprint_wp) + + 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) + end + + backlogs_page.expect_menu_anchored_contextually(sprint_wp) + + backlogs_page.send_keys_to_focused_element(:escape) + backlogs_page.expect_no_work_package_context_menu(sprint_wp) + + backlogs_page.within_work_package_menu(sprint_wp) do |menu| + expect(menu).to have_selector(:menuitem, text: I18n.t(:"js.button_open_details")) + # Still inside the block: the More button has to be anchoring this very + # opening, not merely be back in place after it was dismissed again. + backlogs_page.expect_menu_anchored_at_button(sprint_wp) + end + end + + # Regression (AGILE-348 QA): the menu used to be anchored on a synthetic + # `position: fixed` element placed at the pointer. Primer does reposition the + # overlay on scroll, but a fixed element never moves in viewport space, so + # every reposition recomputed the same point while the card scrolled out from + # under it. Only a real browser can show that: the anchoring the unit specs + # can read is identical either way. + describe "with the page scrolled while the menu is open" do + # Enough cards for the page to actually overflow. Without them the scroll + # below is a no-op and the assertion would hold against any implementation, + # which is what the `moved` guard makes impossible to do silently. + # rubocop:disable FactoryBot/ExcessiveCreateList + let!(:extra_sprint_wps) { create_list(:work_package, 25, sprint:, type:, project:) } + # rubocop:enable FactoryBot/ExcessiveCreateList + + let(:scroll_distance) { 120 } + + before do + # The sprint list arrives in a lazily loaded turbo-frame, and this many + # cards take long enough to render that right-clicking one straight after + # the visit hands Selenium a node about to be replaced. + backlogs_page.expect_sprint_work_package_count(sprint, extra_sprint_wps.size + 1) + end + + it "keeps the menu with the card" do + backlogs_page.right_click_work_package_card(sprint_wp) + + 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) + end + + offset_before = backlogs_page.work_package_menu_offset_from_card(sprint_wp) + moved = backlogs_page.scroll_past_work_package_card(sprint_wp, distance: scroll_distance) + + # The page really did scroll, and the card really did move with it, so + # what follows is a repositioning rather than a pair of untouched rects. + expect(moved).to be_within(scroll_distance / 4).of(scroll_distance) + + # Primer repositions from a scroll listener and applies it a frame later, + # so the menu settles back onto the card rather than being there already. + wait_for { backlogs_page.work_package_menu_offset_from_card(sprint_wp)[:top] } + .to be_within(2).of(offset_before[:top]) + expect(backlogs_page.work_package_menu_offset_from_card(sprint_wp)[:left]) + .to be_within(2).of(offset_before[:left]) + end + end +end diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index e94537fba8fe..d19b837bb759 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -340,6 +340,135 @@ def within_work_package_menu(work_package, &) dismiss_menu(work_package) end + def work_package_card(work_package) + find(work_package_card_selector(work_package)) + end + + # Right-clicks near the card's top-left corner: the offset keeps the + # pointer off the subject link and the actions menu button, both of which + # keep their native context menu on purpose. + def right_click_work_package_card(work_package) + work_package_card(work_package).right_click(x: 6, y: 6, offset: :position) + end + + # The card's own actions button, right-clicked. It is the one interactive + # descendant that does not keep the browser's context menu: the control + # whose whole job is to open this menu opens it here too. + def right_click_work_package_menu_button(work_package) + within_work_package(work_package) do + find(:button, accessible_name: "Work package actions").right_click + end + end + + # Sends keys to whatever currently holds focus. Capybara's + # `element.send_keys` focuses its receiver first, which would destroy the + # very focus state a menu-dismissal assertion is trying to observe. + def send_keys_to_focused_element(*keys) + page.driver.browser.action.send_keys(*keys).perform + end + + def send_work_package_card_keys(work_package, keys) + work_package_card(work_package).send_keys(*keys) + end + + def within_work_package_context_menu(work_package, &) + within(menu_owner_overlay_selector(work_package)) do + yield page.find(:menu) + end + end + + def expect_no_work_package_context_menu(work_package) + expect(page) + .to have_no_css(menu_owner_overlay_selector(work_package), visible: :visible) + end + + def expect_work_package_card_focused(work_package) + expect(page) + .to have_css(work_package_card_selector(work_package), focused: true) + end + + # The presenter takes the overlay's `anchor` idref away for the duration of + # a contextual invocation, so its absence is what distinguishes a menu + # opened at the pointer (or on the card) from one opened at the More + # button, without measuring pixels. + # `page.document` rather than `page`: both are called while the menu is + # open, and the More button case runs inside a `within` scoped to the menu + # itself, where a plain `page` query would search the menu's descendants. + def expect_menu_anchored_contextually(work_package) + expect(page.document) + .to have_css("#{menu_owner_overlay_selector(work_package)}:not([anchor])") + end + + def expect_menu_anchored_at_button(work_package) + expect(page.document) + .to have_css("#{menu_owner_overlay_selector(work_package)}[anchor]") + end + + # Where the open menu sits relative to its card, rounded to whole pixels. + # A menu anchored on the card keeps this constant however the page scrolls; + # one pinned to a point in the viewport drifts by the scroll distance. + def work_package_menu_offset_from_card(work_package) + offset = page.evaluate_script(<<~JS) + (() => { + const overlay = document.querySelector('#{menu_owner_overlay_selector(work_package)}'); + const card = document.querySelector('#{work_package_card_selector(work_package)}'); + + if (!overlay || !card) { return null; } + + const overlayRect = overlay.getBoundingClientRect(); + const cardRect = card.getBoundingClientRect(); + + return { + top: Math.round(overlayRect.top - cardRect.top), + left: Math.round(overlayRect.left - cardRect.left) + }; + })() + JS + + raise "No open menu for work package #{work_package.id}" if offset.nil? + + offset.symbolize_keys + end + + # Scrolls whatever actually scrolls this card — the window on a short page, + # an overflowing ancestor otherwise — and answers how far the card moved in + # viewport space, direction ignored, so a caller can tell a real scroll from + # a silent no-op on a page that never overflowed. + def scroll_past_work_package_card(work_package, distance:) + page.evaluate_script(<<~JS) + (() => { + const card = document.querySelector('#{work_package_card_selector(work_package)}'); + const before = card.getBoundingClientRect().top; + + let scroller = document.scrollingElement; + + for (let node = card.parentElement; node; node = node.parentElement) { + const overflowY = getComputedStyle(node).overflowY; + + if (/(auto|scroll)/.test(overflowY) && node.scrollHeight > node.clientHeight) { + scroller = node; + break; + } + } + + // Whichever direction has room: opening the menu scrolls the card + // into view, which may already have the scroller at one end, and + // scrolling further that way is a silent no-op. + const room = scroller.scrollHeight - scroller.clientHeight; + const target = scroller.scrollTop + #{distance} <= room + ? scroller.scrollTop + #{distance} + : scroller.scrollTop - #{distance}; + + // Explicitly instant: a scroll container inheriting + // `scroll-behavior: smooth` would animate the change, and the rect + // read below would still see the old position. + scroller.scrollTo({ top: Math.max(0, target), behavior: 'instant' }); + + return Math.abs(Math.round(before - card.getBoundingClientRect().top)); + })() + JS + end + def click_in_work_package_menu(work_package, item_name, wait: true) within_work_package_menu(work_package) do |submenu| wait_for_turbo_stream(wait:) do @@ -763,6 +892,20 @@ def work_package_selector(work_package) test_selector("work-package-#{work_package.id}") end + # `.op-work-package-card` is the class the card component itself owns; + # `.Box-card` is a Primer modifier it happens to compose today, so it is + # not something a Backlogs page object should be matching on. + def work_package_card_selector(work_package) + "#{work_package_selector(work_package)} .op-work-package-card" + end + + # Generic over every menu owner `dismiss_menu` handles (sprint, bucket, + # work package): `dom_target` needs nothing work-package-specific, so this + # is the one place that convention lives. + def menu_owner_overlay_selector(menu_owner) + "##{ActionView::RecordIdentifier.dom_target(menu_owner, :menu)}-overlay" + end + def draggable_work_package_selector(work_package) "#{work_package_selector(work_package)}[data-sortable-lists--item-id-value]" end @@ -1173,8 +1316,7 @@ def open_move_submenu(menu) end def dismiss_menu(menu_owner) - overlay_id = "#{ActionView::RecordIdentifier.dom_target(menu_owner, :menu)}-overlay" - selector = "##{overlay_id}" + 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)