From abe468c59bf3fff5d76da379e0cb683b74accb28 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Fri, 2 Oct 2026 23:27:05 +0100 Subject: [PATCH 1/4] Pin the drop over a batch-mate's row Characterises how resolveDropIntent treats a batch released over the row of another selected member when no item target is sticky: the source-row guard matches the dragged item only, so the release lands as a list-only drop. Documents current behaviour ahead of the drag session extraction; the product decision stays open. --- .../dynamic/sortable-lists.controller.spec.ts | 17 +++++++++++++++++ 1 file changed, 17 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 32800c48ee95..7bc58eda8ab7 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -2476,6 +2476,23 @@ describe('Sortable lists controller', () => { await flushPromises(); } + // Characterises today's behaviour, not a verdict on it. The "still over + // the source row" guard in resolveDropIntent compares the row under the + // pointer with the dragged item alone, so a release over a batch-mate's + // row with no sticky item target reads as a list-only drop and sends the + // block to the list's configured drop position. Whether a mate's row + // should count as the source row is an open product decision; flip the + // expectations when it is taken. + it('treats a release over a batch-mate\'s row as a list-only drop', async () => { + selectItems(item1, item2); + vi.spyOn(document, 'elementsFromPoint').mockReturnValue([item2]); + + await simulateDrop({ source: item1, targetList: list1, targetItem: null, edge: null }); + + expect(rowIdsIn(list1)).toEqual(['3', '1', '2']); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + // Item drop targets ask on every dragover; the owner is settled for the // drag at its start and forgotten with the frozen batch. describe('ownerDestinationOf', () => { From f35e8e0d5a3613bc39a26f8e5ff61048513b830d Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Fri, 2 Oct 2026 23:29:07 +0100 Subject: [PATCH 2/4] Add a drag session for sortable lists Introduces DragSession, one object for the state of one drag: prospective members resolved once, permitted destinations and owner memo, a batch frozen at most once, data-dragging marks set on start and cleared on a single end. Nothing uses it yet; the root and item controllers adopt it next. Moves itemElementsByKey into selection.ts so the session and the root share it. --- .../sortable-lists/drag-session.spec.ts | 264 ++++++++++++++++++ .../dynamic/sortable-lists/drag-session.ts | 193 +++++++++++++ .../dynamic/sortable-lists/selection.ts | 13 + 3 files changed, 470 insertions(+) create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.spec.ts create mode 100644 frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.spec.ts new file mode 100644 index 000000000000..481ff0658c2d --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.spec.ts @@ -0,0 +1,264 @@ +//-- copyright +// OpenProject is an open source project management software. +// Copyright (C) the OpenProject GmbH +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License version 3. +// +// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +// Copyright (C) 2006-2013 Jean-Philippe Lang +// Copyright (C) 2010-2013 the ChiliProject Team +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License +// as published by the Free Software Foundation; either version 2 +// of the License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program; if not, write to the Free Software +// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { type SelectionItem } from 'core-common/batch-selection'; +import { DragSession, type DragSessionHost, permittedDestinationsFor } from './drag-session'; +import { type DestinationIdentity } from './list-dom'; + +describe('sortable-lists drag session', () => { + let root:HTMLElement; + let items:HTMLElement[]; + const list1:DestinationIdentity = { type: 'work_package', id: '1' }; + const list2:DestinationIdentity = { type: 'work_package', id: '2' }; + + function item(id:string, mobility = 'free'):HTMLElement { + const element = document.createElement('li'); + element.setAttribute('data-sortable-lists--item-id-value', id); + element.setAttribute('data-sortable-lists--item-type-value', 'work_package'); + element.setAttribute('data-sortable-lists--item-mobility-value', mobility); + return element; + } + + function identity(element:HTMLElement):SelectionItem { + return { type: 'work_package', id: element.getAttribute('data-sortable-lists--item-id-value')! }; + } + + function host(overrides:Partial = {}):DragSessionHost { + return { + rootElement: root, + maxBatchSize: 0, + prospectiveMembers: vi.fn((element:HTMLElement) => [element]), + frozenMembers: vi.fn((element:HTMLElement) => [identity(element)]), + ownedDestinations: vi.fn(() => [list1, list2]), + liveOwnerDestinationOf: vi.fn(() => list1), + ...overrides, + }; + } + + beforeEach(() => { + root = document.createElement('div'); + root.setAttribute('data-controller', 'sortable-lists'); + items = [item('1'), item('2'), item('3', 'confined')]; + root.append(...items); + document.body.append(root); + }); + + afterEach(() => { + document.body.replaceChildren(); + }); + + describe('prospective phase', () => { + it('resolves the members once, in the constructor', () => { + const prospectiveMembers = vi.fn(() => [items[0], items[1]]); + const session = new DragSession(host({ prospectiveMembers }), items[0]); + + expect(session.phase).toBe('prospective'); + expect(session.members).toEqual([items[0], items[1]]); + expect(session.size).toBe(2); + expect(prospectiveMembers).toHaveBeenCalledTimes(1); + }); + + it('is refused only above a positive cap', () => { + const prospectiveMembers = () => [items[0], items[1]]; + + expect(new DragSession(host({ prospectiveMembers, maxBatchSize: 0 }), items[0]).refused).toBe(false); + expect(new DragSession(host({ prospectiveMembers, maxBatchSize: 2 }), items[0]).refused).toBe(false); + expect(new DragSession(host({ prospectiveMembers, maxBatchSize: 1 }), items[0]).refused).toBe(true); + }); + + it('mutates nothing before freeze', () => { + const frozenMembers = vi.fn(() => null); + const session = new DragSession(host({ frozenMembers }), items[0]); + + session.permittedDestinations(); + session.ownerDestinationOf(items[1]); + + expect(frozenMembers).not.toHaveBeenCalled(); + expect(root.querySelector('[data-dragging]')).toBeNull(); + }); + }); + + describe('permitted destinations', () => { + it('is null while every member reaches every owned list', () => { + const session = new DragSession(host({ prospectiveMembers: () => [items[0], items[1]] }), items[0]); + + expect(session.permittedDestinations()).toBeNull(); + }); + + it('is pinned to the list a confined member sits in', () => { + const session = new DragSession(host({ prospectiveMembers: () => [items[0], items[2]] }), items[0]); + + expect(session.permittedDestinations()).toEqual([list1]); + }); + + it('is computed once per session', () => { + const ownedDestinations = vi.fn(() => [list1, list2]); + const session = new DragSession(host({ ownedDestinations }), items[0]); + + session.permittedDestinations(); + session.permittedDestinations(); + + expect(ownedDestinations).toHaveBeenCalledTimes(1); + }); + + it('permittedDestinationsFor returns the empty set when confined members disagree', () => { + const other = item('4', 'confined'); + const owners = new Map([[items[2], list1], [other, list2]]); + + expect(permittedDestinationsFor([items[2], other], [list1, list2], (element) => owners.get(element) ?? null)).toEqual([]); + }); + }); + + describe('owner memo', () => { + it('remembers an owner for the session', () => { + const liveOwnerDestinationOf = vi.fn(() => list1); + const session = new DragSession(host({ liveOwnerDestinationOf }), items[0]); + + expect(session.ownerDestinationOf(items[1])).toEqual(list1); + liveOwnerDestinationOf.mockReturnValue(list2); + expect(session.ownerDestinationOf(items[1])).toEqual(list1); + expect(liveOwnerDestinationOf).toHaveBeenCalledTimes(1); + }); + + it('remembers a missing owner too', () => { + const liveOwnerDestinationOf = vi.fn(() => null); + const session = new DragSession(host({ liveOwnerDestinationOf }), items[0]); + + session.ownerDestinationOf(items[1]); + session.ownerDestinationOf(items[1]); + + expect(liveOwnerDestinationOf).toHaveBeenCalledTimes(1); + }); + + it('re-reads owners after forgetOwners', () => { + const liveOwnerDestinationOf = vi.fn(() => list1); + const session = new DragSession(host({ liveOwnerDestinationOf }), items[0]); + + session.ownerDestinationOf(items[1]); + liveOwnerDestinationOf.mockReturnValue(list2); + session.forgetOwners(); + + expect(session.ownerDestinationOf(items[1])).toEqual(list2); + }); + }); + + describe('freeze and start', () => { + it('freezes once and reports the batch size', () => { + const frozenMembers = vi.fn(() => [identity(items[0]), identity(items[1])]); + const session = new DragSession(host({ frozenMembers }), items[0]); + + expect(session.freeze()).toBe(2); + expect(session.freeze()).toBe(2); + expect(session.phase).toBe('frozen'); + expect(frozenMembers).toHaveBeenCalledTimes(1); + }); + + it('marks every frozen member on start', () => { + const session = new DragSession(host({ frozenMembers: () => [identity(items[0]), identity(items[1])] }), items[0]); + + session.freeze(); + session.start(); + + expect(session.phase).toBe('started'); + expect(items[0]).toHaveAttribute('data-dragging', 'source'); + expect(items[1]).toHaveAttribute('data-dragging', 'source'); + expect(items[2]).not.toHaveAttribute('data-dragging'); + }); + + it('freezes first when started unfrozen', () => { + const frozenMembers = vi.fn(() => [identity(items[0])]); + const session = new DragSession(host({ frozenMembers }), items[0]); + + session.start(); + + expect(frozenMembers).toHaveBeenCalledTimes(1); + expect(session.phase).toBe('started'); + }); + + it('re-marks a stripped member on remark only once started', () => { + const session = new DragSession(host({ frozenMembers: () => [identity(items[0]), identity(items[1])] }), items[0]); + + session.freeze(); + session.remark(); + expect(items[1]).not.toHaveAttribute('data-dragging'); + + session.start(); + items[1].removeAttribute('data-dragging'); + session.remark(); + expect(items[1]).toHaveAttribute('data-dragging', 'source'); + }); + + it('marks a replacement element for the same identity', () => { + const session = new DragSession(host({ frozenMembers: () => [identity(items[0]), identity(items[1])] }), items[0]); + session.start(); + + const replacement = item('2'); + items[1].replaceWith(replacement); + session.remark(); + + expect(replacement).toHaveAttribute('data-dragging', 'source'); + }); + }); + + describe('without a batch contract', () => { + it('reports size one, marks the source only and ends with null', () => { + const session = new DragSession(host({ frozenMembers: () => null }), items[0]); + + expect(session.freeze()).toBe(1); + session.start(); + + expect(items[0]).toHaveAttribute('data-dragging', 'source'); + expect(root.querySelectorAll('[data-dragging]')).toHaveLength(1); + expect(session.end()).toBeNull(); + }); + }); + + describe('end', () => { + it('hands out the frozen batch once and clears every mark under the root', () => { + const batch = [identity(items[0]), identity(items[1])]; + const session = new DragSession(host({ frozenMembers: () => batch }), items[0]); + session.start(); + items[2].setAttribute('data-dragging', 'source'); + + expect(session.end()).toEqual(batch); + expect(session.phase).toBe('ended'); + expect(root.querySelector('[data-dragging]')).toBeNull(); + expect(session.end()).toBeNull(); + }); + + it('is idempotent on a session that never froze', () => { + const session = new DragSession(host(), items[0]); + + expect(session.end()).toBeNull(); + expect(session.phase).toBe('ended'); + session.start(); + expect(session.phase).toBe('ended'); + expect(root.querySelector('[data-dragging]')).toBeNull(); + }); + }); +}); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts new file mode 100644 index 000000000000..5b098cf7502c --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts @@ -0,0 +1,193 @@ +//-- 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 SelectionItem } from 'core-common/batch-selection'; +import { itemAcceptsDestination, type DestinationIdentity } from './list-dom'; +import { itemElementsByKey } from './selection'; + +export const draggingAttribute = 'data-dragging'; + +// What one drag needs from the root that hosts it. +export interface DragSessionHost { + readonly rootElement:HTMLElement; + // 0 means no cap. + readonly maxBatchSize:number; + // The items an action from this item applies to, without touching the + // selection. Never empty: the item itself when nothing wider applies. + prospectiveMembers(itemElement:HTMLElement):HTMLElement[]; + // Commits the batch: an unselected movable item becomes the selection + // first. Null when the root runs no selection, so the drop takes the + // single-item route. + frozenMembers(itemElement:HTMLElement):SelectionItem[]|null; + ownedDestinations():DestinationIdentity[]; + liveOwnerDestinationOf(element:HTMLElement):DestinationIdentity|null; +} + +export type DragSessionPhase = 'prospective'|'frozen'|'started'|'ended'; + +// The destinations every member accepts, or null when that is all of them. +// A batch may span lists, so a confined member pins the block to its own +// list, never to the dragged item's. +export function permittedDestinationsFor( + members:HTMLElement[], + destinations:DestinationIdentity[], + ownerDestinationOf:(item:HTMLElement) => DestinationIdentity|null, +):DestinationIdentity[]|null { + const permitted = destinations + .filter((destination) => members.every((member) => itemAcceptsDestination(member, destination, ownerDestinationOf))); + + return permitted.length === destinations.length ? null : permitted; +} + +/** + * One drag, from the first `canDrag` read to its single end. + * + * Pragmatic asks `canDrag`, then builds the payloads, then renders the + * preview, then starts the drag; the phases follow that order. Members are + * resolved once, in the constructor, and stay fixed; freezing happens at + * most once; `end()` hands the frozen batch out at most once. Rows are held + * as identities only, so a morph that replaces an element mid-drag cannot + * leave the session pointing at a node that left the document. + */ +export class DragSession { + readonly members:HTMLElement[]; + + private currentPhase:DragSessionPhase = 'prospective'; + private frozenBatch:SelectionItem[]|null = null; + private permitted:{ value:DestinationIdentity[]|null }|null = null; + private owners = new WeakMap(); + + constructor( + private readonly host:DragSessionHost, + readonly sourceElement:HTMLElement, + ) { + this.members = host.prospectiveMembers(sourceElement); + } + + get phase():DragSessionPhase { + return this.currentPhase; + } + + get size():number { + return this.members.length; + } + + get refused():boolean { + return this.host.maxBatchSize > 0 && this.members.length > this.host.maxBatchSize; + } + + permittedDestinations():DestinationIdentity[]|null { + this.permitted ??= { + value: permittedDestinationsFor( + this.members, + this.host.ownedDestinations(), + (item) => this.ownerDestinationOf(item), + ), + }; + + return this.permitted.value; + } + + // Every item drop target asks on each dragover, so the answer is kept for + // the drag; the root forgets it when a morph may have moved rows. + ownerDestinationOf(element:HTMLElement):DestinationIdentity|null { + const remembered = this.owners.get(element); + if (remembered !== undefined) { + return remembered; + } + + const destination = this.host.liveOwnerDestinationOf(element); + this.owners.set(element, destination); + return destination; + } + + forgetOwners():void { + this.owners = new WeakMap(); + } + + // Returns the batch size for the preview, which Pragmatic renders before + // the drag starts. + freeze():number { + if (this.currentPhase === 'prospective') { + this.frozenBatch = this.host.frozenMembers(this.sourceElement); + this.currentPhase = 'frozen'; + } + + return Math.max(1, this.frozenBatch?.length ?? 0); + } + + start():void { + if (this.currentPhase === 'started' || this.currentPhase === 'ended') { + return; + } + + this.freeze(); + this.currentPhase = 'started'; + this.markRows(); + } + + // A row a morph replaces mid-drag comes back as fresh server HTML without + // its mark. + remark():void { + if (this.currentPhase === 'started') { + this.markRows(); + } + } + + // Every mark under the root, not just the batch's own rows: a cancelled + // drop, or a row the item controller marked before it had a session, + // would otherwise leave one behind. + end():SelectionItem[]|null { + if (this.currentPhase === 'ended') { + return null; + } + + this.currentPhase = 'ended'; + this.host.rootElement + .querySelectorAll(`[${draggingAttribute}]`) + .forEach((element) => element.removeAttribute(draggingAttribute)); + + return this.frozenBatch; + } + + private markRows():void { + const elements = this.frozenBatch + ? this.frozenBatchElements() + : [this.sourceElement]; + + elements.forEach((element) => element.setAttribute(draggingAttribute, 'source')); + } + + private frozenBatchElements():HTMLElement[] { + const byKey = itemElementsByKey(this.host.rootElement); + + return (this.frozenBatch ?? []) + .map((item) => byKey.get(selectionKey(item))) + .filter((element):element is HTMLElement => element !== undefined); + } +} diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts index d0fd2afec31b..73e337543b74 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/selection.ts @@ -147,6 +147,19 @@ export function orderedSelectedItemElements(root:HTMLElement, keys:ReadonlySet { + const map = new Map(); + orderedItemElements(root).forEach((element) => { + const identity = itemIdentity(element); + if (identity) { + map.set(selectionKey(identity), element); + } + }); + return map; +} + export function liveOrderableItems(root:HTMLElement):SelectionItem[] { return orderedItemElements(root) .filter(isOrderableItem) From bca2a86412e377ca9b71e812fe823058b29209a2 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Fri, 2 Oct 2026 23:37:42 +0100 Subject: [PATCH 3/4] Route drag state through the drag session Replaces the root controller's frozen-batch field, owner memo and row-marking helpers with one DragSession. The item controller begins it in canDrag and reads it back through the root in every later callback; the root's monitor starts it on drag start and ends it once, from the drop monitor or disconnect, so an item controller replaced between preview and drag start cannot strand the batch. The root contract loses freezeDragBatch, markDragBatch, dragPermittedDestinations, dragRefused and externalDragItems and gains beginDrag and dragSession. Pragmatic checks the drag handle after canDrag, so a press outside the handle leaves a session that never froze; owner answers go live again until a session is frozen. Two deliberate changes: the owner memo is forgotten on a morph mid-drag so a reparented row answers with its live list, and canDrag runs the pointer check before the batch cap so a press on a button inside a card no longer announces an oversized batch. --- .../dynamic/sortable-lists.controller.spec.ts | 143 ++++++++--- .../dynamic/sortable-lists.controller.ts | 177 +++++-------- .../dynamic/sortable-lists/README.md | 7 +- .../dynamic/sortable-lists/drag-and-drop.ts | 23 +- .../dynamic/sortable-lists/drag-session.ts | 13 +- .../sortable-lists/item.controller.spec.ts | 238 +++++++++++------- .../dynamic/sortable-lists/item.controller.ts | 38 ++- .../sortable-lists/list.controller.spec.ts | 7 +- .../scrollable.controller.spec.ts | 7 +- 9 files changed, 371 insertions(+), 282 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 7bc58eda8ab7..a60152f3568e 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -63,6 +63,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 type { DragSession } from './sortable-lists/drag-session'; import { selectionTranslations } from './sortable-lists/testing/selection-translations'; import type { sortableItemData as sortableItemDataFn, @@ -1335,7 +1336,7 @@ describe('Sortable lists controller', () => { await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; - controller.freezeDragBatch(firstSourceItem); + controller.beginDrag(firstSourceItem).freeze(); expect(document.querySelectorAll('[data-batch-selected]')).toHaveLength(1); expect(firstSourceItem.hasAttribute('data-batch-selected')).toBe(true); @@ -1377,7 +1378,7 @@ describe('Sortable lists controller', () => { items[2].dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, ctrlKey: true })); announceSpy.mockClear(); - controller.freezeDragBatch(items[3]); + controller.beginDrag(items[3]).freeze(); expect(items.filter((item) => item.hasAttribute('data-batch-selected'))).toEqual([items[3]]); expect(announceSpy.mock.calls.map((call) => [call[0], call[1]])).toEqual([ @@ -2197,9 +2198,22 @@ describe('Sortable lists controller', () => { click(items[0]); click(items[1], { ctrlKey: true }); click(items[2], { ctrlKey: true }); announceSpy.mockClear(); - expect(controller.dragRefused(items[0])).toBe(true); + expect(controller.beginDrag(items[0]).refused).toBe(true); expect(announceSpy).toHaveBeenCalledWith('[batch_too_large:3:2]', { politeness: 'assertive' }); - expect(controller.dragRefused(items[4])).toBe(false); + expect(controller.beginDrag(items[4]).refused).toBe(false); + }); + + it('keeps no refused session and ends the one it replaces', async () => { + const { root, items } = renderSelectableRoot({ collectionMoveUrl: '/collection-move-url' }); + root.setAttribute('data-sortable-lists-max-batch-size-value', '2'); + await ctx.nextFrame(); + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; + const lingering = controller.beginDrag(items[4]); + click(items[0]); click(items[1], { ctrlKey: true }); click(items[2], { ctrlKey: true }); + + expect(controller.beginDrag(items[0]).refused).toBe(true); + expect(lingering.phase).toBe('ended'); + expect(controller.dragSession).toBeNull(); }); it('never refuses without a cap', async () => { @@ -2208,7 +2222,7 @@ describe('Sortable lists controller', () => { const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; click(items[0]); click(items[1], { ctrlKey: true }); click(items[2], { ctrlKey: true }); - expect(controller.dragRefused(items[0])).toBe(false); + expect(controller.beginDrag(items[0]).refused).toBe(false); }); it('does not refuse a batch exactly at the cap', async () => { @@ -2219,7 +2233,7 @@ describe('Sortable lists controller', () => { click(items[0]); click(items[1], { ctrlKey: true }); click(items[2], { ctrlKey: true }); announceSpy.mockClear(); - expect(controller.dragRefused(items[0])).toBe(false); + expect(controller.beginDrag(items[0]).refused).toBe(false); expect(announceSpy).not.toHaveBeenCalled(); }); }); @@ -2389,12 +2403,15 @@ describe('Sortable lists controller', () => { .map((element) => element.getAttribute('data-sortable-lists--item-id-value')!); } - // Mirrors item.controller.ts's onGenerateDragPreview and onDragStart: - // the root freezes the batch this drag represents, then marks its rows, - // before anything else can happen to it. + // Mirrors item.controller.ts: canDrag begins the session, the preview + // callback freezes it, drag start marks its rows. + let currentSession:DragSession|null = null; + function beginDrag(source:HTMLElement) { - controller.freezeDragBatch(source); - controller.markDragBatch(); + currentSession = controller.beginDrag(source); + currentSession.freeze(); + currentSession.start(); + return currentSession; } function batchDropTargets({ targetList, targetItem, edge }:{ @@ -2537,6 +2554,19 @@ describe('Sortable lists controller', () => { expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list2)); }); + // A morph mid-drag can reparent a row, and the destination a drop into + // it reaches is the live one. + it('re-reads the owner after a morph mid-drag', async () => { + beginDrag(item1); + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list1)); + + list2Rows.append(item2); + root.dispatchEvent(new CustomEvent('turbo:morph-element', { bubbles: true })); + await ctx.nextFrame(); + + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list2)); + }); + it('answers live outside a drag', () => { expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list1)); @@ -2544,6 +2574,18 @@ describe('Sortable lists controller', () => { expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list2)); }); + + // Pragmatic checks the drag handle after canDrag, so a press outside + // the handle leaves a session that never froze; it must not pin + // answers given outside any drag. + it('answers live while a session is only prospective', () => { + controller.beginDrag(item1); + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list1)); + + list2Rows.append(item2); + + expect(controller.ownerDestinationOf(item2)).toEqual(destinationOf(list2)); + }); }); // The destination policy is applied over the whole batch, and a batch may @@ -2576,7 +2618,7 @@ describe('Sortable lists controller', () => { itemId: sourceId, type: 'work_package', rootElement: root, - permittedDestinations: controller.dragPermittedDestinations(source), + permittedDestinations: currentSession?.permittedDestinations() ?? null, })), location: { initial: { dropTargets: [], input: input() }, @@ -2591,37 +2633,37 @@ describe('Sortable lists controller', () => { it('permits every list while no member is confined', () => { selectItems(item1, item2); - expect(controller.dragPermittedDestinations(item1)).toBeNull(); + expect(controller.beginDrag(item1).permittedDestinations()).toBeNull(); }); it('pins the drag to the list a selected confined batch-mate sits in', () => { selectItems(item1, item3); - expect(controller.dragPermittedDestinations(item1)).toEqual([destinationOf(list1)]); + expect(controller.beginDrag(item1).permittedDestinations()).toEqual([destinationOf(list1)]); }); it('pins the drag to a confined batch-mate in another list', () => { item4.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); selectItems(item1, item4); - expect(controller.dragPermittedDestinations(item1)).toEqual([destinationOf(list2)]); + expect(controller.beginDrag(item1).permittedDestinations()).toEqual([destinationOf(list2)]); }); it('permits nothing while confined members disagree on their list', () => { item4.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); selectItems(item3, item4); - expect(controller.dragPermittedDestinations(item3)).toEqual([]); + expect(controller.beginDrag(item3).permittedDestinations()).toEqual([]); }); it('does not pin the drag while the confined card is unselected', () => { selectItems(item1, item2); - expect(controller.dragPermittedDestinations(item1)).toBeNull(); + expect(controller.beginDrag(item1).permittedDestinations()).toBeNull(); }); it('pins the confined card itself without any selection', () => { - expect(controller.dragPermittedDestinations(item3)).toEqual([destinationOf(list1)]); + expect(controller.beginDrag(item3).permittedDestinations()).toEqual([destinationOf(list1)]); }); // Unreachable through a drag today, since a fixed card registers no @@ -2630,7 +2672,7 @@ describe('Sortable lists controller', () => { it('permits nothing for a fixed card', () => { item2.setAttribute('data-sortable-lists--item-mobility-value', 'fixed'); - expect(controller.dragPermittedDestinations(item2)).toEqual([]); + expect(controller.beginDrag(item2).permittedDestinations()).toEqual([]); }); it('refuses a cross-list drop of a batch with a confined member', async () => { @@ -2685,27 +2727,27 @@ describe('Sortable lists controller', () => { // getInitialDataForExternal reads this before the batch is frozen, so it // has to answer from the live selection rather than the frozen snapshot. - describe('externalDragItems', () => { + describe('prospective members', () => { it('returns just the card while nothing is selected', () => { - expect(controller.externalDragItems(item1)).toEqual([item1]); + expect(controller.beginDrag(item1).members).toEqual([item1]); }); it('returns every batch member when the card is part of a selection', () => { selectItems(item1, item3); - expect(controller.externalDragItems(item1)).toEqual([item1, item3]); + expect(controller.beginDrag(item1).members).toEqual([item1, item3]); }); it('returns just the card when it is not part of the selection', () => { selectItems(item3); - expect(controller.externalDragItems(item1)).toEqual([item1]); + expect(controller.beginDrag(item1).members).toEqual([item1]); }); it('does not touch the selection', () => { selectItems(item1, item3); - controller.externalDragItems(item1); + controller.beginDrag(item1); expect(selectedRowIds()).toEqual(['1', '3']); }); @@ -2934,11 +2976,11 @@ describe('Sortable lists controller', () => { expect(draggingIds()).toEqual(['2']); }); - it('returns the batch size from freezeDragBatch', () => { + it('returns the batch size from freeze', () => { selectItems(item1, item3); - expect(controller.freezeDragBatch(item1)).toBe(2); - expect(controller.freezeDragBatch(item2)).toBe(1); + expect(controller.beginDrag(item1).freeze()).toBe(2); + expect(controller.beginDrag(item2).freeze()).toBe(1); }); it('clears every dragging mark after a completed drop', async () => { @@ -2957,19 +2999,49 @@ describe('Sortable lists controller', () => { expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); }); - it('sweeps dragging marks defensively on disconnect', () => { + it('ends the drag session on disconnect', () => { selectItems(item1, item3); - beginDrag(item1); + const session = beginDrag(item1); controller.disconnect(); expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); - // Frozen batch nulled, not just its marks cleared: markDragBatch - // has nothing to mark. - controller.markDragBatch(); + // Ended, not just swept: a late remark has nothing to mark. + expect(session.phase).toBe('ended'); + session.remark(); expect(root.querySelectorAll('[data-dragging]')).toHaveLength(0); }); + // Pragmatic dispatches onDragStart a frame after the preview, and an + // item controller reconnecting in between loses nothing: the root's + // monitor starts the session. + it('starts the session from the monitor\'s drag start', () => { + selectItems(item1, item3); + const session = controller.beginDrag(item1); + session.freeze(); + + vi.mocked(monitorForElements).mock.lastCall?.[0].onDragStart?.({ + source: sourcePayload(item1, itemData('1', 'work_package', root)), + location: { + initial: { dropTargets: [], input: input() }, + current: { dropTargets: [], input: input() }, + previous: { dropTargets: [] }, + }, + }); + + expect(session.phase).toBe('started'); + expect(draggingIds()).toEqual(['1', '3']); + }); + + it('replaces a session that never started', () => { + const abandoned = controller.beginDrag(item1); + const session = controller.beginDrag(item2); + + expect(abandoned.phase).toBe('ended'); + expect(session.phase).toBe('prospective'); + expect(session.sourceElement).toBe(item2); + }); + // A stray mark on an element the batch never touched stands in for a // row Pragmatic's own onDrop cleanup never reached. it('sweeps a leftover mark from a row outside the frozen batch on drop', async () => { @@ -2982,7 +3054,7 @@ describe('Sortable lists controller', () => { }); // A morph can replace a batch-mate's row with a fresh element that - // never went through markDraggingRows, so it arrives unmarked while + // never went through the session's marking, so it arrives unmarked while // still part of the frozen batch. it('preserves frozen membership through registration healing and synthetic selection clearing', async () => { selectItems(item1, item3); @@ -3085,8 +3157,9 @@ describe('Sortable lists controller', () => { const item2 = sourceList.querySelector('[data-sortable-lists--item-id-value="2"]')!; await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as SortableListsControllerType; - controller.freezeDragBatch(item2); - controller.markDragBatch(); + const session = controller.beginDrag(item2); + session.freeze(); + session.start(); vi.spyOn(item1, 'getBoundingClientRect').mockReturnValue(rect()); const targetData = attachClosestEdge(sortableItemData({ itemId: '1', type: 'work_package' }), { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index bc24e635be14..cb8e3d11a24f 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -47,11 +47,10 @@ import { type SortableListData, type SortableListsRoot, } from './sortable-lists/drag-and-drop'; -import { selectionKey, type SelectionItem, type SelectionKey } from 'core-common/batch-selection'; +import { selectionKey, type SelectionItem } from 'core-common/batch-selection'; import { captureRowPositions, isOrderableItem, - itemAcceptsDestination, reorderRows, resolveDirectionalPreviousItemId, resolveItemId, @@ -68,7 +67,8 @@ import { type MoveDirection, } from './sortable-lists/list-dom'; import { SelectionOrchestrator, type SelectionHost } from './sortable-lists/selection-orchestrator'; -import { itemIdentity, orderedItemElements } from './sortable-lists/selection'; +import { itemElementsByKey, itemIdentity } from './sortable-lists/selection'; +import { DragSession, type DragSessionHost } from './sortable-lists/drag-session'; type CleanupFn = () => void; type ElementDropPayload = ElementEventPayloadMap['onDrop']; @@ -83,7 +83,7 @@ function relativeUrl(url:URL):string { return `${url.pathname}${url.search}${url.hash}`; } -export default class SortableListsController extends Controller implements SortableListsRoot, SelectionHost { +export default class SortableListsController extends Controller implements SortableListsRoot, SelectionHost, DragSessionHost { static outlets = ['sortable-lists--list', 'sortable-lists--item', 'sortable-lists--scrollable']; static values = { @@ -125,6 +125,11 @@ export default class SortableListsController extends Controller imp this.monitorCleanupFn = monitorForElements({ canMonitor: ({ source }) => !this.busy && isItemFromRoot(this.element, source.data), + // Pragmatic dispatches onDragStart a frame after the preview; an item + // controller reconnecting in between must not leave the batch unmarked. + onDragStart: () => { + this.dragSession?.start(); + }, onDrop: (args) => { void this.handleDrop(args); }, @@ -146,9 +151,7 @@ export default class SortableListsController extends Controller imp this.monitorCleanupFn = undefined; // A drag in flight when the controller disconnects would otherwise leave // its marks in the cached page and its frozen batch in this instance. - this.clearDraggingRows(); - this.activeDragBatch = null; - this.dragOwnerDestinations = null; + this.endDrag(); } // A Turbo morph can toggle the permission-gated value on a live root @@ -235,73 +238,54 @@ export default class SortableListsController extends Controller imp } } - // Frozen at drag start and consumed exactly once per drop, cancelled ones - // included: neither Escape nor a mid-drag morph can change what is - // submitted, and no stale batch leaks into the next drag. - private activeDragBatch:SelectionItem[]|null = null; + // The drag in flight, or the prospective session of a press that did not + // become one (Pragmatic checks the drag handle after canDrag). + dragSession:DragSession|null = null; - // Every item drop target asks for its owner on each dragover, and the - // answer holds for the whole drag, so it is remembered alongside the batch. - private dragOwnerDestinations:WeakMap|null = null; + beginDrag(itemElement:HTMLElement):DragSession { + // A session a press outside the drag handle or an aborted dragstart left + // behind holds no marks and no batch; ending it keeps one session per root. + this.endDrag(); + const session = new DragSession(this, itemElement); - // Pragmatic dispatches onGenerateDragPreview before onDragStart; the - // preview needs the count, the drag start marks the rows. - freezeDragBatch(itemElement:HTMLElement):number { - const scope = this.selection?.selectForAction(itemElement); - this.activeDragBatch = scope?.kind === 'batch' - ? scope.items.map((item) => itemIdentity(item)).filter((item):item is SelectionItem => item !== null) - : null; - this.dragOwnerDestinations = new WeakMap(); + // Announced from canDrag, the earliest point a drag can be stopped, so + // an oversized batch is told before any preview or drop feedback appears. + if (session.refused) { + void announce( + I18n.t(`${this.moveAnnouncementScopeValue}.batch_too_large`, { count: session.size, max: this.maxBatchSizeValue }), + { politeness: 'assertive' }, + ); + return session; + } - return Math.max(1, this.activeDragBatch?.length ?? 0); + this.dragSession = session; + return session; } - markDragBatch():void { - if (this.activeDragBatch) { - this.markDraggingRows(this.activeDragBatch); - } + private endDrag():SelectionItem[]|null { + const batch = this.dragSession?.end() ?? null; + this.dragSession = null; + return batch; } - // The destinations every member of the prospective batch accepts, null when - // the block reaches all of them. A batch may span lists, so a member that - // only accepts its own pins the block there, never to the dragged card's - // list. - dragPermittedDestinations(itemElement:HTMLElement):DestinationIdentity[]|null { - const scope = this.selection?.actionScopeFor(itemElement); - const members = scope?.kind === 'batch' ? scope.items : [itemElement]; - const ownerDestinationOf = (item:HTMLElement) => this.ownerDestinationOf(item); - - const lists = this.ownedListOutlets(); - const permitted = lists - .map((list) => destinationOfList(list.listData)) - .filter((destination) => members.every((member) => itemAcceptsDestination(member, destination, ownerDestinationOf))); - - return permitted.length === lists.length ? null : permitted; + get maxBatchSize():number { + return this.maxBatchSizeValue; } - // Asked in canDrag, the earliest point a drag can be stopped: an oversized - // batch is told so before any preview or drop feedback appears. - dragRefused(itemElement:HTMLElement):boolean { - if (this.maxBatchSizeValue <= 0) { - return false; - } - + prospectiveMembers(itemElement:HTMLElement):HTMLElement[] { const scope = this.selection?.actionScopeFor(itemElement); - const count = scope?.kind === 'batch' ? scope.items.length : 1; - if (count <= this.maxBatchSizeValue) { - return false; - } + return scope?.kind === 'batch' ? scope.items : [itemElement]; + } - void announce( - I18n.t(`${this.moveAnnouncementScopeValue}.batch_too_large`, { count, max: this.maxBatchSizeValue }), - { politeness: 'assertive' }, - ); - return true; + frozenMembers(itemElement:HTMLElement):SelectionItem[]|null { + const scope = this.selection?.selectForAction(itemElement); + return scope?.kind === 'batch' + ? scope.items.map((item) => itemIdentity(item)).filter((item):item is SelectionItem => item !== null) + : null; } - externalDragItems(itemElement:HTMLElement):HTMLElement[] { - const scope = this.selection?.actionScopeFor(itemElement); - return scope?.kind === 'batch' ? scope.items : [itemElement]; + ownedDestinations():DestinationIdentity[] { + return this.ownedListOutlets().map((list) => destinationOfList(list.listData)); } // Outlets match document-wide; another root's lists are not ours. @@ -309,44 +293,6 @@ export default class SortableListsController extends Controller imp return this.sortableListsListOutlets.filter((list) => this.element.contains(list.element)); } - // Marked on the item element itself, the same one the item controller's - // own onDragStart marks, so CSS keys off one convention regardless of - // which controller did the marking. - private markDraggingRows(items:SelectionItem[]):void { - const elements = this.itemElementsByKey(); - items.forEach((item) => { - elements.get(selectionKey(item))?.setAttribute('data-dragging', 'source'); - }); - } - - // Every mark under the root, not just the frozen batch's own rows: a - // cancelled drop, or the item controller's onDrop missing a row, would - // otherwise leave one behind. - private clearDraggingRows():void { - this.element.querySelectorAll('[data-dragging]').forEach((element) => element.removeAttribute('data-dragging')); - } - - // One document query per callback; never kept, so a morph cannot leave it - // stale. Keyed on type as well as id: ids collide across source tables. - private itemElementsByKey():Map { - const map = new Map(); - orderedItemElements(this.element).forEach((element) => { - const identity = itemIdentity(element); - if (identity) { - map.set(selectionKey(identity), element); - } - }); - return map; - } - - private takeActiveDragBatch():SelectionItem[]|null { - const batch = this.activeDragBatch; - this.clearDraggingRows(); - this.activeDragBatch = null; - this.dragOwnerDestinations = null; - return batch; - } - // 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 @@ -393,12 +339,10 @@ export default class SortableListsController extends Controller imp // the model. this.selection?.reconcile(); - // A row a morph replaces mid-drag comes back as fresh server HTML that - // never went through markDragBatch, so it loses data-dragging with the - // element it replaced. - if (this.activeDragBatch) { - this.markDraggingRows(this.activeDragBatch); - } + // A morph can reparent a row the owner memo already answered for and + // replace a marked row with fresh server HTML without its mark. + this.dragSession?.forgetOwners(); + this.dragSession?.remark(); }); }; @@ -520,22 +464,23 @@ export default class SortableListsController extends Controller imp return this.ownerListOf(itemElement)?.rowsContainer ?? null; } + // Remembered only once the drag is real: a prospective session left by a + // press outside the handle must not pin answers given outside any drag. ownerDestinationOf(element:HTMLElement):DestinationIdentity|null { - const remembered = this.dragOwnerDestinations?.get(element); - if (remembered !== undefined) { - return remembered; - } + return this.dragSession && this.dragSession.phase !== 'prospective' + ? this.dragSession.ownerDestinationOf(element) + : this.liveOwnerDestinationOf(element); + } + liveOwnerDestinationOf(element:HTMLElement):DestinationIdentity|null { const listData = this.ownerListOf(element)?.listData; - const destination = listData ? destinationOfList(listData) : null; - this.dragOwnerDestinations?.set(element, destination); - return destination; + return listData ? destinationOfList(listData) : null; } private async handleDrop({ location, source }:ElementDropPayload) { - // Before any bail-out below: a cancelled drop still consumes the frozen - // snapshot rather than leaking it into the next drag. - const frozenBatch = this.takeActiveDragBatch(); + // Before any bail-out below: a cancelled drop still ends the session + // rather than leaking its frozen batch into the next drag. + const frozenBatch = this.endDrag(); if (this.busy) { debugLog('sortable-lists: ignoring drop, a move is already in progress'); @@ -628,7 +573,7 @@ export default class SortableListsController extends Controller imp // Refused whole when a row is missing: a member that vanished mid-drag // means a partial block would diverge from the ids the request claims. private rowsForItems(items:SelectionItem[]):HTMLElement[]|null { - const elements = this.itemElementsByKey(); + const elements = itemElementsByKey(this.element); const rows:HTMLElement[] = []; for (const item of items) { diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md b/frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md index d5170938528a..070272058e29 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/README.md @@ -60,9 +60,10 @@ reconciliation preserves it. ## Batch movement -Dragging a selected item moves the whole batch. The root freezes the batch in -the preview callback (`freezeDragBatch`) and marks its rows at drag start -(`markDragBatch`), so later selection changes do not change the submitted items. +Dragging a selected item moves the whole batch. The drag session +([drag-session.ts](drag-session.ts)) resolves the batch when the drag is first +permitted, freezes it in the preview callback and marks its rows at drag +start, so later selection changes do not change the submitted items. Dragging an unselected item selects it, collapsing any wider selection. A batch may drop only on a destination every member accepts. A `confined` member 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 600f4d84b726..88ba7cd3930c 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 @@ -40,6 +40,7 @@ import { import { getElementFromPointWithoutHoneypot } from '@atlaskit/pragmatic-drag-and-drop/private/get-element-from-point-without-honey-pot'; import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/types'; import { type SelectionItem } from 'core-common/batch-selection'; +import { type DragSession } from './drag-session'; import { isExcludedItem, resolveClosestItemElement, @@ -108,23 +109,15 @@ 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; - // Freezes the drag's batch and returns its size; the preview renders it. - freezeDragBatch(itemElement:HTMLElement):number; - // Marks the frozen batch's rows; a no-op before freezeDragBatch. - markDragBatch():void; - // Asked while the drag payload is built, which Pragmatic dispatches before - // freezeDragBatch freezes the batch, so the answer comes from the live - // selection in the same synchronous dragstart turn. - dragPermittedDestinations(itemElement:HTMLElement):DestinationIdentity[]|null; + // Asked in canDrag. A refused session is announced here and not kept. + beginDrag(itemElement:HTMLElement):DragSession; + // The session begun last, which the item reads in every later drag + // callback; held by the root so an item controller replaced mid-drag + // finds it again. + readonly dragSession:DragSession|null; // The destination of the element's innermost owning list, or null when no - // list the root knows about claims it. + // list the root knows about claims it. Remembered for the drag in flight. ownerDestinationOf(element:HTMLElement):DestinationIdentity|null; - // Asked in canDrag: true when the item's prospective batch exceeds the - // server's cap, so the drag never starts. - dragRefused(itemElement:HTMLElement):boolean; - // The cards an external drop should receive: the prospective batch, read - // before the batch is frozen, without touching the selection. - externalDragItems(itemElement:HTMLElement):HTMLElement[]; } // Implemented by the list, item and scrollable controllers so the root can diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts index 5b098cf7502c..ca361c587bad 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts @@ -68,11 +68,12 @@ export function permittedDestinationsFor( * One drag, from the first `canDrag` read to its single end. * * Pragmatic asks `canDrag`, then builds the payloads, then renders the - * preview, then starts the drag; the phases follow that order. Members are - * resolved once, in the constructor, and stay fixed; freezing happens at - * most once; `end()` hands the frozen batch out at most once. Rows are held - * as identities only, so a morph that replaces an element mid-drag cannot - * leave the session pointing at a node that left the document. + * preview, then starts the drag; the phases follow that order. The + * prospective members are resolved once, in the constructor, as elements + * read within that same dragstart turn; freezing commits the batch once, + * as identities, so a morph that replaces an element mid-drag cannot leave + * the frozen batch pointing at a node that left the document. `end()` hands + * the frozen batch out at most once. */ export class DragSession { readonly members:HTMLElement[]; @@ -101,6 +102,8 @@ export class DragSession { return this.host.maxBatchSize > 0 && this.members.length > this.host.maxBatchSize; } + // The drag-start answer, which the Pragmatic payload carries for the rest + // of the drag; it is not recomputed after a morph. permittedDestinations():DestinationIdentity[]|null { this.permitted ??= { value: permittedDestinationsFor( 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 30a1563b40ad..20f737cb0fb5 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 @@ -60,6 +60,13 @@ import type { ActionEvent } from '@hotwired/stimulus'; import type ItemControllerType from './item.controller'; import type { SortableListsRoot } from './drag-and-drop'; import type { DestinationIdentity } from './list-dom'; +import { type SelectionItem } from 'core-common/batch-selection'; +import { DragSession, type DragSessionHost } from './drag-session'; +import { type Mock } from 'vitest'; + +// The method is a mock property rather than a method signature, so tests +// may read the spy without binding it. +type FakeRoot = Omit & { beginDrag:Mock<(itemElement:HTMLElement) => DragSession> }; describe('Sortable lists item controller', () => { let draggable:typeof draggableFn; @@ -95,35 +102,73 @@ describe('Sortable lists item controller', () => { } function fakeRoot( - element = document.createElement('div'), - { busy = false, ownerDestination = null, ownerRowsContainer = () => null }:{ + element:HTMLElement = document.createElement('div'), + { + busy = false, + ownerDestination = null, + ownerRowsContainer = () => null, + maxBatchSize = 0, + prospectiveMembers = (item:HTMLElement) => [item], + frozenMembers = ():SelectionItem[]|null => null, + // Two owned lists by default: with one, a confined member would permit + // every list and read as unrestricted. + ownedDestinations = [ownerDestination, { type: 'item', id: 'elsewhere' }] + .filter((destination):destination is DestinationIdentity => destination !== null), + }:{ busy?:boolean; ownerDestination?:DestinationIdentity|null; ownerRowsContainer?:(itemElement:HTMLElement) => HTMLElement|null; + maxBatchSize?:number; + prospectiveMembers?:(itemElement:HTMLElement) => HTMLElement[]; + frozenMembers?:(itemElement:HTMLElement) => SelectionItem[]|null; + ownedDestinations?:DestinationIdentity[]; } = {}, - ):SortableListsRoot { + ):FakeRoot { Object.defineProperty(element, 'isConnected', { value: true, configurable: true }); + const host:DragSessionHost = { + rootElement: element, + maxBatchSize, + prospectiveMembers, + frozenMembers, + ownedDestinations: () => ownedDestinations, + liveOwnerDestinationOf: () => ownerDestination, + }; + + // Mirrors the real root: one session at a time, a refused one not kept. + let dragSession:DragSession|null = null; + const beginDrag = vi.fn((itemElement:HTMLElement) => { + dragSession?.end(); + const session = new DragSession(host, itemElement); + dragSession = session.refused ? null : session; + return session; + }); + return { element, busy, moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), ownerRowsContainer: vi.fn(ownerRowsContainer), - freezeDragBatch: vi.fn(() => 1), - markDragBatch: vi.fn(), ownerDestinationOf: vi.fn(() => ownerDestination), - // Mirrors the real root's fallback for a batchless drag: the item's own - // mobility attribute is the whole answer. - dragPermittedDestinations: vi.fn((itemElement:HTMLElement) => ( - itemElement.getAttribute('data-sortable-lists--item-mobility-value') === 'confined' - ? [ownerDestination].filter((destination):destination is DestinationIdentity => destination !== null) - : null - )), - dragRefused: vi.fn(() => false), - externalDragItems: vi.fn((item:HTMLElement) => [item]), + beginDrag, + get dragSession():DragSession|null { + return dragSession; + }, }; } + // Pragmatic asks canDrag before it builds payloads, renders the preview or + // starts the drag; the item begins its session there. The pointer lands on + // nothing, so the point check passes. + function permitDrag(element:HTMLElement, root:FakeRoot):DragSession { + vi.spyOn(document, 'elementFromPoint').mockReturnValue(null); + expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ + element, dragHandle: null, input: { clientX: 10, clientY: 10 } as never, + })).toBe(true); + + return root.beginDrag.mock.results.at(-1)!.value as DragSession; + } + function connectedControllerFor( element:HTMLElement, { @@ -772,11 +817,13 @@ describe('Sortable lists item controller', () => { element.setAttribute('data-sortable-lists--item-external-url-value', 'http://example.org/work_packages/123'); element.setAttribute('data-sortable-lists--item-label-value', 'Card'); + const root = fakeRoot(undefined, { prospectiveMembers: () => [element, mate] }); connectedControllerFor(element, { externalUrl: 'http://example.org/work_packages/123', label: 'Card', - root: { ...fakeRoot(), externalDragItems: vi.fn(() => [element, mate]) }, + root, }); + permitDrag(element, root); const externalData = vi.mocked(draggable).mock.lastCall?.[0].getInitialDataForExternal?.(draggableArgs(element)); @@ -869,8 +916,10 @@ describe('Sortable lists item controller', () => { element.appendChild(text); vi.spyOn(document, 'elementFromPoint').mockReturnValue(text); - const root = fakeRoot(); - root.dragRefused = vi.fn(() => true); + const root = fakeRoot(undefined, { + maxBatchSize: 1, + prospectiveMembers: (item) => [item, document.createElement('article')], + }); connectedControllerFor(element, { root }); expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ @@ -878,6 +927,24 @@ describe('Sortable lists item controller', () => { })).toBe(false); }); + it('does not begin a session when the pointer is on an interactive descendant', () => { + const element = document.createElement('article'); + const button = document.createElement('button'); + element.appendChild(button); + vi.spyOn(document, 'elementFromPoint').mockReturnValue(button); + + const root = fakeRoot(undefined, { + maxBatchSize: 1, + prospectiveMembers: (item) => [item, document.createElement('article')], + }); + connectedControllerFor(element, { root }); + + expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ + element, dragHandle: null, input: { clientX: 10, clientY: 10 } as never, + })).toBe(false); + expect(root.beginDrag).not.toHaveBeenCalled(); + }); + it('refuses to drag before the root reference is connected', () => { const element = document.createElement('article'); const text = document.createElement('span'); @@ -901,12 +968,11 @@ describe('Sortable lists item controller', () => { }); it('includes the root-resolved permitted destinations in the payload', () => { - const root = document.createElement('div'); + const rootElement = document.createElement('div'); const element = document.createElement('article'); - connectedControllerFor(element, { - root: fakeRoot(root, { ownerDestination: { type: 'sprint', id: '7' } }), - mobility: 'confined', - }); + const root = fakeRoot(rootElement, { ownerDestination: { type: 'sprint', id: '7' } }); + connectedControllerFor(element, { root, mobility: 'confined' }); + permitDrag(element, root); expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) .toEqual(expect.objectContaining({ permittedDestinations: [{ type: 'sprint', id: '7' }] })); @@ -915,12 +981,15 @@ describe('Sortable lists item controller', () => { // A confined item still defers to the root: the batch it would carry may // reach every list, and the item's own mobility must not narrow that. it('preserves an unrestricted permitted-destinations answer from the root', () => { - const root = document.createElement('div'); + const rootElement = document.createElement('div'); const element = document.createElement('article'); - connectedControllerFor(element, { - root: { ...fakeRoot(root), dragPermittedDestinations: vi.fn(() => null) }, - mobility: 'confined', + // The one owned list is the item's own, so its confinement restricts nothing. + const root = fakeRoot(rootElement, { + ownerDestination: { type: 'sprint', id: '7' }, + ownedDestinations: [{ type: 'sprint', id: '7' }], }); + connectedControllerFor(element, { root, mobility: 'confined' }); + permitDrag(element, root); expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) .toEqual(expect.objectContaining({ permittedDestinations: null })); @@ -940,13 +1009,17 @@ describe('Sortable lists item controller', () => { // item's own mobility: a free card dragging a confined batch-mate is pinned // to the mate's list, which need not be its own. it('carries the batch-aware permitted destinations of the root in the payload', () => { - const root = document.createElement('div'); + const rootElement = document.createElement('div'); const element = document.createElement('article'); + const mate = document.createElement('article'); + mate.setAttribute('data-sortable-lists--item-mobility-value', 'confined'); const mateDestination = { type: 'sprint', id: '9' }; - connectedControllerFor(element, { - root: { ...fakeRoot(root), dragPermittedDestinations: vi.fn(() => [mateDestination]) }, - mobility: 'free', + const root = fakeRoot(rootElement, { + ownerDestination: mateDestination, + prospectiveMembers: () => [element, mate], }); + connectedControllerFor(element, { root, mobility: 'free' }); + permitDrag(element, root); expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) .toEqual(expect.objectContaining({ permittedDestinations: [mateDestination] })); @@ -1148,19 +1221,11 @@ describe('Sortable lists item controller', () => { await ctx.nextFrame(); const controller = ctx.getController>('sortable-lists--item', row); - controller.connectRoot({ - element: row, - busy: false, - moveInDirection: vi.fn(), - moveAvailability: vi.fn(() => null), - ownerRowsContainer: vi.fn(() => null), - freezeDragBatch: vi.fn(() => 3), - markDragBatch: vi.fn(), - dragPermittedDestinations: vi.fn(() => null), - ownerDestinationOf: vi.fn(() => null), - dragRefused: vi.fn(() => false), - externalDragItems: vi.fn((element:HTMLElement) => [element]), + const root = fakeRoot(row, { + frozenMembers: () => ['1', '2', '3'].map((id) => ({ type: 'work_package', id })), }); + controller.connectRoot(root); + permitDrag(row, root); vi.mocked(draggable).mock.lastCall?.[0].onGenerateDragPreview?.({ ...dragEventPayload(article), @@ -1241,19 +1306,11 @@ describe('Sortable lists item controller', () => { await ctx.nextFrame(); const controller = ctx.getController>('sortable-lists--item', row); - controller.connectRoot({ - element: row, - busy: false, - moveInDirection: vi.fn(), - moveAvailability: vi.fn(() => null), - ownerRowsContainer: vi.fn(() => null), - freezeDragBatch: vi.fn(() => 3), - markDragBatch: vi.fn(), - dragPermittedDestinations: vi.fn(() => null), - ownerDestinationOf: vi.fn(() => null), - dragRefused: vi.fn(() => false), - externalDragItems: vi.fn((element:HTMLElement) => [element]), + const root = fakeRoot(row, { + frozenMembers: () => ['1', '2', '3'].map((id) => ({ type: 'work_package', id })), }); + controller.connectRoot(root); + permitDrag(row, root); vi.mocked(draggable).mock.lastCall?.[0].onGenerateDragPreview?.({ ...dragEventPayload(article), @@ -1647,29 +1704,51 @@ describe('Sortable lists item controller', () => { expect(document.activeElement).toBe(item); }); - it('marks the batch on drag start', async () => { + // Pragmatic dispatches onDragStart a frame after the preview; the root's + // monitor starts the session then, so an item controller replaced in + // between cannot strand the batch. The item itself marks nothing. + it('leaves drag-start marking to the root while a session exists', async () => { const item = await renderItem({ mobility: 'free' }); const controller = controllerFor(item); - const markDragBatch = vi.fn(); - const root:SortableListsRoot = { - element: item, - busy: false, - moveInDirection: vi.fn(), - moveAvailability: vi.fn(() => null), - ownerRowsContainer: vi.fn(() => null), - freezeDragBatch: vi.fn(() => 1), - markDragBatch, - dragPermittedDestinations: vi.fn(() => null), - ownerDestinationOf: vi.fn(() => null), - dragRefused: vi.fn(() => false), - externalDragItems: vi.fn((element:HTMLElement) => [element]), - }; + const root = fakeRoot(item); controller.connectRoot(root); + const session = permitDrag(item, root); vi.mocked(draggable).mock.lastCall?.[0].onDragStart?.(dragEventPayload(item)); - expect(markDragBatch).toHaveBeenCalled(); + expect(session.phase).toBe('prospective'); + expect(item).not.toHaveAttribute('data-dragging'); + }); + + // canDrag refuses every drag without a root, so the item's own marking + // only ever runs when the root went away after permitting the drag. + it('marks and clears itself when the root disconnected after canDrag', async () => { + const item = await renderItem({ mobility: 'free' }); + const controller = controllerFor(item); + const root = fakeRoot(item); + controller.connectRoot(root); + permitDrag(item, root); + + controller.disconnectRoot(); + vi.mocked(draggable).mock.lastCall?.[0].onDragStart?.(dragEventPayload(item)); + expect(item).toHaveAttribute('data-dragging', 'source'); + + vi.mocked(draggable).mock.lastCall?.[0].onDrop?.(dragEventPayload(item)); + expect(item).not.toHaveAttribute('data-dragging'); + }); + + it('leaves a mark the root set alone on drop while the session lives', async () => { + const item = await renderItem({ mobility: 'free' }); + const controller = controllerFor(item); + const root = fakeRoot(item); + controller.connectRoot(root); + permitDrag(item, root); + item.setAttribute('data-dragging', 'source'); + + vi.mocked(draggable).mock.lastCall?.[0].onDrop?.(dragEventPayload(item)); + + expect(item).toHaveAttribute('data-dragging', 'source'); }); // Pragmatic invokes onGenerateDragPreview before onDragStart, so the @@ -1678,29 +1757,18 @@ describe('Sortable lists item controller', () => { it('freezes the batch at the top of onGenerateDragPreview, before the preview renders', async () => { const item = await renderItem({ mobility: 'free' }); const controller = controllerFor(item); - const freezeDragBatch = vi.fn(() => 1); - const root:SortableListsRoot = { - element: item, - busy: false, - moveInDirection: vi.fn(), - moveAvailability: vi.fn(() => null), - ownerRowsContainer: vi.fn(() => null), - freezeDragBatch, - markDragBatch: vi.fn(), - dragPermittedDestinations: vi.fn(() => null), - ownerDestinationOf: vi.fn(() => null), - dragRefused: vi.fn(() => false), - externalDragItems: vi.fn((element:HTMLElement) => [element]), - }; + const root = fakeRoot(item); controller.connectRoot(root); + const session = permitDrag(item, root); vi.mocked(draggable).mock.lastCall?.[0].onGenerateDragPreview?.({ ...dragEventPayload(item), nativeSetDragImage: vi.fn(), }); - expect(freezeDragBatch).toHaveBeenCalledWith(item); + expect(root.beginDrag).toHaveBeenCalledWith(item); + expect(session.phase).toBe('frozen'); }); }); }); 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 80e7f2e281d5..a84a1677fcf3 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -58,6 +58,7 @@ import { resolveItemLabel, sortableItemSelector, } from './list-dom'; +import { draggingAttribute } from './drag-session'; import { webLinkHref } from './external-data'; import { renderDragPreview } from './preview'; @@ -227,29 +228,40 @@ export default class ItemController extends Controller implements R } : {}), canDrag: ({ input }) => { const { root } = this; - if (root == null || root.busy || root.dragRefused(this.element)) { + if (root == null || root.busy || !this.canDragFromPoint(input.clientX, input.clientY)) { return false; } - return this.canDragFromPoint(input.clientX, input.clientY); + + // After the point check: a press on a button inside the card must + // not announce an oversized batch for a drag that never starts. + return !root.beginDrag(this.element).refused; }, getInitialData: () => this.getItemData(), onDragStart: () => { - this.root?.markDragBatch(); + // The root's monitor starts its session and marks the batch. The item + // marks itself only when the root lost its session after permitting + // the drag, such as a disconnect in between. + if (!this.root?.dragSession) { + this.element.setAttribute(draggingAttribute, 'source'); + } // 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. preventUnhandled.start(); - this.element.setAttribute('data-dragging', 'source'); }, onDrop: () => { preventUnhandled.stop(); this.clearDropIndicator(); - this.element.removeAttribute('data-dragging'); + // The root's monitor ends the session and clears every mark under + // it; the item cleans up only the mark it set itself above. + if (!this.root?.dragSession) { + this.element.removeAttribute(draggingAttribute); + } }, onGenerateDragPreview: ({ location, nativeSetDragImage }) => { - // Pragmatic dispatches this before onDragStart, so the batch has to - // be frozen by the time the preview renders. - const batchSize = this.root?.freezeDragBatch(this.element) ?? 1; + // Pragmatic dispatches this before onDragStart, so the batch is + // frozen here, in time for the preview to show its size. + const batchSize = this.root?.dragSession?.freeze() ?? 1; if (!this.hasPreviewTarget) { return; @@ -303,7 +315,7 @@ export default class ItemController extends Controller implements R return isItemFromRoot(root.element, source.data) && source.data.itemId !== this.idValue && source.data.type === this.typeValue - && !this.element.hasAttribute('data-dragging') + && !this.element.hasAttribute(draggingAttribute) && permittedDestinationsAllowDrop(source.data, this.root?.ownerDestinationOf(this.element) ?? null); }, // Only the identity a drop needs; the batch-aware fields are computed @@ -365,7 +377,7 @@ export default class ItemController extends Controller implements R // Every member of the prospective batch, so an external drop receives the // whole block; text/html joins in as one link per labelled member. private externalDragData():Record { - const members = this.root?.externalDragItems(this.element) ?? [this.element]; + const members = this.root?.dragSession?.members ?? [this.element]; const entries = members .map((member) => ({ url: resolveItemExternalUrl(member), label: resolveItemLabel(member) })) .filter((entry):entry is { url:string; label:string|null } => entry.url !== null); @@ -400,8 +412,8 @@ export default class ItemController extends Controller implements R // A rootless item can carry no batch, so its own mobility is the // whole answer, and it can name no list either: anything short of free // movement leaves it accepting nothing. - permittedDestinations: this.root - ? this.root.dragPermittedDestinations(this.element) + permittedDestinations: this.root?.dragSession + ? this.root.dragSession.permittedDestinations() : (itemMobility(this.element) === 'free' ? null : []), }); } @@ -433,7 +445,7 @@ export default class ItemController extends Controller implements R } let next = this.element.nextElementSibling; - while (next instanceof HTMLElement && next.matches(sortableItemSelector) && next.hasAttribute('data-dragging')) { + while (next instanceof HTMLElement && next.matches(sortableItemSelector) && next.hasAttribute(draggingAttribute)) { next = next.nextElementSibling; } 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 5007920f8290..e709f04241a0 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 @@ -80,12 +80,9 @@ describe('Sortable lists list controller', () => { moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), - freezeDragBatch: vi.fn(() => 1), - markDragBatch: vi.fn(), - dragPermittedDestinations: vi.fn(() => null), + beginDrag: vi.fn(), + dragSession: null, ownerDestinationOf: vi.fn(() => null), - dragRefused: vi.fn(() => false), - externalDragItems: vi.fn((item:HTMLElement) => [item]), }; } 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 10209dda3f3c..b27249276403 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 @@ -73,12 +73,9 @@ describe('Sortable lists scrollable controller', () => { moveInDirection: vi.fn(), moveAvailability: vi.fn(() => null), ownerRowsContainer: vi.fn(() => null), - freezeDragBatch: vi.fn(() => 1), - markDragBatch: vi.fn(), - dragPermittedDestinations: vi.fn(() => null), + beginDrag: vi.fn(), + dragSession: null, ownerDestinationOf: vi.fn(() => null), - dragRefused: vi.fn(() => false), - externalDragItems: vi.fn((item:HTMLElement) => [item]), }; } From 97a6232de1b9b1c08ab6219239726638d0645b41 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Fri, 2 Oct 2026 23:45:05 +0100 Subject: [PATCH 4/4] Name the Stimulus drag payload after its role Renames the Stimulus SortableItemData to SortableDragSourceData. Two types shared the name with different shapes: the engine payload in core-common names an item and its list id, the Stimulus one the dragged source, its root and the destinations its batch may reach. The identity symbol follows suit. The wire and the DOM contract are unchanged. --- .../dynamic/sortable-lists.controller.spec.ts | 18 +++--- .../sortable-lists/drag-and-drop.spec.ts | 56 +++++++++---------- .../dynamic/sortable-lists/drag-and-drop.ts | 25 +++++---- .../sortable-lists/item.controller.spec.ts | 20 +++---- .../dynamic/sortable-lists/item.controller.ts | 8 +-- .../sortable-lists/list.controller.spec.ts | 12 ++-- .../scrollable.controller.spec.ts | 12 ++-- 7 files changed, 77 insertions(+), 74 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 a60152f3568e..cc6d115873a0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -66,7 +66,7 @@ import type SortableListsControllerType from './sortable-lists.controller'; import type { DragSession } from './sortable-lists/drag-session'; import { selectionTranslations } from './sortable-lists/testing/selection-translations'; import type { - sortableItemData as sortableItemDataFn, + sortableDragSourceData as sortableDragSourceDataFn, sortableListData as sortableListDataFn, } from './sortable-lists/drag-and-drop'; @@ -75,7 +75,7 @@ describe('Sortable lists controller', () => { let monitorForElements:typeof monitorForElementsFn; let SortableListsController:typeof SortableListsControllerType; - let sortableItemData:typeof sortableItemDataFn; + let sortableDragSourceData:typeof sortableDragSourceDataFn; let sortableListData:typeof sortableListDataFn; let ctx:StimulusTestContext; @@ -90,7 +90,7 @@ describe('Sortable lists controller', () => { beforeAll(async () => { ({ monitorForElements } = await import('@atlaskit/pragmatic-drag-and-drop/element/adapter')); ({ default: SortableListsController } = await import('./sortable-lists.controller')); - ({ sortableItemData, sortableListData } = await import('./sortable-lists/drag-and-drop')); + ({ sortableDragSourceData, sortableListData } = await import('./sortable-lists/drag-and-drop')); }); function input({ clientY = 10 }:{ clientY?:number } = {}) { @@ -218,7 +218,7 @@ describe('Sortable lists controller', () => { } function itemData(itemId = '1', type = 'work_package', rootElement:HTMLElement|null = null) { - return sortableItemData({ itemId, type, rootElement }); + return sortableDragSourceData({ itemId, type, rootElement }); } function sourcePayload(element:HTMLElement, data:Record = itemData()) { @@ -776,11 +776,11 @@ describe('Sortable lists controller', () => { const canMonitor = vi.mocked(monitorForElements).mock.lastCall?.[0].canMonitor; expect(canMonitor?.({ - source: sourcePayload(firstSourceItem, sortableItemData({ itemId: '1', type: 'work_package', rootElement: root })), + source: sourcePayload(firstSourceItem, sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: root })), initial: {} as never, })).toBe(true); expect(canMonitor?.({ - source: sourcePayload(firstSourceItem, sortableItemData({ itemId: '1', type: 'work_package', rootElement: document.createElement('div') })), + source: sourcePayload(firstSourceItem, sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: document.createElement('div') })), initial: {} as never, })).toBe(false); }); @@ -2424,7 +2424,7 @@ describe('Sortable lists controller', () => { if (targetItem && edge) { vi.spyOn(targetItem, 'getBoundingClientRect').mockReturnValue(rect()); const targetItemId = targetItem.getAttribute('data-sortable-lists--item-id-value')!; - const data = attachClosestEdge(sortableItemData({ itemId: targetItemId, type: 'work_package' }), { + const data = attachClosestEdge(sortableDragSourceData({ itemId: targetItemId, type: 'work_package' }), { element: targetItem, input: input({ clientY: edge === 'bottom' ? 90 : 10 }), allowedEdges: ['top', 'bottom'], @@ -2614,7 +2614,7 @@ describe('Sortable lists controller', () => { const sourceId = source.getAttribute('data-sortable-lists--item-id-value')!; monitorOptions?.onDrop?.({ - source: sourcePayload(source, sortableItemData({ + source: sourcePayload(source, sortableDragSourceData({ itemId: sourceId, type: 'work_package', rootElement: root, @@ -3162,7 +3162,7 @@ describe('Sortable lists controller', () => { session.start(); vi.spyOn(item1, 'getBoundingClientRect').mockReturnValue(rect()); - const targetData = attachClosestEdge(sortableItemData({ itemId: '1', type: 'work_package' }), { + const targetData = attachClosestEdge(sortableDragSourceData({ itemId: '1', type: 'work_package' }), { element: item1, input: input({ clientY: 10 }), allowedEdges: ['top', 'bottom'], diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts index 29038d423b59..9763ece17e07 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-and-drop.spec.ts @@ -38,7 +38,7 @@ import { isSortableListData, resolveDropIntent, resolvePreviousSortableItemId, - sortableItemData, + sortableDragSourceData, sortableItemIdentity, sortableListData, } from './drag-and-drop'; @@ -108,7 +108,7 @@ describe('sortable lists drag and drop helpers', () => { }); it('accepts backlogs item data', () => { - expect(isSortableItemIdentity(sortableItemData({ type: 'work_package', itemId: '42' }))).toBe(true); + expect(isSortableItemIdentity(sortableDragSourceData({ type: 'work_package', itemId: '42' }))).toBe(true); }); it('rejects lookalike data from another drag source', () => { @@ -120,11 +120,11 @@ describe('sortable lists drag and drop helpers', () => { }); it('rejects data with a blank item id', () => { - expect(isSortableItemIdentity(sortableItemData({ type: 'work_package', itemId: '' }))).toBe(false); + expect(isSortableItemIdentity(sortableDragSourceData({ type: 'work_package', itemId: '' }))).toBe(false); }); it('rejects data with a blank type', () => { - expect(isSortableItemIdentity(sortableItemData({ type: '', itemId: '1' }))).toBe(false); + expect(isSortableItemIdentity(sortableDragSourceData({ type: '', itemId: '1' }))).toBe(false); }); }); @@ -138,9 +138,9 @@ describe('sortable lists drag and drop helpers', () => { }); }); - describe('sortableItemData', () => { + describe('sortableDragSourceData', () => { it('uses the item type as the public source type', () => { - const data = sortableItemData({ type: 'work_package', itemId: '42' }); + const data = sortableDragSourceData({ type: 'work_package', itemId: '42' }); expect(data.type).toEqual('work_package'); expect(data.itemId).toEqual('42'); @@ -149,14 +149,14 @@ describe('sortable lists drag and drop helpers', () => { it('carries the root element on the item payload when provided', () => { const root = document.createElement('div'); - const data = sortableItemData({ itemId: '1', type: 'work_package', rootElement: root }); + const data = sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: root }); expect(data.rootElement).toBe(root); expect(isSortableItemIdentity(data)).toBe(true); }); it('defaults the item payload root element to null', () => { - const data = sortableItemData({ itemId: '1', type: 'work_package' }); + const data = sortableDragSourceData({ itemId: '1', type: 'work_package' }); expect(data.rootElement).toBeNull(); }); @@ -189,7 +189,7 @@ describe('sortable lists drag and drop helpers', () => { const root = document.createElement('div'); it('accepts a sortable item whose rootElement is this root', () => { - const data = sortableItemData({ itemId: '1', type: 'work_package', rootElement: root }); + const data = sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: root }); expect(isItemFromRoot(root, data)).toBe(true); }); @@ -204,17 +204,17 @@ describe('sortable lists drag and drop helpers', () => { }); it('rejects a sortable item from another root', () => { - const data = sortableItemData({ itemId: '1', type: 'work_package', rootElement: document.createElement('div') }); + const data = sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: document.createElement('div') }); expect(isItemFromRoot(root, data)).toBe(false); }); it('rejects a sortable item with no root reference', () => { - const data = sortableItemData({ itemId: '1', type: 'work_package', rootElement: null }); + const data = sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: null }); expect(isItemFromRoot(root, data)).toBe(false); }); it('rejects a null root element', () => { - const data = sortableItemData({ itemId: '1', type: 'work_package', rootElement: root }); + const data = sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: root }); expect(isItemFromRoot(null, data)).toBe(false); }); @@ -460,7 +460,7 @@ describe('sortable lists drag and drop helpers', () => { document.body.appendChild(root); vi.spyOn(target, 'getBoundingClientRect').mockReturnValue(rect()); - const data = attachClosestEdge(sortableItemData({ type: 'work_package', itemId: '2' }), { + const data = attachClosestEdge(sortableDragSourceData({ type: 'work_package', itemId: '2' }), { element: target, input: input({ clientY: 90 }), allowedEdges: ['top', 'bottom'], @@ -474,7 +474,7 @@ describe('sortable lists drag and drop helpers', () => { ], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent?.listElement).toBe(list); @@ -491,7 +491,7 @@ describe('sortable lists drag and drop helpers', () => { document.body.appendChild(root); vi.spyOn(target, 'getBoundingClientRect').mockReturnValue(rect()); - const data = attachClosestEdge(sortableItemData({ type: 'work_package', itemId: '2' }), { + const data = attachClosestEdge(sortableDragSourceData({ type: 'work_package', itemId: '2' }), { element: target, input: input({ clientY: 90 }), allowedEdges: ['top', 'bottom'], @@ -500,7 +500,7 @@ describe('sortable lists drag and drop helpers', () => { const intent = resolveDropIntent({ location: dropLocation({ dropTargets: [{ data, element: target }] }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent).toBeNull(); @@ -521,7 +521,7 @@ describe('sortable lists drag and drop helpers', () => { dropTargets: [{ data: sortableListData({ type: 'backlog_bucket', listId: '7' }), element: list }], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent?.listElement).toBe(list); @@ -548,7 +548,7 @@ describe('sortable lists drag and drop helpers', () => { dropTargets: [{ data: sortableListData({ type: 'backlog_bucket', listId: '7' }), element: list }], }), root, - sourceData: sortableItemData({ + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1', permittedDestinations: [{ type: 'sprint', id: '9' }], @@ -569,7 +569,7 @@ describe('sortable lists drag and drop helpers', () => { dropTargets: [{ data: sortableListData({ type: 'sprint', listId: '7' }), element: list }], }), root, - sourceData: sortableItemData({ + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1', permittedDestinations: [{ type: 'sprint', id: '7' }], @@ -595,7 +595,7 @@ describe('sortable lists drag and drop helpers', () => { dropTargets: [{ data: sortableListData({ type: 'backlog_bucket', listId: '7', dropPosition: 'start' }), element: list }], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent?.listElement).toBe(list); @@ -613,7 +613,7 @@ describe('sortable lists drag and drop helpers', () => { dropTargets: [{ data: sortableListData({ type: 'backlog_bucket', listId: '7' }), element: list }], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent?.listElement).toBe(list); @@ -631,7 +631,7 @@ describe('sortable lists drag and drop helpers', () => { dropTargets: [{ data: sortableListData({ type: 'backlog_bucket', listId: '7', dropPosition: 'start' }), element: list }], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '2' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '2' }), }); expect(intent?.listElement).toBe(list); @@ -649,7 +649,7 @@ describe('sortable lists drag and drop helpers', () => { const intent = resolveDropIntent({ location: dropLocation({ clientY: 90 }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent).toBeNull(); @@ -671,7 +671,7 @@ describe('sortable lists drag and drop helpers', () => { dropTargets: [{ data: {}, element: header }], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent).toBeNull(); @@ -686,7 +686,7 @@ describe('sortable lists drag and drop helpers', () => { const intent = resolveDropIntent({ location: dropLocation(), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent).toBeNull(); @@ -701,7 +701,7 @@ describe('sortable lists drag and drop helpers', () => { document.body.appendChild(root); vi.spyOn(target, 'getBoundingClientRect').mockReturnValue(rect()); - const data = attachClosestEdge(sortableItemData({ type: 'work_package', itemId: '2' }), { + const data = attachClosestEdge(sortableDragSourceData({ type: 'work_package', itemId: '2' }), { element: target, input: input({ clientY: 90 }), allowedEdges: ['top', 'bottom'], @@ -715,7 +715,7 @@ describe('sortable lists drag and drop helpers', () => { ], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); // No rows container on the payload falls back to the list element. @@ -744,7 +744,7 @@ describe('sortable lists drag and drop helpers', () => { }], }), root, - sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + sourceData: sortableDragSourceData({ type: 'work_package', itemId: '1' }), }); expect(intent?.rowsContainer).toBe(rowsContainer); 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 88ba7cd3930c..4e7fcc154a4b 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 @@ -59,19 +59,22 @@ import { // The Pragmatic DnD payloads exchanged between the sortable-lists root and // item controllers, built on top of the DOM contract in list-dom.ts. -const sortableItemDataKey = Symbol('sortable-list-item'); +const sortableItemIdentityKey = Symbol('sortable-list-item'); const sortableListDataKey = Symbol('sortable-list'); // What a drop target exposes: the identity a drop resolves against, and // nothing that would have to be recomputed on every dragover. export interface SortableItemIdentity extends Record { - [sortableItemDataKey]:true; + [sortableItemIdentityKey]:true; type:string; itemId:string; } -// What the dragged source carries, resolved once at drag start. -export interface SortableItemData extends SortableItemIdentity { +// What the dragged source carries, resolved once at drag start. Distinct +// from the engine payload of the same shape family in +// core-common/drag-and-drop/payload.ts, which names a list id rather than +// a root. +export interface SortableDragSourceData extends SortableItemIdentity { rootElement:HTMLElement|null; // The destinations this drag may land in, resolved across the whole batch // at drag start; null when nothing restricts it, empty when nothing @@ -131,7 +134,7 @@ export interface RootAwareChild { } export function sortableItemIdentity({ type, itemId }:{ type:string; itemId:string }):SortableItemIdentity { - return { [sortableItemDataKey]: true, type, itemId }; + return { [sortableItemIdentityKey]: true, type, itemId }; } export function singleItemBatch({ type, itemId }:{ type:string; itemId:string }):SelectionItem[] { @@ -140,7 +143,7 @@ export function singleItemBatch({ type, itemId }:{ type:string; itemId:string }) // The source-only fields are what isItemFromRoot narrows on beyond this. export function isSortableItemIdentity(data:Record):data is SortableItemIdentity { - return data[sortableItemDataKey] === true + return data[sortableItemIdentityKey] === true && typeof data.type === 'string' && data.type.length > 0 && typeof data.itemId === 'string' @@ -154,7 +157,7 @@ export function isSortableListData(data:Record):data is && (typeof data.listId === 'string' || data.listId === null); } -export function sortableItemData({ +export function sortableDragSourceData({ type, itemId, rootElement = null, @@ -164,7 +167,7 @@ export function sortableItemData({ itemId:string; rootElement?:HTMLElement|null; permittedDestinations?:DestinationIdentity[]|null; -}):SortableItemData { +}):SortableDragSourceData { return { ...sortableItemIdentity({ type, itemId }), rootElement, @@ -222,7 +225,7 @@ export function buildMoveFormData({ export function isItemFromRoot( rootElement:HTMLElement|null, data:Record, -):data is SortableItemData { +):data is SortableDragSourceData { return rootElement != null && isSortableItemIdentity(data) && data.rootElement === rootElement @@ -244,7 +247,7 @@ export function isItemFromRoot( // the drop-indicator layers consult it too — rows never show a drop position // for it, and the list marks its container refused instead of active. export function permittedDestinationsAllowDrop( - data:SortableItemData, + data:SortableDragSourceData, destination:DestinationIdentity|null, ):boolean { return data.permittedDestinations === null @@ -329,7 +332,7 @@ export function resolveDropIntent({ }:{ location:DragLocationHistory; root:HTMLElement; - sourceData:SortableItemData; + sourceData:SortableDragSourceData; excludedItems?:ExcludedItems; }):DropIntent|null { const targetList = location.current.dropTargets.find( 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 20f737cb0fb5..8ba81269ed3a 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 @@ -74,7 +74,7 @@ describe('Sortable lists item controller', () => { let preventUnhandled:typeof preventUnhandledType; let setCustomNativeDragPreview:typeof setCustomNativeDragPreviewFn; let ItemController:typeof ItemControllerType; - let sortableItemData:typeof import('./drag-and-drop').sortableItemData; + let sortableDragSourceData:typeof import('./drag-and-drop').sortableDragSourceData; let sortableItemIdentity:typeof import('./drag-and-drop').sortableItemIdentity; interface TestItemController { @@ -88,7 +88,7 @@ describe('Sortable lists item controller', () => { ({ preventUnhandled } = await import('@atlaskit/pragmatic-drag-and-drop/prevent-unhandled')); ({ setCustomNativeDragPreview } = await import('@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview')); ({ default: ItemController } = await import('./item.controller')); - ({ sortableItemData, sortableItemIdentity } = await import('./drag-and-drop')); + ({ sortableDragSourceData, sortableItemIdentity } = await import('./drag-and-drop')); }); function controllerFor(element:HTMLElement) { @@ -549,7 +549,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '456', rootElement: root }), + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: root }), element: document.createElement('article'), } as never, })).toBe(false); @@ -565,7 +565,7 @@ describe('Sortable lists item controller', () => { element, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '123', rootElement: root }), + data: sortableDragSourceData({ type: 'item', itemId: '123', rootElement: root }), element: document.createElement('article'), } as never, })).toBe(false); @@ -582,7 +582,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '456', rootElement: foreignRoot }), + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: foreignRoot }), element: document.createElement('article'), } as never, })).toBe(false); @@ -598,7 +598,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ type: 'meeting_agenda_item', itemId: '456', rootElement: root }), + data: sortableDragSourceData({ type: 'meeting_agenda_item', itemId: '456', rootElement: root }), element: document.createElement('article'), } as never, })).toBe(false); @@ -614,7 +614,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '456', rootElement: root }), + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: root }), element: document.createElement('article'), } as never, })).toBe(true); @@ -632,7 +632,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: root, @@ -700,7 +700,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: root, @@ -720,7 +720,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '456', rootElement: root }), + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: root }), element: document.createElement('article'), } as never, })).toBe(false); 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 a84a1677fcf3..94beadcb0cf0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -44,10 +44,10 @@ import { closestDragBlockingElement } from 'core-stimulus/helpers/interactive-el import { permittedDestinationsAllowDrop, isItemFromRoot, - sortableItemData, + sortableDragSourceData, sortableItemIdentity, type RootAwareChild, - type SortableItemData, + type SortableDragSourceData, type SortableListsRoot, } from './drag-and-drop'; import { @@ -404,8 +404,8 @@ export default class ItemController extends Controller implements R return data; } - private getItemData():SortableItemData { - return sortableItemData({ + private getItemData():SortableDragSourceData { + return sortableDragSourceData({ itemId: this.idValue, type: this.typeValue, rootElement: this.root?.element ?? null, 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 e709f04241a0..d3056ebcdc41 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 @@ -37,7 +37,7 @@ vi.doMock('@atlaskit/pragmatic-drag-and-drop/element/adapter', () => ({ import type { dropTargetForElements as dropTargetForElementsFn } from '@atlaskit/pragmatic-drag-and-drop/element/adapter'; import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; import type ListControllerType from './list.controller'; -import type { sortableItemData as sortableItemDataFn, SortableListsRoot } from './drag-and-drop'; +import type { sortableDragSourceData as sortableDragSourceDataFn, SortableListsRoot } from './drag-and-drop'; import type { DestinationIdentity } from './list-dom'; // The list controller is tested in ISOLATION: the root drives the outlet @@ -47,7 +47,7 @@ import type { DestinationIdentity } from './list-dom'; describe('Sortable lists list controller', () => { let dropTargetForElements:typeof dropTargetForElementsFn; let ListController:typeof ListControllerType; - let sortableItemData:typeof sortableItemDataFn; + let sortableDragSourceData:typeof sortableDragSourceDataFn; let ctx:StimulusTestContext; let fixture:HTMLElement; @@ -55,7 +55,7 @@ describe('Sortable lists list controller', () => { beforeAll(async () => { ({ dropTargetForElements } = await import('@atlaskit/pragmatic-drag-and-drop/element/adapter')); ({ default: ListController } = await import('./list.controller')); - ({ sortableItemData } = await import('./drag-and-drop')); + ({ sortableDragSourceData } = await import('./drag-and-drop')); }); beforeEach(async () => { @@ -127,7 +127,7 @@ describe('Sortable lists list controller', () => { { permittedDestinations = null }:{ permittedDestinations?:DestinationIdentity[]|null } = {}, ) { return { - data: sortableItemData({ itemId: '1', type, rootElement, permittedDestinations }), + data: sortableDragSourceData({ itemId: '1', type, rootElement, permittedDestinations }), element: document.createElement('li'), } as never; } @@ -293,7 +293,7 @@ describe('Sortable lists list controller', () => { const options = dropTargetOptionsFor(list); options?.onDrag?.({ - location: locationOver({ data: sortableItemData({ itemId: '1', type: 'work_package', rootElement: null }) }), + location: locationOver({ data: sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: null }) }), source: source(rootElement), } as never); @@ -350,7 +350,7 @@ describe('Sortable lists list controller', () => { expect(list.dataset.dropContainer).toEqual('active'); options?.onDrag?.({ - location: locationOver({ data: sortableItemData({ itemId: '1', type: 'work_package', rootElement: null }) }), + location: locationOver({ data: sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: null }) }), source: source(rootElement), } as never); expect(list.dataset.dropContainer).toBeUndefined(); 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 b27249276403..276ac7985b4a 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 @@ -29,7 +29,7 @@ import { type autoScrollForElements as autoScrollForElementsFn } from '@atlaskit/pragmatic-drag-and-drop-auto-scroll/element'; import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; import type ScrollableControllerType from './scrollable.controller'; -import type { sortableItemData as sortableItemDataFn, SortableListsRoot } from './drag-and-drop'; +import type { sortableDragSourceData as sortableDragSourceDataFn, SortableListsRoot } from './drag-and-drop'; vi.doMock('@atlaskit/pragmatic-drag-and-drop-auto-scroll/element', () => ({ autoScrollForElements: vi.fn(() => vi.fn()), @@ -38,13 +38,13 @@ vi.doMock('@atlaskit/pragmatic-drag-and-drop-auto-scroll/element', () => ({ describe('Sortable lists scrollable controller', () => { let autoScrollForElements:typeof autoScrollForElementsFn; let ScrollableController:typeof ScrollableControllerType; - let sortableItemData:typeof sortableItemDataFn; + let sortableDragSourceData:typeof sortableDragSourceDataFn; let ctx:StimulusTestContext; beforeAll(async () => { ({ autoScrollForElements } = await import('@atlaskit/pragmatic-drag-and-drop-auto-scroll/element')); ({ default: ScrollableController } = await import('./scrollable.controller')); - ({ sortableItemData } = await import('./drag-and-drop')); + ({ sortableDragSourceData } = await import('./drag-and-drop')); }); beforeEach(async () => { @@ -99,7 +99,7 @@ describe('Sortable lists scrollable controller', () => { it('refuses to scroll before a root is connected', async () => { const { element } = await mount(); const options = vi.mocked(autoScrollForElements).mock.lastCall?.[0]; - expect(options?.canScroll?.(scrollArgs(element, sortableItemData({ itemId: '1', type: 'work_package', rootElement: element })))).toBe(false); + expect(options?.canScroll?.(scrollArgs(element, sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: element })))).toBe(false); }); it('scrolls only for sortable items owned by the connected root', async () => { @@ -108,8 +108,8 @@ describe('Sortable lists scrollable controller', () => { controller.connectRoot(stubRoot(root)); const options = vi.mocked(autoScrollForElements).mock.lastCall?.[0]; - expect(options?.canScroll?.(scrollArgs(element, sortableItemData({ itemId: '1', type: 'work_package', rootElement: root })))).toBe(true); - expect(options?.canScroll?.(scrollArgs(element, sortableItemData({ itemId: '1', type: 'work_package', rootElement: document.createElement('div') })))).toBe(false); + expect(options?.canScroll?.(scrollArgs(element, sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: root })))).toBe(true); + expect(options?.canScroll?.(scrollArgs(element, sortableDragSourceData({ itemId: '1', type: 'work_package', rootElement: document.createElement('div') })))).toBe(false); expect(options?.canScroll?.(scrollArgs(element, { type: 'unrelated' }))).toBe(false); });