From f4b2c6d8a8282d00d09a6729f7320c8031870d4e Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 15 Aug 2026 15:15:16 +0100 Subject: [PATCH 01/22] [AGILE-361] Add the batch selection model Adds a framework-agnostic anchor/range/toggle model over opaque (type, id) identities, so card and table views can share one selection policy without inheriting any view's concepts. https://community.openproject.org/wp/AGILE-361 --- frontend/src/common/batch-selection.spec.ts | 170 +++++++++++++++++++ frontend/src/common/batch-selection.ts | 172 ++++++++++++++++++++ 2 files changed, 342 insertions(+) create mode 100644 frontend/src/common/batch-selection.spec.ts create mode 100644 frontend/src/common/batch-selection.ts diff --git a/frontend/src/common/batch-selection.spec.ts b/frontend/src/common/batch-selection.spec.ts new file mode 100644 index 000000000000..ea1c6277f428 --- /dev/null +++ b/frontend/src/common/batch-selection.spec.ts @@ -0,0 +1,170 @@ +//-- 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 { BatchSelection, selectionKey } from './batch-selection'; + +describe('BatchSelection', () => { + let selection:BatchSelection; + + beforeEach(() => { + selection = new BatchSelection(); + }); + + it('starts empty with no anchor', () => { + expect(selection.size).toBe(0); + expect(selection.anchor).toBeNull(); + }); + + it('replaces the batch and establishes the anchor', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + selection.replace({ type: 'work_package', id: '2' }, 'sprint:7'); + + expect(selection.items().map((entry) => entry.id)).toEqual(['2']); + expect(selection.anchor).toEqual({ type: 'work_package', id: '2', listKey: 'sprint:7' }); + }); + + it('re-bases the anchor when toggling on', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + selection.toggle({ type: 'work_package', id: '2' }, 'sprint:7'); + + expect(selection.items().map((entry) => entry.id)).toEqual(['1', '2']); + expect(selection.anchor).toEqual({ type: 'work_package', id: '2', listKey: 'sprint:7' }); + }); + + // The anchor deliberately survives its own deselection: the next Shift + // gesture still measures its range from the card the user last touched. + it('re-bases the anchor when toggling off', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + selection.toggle({ type: 'work_package', id: '1' }, 'sprint:7'); + + expect(selection.size).toBe(0); + expect(selection.anchor).toEqual({ type: 'work_package', id: '1', listKey: 'sprint:7' }); + }); + + it('replaces the batch with a range and preserves the anchor', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + selection.range([{ type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }, { type: 'work_package', id: '3' }]); + selection.range([{ type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }]); + + expect(selection.items().map((entry) => entry.id)).toEqual(['1', '2']); + expect(selection.anchor).toEqual({ type: 'work_package', id: '1', listKey: 'sprint:7' }); + }); + + it('selects all with an explicit anchor', () => { + selection.selectAll([{ type: 'work_package', id: '3' }, { type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }], { type: 'work_package', id: '2', listKey: 'sprint:7' }); + + expect(selection.items().map((entry) => entry.id)).toEqual(['3', '1', '2']); + expect(selection.anchor).toEqual({ type: 'work_package', id: '2', listKey: 'sprint:7' }); + }); + + it('clears the batch and the anchor', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + selection.clear(); + + expect(selection.size).toBe(0); + expect(selection.anchor).toBeNull(); + }); + + it('prunes ids that are no longer live and reports the change', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + selection.toggle({ type: 'work_package', id: '2' }, 'sprint:7'); + + expect(selection.prune(new Set([selectionKey({ type: 'work_package', id: '1' })]))).toBe(true); + expect(selection.items().map((entry) => entry.id)).toEqual(['1']); + expect(selection.prune(new Set([selectionKey({ type: 'work_package', id: '1' })]))).toBe(false); + }); + + it('drops an anchor whose card is gone', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + selection.prune(new Set()); + + expect(selection.anchor).toBeNull(); + }); + + it('reports membership without exposing mutable state', () => { + selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); + + expect(selection.has({ type: 'work_package', id: '1' })).toBe(true); + expect(selection.has({ type: 'work_package', id: '2' })).toBe(false); + }); + + describe('composite identity', () => { + // Ids are unique per table, not per root: a section and a custom field + // can both be id 5, and a nested topology puts them under one root. + it('keeps items of different types that share an id apart', () => { + selection.toggle({ type: 'section', id: '5' }, 'sections'); + selection.toggle({ type: 'custom_field', id: '5' }, 'custom_field:5'); + + expect(selection.size).toBe(2); + expect(selection.has({ type: 'section', id: '5' })).toBe(true); + expect(selection.has({ type: 'custom_field', id: '5' })).toBe(true); + expect(selection.has({ type: 'work_package', id: '5' })).toBe(false); + }); + + it('prunes by composite key, not by bare id', () => { + selection.toggle({ type: 'custom_field', id: '5' }, 'custom_field:5'); + + // The id survives — under a different type, which is a different item. + selection.prune(new Set([selectionKey({ type: 'section', id: '5' })])); + + expect(selection.size).toBe(0); + }); + + it('drops an anchor whose type no longer exists even when the id survives', () => { + selection.replace({ type: 'custom_field', id: '5' }, 'custom_field:5'); + + selection.prune(new Set([selectionKey({ type: 'section', id: '5' })])); + + expect(selection.anchor).toBeNull(); + }); + + it('builds a key that no type or id can forge a collision in', () => { + expect(selectionKey({ type: 'a', id: 'b' })) + .not.toEqual(selectionKey({ type: 'a\u001Fb', id: '' })); + }); + }); + + describe('#rebindAnchor', () => { + // A card can be moved to another list while staying the anchor, so the + // caller re-derives the key and hands it back. + it('points the anchor at a different list without disturbing membership', () => { + selection.replace({ type: 'work_package', id: '7' }, 'sprint:1'); + + selection.rebindAnchor('sprint:2'); + + expect(selection.anchor).toEqual({ type: 'work_package', id: '7', listKey: 'sprint:2' }); + expect(selection.items().map((entry) => entry.id)).toEqual(['7']); + }); + + it('does nothing when there is no anchor', () => { + selection.rebindAnchor('sprint:2'); + + expect(selection.anchor).toBeNull(); + }); + }); +}); diff --git a/frontend/src/common/batch-selection.ts b/frontend/src/common/batch-selection.ts new file mode 100644 index 000000000000..0605f471f1d8 --- /dev/null +++ b/frontend/src/common/batch-selection.ts @@ -0,0 +1,172 @@ +//-- 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. +//++ + +/** + * One selectable item's identity. + * + * Type as well as id, because ids are unique per source table rather than per + * root: a section and a custom field can both be id 5, and a nested list puts + * them under one root. Keying on the id alone would let one paint the other. + */ +export interface SelectionItem { + type:string; + id:string; +} + +/** An opaque key for one item. Build it with {@link selectionKey}. */ +export type SelectionKey = string; + +// U+001F (unit separator) cannot appear in an HTML attribute value that +// reached us through the DOM, so no type or id can forge a collision. Written +// as an escape rather than a literal control character in source. +const KEY_SEPARATOR = '\u001F'; + +export function selectionKey({ type, id }:SelectionItem):SelectionKey { + return `${type}${KEY_SEPARATOR}${id}`; +} + +/** + * The item from which a contiguous batch range is measured. + * + * `listKey` is opaque here: the model records which list a range may span + * without learning what a list is. Its holder decides how to derive it, and + * rebinds it through {@link BatchSelection#rebindAnchor} when the item moves. + */ +export interface SelectionAnchor extends SelectionItem { + listKey:string; +} + +/** + * Batch selection state and anchor semantics, free of any framework. + * + * Stores membership and an anchor, and nothing else. Visual order belongs to + * the live document, and range feasibility is resolved outside: callers hand + * in an already resolved range. Whether the members agree on a type is + * likewise their caller's policy. + */ +export class BatchSelection { + private selectedItems = new Map(); + private selectionAnchor:SelectionAnchor|null = null; + + // Keys, for membership tests and presentation matching. + get keys():ReadonlySet { + return new Set(this.selectedItems.keys()); + } + + // The pairs themselves, for callers that need the type back. + items():SelectionItem[] { + return [...this.selectedItems.values()]; + } + + get anchor():SelectionAnchor|null { + return this.selectionAnchor; + } + + get size():number { + return this.selectedItems.size; + } + + has(item:SelectionItem):boolean { + return this.selectedItems.has(selectionKey(item)); + } + + replace(item:SelectionItem, listKey:string):void { + this.selectedItems = new Map([[selectionKey(item), item]]); + this.selectionAnchor = { ...item, listKey }; + } + + // Re-bases the anchor even when the toggle deselects: the user's last + // touched card is where they expect the next range to start from. + toggle(item:SelectionItem, listKey:string):void { + const key = selectionKey(item); + + if (this.selectedItems.has(key)) { + this.selectedItems.delete(key); + } else { + this.selectedItems.set(key, item); + } + + this.selectionAnchor = { ...item, listKey }; + } + + // The anchor stays put so repeated Shift gestures resize one range rather + // than walking it across the list. + range(rangeItems:readonly SelectionItem[]):void { + this.selectedItems = new Map(rangeItems.map((item) => [selectionKey(item), item])); + } + + selectAll(items:readonly SelectionItem[], anchor:SelectionAnchor|null):void { + this.selectedItems = new Map(items.map((item) => [selectionKey(item), item])); + this.selectionAnchor = anchor; + } + + clear():void { + this.selectedItems = new Map(); + this.selectionAnchor = null; + } + + /** + * Points the existing anchor at a different list. + * + * A card can be moved to another list while remaining the anchor — a + * cross-list drag does exactly that. The caller re-derives the key from the + * live document and hands it back. + */ + rebindAnchor(listKey:string):void { + if (this.selectionAnchor) { + this.selectionAnchor = { ...this.selectionAnchor, listKey }; + } + } + + /** + * Drops members and an anchor that no longer exist in the document. + * + * `liveKeys` is keyed the same way membership is, so an id that survives + * under a *different* type does not keep a stale member alive. + * + * @return whether membership or the anchor changed, for a caller that can + * skip redundant work when nothing did. + */ + prune(liveKeys:ReadonlySet):boolean { + let changed = false; + + for (const key of [...this.selectedItems.keys()]) { + if (!liveKeys.has(key)) { + this.selectedItems.delete(key); + changed = true; + } + } + + if (this.selectionAnchor && !liveKeys.has(selectionKey(this.selectionAnchor))) { + this.selectionAnchor = null; + changed = true; + } + + return changed; + } +} From 383479fd6c5da9a553488c3be085d4b571f3e75d Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 15 Aug 2026 15:15:16 +0100 Subject: [PATCH 02/22] [AGILE-361] Model item mobility on sortable items Renders every card as a sortable item and replaces presence-based movability with a mobility value (free, confined or fixed), so a card the user cannot move still anchors drops and position counting. An unrecognised value falls closed to fixed. https://community.openproject.org/wp/AGILE-361 --- .../sortable-lists/item.controller.spec.ts | 133 +++++++++++++++++- .../dynamic/sortable-lists/item.controller.ts | 39 +++-- .../dynamic/sortable-lists/list-dom.spec.ts | 45 ++++++ .../dynamic/sortable-lists/list-dom.ts | 49 ++++++- 4 files changed, 245 insertions(+), 21 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index 8bc94713becc..e75a2bf7f9a4 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts @@ -105,6 +105,7 @@ describe('Sortable lists item controller', () => { moveAvailability: vi.fn(() => null), ownerListElementOf: vi.fn(() => ownerListElement), ownerRowsContainer: vi.fn(ownerRowsContainer), + collapseSelectionForDrag: vi.fn(), }; } @@ -113,15 +114,15 @@ describe('Sortable lists item controller', () => { { handle = null, root = fakeRoot(), - confinedValue = false, externalUrl = null, label = null, + mobility = 'free', }:{ handle?:HTMLElement|null; root?:SortableListsRoot|null; - confinedValue?:boolean; externalUrl?:string|null; label?:string|null; + mobility?:string; } = {}, ) { const controller = Object.create(ItemController.prototype) as InstanceType; @@ -131,11 +132,14 @@ describe('Sortable lists item controller', () => { Object.defineProperty(controller, 'hasIdValue', { value: true }); Object.defineProperty(controller, 'typeValue', { value: 'item' }); Object.defineProperty(controller, 'hasTypeValue', { value: true }); - Object.defineProperty(controller, 'confinedValue', { value: confinedValue }); Object.defineProperty(controller, 'externalUrlValue', { value: externalUrl ?? '' }); Object.defineProperty(controller, 'hasExternalUrlValue', { value: externalUrl !== null }); Object.defineProperty(controller, 'labelValue', { value: label ?? '' }); Object.defineProperty(controller, 'hasLabelValue', { value: label !== null }); + // Written to the element, not stubbed as a controller property: the + // controller reads mobility through list-dom's parser, which stubbing + // would bypass. + element.setAttribute('data-sortable-lists--item-mobility-value', mobility); Object.defineProperty(controller, 'hasHandleTarget', { value: handle !== null }); if (handle) { Object.defineProperty(controller, 'handleTarget', { value: handle }); @@ -177,6 +181,15 @@ describe('Sortable lists item controller', () => { element.removeAttribute('data-drop-target-for-element'); }); }); + // Mirrors the real adapter's addAttribute side effect, so movability tests + // can assert on the same `draggable` attribute Pragmatic marks in production. + vi.mocked(draggable).mockImplementation(({ element }) => { + element.setAttribute('draggable', 'true'); + + return vi.fn(() => { + element.removeAttribute('draggable'); + }); + }); }); function connectItem({ id, type }:{ id:string; type:string }) { @@ -188,7 +201,7 @@ describe('Sortable lists item controller', () => { Object.defineProperty(controller, 'hasIdValue', { value: id !== '' }); Object.defineProperty(controller, 'typeValue', { value: type }); Object.defineProperty(controller, 'hasTypeValue', { value: type !== '' }); - Object.defineProperty(controller, 'confinedValue', { value: false }); + Object.defineProperty(controller, 'hasHandleTarget', { value: false }); controller.connect(); @@ -498,7 +511,7 @@ describe('Sortable lists item controller', () => { it('still registers a drag source', () => { const element = document.createElement('li'); - connectedControllerFor(element, { confinedValue: true }); + connectedControllerFor(element, { mobility: 'confined' }); expect(draggable).toHaveBeenCalledWith(expect.objectContaining({ element })); }); @@ -740,7 +753,7 @@ describe('Sortable lists item controller', () => { const element = document.createElement('article'); connectedControllerFor(element, { root: fakeRoot(root, { ownerListElement: sourceList }), - confinedValue: true, + mobility: 'confined', }); expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) @@ -1227,4 +1240,112 @@ describe('Sortable lists item controller', () => { expect(moveInDirection).not.toHaveBeenCalled(); }); }); + + describe('movability and focus', () => { + let ctx:StimulusTestContext; + + afterEach(() => { + ctx?.dispose(); + }); + + // Mounts a minimal item row through the real Stimulus lifecycle, so + // mobility and the focus target come from real value/target wiring. + // Omitting `mobility` renders no attribute at all, exercising the free + // default; the root's tabindex stands in for a consumer that makes the + // row itself focusable. + async function renderItem( + { mobility, withFocusTarget = false }:{ mobility?:string; withFocusTarget?:boolean } = {}, + ):Promise { + ctx = await setupStimulusTest({ + controllers: { + 'sortable-lists--item': ItemController, + }, + }); + + const mobilityAttr = mobility === undefined ? '' : ` data-sortable-lists--item-mobility-value="${mobility}"`; + const focusTargetHtml = withFocusTarget + ? '' + : ''; + + await ctx.mount(` +
  • ${focusTargetHtml}
    • + `); + + return ctx.container.querySelector('[data-controller="sortable-lists--item"]')!; + } + + function controllerFor(element:HTMLElement) { + return ctx.getController>('sortable-lists--item', element); + } + + it('registers a draggable when the item takes part in ordering', async () => { + const item = await renderItem({ mobility: 'free' }); + + expect(item.hasAttribute('draggable')).toBe(true); + }); + + it('does not register a draggable when the item is fixed', async () => { + const item = await renderItem({ mobility: 'fixed' }); + + expect(item.hasAttribute('draggable')).toBe(false); + }); + + // A fixed row is still an ordered participant: it anchors drops for its + // orderable neighbours, so its drop target must stay registered. + it('still registers a drop target when the item is fixed', async () => { + const item = await renderItem({ mobility: 'fixed' }); + + expect(item.hasAttribute('data-drop-target-for-element')).toBe(true); + }); + + it('treats a missing mobility attribute as free', async () => { + const item = await renderItem({}); + + expect(item.hasAttribute('draggable')).toBe(true); + }); + + it('focuses its focus target', async () => { + const item = await renderItem({ mobility: 'free', withFocusTarget: true }); + const controller = controllerFor(item); + + controller.focusItem(); + + expect(document.activeElement).toBe(item.querySelector('[data-sortable-lists--item-target~="focus"]')); + }); + + it('focuses itself when it has no focus target', async () => { + const item = await renderItem({ mobility: 'free' }); + const controller = controllerFor(item); + + controller.focusItem(); + + expect(document.activeElement).toBe(item); + }); + + it('collapses the batch onto the dragged item when a drag starts', async () => { + const item = await renderItem({ mobility: 'free' }); + const controller = controllerFor(item); + const collapseSelectionForDrag = vi.fn(); + const root:SortableListsRoot = { + element: item, + busy: false, + moveInDirection: vi.fn(), + moveAvailability: vi.fn(() => null), + ownerListElementOf: vi.fn(() => null), + ownerRowsContainer: vi.fn(() => null), + collapseSelectionForDrag, + }; + + controller.connectRoot(root); + + vi.mocked(draggable).mock.lastCall?.[0].onDragStart?.(dragEventPayload(item)); + + expect(collapseSelectionForDrag).toHaveBeenCalledWith(item); + }); + }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index 409483e43657..15c373f7906d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -48,13 +48,13 @@ import { type SortableItemData, type SortableListsRoot, } from './drag-and-drop'; -import { isMoveDirection, sortableItemSelector } from './list-dom'; +import { isConfinedItem, isMoveDirection, isOrderableItem, sortableItemSelector } from './list-dom'; import { renderDragPreview } from './preview'; type CleanupFn = () => void; export default class ItemController extends Controller implements RootAwareChild { - static targets = ['handle', 'preview', 'moveItem', 'moveMenu', 'moveDivider']; + static targets = ['handle', 'preview', 'moveItem', 'moveMenu', 'moveDivider', 'focus']; static elements = { menu: 'action-menu' }; static values = { @@ -62,13 +62,11 @@ export default class ItemController extends Controller implements R type: String, externalUrl: String, hideUnavailable: { type: Boolean, default: true }, - // A confined item is still a full drag source, but only its own list and - // that list's rows accept it as a drop target; foreign containers refuse - // it, so a release there lands nowhere and the item stays put. Consumers - // use this for items the server allows to reorder in place but refuses to - // relocate to another container. - confined: { type: Boolean, default: false }, label: String, + // See ItemMobility in list-dom. A `confined` item is still a full drag + // source; only its own list accepts it as a drop target, so a release + // anywhere else lands nowhere and the item stays put. + mobility: { type: String, default: 'free' }, }; declare readonly idValue:string; @@ -78,9 +76,9 @@ export default class ItemController extends Controller implements R declare readonly externalUrlValue:string; declare readonly hasExternalUrlValue:boolean; declare readonly hideUnavailableValue:boolean; - declare readonly confinedValue:boolean; declare readonly labelValue:string; declare readonly hasLabelValue:boolean; + declare readonly mobilityValue:string; declare readonly handleTarget:HTMLElement; declare readonly hasHandleTarget:boolean; @@ -91,6 +89,8 @@ export default class ItemController extends Controller implements R declare readonly hasMoveMenuTarget:boolean; declare readonly moveDividerTarget:HTMLElement; declare readonly hasMoveDividerTarget:boolean; + declare readonly focusTarget:HTMLElement; + declare readonly hasFocusTarget:boolean; // Provided by the stimulus-elements blessing; absent when the item is not // inside a Primer action-menu (a drag-only consumer), in which case the move @@ -143,7 +143,7 @@ export default class ItemController extends Controller implements R move(event:ActionEvent):void { const item = event.currentTarget; - if (!this.hasMenuElement || !(item instanceof HTMLElement)) { + if (!isOrderableItem(this.element) || !this.hasMenuElement || !(item instanceof HTMLElement)) { return; } @@ -157,6 +157,12 @@ export default class ItemController extends Controller implements R } } + // The focus host is the consumer's business: Backlogs puts the tab stop on + // the card inside the row, another consumer may focus the row itself. + focusItem():void { + (this.hasFocusTarget ? this.focusTarget : this.element).focus(); + } + // Called by the root controller's outlet-connected callback. connectRoot(root:SortableListsRoot):void { this.root = root; @@ -178,14 +184,16 @@ export default class ItemController extends Controller implements R private register():void { this.cleanupFn = combine( - this.registerDraggable(), + // A non-movable item registers no draggable but stays a drop target: + // its movable neighbours still anchor on it. + isOrderableItem(this.element) ? this.registerDraggable() : () => undefined, this.registerDropTarget(), ); } // Both values are required: an item with an empty id can never be persisted, - // and an empty type never matches a list's accepted type, so the item would - // appear draggable yet silently refuse every drop. Surface that wiring mistake. + // and an empty type would never match a list's accepted type, so it could + // neither be dropped nor anchor a drop. Surface that wiring mistake. private warnOnMissingValues():void { if (!this.hasIdValue) { console.warn( @@ -221,6 +229,9 @@ export default class ItemController extends Controller implements R }, getInitialData: () => this.getItemData(), onDragStart: () => { + // One drag moves one item until AGILE-278 lands, so a wider batch + // collapses onto it rather than appearing to come along. + this.root?.collapseSelectionForDrag(this.element); // Cancels drops landing outside registered drop targets. This also // guards the external data channel: a misdropped card carrying // text/uri-list would otherwise navigate the current tab to that URL. @@ -357,7 +368,7 @@ export default class ItemController extends Controller implements R type: this.typeValue, rootElement: this.root?.element ?? null, sourceListElement: this.root?.ownerListElementOf(this.element) ?? null, - confined: this.confinedValue, + confined: isConfinedItem(this.element), }); } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts index 55677dcbd0aa..610257aa4305 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts @@ -28,7 +28,11 @@ import { captureRowPositions, + isConfinedItem, + isOrderableItem, + itemMobility, reorderRows, + sortableItemMobilityAttribute, resolveDirectionalPreviousItemId, resolveMoveAvailability, resolveListAppendPreviousItemId, @@ -358,6 +362,47 @@ describe('sortable lists DOM helpers', () => { }); }); +describe('itemMobility', () => { + function itemWith(mobility?:string):HTMLElement { + const item = document.createElement('li'); + item.setAttribute('data-sortable-lists--item-id-value', '1'); + + if (mobility !== undefined) { + item.setAttribute(sortableItemMobilityAttribute, mobility); + } + + return item; + } + + it('defaults a missing attribute to free', () => { + expect(itemMobility(itemWith())).toBe('free'); + }); + + it.each(['fixed', 'confined', 'free'])('reads the recognised value %s', (value) => { + expect(itemMobility(itemWith(value))).toBe(value); + }); + + // A typo must not hand the user a draggable, selectable card that only + // fails once the request comes back. + it('falls back to fixed for an unrecognised value', () => { + expect(itemMobility(itemWith('movable'))).toBe('fixed'); + }); + + it('treats an empty attribute as unrecognised', () => { + expect(itemMobility(itemWith(''))).toBe('fixed'); + }); + + it('derives orderable and confined from the union', () => { + expect(isOrderableItem(itemWith('free'))).toBe(true); + expect(isOrderableItem(itemWith('confined'))).toBe(true); + expect(isOrderableItem(itemWith('fixed'))).toBe(false); + + expect(isConfinedItem(itemWith('confined'))).toBe(true); + expect(isConfinedItem(itemWith('free'))).toBe(false); + expect(isConfinedItem(itemWith('fixed'))).toBe(false); + }); +}); + describe('directional move helpers', () => { function container(ids:string[]):HTMLElement { const ul = document.createElement('ul'); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts index c4ae78c451b4..0305fa2f82b6 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -26,9 +26,13 @@ // See COPYRIGHT and LICENSE files for more details. //++ +import { debugLog } from 'core-app/shared/helpers/debug_output'; + // Sortable lists use a DOM contract shared by the root and item controllers: // the root has data-controller~="sortable-lists"; lists are sortable-lists--list -// controllers wired to the root via outlets; items expose sortable-lists--item values; +// controllers wired to the root via outlets; items expose sortable-lists--item +// values and a mobility, which says what ordering the item takes part in; an +// item that takes none still participates in list order and accepts drops; // sparse non-item rows may expose data-sortable-lists-prev-item-id. // // This module holds the drag-and-drop-agnostic half of that contract: reading @@ -41,6 +45,18 @@ export const sortableItemSelector = '[data-sortable-lists--item-id-value]'; export const sortableListSelector = '[data-controller~="sortable-lists--list"]'; export const sortablePreviousItemIdAttribute = 'data-sortable-lists-prev-item-id'; export const sortableOmittedCountAttribute = 'data-sortable-lists-omitted-count'; +export const sortableItemMobilityAttribute = 'data-sortable-lists--item-mobility-value'; + +/** + * What ordering an item takes part in. + * + * `fixed` takes no part at all: no drag, no positional move, no selection. + * `confined` reorders within its own list but is refused by every other + * container. `free` may move to any list that accepts its type. + */ +export type ItemMobility = 'fixed'|'confined'|'free'; + +const recognisedMobilities = new Set(['fixed', 'confined', 'free']); // Rows are the direct children of the list's resolved rows container. The // rows container itself (a nested
      , the list element, ...) is decided by the list @@ -71,6 +87,37 @@ export function resolveItemId(element:Element):string|null { export const sortableItemTypeAttribute = 'data-sortable-lists--item-type-value'; +/** + * The item's mobility. + * + * An absent attribute means `free`, so a consumer that renders no mobility + * keeps working. A present but unrecognised value fails closed to `fixed`: a + * typo must not hand the user a draggable card, live move actions and a + * selectable row that only fail once the request comes back. + */ +export function itemMobility(itemElement:Element):ItemMobility { + const value = itemElement.getAttribute(sortableItemMobilityAttribute); + + if (value === null) { + return 'free'; + } + + if (!recognisedMobilities.has(value)) { + debugLog(`sortable-lists: unrecognised mobility "${value}", treating the item as fixed`); + return 'fixed'; + } + + return value as ItemMobility; +} + +export function isOrderableItem(itemElement:Element):boolean { + return itemMobility(itemElement) !== 'fixed'; +} + +export function isConfinedItem(itemElement:Element):boolean { + return itemMobility(itemElement) === 'confined'; +} + export function resolveItemType(element:Element):string|null { const type = element.getAttribute(sortableItemTypeAttribute); From dfc239c6ddf05711372492ed697c8a2d61a11b3f Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 15 Aug 2026 15:15:25 +0100 Subject: [PATCH 03/22] [AGILE-361] Add the selection adapter and gestures Adds the DOM-facing selection adapter and an orchestrator turning pointer and keyboard gestures into model changes and announcements. Ranges and Ctrl/Cmd+A are confined to the focused card's list; Escape clears the selection from anywhere on the page. https://community.openproject.org/wp/AGILE-361 --- config/locales/js-en.yml | 11 + .../selection-orchestrator.spec.ts | 458 ++++++++++++++++ .../sortable-lists/selection-orchestrator.ts | 496 ++++++++++++++++++ .../dynamic/sortable-lists/selection.spec.ts | 369 +++++++++++++ .../dynamic/sortable-lists/selection.ts | 330 ++++++++++++ frontend/src/stimulus/helpers/platform.ts | 49 ++ 6 files changed, 1713 insertions(+) create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts create mode 100644 frontend/src/stimulus/helpers/platform.ts diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index dc2a37bfd2c2..7c54ca744684 100644 --- a/config/locales/js-en.yml +++ b/config/locales/js-en.yml @@ -867,6 +867,17 @@ en: move_failed_rolled_back: "Move failed. %{label} returned to its previous position." moved: "%{label} moved to position %{position} of %{total}" moved_to_list: "%{label} moved to %{list}, position %{position} of %{total}" + selection: + cleared: "Selection cleared." + not_selectable: "Selection unchanged. This item takes no part in this list's ordering." + range_blocked: "Selection unchanged. That range contains an item that takes no part in this list's ordering." + range_restarted: + one: "Could not extend the range. 1 item selected." + other: "Could not extend the range. %{count} items selected." + range_unavailable: "Selection unchanged. Expand this list to select that range." + selected: + one: "1 item selected." + other: "%{count} items selected." spot: drop_modal: diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts new file mode 100644 index 000000000000..b5f9d9ec1f04 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -0,0 +1,458 @@ +//-- 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 { LiveRegionElement } from '@primer/live-region-element'; +import { type MockInstance } from 'vitest'; +import type { SelectionHost } from './selection-orchestrator'; + +// No Stimulus application, no outlets, no controller lifecycle: the host +// port lets selection be driven over a plain DOM. +describe('SelectionOrchestrator', () => { + // Imported dynamically, after the mock above registers: spec files share + // one module registry (the runner does not isolate them), so a static + // import here would bind whatever another spec already pulled in — with + // the real platform helper baked in. + let SelectionOrchestrator:typeof import('./selection-orchestrator').SelectionOrchestrator; + let batchSelectedAttribute:string; + + beforeAll(async () => { + ({ SelectionOrchestrator } = await import('./selection-orchestrator')); + ({ batchSelectedAttribute } = await import('./selection')); + }); + + let root:HTMLElement; + let announceSpy:MockInstance; + let busy = false; + let focused:HTMLElement|null = null; + + function hostFor(element:HTMLElement):SelectionHost { + return { + rootElement: element, + get busy() { return busy; }, + announcementScope: 'js.sortable_lists.selection', + descriptionId: 'selection-description', + focusItem: (item) => { focused = item; }, + ownerRowsContainer: (item) => { + const list = item.closest('[data-controller~="sortable-lists--list"]'); + return list ? (list.querySelector(':scope > ul') ?? list) : null; + }, + }; + } + + beforeEach(() => { + busy = false; + focused = null; + root = document.createElement('div'); + root.setAttribute('data-controller', 'sortable-lists'); + root.innerHTML = ` +
      +
        +
      • +
      • +
      • +
      +
      +
      +
        +
      • +
      +
      +
      +
        +
      • +
      • +
      +
      + `; + document.body.append(root, document.createElement('live-region')); + announceSpy = vi.spyOn(LiveRegionElement.prototype, 'announce'); + pretendPlatform('Windows'); + window.I18n.store({ + en: { + js: { + sortable_lists: { + selection: { + cleared: 'Selection cleared.', + not_selectable: 'Selection unchanged. This item cannot be selected.', + range_blocked: 'Selection unchanged. That range contains an item that takes no part in this list\'s ordering.', + range_restarted: { one: 'Could not extend the range. 1 item selected.', other: 'Could not extend the range. %{count} items selected.' }, + range_unavailable: 'Selection unchanged. Expand this list to select that range.', + selected: { one: '1 item selected.', other: '%{count} items selected.' }, + }, + }, + }, + }, + }); + }); + + afterEach(() => { + root.remove(); + document.querySelector('live-region')?.remove(); + announceSpy.mockRestore(); + }); + + // navigator.platform and userAgentData are read-only accessors, so the + // stub is installed per test and torn down with the fixture. + function pretendPlatform(platform:string):void { + Object.defineProperty(navigator, 'userAgentData', { + value: { platform }, + configurable: true, + }); + } + + // Type-qualified, because the fixture deliberately holds a section and a + // work package that share id 1 — the collision a nested topology makes + // routine. + const itemOfType = (type:string, id:string) => root.querySelector( + `[data-sortable-lists--item-type-value="${type}"][data-sortable-lists--item-id-value="${id}"]`, + )!; + const item = (id:string) => itemOfType('work_package', id); + const sectionItem = (id:string) => itemOfType('section', id); + const isSelected = (element:HTMLElement) => element.hasAttribute(batchSelectedAttribute); + const clickOn = (element:HTMLElement, init:MouseEventInit = {}) => { + const event = new MouseEvent('click', { bubbles: true, cancelable: true, ...init }); + Object.defineProperty(event, 'target', { value: element }); + return event; + }; + + it('selects the clicked card without any Stimulus wiring', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + orchestrator.handleClick(clickOn(item('1'))); + + expect(isSelected(item('1'))).toBe(true); + expect(orchestrator.selectedIds()).toEqual(['1']); + }); + + it('extends a range from the anchor', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + }); + + it('routes focus through the host rather than touching the element', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true, cancelable: true }); + Object.defineProperty(event, 'target', { value: item('1') }); + + orchestrator.handleKeydown(event); + + expect(focused).toBe(item('2')); + }); + + it('reads busy from the host and refuses to mutate while a move is in flight', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + busy = true; + + orchestrator.handleClick(clickOn(item('1'))); + + expect(orchestrator.selectedIds()).toEqual([]); + }); + + it('clears presentation without disturbing the model', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + + orchestrator.clearPresentation(); + + expect(isSelected(item('1'))).toBe(false); + expect(orchestrator.selectedIds()).toEqual(['1']); + }); + + // Falling through mid-move would open the details pane on a card the batch + // was not allowed to follow. + it('consumes a plain click while a move is in flight', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + busy = true; + + const event = clickOn(item('2')); + orchestrator.handleClick(event); + + expect(event.defaultPrevented).toBe(true); + expect(orchestrator.selectedIds()).toEqual(['1']); + }); + + describe('announcements', () => { + const spoken = () => announceSpy.mock.calls.map((call) => call[0]); + + // A resized range of the same size still changed, and has no details + // pane to serve as its own feedback. + it('announces a range that swaps membership at the same size', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('2'))); + orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); + announceSpy.mockClear(); + + orchestrator.handleClick(clickOn(item('1'), { shiftKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['1', '2']); + expect(spoken()).toEqual(['2 items selected.']); + }); + + // The baseline is what the previous render painted: tracking the last + // announcement would leave a stale one behind a silent render. + it('stays silent when a selection gesture changes nothing after a silent click', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('2'))); + announceSpy.mockClear(); + + orchestrator.handleClick(clickOn(item('2'), { shiftKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['2']); + expect(spoken()).toEqual([]); + }); + + it('stays silent on a plain click that swaps a one-card selection', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + announceSpy.mockClear(); + + orchestrator.handleClick(clickOn(item('2'))); + + expect(orchestrator.selectedIds()).toEqual(['2']); + expect(spoken()).toEqual([]); + }); + + it('still announces a plain click that changes the count', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); + announceSpy.mockClear(); + + orchestrator.handleClick(clickOn(item('2'))); + + expect(spoken()).toEqual(['1 item selected.']); + }); + }); + + describe('keyboard and pointer edge cases', () => { + const keydownOn = (element:HTMLElement, key:string, init:KeyboardEventInit = {}) => { + const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true, ...init }); + Object.defineProperty(event, 'target', { value: element }); + return event; + }; + + // Consuming the key with nothing to select would block the browser's own + // select-all and announce nothing in its place. + it('leaves Ctrl/Cmd+A alone when nothing is selectable', () => { + root.querySelectorAll('[data-sortable-lists--item-id-value]') + .forEach((el) => el.setAttribute('data-sortable-lists--item-mobility-value', 'fixed')); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'a', { metaKey: true }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + + it('still consumes Ctrl/Cmd+A when there is something to select', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'a', { metaKey: true }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(true); + expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + }); + + it('anchors select-all inside the list when the focused card is fixed', () => { + item('1').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + orchestrator.handleKeydown(keydownOn(item('1'), 'a', { metaKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['2', '3']); + // The fallback anchor is the first orderable card of the same list, so + // a follow-up Shift ranges within it rather than from another list. + orchestrator.handleKeydown(keydownOn(item('3'), ' ', { shiftKey: true })); + expect(orchestrator.selectedIds()).toEqual(['2', '3']); + }); + + // With nothing selectable in this list, the browser's own select-all + // keeps the gesture even though another list has cards. + it('leaves Ctrl/Cmd+A alone when only other lists are selectable', () => { + root.querySelectorAll('[data-sortable-lists--list-id-value="7"] [data-sortable-lists--item-id-value]') + .forEach((el) => el.setAttribute('data-sortable-lists--item-mobility-value', 'fixed')); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'a', { metaKey: true }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + + // Holding Space would otherwise toggle the card over and over. + it('ignores a repeated Space keydown', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleKeydown(keydownOn(item('1'), ' ')); + + orchestrator.handleKeydown(keydownOn(item('1'), ' ', { repeat: true })); + + expect(orchestrator.selectedIds()).toEqual(['1']); + }); + + it('refuses to extend a range onto a fixed card', () => { + item('2').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + orchestrator.handleKeydown(keydownOn(item('1'), 'ArrowDown', { shiftKey: true })); + + expect(isSelected(item('2'))).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + + // Ctrl-click opens the contextual menu on Apple platforms: it must + // neither toggle nor fall through to the ordinary-click path. + it('ignores Ctrl-click entirely on Apple platforms', () => { + pretendPlatform('macOS'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + + const event = clickOn(item('2'), { ctrlKey: true }); + orchestrator.handleClick(event); + + expect(orchestrator.selectedIds()).toEqual(['1']); + expect(isSelected(item('2'))).toBe(false); + expect(event.defaultPrevented).toBe(false); + }); + + it('still treats Ctrl-click as multi-select elsewhere', () => { + pretendPlatform('Windows'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + + orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['1', '2']); + }); + }); + + // collapseForDrag stamps the anchor at drag start, so a cross-list drop + // leaves it naming the source list. Without a rebind the next Shift in the + // destination reads as cross-list and restarts instead of extending. + it('extends a range after the anchored card moved to another list', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const moved = item('1'); + orchestrator.handleClick(clickOn(moved)); + + root.querySelector('[data-sortable-lists--list-id-value="8"] ul')!.prepend(moved); + orchestrator.reconcile(); + orchestrator.handleClick(clickOn(item('4'), { shiftKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['1', '4']); + }); + + describe('one batch, one item type', () => { + // Section 1 and work package 1 are different items that happen to share + // an id, which is what a nested topology makes routine. + it('does not paint an item of another type that shares an id', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + orchestrator.handleClick(clickOn(item('1'))); + + expect(isSelected(item('1'))).toBe(true); + expect(isSelected(sectionItem('1'))).toBe(false); + }); + + it('restarts the batch when Ctrl/Cmd-click lands on another type', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('2'), { metaKey: true })); + + orchestrator.handleClick(clickOn(sectionItem('1'), { metaKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['1']); + expect(isSelected(sectionItem('1'))).toBe(true); + expect(isSelected(item('1'))).toBe(false); + }); + + it('restarts the batch when Space toggles another type', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + + const event = new KeyboardEvent('keydown', { key: ' ', bubbles: true, cancelable: true }); + Object.defineProperty(event, 'target', { value: sectionItem('1') }); + orchestrator.handleKeydown(event); + + expect(isSelected(sectionItem('1'))).toBe(true); + expect(isSelected(item('1'))).toBe(false); + }); + + it('scopes select-all to the anchoring candidate type', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = new KeyboardEvent('keydown', { key: 'a', metaKey: true, bubbles: true, cancelable: true }); + Object.defineProperty(event, 'target', { value: sectionItem('1') }); + + orchestrator.handleKeydown(event); + + expect(orchestrator.selectedIds()).toEqual(['1']); + expect(isSelected(sectionItem('1'))).toBe(true); + expect(isSelected(item('2'))).toBe(false); + }); + + // A bare-id lookup would find the section first, since it appears + // earlier in document order, and rebind the anchor to the wrong list. + it('rebinds the anchor to its own type when an earlier item shares its id', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const moved = item('1'); + orchestrator.handleClick(clickOn(moved)); + + root.querySelector('[data-sortable-lists--list-id-value="8"] ul')!.prepend(moved); + orchestrator.reconcile(); + orchestrator.handleClick(clickOn(item('4'), { shiftKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['1', '4']); + }); + }); + + it('drops members that a morph removed from the document', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + orchestrator.handleClick(clickOn(item('2'), { metaKey: true })); + item('1').remove(); + + orchestrator.reconcile(); + + expect(orchestrator.selectedIds()).toEqual(['2']); + expect(announceSpy).toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts new file mode 100644 index 000000000000..5ab921111574 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -0,0 +1,496 @@ +//-- 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 { BatchSelection, type SelectionAnchor, type SelectionKey } from 'core-common/batch-selection'; +import { announce } from '@primer/live-region-element'; +import { resolveItemId, resolveItemType } from './list-dom'; +import { + applySelectionPresentation, + listBoundaryItem, + liveOrderableKeys, + liveOrderableListItems, + neighbourItem, + orderedItemElements, + orderedSelectedItems, + resolveCandidate, + resolveRangeItems, + type SelectionCandidate, +} from './selection'; +import { closestInteractiveElement } from 'core-common/interactive-element-helper'; +import { isApplePlatform } from 'core-stimulus/helpers/platform'; + +/** + * What the orchestrator needs from whatever hosts it. + */ +export interface SelectionHost { + readonly rootElement:HTMLElement; + readonly busy:boolean; + readonly announcementScope:string; + readonly descriptionId:string; + // The consumer decides which element inside a row holds the tab stop. + focusItem(itemElement:HTMLElement):void; + // Must be the container moves use, so ranges and moves agree on what a + // list's rows are. + ownerRowsContainer(itemElement:HTMLElement):HTMLElement|null; +} + +/** + * Batch selection: gestures in, model and presentation out. + */ +export class SelectionOrchestrator { + private readonly selection = new BatchSelection(); + + // Last painted membership, not last announced: a silent navigation render + // in between would leave a stale baseline and make the next no-op speak. + private lastRenderedKeys:ReadonlySet = new Set(); + + constructor(private readonly host:SelectionHost) {} + + // Live ordered membership, for AGILE-278's batch move. + selectedIds():string[] { + return orderedSelectedItems(this.host.rootElement, this.selection.keys).map((item) => item.id); + } + + // A menu move relocates exactly one card, so it collapses like a drag. + collapseForMove(itemElement:HTMLElement):void { + this.collapseForDrag(itemElement); + } + + collapseForDrag(itemElement:HTMLElement):void { + // Nothing selected is nothing to collapse: a drag must not manufacture + // a one-card batch. + if (this.selection.size === 0) { + return; + } + + const candidate = resolveCandidate(this.host.rootElement, itemElement); + if (!candidate?.orderable) { + return; + } + + this.selection.replace({ type: candidate.type, id: candidate.id }, candidate.listKey); + this.renderSelection('selection'); + } + + readonly handleClick = (event:MouseEvent):void => { + // Ctrl-click is the secondary click on Apple platforms, where it opens + // the contextual menu and Cmd is the multi-select key instead. + if (event.ctrlKey && !event.metaKey && !event.shiftKey && isApplePlatform()) { + return; + } + + const multiSelect = event.metaKey || event.ctrlKey; + const modified = event.shiftKey || multiSelect; + const candidate = this.candidateForGesture(event.target); + if (!candidate) { + return; + } + + if (!modified) { + // Consumed before the busy check: falling through mid-move would open + // the details pane on a card the batch was not allowed to follow. + if (this.host.busy) { + event.preventDefault(); + event.stopPropagation(); + return; + } + + // A fixed card cannot join the batch, but must still clear the one + // behind it. + if (candidate.orderable) { + this.selection.replace({ type: candidate.type, id: candidate.id }, candidate.listKey); + this.renderSelection('navigation'); + } else { + this.selection.clear(); + this.renderSelection('navigation'); + } + return; + } + + // Consumed regardless of `busy`: a modified gesture reaching the card's + // own click handler would open the details pane on a selection toggle. + event.preventDefault(); + event.stopPropagation(); + + if (this.host.busy) { + return; + } + + if (!candidate.orderable) { + this.announceSelection('not_selectable'); + return; + } + + if (event.shiftKey) { + this.extendSelectionTo(candidate); + } else { + this.toggleWithinCohort(candidate); + this.renderSelection('selection'); + } + }; + + // A link inside a card is a link first. The walk stops at the focus host + // when the gesture landed inside it — the host may be focusable itself, + // Backlogs cards carry tabindex — and at the item otherwise, which keeps + // it bounded to the row for a host nested deeper than the item. + private candidateForGesture(target:EventTarget|null):SelectionCandidate|null { + const candidate = resolveCandidate(this.host.rootElement, target); + if (!candidate) { + return null; + } + + if (!(target instanceof Element)) { + return candidate; + } + + const boundary = candidate.focusHost.contains(target) ? candidate.focusHost : candidate.itemElement; + const interactive = closestInteractiveElement(target, boundary); + + return interactive ? null : candidate; + } + + readonly handleKeydown = (event:KeyboardEvent):void => { + const candidate = this.candidateForGesture(event.target); + if (!candidate) { + return; + } + + switch (event.key) { + case ' ': + this.handleSpace(event, candidate); + break; + case 'ArrowDown': + case 'ArrowUp': + this.handleArrow(event, candidate, event.key === 'ArrowDown' ? 1 : -1); + break; + case 'Home': + case 'End': + this.handleBoundary(event, candidate, event.key === 'Home' ? 'first' : 'last'); + break; + case 'a': + case 'A': + this.handleSelectAll(event, candidate); + break; + default: + // Enter belongs to the card's own activation handler; Escape is + // handled at the document. + break; + } + }; + + private handleSpace(event:KeyboardEvent, candidate:SelectionCandidate):void { + event.preventDefault(); + + // A held Space would otherwise toggle the card over and over. + if (event.repeat) { + return; + } + + if (this.host.busy) { + return; + } + + if (!candidate.orderable) { + this.announceSelection('not_selectable'); + return; + } + + if (event.shiftKey) { + this.extendSelectionTo(candidate); + } else { + this.toggleWithinCohort(candidate); + this.renderSelection('selection'); + } + } + + // Consumed even at a list boundary: an unconsumed arrow scrolls the page + // while focus stays put. + private handleArrow(event:KeyboardEvent, candidate:SelectionCandidate, offset:1|-1):void { + event.preventDefault(); + + const next = neighbourItem(this.host.rootElement, candidate.itemElement, offset); + if (!next) { + return; + } + + if (this.host.busy) { + return; + } + + this.focusAndMaybeExtend(event, next); + } + + // Consumed like an arrow, including in both no-op cases below. + private handleBoundary(event:KeyboardEvent, candidate:SelectionCandidate, edge:'first'|'last'):void { + event.preventDefault(); + + const target = listBoundaryItem(this.host.rootElement, candidate.itemElement, edge); + if (!target) { + return; + } + + // With Shift held the range still resizes out to the boundary. + if (target === candidate.itemElement && !event.shiftKey) { + return; + } + + if (this.host.busy) { + return; + } + + this.focusAndMaybeExtend(event, target); + } + + private focusAndMaybeExtend(event:KeyboardEvent, target:HTMLElement):void { + this.focusItemElement(target); + + if (!event.shiftKey) { + return; + } + + const candidate = resolveCandidate(this.host.rootElement, target); + if (!candidate) { + return; + } + + if (candidate.orderable) { + this.extendSelectionTo(candidate); + } else { + this.announceSelection('not_selectable'); + } + } + + private focusItemElement(target:HTMLElement):void { + this.host.focusItem(target); + } + + // Confined to the focused card's list, like a range. + private handleSelectAll(event:KeyboardEvent, candidate:SelectionCandidate):void { + if (!event.metaKey && !event.ctrlKey) { + return; + } + + const items = liveOrderableListItems(this.host.rootElement, candidate.itemElement) + .filter((item) => item.type === candidate.type); + // Only consumed once there is something to select: otherwise the + // browser's own select-all still has to work. + if (items.length === 0) { + return; + } + + event.preventDefault(); + + if (this.host.busy) { + return; + } + + // A fixed focused card cannot anchor the batch, so the first orderable + // card of the same list stands in. + const anchor:SelectionAnchor = candidate.orderable + ? { type: candidate.type, id: candidate.id, listKey: candidate.listKey } + : { ...items[0], listKey: candidate.listKey }; + + this.selection.selectAll(items, anchor); + this.renderSelection('selection'); + } + + // At the document, in the bubble phase: focus routinely sits off the root + // after a mouse selection, and an overlay's own Escape must run first. + readonly handleEscape = (event:KeyboardEvent):void => { + // A consumed Escape already answered the keystroke. + if (event.key !== 'Escape' || event.defaultPrevented) { + return; + } + + if (!this.escapeConcernsSelection(event.target)) { + return; + } + + // BatchSelection#toggle re-bases the anchor even on a deselect, so an + // emptied selection can still leave one behind for Escape to drop. + const hadSelection = this.selection.size > 0; + if (!hadSelection && this.selection.anchor === null) { + return; + } + + event.preventDefault(); + this.selection.clear(); + this.renderSelection('selection'); + }; + + // An overlay and an unrelated widget both own their own Escape, which + // means "dismiss that thing" rather than "drop this selection". + private escapeConcernsSelection(target:EventTarget|null):boolean { + if (!(target instanceof Element)) { + return true; + } + + if (target.closest('dialog, [popover], [role="dialog"], [role="menu"], [role="listbox"]')) { + return false; + } + + return target === document.body + || target === document.documentElement + || this.host.rootElement.contains(target); + } + + // A batch holds one item type: "all of these together" has no meaning + // across two kinds of thing, and a collection move sends one list of ids + // to one endpoint. + private cohortMatches(candidate:SelectionCandidate):boolean { + const { anchor } = this.selection; + + return anchor === null || anchor.type === candidate.type; + } + + // Every adding gesture routes through here, which is what keeps a mixed + // batch unreachable. + private toggleWithinCohort(candidate:SelectionCandidate):void { + if (!this.cohortMatches(candidate)) { + this.renderRangeRestart(candidate); + return; + } + + this.selection.toggle({ type: candidate.type, id: candidate.id }, candidate.listKey); + this.renderSelection('selection'); + } + + private extendSelectionTo(candidate:SelectionCandidate):void { + const { anchor } = this.selection; + + if (anchor && !this.cohortMatches(candidate)) { + this.renderRangeRestart(candidate); + return; + } + + if (!anchor) { + this.renderRangeRestart(candidate); + return; + } + + const range = resolveRangeItems( + this.host.rootElement, + anchor, + candidate, + this.host.ownerRowsContainer(candidate.itemElement), + ); + + if (range.ok) { + this.selection.range(range.items); + this.renderSelection('selection'); + return; + } + + if (range.reason === 'crossList') { + this.renderRangeRestart(candidate); + return; + } + + // Expanding the list can surface a truncated block, but never makes a + // locked card orderable, so the two speak different messages. + this.announceSelection(range.reason === 'locked' ? 'range_blocked' : 'range_unavailable'); + } + + // Always speaks, unlike renderSelection's count rule: a Shift gesture that + // failed to form a range has no other feedback, even when the resulting + // count is unchanged. + private renderRangeRestart(candidate:SelectionCandidate):void { + this.selection.replace({ type: candidate.type, id: candidate.id }, candidate.listKey); + this.syncSelectionPresentation(); + this.lastRenderedKeys = this.selection.keys; + this.announceSelection('range_restarted'); + } + + // Announcement follows the gesture class, not the count: `navigation` (a + // plain click) speaks only when the count moves, since the details pane it + // opens is its own feedback; `selection` speaks on any membership change. + private renderSelection(kind:'navigation'|'selection'):void { + const previous = this.lastRenderedKeys; + this.syncSelectionPresentation(); + + const current = this.selection.keys; + this.lastRenderedKeys = current; + + const changed = kind === 'navigation' + ? current.size !== previous.size + : current.size !== previous.size || [...current].some((id) => !previous.has(id)); + + if (changed) { + this.announceSelection(current.size === 0 ? 'cleared' : 'selected'); + } + } + + private syncSelectionPresentation():void { + applySelectionPresentation(this.host.rootElement, this.selection.keys, this.host.descriptionId); + } + + private announceSelection(key:'selected'|'cleared'|'not_selectable'|'range_unavailable'|'range_blocked'|'range_restarted'):void { + void announce(this.selectionMessage(key), { politeness: 'polite' }); + } + + // The consumer's vocabulary: Backlogs says "work package", not "item". + private selectionMessage(key:string):string { + return I18n.t(`${this.host.announcementScope}.${key}`, { count: this.selection.size }); + } + + // Repaints whether or not prune dropped anything: a morph can strip or + // preserve the marker attribute independently of the model. + reconcile():void { + this.selection.prune(liveOrderableKeys(this.host.rootElement)); + this.rebindAnchorList(); + this.renderSelection('selection'); + } + + // The anchor's list key is stamped when the anchor is set — drag *start* + // for a drag — so a card dropped elsewhere still names its old list. Runs + // after prune, so it only ever sees an anchor that still exists. + private rebindAnchorList():void { + const { anchor } = this.selection; + if (!anchor) { + return; + } + + // Matched on type as well as id: ids collide across source tables. + const element = orderedItemElements(this.host.rootElement) + .find((item) => resolveItemId(item) === anchor.id && resolveItemType(item) === anchor.type); + const candidate = element ? resolveCandidate(this.host.rootElement, element) : null; + + if (candidate) { + this.selection.rebindAnchor(candidate.listKey); + } + } + + // Presentation only: whatever restores the page builds a fresh orchestrator. + clearPresentation():void { + applySelectionPresentation(this.host.rootElement, new Set(), this.host.descriptionId); + } + + teardown():void { + this.clearPresentation(); + } +} diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts new file mode 100644 index 000000000000..06cb29760161 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts @@ -0,0 +1,369 @@ +//-- 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 { selectionKey } from 'core-common/batch-selection'; +import { + applySelectionPresentation, + batchSelectedAttribute, + listBoundaryItem, + liveOrderableItems, + liveOrderableListItems, + neighbourItem, + orderedItemElements, + orderedSelectedItems, + resolveCandidate, + resolveRangeItems, +} from './selection'; + +describe('sortable-lists selection adapter', () => { + let root:HTMLElement; + + // Two lists. Sprint 7 holds movable 1 and 2, a truncation marker, and + // movable 3. Sprint 8 holds movable 4 and non-movable 5. + beforeEach(() => { + root = document.createElement('div'); + root.setAttribute('data-controller', 'sortable-lists'); + root.innerHTML = ` +
      +
        +
      • one
        • +
      • +
      • +
      • +
      +
      +
      +
        +
      • +
      • +
      +
      + `; + document.body.appendChild(root); + }); + + afterEach(() => { + root.remove(); + }); + + // The rows container the host resolves in production; supplied directly + // here because this spec drives the adapter without one. + const rowsContainerFor = (item:HTMLElement) => { + const list = item.closest('[data-controller~="sortable-lists--list"]'); + return list ? (list.querySelector(':scope > ul') ?? list) : null; + }; + + const itemFor = (id:string) => root.querySelector(`[data-sortable-lists--item-id-value="${id}"]`)!; + const candidateFor = (id:string) => resolveCandidate(root, itemFor(id))!; + + // A trailing item in sprint 7 that hosts a list of its own, the nested + // topology a section-and-fields consumer renders. + const appendNestedList = () => { + const section = document.createElement('li'); + section.setAttribute('data-controller', 'sortable-lists--item'); + section.setAttribute('data-sortable-lists--item-id-value', '6'); + section.setAttribute('data-sortable-lists--item-type-value', 'section'); + section.innerHTML = ` +
      +
        +
      • +
      +
      + `; + root.querySelector('ul')!.appendChild(section); + }; + + it('resolves a candidate from a descendant of the item', () => { + const candidate = resolveCandidate(root, itemFor('1').querySelector('span')); + + expect(candidate).toEqual({ + type: 'work_package', + itemElement: itemFor('1'), + focusHost: itemFor('1'), + id: '1', + listKey: 'sprint:7', + orderable: true, + }); + }); + + // The focus host is also the boundary the interactive-descendant check + // stops at, so a focusable host does not disqualify its own card. + it('reports the focus target as the focus host when there is one', () => { + const card = document.createElement('div'); + card.setAttribute('data-sortable-lists--item-target', 'focus'); + card.tabIndex = 0; + itemFor('2').appendChild(card); + + expect(resolveCandidate(root, card)!.focusHost).toBe(card); + }); + + it('resolves a non-movable candidate', () => { + expect(candidateFor('5').orderable).toBe(false); + }); + + it('does not resolve a truncation marker as a candidate', () => { + const marker = root.querySelector('[data-sortable-lists-prev-item-id]')!; + + expect(resolveCandidate(root, marker)).toBeNull(); + }); + + it('does not resolve anything outside the root', () => { + expect(resolveCandidate(root, document.body)).toBeNull(); + }); + + it('refuses to resolve a candidate that declares no type', () => { + const untyped = document.createElement('li'); + untyped.setAttribute('data-controller', 'sortable-lists--item'); + untyped.setAttribute('data-sortable-lists--item-id-value', '99'); + root.querySelector('ul')!.appendChild(untyped); + + expect(resolveCandidate(root, untyped)).toBeNull(); + }); + + // An independently nested root is an ownership boundary. + it('does not own items belonging to a nested root', () => { + const nested = document.createElement('div'); + nested.setAttribute('data-controller', 'sortable-lists'); + nested.innerHTML = ` +
      +
        +
      • +
      +
      + `; + root.appendChild(nested); + const inner = nested.querySelector('[data-sortable-lists--item-id-value="90"]')!; + + expect(resolveCandidate(root, inner)).toBeNull(); + expect(orderedItemElements(root)).not.toContain(inner); + }); + + it('orders selected items by live document order across lists', () => { + const keys = new Set(['4', '1', '3'].map((id) => selectionKey({ type: 'work_package', id }))); + + expect(orderedSelectedItems(root, keys).map((item) => item.id)).toEqual(['1', '3', '4']); + }); + + it('lists only live orderable items', () => { + expect(liveOrderableItems(root).map((item) => item.id)).toEqual(['1', '2', '3', '4']); + }); + + it('resolves an ascending range within one list', () => { + const anchor = { type: 'work_package', id: '1', listKey: 'sprint:7' }; + + expect(resolveRangeItems(root, anchor, candidateFor('2'), rowsContainerFor(itemFor('2')))).toEqual({ ok: true, items: [{ type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }] }); + }); + + it('resolves a descending range within one list', () => { + const anchor = { type: 'work_package', id: '2', listKey: 'sprint:7' }; + + expect(resolveRangeItems(root, anchor, candidateFor('1'), rowsContainerFor(itemFor('1')))).toEqual({ ok: true, items: [{ type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }] }); + }); + + it('rejects a range that would cross a truncation marker', () => { + const anchor = { type: 'work_package', id: '2', listKey: 'sprint:7' }; + + expect(resolveRangeItems(root, anchor, candidateFor('3'), rowsContainerFor(itemFor('3')))).toEqual({ ok: false, reason: 'unavailable' }); + }); + + it('rejects a range that would cross a list boundary', () => { + const anchor = { type: 'work_package', id: '3', listKey: 'sprint:7' }; + + expect(resolveRangeItems(root, anchor, candidateFor('4'), rowsContainerFor(itemFor('4')))).toEqual({ ok: false, reason: 'crossList' }); + }); + + it('rejects a range whose anchor id was never a row in this list', () => { + const anchor = { type: 'work_package', id: '99', listKey: 'sprint:7' }; + + expect(resolveRangeItems(root, anchor, candidateFor('2'), rowsContainerFor(itemFor('2')))).toEqual({ ok: false, reason: 'unavailable' }); + }); + + // Refused, not trimmed to the movable cards — and with its own reason, + // since expanding the list can never resolve a locked card. + it('refuses a range that would include a non-movable card', () => { + const anchor = { type: 'work_package', id: '4', listKey: 'sprint:8' }; + + expect(resolveRangeItems(root, anchor, candidateFor('5'), rowsContainerFor(itemFor('5')))).toEqual({ ok: false, reason: 'locked' }); + }); + + // list-dom's contract lets a row wrap its item instead of being it. + it('resolves a range across rows that wrap their item element', () => { + const wrappingRoot = document.createElement('div'); + wrappingRoot.setAttribute('data-controller', 'sortable-lists'); + wrappingRoot.innerHTML = ` +
      +
        +
        • +
        • +
        • +
      +
      + `; + document.body.appendChild(wrappingRoot); + + try { + const wrappedItemFor = (id:string) => wrappingRoot.querySelector( + `[data-sortable-lists--item-id-value="${id}"]`, + )!; + const anchor = { type: 'work_package', id: '20', listKey: 'sprint:20' }; + const candidate = resolveCandidate(wrappingRoot, wrappedItemFor('22'))!; + + expect(resolveRangeItems(wrappingRoot, anchor, candidate, rowsContainerFor(candidate.itemElement))).toEqual({ ok: true, items: [{ type: 'work_package', id: '20' }, { type: 'work_package', id: '21' }, { type: 'work_package', id: '22' }] }); + } finally { + wrappingRoot.remove(); + } + }); + + // A candidate whose item lives outside the list's rows container has no + // row to find there. + it('rejects a range when the candidate item sits outside the rows container', () => { + const list = root.querySelector('[data-sortable-lists--list-id-value="7"]')!; + const strayItem = document.createElement('div'); + strayItem.setAttribute('data-controller', 'sortable-lists--item'); + strayItem.setAttribute('data-sortable-lists--item-id-value', '30'); + strayItem.setAttribute('data-sortable-lists--item-type-value', 'work_package'); + list.appendChild(strayItem); + + const anchor = { type: 'work_package', id: '1', listKey: 'sprint:7' }; + const candidate = resolveCandidate(root, strayItem)!; + + expect(resolveRangeItems(root, anchor, candidate, rowsContainerFor(candidate.itemElement))).toEqual({ ok: false, reason: 'unavailable' }); + }); + + it('applies and clears the batch presentation', () => { + applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' }), selectionKey({ type: 'work_package', id: '3' })]), 'selected-description'); + + expect(itemFor('1').hasAttribute(batchSelectedAttribute)).toBe(true); + expect(itemFor('2').hasAttribute(batchSelectedAttribute)).toBe(false); + + applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '3' })]), 'selected-description'); + + expect(itemFor('1').hasAttribute(batchSelectedAttribute)).toBe(false); + expect(itemFor('3').hasAttribute(batchSelectedAttribute)).toBe(true); + }); + + // An accessible description is computed from the focused element's own + // `aria-describedby` and never inherited from an ancestor, so it belongs on + // the focus host: in Backlogs the row never receives focus, the card does. + it('describes a selected card and stops describing a deselected one', () => { + const focusHost = document.createElement('div'); + focusHost.setAttribute('data-sortable-lists--item-target', 'focus'); + itemFor('1').appendChild(focusHost); + + applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' })]), 'selected-description'); + + expect(focusHost.getAttribute('aria-describedby')).toBe('selected-description'); + expect(itemFor('1').hasAttribute('aria-describedby')).toBe(false); + + applySelectionPresentation(root, new Set(), 'selected-description'); + + expect(focusHost.hasAttribute('aria-describedby')).toBe(false); + }); + + // Nothing prunes duplicates on read, so a repeated apply is the only thing + // that catches a regression of the write-time de-duplication. + it('does not accumulate duplicate description tokens on repeated apply', () => { + applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' })]), 'selected-description'); + applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' })]), 'selected-description'); + + expect(itemFor('1').getAttribute('aria-describedby')).toBe('selected-description'); + }); + + it('leaves a description the card already had', () => { + itemFor('1').setAttribute('aria-describedby', 'card-hint'); + + applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' })]), 'selected-description'); + expect(itemFor('1').getAttribute('aria-describedby')).toBe('card-hint selected-description'); + + applySelectionPresentation(root, new Set(), 'selected-description'); + expect(itemFor('1').getAttribute('aria-describedby')).toBe('card-hint'); + }); + + it('skips the description wiring when no description element is configured', () => { + applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' })]), ''); + + expect(itemFor('1').hasAttribute('aria-describedby')).toBe(false); + expect(itemFor('1').hasAttribute(batchSelectedAttribute)).toBe(true); + }); + + it('finds the next and previous item within one list', () => { + expect(neighbourItem(root, itemFor('1'), 1)).toBe(itemFor('2')); + expect(neighbourItem(root, itemFor('2'), -1)).toBe(itemFor('1')); + }); + + it('does not step across a list boundary', () => { + expect(neighbourItem(root, itemFor('3'), 1)).toBeNull(); + }); + + // Not filtered by movability, unlike listBoundaryItem below. + it('steps onto a non-movable card with the arrow', () => { + expect(neighbourItem(root, itemFor('4'), 1)).toBe(itemFor('5')); + }); + + it('finds the first and last item of the containing list', () => { + expect(listBoundaryItem(root, itemFor('2'), 'first')).toBe(itemFor('1')); + expect(listBoundaryItem(root, itemFor('2'), 'last')).toBe(itemFor('3')); + }); + + // Sprint 8 holds movable 4 and non-movable 5 (see the fixture comment + // above): the trailing non-movable card must not become the End target. + it('skips a non-movable card at the list boundary', () => { + expect(listBoundaryItem(root, itemFor('4'), 'last')).toBe(itemFor('4')); + }); + + // A section item hosting its own list is an ownership boundary too. + it('does not step into a list nested inside the list', () => { + appendNestedList(); + + expect(neighbourItem(root, itemFor('6'), 1)).toBeNull(); + }); + + it('confines list-scoped select-all to the items the list itself owns', () => { + appendNestedList(); + + expect(liveOrderableListItems(root, itemFor('1')).map((item) => item.id)).toEqual(['1', '2', '3', '6']); + }); + + it('returns null when no movable card remains in the list', () => { + itemFor('4').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + + expect(listBoundaryItem(root, itemFor('4'), 'first')).toBeNull(); + expect(listBoundaryItem(root, itemFor('4'), 'last')).toBeNull(); + }); +}); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts new file mode 100644 index 000000000000..400bb11998bc --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts @@ -0,0 +1,330 @@ +//-- 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 { selectionKey, type SelectionAnchor, type SelectionItem, type SelectionKey } from 'core-common/batch-selection'; +import { attributeTokenList } from 'core-app/shared/helpers/dom-helpers'; +import { + isOrderableItem, + resolveItemType, + sortableListsRootSelector, + resolveItemElement, + resolveItemId, + rowOf, + sortableItemSelector, + sortableListSelector, +} from './list-dom'; + +// Distinct from `aria-current`: a card may be either, both, or neither. +export const batchSelectedAttribute = 'data-batch-selected'; + +export interface SelectionCandidate extends SelectionItem { + itemElement:HTMLElement; + // The element the consumer made focusable, and the boundary an + // interactive-descendant check stops at. + focusHost:HTMLElement; + listKey:string; + orderable:boolean; +} + +export const itemFocusTargetSelector = '[data-sortable-lists--item-target~="focus"]'; + +// Decides only whether a range stays inside the list it started in. The DOM +// id survives a morph; the type/id pair is the fallback for consumers that +// render none. +function listKeyOf(listElement:HTMLElement):string { + if (listElement.id !== '') { + return listElement.id; + } + + const type = listElement.getAttribute('data-sortable-lists--list-type-value') ?? ''; + const id = listElement.getAttribute('data-sortable-lists--list-id-value') ?? ''; + + return `${type}:${id}`; +} + +// A child belongs to the nearest root, not to any root containing it: an +// independently nested root is an ownership boundary. +function ownsElement(root:HTMLElement, element:Element):boolean { + return element.closest(sortableListsRootSelector) === root; +} + +function ownerList(root:HTMLElement, itemElement:HTMLElement):HTMLElement|null { + const list = itemElement.closest(sortableListSelector); + + return list && ownsElement(root, list) ? list : null; +} + +// Bounded by the rows container: resolveItemElement's querySelector fallback +// descends unbounded, so a section row would otherwise resolve to the first +// field of the list nested inside it. +function rowItemId(row:Element, rowsContainer:Element):string|null { + const item = resolveItemElement(row, rowsContainer); + + return item ? resolveItemId(item) : null; +} + +export function orderedItemElements(root:HTMLElement):HTMLElement[] { + return Array.from(root.querySelectorAll(sortableItemSelector)) + .filter((item) => ownsElement(root, item)); +} + +/** + * The item a gesture landed on, or null when the gesture did not land on one. + * + * A structural row such as a truncation marker is not an item, and so not a + * candidate whose selection could be refused either. + */ +export function resolveCandidate(root:HTMLElement, target:EventTarget|null):SelectionCandidate|null { + if (!(target instanceof Element) || !root.contains(target)) { + return null; + } + + const itemElement = target.closest(sortableItemSelector); + const id = itemElement ? resolveItemId(itemElement) : null; + if (!itemElement || !id || !root.contains(itemElement)) { + return null; + } + + const list = ownerList(root, itemElement); + if (!list) { + return null; + } + + // Type is half of identity: an item declaring none cannot be identified, + // and so cannot be selected. + const type = resolveItemType(itemElement); + if (type === null) { + return null; + } + + return { + type, + itemElement, + focusHost: itemElement.querySelector(itemFocusTargetSelector) ?? itemElement, + id, + listKey: listKeyOf(list), + orderable: isOrderableItem(itemElement), + }; +} + +// Live document order: a morph can reorder rows underneath a selection formed +// minutes ago. +export function orderedSelectedItems(root:HTMLElement, keys:ReadonlySet):SelectionItem[] { + return orderedItemElements(root) + .map((item) => itemIdentity(item)) + .filter((item):item is SelectionItem => item !== null && keys.has(selectionKey(item))); +} + +function itemIdentity(itemElement:Element):SelectionItem|null { + const id = resolveItemId(itemElement); + const type = resolveItemType(itemElement); + + return id && type ? { type, id } : null; +} + +export function liveOrderableItems(root:HTMLElement):SelectionItem[] { + return orderedItemElements(root) + .filter(isOrderableItem) + .map((item) => itemIdentity(item)) + .filter((item):item is SelectionItem => item !== null); +} + +export function liveOrderableListItems(root:HTMLElement, from:HTMLElement):SelectionItem[] { + return listItems(root, from) + .filter(isOrderableItem) + .map((item) => itemIdentity(item)) + .filter((item):item is SelectionItem => item !== null); +} + +export function liveOrderableKeys(root:HTMLElement):Set { + return new Set(liveOrderableItems(root).map(selectionKey)); +} + +// `unavailable` is remediable by expanding the list, most often a truncation +// marker in the span. `locked` is not: a card the user may not move sits in +// it, and expanding changes nothing. +export type RangeUnavailableReason = 'crossList'|'unavailable'|'locked'; + +export type RangeResolution = + | { ok:true; items:SelectionItem[] } + | { ok:false; reason:RangeUnavailableReason }; + +/** + * The contiguous, orderable range between the anchor and the candidate, or a + * reason the range cannot be expressed. + * + * A range that would cross a list boundary, a truncation marker or a card the + * user may not move is refused whole rather than trimmed. + */ +export function resolveRangeItems( + root:HTMLElement, + anchor:SelectionAnchor, + candidate:SelectionCandidate, + // Must be the container moves use: a row is any direct child of it, not + // necessarily an item element, so it cannot be derived from the item's own + // parent. + rowsContainer:HTMLElement|null, +):RangeResolution { + if (anchor.listKey !== candidate.listKey) { + return { ok: false, reason: 'crossList' }; + } + + if (!ownerList(root, candidate.itemElement) || !rowsContainer) { + return { ok: false, reason: 'unavailable' }; + } + + const rows = Array.from(rowsContainer.children); + const anchorRow = rows.find((row) => rowItemId(row, rowsContainer) === anchor.id); + // A candidate whose item sits outside the rows container has no row here. + const candidateRow = rowOf(rowsContainer, candidate.itemElement); + if (!anchorRow || !candidateRow) { + return { ok: false, reason: 'unavailable' }; + } + + const from = rows.indexOf(anchorRow); + const to = rows.indexOf(candidateRow); + const span = rows.slice(Math.min(from, to), Math.max(from, to) + 1); + + const items:SelectionItem[] = []; + for (const row of span) { + const item = resolveItemElement(row, rowsContainer); + const id = item ? resolveItemId(item) : null; + // A truncation marker in the span and a card the user cannot move are + // both hard boundaries, but only the first can be resolved by expanding. + if (!item || !id) { + return { ok: false, reason: 'unavailable' }; + } + + if (!isOrderableItem(item)) { + return { ok: false, reason: 'locked' }; + } + + const type = resolveItemType(item); + if (!type) { + return { ok: false, reason: 'unavailable' }; + } + + items.push({ type, id }); + } + + return { ok: true, items }; +} + +/** + * Marks the selected items and describes them to assistive technology. + * + * `describedById` points at an element the consumer renders once, holding the + * word for "selected"; an empty string skips the description entirely. + * + * `data-batch-selected` goes on the item element, `aria-describedby` on the + * focus host: an accessible description is computed from the focused + * element's own attribute and never inherited from an ancestor. + */ +export function applySelectionPresentation( + root:HTMLElement, + keys:ReadonlySet, + describedById:string, +):void { + for (const item of orderedItemElements(root)) { + const identity = itemIdentity(item); + const focusHost = item.querySelector(itemFocusTargetSelector) ?? item; + + if (identity && keys.has(selectionKey(identity))) { + item.setAttribute(batchSelectedAttribute, ''); + addDescription(focusHost, describedById); + } else { + item.removeAttribute(batchSelectedAttribute); + removeDescription(focusHost, describedById); + } + } +} + +// The card may already be described by something of its own, so the shared +// reference is added to the token list rather than replacing it. +function addDescription(item:HTMLElement, describedById:string):void { + if (describedById === '') { + return; + } + + attributeTokenList(item, 'aria-describedby').add(describedById); +} + +function removeDescription(item:HTMLElement, describedById:string):void { + if (describedById === '') { + return; + } + + const describedBy = attributeTokenList(item, 'aria-describedby'); + describedBy.remove(describedById); + + // `remove` leaves an empty attribute behind rather than dropping it, and a + // card that describes nothing should carry no `aria-describedby` at all. + if (describedBy.length === 0) { + item.removeAttribute('aria-describedby'); + } +} + +// Filtered back to the items this list owns: a nested topology puts another +// list's items inside this one's subtree. +function listItems(root:HTMLElement, from:HTMLElement):HTMLElement[] { + const list = ownerList(root, from); + + return list + ? Array.from(list.querySelectorAll(sortableItemSelector)) + .filter((item) => ownerList(root, item) === list) + : []; +} + +// Arrows step through the list as rendered, fixed cards included. Not +// symmetric with listBoundaryItem below, which does filter. +export function neighbourItem(root:HTMLElement, from:HTMLElement, offset:1|-1):HTMLElement|null { + const items = listItems(root, from); + const index = items.indexOf(from); + + if (index === -1) { + return null; + } + + return items[index + offset] ?? null; +} + +// Home/End land on the first/last *orderable* card, so a leading or trailing +// fixed card is skipped rather than becoming the jump target. +export function listBoundaryItem( + root:HTMLElement, + from:HTMLElement, + edge:'first'|'last', +):HTMLElement|null { + const items = listItems(root, from).filter(isOrderableItem); + + if (items.length === 0) { + return null; + } + + return edge === 'first' ? items[0] : items[items.length - 1]; +} diff --git a/frontend/src/stimulus/helpers/platform.ts b/frontend/src/stimulus/helpers/platform.ts new file mode 100644 index 000000000000..a80e8bace45a --- /dev/null +++ b/frontend/src/stimulus/helpers/platform.ts @@ -0,0 +1,49 @@ +//-- 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. +//++ + +interface NavigatorUAData { + platform?:string; +} + +/** + * Whether the browser is running on an Apple platform. + * + * Matters for modifier keys: Ctrl-click is the secondary-click gesture on + * macOS, where Cmd is the multi-select modifier instead. Treating Ctrl as + * multi-select there fights the contextual menu. + * + * `navigator.platform` is deprecated but still the only universally + * available signal, so `userAgentData` is preferred where a browser exposes + * it. + */ +export function isApplePlatform():boolean { + const uaData = (navigator as Navigator & { userAgentData?:NavigatorUAData }).userAgentData; + const platform = uaData?.platform ?? navigator.platform; + + return /mac|iphone|ipad|ipod/i.test(platform); +} From cc74f9d66e3c2e34b9d0b432f0269fe043c2a6b5 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 15 Aug 2026 15:15:25 +0100 Subject: [PATCH 04/22] [AGILE-361] Wire batch selection into the root Attaches selection opt-in to the sortable root: capture-phase listeners beat the card's own click handler, a drag collapses the batch onto the dragged card, and membership survives Turbo morphs and cache restores. https://community.openproject.org/wp/AGILE-361 --- .../dynamic/sortable-lists.controller.spec.ts | 883 +++++++++++++++++- .../dynamic/sortable-lists.controller.ts | 148 ++- .../dynamic/sortable-lists/drag-and-drop.ts | 3 + .../sortable-lists/list.controller.spec.ts | 1 + .../dynamic/sortable-lists/preview.spec.ts | 2 - .../dynamic/sortable-lists/preview.ts | 5 +- .../scrollable.controller.spec.ts | 1 + .../pragmatic-dnd-morph-attributes.spec.ts | 4 +- .../turbo/pragmatic-dnd-morph-attributes.ts | 12 +- 9 files changed, 1040 insertions(+), 19 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 76c264e3aa49..17ed7c05e0d0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -119,13 +119,15 @@ describe('Sortable lists controller', () => { function renderFixture({ moveUrlTemplate = '/move/{id}', optimistic = false, - }:{ moveUrlTemplate?:string|null; optimistic?:boolean } = {}) { + selectionEnabled = false, + }:{ moveUrlTemplate?:string|null; optimistic?:boolean; selectionEnabled?:boolean } = {}) { fixture.innerHTML = `
      { sourceList.append(itemRow('1'), itemRow('2'), itemRow('3')); targetList.append(itemRow('4'), itemRow('5')); return { - root, sourceList, targetList, scrollable, firstSourceItem: sourceList.querySelector('[data-sortable-lists--item-id-value="1"]')!, + root, + sourceList, + targetList, + scrollable, + firstSourceItem: sourceList.querySelector('[data-sortable-lists--item-id-value="1"]')!, + items: Array.from(root.querySelectorAll('[data-sortable-lists--item-id-value]')), }; } @@ -316,6 +323,20 @@ describe('Sortable lists controller', () => { }; } + // Layered on renderFixture, plus selection and a tabindex per item. The + // value is set at creation time: Stimulus picks up an attribute mutated + // after connect through its async MutationObserver channel, too late for a + // synchronous test. The tabindex mirrors Backlogs cards, whose item element + // is itself focusable. + function renderSelectableRoot({ + moveUrlTemplate = '/move/{id}', + }:{ moveUrlTemplate?:string|null } = {}) { + const fixtureElements = renderFixture({ moveUrlTemplate, selectionEnabled: true }); + fixtureElements.items.forEach((item) => item.setAttribute('tabindex', '0')); + + return fixtureElements; + } + beforeEach(async () => { vi.clearAllMocks(); @@ -353,6 +374,20 @@ describe('Sortable lists controller', () => { moved: '%{label} moved to position %{position} of %{total}', moved_to_list: '%{label} moved to %{list}, position %{position} of %{total}', }, + selection: { + cleared: 'Selection cleared.', + not_selectable: 'Selection unchanged. This item takes no part in this list\'s ordering.', + range_blocked: 'Selection unchanged. That range contains an item that takes no part in this list\'s ordering.', + range_restarted: { + one: 'Could not extend the range. 1 item selected.', + other: 'Could not extend the range. %{count} items selected.', + }, + range_unavailable: 'Selection unchanged. Expand this list to select that range.', + selected: { + one: '1 item selected.', + other: '%{count} items selected.', + }, + }, }, }, }, @@ -1101,6 +1136,19 @@ describe('Sortable lists controller', () => { expect(body.body.get('prev_id')).toBe('2'); }); + it('refuses a directional move for a non-movable item', async () => { + const { root, sourceList, firstSourceItem } = renderFixture(); + firstSourceItem.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + await ctx.nextFrame(); + + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + controller.moveInDirection(firstSourceItem, 'down'); + await flushPromises(); + + expect(itemIds(sourceList)).toEqual(['1', '2', '3']); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it('reports per-direction move availability for gating', async () => { const { root, firstSourceItem } = renderFixture(); await ctx.nextFrame(); @@ -1112,6 +1160,19 @@ describe('Sortable lists controller', () => { }); }); + // A card that takes no part in ordering must not be offered a move the + // click path would then refuse. + it('reports no available direction for a non-movable item', async () => { + const { root, firstSourceItem } = renderFixture(); + firstSourceItem.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + + expect(controller.moveAvailability(firstSourceItem)).toEqual({ + top: false, up: false, down: false, bottom: false, + }); + }); + it('reports null availability for an item outside any owned list', async () => { const { root } = renderFixture(); await ctx.nextFrame(); @@ -1227,4 +1288,822 @@ describe('Sortable lists controller', () => { ); }); }); + + // Collapsing a batch and selecting the dragged card are different things: + // with nothing selected, a drag must not manufacture a one-card batch. + it('leaves an empty selection empty when a drag starts with nothing selected', async () => { + const { root, firstSourceItem } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + + controller.collapseSelectionForDrag(firstSourceItem); + + expect(document.querySelector('[data-batch-selected]')).toBeNull(); + }); + + // A drag that narrows a larger batch to one card is a count change a + // screen-reader user has to hear. + it('announces the new count when a drag collapses a multi-card batch', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + items[0].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); + items[1].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true })); + items[2].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true })); + announceSpy.mockClear(); + + controller.collapseSelectionForDrag(items[0]); + + expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0]]); + expect(announceSpy.mock.calls.map((call) => [call[0], call[1]])).toEqual([ + ['1 item selected.', { politeness: 'polite' }], + ]); + }); + + const click = (element:HTMLElement, init:MouseEventInit = {}) => { + element.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ...init })); + }; + + // A gesture the root leaves alone keeps its native default, so a real + // anchor would navigate the test page away. Cancelled in the bubble phase, + // after the root's capture-phase listener has had its turn. + const clickWithoutNavigating = (element:HTMLElement, init:MouseEventInit = {}) => { + const blockNavigation = (event:Event) => event.preventDefault(); + window.addEventListener('click', blockNavigation, { once: true }); + + click(element, init); + + window.removeEventListener('click', blockNavigation); + }; + + const isSelected = (element:HTMLElement) => element.hasAttribute('data-batch-selected'); + + // A restored page brings its markup back but not the model. Not driven + // through turbo:before-cache: that also fires for the details-pane + // navigation, where clearing would strip a live batch. + it('clears stale batch presentation left in the DOM when it connects', async () => { + fixture.innerHTML = ` +
      +
        +
      • +
      +
      + `; + const stale = fixture.querySelector('#stale-root')!; + const row = stale.querySelector('[data-sortable-lists--item-id-value="1"]')!; + expect(row.hasAttribute('data-batch-selected')).toBe(true); + + await ctx.nextFrame(); + + expect(row.hasAttribute('data-batch-selected')).toBe(false); + }); + + it('selects only the clicked card on a plain click', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + + click(items[0]); + + expect(isSelected(items[0])).toBe(true); + expect(isSelected(items[1])).toBe(false); + }); + + it('lets a plain click continue to the navigation handler', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + const event = new MouseEvent('click', { bubbles: true, cancelable: true }); + + items[0].dispatchEvent(event); + + expect(event.defaultPrevented).toBe(false); + }); + + it('lets a plain click on a non-movable card continue to navigate without selecting', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const event = new MouseEvent('click', { bubbles: true, cancelable: true }); + + items[0].dispatchEvent(event); + + expect(isSelected(items[0])).toBe(false); + expect(event.defaultPrevented).toBe(false); + }); + + // A fixed card does not join the batch, but the click still opens its + // details pane, so it must not leave an unrelated batch behind. + it('clears an existing batch on a plain click that lands on a non-movable card', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { metaKey: true }); + items[2].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const event = new MouseEvent('click', { bubbles: true, cancelable: true }); + + items[2].dispatchEvent(event); + + expect(items.some(isSelected)).toBe(false); + expect(event.defaultPrevented).toBe(false); + }); + + it('announces the new count when a plain click collapses a multi-card batch', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { metaKey: true }); + click(items[2], { metaKey: true }); + announceSpy.mockClear(); + + click(items[0]); + + expect(items.filter(isSelected)).toEqual([items[0]]); + expect(announcedMessages()).toEqual([ + ['1 item selected.', { politeness: 'polite' }], + ]); + }); + + // The click also opens the details pane, which is its own feedback. + it('stays silent on a plain click that does not change the selected count', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + announceSpy.mockClear(); + + click(items[1]); + + expect(items.filter(isSelected)).toEqual([items[1]]); + expect(announceSpy).not.toHaveBeenCalled(); + }); + + // An unconsumed modified gesture would reach the card's own click handler + // and open the details pane on a selection toggle. + it('consumes a modified click during a busy move without changing the selection', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + root.setAttribute('data-sortable-lists-busy', 'true'); + const event = new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true }); + + items[0].dispatchEvent(event); + + expect(event.defaultPrevented).toBe(true); + expect(items.some(isSelected)).toBe(false); + }); + + it('toggles a card without navigating on a meta click', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + const event = new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true }); + + click(items[0]); + items[1].dispatchEvent(event); + + expect(isSelected(items[0])).toBe(true); + expect(isSelected(items[1])).toBe(true); + expect(event.defaultPrevented).toBe(true); + }); + + it('selects a range from the anchor on a shift click', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + + click(items[0]); + click(items[2], { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0], items[1], items[2]]); + }); + + it('keeps the anchor fixed so a range can be resized', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + + click(items[0]); + click(items[2], { shiftKey: true }); + click(items[1], { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0], items[1]]); + }); + + it('starts a single selection when shift is pressed without an anchor', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + + click(items[1], { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[1]]); + }); + + // The count rule alone would fall silent whenever the prior selection was + // already a single card, leaving the failed range with no feedback. + it('announces a distinct message when a cross-list Shift-click restarts the range', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + announceSpy.mockClear(); + + click(items[3], { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[3]]); + expect(announcedMessages()).toEqual([ + ['Could not extend the range. 1 item selected.', { politeness: 'polite' }], + ]); + }); + + // Same restart, reached the other way: an anchor pruned after a morph + // leaves a one-card selection with nothing to range from. + it('announces a distinct message when a Shift-click restarts the range after its anchor was pruned', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { metaKey: true }); + items[1].remove(); + morphRoot(root); + await ctx.nextFrame(); + announceSpy.mockClear(); + + click(items[2], { shiftKey: true }); + + // items[1] was removed from the document above; a stale reference to it + // would still carry the attribute it had before removal, so membership + // is checked on the two live cards rather than filtering the whole + // `items` array. + expect(isSelected(items[0])).toBe(false); + expect(isSelected(items[2])).toBe(true); + expect(announcedMessages()).toEqual([ + ['Could not extend the range. 1 item selected.', { politeness: 'polite' }], + ]); + }); + + it('tells the user to expand the list when a range crosses a truncation marker', async () => { + const { sourceList, items } = renderSelectableRoot(); + await ctx.nextFrame(); + const marker = document.createElement('li'); + marker.setAttribute('data-sortable-lists-prev-item-id', '1'); + marker.setAttribute('data-sortable-lists-omitted-count', '9'); + sourceList.insertBefore(marker, items[1]); + + click(items[0]); + announceSpy.mockClear(); + click(items[2], { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0]]); + expect(announcedMessages()).toEqual([ + ['Selection unchanged. Expand this list to select that range.', { politeness: 'polite' }], + ]); + }); + + // Expanding the list can never resolve a non-movable card in the span. + it('tells the user a locked card blocks the range rather than to expand the list', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[1].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + + click(items[0]); + announceSpy.mockClear(); + click(items[2], { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0]]); + expect(announcedMessages()).toEqual([ + ['Selection unchanged. That range contains an item that takes no part in this list\'s ordering.', { politeness: 'polite' }], + ]); + }); + + it('preserves the batch and announces when a non-movable card is meta clicked', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[1].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const event = new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true }); + + click(items[0]); + announceSpy.mockClear(); + items[1].dispatchEvent(event); + + expect(items.filter(isSelected)).toEqual([items[0]]); + expect(event.defaultPrevented).toBe(true); + expect(announcedMessages()).toEqual([ + ['Selection unchanged. This item takes no part in this list\'s ordering.', { politeness: 'polite' }], + ]); + }); + + it('ignores gestures that start on an interactive descendant', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + const link = document.createElement('a'); + link.href = '/somewhere'; + items[0].appendChild(link); + + clickWithoutNavigating(link, { metaKey: true }); + + expect(items.some(isSelected)).toBe(false); + }); + + // Stopping the walk at the item would classify every gesture on a Backlogs + // card as interactive, since the card itself carries tabindex. Only + // observable once the focus host is a distinct, nested element. + it('selects when a gesture lands on a non-interactive descendant of a nested focus host', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + const focusHost = document.createElement('div'); + focusHost.setAttribute('data-sortable-lists--item-target', 'focus'); + focusHost.tabIndex = 0; + const label = document.createElement('span'); + focusHost.appendChild(label); + items[0].appendChild(focusHost); + + click(label); + + expect(isSelected(items[0])).toBe(true); + }); + + // A nested focus host sits below the item, so a gesture landing elsewhere + // on the row never reaches it walking up: stopping there unconditionally + // would leave the walk unbounded past the item. + it('still selects when a gesture lands on the row outside a nested focus host', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + const focusHost = document.createElement('div'); + focusHost.setAttribute('data-sortable-lists--item-target', 'focus'); + focusHost.tabIndex = 0; + items[0].appendChild(focusHost); + + click(items[0]); + + expect(isSelected(items[0])).toBe(true); + }); + + it('does not select at all when selection is not enabled', async () => { + const { items } = renderFixture(); + await ctx.nextFrame(); + + click(items[0]); + + expect(items.some(isSelected)).toBe(false); + }); + + // A Turbo morph can flip the permission-gated value on a live root without + // reconnecting the controller, so the gestures follow the flip. + it('stops selecting when selection is disabled on the live root', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + expect(isSelected(items[0])).toBe(true); + + root.setAttribute('data-sortable-lists-selection-enabled-value', 'false'); + await ctx.nextFrame(); + + expect(items.some(isSelected)).toBe(false); + click(items[1]); + expect(items.some(isSelected)).toBe(false); + }); + + it('starts selecting when selection is enabled on the live root', async () => { + const { root, items } = renderFixture(); + await ctx.nextFrame(); + click(items[0]); + expect(items.some(isSelected)).toBe(false); + + items.forEach((item) => item.setAttribute('tabindex', '0')); + root.setAttribute('data-sortable-lists-selection-enabled-value', 'true'); + await ctx.nextFrame(); + + click(items[0]); + expect(isSelected(items[0])).toBe(true); + }); + + // A cached root re-attaching reconnects the same controller instance, and + // Stimulus fires no value callback for an unchanged attribute. + it('restores selection when the same root reconnects', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + expect(isSelected(items[0])).toBe(true); + + root.remove(); + await ctx.nextFrame(); + fixture.append(root); + await ctx.nextFrame(); + + click(items[1]); + expect(isSelected(items[1])).toBe(true); + }); + + const keydown = (target:HTMLElement, key:string, init:KeyboardEventInit = {}) => { + const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true, ...init }); + target.dispatchEvent(event); + return event; + }; + + it('toggles the focused card on Space', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + const event = keydown(items[0], ' '); + + expect(isSelected(items[0])).toBe(true); + expect(event.defaultPrevented).toBe(true); + }); + + it('extends the range on Shift+Space', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + items[2].focus(); + + keydown(items[2], ' ', { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0], items[1], items[2]]); + }); + + it('moves focus within the list on ArrowDown', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + keydown(items[0], 'ArrowDown'); + + expect(document.activeElement).toBe(items[1]); + }); + + // An unconsumed arrow falls through to native scrolling, moving the page + // while focus stays put. + it('consumes ArrowUp at the first card even though focus cannot move', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + const event = keydown(items[0], 'ArrowUp'); + + expect(document.activeElement).toBe(items[0]); + expect(event.defaultPrevented).toBe(true); + }); + + it('consumes ArrowDown at the last card of a list even though focus cannot move', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[2].focus(); + + const event = keydown(items[2], 'ArrowDown'); + + expect(document.activeElement).toBe(items[2]); + expect(event.defaultPrevented).toBe(true); + }); + + it('extends the range while moving focus on Shift+ArrowDown', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + items[0].focus(); + + keydown(items[0], 'ArrowDown', { shiftKey: true }); + + expect(document.activeElement).toBe(items[1]); + expect(items.filter(isSelected)).toEqual([items[0], items[1]]); + }); + + it('moves focus to the list boundaries on Home and End', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[1].focus(); + + keydown(items[1], 'End'); + expect(document.activeElement).toBe(items[2]); + + keydown(items[2], 'Home'); + expect(document.activeElement).toBe(items[0]); + }); + + // Same boundary-scroll problem as the plain arrows above. + it('consumes Home at the first card even though focus cannot move', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + const event = keydown(items[0], 'Home'); + + expect(document.activeElement).toBe(items[0]); + expect(event.defaultPrevented).toBe(true); + }); + + it('consumes End at the last card even though focus cannot move', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[2].focus(); + + const event = keydown(items[2], 'End'); + + expect(document.activeElement).toBe(items[2]); + expect(event.defaultPrevented).toBe(true); + }); + + // The design's keyboard table specifies the first/last *movable* card for + // Home/End (unlike plain arrow steps, which are not qualified that way). + it('skips a non-movable card when moving to the list boundary on End', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[2].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + items[0].focus(); + + keydown(items[0], 'End'); + + expect(document.activeElement).toBe(items[1]); + }); + + // Focus cannot move further, but the range still resizes to the boundary. + it('resizes the range to the list boundary on Shift+Home even when focus cannot move', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[2]); + items[2].focus(); + keydown(items[2], 'ArrowUp'); + keydown(items[1], 'ArrowUp'); + + keydown(items[0], 'Home', { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0], items[1], items[2]]); + }); + + // The fixture spans two lists: items[0..2] in the source, items[3..4] in + // the target. + it('selects every movable card in the focused list on meta A', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[1].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + items[0].focus(); + + keydown(items[0], 'a', { metaKey: true }); + + const selected = items.filter(isSelected); + expect(selected).toEqual([items[0], items[2]]); + expect(selected).not.toContain(items[1]); + }); + + // Would pass under a root-wide select-all. + it('leaves movable cards of other lists unselected on meta A', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + keydown(items[0], 'a', { metaKey: true }); + + expect(isSelected(items[3])).toBe(false); + expect(isSelected(items[4])).toBe(false); + }); + + it('selects every movable card of the focused list on ctrl A', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + keydown(items[0], 'a', { ctrlKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0], items[1], items[2]]); + }); + + it('does nothing on a bare "a" without a modifier', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + const event = keydown(items[0], 'a'); + + expect(items.some(isSelected)).toBe(false); + expect(event.defaultPrevented).toBe(false); + }); + + // Focusing something other than the first card rules out an anchor that + // merely coincides with a document-order fallback. + it('anchors the batch on the focused card after meta A when it is movable', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[2].focus(); + + keydown(items[2], 'a', { metaKey: true }); + keydown(items[0], ' ', { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[0], items[1], items[2]]); + }); + + // A subsequent Shift gesture proves the fallback anchor is set: no anchor + // at all would collapse the range to a single card. + it('anchors the batch on the first movable card after meta A when the focused card is not movable', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + items[0].focus(); + + keydown(items[0], 'a', { metaKey: true }); + keydown(items[2], ' ', { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[1], items[2]]); + }); + + it('clears the batch on Escape', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + items[0].focus(); + + keydown(items[0], 'Escape'); + + expect(items.some(isSelected)).toBe(false); + }); + + // Escape clears local state only, so the busy gate must not swallow it. + it('clears the batch on Escape even during a busy move', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + items[0].focus(); + root.setAttribute('data-sortable-lists-busy', 'true'); + + keydown(items[0], 'Escape'); + + expect(items.some(isSelected)).toBe(false); + }); + + // An unconsumed Escape still reaches dialogs and menus. + it('leaves Escape unconsumed when there is nothing to clear', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + const event = keydown(items[0], 'Escape'); + + expect(event.defaultPrevented).toBe(false); + }); + + // BatchSelection#toggle re-bases the anchor even on a deselect, so an + // emptied selection leaves one behind. The Shift gesture below would range + // from it had Escape not dropped it. + it('clears the stale anchor left by a deselect, not just a visible selection', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + keydown(items[0], ' '); + keydown(items[0], ' '); + + keydown(items[0], 'Escape'); + keydown(items[2], ' ', { shiftKey: true }); + + expect(items.filter(isSelected)).toEqual([items[2]]); + }); + + // A click on empty column space focuses nothing, and Escape there still + // means "drop the selection". + it('clears the batch on Escape when focus has left the root entirely', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + + keydown(document.body, 'Escape'); + + expect(items.some(isSelected)).toBe(false); + }); + + // An interactive descendant keeps its own clicks and Space, but Escape + // still belongs to the selection. + it('clears the batch on Escape from a link inside a card', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + const link = document.createElement('a'); + link.href = '#'; + items[1].append(link); + + keydown(link, 'Escape'); + + expect(items.some(isSelected)).toBe(false); + }); + + it('leaves Escape inside an open menu to the menu', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + const menu = document.createElement('div'); + menu.setAttribute('role', 'menu'); + const menuButton = document.createElement('button'); + menu.append(menuButton); + root.append(menu); + + const event = keydown(menuButton, 'Escape'); + + expect(isSelected(items[0])).toBe(true); + expect(event.defaultPrevented).toBe(false); + }); + + it('ignores an Escape another handler already consumed', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + items[1].addEventListener('keydown', (event) => event.preventDefault(), { once: true }); + + keydown(items[1], 'Escape'); + + expect(isSelected(items[0])).toBe(true); + }); + + it('leaves Enter to the navigation handler', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[0].focus(); + + const event = keydown(items[0], 'Enter'); + + expect(event.defaultPrevented).toBe(false); + }); + + it('ignores keys from an interactive descendant', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + const input = document.createElement('input'); + items[0].appendChild(input); + input.focus(); + + keydown(input, ' '); + + expect(items.some(isSelected)).toBe(false); + }); + + function morphRoot(root:HTMLElement) { + root.dispatchEvent(new CustomEvent('turbo:morph-element', { bubbles: true })); + } + + describe('reconciling the batch after a morph', () => { + it('reapplies the batch presentation after a morph strips it', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + items[0].removeAttribute('data-batch-selected'); + + morphRoot(root); + await ctx.nextFrame(); + + expect(isSelected(items[0])).toBe(true); + }); + + it('clears a stale marker the morph preserved', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + items[2].setAttribute('data-batch-selected', ''); + + morphRoot(root); + await ctx.nextFrame(); + + expect(isSelected(items[2])).toBe(false); + }); + + it('announces the new count when a morph prune removes a selected card', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { metaKey: true }); + click(items[2], { metaKey: true }); + items[1].remove(); + announceSpy.mockClear(); + + morphRoot(root); + await ctx.nextFrame(); + + expect(announcedMessages()).toEqual([ + ['2 items selected.', { politeness: 'polite' }], + ]); + }); + + // renderSelection runs on every morph and decides from membership alone, + // so a prune that drops nothing has to stay silent by itself. + it('stays silent when a morph prunes nothing', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { metaKey: true }); + announceSpy.mockClear(); + + morphRoot(root); + await ctx.nextFrame(); + + expect(announceSpy).not.toHaveBeenCalled(); + }); + + // selectedIds() filters to elements still in the document, so it would + // pass even with the model unpruned. The anchor is the one place an + // unpruned model is observable: a dangling one makes the Shift+click + // below report an unavailable range instead of restarting the selection. + it('drops a removed member and its stale anchor from the model', async () => { + const { root, items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { metaKey: true }); + items[1].remove(); + + morphRoot(root); + await ctx.nextFrame(); + click(items[2], { shiftKey: true }); + + expect(isSelected(items[0])).toBe(false); + expect(isSelected(items[2])).toBe(true); + }); + }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index 2c5b1df1d9d2..052959e35a14 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -47,6 +47,7 @@ import { } from './sortable-lists/drag-and-drop'; import { captureRowPositions, + isOrderableItem, reorderRows, resolveDirectionalPreviousItemId, resolveItemId, @@ -61,23 +62,27 @@ import { type MoveAvailability, type MoveDirection, } from './sortable-lists/list-dom'; +import { SelectionOrchestrator, type SelectionHost } from './sortable-lists/selection-orchestrator'; type CleanupFn = () => void; type ElementDropPayload = ElementEventPayloadMap['onDrop']; type MoveResult = { ok:true }|{ ok:false; showToast:boolean }; interface MoveAnnouncementContext { label:string|null; listName:string|null; crossList:boolean } -export default class SortableListsController extends Controller implements SortableListsRoot { +export default class SortableListsController extends Controller implements SortableListsRoot, SelectionHost { static outlets = ['sortable-lists--list', 'sortable-lists--item', 'sortable-lists--scrollable']; static values = { moveUrlTemplate: String, moveUrlTemplates: Object, optimistic: { type: Boolean, default: false }, + selectionEnabled: { type: Boolean, default: false }, + announcementScope: { type: String, default: 'js.sortable_lists.selection' }, + selectionDescriptionId: { type: String, default: '' }, }; declare readonly sortableListsListOutlets:import('./sortable-lists/list.controller').default[]; - declare readonly sortableListsItemOutlets:RootAwareChild[]; + declare readonly sortableListsItemOutlets:(RootAwareChild & { focusItem():void })[]; declare readonly sortableListsScrollableOutlets:RootAwareChild[]; declare readonly moveUrlTemplateValue:string; @@ -85,6 +90,11 @@ export default class SortableListsController extends Controller imp declare readonly moveUrlTemplatesValue:Record; declare readonly hasMoveUrlTemplatesValue:boolean; declare readonly optimisticValue:boolean; + declare readonly selectionEnabledValue:boolean; + declare readonly announcementScopeValue:string; + declare readonly selectionDescriptionIdValue:string; + + private selection?:SelectionOrchestrator; private monitorCleanupFn?:CleanupFn; private healScheduled = false; @@ -99,14 +109,115 @@ export default class SortableListsController extends Controller imp }, }); this.element.addEventListener('turbo:morph-element', this.scheduleRegistrationHeal); + // The value callback misses a reconnect of the same controller instance: + // Stimulus fires it only when the attribute string changes, and a cached + // root re-attaching still carries the old one. setupSelection is guarded + // against the overlap. + if (this.selectionEnabledValue) { + this.setupSelection(); + } } disconnect():void { this.element.removeEventListener('turbo:morph-element', this.scheduleRegistrationHeal); + this.teardownSelection(); this.monitorCleanupFn?.(); this.monitorCleanupFn = undefined; } + // A Turbo morph can toggle the permission-gated value on a live root + // without reconnecting the controller, so the gestures follow the flip + // rather than waiting for one. + selectionEnabledValueChanged():void { + if (this.selectionEnabledValue) { + this.setupSelection(); + } else { + this.teardownSelection(); + } + } + + // Constructed only for a consumer that opted in, listeners and all: a + // root that never opted in must not swallow Space or the arrows. + private setupSelection():void { + // The callback also fires for a later write of the same value, which + // must not stack listeners or replace the live model. + if (this.selection) { + return; + } + + this.selection = new SelectionOrchestrator(this); + // Capture phase: a modified gesture has to be consumed before the + // card's own navigation listener sees it, whichever connected first. + this.element.addEventListener('click', this.onSelectionClick, true); + this.element.addEventListener('keydown', this.onSelectionKeydown, true); + // At the document, in the bubble phase: clearing must not depend on + // focus sitting on a card row, and an overlay's Escape runs first. + document.addEventListener('keydown', this.onSelectionEscape); + // A restored page brings its markup back but not the model, so batch + // presentation present at connect time claims a selection nothing + // holds. Not `turbo:before-cache`: that also fires for the details-pane + // navigation that morphs this page in place, where the controller never + // went away and the highlight has to survive. + this.selection.clearPresentation(); + } + + private teardownSelection():void { + if (!this.selection) { + return; + } + + this.element.removeEventListener('click', this.onSelectionClick, true); + this.element.removeEventListener('keydown', this.onSelectionKeydown, true); + document.removeEventListener('keydown', this.onSelectionEscape); + this.selection.teardown(); + this.selection = undefined; + } + + private readonly onSelectionClick = (event:MouseEvent):void => { + this.selection?.handleClick(event); + }; + + private readonly onSelectionKeydown = (event:KeyboardEvent):void => { + this.selection?.handleKeydown(event); + }; + + private readonly onSelectionEscape = (event:KeyboardEvent):void => { + this.selection?.handleEscape(event); + }; + + get rootElement():HTMLElement { + return this.element; + } + + get announcementScope():string { + return this.announcementScopeValue; + } + + get descriptionId():string { + return this.selectionDescriptionIdValue; + } + + // Through the item's own outlet, so the consumer decides which element + // inside the row holds the tab stop. + focusItem(target:HTMLElement):void { + const outlet = this.sortableListsItemOutlets.find((item) => item.element === target); + + if (outlet) { + outlet.focusItem(); + } else { + target.focus(); + } + } + + // Live ordered membership, for AGILE-278's batch move. + selectedIds():string[] { + return this.selection?.selectedIds() ?? []; + } + + collapseSelectionForDrag(itemElement:HTMLElement):void { + this.selection?.collapseForDrag(itemElement); + } + // A morph desyncs the children's drag-and-drop state in two ways. Stimulus // outlet-connected callbacks do not fire reliably for elements a morph // replaces, so those children never receive the root reference and refuse @@ -144,6 +255,14 @@ export default class SortableListsController extends Controller imp child.connectRoot(this); child.reregister(); }); + + // Once per morph batch, not per disconnect: a morph can replace a row + // with a fresh element for the same work package, and the disconnect + // alone would drop a member about to come straight back. + // Re-synced on every morph whether or not prune dropped anything: a + // morph can strip or preserve the marker attribute independently of + // the model. + this.selection?.reconcile(); }); }; @@ -175,19 +294,27 @@ export default class SortableListsController extends Controller imp return this.element.hasAttribute(sortableListsBusyAttribute); } - // Availability mirrors executability: a direction is offered exactly when the - // move resolver can produce a target for it. This keeps the menu honest about - // truncated lists, where a one-step move across the hidden block is not - // addressable. Null means the item is not in an owned list (yet). The result - // is a snapshot for menu gating; the click path re-resolves the live DOM. + // A direction is offered exactly when the move resolver can produce a + // target for it, and never for an item moveInDirection would refuse below. + // Null means the item is not in an owned list yet. A snapshot for menu + // gating; the click path re-resolves the live DOM. moveAvailability(itemElement:HTMLElement):MoveAvailability|null { + if (!isOrderableItem(itemElement)) { + return { + top: false, up: false, down: false, bottom: false, + }; + } + const list = this.ownerListOf(itemElement); return list ? resolveMoveAvailability({ itemElement, rowsContainer: list.rowsContainer }) : null; } moveInDirection(itemElement:HTMLElement, direction:MoveDirection):void { - if (this.busy) { + // The menu is rendered server-side from a permission check that does not + // know about per-work-package movability, so a stale or over-permissive + // menu must not execute a move the server will refuse. + if (this.busy || !isOrderableItem(itemElement)) { return; } @@ -212,6 +339,10 @@ export default class SortableListsController extends Controller imp return; } + // Last, after every resolution above: several of them bail, and + // collapsing earlier would destroy the batch for a move that never runs. + this.selection?.collapseForMove(itemElement); + void this.performMove({ sourceRow, rowsContainer: list.rowsContainer, @@ -483,4 +614,5 @@ export default class SortableListsController extends Controller imp void announce(message, { politeness: 'assertive' }); } + } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts index 2cae90ec252e..812fdb8972e1 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.ts @@ -101,6 +101,9 @@ export interface SortableListsRoot { // The rows container of the item's innermost owning list, or null when the // item is not (yet) inside a list the root knows about. ownerRowsContainer(itemElement:HTMLElement):HTMLElement|null; + // A drag moves exactly one item until AGILE-278 lands, so it collapses any + // wider batch onto the dragged card. + collapseSelectionForDrag(itemElement:HTMLElement):void; } // Implemented by the list, item and scrollable controllers so the root can diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts index 018b9a38d8f4..e1eaa25599f0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts @@ -78,6 +78,7 @@ describe('Sortable lists list controller', () => { moveAvailability: vi.fn(() => null), ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), + collapseSelectionForDrag: vi.fn(), }; } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts index 69b4697601d7..2595cc54dff1 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.spec.ts @@ -37,7 +37,6 @@ describe('sortable lists drag preview', () => { article.setAttribute('data-dragging', 'source'); article.setAttribute('data-drop-position', 'top'); article.setAttribute('data-drop-position-owner', '123'); - article.setAttribute('data-selected', ''); article.setAttribute('aria-current', 'true'); article.setAttribute('aria-roledescription', 'draggable'); @@ -65,7 +64,6 @@ describe('sortable lists drag preview', () => { expect(element.hasAttribute('data-dragging')).toBe(false); expect(element.hasAttribute('data-drop-position')).toBe(false); expect(element.hasAttribute('data-drop-position-owner')).toBe(false); - expect(element.hasAttribute('data-selected')).toBe(false); expect(element.hasAttribute('aria-current')).toBe(false); expect(element.hasAttribute('aria-roledescription')).toBe(false); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts index f92bd1a9c41e..df42edadfa61 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/preview.ts @@ -33,13 +33,16 @@ // Attributes stripped from the cloned drag preview so it carries no behaviour or // stale interaction state. The dynamic `data-*--*-target` attributes are removed // separately in sanitizePreview. +// `data-batch-selected` is deliberately absent: it lives on the sortable +// item element (the row, in Backlogs), while the preview target is the card +// — the row's child, never its clone. It can never end up on this clone, so +// stripping it here would be dead code. const PREVIEW_STRIPPED_ATTRIBUTES = [ 'data-controller', 'data-action', 'data-dragging', 'data-drop-position', 'data-drop-position-owner', - 'data-selected', 'aria-current', 'aria-describedby', 'aria-disabled', diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts index 09bc7bcb20d9..8729e01054bc 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts @@ -74,6 +74,7 @@ describe('Sortable lists scrollable controller', () => { moveAvailability: vi.fn(() => null), ownerListElementOf: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), + collapseSelectionForDrag: vi.fn(), }; } diff --git a/frontend/src/turbo/pragmatic-dnd-morph-attributes.spec.ts b/frontend/src/turbo/pragmatic-dnd-morph-attributes.spec.ts index 4a25a78ef827..77399faf58f8 100644 --- a/frontend/src/turbo/pragmatic-dnd-morph-attributes.spec.ts +++ b/frontend/src/turbo/pragmatic-dnd-morph-attributes.spec.ts @@ -92,8 +92,8 @@ describe('Pragmatic DnD morph attribute preservation', () => { }); }); - it('preserves the split view selection markers during Turbo morphs', () => { - ['data-selected', 'aria-current'].forEach((attributeName) => { + it('preserves the batch selection and current-work-package markers during Turbo morphs', () => { + ['data-batch-selected', 'aria-current'].forEach((attributeName) => { const element = appendSortableRow(); const event = beforeMorphAttributeEvent(attributeName); diff --git a/frontend/src/turbo/pragmatic-dnd-morph-attributes.ts b/frontend/src/turbo/pragmatic-dnd-morph-attributes.ts index 1efac1248b15..9717e24baeef 100644 --- a/frontend/src/turbo/pragmatic-dnd-morph-attributes.ts +++ b/frontend/src/turbo/pragmatic-dnd-morph-attributes.ts @@ -27,6 +27,7 @@ //++ import { sortableListsRootSelector } from '../stimulus/controllers/dynamic/sortable-lists/list-dom'; +import { batchSelectedAttribute } from '../stimulus/controllers/dynamic/sortable-lists/selection'; // `data-dragging` is shared with the legacy generic-drag-and-drop controller, // so preservation is scoped to elements inside a sortable-lists root. @@ -45,10 +46,13 @@ const preservedAttributes = new Set([ 'data-drop-position', 'data-drop-position-owner', 'data-drop-container', - // Split view selection markers set by the backlogs work-package - // controller; the controller only re-syncs them on `turbo:visit`, so a - // morph would otherwise strip the highlight of the open work package. - 'data-selected', + // Batch membership, owned by the sortable-lists root. The root reconciles it + // after every morph batch anyway, but preserving it avoids a visible flash + // between the morph and that reconciliation. + batchSelectedAttribute, + // Current-work-package marker set by the backlogs work-package controller, + // which only re-syncs it on `turbo:visit`; a morph would otherwise strip + // the highlight of the open work package. 'aria-current', ]); From b7892e6db97b3c1cf57ed4a4959a5ace7dfdb0b9 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 15 Aug 2026 15:15:33 +0100 Subject: [PATCH 05/22] [AGILE-361] Enable batch selection in Backlogs Opts the sprint planning page in, speaks "work package" through the announcement scope, and describes membership through one shared hidden element every selected card references via aria-describedby. https://community.openproject.org/wp/AGILE-361 --- .../common/border_box_list_component.sass | 13 +- .../backlogs/work-package.controller.spec.ts | 70 ++++---- .../backlogs/work-package.controller.ts | 28 +--- .../concerns/work_package_movability.rb | 13 ++ .../selection_description_component.html.erb | 34 ++++ .../selection_description_component.rb | 38 +++++ .../work_package_card_list_item_component.rb | 29 +--- .../app/helpers/backlogs/common_helper.rb | 10 ++ .../backlogs/backlog/_backlog_list.html.erb | 4 + .../app/views/backlogs/backlog/show.html.erb | 4 + modules/backlogs/config/locales/js-en.yml | 13 ++ .../backlogs/bucket_component_spec.rb | 12 +- .../selection_description_component_spec.rb | 51 ++++++ .../backlogs/sprint_component_spec.rb | 12 +- .../work_package_card_list_component_spec.rb | 2 +- ...k_package_card_list_item_component_spec.rb | 25 ++- .../helpers/backlogs/common_helper_spec.rb | 33 ++++ .../spec/requests/backlogs/backlog_spec.rb | 37 +++++ .../backlogs/spec/support/pages/backlog.rb | 150 +++++++++++++++--- 19 files changed, 457 insertions(+), 121 deletions(-) create mode 100644 modules/backlogs/app/components/backlogs/selection_description_component.html.erb create mode 100644 modules/backlogs/app/components/backlogs/selection_description_component.rb create mode 100644 modules/backlogs/spec/components/backlogs/selection_description_component_spec.rb diff --git a/app/components/open_project/common/border_box_list_component.sass b/app/components/open_project/common/border_box_list_component.sass index b6ebd825a0ca..b9bef95c1eff 100644 --- a/app/components/open_project/common/border_box_list_component.sass +++ b/app/components/open_project/common/border_box_list_component.sass @@ -61,7 +61,7 @@ > ul > .Box-row:first-of-type border-top-color: transparent - &:has(> .Box-card[data-selected]) + &[data-batch-selected] border-top-color: var(--box-list-item-selected-border-color) &:has(> .Box-card[aria-current="true"]) @@ -193,10 +193,11 @@ &:has(> .Box-card:focus-visible) background-color: var(--bgColor-muted) -// Selected state: applied synchronously on activation by the -// backlogs--work-package controller (single selection), so the list reacts -// before the split screen has loaded. -.Box-row:has(> .Box-card[data-selected]) +// Batch selection is written on the `.Box-row` itself, while the +// current-work-package rule below matches `aria-current` on the `.Box-card` +// nested inside it. The two selectors are asymmetric because the attributes +// are. +.Box-row[data-batch-selected] background-color: var(--box-list-item-selected-bg-color) @include op-box-list-item-edge-borders(var(--box-list-item-selected-border-color)) @@ -206,7 +207,7 @@ // Current state: the card is open in the split screen. aria-current is // maintained by the backlogs--work-package controller once the URL reflects -// the details pane; the strong border wins over a simultaneous selected state. +// the details pane; the strong border wins over a simultaneous batch selection. .Box-row:has(> .Box-card[aria-current="true"]) @include op-box-list-item-edge-borders(var(--box-list-item-pressed-border-color)) diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts index 9e096f2aed93..dfed73434f28 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.spec.ts @@ -68,18 +68,26 @@ describe('Backlogs work package controller', () => { vi.restoreAllMocks(); }); + // The card is wrapped in its row, matching production: `data-batch-selected` + // is written on the row (the
    • ), never on the card itself (see + // sortable-lists/selection.ts). This controller only ever touches the card + // (`this.element`), so wrapping it here is what makes "leaves batch + // membership alone" below a meaningful assertion instead of one that would + // pass unchanged even if the two elements were confused. function renderWorkPackage() { fixture.innerHTML = ` -
      - Work package -
      +
    • +
      + Work package +
      +
      • `; return fixture.querySelector('[data-controller="backlogs--work-package"]')!; @@ -113,6 +121,7 @@ describe('Backlogs work package controller', () => { expect(event.defaultPrevented).toBe(true); expect(navigation.openSplitPane).toHaveBeenCalledTimes(1); expect(navigation.openFullPane).not.toHaveBeenCalled(); + expect(workPackage.hasAttribute('aria-current')).toBe(false); }); it('opens the full pane when Shift+Enter is pressed', async () => { @@ -139,13 +148,12 @@ describe('Backlogs work package controller', () => { expect(navigation.openFullPane).not.toHaveBeenCalled(); }); - it('marks the card as selected immediately on click, before the pane opens', async () => { + it('does not mark a card as current merely because it was clicked', async () => { const workPackage = renderWorkPackage(); await nextFrame(); workPackage.dispatchEvent(new MouseEvent('click', { bubbles: true })); - expect(workPackage.hasAttribute('data-selected')).toBe(true); expect(workPackage.hasAttribute('aria-current')).toBe(false); expect(navigation.openSplitPane).not.toHaveBeenCalled(); }); @@ -169,46 +177,46 @@ describe('Backlogs work package controller', () => { } }); - it('marks the card as selected when Enter opens the split pane', async () => { + it('marks the card as current when the URL points at it', async () => { const workPackage = renderWorkPackage(); await nextFrame(); - keydown(workPackage, 'Enter'); + document.dispatchEvent(new CustomEvent('turbo:visit', { + detail: { url: '/projects/demo/backlogs/details/SP-42' }, + })); - expect(workPackage.hasAttribute('data-selected')).toBe(true); - expect(navigation.openSplitPane).toHaveBeenCalledTimes(1); + expect(workPackage.getAttribute('aria-current')).toBe('true'); }); - it('clears the selection of other work packages when a card is selected', async () => { + it('unmarks the card as current once the URL moves elsewhere', async () => { const workPackage = renderWorkPackage(); - const other = document.createElement('article'); - other.setAttribute('data-controller', 'backlogs--work-package'); - fixture.appendChild(other); await nextFrame(); - other.setAttribute('data-selected', ''); - workPackage.dispatchEvent(new MouseEvent('click', { bubbles: true })); + document.dispatchEvent(new CustomEvent('turbo:visit', { + detail: { url: '/projects/demo/backlogs/details/SP-42' }, + })); + document.dispatchEvent(new CustomEvent('turbo:visit', { + detail: { url: '/projects/demo/backlogs' }, + })); - expect(other.hasAttribute('data-selected')).toBe(false); - expect(workPackage.hasAttribute('data-selected')).toBe(true); + expect(workPackage.hasAttribute('aria-current')).toBe(false); }); - it('syncs selection and aria-current from the visited URL', async () => { + it('leaves batch membership alone when the URL changes', async () => { const workPackage = renderWorkPackage(); + const row = workPackage.parentElement!; await nextFrame(); + row.setAttribute('data-batch-selected', ''); + document.dispatchEvent(new CustomEvent('turbo:visit', { detail: { url: '/projects/demo/backlogs/details/SP-42' }, })); - - expect(workPackage.getAttribute('aria-current')).toBe('true'); - expect(workPackage.hasAttribute('data-selected')).toBe(true); + expect(row.hasAttribute('data-batch-selected')).toBe(true); document.dispatchEvent(new CustomEvent('turbo:visit', { detail: { url: '/projects/demo/backlogs' }, })); - - expect(workPackage.hasAttribute('aria-current')).toBe(false); - expect(workPackage.hasAttribute('data-selected')).toBe(false); + expect(row.hasAttribute('data-batch-selected')).toBe(true); }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts index 27f037fdc55e..8e1767399e34 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts @@ -79,34 +79,15 @@ export default class WorkPackageController extends Controller imple // Bookmarks and external links may still carry a numeric ID after the // switch to semantic mode, so accept either form here. if (id !== undefined && (id === this.idValue.toString() || id === this.displayIdValue)) { - this.markAsSelected(); this.markAsCurrent(); } else { - this.unmarkAsSelected(); this.unmarkAsCurrent(); } } - // Selection is applied synchronously on activation, before the split screen - // has loaded, so the list reacts immediately. With single selection any - // previously selected work package is cleared right away instead of waiting - // for its own URL sync. - markAsSelected():void { - document - .querySelectorAll('[data-controller~="backlogs--work-package"][data-selected]') - .forEach((other) => { - if (other !== this.element) { - other.removeAttribute('data-selected'); - } - }); - - this.element.setAttribute('data-selected', ''); - } - - unmarkAsSelected():void { - this.element.removeAttribute('data-selected'); - } - + // Not set optimistically: activation waits out the double-click delay below + // and may resolve to the full view instead, so asserting a current work + // package here would announce a navigation that may never happen. markAsCurrent():void { this.element.setAttribute('aria-current', 'true'); } @@ -137,8 +118,6 @@ export default class WorkPackageController extends Controller imple if (this.clickTimeout !== null) return; - this.markAsSelected(); - this.clickTimeout = window.setTimeout(() => { this.clickTimeout = null; this.openSplitPane(); @@ -172,7 +151,6 @@ export default class WorkPackageController extends Controller imple if (event.shiftKey) { this.openFullPane(); } else { - this.markAsSelected(); this.openSplitPane(); } } diff --git a/modules/backlogs/app/components/backlogs/concerns/work_package_movability.rb b/modules/backlogs/app/components/backlogs/concerns/work_package_movability.rb index d9b4cf9cf6f2..ca018d86e662 100644 --- a/modules/backlogs/app/components/backlogs/concerns/work_package_movability.rb +++ b/modules/backlogs/app/components/backlogs/concerns/work_package_movability.rb @@ -75,5 +75,18 @@ def sortable? def movable? sortable? && !work_package.readonly_status? end + + # What ordering this card takes part in. + # + # `fixed` is no part at all: no drag, no positional move, no selection. + # `confined` reorders within its own list but is refused by every other + # container. `free` may move to any list that accepts it. + # + # @return [String] one of "fixed", "confined", "free". + def mobility + return "fixed" unless sortable? + + movable? ? "free" : "confined" + end end end diff --git a/modules/backlogs/app/components/backlogs/selection_description_component.html.erb b/modules/backlogs/app/components/backlogs/selection_description_component.html.erb new file mode 100644 index 000000000000..ed8eef46f316 --- /dev/null +++ b/modules/backlogs/app/components/backlogs/selection_description_component.html.erb @@ -0,0 +1,34 @@ +<%# -- 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. + +++%> + +<%# Permanently hidden: `aria-describedby` reaches it regardless, and the + planning columns keep their geometry. %> +<%= render(Primer::Box.new(tag: :span, id: DESCRIPTION_ID, hidden: true)) do %> + <%= I18n.t("js.backlogs.selection.card_state") %> +<% end %> diff --git a/modules/backlogs/app/components/backlogs/selection_description_component.rb b/modules/backlogs/app/components/backlogs/selection_description_component.rb new file mode 100644 index 000000000000..f49243af0aa2 --- /dev/null +++ b/modules/backlogs/app/components/backlogs/selection_description_component.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Backlogs + # The single description every selected card points at through + # `aria-describedby`, so batch membership reaches assistive technology + # without duplicating the string onto every card. + class SelectionDescriptionComponent < ApplicationComponent + DESCRIPTION_ID = "op-backlogs-batch-selected-description" + end +end 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 d68b9d3f2b9f..02e53d4c5e3f 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 @@ -44,7 +44,7 @@ def build_card end # Every sortable card drags: a read-only one stays a drag source too, only - # confined to its own list (see {#confined?}). + # confined to its own list (see {#mobility}). def draggable? sortable? end @@ -75,7 +75,7 @@ def card_arguments end def card_data - data = { + { story: true, # Non-movable cards opt in too: they have no move actions, but their # singular menu is still worth reaching contextually. @@ -83,12 +83,10 @@ def card_data 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, - backlogs__work_package_full_url_value: full_url + backlogs__work_package_full_url_value: full_url, + # The card, not the row, carries the tab stop. + sortable_lists__item_target: "preview handle focus" } - - return data unless draggable? - - data.merge(sortable_lists__item_target: "preview handle") end # @return [Hash] ARIA wiring announcing the card's Enter activation and its @@ -103,20 +101,15 @@ def card_aria } end - # An unmovable card registers as a sortable item like any other: it keeps - # its drag and its positional moves, stays a drop target and keeps counting - # as a row of its list. The confined value is what pins it to that list. + # Every card row is a sortable item, movable or not: a non-movable row is + # still an addressable position its neighbours anchor drops on. def row_data - sortable? ? sortable_item_data : {} - end - - def sortable_item_data { controller: "sortable-lists--item", sortable_lists__item_id_value: work_package.id, sortable_lists__item_label_value: work_package.to_fs(:caption), sortable_lists__item_type_value: "work_package", - sortable_lists__item_confined_value: confined?, + sortable_lists__item_mobility_value: mobility, # Native drag payload for external consumers; the same absolute URL # as the card menu's "Copy URL to clipboard" item. The label above # doubles as the link text of the text/html flavour. @@ -124,12 +117,6 @@ def sortable_item_data } end - # Whether the card's drag is pinned to its own list: it may reorder in - # place, but no other container accepts it. - def confined? - sortable? && !movable? - end - public def row_args diff --git a/modules/backlogs/app/helpers/backlogs/common_helper.rb b/modules/backlogs/app/helpers/backlogs/common_helper.rb index dd5766caf289..1ed6d9dd22aa 100644 --- a/modules/backlogs/app/helpers/backlogs/common_helper.rb +++ b/modules/backlogs/app/helpers/backlogs/common_helper.rb @@ -49,6 +49,16 @@ def sprint_management_allowed? user_allowed?(:share_sprint) end + # Batch selection is the same capability as ordering: without the right to + # reorder, enabling it would only cost the viewer their Space, arrow, + # Home/End and Ctrl/Cmd+A keys. + # + # Takes the project because the backlog view, unlike the components the + # predicates above serve, exposes no `project` method. + def batch_selection_allowed?(project) + user_allowed?(:manage_sprint_items, project:) + end + def backlog_filters RequestStore.fetch(:backlog_filters) do Backlogs::BacklogFilters.from_params(permitted_params.backlog_filters) diff --git a/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb b/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb index 43fde6457009..bad4c3f83314 100644 --- a/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb @@ -29,6 +29,10 @@ See COPYRIGHT and LICENSE files for more details. <%# Attributes on this frame tag are ignored; only its children are reconciled — set them in show.html.erb. %> <%= turbo_frame_tag "backlogs_container" do %> + <%# Rendered once for the whole page: every selected card in either column + points its `aria-describedby` at this one element. %> + <%= render(Backlogs::SelectionDescriptionComponent.new) %> +
      <%= diff --git a/modules/backlogs/app/views/backlogs/backlog/show.html.erb b/modules/backlogs/app/views/backlogs/backlog/show.html.erb index 555d771c275d..961b551c20d6 100644 --- a/modules/backlogs/app/views/backlogs/backlog/show.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/show.html.erb @@ -53,6 +53,10 @@ See COPYRIGHT and LICENSE files for more details. sortable_lists_optimistic_value: true, action: "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->backlogs--split-view-sync#onWorkPackageMoved", sortable_lists_move_url_template_value: backlogs_move_url_template(@project), + sortable_lists_selection_enabled_value: batch_selection_allowed?(@project), + sortable_lists_announcement_scope_value: "js.backlogs.selection", + sortable_lists_selection_description_id_value: + Backlogs::SelectionDescriptionComponent::DESCRIPTION_ID, sortable_lists_sortable_lists__list_outlet: "#backlogs_container [data-controller~='sortable-lists--list']", sortable_lists_sortable_lists__item_outlet: "#backlogs_container [data-controller~='sortable-lists--item']", sortable_lists_sortable_lists__scrollable_outlet: "#backlogs_container [data-controller~='sortable-lists--scrollable']" diff --git a/modules/backlogs/config/locales/js-en.yml b/modules/backlogs/config/locales/js-en.yml index 2b16dfa89bf4..a07d340d8d2d 100644 --- a/modules/backlogs/config/locales/js-en.yml +++ b/modules/backlogs/config/locales/js-en.yml @@ -29,6 +29,19 @@ --- en: js: + backlogs: + selection: + card_state: "Selected" + cleared: "Selection cleared." + not_selectable: "Selection unchanged. This work package takes no part in this list's ordering." + range_blocked: "Selection unchanged. That range contains a work package that takes no part in this list's ordering." + range_restarted: + one: "Could not extend the range. 1 work package selected." + other: "Could not extend the range. %{count} work packages selected." + range_unavailable: "Selection unchanged. Expand this list to select that range." + selected: + one: "1 work package selected." + other: "%{count} work packages selected." burndown: day: "Day" points: "Points" diff --git a/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb b/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb index e55610b570c1..19fbdf378900 100644 --- a/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb @@ -164,7 +164,7 @@ def render_component expect(rendered_component).to have_css(".op-work-package-card") do |card| expect(card["data-controller"].split).to include("backlogs--work-package") - expect(card["data-sortable-lists--item-target"]).to eq("preview handle") + expect(card["data-sortable-lists--item-target"]).to eq("preview handle focus") expect(card["data-backlogs--work-package-split-url-value"]) .to end_with(project_backlogs_backlog_details_path(project, work_package)) expect(card["data-backlogs--work-package-full-url-value"]) @@ -241,13 +241,13 @@ def render_component position: 1) end - it "does not mark work package rows as draggable" do - expect(rendered_component).to have_css(".Box-row#work_package_#{work_package.id}") - expect(rendered_component) - .to have_no_css(".Box-row#work_package_#{work_package.id}[data-sortable-lists--item-id-value]") + it "still renders the row as a sortable item, but fixed and not draggable" do + expect(rendered_component).to have_css(".Box-row#work_package_#{work_package.id}") do |row| + expect(row["data-controller"]).to eq("sortable-lists--item") + expect(row["data-sortable-lists--item-mobility-value"]).to eq("fixed") + end expect(rendered_component).to have_no_css(".Box-row#work_package_#{work_package.id}[draggable='true']") expect(rendered_component).to have_no_css(".op-work-package-card[data-sortable-lists--item-id-value]") - expect(rendered_component).to have_no_css(".op-work-package-card[data-sortable-lists--item-target]") expect(rendered_component).to have_no_css(".op-work-package-card[draggable='true']") expect(rendered_component).to have_no_css(".op-work-package-card.Box-card--draggable") end diff --git a/modules/backlogs/spec/components/backlogs/selection_description_component_spec.rb b/modules/backlogs/spec/components/backlogs/selection_description_component_spec.rb new file mode 100644 index 000000000000..0db999264b17 --- /dev/null +++ b/modules/backlogs/spec/components/backlogs/selection_description_component_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "rails_helper" + +RSpec.describe Backlogs::SelectionDescriptionComponent, type: :component do + subject(:rendered_component) { render_inline(described_class.new) && page } + + # `visible: :all` because the element is permanently `hidden`, which is the + # one form of invisibility Capybara::Node::Simple does honour. + + it "renders the description every selected card points at" do + expect(rendered_component) + .to have_css("##{described_class::DESCRIPTION_ID}", text: I18n.t("js.backlogs.selection.card_state"), + visible: :all) + end + + # `hidden` keeps this out of the planning columns' layout: reserving space + # above two independently scrolling columns moved the cards under the user's + # cursor mid-drag. + it "keeps the description hidden, since only aria-describedby reaches it" do + expect(rendered_component).to have_css("##{described_class::DESCRIPTION_ID}[hidden]", visible: :all) + end +end diff --git a/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb b/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb index c36cc73b6a82..32477700145a 100644 --- a/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb @@ -131,7 +131,7 @@ def menu_items expect(rendered_component).to have_css(".Box-row#work_package_#{work_package1.id} .op-work-package-card") do |card| expect(card["data-controller"].split).to include("backlogs--work-package") - expect(card["data-sortable-lists--item-target"]).to eq("preview handle") + expect(card["data-sortable-lists--item-target"]).to eq("preview handle focus") expect(card["data-backlogs--work-package-display-id-value"]).to eq(work_package1.display_id.to_s) end end @@ -159,13 +159,13 @@ def menu_items priority: default_priority, story_points: 5, position: 1, sprint:) end - it "does not mark work package rows as draggable" do - expect(rendered_component).to have_css(".Box-row#work_package_#{work_package1.id}") - expect(rendered_component) - .to have_no_css(".Box-row#work_package_#{work_package1.id}[data-sortable-lists--item-id-value]") + it "still renders the row as a sortable item, but fixed and not draggable" do + expect(rendered_component).to have_css(".Box-row#work_package_#{work_package1.id}") do |row| + expect(row["data-controller"]).to eq("sortable-lists--item") + expect(row["data-sortable-lists--item-mobility-value"]).to eq("fixed") + end expect(rendered_component).to have_no_css(".Box-row#work_package_#{work_package1.id}[draggable='true']") expect(rendered_component).to have_no_css(".op-work-package-card[data-sortable-lists--item-id-value]") - expect(rendered_component).to have_no_css(".op-work-package-card[data-sortable-lists--item-target]") expect(rendered_component).to have_no_css(".op-work-package-card[draggable='true']") expect(rendered_component).to have_no_css(".op-work-package-card.Box-card--draggable") 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 a57a7e8b1955..0aaf1ae8129a 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 @@ -121,7 +121,7 @@ def render_component(work_packages:, container:, drag_and_drop:) expect(rendered_component).to have_css(".Box-row#work_package_#{work_package.id} .op-work-package-card") do |card| expect(card["data-story"]).to be_present expect(card["data-backlogs--work-package-id-value"]).to eq(work_package.id.to_s) - expect(card["data-sortable-lists--item-target"]).to eq("preview handle") + expect(card["data-sortable-lists--item-target"]).to eq("preview handle focus") end 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 0de978d3f746..6c97cf845e0f 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 @@ -81,8 +81,8 @@ controller: "sortable-lists--item", sortable_lists__item_id_value: work_package.id, sortable_lists__item_type_value: "work_package", - sortable_lists__item_confined_value: false, - sortable_lists__item_label_value: work_package.to_fs(:caption) + sortable_lists__item_label_value: work_package.to_fs(:caption), + sortable_lists__item_mobility_value: "free" ) expect(item.row_args[:draggable]).to be(true) expect(item.row_args).not_to include(:tabindex) @@ -122,11 +122,22 @@ described_class.new(work_package:, project:, container:, params:, current_user: limited_user) end + it "still renders the row as a sortable item" do + expect(item.row_args[:data]).to include(controller: "sortable-lists--item") + end + + it "marks the item as taking no part in ordering" do + expect(item.row_args[:data]).to include(sortable_lists__item_mobility_value: "fixed") + end + it "does not mark the row as draggable" do expect(item.row_args[:data]).not_to include(:sortable_lists_prev_item_id) expect(item.row_args).not_to include(:draggable) expect(item.row_args).not_to include(:tabindex) - expect(item.row_args[:data]).not_to include(:sortable_lists__item_external_url_value) + end + + it "still exposes the external URL for drag consumers" do + expect(item.row_args[:data]).to include(:sortable_lists__item_external_url_value) end it "keeps the card focusable so keyboard users can interact with it" do @@ -160,7 +171,7 @@ expect(item.row_args[:data]).to include( controller: "sortable-lists--item", sortable_lists__item_id_value: work_package.id, - sortable_lists__item_confined_value: true + sortable_lists__item_mobility_value: "confined" ) end @@ -171,7 +182,7 @@ it "still offers the card as a drag handle", :aggregate_failures do render_inline(item.card) - expect(page).to have_css(".op-work-package-card[data-sortable-lists--item-target='preview handle']") + expect(page).to have_css(".op-work-package-card[data-sortable-lists--item-target='preview handle focus']") expect(page).to have_css(".Box-card--draggable") end end @@ -183,7 +194,7 @@ it "does not confine the row" do expect(item.row_args[:draggable]).to be(true) - expect(item.row_args[:data]).to include(sortable_lists__item_confined_value: false) + expect(item.row_args[:data]).to include(sortable_lists__item_mobility_value: "free") end end end @@ -236,7 +247,7 @@ "[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)}']" \ - "[data-sortable-lists--item-target='preview handle']" \ + "[data-sortable-lists--item-target='preview handle focus']" \ "[tabindex='0']" ) end diff --git a/modules/backlogs/spec/helpers/backlogs/common_helper_spec.rb b/modules/backlogs/spec/helpers/backlogs/common_helper_spec.rb index 7140946c62f4..8edbdcdb9ed0 100644 --- a/modules/backlogs/spec/helpers/backlogs/common_helper_spec.rb +++ b/modules/backlogs/spec/helpers/backlogs/common_helper_spec.rb @@ -62,6 +62,39 @@ end end + describe "#batch_selection_allowed?" do + let(:project) { build_stubbed(:project) } + + before do + # The helper resolves the actor through `current_user`, which a bare + # helper object does not define; the describe above stubs it the same + # way. + without_partial_double_verification do + allow(helper).to receive_messages(current_user:) + end + + mock_permissions_for(current_user) do |mock| + mock.allow_in_project(*permissions, project:) + end + end + + context "when the user may manage sprint items" do + let(:permissions) { %i[manage_sprint_items] } + + it "offers batch selection" do + expect(helper.batch_selection_allowed?(project)).to be true + end + end + + context "when the user may only view" do + let(:permissions) { %i[view_sprints view_work_packages] } + + it "does not offer batch selection" do + expect(helper.batch_selection_allowed?(project)).to be false + end + end + end + describe "#backlog_filters" do context "with no params" do let(:params) { {} } diff --git a/modules/backlogs/spec/requests/backlogs/backlog_spec.rb b/modules/backlogs/spec/requests/backlogs/backlog_spec.rb index 479958b5c08e..3618bd98ca13 100644 --- a/modules/backlogs/spec/requests/backlogs/backlog_spec.rb +++ b/modules/backlogs/spec/requests/backlogs/backlog_spec.rb @@ -156,6 +156,43 @@ expect(response.body).to include(%(data-sortable-lists--list-id-value="#{backlog_bucket.id}")) end end + + context "when a bucket filter hides the inbox" do + shared_let(:backlog_bucket) { create(:backlog_bucket, project:) } + + it "still renders the shared selection description" do + get "/projects/#{project.identifier}/backlogs/backlog", + params: { bucket_ids: [backlog_bucket.id] }, + headers: { "Turbo-Frame" => "backlogs_container" } + + expect(response).to have_http_status(:ok) + # The inbox itself is filtered out... + expect(response.body).not_to include(%(id="inbox_project_#{project.id}")) + # ...but the shared description survives for the lists that remain. + expect(response.body).to include(%(id="#{Backlogs::SelectionDescriptionComponent::DESCRIPTION_ID}")) + end + end + + # With every card fixed, opting in would only cost the viewer their + # Space, arrow, Home/End and Ctrl/Cmd+A keys. + context "when the user cannot manage sprint items" do + shared_let(:viewer_role) do + create(:project_role, permissions: %i[view_sprints view_work_packages]) + end + shared_let(:viewer) { create(:user, member_with_roles: { project => viewer_role }) } + + current_user { viewer } + + # A full page load, not a frame request: the sortable root and its + # selection values live on the wrapper in show.html.erb, which a + # Turbo-Frame request never renders. + it "does not enable batch selection" do + get "/projects/#{project.identifier}/backlogs/backlog" + + expect(response).to have_http_status(:ok) + expect(response.body).to include('data-sortable-lists-selection-enabled-value="false"') + end + end end end diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index efe9b88d2a61..3982a241249b 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -342,6 +342,31 @@ def work_package_card(work_package) find(work_package_card_selector(work_package)) end + # The row wrapper, not the card: it carries the item id and, once + # selected, `data-batch-selected`. The description lives on the card, + # which is the focus host, so it is checked through {#work_package_card}. + def work_package_row(work_package) + find(work_package_selector(work_package)) + end + + # The row's resolved background colour. Read it twice around one isolated + # change, never across two rows and never around a gesture that also + # focuses or navigates: the background depends on focus and `aria-current` + # too, so anything else makes the difference unattributable. + def row_background_color(work_package) + work_package_row(work_package).style("background-color").fetch("background-color") + end + + # Moves focus onto the card without a pointer and without any event a + # Backlogs controller listens for. Establishes focus before a + # background-colour control read, so a later keyboard gesture on the same + # card does not change it a second time. + def focus_work_package_card(work_package) + work_package_card(work_package).execute_script( + "this.focus({ focusVisible: true, preventScroll: true })" + ) + 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. @@ -385,6 +410,20 @@ def expect_work_package_card_focused(work_package) .to have_css(work_package_card_selector(work_package), focused: true) end + # The *computed* accessible description, not the raw `aria-describedby` + # token: an id that resolves to nothing, or an element the browser + # declines to traverse into, leaves this empty while the attribute reads + # exactly as intended. + def expect_work_package_card_described_as(work_package, description) + expect(page) + .to have_css(work_package_card_selector(work_package), accessible_description: description) + end + + def expect_work_package_card_not_described(work_package) + expect(page) + .to have_css(work_package_card_selector(work_package), accessible_description: "") + 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 @@ -536,12 +575,26 @@ def expect_no_sprint_menu_item(sprint, item_name) end end + # Opening details morphs the row into its "current work package" state, so + # a reference captured beforehand can go stale mid-morph. `retry_block` + # spaces the attempts out and finds the button, menu and view fresh on + # each one. def open_work_package_details(work_package) - within_work_package(work_package) do - button = find(:button, accessible_name: "Work package actions") - open_controlled_menu(button).find(:menuitem, text: I18n.t(:"js.button_open_details")).click + retry_block( + args: { + tries: 3, + on: [ + Capybara::Cuprite::ObsoleteNode, + Selenium::WebDriver::Error::StaleElementReferenceError + ] + } + ) do + within_work_package(work_package) do + button = find(:button, accessible_name: "Work package actions") + open_controlled_menu(button).find(:menuitem, text: I18n.t(:"js.button_open_details")).click + end + expect_details_view(work_package) end - expect_details_view(work_package) end def expect_details_view(work_package) @@ -606,9 +659,24 @@ def expect_and_confirm_backlog_bucket_delete_modal end end + # Every row carries the item id whether or not the user may move it, so + # the id no longer distinguishes an orderable row. `draggable` and + # `mobility` are set independently and read by separate consumers, so + # both are checked. + def expect_work_package_draggable(work_package) + selector = work_package_selector(work_package) + expect(page).to have_css("#{selector}[draggable='true']") + expect(page).to have_no_css("#{selector}[data-sortable-lists--item-mobility-value='fixed']") + end + + # Both assertions are negative, so they also pass against a page that + # never rendered the card — including the rack-session page a racing + # Selenium login can strand the browser on. Asserting the row exists first + # is blocked on fix/selenium-rack-session-login-flake. def expect_work_package_not_draggable(work_package) - expect(page) - .to have_no_css(draggable_work_package_selector(work_package)) + selector = work_package_selector(work_package) + expect(page).to have_no_css("#{selector}[draggable='true']") + expect(page).to have_no_css("#{selector}[data-sortable-lists--item-mobility-value='free']") end # A read-only card keeps its drag but is confined to its own list: it can @@ -616,8 +684,8 @@ def expect_work_package_not_draggable(work_package) # confined value is what the foreign drop targets read. def expect_work_package_confined(work_package) expect(page) - .to have_css("#{draggable_work_package_selector(work_package)}" \ - "[data-sortable-lists--item-confined-value='true']") + .to have_css("#{work_package_selector(work_package)}" \ + "[data-sortable-lists--item-mobility-value='confined']") expect(page) .to have_css("#{work_package_selector(work_package)}[draggable]") end @@ -637,6 +705,47 @@ def expect_work_package_not_locked(work_package) end end + # An unmodified click: narrows the batch to this card and opens its + # details pane. Offset near the top-left corner because the card's centre + # sits on the subject link or the actions menu button, both of which the + # selection root ignores as interactive descendants. + def select_card(work_package) + work_package_card(work_package).click(x: 6, y: 6, offset: :position) + end + + # Toggles membership without navigating, and re-bases the anchor to this + # card even when the toggle deselects it. + def toggle_card(work_package) + modified_click(work_package, :meta) + end + + # Selects the contiguous range from the anchor to this card. Repeated + # calls resize one range rather than walking it. + def extend_selection_to(work_package) + modified_click(work_package, :shift) + end + + # Asserted directly because an empty batch proves nothing: a root that + # opted in and refused every gesture looks identical from outside, yet + # differs in whether the browser still gets the keystroke. + def expect_batch_selection_disabled + expect(page).to have_css("[data-controller~='sortable-lists'][data-sortable-lists-selection-enabled-value='false']", + visible: :all) + end + + # Live batch membership, in document order. + def selected_card_ids + all("[data-batch-selected]").pluck("data-sortable-lists--item-id-value") + end + + # The shared description every selected card's `aria-describedby` points + # at. Rendered once, permanently `hidden` — screen readers still reach it + # through the reference despite that — so `visible: :all` is required. + def expect_selection_description_present + expect(page).to have_css("##{Backlogs::SelectionDescriptionComponent::DESCRIPTION_ID}", + visible: :all, count: 1) + end + def pick_up_and_release_work_package(work_package) # A mid-drag list refresh can detach the grabbed row, so retry a bounded # number of times on a stale node. retry_block no-ops under @@ -652,7 +761,7 @@ def pick_up_and_release_work_package(work_package) ] } ) do - moved_element = find(draggable_work_package_selector(work_package)) + moved_element = find(work_package_selector(work_package)) install_backlogs_move_request_probe begin pick_up_and_release_backlogs_item(moved_element) @@ -756,7 +865,7 @@ def drag_work_package(moved, before: nil, after: nil, into: nil) raise ArgumentError, "You must specify exactly one of before, after or into" end - moved_element = find(draggable_work_package_selector(moved)) + moved_element = find(work_package_selector(moved)) target_element, edge = if before [find(work_package_selector(before)), :top] @@ -788,7 +897,7 @@ def drag_work_package_without_move(moved, into:) ] } ) do - moved_element = find(draggable_work_package_selector(moved)) + moved_element = find(work_package_selector(moved)) target_element = find(list_body_selector(sprint_selector(into))) install_backlogs_move_request_probe begin @@ -838,7 +947,7 @@ def expect_backlogs_drag_refused end def drag_work_package_to_backlog_inbox(work_package) - moved_element = find(draggable_work_package_selector(work_package)) + moved_element = find(work_package_selector(work_package)) inbox = find(backlog_inbox_selector) target_item = inbox.all("[data-sortable-lists--item-id-value]", minimum: 0).last target_element = target_item || inbox.find("[data-empty-list-item]") @@ -853,7 +962,7 @@ def drag_work_package_to_backlog_inbox(work_package) end def drag_work_package_to_backlog_bucket(work_package, bucket) - moved_element = find(draggable_work_package_selector(work_package)) + moved_element = find(work_package_selector(work_package)) target_element = find(list_body_selector(bucket_selector(bucket))) wait_for_backlogs_turbo_stream(frame_reload: true) do @@ -865,7 +974,7 @@ def drag_work_package_to_backlog_bucket(work_package, bucket) end def drag_work_package_to_sprint(work_package, sprint) - moved_element = find(draggable_work_package_selector(work_package)) + moved_element = find(work_package_selector(work_package)) target_element = find(list_body_selector(sprint_selector(sprint))) wait_for_backlogs_turbo_stream(frame_reload: true) do drag_backlogs_item(source: moved_element, target: target_element) @@ -946,6 +1055,15 @@ def within_work_package(work_package, &) private + # Node::Element#click takes the held key and positional options, so no + # action chain is needed. The offset avoids the card's centre, where the + # subject link or actions menu button sits: the selection root ignores a + # gesture starting on either, and which one lands dead centre varies with + # the card's content. + def modified_click(work_package, key) + work_package_card(work_package).click(key, x: 6, y: 6, offset: :position) + end + def within_sprint(sprint, &) within(sprint_selector(sprint), &) end @@ -1020,10 +1138,6 @@ 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 - # Located by the lock's accessible name so the expectation fails if the # icon ever loses the text that explains it. def readonly_lock_selector From d12f3e55274c6c78783f203effd86b001bbe2a19 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 15 Aug 2026 15:15:34 +0100 Subject: [PATCH 06/22] [AGILE-361] Cover batch selection end to end Adds a Selenium feature spec for what units cannot prove: the capture-phase listeners really beat the card's own click and Enter handlers in a real page. https://community.openproject.org/wp/AGILE-361 --- .../work_packages/batch_selection_spec.rb | 274 ++++++++++++++++++ 1 file changed, 274 insertions(+) create mode 100644 modules/backlogs/spec/features/work_packages/batch_selection_spec.rb diff --git a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb new file mode 100644 index 000000000000..c48bebeb6720 --- /dev/null +++ b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb @@ -0,0 +1,274 @@ +# 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 modifier combinations are driven through native +# `send_keys`, and only a real page can prove the root's capture-phase +# listener beats the card's own click and Enter handlers. +RSpec.describe "Backlogs batch selection", :js, :selenium, :settings_reset 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!(:story1) { create(:work_package, sprint:, type:, project:) } + let!(:story2) { create(:work_package, sprint:, type:, project:) } + let!(:story3) { create(:work_package, sprint:, type:, project:) } + let!(:story4) { create(:work_package, sprint:, type:, project:) } + let!(:bucket) { create(:backlog_bucket, project:, name: "Backlog bucket") } + let!(:bucket_wp1) { create(:work_package, backlog_bucket: bucket, position: 1, type:, project:) } + let!(:bucket_wp2) { create(:work_package, backlog_bucket: bucket, position: 2, type:, project:) } + + 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 + + describe "mouse gestures" do + it "collapses a wider selection onto the clicked card and still opens its details" do + backlogs_page.select_card(story1) + backlogs_page.toggle_card(story2) + backlogs_page.select_card(story3) + + expect(page).to have_css("[data-batch-selected]", count: 1) + expect(backlogs_page.selected_card_ids).to eq([story3.id.to_s]) + backlogs_page.expect_details_view(story3) + end + + it "toggles a sparse selection across two lists without navigating" do + # Seeded with a toggle, not a plain click: a plain click schedules its + # details-pane visit on a timer, so the path assertion below would pass + # with a still-pending navigation either way. + backlogs_page.toggle_card(story1) + backlogs_page.toggle_card(bucket_wp1) + + expect(page).to have_css("[data-batch-selected]", count: 2) + expect(backlogs_page.selected_card_ids).to contain_exactly(story1.id.to_s, bucket_wp1.id.to_s) + # Had the modified click reached the card's own handler, the details + # pane would have opened and the path would have changed. + expect(page).to have_current_path(backlogs_page.path, ignore_query: true) + end + + # BatchSelection#toggle re-bases the anchor even on a deselect, so a + # later range starts from story2 rather than story1. + it "re-bases the anchor to a toggled card even when the toggle deselects it" do + backlogs_page.select_card(story1) + backlogs_page.toggle_card(story2) + backlogs_page.toggle_card(story2) + + expect(page).to have_css("[data-batch-selected]", count: 1) + + backlogs_page.extend_selection_to(story4) + + expect(page).to have_css("[data-batch-selected]", count: 3) + expect(backlogs_page.selected_card_ids).to eq([story2, story3, story4].map { it.id.to_s }) + end + + it "resizes one fixed-anchor range rather than walking it" do + backlogs_page.select_card(story1) + backlogs_page.extend_selection_to(story3) + + expect(page).to have_css("[data-batch-selected]", count: 3) + expect(backlogs_page.selected_card_ids).to eq([story1, story2, story3].map { it.id.to_s }) + + backlogs_page.extend_selection_to(story2) + + expect(page).to have_css("[data-batch-selected]", count: 2) + expect(backlogs_page.selected_card_ids).to eq([story1, story2].map { it.id.to_s }) + end + + # The follow-up range proves the cross-list gesture re-anchored rather + # than being refused outright. + it "refuses to extend a range across lists and starts a fresh selection there" do + backlogs_page.select_card(story1) + backlogs_page.extend_selection_to(bucket_wp2) + + expect(page).to have_css("[data-batch-selected]", count: 1) + expect(backlogs_page.selected_card_ids).to eq([bucket_wp2.id.to_s]) + + backlogs_page.extend_selection_to(bucket_wp1) + + expect(page).to have_css("[data-batch-selected]", count: 2) + expect(backlogs_page.selected_card_ids).to eq([bucket_wp1, bucket_wp2].map { it.id.to_s }) + end + end + + describe "keyboard interaction" do + it "paints a row with a different background once it is selected" do + # Focus is established first and held constant across both reads: a + # click would move focus, open the details pane and toggle membership at + # once, leaving the colour difference unattributable. + backlogs_page.focus_work_package_card(story1) + unselected_color = backlogs_page.row_background_color(story1) + + backlogs_page.work_package_card(story1).send_keys(:space) + + expect(page).to have_css("[data-batch-selected]", count: 1) + expect(backlogs_page.row_background_color(story1)).not_to eq(unselected_color) + end + + it "selects and clears with the keyboard" do + backlogs_page.work_package_card(story1).send_keys(:space) + + expect(page).to have_css("[data-batch-selected]", count: 1) + expect(backlogs_page.selected_card_ids).to eq([story1.id.to_s]) + + backlogs_page.work_package_card(story1).send_keys(:escape) + + expect(page).to have_no_css("[data-batch-selected]") + expect(backlogs_page.selected_card_ids).to be_empty + end + + # A click on empty column space leaves focus nowhere, and Escape there + # still means "drop the selection". + it "clears the selection with Escape when no card holds focus" do + backlogs_page.toggle_card(story1) + expect(page).to have_css("[data-batch-selected]", count: 1) + + page.execute_script("document.activeElement.blur()") + backlogs_page.send_keys_to_focused_element(:escape) + + expect(page).to have_no_css("[data-batch-selected]") + expect(backlogs_page.selected_card_ids).to be_empty + end + + # The fixtures span a sprint and a bucket, so a regression to root-wide + # select-all would take this past the sprint's four cards. + it "selects every orderable card of the focused card's list with Ctrl/Cmd+A" do + backlogs_page.send_work_package_card_keys(story1, [:control, "a"]) + + expect(page).to have_css("[data-batch-selected]", count: 4) + expect(backlogs_page.selected_card_ids).to contain_exactly( + story1.id.to_s, story2.id.to_s, story3.id.to_s, story4.id.to_s + ) + end + + it "extends the selection from a fixed anchor with Shift+ArrowDown" do + backlogs_page.work_package_card(story1).send_keys(:space) + expect(page).to have_css("[data-batch-selected]", count: 1) + + backlogs_page.send_work_package_card_keys(story1, %i[shift arrow_down]) + + expect(page).to have_css("[data-batch-selected]", count: 2) + expect(backlogs_page.selected_card_ids).to eq([story1, story2].map { it.id.to_s }) + end + + # Enter belongs to the card's own activation handler, which only a real + # page can prove. + it "opens details with Enter without the selection handler swallowing it" do + backlogs_page.send_work_package_card_keys(story2, [:enter]) + + backlogs_page.expect_details_view(story2) + end + end + + describe "selection persistence and accessibility" do + it "keeps the batch selected when the current work package changes" do + backlogs_page.select_card(story1) + # A plain click waits out a double-click window before it visits, so + # without this wait story1's pending visit lands while story3's lazily + # loaded actions menu is opening below, racing its fetch. + backlogs_page.expect_details_view(story1) + backlogs_page.toggle_card(story2) + + expect(page).to have_css("[data-batch-selected]", count: 2) + + backlogs_page.open_work_package_details(story3) + + expect(page).to have_css("[data-batch-selected]", count: 2) + expect(backlogs_page.selected_card_ids).to contain_exactly(story1.id.to_s, story2.id.to_s) + end + + it "describes a selected card to assistive technology via the shared description" do + backlogs_page.select_card(story1) + backlogs_page.toggle_card(story2) + + expect(page).to have_css("[data-batch-selected]", count: 2) + # Rendered exactly once, so the assertions below speak about one + # element rather than two that happen to agree. + backlogs_page.expect_selection_description_present + + # The card, not the row: a description is computed from the focused + # element's own `aria-describedby`, never inherited. The computed value + # also proves the browser traverses into the `hidden` element. + description = I18n.t("js.backlogs.selection.card_state") + backlogs_page.expect_work_package_card_described_as(story1, description) + backlogs_page.expect_work_package_card_described_as(story2, description) + # Without this, the two above would pass with every card described. + backlogs_page.expect_work_package_card_not_described(story3) + end + end + + # Selection consumes Space, the arrows, Home/End and Ctrl/Cmd+A. With every + # card fixed those keys would stop scrolling the page for a capability it + # never offers, so the root does not opt in at all. + describe "without the permission to manage sprint items" do + let(:view_role) do + create(:project_role, permissions: %i(view_sprints view_work_packages)) + end + + current_user do + create(:user, member_with_roles: { project => view_role }) + end + + # `current_user` declared here registers its login after the outer + # `before` has already visited, so the page is visited again. + before do + backlogs_page.visit! + end + + it "does not enable selection, and leaves its gestures to the browser" do + backlogs_page.expect_batch_selection_disabled + + backlogs_page.toggle_card(story1) + backlogs_page.send_work_package_card_keys(story2, [:space]) + + expect(page).to have_no_css("[data-batch-selected]") + end + end +end From 3d5cd62317c40ab40123cb4d434a6d6bbe50c238 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 15 Aug 2026 15:15:34 +0100 Subject: [PATCH 07/22] [AGILE-361] Document the selection capability --- frontend/AGENTS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 6fccc2fec73c..e40327563ee6 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -7,6 +7,8 @@ - `./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 +- `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list; selecting across lists is a deliberate gap, reserved for a separate mechanism. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Batch movement is not implemented: a drag still moves one card and collapses any wider selection onto it — that's a later work package. +- `data-batch-selected` is written on the sortable item element — the row, in Backlogs — while `aria-current` is written on the card inside it. A stylesheet assuming both live on the same element will silently paint nothing while attribute assertions stay green. ## Configuration Files From db7e0037ec951b053335c2e848aa0f92d11e0afd Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:14:03 -0800 Subject: [PATCH 08/22] Bind multi-select to the platform's own modifier Restores the change lost in the 2026-08-18 squash: Cmd on Apple platforms, Ctrl elsewhere, never the other. Meta on Windows or Linux classifies as an ordinary click again. --- .../dynamic/sortable-lists.controller.spec.ts | 63 ++++++++++------ .../selection-orchestrator.spec.ts | 75 ++++++++++++++++--- .../sortable-lists/selection-orchestrator.ts | 11 ++- .../work_packages/batch_selection_spec.rb | 2 +- .../backlogs/spec/support/pages/backlog.rb | 7 +- spec/support/browsers/browser_platform.rb | 29 +++++++ .../browsers/browser_platform_spec.rb | 62 +++++++++++++++ 7 files changed, 211 insertions(+), 38 deletions(-) create mode 100644 spec/support/browsers/browser_platform.rb create mode 100644 spec/support_spec/browsers/browser_platform_spec.rb diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 17ed7c05e0d0..73aea419d6ba 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -81,6 +81,7 @@ describe('Sortable lists controller', () => { // cannot resolve `.mock.calls`'s element type from; pin the spied method's // own signature instead so calls stay typed. let announceSpy:MockInstance; + let userAgentDataDescriptor:PropertyDescriptor|undefined; beforeAll(async () => { ({ monitorForElements } = await import('@atlaskit/pragmatic-drag-and-drop/element/adapter')); @@ -340,6 +341,15 @@ describe('Sortable lists controller', () => { beforeEach(async () => { vi.clearAllMocks(); + // Multi-select gestures below use Ctrl, which is only the multi-select + // modifier off Apple platforms — pin the platform so the suite behaves + // the same on a macOS workstation and on Linux CI. + userAgentDataDescriptor = Object.getOwnPropertyDescriptor(navigator, 'userAgentData'); + Object.defineProperty(navigator, 'userAgentData', { + value: { platform: 'Windows' }, + configurable: true, + }); + // The synthetic drop input below carries fixed coordinates that bear no // relation to where the fixture's rows actually lay out, so a real // hit-test would report the source's own row for every drop and the @@ -409,6 +419,11 @@ describe('Sortable lists controller', () => { document.body.querySelector('live-region')?.remove(); vi.restoreAllMocks(); vi.unstubAllGlobals(); + if (userAgentDataDescriptor) { + Object.defineProperty(navigator, 'userAgentData', userAgentDataDescriptor); + } else { + delete (navigator as { userAgentData?:unknown }).userAgentData; + } }); it('moves a list-only drop onto the source list to its configured position', async () => { @@ -1308,8 +1323,8 @@ describe('Sortable lists controller', () => { await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; items[0].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); - items[1].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true })); - items[2].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true })); + items[1].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true })); + items[2].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true })); announceSpy.mockClear(); controller.collapseSelectionForDrag(items[0]); @@ -1399,7 +1414,7 @@ describe('Sortable lists controller', () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); click(items[0]); - click(items[1], { metaKey: true }); + click(items[1], { ctrlKey: true }); items[2].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); const event = new MouseEvent('click', { bubbles: true, cancelable: true }); @@ -1413,8 +1428,8 @@ describe('Sortable lists controller', () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); click(items[0]); - click(items[1], { metaKey: true }); - click(items[2], { metaKey: true }); + click(items[1], { ctrlKey: true }); + click(items[2], { ctrlKey: true }); announceSpy.mockClear(); click(items[0]); @@ -1444,7 +1459,7 @@ describe('Sortable lists controller', () => { const { root, items } = renderSelectableRoot(); await ctx.nextFrame(); root.setAttribute('data-sortable-lists-busy', 'true'); - const event = new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true }); + const event = new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true }); items[0].dispatchEvent(event); @@ -1452,10 +1467,10 @@ describe('Sortable lists controller', () => { expect(items.some(isSelected)).toBe(false); }); - it('toggles a card without navigating on a meta click', async () => { + it('toggles a card without navigating on a modified click', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); - const event = new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true }); + const event = new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true }); click(items[0]); items[1].dispatchEvent(event); @@ -1517,7 +1532,7 @@ describe('Sortable lists controller', () => { const { root, items } = renderSelectableRoot(); await ctx.nextFrame(); click(items[0]); - click(items[1], { metaKey: true }); + click(items[1], { ctrlKey: true }); items[1].remove(); morphRoot(root); await ctx.nextFrame(); @@ -1570,11 +1585,11 @@ describe('Sortable lists controller', () => { ]); }); - it('preserves the batch and announces when a non-movable card is meta clicked', async () => { + it('preserves the batch and announces when a non-movable card is modifier-clicked', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); items[1].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); - const event = new MouseEvent('click', { bubbles: true, cancelable: true, metaKey: true }); + const event = new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true }); click(items[0]); announceSpy.mockClear(); @@ -1594,7 +1609,7 @@ describe('Sortable lists controller', () => { link.href = '/somewhere'; items[0].appendChild(link); - clickWithoutNavigating(link, { metaKey: true }); + clickWithoutNavigating(link, { ctrlKey: true }); expect(items.some(isSelected)).toBe(false); }); @@ -1827,13 +1842,13 @@ describe('Sortable lists controller', () => { // The fixture spans two lists: items[0..2] in the source, items[3..4] in // the target. - it('selects every movable card in the focused list on meta A', async () => { + it('selects every movable card in the focused list on Ctrl+A', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); items[1].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); items[0].focus(); - keydown(items[0], 'a', { metaKey: true }); + keydown(items[0], 'a', { ctrlKey: true }); const selected = items.filter(isSelected); expect(selected).toEqual([items[0], items[2]]); @@ -1841,12 +1856,12 @@ describe('Sortable lists controller', () => { }); // Would pass under a root-wide select-all. - it('leaves movable cards of other lists unselected on meta A', async () => { + it('leaves movable cards of other lists unselected on Ctrl+A', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); items[0].focus(); - keydown(items[0], 'a', { metaKey: true }); + keydown(items[0], 'a', { ctrlKey: true }); expect(isSelected(items[3])).toBe(false); expect(isSelected(items[4])).toBe(false); @@ -1875,12 +1890,12 @@ describe('Sortable lists controller', () => { // Focusing something other than the first card rules out an anchor that // merely coincides with a document-order fallback. - it('anchors the batch on the focused card after meta A when it is movable', async () => { + it('anchors the batch on the focused card after Ctrl+A when it is movable', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); items[2].focus(); - keydown(items[2], 'a', { metaKey: true }); + keydown(items[2], 'a', { ctrlKey: true }); keydown(items[0], ' ', { shiftKey: true }); expect(items.filter(isSelected)).toEqual([items[0], items[1], items[2]]); @@ -1888,13 +1903,13 @@ describe('Sortable lists controller', () => { // A subsequent Shift gesture proves the fallback anchor is set: no anchor // at all would collapse the range to a single card. - it('anchors the batch on the first movable card after meta A when the focused card is not movable', async () => { + it('anchors the batch on the first movable card after Ctrl+A when the focused card is not movable', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); items[0].setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); items[0].focus(); - keydown(items[0], 'a', { metaKey: true }); + keydown(items[0], 'a', { ctrlKey: true }); keydown(items[2], ' ', { shiftKey: true }); expect(items.filter(isSelected)).toEqual([items[1], items[2]]); @@ -2059,8 +2074,8 @@ describe('Sortable lists controller', () => { const { root, items } = renderSelectableRoot(); await ctx.nextFrame(); click(items[0]); - click(items[1], { metaKey: true }); - click(items[2], { metaKey: true }); + click(items[1], { ctrlKey: true }); + click(items[2], { ctrlKey: true }); items[1].remove(); announceSpy.mockClear(); @@ -2078,7 +2093,7 @@ describe('Sortable lists controller', () => { const { root, items } = renderSelectableRoot(); await ctx.nextFrame(); click(items[0]); - click(items[1], { metaKey: true }); + click(items[1], { ctrlKey: true }); announceSpy.mockClear(); morphRoot(root); @@ -2095,7 +2110,7 @@ describe('Sortable lists controller', () => { const { root, items } = renderSelectableRoot(); await ctx.nextFrame(); click(items[0]); - click(items[1], { metaKey: true }); + click(items[1], { ctrlKey: true }); items[1].remove(); morphRoot(root); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index b5f9d9ec1f04..2c84cf8b6ebe 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -117,14 +117,15 @@ describe('SelectionOrchestrator', () => { }); }); + const userAgentDataDescriptor = Object.getOwnPropertyDescriptor(navigator, 'userAgentData'); + afterEach(() => { root.remove(); document.querySelector('live-region')?.remove(); announceSpy.mockRestore(); + restorePlatform(); }); - // navigator.platform and userAgentData are read-only accessors, so the - // stub is installed per test and torn down with the fixture. function pretendPlatform(platform:string):void { Object.defineProperty(navigator, 'userAgentData', { value: { platform }, @@ -132,6 +133,14 @@ describe('SelectionOrchestrator', () => { }); } + function restorePlatform():void { + if (userAgentDataDescriptor) { + Object.defineProperty(navigator, 'userAgentData', userAgentDataDescriptor); + } else { + delete (navigator as { userAgentData?:unknown }).userAgentData; + } + } + // Type-qualified, because the fixture deliberately holds a section and a // work package that share id 1 — the collision a nested topology makes // routine. @@ -275,7 +284,7 @@ describe('SelectionOrchestrator', () => { root.querySelectorAll('[data-sortable-lists--item-id-value]') .forEach((el) => el.setAttribute('data-sortable-lists--item-mobility-value', 'fixed')); const orchestrator = new SelectionOrchestrator(hostFor(root)); - const event = keydownOn(item('1'), 'a', { metaKey: true }); + const event = keydownOn(item('1'), 'a', { ctrlKey: true }); orchestrator.handleKeydown(event); @@ -285,7 +294,7 @@ describe('SelectionOrchestrator', () => { it('still consumes Ctrl/Cmd+A when there is something to select', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); - const event = keydownOn(item('1'), 'a', { metaKey: true }); + const event = keydownOn(item('1'), 'a', { ctrlKey: true }); orchestrator.handleKeydown(event); @@ -297,7 +306,7 @@ describe('SelectionOrchestrator', () => { item('1').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); const orchestrator = new SelectionOrchestrator(hostFor(root)); - orchestrator.handleKeydown(keydownOn(item('1'), 'a', { metaKey: true })); + orchestrator.handleKeydown(keydownOn(item('1'), 'a', { ctrlKey: true })); expect(orchestrator.selectedIds()).toEqual(['2', '3']); // The fallback anchor is the first orderable card of the same list, so @@ -306,13 +315,47 @@ describe('SelectionOrchestrator', () => { expect(orchestrator.selectedIds()).toEqual(['2', '3']); }); + // Select all binds to the platform's one multi-select modifier: ⌘ on + // Apple platforms, Ctrl elsewhere — never the other way around. + it('consumes Cmd+A on Apple platforms', () => { + pretendPlatform('macOS'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'a', { metaKey: true }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(true); + expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + }); + + it('leaves Ctrl+A alone on Apple platforms', () => { + pretendPlatform('macOS'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'a', { ctrlKey: true }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + + it('leaves Meta+A alone off Apple platforms', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'a', { metaKey: true }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + // With nothing selectable in this list, the browser's own select-all // keeps the gesture even though another list has cards. it('leaves Ctrl/Cmd+A alone when only other lists are selectable', () => { root.querySelectorAll('[data-sortable-lists--list-id-value="7"] [data-sortable-lists--item-id-value]') .forEach((el) => el.setAttribute('data-sortable-lists--item-mobility-value', 'fixed')); const orchestrator = new SelectionOrchestrator(hostFor(root)); - const event = keydownOn(item('1'), 'a', { metaKey: true }); + const event = keydownOn(item('1'), 'a', { ctrlKey: true }); orchestrator.handleKeydown(event); @@ -364,6 +407,18 @@ describe('SelectionOrchestrator', () => { expect(orchestrator.selectedIds()).toEqual(['1', '2']); }); + + // Meta is not an alternate multi-select modifier off Apple platforms: a + // Meta-click classifies as an ordinary click and replaces the batch. + it('does not treat Meta-click as multi-select elsewhere', () => { + pretendPlatform('Windows'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + + orchestrator.handleClick(clickOn(item('2'), { metaKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['2']); + }); }); // collapseForDrag stamps the anchor at drag start, so a cross-list drop @@ -396,9 +451,9 @@ describe('SelectionOrchestrator', () => { it('restarts the batch when Ctrl/Cmd-click lands on another type', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); orchestrator.handleClick(clickOn(item('1'))); - orchestrator.handleClick(clickOn(item('2'), { metaKey: true })); + orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); - orchestrator.handleClick(clickOn(sectionItem('1'), { metaKey: true })); + orchestrator.handleClick(clickOn(sectionItem('1'), { ctrlKey: true })); expect(orchestrator.selectedIds()).toEqual(['1']); expect(isSelected(sectionItem('1'))).toBe(true); @@ -419,7 +474,7 @@ describe('SelectionOrchestrator', () => { it('scopes select-all to the anchoring candidate type', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); - const event = new KeyboardEvent('keydown', { key: 'a', metaKey: true, bubbles: true, cancelable: true }); + const event = new KeyboardEvent('keydown', { key: 'a', ctrlKey: true, bubbles: true, cancelable: true }); Object.defineProperty(event, 'target', { value: sectionItem('1') }); orchestrator.handleKeydown(event); @@ -447,7 +502,7 @@ describe('SelectionOrchestrator', () => { it('drops members that a morph removed from the document', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); orchestrator.handleClick(clickOn(item('1'))); - orchestrator.handleClick(clickOn(item('2'), { metaKey: true })); + orchestrator.handleClick(clickOn(item('2'), { ctrlKey: true })); item('1').remove(); orchestrator.reconcile(); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index 5ab921111574..126f85de894f 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -97,6 +97,13 @@ export class SelectionOrchestrator { this.renderSelection('selection'); } + // The platform's one multi-select modifier: ⌘ on Apple platforms, Ctrl + // elsewhere. Meta is not an alternate modifier on Windows or Linux, and + // Ctrl on Apple is the secondary click, never multi-select. + private multiSelectModifier(event:MouseEvent|KeyboardEvent):boolean { + return isApplePlatform() ? event.metaKey : event.ctrlKey; + } + readonly handleClick = (event:MouseEvent):void => { // Ctrl-click is the secondary click on Apple platforms, where it opens // the contextual menu and Cmd is the multi-select key instead. @@ -104,7 +111,7 @@ export class SelectionOrchestrator { return; } - const multiSelect = event.metaKey || event.ctrlKey; + const multiSelect = this.multiSelectModifier(event); const modified = event.shiftKey || multiSelect; const candidate = this.candidateForGesture(event.target); if (!candidate) { @@ -291,7 +298,7 @@ export class SelectionOrchestrator { // Confined to the focused card's list, like a range. private handleSelectAll(event:KeyboardEvent, candidate:SelectionCandidate):void { - if (!event.metaKey && !event.ctrlKey) { + if (!this.multiSelectModifier(event)) { return; } diff --git a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb index c48bebeb6720..371bbf45cdae 100644 --- a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb +++ b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb @@ -180,7 +180,7 @@ # The fixtures span a sprint and a bucket, so a regression to root-wide # select-all would take this past the sprint's four cards. it "selects every orderable card of the focused card's list with Ctrl/Cmd+A" do - backlogs_page.send_work_package_card_keys(story1, [:control, "a"]) + backlogs_page.send_work_package_card_keys(story1, [backlogs_page.multi_select_modifier, "a"]) expect(page).to have_css("[data-batch-selected]", count: 4) expect(backlogs_page.selected_card_ids).to contain_exactly( diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index 3982a241249b..23d7327018d8 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -29,6 +29,7 @@ #++ require "support/pages/page" +require "support/browsers/browser_platform" module Pages class Backlog < Page @@ -716,7 +717,11 @@ def select_card(work_package) # Toggles membership without navigating, and re-bases the anchor to this # card even when the toggle deselects it. def toggle_card(work_package) - modified_click(work_package, :meta) + modified_click(work_package, multi_select_modifier) + end + + def multi_select_modifier + BrowserPlatform.multi_select_modifier(page) end # Selects the contiguous range from the anchor to this card. Repeated diff --git a/spec/support/browsers/browser_platform.rb b/spec/support/browsers/browser_platform.rb new file mode 100644 index 000000000000..cba8e2b0dd0e --- /dev/null +++ b/spec/support/browsers/browser_platform.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +# The modifier a page under test honours for multi-select: Cmd on an Apple +# platform, Ctrl elsewhere. The browser under test decides, since a grid can +# run it on another OS than this host. +module BrowserPlatform + module_function + + def multi_select_modifier(session = Capybara.current_session) + apple?(session) ? :meta : :control + end + + def apple?(session = Capybara.current_session) + platform_name(session).match?(/mac|darwin|ios/i) + end + + # Selenium and Cuprite can both drive a browser on another machine; only + # the latter has no platform capability, but its user agent names the OS. + def platform_name(session) + driver = session.driver + reported = + case driver + when Capybara::Selenium::Driver then driver.browser.capabilities.platform_name + when Capybara::Cuprite::Driver then driver.browser.version.user_agent + end + + reported.presence || RbConfig::CONFIG["host_os"] + end +end diff --git a/spec/support_spec/browsers/browser_platform_spec.rb b/spec/support_spec/browsers/browser_platform_spec.rb new file mode 100644 index 000000000000..9a3ff86c5a21 --- /dev/null +++ b/spec/support_spec/browsers/browser_platform_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require "spec_helper" +require "support/browsers/browser_platform" + +RSpec.describe BrowserPlatform do + let(:app) { ->(_env) { [200, {}, []] } } + + # Real drivers, so `case` dispatches on the class; none of them starts a + # browser until `browser` is called, which is the one method stubbed. + def selenium_session(platform_name) + capabilities = instance_double(Selenium::WebDriver::Remote::Capabilities, platform_name:) + driver = Capybara::Selenium::Driver.new(app) + allow(driver).to receive(:browser).and_return(instance_double(Selenium::WebDriver::Driver, capabilities:)) + instance_double(Capybara::Session, driver:) + end + + def cuprite_session(user_agent) + version = instance_double(Ferrum::Browser::VersionInfo, user_agent:) + driver = Capybara::Cuprite::Driver.new(app) + allow(driver).to receive(:browser).and_return(instance_double(Capybara::Cuprite::Browser, version:)) + instance_double(Capybara::Session, driver:) + end + + def host_os(value) + stub_const("RbConfig::CONFIG", RbConfig::CONFIG.merge("host_os" => value)) + end + + describe ".multi_select_modifier" do + it "is Cmd when a Selenium browser runs on macOS, whatever the host" do + host_os("linux-gnu") + + expect(described_class.multi_select_modifier(selenium_session("mac"))).to eq(:meta) + end + + it "is Ctrl when a Selenium browser runs elsewhere, whatever the host" do + host_os("darwin24") + + expect(described_class.multi_select_modifier(selenium_session("linux"))).to eq(:control) + end + + it "reads the platform from a Cuprite browser's user agent" do + host_os("darwin24") + linux_ua = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/140.0 Safari/537.36" + + expect(described_class.multi_select_modifier(cuprite_session(linux_ua))).to eq(:control) + end + + it "falls back to the host when the browser reports no platform" do + host_os("darwin24") + + expect(described_class.multi_select_modifier(selenium_session(nil))).to eq(:meta) + end + + it "falls back to the host for a RackTest driver" do + host_os("linux-gnu") + session = instance_double(Capybara::Session, driver: Capybara::RackTest::Driver.new(app)) + + expect(described_class.multi_select_modifier(session)).to eq(:control) + end + end +end From eecd5222330f96e18db6553319e129be89c2f5e7 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:16:10 -0800 Subject: [PATCH 09/22] Tighten the selection adapter's identity checks Matches the anchor row by (type, id) like every other lookup, bounds the focus host to the item's own subtree so a nested item's target cannot stand in, keys lists by their values rather than DOM id, and keeps range rows to items the list itself owns: the rows-container boundary bounds only the upward walk, and the downward fallback handed a structural row the first item of a list nested inside it. --- .../dynamic/sortable-lists/selection.spec.ts | 56 +++++++++++++++++++ .../dynamic/sortable-lists/selection.ts | 44 +++++++++------ 2 files changed, 83 insertions(+), 17 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts index 06cb29760161..bf9de05b3679 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.spec.ts @@ -118,6 +118,12 @@ describe('sortable-lists selection adapter', () => { }); }); + it('keys the list by its type and id, not its DOM id', () => { + itemFor('1').closest('[data-controller~="sortable-lists--list"]')!.id = 'inbox_project_4'; + + expect(candidateFor('1').listKey).toBe('sprint:7'); + }); + // The focus host is also the boundary the interactive-descendant check // stops at, so a focusable host does not disqualify its own card. it('reports the focus target as the focus host when there is one', () => { @@ -129,6 +135,13 @@ describe('sortable-lists selection adapter', () => { expect(resolveCandidate(root, card)!.focusHost).toBe(card); }); + it('does not take a nested item\'s focus target as the focus host', () => { + appendNestedList(); + itemFor('60').setAttribute('data-sortable-lists--item-target', 'focus'); + + expect(candidateFor('6').focusHost).toBe(itemFor('6')); + }); + it('resolves a non-movable candidate', () => { expect(candidateFor('5').orderable).toBe(false); }); @@ -206,6 +219,38 @@ describe('sortable-lists selection adapter', () => { expect(resolveRangeItems(root, anchor, candidateFor('4'), rowsContainerFor(itemFor('4')))).toEqual({ ok: false, reason: 'crossList' }); }); + it('does not take a same-id item of another type for the anchor', () => { + const decoy = document.createElement('li'); + decoy.setAttribute('data-controller', 'sortable-lists--item'); + decoy.setAttribute('data-sortable-lists--item-id-value', '1'); + decoy.setAttribute('data-sortable-lists--item-type-value', 'decoy'); + root.querySelector('ul')!.prepend(decoy); + const anchor = { type: 'work_package', id: '1', listKey: 'sprint:7' }; + const candidate = candidateFor('2'); + + expect(resolveRangeItems(root, anchor, candidate, rowsContainerFor(candidate.itemElement))).toEqual({ ok: true, items: [{ type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }] }); + }); + + // A structural row holding only a nested list has no item of its own; the + // nested list's first field must not be mistaken for one. + it('rejects a range across a row that only hosts a nested list', () => { + const hostRow = document.createElement('li'); + hostRow.innerHTML = ` +
      +
        +
      • +
      +
      + `; + itemFor('1').after(hostRow); + const anchor = { type: 'work_package', id: '1', listKey: 'sprint:7' }; + const candidate = candidateFor('2'); + + expect(resolveRangeItems(root, anchor, candidate, rowsContainerFor(candidate.itemElement))).toEqual({ ok: false, reason: 'unavailable' }); + }); + it('rejects a range whose anchor id was never a row in this list', () => { const anchor = { type: 'work_package', id: '99', listKey: 'sprint:7' }; @@ -298,6 +343,17 @@ describe('sortable-lists selection adapter', () => { // Nothing prunes duplicates on read, so a repeated apply is the only thing // that catches a regression of the write-time de-duplication. + it('describes the outer item, not a nested item\'s focus target', () => { + appendNestedList(); + const nestedTarget = itemFor('60'); + nestedTarget.setAttribute('data-sortable-lists--item-target', 'focus'); + + applySelectionPresentation(root, new Set([selectionKey({ type: 'section', id: '6' })]), 'selected-description'); + + expect(itemFor('6').getAttribute('aria-describedby')).toBe('selected-description'); + expect(nestedTarget.hasAttribute('aria-describedby')).toBe(false); + }); + it('does not accumulate duplicate description tokens on repeated apply', () => { applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' })]), 'selected-description'); applySelectionPresentation(root, new Set([selectionKey({ type: 'work_package', id: '1' })]), 'selected-description'); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts index 400bb11998bc..e284002367c7 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts @@ -53,20 +53,24 @@ export interface SelectionCandidate extends SelectionItem { export const itemFocusTargetSelector = '[data-sortable-lists--item-target~="focus"]'; -// Decides only whether a range stays inside the list it started in. The DOM -// id survives a morph; the type/id pair is the fallback for consumers that -// render none. +// Decides only whether a range stays inside the list it started in. Derived +// from the list's own values rather than its DOM id, so every consumer keys +// its lists the same way. function listKeyOf(listElement:HTMLElement):string { - if (listElement.id !== '') { - return listElement.id; - } - const type = listElement.getAttribute('data-sortable-lists--list-type-value') ?? ''; const id = listElement.getAttribute('data-sortable-lists--list-id-value') ?? ''; return `${type}:${id}`; } +// Bounded to the item's own subtree: a nested list's items carry focus +// targets of their own, and a descendant's must never stand in for the +// outer item's. +function focusHostOf(itemElement:HTMLElement):HTMLElement { + return Array.from(itemElement.querySelectorAll(itemFocusTargetSelector)) + .find((target) => target.closest(sortableItemSelector) === itemElement) ?? itemElement; +} + // A child belongs to the nearest root, not to any root containing it: an // independently nested root is an ownership boundary. function ownsElement(root:HTMLElement, element:Element):boolean { @@ -79,13 +83,13 @@ function ownerList(root:HTMLElement, itemElement:HTMLElement):HTMLElement|null { return list && ownsElement(root, list) ? list : null; } -// Bounded by the rows container: resolveItemElement's querySelector fallback -// descends unbounded, so a section row would otherwise resolve to the first -// field of the list nested inside it. -function rowItemId(row:Element, rowsContainer:Element):string|null { +// The boundary bounds only the upward walk; resolveItemElement's downward +// fallback would hand a structural row the first item of a list nested +// inside it, so the resolved item must also belong to this list. +function rowItem(row:Element, rowsContainer:Element, root:HTMLElement, list:HTMLElement):HTMLElement|null { const item = resolveItemElement(row, rowsContainer); - return item ? resolveItemId(item) : null; + return item && ownerList(root, item) === list ? item : null; } export function orderedItemElements(root:HTMLElement):HTMLElement[] { @@ -125,7 +129,7 @@ export function resolveCandidate(root:HTMLElement, target:EventTarget|null):Sele return { type, itemElement, - focusHost: itemElement.querySelector(itemFocusTargetSelector) ?? itemElement, + focusHost: focusHostOf(itemElement), id, listKey: listKeyOf(list), orderable: isOrderableItem(itemElement), @@ -194,12 +198,18 @@ export function resolveRangeItems( return { ok: false, reason: 'crossList' }; } - if (!ownerList(root, candidate.itemElement) || !rowsContainer) { + const list = ownerList(root, candidate.itemElement); + if (!list || !rowsContainer) { return { ok: false, reason: 'unavailable' }; } const rows = Array.from(rowsContainer.children); - const anchorRow = rows.find((row) => rowItemId(row, rowsContainer) === anchor.id); + const anchorKey = selectionKey(anchor); + const anchorRow = rows.find((row) => { + const item = rowItem(row, rowsContainer, root, list); + const identity = item && itemIdentity(item); + return identity !== null && selectionKey(identity) === anchorKey; + }); // A candidate whose item sits outside the rows container has no row here. const candidateRow = rowOf(rowsContainer, candidate.itemElement); if (!anchorRow || !candidateRow) { @@ -212,7 +222,7 @@ export function resolveRangeItems( const items:SelectionItem[] = []; for (const row of span) { - const item = resolveItemElement(row, rowsContainer); + const item = rowItem(row, rowsContainer, root, list); const id = item ? resolveItemId(item) : null; // A truncation marker in the span and a card the user cannot move are // both hard boundaries, but only the first can be resolved by expanding. @@ -252,7 +262,7 @@ export function applySelectionPresentation( ):void { for (const item of orderedItemElements(root)) { const identity = itemIdentity(item); - const focusHost = item.querySelector(itemFocusTargetSelector) ?? item; + const focusHost = focusHostOf(item); if (identity && keys.has(selectionKey(identity))) { item.setAttribute(batchSelectedAttribute, ''); From b42f2f9713037436aa8aaca32bc51d9a3d71a46a Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:19:37 -0800 Subject: [PATCH 10/22] Stub selection announcements with keyed stand-ins Both specs carried a copy of the production strings and one had already drifted. A shared fixture of key tokens asserts message choice and pluralisation without a copy to keep in step. Also covers a Shift range spanning a fixed card, not only ending on one. --- .../dynamic/sortable-lists.controller.spec.ts | 32 ++++++----------- .../selection-orchestrator.spec.ts | 35 +++++++++--------- .../testing/selection-translations.ts | 36 +++++++++++++++++++ 3 files changed, 63 insertions(+), 40 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/testing/selection-translations.ts diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 73aea419d6ba..9a97167fae04 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -60,6 +60,7 @@ import { type Mock, type MockInstance } from 'vitest'; import { LiveRegionElement } from '@primer/live-region-element'; import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; import type SortableListsControllerType from './sortable-lists.controller'; +import { selectionTranslations } from './sortable-lists/testing/selection-translations'; import type { sortableItemData as sortableItemDataFn, sortableListData as sortableListDataFn, @@ -384,20 +385,7 @@ describe('Sortable lists controller', () => { moved: '%{label} moved to position %{position} of %{total}', moved_to_list: '%{label} moved to %{list}, position %{position} of %{total}', }, - selection: { - cleared: 'Selection cleared.', - not_selectable: 'Selection unchanged. This item takes no part in this list\'s ordering.', - range_blocked: 'Selection unchanged. That range contains an item that takes no part in this list\'s ordering.', - range_restarted: { - one: 'Could not extend the range. 1 item selected.', - other: 'Could not extend the range. %{count} items selected.', - }, - range_unavailable: 'Selection unchanged. Expand this list to select that range.', - selected: { - one: '1 item selected.', - other: '%{count} items selected.', - }, - }, + selection: selectionTranslations, }, }, }, @@ -1331,7 +1319,7 @@ describe('Sortable lists controller', () => { expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[0]]); expect(announceSpy.mock.calls.map((call) => [call[0], call[1]])).toEqual([ - ['1 item selected.', { politeness: 'polite' }], + ['[selected:1]', { politeness: 'polite' }], ]); }); @@ -1436,7 +1424,7 @@ describe('Sortable lists controller', () => { expect(items.filter(isSelected)).toEqual([items[0]]); expect(announcedMessages()).toEqual([ - ['1 item selected.', { politeness: 'polite' }], + ['[selected:1]', { politeness: 'polite' }], ]); }); @@ -1522,7 +1510,7 @@ describe('Sortable lists controller', () => { expect(items.filter(isSelected)).toEqual([items[3]]); expect(announcedMessages()).toEqual([ - ['Could not extend the range. 1 item selected.', { politeness: 'polite' }], + ['[range_restarted:1]', { politeness: 'polite' }], ]); }); @@ -1547,7 +1535,7 @@ describe('Sortable lists controller', () => { expect(isSelected(items[0])).toBe(false); expect(isSelected(items[2])).toBe(true); expect(announcedMessages()).toEqual([ - ['Could not extend the range. 1 item selected.', { politeness: 'polite' }], + ['[range_restarted:1]', { politeness: 'polite' }], ]); }); @@ -1565,7 +1553,7 @@ describe('Sortable lists controller', () => { expect(items.filter(isSelected)).toEqual([items[0]]); expect(announcedMessages()).toEqual([ - ['Selection unchanged. Expand this list to select that range.', { politeness: 'polite' }], + ['[range_unavailable]', { politeness: 'polite' }], ]); }); @@ -1581,7 +1569,7 @@ describe('Sortable lists controller', () => { expect(items.filter(isSelected)).toEqual([items[0]]); expect(announcedMessages()).toEqual([ - ['Selection unchanged. That range contains an item that takes no part in this list\'s ordering.', { politeness: 'polite' }], + ['[range_blocked]', { politeness: 'polite' }], ]); }); @@ -1598,7 +1586,7 @@ describe('Sortable lists controller', () => { expect(items.filter(isSelected)).toEqual([items[0]]); expect(event.defaultPrevented).toBe(true); expect(announcedMessages()).toEqual([ - ['Selection unchanged. This item takes no part in this list\'s ordering.', { politeness: 'polite' }], + ['[not_selectable]', { politeness: 'polite' }], ]); }); @@ -2083,7 +2071,7 @@ describe('Sortable lists controller', () => { await ctx.nextFrame(); expect(announcedMessages()).toEqual([ - ['2 items selected.', { politeness: 'polite' }], + ['[selected:2]', { politeness: 'polite' }], ]); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index 2c84cf8b6ebe..b713a755704f 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -29,6 +29,7 @@ import { LiveRegionElement } from '@primer/live-region-element'; import { type MockInstance } from 'vitest'; import type { SelectionHost } from './selection-orchestrator'; +import { selectionTranslations } from './testing/selection-translations'; // No Stimulus application, no outlets, no controller lifecycle: the host // port lets selection be driven over a plain DOM. @@ -99,22 +100,7 @@ describe('SelectionOrchestrator', () => { document.body.append(root, document.createElement('live-region')); announceSpy = vi.spyOn(LiveRegionElement.prototype, 'announce'); pretendPlatform('Windows'); - window.I18n.store({ - en: { - js: { - sortable_lists: { - selection: { - cleared: 'Selection cleared.', - not_selectable: 'Selection unchanged. This item cannot be selected.', - range_blocked: 'Selection unchanged. That range contains an item that takes no part in this list\'s ordering.', - range_restarted: { one: 'Could not extend the range. 1 item selected.', other: 'Could not extend the range. %{count} items selected.' }, - range_unavailable: 'Selection unchanged. Expand this list to select that range.', - selected: { one: '1 item selected.', other: '%{count} items selected.' }, - }, - }, - }, - }, - }); + window.I18n.store({ en: { js: { sortable_lists: { selection: selectionTranslations } } } }); }); const userAgentDataDescriptor = Object.getOwnPropertyDescriptor(navigator, 'userAgentData'); @@ -231,7 +217,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('1'), { shiftKey: true })); expect(orchestrator.selectedIds()).toEqual(['1', '2']); - expect(spoken()).toEqual(['2 items selected.']); + expect(spoken()).toEqual(['[selected:2]']); }); // The baseline is what the previous render painted: tracking the last @@ -267,7 +253,7 @@ describe('SelectionOrchestrator', () => { orchestrator.handleClick(clickOn(item('2'))); - expect(spoken()).toEqual(['1 item selected.']); + expect(spoken()).toEqual(['[selected:1]']); }); }); @@ -383,6 +369,19 @@ describe('SelectionOrchestrator', () => { expect(orchestrator.selectedIds()).toEqual([]); }); + it('refuses to extend a range over a fixed card', () => { + item('2').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + announceSpy.mockClear(); + + orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); + + expect(orchestrator.selectedIds()).toEqual(['1']); + expect(isSelected(item('3'))).toBe(false); + expect(announceSpy.mock.calls.map((call) => call[0])).toEqual(['[range_blocked]']); + }); + // Ctrl-click opens the contextual menu on Apple platforms: it must // neither toggle nor fall through to the ordinary-click path. it('ignores Ctrl-click entirely on Apple platforms', () => { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/testing/selection-translations.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/testing/selection-translations.ts new file mode 100644 index 000000000000..ee586e2cc921 --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/testing/selection-translations.ts @@ -0,0 +1,36 @@ +//-- 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. +//++ + +export const selectionTranslations = { + cleared: '[cleared]', + not_selectable: '[not_selectable]', + range_blocked: '[range_blocked]', + range_restarted: { one: '[range_restarted:1]', other: '[range_restarted:%{count}]' }, + range_unavailable: '[range_unavailable]', + selected: { one: '[selected:1]', other: '[selected:%{count}]' }, +}; From b803a9d7c13cae76e7b1a504b3ee4958640ecf04 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:21:54 -0800 Subject: [PATCH 11/22] Tidy selection leftovers from review Drops the unread mobility value declaration, makes teardown reset the model it discards, and has the card controller ignore the same inner controls the orchestrator does, under a name that says it tracks the current card rather than a selection. --- .../backlogs/work-package.controller.ts | 32 +++++++------------ .../dynamic/sortable-lists/item.controller.ts | 1 - .../sortable-lists/selection-orchestrator.ts | 5 ++- 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts index 8e1767399e34..7821095d7000 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/work-package.controller.ts @@ -30,6 +30,7 @@ import { Controller } from '@hotwired/stimulus'; import * as Turbo from '@hotwired/turbo'; import type { TurboVisitEvent } from '@hotwired/turbo'; import { WP_ID_URL_PATTERN } from 'core-app/shared/helpers/work-package-id-pattern'; +import { closestInteractiveElement } from 'core-common/interactive-element-helper'; const DETAILS_URL_PATTERN = new RegExp(`/details/(${WP_ID_URL_PATTERN})(?:/|$)`); @@ -57,10 +58,10 @@ export default class WorkPackageController extends Controller imple this.element.addEventListener('dblclick', this, { signal }); this.element.addEventListener('keydown', this, { signal }); document.addEventListener('turbo:visit', (event:TurboVisitEvent) => { - this.syncSelectionFromUrl(event.detail.url); + this.syncCurrentFromUrl(event.detail.url); }, { signal }); - this.syncSelectionFromUrl(window.location.href); + this.syncCurrentFromUrl(window.location.href); } disconnect():void { @@ -73,7 +74,7 @@ export default class WorkPackageController extends Controller imple } } - private syncSelectionFromUrl(locationUrl:string):void { + private syncCurrentFromUrl(locationUrl:string):void { const { pathname } = new URL(locationUrl, window.location.origin); const [, id] = DETAILS_URL_PATTERN.exec(pathname) ?? []; // Bookmarks and external links may still carry a numeric ID after the @@ -114,7 +115,7 @@ export default class WorkPackageController extends Controller imple const target = event.target; if (!(target instanceof HTMLElement)) return; - if (this.shouldIgnoreMouseTarget(target)) return; + if (this.shouldIgnoreTarget(target)) return; if (this.clickTimeout !== null) return; @@ -128,7 +129,7 @@ export default class WorkPackageController extends Controller imple const target = event.target; if (!(target instanceof HTMLElement)) return; - if (this.shouldIgnoreMouseTarget(target)) return; + if (this.shouldIgnoreTarget(target)) return; if (this.clickTimeout !== null) { clearTimeout(this.clickTimeout); @@ -144,7 +145,7 @@ export default class WorkPackageController extends Controller imple const target = event.target; if (!(target instanceof HTMLElement)) return; - if (this.shouldIgnoreKeyboardTarget(target)) return; + if (this.shouldIgnoreTarget(target)) return; event.preventDefault(); @@ -163,20 +164,9 @@ export default class WorkPackageController extends Controller imple Turbo.visit(this.fullUrlValue, { frame: '_top' }); } - private shouldIgnoreMouseTarget(target:HTMLElement):boolean { - return [ - 'a', - 'button', - 'clipboard-copy', - ].some((selector) => target.closest(selector) !== null); - } - - private shouldIgnoreKeyboardTarget(target:HTMLElement):boolean { - return this.shouldIgnoreMouseTarget(target) || [ - 'input', - 'textarea', - 'select', - "[contenteditable='true']", - ].some((selector) => target.closest(selector) !== null); + // The same rule the selection orchestrator applies: a control inside the + // card is that control first. The card itself is the boundary, not a hit. + private shouldIgnoreTarget(target:HTMLElement):boolean { + return closestInteractiveElement(target, this.element) !== null; } } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts index 15c373f7906d..9ee39dd476b4 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -78,7 +78,6 @@ export default class ItemController extends Controller implements R declare readonly hideUnavailableValue:boolean; declare readonly labelValue:string; declare readonly hasLabelValue:boolean; - declare readonly mobilityValue:string; declare readonly handleTarget:HTMLElement; declare readonly hasHandleTarget:boolean; diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index 126f85de894f..37934d33bdff 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -492,12 +492,15 @@ export class SelectionOrchestrator { } } - // Presentation only: whatever restores the page builds a fresh orchestrator. + // Presentation only: a restored page brings its markup back under a fresh + // orchestrator, whose model and render baseline are already empty. clearPresentation():void { applySelectionPresentation(this.host.rootElement, new Set(), this.host.descriptionId); } teardown():void { + this.selection.clear(); + this.lastRenderedKeys = new Set(); this.clearPresentation(); } } From 925c865e62ad08480e3051551842631aff4feedd Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:39:09 -0800 Subject: [PATCH 12/22] Read select-all the way the browser does Accepts the physical A key when the layout prints another script on it, while still following the key's meaning on Latin layouts such as AZERTY, where Ctrl+A sits on KeyQ. --- .../selection-orchestrator.spec.ts | 63 +++++++++++++++++++ .../sortable-lists/selection-orchestrator.ts | 20 ++++-- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index b713a755704f..d4354f3bcd3d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -335,6 +335,69 @@ describe('SelectionOrchestrator', () => { expect(orchestrator.selectedIds()).toEqual([]); }); + // Non-Latin layouts print another letter on the A key; AZERTY prints A + // on KeyQ. Both users expect Ctrl+A where their keyboard says A. + it('selects all from the physical A key on a non-Latin layout', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'ф', { ctrlKey: true, code: 'KeyA' }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(true); + expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + }); + + it('selects all from the A key of an AZERTY layout', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'a', { ctrlKey: true, code: 'KeyQ' }); + + orchestrator.handleKeydown(event); + + expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + }); + + it('leaves Ctrl+Q alone on an AZERTY layout although it sits on KeyA', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'q', { ctrlKey: true, code: 'KeyA' }); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + + // Non-Latin keys throughout, so that only the guard under test, never + // the Latin exclusion, can be what leaves the key alone. + it.each([ + ['a dead key', { key: 'Dead', code: 'KeyA', ctrlKey: true }], + ['IME composition', { key: 'Process', code: 'KeyA', ctrlKey: true }], + ['a composing letter', { key: 'ф', code: 'KeyA', ctrlKey: true, isComposing: true }], + ['punctuation on the A key', { key: ';', code: 'KeyA', ctrlKey: true }], + ['an AltGr chord', { key: 'ф', code: 'KeyA', ctrlKey: true, altKey: true }], + ])('leaves %s alone although it sits on KeyA', (_label, init) => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const { key, ...rest } = init as KeyboardEventInit & { key:string }; + const event = keydownOn(item('1'), key, rest); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + + // Stubbed rather than passed as `modifierAltGraph`: not every engine maps + // the init member onto getModifierState. + it('leaves an AltGraph chord alone although it sits on KeyA', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const event = keydownOn(item('1'), 'ф', { code: 'KeyA', ctrlKey: true }); + vi.spyOn(event, 'getModifierState').mockImplementation((key) => key === 'AltGraph'); + + orchestrator.handleKeydown(event); + + expect(event.defaultPrevented).toBe(false); + expect(orchestrator.selectedIds()).toEqual([]); + }); + // With nothing selectable in this list, the browser's own select-all // keeps the gesture even though another list has cards. it('leaves Ctrl/Cmd+A alone when only other lists are selectable', () => { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index 37934d33bdff..f91a90b324e2 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -62,6 +62,19 @@ export interface SelectionHost { /** * Batch selection: gestures in, model and presentation out. */ +// Read the way browsers bind their own select-all: by the key's meaning on +// Latin layouts (AZERTY's Ctrl+A sits on physical KeyQ), by the physical key +// where the layout prints another letter (Cyrillic ф, Greek α on KeyA). A +// dead key, composition or an AltGr chord is input, not a shortcut. +function isSelectAllKey(event:KeyboardEvent):boolean { + if (event.isComposing || event.altKey || event.getModifierState('AltGraph')) { + return false; + } + + return event.key === 'a' || event.key === 'A' + || (event.code === 'KeyA' && /^\p{L}$/u.test(event.key) && !/^\p{Script=Latin}$/u.test(event.key)); +} + export class SelectionOrchestrator { private readonly selection = new BatchSelection(); @@ -199,13 +212,12 @@ export class SelectionOrchestrator { case 'End': this.handleBoundary(event, candidate, event.key === 'Home' ? 'first' : 'last'); break; - case 'a': - case 'A': - this.handleSelectAll(event, candidate); - break; default: // Enter belongs to the card's own activation handler; Escape is // handled at the document. + if (isSelectAllKey(event)) { + this.handleSelectAll(event, candidate); + } break; } }; From 6396db5c553d80020fdc6781f9aa701a2d0c03b7 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:40:03 -0800 Subject: [PATCH 13/22] Let arrows move focus while a move is in flight Only the Shift range mutation waits for the host; plain navigation never touched the model and has no reason to stall. --- .../selection-orchestrator.spec.ts | 20 +++++++++++++++++++ .../sortable-lists/selection-orchestrator.ts | 11 ++-------- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index d4354f3bcd3d..a7e499c63b1f 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -264,6 +264,26 @@ describe('SelectionOrchestrator', () => { return event; }; + it('still moves focus with the arrows while a move is in flight', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + busy = true; + + orchestrator.handleKeydown(keydownOn(item('1'), 'ArrowDown')); + + expect(focused).toBe(item('2')); + }); + + it('moves focus but does not extend the range with Shift+Arrow while busy', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + busy = true; + + orchestrator.handleKeydown(keydownOn(item('1'), 'ArrowDown', { shiftKey: true })); + + expect(focused).toBe(item('2')); + expect(orchestrator.selectedIds()).toEqual(['1']); + }); + // Consuming the key with nothing to select would block the browser's own // select-all and announce nothing in its place. it('leaves Ctrl/Cmd+A alone when nothing is selectable', () => { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index f91a90b324e2..488ed432e662 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -257,10 +257,6 @@ export class SelectionOrchestrator { return; } - if (this.host.busy) { - return; - } - this.focusAndMaybeExtend(event, next); } @@ -278,17 +274,14 @@ export class SelectionOrchestrator { return; } - if (this.host.busy) { - return; - } - this.focusAndMaybeExtend(event, target); } + // Focus moves even mid-move: only the range mutation waits for the host. private focusAndMaybeExtend(event:KeyboardEvent, target:HTMLElement):void { this.focusItemElement(target); - if (!event.shiftKey) { + if (!event.shiftKey || this.host.busy) { return; } From 62e90dc1cf5f23c9546c772c7030005138defe92 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:42:17 -0800 Subject: [PATCH 14/22] Announce a plain click only when a batch collapses Opening the details pane is feedback enough for the card itself; the count is worth hearing only once a wider selection is lost. --- .../selection-orchestrator.spec.ts | 23 +++++++++++++++++++ .../sortable-lists/selection-orchestrator.ts | 7 +++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index a7e499c63b1f..1a9b0850cf88 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -234,6 +234,29 @@ describe('SelectionOrchestrator', () => { expect(spoken()).toEqual([]); }); + // The details pane opening is the feedback for that card; the count only + // matters once a wider batch is lost. + it('stays silent on the first plain click', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + + orchestrator.handleClick(clickOn(item('1'))); + + expect(orchestrator.selectedIds()).toEqual(['1']); + expect(spoken()).toEqual([]); + }); + + it('stays silent when a plain click on a fixed card drops a single selected card', () => { + item('2').setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + announceSpy.mockClear(); + + orchestrator.handleClick(clickOn(item('2'))); + + expect(orchestrator.selectedIds()).toEqual([]); + expect(spoken()).toEqual([]); + }); + it('stays silent on a plain click that swaps a one-card selection', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); orchestrator.handleClick(clickOn(item('1'))); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index 488ed432e662..4a6527b2b52d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -439,8 +439,9 @@ export class SelectionOrchestrator { } // Announcement follows the gesture class, not the count: `navigation` (a - // plain click) speaks only when the count moves, since the details pane it - // opens is its own feedback; `selection` speaks on any membership change. + // plain click) speaks only when it collapsed a batch, since the details + // pane it opens is its own feedback for the card itself; `selection` + // speaks on any membership change. private renderSelection(kind:'navigation'|'selection'):void { const previous = this.lastRenderedKeys; this.syncSelectionPresentation(); @@ -449,7 +450,7 @@ export class SelectionOrchestrator { this.lastRenderedKeys = current; const changed = kind === 'navigation' - ? current.size !== previous.size + ? previous.size > 1 && current.size !== previous.size : current.size !== previous.size || [...current].some((id) => !previous.has(id)); if (changed) { From 66bb84122992df738760c1845617f761ef2534b9 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:43:13 -0800 Subject: [PATCH 15/22] Clear the selection on Escape from anywhere Focus routinely rests in the details pane or the page header after a selection. Only an overlay or a field whose widget owns Escape keeps the key. --- .../dynamic/sortable-lists.controller.spec.ts | 96 +++++++++++++++++++ .../selection-orchestrator.spec.ts | 35 +++++++ .../sortable-lists/selection-orchestrator.ts | 31 +++--- 3 files changed, 151 insertions(+), 11 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 9a97167fae04..75df6aaeb142 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -2008,6 +2008,102 @@ describe('Sortable lists controller', () => { expect(isSelected(items[0])).toBe(true); }); + // Focus routinely rests in the details pane or the page header after a + // selection; Escape there still means "drop the selection". + it('clears the batch on Escape from a control outside the root', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + const button = document.createElement('button'); + document.body.appendChild(button); + button.focus(); + + try { + keydown(button, 'Escape'); + } finally { + button.remove(); + } + + expect(items.some(isSelected)).toBe(false); + }); + + it('leaves Escape to a text field outside the root', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + const input = document.createElement('input'); + document.body.appendChild(input); + input.focus(); + + try { + const event = keydown(input, 'Escape'); + expect(event.defaultPrevented).toBe(false); + } finally { + input.remove(); + } + + expect(isSelected(items[0])).toBe(true); + }); + + it('leaves Escape to an open dialog', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + const dialog = document.createElement('dialog'); + const button = document.createElement('button'); + dialog.appendChild(button); + document.body.appendChild(dialog); + dialog.showModal(); + + try { + const event = keydown(button, 'Escape'); + expect(event.defaultPrevented).toBe(false); + } finally { + dialog.close(); + dialog.remove(); + } + + expect(isSelected(items[0])).toBe(true); + }); + + // A menu opens its popover before moving focus into it on the next frame, + // so an Escape in between still targets the invoker inside the card. + it('leaves Escape to an open popover while focus is still on its invoker', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + const invoker = document.createElement('button'); + const popover = document.createElement('div'); + popover.setAttribute('popover', ''); + items[1].append(invoker, popover); + popover.showPopover(); + invoker.focus(); + + try { + const event = keydown(invoker, 'Escape'); + expect(event.defaultPrevented).toBe(false); + } finally { + popover.hidePopover(); + } + + expect(isSelected(items[0])).toBe(true); + }); + + it('clears the batch on Escape once the popover has closed', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + const popover = document.createElement('div'); + popover.setAttribute('popover', ''); + items[1].appendChild(popover); + popover.showPopover(); + popover.hidePopover(); + + keydown(items[1], 'Escape'); + + expect(isSelected(items[0])).toBe(false); + }); + it('leaves Enter to the navigation handler', async () => { const { items } = renderSelectableRoot(); await ctx.nextFrame(); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index 1a9b0850cf88..59c664fc8bc0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -203,6 +203,41 @@ describe('SelectionOrchestrator', () => { expect(orchestrator.selectedIds()).toEqual(['1']); }); + // Both roots listen at the document; the first to clear consumes the key, + // which must not read as an overlay's Escape to the second. + it('clears every root\'s batch on one Escape, not only the first listener\'s', () => { + const secondRoot = root.cloneNode(true) as HTMLElement; + document.body.appendChild(secondRoot); + const first = new SelectionOrchestrator(hostFor(root)); + const second = new SelectionOrchestrator(hostFor(secondRoot)); + first.handleClick(clickOn(item('1'))); + second.handleClick(clickOn(secondRoot.querySelector('[data-sortable-lists--item-id-value="2"]')!)); + const event = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }); + Object.defineProperty(event, 'target', { value: document.body }); + + try { + first.handleEscape(event); + second.handleEscape(event); + } finally { + secondRoot.remove(); + } + + expect(first.selectedIds()).toEqual([]); + expect(second.selectedIds()).toEqual([]); + }); + + it('still leaves an Escape an overlay consumed alone', () => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('1'))); + const event = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }); + Object.defineProperty(event, 'target', { value: document.body }); + event.preventDefault(); + + orchestrator.handleEscape(event); + + expect(orchestrator.selectedIds()).toEqual(['1']); + }); + describe('announcements', () => { const spoken = () => announceSpy.mock.calls.map((call) => call[0]); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index 4a6527b2b52d..b354f224d642 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -75,6 +75,10 @@ function isSelectAllKey(event:KeyboardEvent):boolean { || (event.code === 'KeyA' && /^\p{L}$/u.test(event.key) && !/^\p{Script=Latin}$/u.test(event.key)); } +// Every root listens for Escape at the document, so the first to clear +// would otherwise look to the next like an overlay that consumed the key. +const escapesClearedBySelection = new WeakSet(); + export class SelectionOrchestrator { private readonly selection = new BatchSelection(); @@ -334,8 +338,10 @@ export class SelectionOrchestrator { // At the document, in the bubble phase: focus routinely sits off the root // after a mouse selection, and an overlay's own Escape must run first. readonly handleEscape = (event:KeyboardEvent):void => { - // A consumed Escape already answered the keystroke. - if (event.key !== 'Escape' || event.defaultPrevented) { + // A consumed Escape already answered the keystroke — unless it was + // another root's selection that consumed it, in which case this root's + // batch still has to go. + if (event.key !== 'Escape' || (event.defaultPrevented && !escapesClearedBySelection.has(event))) { return; } @@ -351,24 +357,27 @@ export class SelectionOrchestrator { } event.preventDefault(); + escapesClearedBySelection.add(event); this.selection.clear(); this.renderSelection('selection'); }; - // An overlay and an unrelated widget both own their own Escape, which - // means "dismiss that thing" rather than "drop this selection". + // Escape drops the selection from wherever focus sits, except where the + // key already means "dismiss this thing": an open overlay anywhere — focus + // can still sit on its invoker for a frame after it opens — or a field + // whose widget owns it (a picker, an inline editor, an autocompleter). private escapeConcernsSelection(target:EventTarget|null):boolean { - if (!(target instanceof Element)) { - return true; + if (document.querySelector('dialog[open], :popover-open')) { + return false; } - if (target.closest('dialog, [popover], [role="dialog"], [role="menu"], [role="listbox"]')) { - return false; + if (!(target instanceof Element)) { + return true; } - return target === document.body - || target === document.documentElement - || this.host.rootElement.contains(target); + return target.closest( + '[role="dialog"], [role="menu"], [role="listbox"], input, textarea, select, [contenteditable]', + ) === null; } // A batch holds one item type: "all of these together" has no meaning From f4fa2b7e70479309521999a5e3851df54d434913 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 1 Sep 2026 13:46:33 -0800 Subject: [PATCH 16/22] Reconcile the selection when rows are replaced The "show more" expander is a frame navigation, not a morph: it swaps the rows wholesale and no morph event follows, so the fresh rows came back unmarked while the model and live region still held the batch. Item outlet churn now schedules the same reconcile a morph does. --- .../dynamic/sortable-lists.controller.spec.ts | 45 +++++++++++++++++++ .../dynamic/sortable-lists.controller.ts | 20 +++++++++ .../work_packages/batch_selection_spec.rb | 25 +++++++++++ 3 files changed, 90 insertions(+) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 75df6aaeb142..814d026f2a71 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -2130,6 +2130,51 @@ describe('Sortable lists controller', () => { root.dispatchEvent(new CustomEvent('turbo:morph-element', { bubbles: true })); } + // A frame navigation, such as the "show more" expander, swaps the rows + // wholesale without a morph event; the fresh rows arrive unmarked. + describe('reconciling the batch after rows are replaced', () => { + const replaceRows = (items:HTMLElement[], keep:(item:HTMLElement) => boolean) => { + const rowsContainer = items[0].parentElement!; + const fresh = items.filter(keep).map((item) => { + const clone = item.cloneNode(true) as HTMLElement; + clone.removeAttribute('data-batch-selected'); + return clone; + }); + rowsContainer.replaceChildren(...fresh); + return fresh; + }; + + it('repaints the surviving members on the fresh rows without announcing', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { ctrlKey: true }); + announceSpy.mockClear(); + + const fresh = replaceRows(items, () => true); + await ctx.nextFrame(); + + expect(fresh.filter(isSelected)).toEqual([fresh[0], fresh[1]]); + expect(announceSpy).not.toHaveBeenCalled(); + }); + + it('prunes a member the replacement dropped and announces the new count', async () => { + const { items } = renderSelectableRoot(); + await ctx.nextFrame(); + click(items[0]); + click(items[1], { ctrlKey: true }); + announceSpy.mockClear(); + + const fresh = replaceRows(items, (item) => item !== items[1]); + await ctx.nextFrame(); + + expect(fresh.filter(isSelected)).toEqual([fresh[0]]); + expect(announcedMessages()).toEqual([ + ['[selected:1]', { politeness: 'polite' }], + ]); + }); + }); + describe('reconciling the batch after a morph', () => { it('reapplies the batch presentation after a morph strips it', async () => { const { root, items } = renderSelectableRoot(); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index 052959e35a14..6458d539bbe9 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -98,6 +98,7 @@ export default class SortableListsController extends Controller imp private monitorCleanupFn?:CleanupFn; private healScheduled = false; + private reconcileScheduled = false; connect():void { this.monitorCleanupFn = monitorForElements({ @@ -276,10 +277,29 @@ export default class SortableListsController extends Controller imp sortableListsItemOutletConnected(item:RootAwareChild):void { item.connectRoot(this); + this.scheduleSelectionReconcile(); } sortableListsItemOutletDisconnected(item:RootAwareChild):void { item.disconnectRoot(); + this.scheduleSelectionReconcile(); + } + + // Rows can be replaced without a morph — a frame navigation such as the + // "show more" expander swaps them wholesale — and the fresh rows carry no + // marker. Outlet callbacks arrive after the whole mutation batch, so one + // microtask later the members that survived are present to be repainted + // and the ones that vanished can be pruned. + private scheduleSelectionReconcile():void { + if (this.reconcileScheduled) { + return; + } + + this.reconcileScheduled = true; + queueMicrotask(() => { + this.reconcileScheduled = false; + this.selection?.reconcile(); + }); } sortableListsScrollableOutletConnected(scrollable:RootAwareChild):void { diff --git a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb index 371bbf45cdae..c642412aef60 100644 --- a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb +++ b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb @@ -242,6 +242,31 @@ # Without this, the two above would pass with every card described. backlogs_page.expect_work_package_card_not_described(story3) end + + # The expander is a frame navigation, not a morph: it swaps the inbox rows + # wholesale. The selection made before it must be back on the fresh rows, + # not stranded in the model while the cards paint as unselected. + context "with a truncated inbox" do + let!(:inbox_wps) { create_list(:work_package, 5, project:, type:) } + + # The constant has to be stubbed before the page renders, so the visit + # from the outer hook is repeated here. + before do + stub_const("Backlogs::InboxComponent::TRUNCATE_MIDDLE", 2) + backlogs_page.visit! + end + + it "keeps the selection across the show-more expander" do + backlogs_page.expect_inbox_show_more + backlogs_page.toggle_card(inbox_wps.first) + expect(backlogs_page.selected_card_ids).to eq([inbox_wps.first.id.to_s]) + + backlogs_page.click_inbox_show_more + backlogs_page.expect_no_inbox_show_more + + expect(backlogs_page.selected_card_ids).to eq([inbox_wps.first.id.to_s]) + end + end end # Selection consumes Space, the arrows, Home/End and Ctrl/Cmd+A. With every From 258f64ca1e58def05ac07598767908a4c0563df7 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Fri, 4 Sep 2026 18:37:07 +0100 Subject: [PATCH 17/22] Bound the downward item lookup to the row A row that wraps a nested list could resolve to the nested list's first item instead of nothing. resolveItemElement's downward fallback now checks only the row's own direct children, matching the bound already applied to its upward climb. --- .../dynamic/sortable-lists/list-dom.spec.ts | 24 +++++++++++++++++++ .../dynamic/sortable-lists/list-dom.ts | 13 ++++++---- .../dynamic/sortable-lists/selection.ts | 3 --- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts index 610257aa4305..da677c3b14f9 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts @@ -39,6 +39,7 @@ import { resolveItemPosition, resolveItemLabel, resolveItemType, + resolveItemElement, restoreRowPositions, rowOf, rowsRemainAt, @@ -116,6 +117,29 @@ describe('sortable lists DOM helpers', () => { }); }); + describe('resolveItemElement', () => { + it('does not descend into a nested list for a wrapper row', () => { + const list = listElement(); + const wrapper = document.createElement('li'); + const nested = listElement(); + nested.append(itemRow('9')); + wrapper.append(nested); + list.append(wrapper); + + expect(resolveItemElement(wrapper, list)).toBeNull(); + }); + + it('resolves the item a row wraps directly', () => { + const list = listElement(); + const wrapper = document.createElement('li'); + const item = itemRow('1'); + wrapper.append(item); + list.append(wrapper); + + expect(resolveItemElement(wrapper, list)).toBe(item); + }); + }); + describe('reorderRows', () => { it('moves a row to sit immediately after the previous item anchor', () => { const list = listElement(); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts index 0305fa2f82b6..1ef732f7e504 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -132,9 +132,7 @@ export function resolveItemType(element:Element):string|null { // wrong list entirely — which is exactly what a non-item marker row (e.g. an // empty list's placeholder) would otherwise resolve to instead of "no item // here". `boundary.contains(match)` accepts a self-or-ancestor match found -// inside the rows container and rejects one outside it. Only this ancestor -// climb is bounded — resolveItemElement's querySelector fallback below -// descends unbounded and can match an item belonging to a nested inner list. +// inside the rows container and rejects one outside it. export function resolveClosestItemElement(element:Element, boundary:Element):HTMLElement|null { if (!(element instanceof HTMLElement)) { return null; @@ -144,9 +142,14 @@ export function resolveClosestItemElement(element:Element, boundary:Element):HTM return match && boundary.contains(match) ? match : null; } +// The upward climb is bounded by `boundary`; the downward fallback only +// looks at the row's own direct children, so a wrapper row cannot resolve +// to an item of a list nested inside it. export function resolveItemElement(element:Element, boundary:Element):HTMLElement|null { - return resolveClosestItemElement(element, boundary) ?? - element.querySelector(sortableItemSelector); + return resolveClosestItemElement(element, boundary) + ?? Array.from(element.children).find((child):child is HTMLElement => ( + child instanceof HTMLElement && child.matches(sortableItemSelector) + )) ?? null; } export function resolvePreviousItemId(element:Element, boundary:Element):string|null { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts index e284002367c7..ee3ef3044692 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts @@ -83,9 +83,6 @@ function ownerList(root:HTMLElement, itemElement:HTMLElement):HTMLElement|null { return list && ownsElement(root, list) ? list : null; } -// The boundary bounds only the upward walk; resolveItemElement's downward -// fallback would hand a structural row the first item of a list nested -// inside it, so the resolved item must also belong to this list. function rowItem(row:Element, rowsContainer:Element, root:HTMLElement, list:HTMLElement):HTMLElement|null { const item = resolveItemElement(row, rowsContainer); From 7b6ae7ae79826c250ce29c126eef1dda72deb647 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 9 Sep 2026 13:17:25 +0100 Subject: [PATCH 18/22] Preserve selection during ranges Keeps independently selected items while Shift gestures extend, shrink or reverse a range. Resets the range baseline on individual selection changes and removes stale members during reconciliation. Allows Shift ranges to narrow a selection produced by Select All. Covers shared state, mouse/keyboard parity, same-list preservation and baseline pruning after DOM changes. Makes the feature scenario detect an incorrect anchor after deselection. --- frontend/src/common/batch-selection.spec.ts | 85 +++++++++++++++++- frontend/src/common/batch-selection.ts | 24 ++++-- .../selection-orchestrator.spec.ts | 86 +++++++++++++++++-- .../work_packages/batch_selection_spec.rb | 8 +- 4 files changed, 184 insertions(+), 19 deletions(-) diff --git a/frontend/src/common/batch-selection.spec.ts b/frontend/src/common/batch-selection.spec.ts index ea1c6277f428..54d785fbc865 100644 --- a/frontend/src/common/batch-selection.spec.ts +++ b/frontend/src/common/batch-selection.spec.ts @@ -66,7 +66,7 @@ describe('BatchSelection', () => { expect(selection.anchor).toEqual({ type: 'work_package', id: '1', listKey: 'sprint:7' }); }); - it('replaces the batch with a range and preserves the anchor', () => { + it('resizes a range and preserves the anchor', () => { selection.replace({ type: 'work_package', id: '1' }, 'sprint:7'); selection.range([{ type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }, { type: 'work_package', id: '3' }]); selection.range([{ type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }]); @@ -75,6 +75,89 @@ describe('BatchSelection', () => { expect(selection.anchor).toEqual({ type: 'work_package', id: '1', listKey: 'sprint:7' }); }); + describe('range sessions', () => { + const wp = (id:string) => ({ type: 'work_package', id }); + const ids = () => selection.items().map((entry) => entry.id).sort(); + + beforeEach(() => { + selection.replace(wp('1'), 'sprint:7'); + selection.toggle(wp('3'), 'sprint:7'); + }); + + it('preserves independent selections while extending, shrinking and reversing', () => { + selection.range(['3', '4', '5'].map(wp)); + expect(ids()).toEqual(['1', '3', '4', '5']); + selection.range(['3', '4'].map(wp)); + expect(ids()).toEqual(['1', '3', '4']); + selection.range(['2', '3'].map(wp)); + expect(ids()).toEqual(['1', '2', '3']); + }); + + it('preserves baseline members even when they overlap a previous range', () => { + selection.range(['1', '2', '3'].map(wp)); + selection.range(['3', '4'].map(wp)); + expect(ids()).toEqual(['1', '3', '4']); + }); + + it('starts a new baseline after toggling off the anchor', () => { + selection.range(['3', '4'].map(wp)); + selection.toggle(wp('3'), 'sprint:7'); + selection.range(['2', '3'].map(wp)); + expect(ids()).toEqual(['1', '2', '3', '4']); + expect(selection.anchor?.id).toBe('3'); + }); + + it.each(['replace', 'clear', 'selectAll'] as const)('ends the session on %s', (operation) => { + selection.range(['3', '4'].map(wp)); + if (operation === 'replace') { + selection.replace(wp('5'), 'sprint:7'); + } else if (operation === 'selectAll') { + selection.selectAll([wp('5')], { ...wp('5'), listKey: 'sprint:7' }); + } else { + selection.clear(); + } + selection.range(['5', '6'].map(wp)); + expect(ids()).toEqual(['5', '6']); + }); + + it('narrows and reverses a selection made with select-all', () => { + selection.selectAll(['1', '2', '3', '4', '5'].map(wp), { ...wp('3'), listKey: 'sprint:7' }); + selection.range(['3', '4'].map(wp)); + expect(ids()).toEqual(['3', '4']); + selection.range(['2', '3'].map(wp)); + expect(ids()).toEqual(['2', '3']); + }); + + it('starts a fresh baseline when toggling after select-all', () => { + selection.selectAll(['1', '2', '3', '4'].map(wp), { ...wp('3'), listKey: 'sprint:7' }); + selection.toggle(wp('2'), 'sprint:7'); + selection.range(['2', '3'].map(wp)); + expect(ids()).toEqual(['1', '2', '3', '4']); + }); + + it('does not restore pruned baseline members', () => { + selection.range(['3', '4'].map(wp)); + selection.prune(new Set(['3', '4', '5'].map((id) => selectionKey(wp(id))))); + selection.range(['3', '5'].map(wp)); + expect(ids()).toEqual(['3', '5']); + }); + + it('keeps the session through unchanged reconciliation', () => { + selection.range(['3', '4', '5'].map(wp)); + selection.prune(new Set(['1', '3', '4', '5'].map((id) => selectionKey(wp(id))))); + selection.rebindAnchor('sprint:7'); + selection.range(['3', '4'].map(wp)); + expect(ids()).toEqual(['1', '3', '4']); + }); + + it('ends the session when the anchor moves to another list', () => { + selection.range(['3', '4'].map(wp)); + selection.rebindAnchor('sprint:8'); + selection.range(['3', '5'].map(wp)); + expect(ids()).toEqual(['1', '3', '4', '5']); + }); + }); + it('selects all with an explicit anchor', () => { selection.selectAll([{ type: 'work_package', id: '3' }, { type: 'work_package', id: '1' }, { type: 'work_package', id: '2' }], { type: 'work_package', id: '2', listKey: 'sprint:7' }); diff --git a/frontend/src/common/batch-selection.ts b/frontend/src/common/batch-selection.ts index 0605f471f1d8..a24165679839 100644 --- a/frontend/src/common/batch-selection.ts +++ b/frontend/src/common/batch-selection.ts @@ -62,16 +62,13 @@ export interface SelectionAnchor extends SelectionItem { } /** - * Batch selection state and anchor semantics, free of any framework. - * - * Stores membership and an anchor, and nothing else. Visual order belongs to - * the live document, and range feasibility is resolved outside: callers hand - * in an already resolved range. Whether the members agree on a type is - * likewise their caller's policy. + * Visual order belongs to the live document. Callers resolve ranges and + * determine whether their members may belong to the same batch. */ export class BatchSelection { private selectedItems = new Map(); private selectionAnchor:SelectionAnchor|null = null; + private rangeBaseline:Map|null = null; // Keys, for membership tests and presentation matching. get keys():ReadonlySet { @@ -96,6 +93,7 @@ export class BatchSelection { } replace(item:SelectionItem, listKey:string):void { + this.rangeBaseline = null; this.selectedItems = new Map([[selectionKey(item), item]]); this.selectionAnchor = { ...item, listKey }; } @@ -103,6 +101,7 @@ export class BatchSelection { // Re-bases the anchor even when the toggle deselects: the user's last // touched card is where they expect the next range to start from. toggle(item:SelectionItem, listKey:string):void { + this.rangeBaseline = null; const key = selectionKey(item); if (this.selectedItems.has(key)) { @@ -117,15 +116,21 @@ export class BatchSelection { // The anchor stays put so repeated Shift gestures resize one range rather // than walking it across the list. range(rangeItems:readonly SelectionItem[]):void { - this.selectedItems = new Map(rangeItems.map((item) => [selectionKey(item), item])); + this.rangeBaseline ??= new Map(this.selectedItems); + this.selectedItems = new Map(this.rangeBaseline); + for (const item of rangeItems) { + this.selectedItems.set(selectionKey(item), item); + } } selectAll(items:readonly SelectionItem[], anchor:SelectionAnchor|null):void { + this.rangeBaseline = new Map(); this.selectedItems = new Map(items.map((item) => [selectionKey(item), item])); this.selectionAnchor = anchor; } clear():void { + this.rangeBaseline = null; this.selectedItems = new Map(); this.selectionAnchor = null; } @@ -138,7 +143,8 @@ export class BatchSelection { * live document and hands it back. */ rebindAnchor(listKey:string):void { - if (this.selectionAnchor) { + if (this.selectionAnchor && this.selectionAnchor.listKey !== listKey) { + this.rangeBaseline = null; this.selectionAnchor = { ...this.selectionAnchor, listKey }; } } @@ -158,12 +164,14 @@ export class BatchSelection { for (const key of [...this.selectedItems.keys()]) { if (!liveKeys.has(key)) { this.selectedItems.delete(key); + this.rangeBaseline?.delete(key); changed = true; } } if (this.selectionAnchor && !liveKeys.has(selectionKey(this.selectionAnchor))) { this.selectionAnchor = null; + this.rangeBaseline = null; changed = true; } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index 59c664fc8bc0..756f3ca95bba 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -142,6 +142,12 @@ describe('SelectionOrchestrator', () => { return event; }; + const keydownOn = (element:HTMLElement, key:string, init:KeyboardEventInit = {}) => { + const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true, ...init }); + Object.defineProperty(event, 'target', { value: element }); + return event; + }; + it('selects the clicked card without any Stimulus wiring', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); @@ -160,6 +166,80 @@ describe('SelectionOrchestrator', () => { expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); }); + it.each(['mouse', 'keyboard'] as const)('preserves independent selection when resizing with %s', (input) => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + pretendPlatform('macOS'); + const gesture = (id:string, shiftKey = false) => { + if (input === 'mouse') { + orchestrator.handleClick(clickOn(item(id), { metaKey: !shiftKey, shiftKey })); + } else { + orchestrator.handleKeydown(keydownOn(item(id), ' ', { metaKey: !shiftKey, shiftKey })); + } + }; + + gesture('4'); + gesture('2'); + gesture('3', true); + expect(orchestrator.selectedIds()).toEqual(['2', '3', '4']); + expect(isSelected(item('4'))).toBe(true); + + orchestrator.reconcile(); + gesture('2', true); + expect(orchestrator.selectedIds()).toEqual(['2', '4']); + expect(isSelected(item('3'))).toBe(false); + + gesture('1', true); + expect(orchestrator.selectedIds()).toEqual(['1', '2', '4']); + }); + + it.each(['mouse', 'keyboard'] as const)('narrows select-all with %s while replacing picks outside the list', (input) => { + const orchestrator = new SelectionOrchestrator(hostFor(root)); + orchestrator.handleClick(clickOn(item('4'), { ctrlKey: true })); + orchestrator.handleKeydown(keydownOn(item('2'), 'a', { ctrlKey: true })); + expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + + if (input === 'mouse') { + orchestrator.handleClick(clickOn(item('3'), { shiftKey: true })); + } else { + orchestrator.handleKeydown(keydownOn(item('3'), ' ', { shiftKey: true })); + } + expect(orchestrator.selectedIds()).toEqual(['2', '3']); + expect(isSelected(item('1'))).toBe(false); + expect(isSelected(item('4'))).toBe(false); + }); + + it.each(['mouse', 'keyboard'] as const)('preserves same-list picks and prunes a removed baseline card with %s', (input) => { + item('3').after(item('4'), item('5')); + const orchestrator = new SelectionOrchestrator(hostFor(root)); + const gesture = (id:string, shiftKey = false) => { + if (input === 'mouse') { + orchestrator.handleClick(clickOn(item(id), { ctrlKey: !shiftKey, shiftKey })); + } else { + orchestrator.handleKeydown(keydownOn(item(id), ' ', { ctrlKey: !shiftKey, shiftKey })); + } + }; + + gesture('1'); + gesture('3'); + gesture('5', true); + expect(orchestrator.selectedIds()).toEqual(['1', '3', '4', '5']); + gesture('4', true); + expect(orchestrator.selectedIds()).toEqual(['1', '3', '4']); + gesture('2', true); + expect(orchestrator.selectedIds()).toEqual(['1', '2', '3']); + + const removed = item('1'); + removed.remove(); + orchestrator.reconcile(); + gesture('4', true); + expect(orchestrator.selectedIds()).toEqual(['3', '4']); + item('2').before(removed); + orchestrator.reconcile(); + gesture('5', true); + expect(orchestrator.selectedIds()).toEqual(['3', '4', '5']); + expect(isSelected(removed)).toBe(false); + }); + it('routes focus through the host rather than touching the element', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); const event = new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true, cancelable: true }); @@ -316,12 +396,6 @@ describe('SelectionOrchestrator', () => { }); describe('keyboard and pointer edge cases', () => { - const keydownOn = (element:HTMLElement, key:string, init:KeyboardEventInit = {}) => { - const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true, ...init }); - Object.defineProperty(event, 'target', { value: element }); - return event; - }; - it('still moves focus with the arrows while a move is in flight', () => { const orchestrator = new SelectionOrchestrator(hostFor(root)); busy = true; diff --git a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb index c642412aef60..b03959102b89 100644 --- a/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb +++ b/modules/backlogs/spec/features/work_packages/batch_selection_spec.rb @@ -95,18 +95,18 @@ end # BatchSelection#toggle re-bases the anchor even on a deselect, so a - # later range starts from story2 rather than story1. + # later range starts from story3 rather than story1. it "re-bases the anchor to a toggled card even when the toggle deselects it" do backlogs_page.select_card(story1) - backlogs_page.toggle_card(story2) - backlogs_page.toggle_card(story2) + backlogs_page.toggle_card(story3) + backlogs_page.toggle_card(story3) expect(page).to have_css("[data-batch-selected]", count: 1) backlogs_page.extend_selection_to(story4) expect(page).to have_css("[data-batch-selected]", count: 3) - expect(backlogs_page.selected_card_ids).to eq([story2, story3, story4].map { it.id.to_s }) + expect(backlogs_page.selected_card_ids).to eq([story1, story3, story4].map { it.id.to_s }) end it "resizes one fixed-anchor range rather than walking it" do From 100437963515f546ad3cb19970792a26b03de447 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 9 Sep 2026 13:51:33 +0100 Subject: [PATCH 19/22] Skip wait_for_reload in visit! outside Cuprite Every Selenium spec that visits through a page object warned about the Cuprite-only helper on each call, since the base class waited without checking the driver. Selenium's visit already blocks on the load event, so `visit!` now guards the wait the same way `reload!` does. --- spec/support/pages/page.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/pages/page.rb b/spec/support/pages/page.rb index 06fe051d8ff1..2fa6fe024acd 100644 --- a/spec/support/pages/page.rb +++ b/spec/support/pages/page.rb @@ -53,7 +53,7 @@ def visit! visit(path) - wait_for_reload + wait_for_reload if using_cuprite? end def reload! From 0fa15c20c8c853cb621c21d623cb98c76646deb8 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 9 Sep 2026 13:56:11 +0100 Subject: [PATCH 20/22] Ignore Ctrl-click on Apple whatever else is held The guard only stepped aside for a bare Ctrl-click, so Ctrl+Shift-click reached the range path although macOS still treats it as the secondary click. Ctrl now bows out on Apple platforms regardless of the other modifiers held with it. --- .../sortable-lists/selection-orchestrator.spec.ts | 13 +++++++++---- .../sortable-lists/selection-orchestrator.ts | 6 +++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts index 756f3ca95bba..446c606916be 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.spec.ts @@ -597,14 +597,19 @@ describe('SelectionOrchestrator', () => { expect(announceSpy.mock.calls.map((call) => call[0])).toEqual(['[range_blocked]']); }); - // Ctrl-click opens the contextual menu on Apple platforms: it must - // neither toggle nor fall through to the ordinary-click path. - it('ignores Ctrl-click entirely on Apple platforms', () => { + // Ctrl-click opens the contextual menu on Apple platforms whatever else + // is held: it must neither toggle, nor range, nor fall through to the + // ordinary-click path. + it.each([ + ['on its own', {}], + ['with Shift', { shiftKey: true }], + ['with Cmd', { metaKey: true }], + ])('ignores Ctrl-click %s on Apple platforms', (_label, extra) => { pretendPlatform('macOS'); const orchestrator = new SelectionOrchestrator(hostFor(root)); orchestrator.handleClick(clickOn(item('1'))); - const event = clickOn(item('2'), { ctrlKey: true }); + const event = clickOn(item('2'), { ctrlKey: true, ...extra }); orchestrator.handleClick(event); expect(orchestrator.selectedIds()).toEqual(['1']); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts index b354f224d642..bdf75f529dfe 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection-orchestrator.ts @@ -122,9 +122,9 @@ export class SelectionOrchestrator { } readonly handleClick = (event:MouseEvent):void => { - // Ctrl-click is the secondary click on Apple platforms, where it opens - // the contextual menu and Cmd is the multi-select key instead. - if (event.ctrlKey && !event.metaKey && !event.shiftKey && isApplePlatform()) { + // Ctrl-click is the secondary click on Apple platforms whatever else is + // held, so it opens the contextual menu; Cmd is the multi-select key. + if (event.ctrlKey && isApplePlatform()) { return; } From 23a8af347a2c6d4a2b55d01eba9e4685db0d413c Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Wed, 9 Sep 2026 13:56:11 +0100 Subject: [PATCH 21/22] Document that toggles may span lists The note called selecting across lists a deliberate gap, yet Ctrl/Cmd click toggles do build a sparse batch across lists; only ranges and select-all are list-confined. Spelled out so a future consumer does not remove or work around supported behavior. --- frontend/AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index e40327563ee6..35d115f452a3 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -7,7 +7,7 @@ - `./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 -- `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list; selecting across lists is a deliberate gap, reserved for a separate mechanism. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Batch movement is not implemented: a drag still moves one card and collapses any wider selection onto it — that's a later work package. +- `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list, and a Shift gesture into another list restarts the selection there; Ctrl/Cmd+click toggles are not confined, so a sparse batch spanning lists is supported behavior. Broader select-all scopes and the cross-list restart policy are open decisions in AGILE-181. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Batch movement is not implemented: a drag still moves one card and collapses any wider selection onto it — that's a later work package. - `data-batch-selected` is written on the sortable item element — the row, in Backlogs — while `aria-current` is written on the card inside it. A stylesheet assuming both live on the same element will silently paint nothing while attribute assertions stay green. ## Configuration Files From 7f0295cfaf187531e50c4c66a659b843dbbb3e6f Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Thu, 10 Sep 2026 16:05:32 +0100 Subject: [PATCH 22/22] Move selection notes beside sortable lists Keeps component behavior out of the frontend-wide agent instructions. Places the durable selection contract beside its implementation and removes status notes that become stale as the stack progresses. --- frontend/AGENTS.md | 3 +- .../dynamic/sortable-lists/README.md | 73 +++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index 35d115f452a3..202d9b389869 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -7,8 +7,7 @@ - `./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 -- `sortable-lists` batch selection is opt-in: a root enables it with a `selectionEnabled` value, and no other consumer's behavior changes. A root also sets `announcementScope`, so the shared controller's announcements speak the consumer's vocabulary instead of "item", and `selectionDescriptionId`, pointing at one shared element every selected card references via `aria-describedby`. Items declare `mobility` — `fixed`, `confined` or `free` — which gates dragging, selection eligibility, and positional moves alike. A missing value means `free`, so a consumer that renders none keeps working; an unrecognised one falls closed to `fixed` rather than handing the user controls the server will refuse. The pure selection model lives in `./src/common/batch-selection.ts` (framework-agnostic, so Angular consumers can adopt it); the DOM-facing adapter is `sortable-lists/selection.ts`, and gesture interpretation sits behind `sortable-lists/selection-orchestrator.ts`, which takes a narrow host port and imports no Stimulus. Selection identity is `(type, id)`, never the id alone: ids are unique per source table, so a nested list of another type can hold a colliding one. A root must render exactly one instance of each `(type, id)`, and an item declaring no type is refused as a candidate. A batch holds one item type — that cohort rule is orchestrator policy, not a constraint of the model, since identity namespacing and batch compatibility are different concerns. Ranges and select-all (Ctrl/Cmd+A) are both confined to the focused card's list, and a Shift gesture into another list restarts the selection there; Ctrl/Cmd+click toggles are not confined, so a sparse batch spanning lists is supported behavior. Broader select-all scopes and the cross-list restart policy are open decisions in AGILE-181. An item belongs to its nearest ancestor root, so an independently nested root is an ownership boundary. Batch movement is not implemented: a drag still moves one card and collapses any wider selection onto it — that's a later work package. -- `data-batch-selected` is written on the sortable item element — the row, in Backlogs — while `aria-current` is written on the card inside it. A stylesheet assuming both live on the same element will silently paint nothing while attribute assertions stay green. +- [Sortable lists selection contract](src/stimulus/controllers/dynamic/sortable-lists/README.md) - Consumer wiring, item identity, selection gestures, and presentation. ## Configuration Files diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md b/frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md new file mode 100644 index 000000000000..713c58b2432f --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md @@ -0,0 +1,73 @@ +# Sortable lists selection + +## Consumer wiring + +Batch selection is opt-in per sortable root through `selectionEnabled`. The root +sets `announcementScope` for its translation vocabulary and +`selectionDescriptionId` for one shared description element. Selected items +reference that description through `aria-describedby` on their focus host. + +Items declare `mobility`: `fixed`, `confined`, or `free`. Mobility gates dragging, +selection eligibility, and positional moves. A missing value defaults to `free`; +an unrecognised value resolves to `fixed`. Structural rows such as “Show more” +are not sortable items and do not participate in selection. + +## Model and ownership + +- [BatchSelection](../../../../common/batch-selection.ts) stores membership, + the anchor, and the baseline preserved while resizing a range. It has no + framework or DOM dependency. +- [selection.ts](selection.ts) resolves candidates, ranges, live document order, + and presentation. +- [selection-orchestrator.ts](selection-orchestrator.ts) interprets gestures + through a narrow host interface and imports no Stimulus. + +Selection identity is `(type, id)`. Each root must render exactly one instance +of each pair, and an item without a type is not a selection candidate. The +orchestrator permits one item type per batch; the model itself does not impose +that compatibility policy. + +An item belongs to its nearest ancestor sortable root. Independently nested +roots are ownership boundaries. Item lookup, focus targets, and range resolution +must respect the owning item and list. + +## Gestures and range sessions + +Ctrl/Cmd-click toggles individual items and can build a sparse batch across +lists. The toggled item becomes the anchor even when deselected. Plain click +replaces the selection with the movable item while allowing its activation. + +Shift-click, Shift+Space, and Shift+Arrow resize a range within the anchor's +list. Independent selections present when the range starts remain selected, +including selections in other lists. Repeated Shift gestures extend, shrink, +or reverse only the range's contribution. Without an anchor, or when Shift +crosses into another list, selection restarts at the acted-on item. Ranges +crossing unloaded or non-movable items are rejected without changing selection. + +Ctrl/Cmd+A replaces the batch with loaded movable items of the focused item's +type in its list. A subsequent Shift gesture can narrow or reverse that +selection. An individual toggle starts a fresh range baseline. + +Space toggles the focused item. Arrows move focus between items, including fixed +items; Home/End target the first/last movable item. Focus movement remains +available during a move, but selection mutations are blocked. Escape clears +selection and the anchor at document level, while respecting fields and overlays +that own the key. + +Reconciliation prunes unavailable items and rebinds the anchor to its live list. +Losing the anchor or moving it to another list ends the range session; unchanged +reconciliation preserves it. + +## Presentation and feedback + +`data-batch-selected` belongs on the sortable item element. In Backlogs this is +the row, while `aria-current` belongs on the card inside it and represents the +work package open in the details pane. Styles must account for those distinct +elements and states. Updating the current work package does not change batch +membership. + +Selection gestures announce membership changes, including changes that retain +the same count. Plain navigation clicks announce only when a wider batch +collapses. Rejected ranges and range restarts have distinct translation keys. +Tests use [keyed translation fixtures](testing/selection-translations.ts) to +assert the selected key and plural form without duplicating production copy.