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..cc6d115873a0 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -63,9 +63,10 @@ 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, + sortableDragSourceData as sortableDragSourceDataFn, sortableListData as sortableListDataFn, } from './sortable-lists/drag-and-drop'; @@ -74,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; @@ -89,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 } = {}) { @@ -217,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()) { @@ -775,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); }); @@ -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 }:{ @@ -2407,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'], @@ -2476,6 +2493,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', () => { @@ -2520,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)); @@ -2527,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 @@ -2555,11 +2614,11 @@ 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, - permittedDestinations: controller.dragPermittedDestinations(source), + permittedDestinations: currentSession?.permittedDestinations() ?? null, })), location: { initial: { dropTargets: [], input: input() }, @@ -2574,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 @@ -2613,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 () => { @@ -2668,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']); }); @@ -2917,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 () => { @@ -2940,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 () => { @@ -2965,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); @@ -3068,11 +3157,12 @@ 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' }), { + 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.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.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 600f4d84b726..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 @@ -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, @@ -58,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 @@ -108,23 +112,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 @@ -138,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[] { @@ -147,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' @@ -161,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, @@ -171,7 +167,7 @@ export function sortableItemData({ itemId:string; rootElement?:HTMLElement|null; permittedDestinations?:DestinationIdentity[]|null; -}):SortableItemData { +}):SortableDragSourceData { return { ...sortableItemIdentity({ type, itemId }), rootElement, @@ -229,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 @@ -251,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 @@ -336,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/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..ca361c587bad --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/drag-session.ts @@ -0,0 +1,196 @@ +//-- 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. 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[]; + + 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; + } + + // 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( + 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/item.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts index 30a1563b40ad..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 @@ -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; @@ -67,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 { @@ -81,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) { @@ -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, { @@ -504,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); @@ -520,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); @@ -537,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); @@ -553,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); @@ -569,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); @@ -587,7 +632,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: root, @@ -655,7 +700,7 @@ describe('Sortable lists item controller', () => { element: targetElement, input: {} as never, source: { - data: sortableItemData({ + data: sortableDragSourceData({ type: 'item', itemId: '456', rootElement: root, @@ -675,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); @@ -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..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 { @@ -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); @@ -392,16 +404,16 @@ 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, // 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..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 () => { @@ -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]), }; } @@ -130,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; } @@ -296,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); @@ -353,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 10209dda3f3c..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 () => { @@ -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]), }; } @@ -102,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 () => { @@ -111,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); }); 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)