diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts index e70d11e671c6..c265dcaeed1c 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.spec.ts @@ -33,6 +33,7 @@ import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test- import type SplitViewSyncControllerType from './split-view-sync.controller'; const MOVED_EVENT = 'op-dispatched:backlogs:work-package-moved'; +const SORTABLE_MOVED_EVENT = 'sortable-lists:moved'; describe('Backlogs split-view-sync controller', () => { let ctx:StimulusTestContext; @@ -77,7 +78,7 @@ describe('Backlogs split-view-sync controller', () => { async function renderHost() { await ctx.mount(`
+ data-action="${MOVED_EVENT}@document->backlogs--split-view-sync#onWorkPackageMoved ${SORTABLE_MOVED_EVENT}@document->backlogs--split-view-sync#onSortableListsMoved"> `); const host = ctx.container.querySelector('[data-controller="backlogs--split-view-sync"]')!; @@ -91,6 +92,10 @@ describe('Backlogs split-view-sync controller', () => { document.dispatchEvent(new CustomEvent(MOVED_EVENT, { detail })); } + function dispatchSortableMoved(detail:object) { + document.dispatchEvent(new CustomEvent(SORTABLE_MOVED_EVENT, { detail })); + } + it('refreshes the moved work package cache when it is loaded', async () => { await renderHost(); @@ -158,4 +163,25 @@ describe('Backlogs split-view-sync controller', () => { expect(refresh).not.toHaveBeenCalled(); }); + + it('refreshes a cached work package on sortable-lists:moved', async () => { + await renderHost(); + + dispatchSortableMoved({ itemId: '1234' }); + + await waitFor(() => { + expect(state).toHaveBeenCalledWith('1234'); + expect(id).toHaveBeenCalledWith('1234'); + expect(refresh).toHaveBeenCalled(); + }); + }); + + it('ignores a sortable-lists:moved event without an item id', async () => { + await renderHost(); + + dispatchSortableMoved({}); + await ctx.nextFrame(); + + expect(refresh).not.toHaveBeenCalled(); + }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts index 33b9c294229a..2a588f2e3d93 100644 --- a/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/backlogs/split-view-sync.controller.ts @@ -32,9 +32,12 @@ import { useAngularServices, type ServiceKey } from 'core-stimulus/mixins/use-an // A split view open on a moved work package caches the lock_version it fetched // on opening, so the next edit after a move would fail with a -// conflicting-modifications error. The server signals every successful move via -// a document event; this controller refreshes the moved work package in the -// Angular cache so the split view stays editable. +// conflicting-modifications error. Every successful move is signalled via a +// document event, either dispatched by the server (cross-list moves) or by +// the client-side sortable-lists controller (same-list moves, which resolve +// with a 204 and never reach the server-dispatched event); this controller +// refreshes the moved work package in the Angular cache so the split view +// stays editable. export default class SplitViewSyncController extends Controller { static services:ServiceKey[] = ['apiV3Service']; @@ -55,11 +58,25 @@ export default class SplitViewSyncController extends Controller { // is correct as well. onWorkPackageMoved(event:CustomEvent<{ work_package_id?:number }>):void { const workPackageId = event.detail?.work_package_id; - // apiV3Service is wired asynchronously via useAngularServices, so it may be absent - // if the event somehow fires before the services resolve. - if (workPackageId === undefined || !this.apiV3Service) { return; } + if (workPackageId === undefined) { return; } + this.refreshWorkPackage(workPackageId.toString()); + } + + // Bound to the client-dispatched `sortable-lists:moved` document event. Same + // refresh as onWorkPackageMoved: optimistic same-list moves answer with 204, + // so no server-dispatched moved event exists on that path. On cross-list + // moves both events fire; the double refresh is idempotent. + onSortableListsMoved(event:CustomEvent<{ itemId?:string }>):void { + const itemId = event.detail?.itemId; + if (itemId === undefined) { return; } + this.refreshWorkPackage(itemId); + } + + private refreshWorkPackage(id:string):void { + // apiV3Service is wired asynchronously via useAngularServices, so it may be + // absent if an event somehow fires before the services resolve. + if (!this.apiV3Service) { return; } - const id = workPackageId.toString(); const { work_packages: workPackages } = this.apiV3Service; if (workPackages.cache.state(id).hasValue()) { 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 3027c4935aec..79b1e09fce2a 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -64,24 +64,6 @@ describe('Sortable lists controller', () => { autoScrollForElements: vi.fn(() => vi.fn()), })); - // This spec mounts the real item controller, which pulls in these modules. - // Tests share one module registry (the runner does not isolate spec files), - // so importing the real versions here would leak into the item controller - // spec and break its spies. Mock them to keep the shared cache inert. - vi.doMock('@atlaskit/pragmatic-drag-and-drop/combine', () => ({ - combine: vi.fn((...cleanups:(() => void)[]) => vi.fn(() => { - cleanups.forEach((cleanup) => cleanup()); - })), - })); - - vi.doMock('@atlaskit/pragmatic-drag-and-drop/prevent-unhandled', () => ({ - preventUnhandled: { start: vi.fn(), stop: vi.fn() }, - })); - - vi.doMock('@atlaskit/pragmatic-drag-and-drop/element/set-custom-native-drag-preview', () => ({ - setCustomNativeDragPreview: vi.fn(), - })); - ({ monitorForElements } = await import('@atlaskit/pragmatic-drag-and-drop/element/adapter')); ({ autoScrollForElements } = await import('@atlaskit/pragmatic-drag-and-drop-auto-scroll/element')); ({ default: SortableListsController } = await import('./sortable-lists.controller')); @@ -103,26 +85,27 @@ describe('Sortable lists controller', () => { }; } - function itemRow(id:string):HTMLLIElement { + // The root spec never mounts the real list/item controllers: they are + // covered by their own dedicated spec files. Stimulus silently ignores + // data-controller identifiers with no registered module, so plain markup + // carrying those attributes is enough for the DOM-contract helpers + // (resolveSourceRow, resolveDropIntent, ...) that only look at attributes. + function itemRow(id:string, type = 'work_package'):HTMLLIElement { const row = document.createElement('li'); row.setAttribute('data-controller', 'sortable-lists--item'); row.setAttribute('data-sortable-lists--item-id-value', id); - row.setAttribute('data-sortable-lists--item-type-value', 'work_package'); + row.setAttribute('data-sortable-lists--item-type-value', type); return row; } function renderFixture({ - acceptedType = null, - moveUrlTemplate = '/move/{id}', - }:{ acceptedType?:string|null; moveUrlTemplate?:string|null } = {}) { + moveUrlTemplates = { work_package: '/move/{id}' }, + }:{ moveUrlTemplates?:Record|null } = {}) { fixture.innerHTML = `
      @@ -135,6 +118,12 @@ describe('Sortable lists controller', () => { return { root, sourceList, targetList, firstSourceItem: sourceList.querySelector('[data-sortable-lists--item-id-value="1"]')! }; } + function truncationMarkerRow(previousItemId = 'hidden-item'):HTMLLIElement { + const row = document.createElement('li'); + row.setAttribute('data-sortable-lists-prev-item-id', previousItemId); + return row; + } + function renderScrollableFixture(values = '') { fixture.innerHTML = `
      @@ -151,7 +140,10 @@ describe('Sortable lists controller', () => { monitorOptions?.onDrop?.({ source: sourcePayload( sourceElement, - itemData(sourceElement.getAttribute('data-sortable-lists--item-id-value')!), + itemData( + sourceElement.getAttribute('data-sortable-lists--item-id-value')!, + sourceElement.getAttribute('data-sortable-lists--item-type-value') ?? 'work_package', + ), ), location: { initial: { @@ -219,8 +211,6 @@ describe('Sortable lists controller', () => { ctx = await setupStimulusTest({ controllers: { 'sortable-lists': SortableListsController, - 'sortable-lists--list': (await import('./sortable-lists/list.controller')).default, - 'sortable-lists--item': (await import('./sortable-lists/item.controller')).default, }, }); fixture = ctx.container; @@ -246,10 +236,7 @@ describe('Sortable lists controller', () => {
      • @@ -258,10 +245,7 @@ describe('Sortable lists controller', () => {
        • @@ -313,9 +297,32 @@ describe('Sortable lists controller', () => { expect(options.body.get('prev_id')).toEqual('5'); }); - it('builds the move URL from the controller URI template', async () => { + it('marks the move optimistic when the target list has no truncation marker row', async () => { + const { targetList, firstSourceItem } = renderFixture(); + + await ctx.nextFrame(); + await dropCurrentItemOnList(firstSourceItem, targetList); + + const options = fetchMock.mock.lastCall?.[1] as { body:FormData }; + + expect(options.body.get('optimistic')).toEqual('true'); + }); + + it('does not mark the move optimistic when the target list has a truncation marker row', async () => { + const { targetList, firstSourceItem } = renderFixture(); + targetList.append(truncationMarkerRow()); + + await ctx.nextFrame(); + await dropCurrentItemOnList(firstSourceItem, targetList); + + const options = fetchMock.mock.lastCall?.[1] as { body:FormData }; + + expect(options.body.get('optimistic')).toBeNull(); + }); + + it('resolves the move url from the template matching the item type', async () => { const { targetList, firstSourceItem } = renderFixture({ - moveUrlTemplate: '/projects/demo/backlogs/work_packages/{id}/move', + moveUrlTemplates: { work_package: '/projects/demo/backlogs/work_packages/{id}/move' }, }); await ctx.nextFrame(); @@ -327,16 +334,27 @@ describe('Sortable lists controller', () => { ); }); - it('adds the current turbo frame query to the move URL', async () => { + it('ignores a drop whose item type has no move url template', async () => { + const { sourceList, targetList } = renderFixture({ + moveUrlTemplates: { work_package: '/move/{id}' }, + }); + const unknownItem = itemRow('9', 'unknown'); + sourceList.append(unknownItem); + + await ctx.nextFrame(); + await dropCurrentItemOnList(unknownItem, targetList); + + expect(fetchMock).not.toHaveBeenCalled(); + expect(itemIds(targetList)).toEqual(['4', '5']); + }); + + it('keeps the current turbo-frame query params on the resolved move url', async () => { fixture.innerHTML = `
              @@ -361,62 +379,6 @@ describe('Sortable lists controller', () => { ); }); - it('does nothing when the controller has no move URL template', async () => { - const { targetList, firstSourceItem } = renderFixture({ moveUrlTemplate: null }); - - await ctx.nextFrame(); - await dropCurrentItemOnList(firstSourceItem, targetList); - - expect(fetchMock).not.toHaveBeenCalled(); - }); - - it('marks the moving state and busy lists while moving an item', async () => { - let resolveMove:(response:Response) => void; - - fetchMock.mockImplementationOnce(() => { - return new Promise((resolve) => { - resolveMove = resolve; - }); - }); - - const { root, targetList, firstSourceItem } = renderFixture(); - - await ctx.nextFrame(); - await dropCurrentItemOnList(firstSourceItem, targetList); - - expect(root.dataset.sortableListsMoving).toEqual('true'); - expect(targetList.getAttribute('aria-busy')).toEqual('true'); - - resolveMove!(new Response('', { status: 200 })); - await flushPromises(); - - expect(root.hasAttribute('data-sortable-lists-moving')).toBe(false); - expect(targetList.hasAttribute('aria-busy')).toBe(false); - }); - - it('rejects new sortable-list drags and drops while a move is pending', async () => { - let resolveMove:(response:Response) => void; - - fetchMock.mockImplementationOnce(() => { - return new Promise((resolve) => { - resolveMove = resolve; - }); - }); - - const { targetList, firstSourceItem } = renderFixture(); - - await ctx.nextFrame(); - await dropCurrentItemOnList(firstSourceItem, targetList); - - expect(vi.mocked(monitorForElements).mock.lastCall?.[0].canMonitor?.({ - source: sourcePayload(firstSourceItem), - initial: {} as never, - })).toBe(false); - - resolveMove!(new Response('', { status: 200 })); - await flushPromises(); - }); - it('only monitors drags belonging to its own root', async () => { const { root, firstSourceItem } = renderFixture(); @@ -452,7 +414,7 @@ describe('Sortable lists controller', () => { message: expect.any(String), type: 'error', })); - expect(root.hasAttribute('data-sortable-lists-moving')).toBe(false); + expect(root.hasAttribute('aria-busy')).toBe(false); window.removeEventListener('op:toasters:add', onToast); }); @@ -540,6 +502,68 @@ describe('Sortable lists controller', () => { window.removeEventListener('op:toasters:add', onToast); }); + it('dispatches sortable-lists:moved after the move settled', async () => { + const { root, targetList, firstSourceItem } = renderFixture(); + await ctx.nextFrame(); + + const controller = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as unknown as { moving:boolean }; + const events:CustomEvent[] = []; + let movingAtDispatch:boolean|undefined; + const onMoved = (event:Event) => { + events.push(event as CustomEvent); + movingAtDispatch = controller.moving; + }; + document.addEventListener('sortable-lists:moved', onMoved); + + await dropCurrentItemOnList(firstSourceItem, targetList); + + expect(events).toHaveLength(1); + expect(events[0].detail).toEqual({ itemId: '1' }); + // ordering requirement: moving must already be false when the event fires. + expect(movingAtDispatch).toBe(false); + + document.removeEventListener('sortable-lists:moved', onMoved); + }); + + it('does not dispatch sortable-lists:moved for a failed move', async () => { + fetchMock.mockRejectedValueOnce(new Error('Network failure')); + + const events:CustomEvent[] = []; + const onMoved = (event:Event) => events.push(event as CustomEvent); + document.addEventListener('sortable-lists:moved', onMoved); + + const { targetList, firstSourceItem } = renderFixture(); + await ctx.nextFrame(); + await dropCurrentItemOnList(firstSourceItem, targetList); + await flushPromises(); + + expect(events).toHaveLength(0); + + document.removeEventListener('sortable-lists:moved', onMoved); + }); + + it('sets aria-busy on the root element only while the move request is in flight', async () => { + let resolveMove:(response:Response) => void; + + fetchMock.mockImplementationOnce(() => { + return new Promise((resolve) => { + resolveMove = resolve; + }); + }); + + const { root, targetList, firstSourceItem } = renderFixture(); + + await ctx.nextFrame(); + await dropCurrentItemOnList(firstSourceItem, targetList); + + expect(root.getAttribute('aria-busy')).toEqual('true'); + + resolveMove!(new Response('', { status: 200 })); + await flushPromises(); + + expect(root.hasAttribute('aria-busy')).toBe(false); + }); + it('registers scrollable targets for vertical sortable list auto-scrolling', async () => { const scrollable = renderScrollableFixture(); const root = fixture.querySelector('[data-controller~="sortable-lists"]')!; @@ -592,19 +616,4 @@ describe('Sortable lists controller', () => { expect(scrollableCleanup).toHaveBeenCalledOnce(); }); - - it('hands the root reference to connected list and item controllers', async () => { - const { root, sourceList, firstSourceItem } = renderFixture({ acceptedType: 'work_package' }); - await ctx.nextFrame(); - // Outlet connections happen after the controller frame; a second frame - // ensures the hand-over callbacks have fired. - await ctx.nextFrame(); - - const listController = ctx.application.getControllerForElementAndIdentifier(sourceList, 'sortable-lists--list') as unknown as { ['root']?:unknown }; - const itemController = ctx.application.getControllerForElementAndIdentifier(firstSourceItem, 'sortable-lists--item') as unknown as { ['root']?:unknown }; - const rootController = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists'); - - expect(listController.root).toBe(rootController); - expect(itemController.root).toBe(rootController); - }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index 27b263cf4262..19a1af50f65e 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -40,15 +40,16 @@ import { buildMoveFormData, isSortableItemData, resolveDropIntent, - type RootAwareChild, - type SortableListData, - type SortableListsRoot, + type DropIntent, + type SortableItemData, + type SortablePositionMode, } from './sortable-lists/drag-and-drop'; import { captureRowPositions, + hasTruncationMarkerRow, reorderRows, + resolveSourceRow, restoreRowPositions, - sortableListsMovingAttribute, } from './sortable-lists/list-dom'; type CleanupFn = () => void; @@ -60,36 +61,34 @@ type MoveResult = { ok:true }|{ ok:false; showToast:boolean }; const allowedAxes = new Set(['vertical', 'horizontal', 'all']); const maxScrollSpeeds = new Set(['standard', 'fast']); -export default class SortableListsController extends Controller implements SortableListsRoot { +export default class SortableListsController extends Controller { static targets = ['scrollable']; - static outlets = ['sortable-lists--list', 'sortable-lists--item']; static values = { - acceptedType: String, - moveUrlTemplate: String, + moveUrlTemplates: { type: Object, default: {} }, + positionMode: { type: String, default: 'relative' }, allowedAxis: { type: String, default: 'vertical' }, maxScrollSpeed: { type: String, default: 'standard' }, }; declare readonly scrollableTargets:HTMLElement[]; - declare readonly sortableListsListOutlets:import('./sortable-lists/list.controller').default[]; - declare readonly sortableListsItemOutlets:RootAwareChild[]; - declare readonly acceptedTypeValue:string; - declare readonly hasAcceptedTypeValue:boolean; - declare readonly moveUrlTemplateValue:string; - declare readonly hasMoveUrlTemplateValue:boolean; + declare readonly moveUrlTemplatesValue:Record; + declare readonly positionModeValue:string; declare readonly allowedAxisValue:string; declare readonly maxScrollSpeedValue:string; private monitorCleanupFn?:CleanupFn; private scrollableCleanupFns = new Map(); + private movingFlag = false; + + get moving():boolean { + return this.movingFlag; + } connect():void { this.monitorCleanupFn = monitorForElements({ - canMonitor: ({ source }) => !this.moving - && isSortableItemData(source.data) - && source.data.rootElement === this.element, + canMonitor: ({ source }) => isSortableItemData(source.data) && source.data.rootElement === this.element, onDrop: (args) => { void this.handleDrop(args); }, @@ -103,22 +102,6 @@ export default class SortableListsController extends Controller imp this.scrollableCleanupFns.clear(); } - sortableListsListOutletConnected(list:RootAwareChild):void { - list.connectRoot(this); - } - - sortableListsListOutletDisconnected(list:RootAwareChild):void { - list.disconnectRoot(); - } - - sortableListsItemOutletConnected(item:RootAwareChild):void { - item.connectRoot(this); - } - - sortableListsItemOutletDisconnected(item:RootAwareChild):void { - item.disconnectRoot(); - } - scrollableTargetConnected(element:HTMLElement):void { const cleanup = autoScrollForElements({ element, @@ -139,33 +122,15 @@ export default class SortableListsController extends Controller imp return allowedAxes.has(this.allowedAxisValue) ? this.allowedAxisValue as AutoScrollAllowedAxis : 'vertical'; } - get acceptedType():string|null { - // The accepted type is scoped to this controller instance, so every list - // outlet inside one sortable-lists root accepts the same sortable item type. - return this.hasAcceptedTypeValue ? this.acceptedTypeValue : null; - } - private get maxScrollSpeed():AutoScrollMaxScrollSpeed { return maxScrollSpeeds.has(this.maxScrollSpeedValue) ? this.maxScrollSpeedValue as AutoScrollMaxScrollSpeed : 'standard'; } - get moving():boolean { - return this.element.hasAttribute(sortableListsMovingAttribute); - } - private async handleDrop({ location, source }:ElementDropPayload) { - if (this.moving) { - return; - } - if (!isSortableItemData(source.data) || !(source.element instanceof HTMLElement)) { return; } - if (!this.element.contains(source.element)) { - return; - } - const moveUrl = this.resolveMoveUrl(source.data); if (!moveUrl) { return; @@ -181,45 +146,54 @@ export default class SortableListsController extends Controller imp return; } - const sourceRow = source.element.closest('li'); - if (!(sourceRow instanceof HTMLElement)) { + const sourceRow = resolveSourceRow(source.element); + if (!sourceRow) { return; } - // Move the row optimistically, then persist. The server response (a - // turbo-stream) reconciles the list on success; a failure rolls the row - // back to where it started. + // Move the row optimistically, then persist. A same-list move into a + // non-truncated list is answered with 204 and this DOM order is final; a + // cross-list move, or a same-list move into a truncated list (whose + // visible window is server-computed), gets a turbo-stream frame reload + // that reconciles the list. A failure rolls the row back to where it + // started. const rows = [sourceRow]; const rollback = captureRowPositions(rows); - reorderRows({ rows, list: intent.listElement, previousItemId: intent.previousItemId }); + reorderRows({ rows, container: intent.rowsContainer, previousItemId: intent.previousItemId }); - const result = await this.moveItem({ - listData: intent.listData, - previousItemId: intent.previousItemId, - moveUrl, - }); + const optimistic = !hasTruncationMarkerRow(intent.rowsContainer); + const result = await this.moveItem({ intent, moveUrl, optimistic }); - if (!result.ok) { - try { - flipMove(rows, () => restoreRowPositions(rollback)); - } catch (error) { - debugLog('Failed to roll back sortable list item move', error); - } + if (result.ok) { + // After moveItem's finally: moving is false again, so listeners resumed + // by this event (split-view sync, feature-spec waits) observe a root + // that accepts the next drag. + this.dispatch('moved', { detail: { itemId: source.data.itemId } }); + return; + } - if (result.showToast) { - this.dispatchErrorToast(); - } + try { + flipMove(rows, () => restoreRowPositions(rollback)); + } catch (error) { + debugLog('Failed to roll back sortable list item move', error); + } + + if (result.showToast) { + this.dispatchErrorToast(); } } - private resolveMoveUrl(data:{ itemId:string }):string|null { - if (this.hasMoveUrlTemplateValue) { - return this.withCurrentFrameQuery( - parseTemplate(this.moveUrlTemplateValue).expand({ id: data.itemId }), - ); + private resolveMoveUrl(data:SortableItemData):string|null { + const template = this.moveUrlTemplatesValue[data.type]; + if (!template) { + return null; } - return null; + return this.withCurrentFrameQuery(parseTemplate(template).expand({ id: data.itemId })); + } + + private get positionMode():SortablePositionMode { + return this.positionModeValue === 'absolute' ? 'absolute' : 'relative'; } private withCurrentFrameQuery(moveUrl:string):string { @@ -244,26 +218,18 @@ export default class SortableListsController extends Controller imp } private async moveItem({ - listData, - previousItemId, + intent, moveUrl, + optimistic, }:{ - listData:SortableListData; - previousItemId:string|null; + intent:DropIntent; moveUrl:string; + optimistic:boolean; }):Promise { - const request = new FetchRequest( - 'put', - moveUrl, - { - body: buildMoveFormData({ - listId: listData.listId, - previousItemId, - type: listData.type, - }), - responseKind: 'turbo-stream', - }, - ); + const request = new FetchRequest('put', moveUrl, { + body: buildMoveFormData({ intent, positionMode: this.positionMode, optimistic }), + responseKind: 'turbo-stream', + }); this.setMoving(true); try { @@ -284,13 +250,15 @@ export default class SortableListsController extends Controller imp } } + // aria-busy on the root covers all its lists; per-list precision was not + // needed (review consensus), which lets the root avoid tracking children. private setMoving(moving:boolean):void { + this.movingFlag = moving; if (moving) { - this.element.setAttribute(sortableListsMovingAttribute, 'true'); + this.element.setAttribute('aria-busy', 'true'); } else { - this.element.removeAttribute(sortableListsMovingAttribute); + this.element.removeAttribute('aria-busy'); } - this.sortableListsListOutlets.forEach((list) => list.reflectMoving(moving)); } private dispatchErrorToast():void { 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 c1b1b50363bd..45d4e9b45714 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 @@ -31,10 +31,12 @@ import { vi } from 'vitest'; import { attachClosestEdge } from '@atlaskit/pragmatic-drag-and-drop-hitbox/closest-edge'; import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/types'; import { - acceptsSortableItemType, buildMoveFormData, + type DropIntent, + isItemFromRoot, isSortableItemData, isSortableListData, + listAcceptsType, resolveDropIntent, resolvePreviousSortableItemId, sortableItemData, @@ -146,64 +148,112 @@ describe('sortable lists drag and drop helpers', () => { }); }); - describe('acceptsSortableItemType', () => { - it('allows drops when the controller has no accepted type filter', () => { - expect(acceptsSortableItemType({ acceptedType: null, type: 'work_package' })).toBe(true); + describe('isItemFromRoot', () => { + const root = document.createElement('div'); + + it('accepts a sortable item tagged with the identical root element', () => { + expect(isItemFromRoot(root, sortableItemData({ type: 'work_package', itemId: '1', rootElement: root }))).toBe(true); }); - it('allows drops when the source type matches the accepted type', () => { - expect(acceptsSortableItemType({ acceptedType: 'work_package', type: 'work_package' })).toBe(true); + it('rejects an item tagged with a different root element', () => { + expect(isItemFromRoot(root, sortableItemData({ type: 'work_package', itemId: '1', rootElement: document.createElement('div') }))).toBe(false); }); - it('rejects drops when the source type does not match the accepted type', () => { - expect(acceptsSortableItemType({ acceptedType: 'work_package', type: 'meeting_agenda_item' })).toBe(false); + it('rejects an item without a root element', () => { + expect(isItemFromRoot(root, sortableItemData({ type: 'work_package', itemId: '1' }))).toBe(false); }); - }); - describe('buildMoveFormData', () => { - it('serializes list data and previous item id for the move endpoint', () => { - const data = buildMoveFormData({ type: 'backlog_bucket', listId: '7', previousItemId: '12' }); + it('rejects a null root', () => { + expect(isItemFromRoot(null, sortableItemData({ type: 'work_package', itemId: '1', rootElement: root }))).toBe(false); + }); - expect(data.get('list_type')).toEqual('backlog_bucket'); - expect(data.get('list_id')).toEqual('7'); - expect(data.get('prev_id')).toEqual('12'); + it('rejects non-item payloads', () => { + expect(isItemFromRoot(root, { anything: true })).toBe(false); + }); + }); + + describe('listAcceptsType', () => { + it('accepts a type contained in acceptedTypes', () => { + expect(listAcceptsType({ acceptedTypes: ['work_package', 'sprint'], type: 'sprint' })).toBe(true); }); - it('serializes a top-of-list move as an empty previous item id', () => { - const data = buildMoveFormData({ type: 'inbox', listId: null, previousItemId: null }); + it('rejects a type not contained in acceptedTypes', () => { + expect(listAcceptsType({ acceptedTypes: ['work_package'], type: 'sprint' })).toBe(false); + }); - expect(data.get('list_type')).toEqual('inbox'); - expect(data.get('list_id')).toEqual(''); - expect(data.get('prev_id')).toEqual(''); + it('rejects everything for an empty acceptedTypes', () => { + expect(listAcceptsType({ acceptedTypes: [], type: 'work_package' })).toBe(false); }); }); - describe('resolvePreviousSortableItemId', () => { - it('uses the target item as previous item when dropping on the bottom edge', () => { - const target = itemRow('3').querySelector('article')!; + function intentFixture({ listId = '42', previousItemId = 'a' }:{ listId?:string|null; previousItemId?:string|null } = {}):DropIntent { + const listElement = document.createElement('div'); + const container = document.createElement('ul'); + container.innerHTML = ` +
            • +
            • `; + listElement.append(container); - expect(resolvePreviousSortableItemId({ sourceItemId: '1', targetItem: target, closestEdge: 'bottom' })).toEqual('3'); + return { + listElement, + rowsContainer: container, + previousItemId, + listData: sortableListData({ type: 'sprint', listId, dropPosition: 'end' }), + }; + } + + describe('buildMoveFormData', () => { + it('builds a relative payload from the intent', () => { + const data = buildMoveFormData({ intent: intentFixture(), positionMode: 'relative', optimistic: true }); + expect(data.get('list_type')).toBe('sprint'); + expect(data.get('list_id')).toBe('42'); + expect(data.get('prev_id')).toBe('a'); + expect(data.get('optimistic')).toBe('true'); + expect(data.get('position')).toBeNull(); }); - it('uses the row item as previous item when the drop target is the row', () => { - const target = itemRow('3'); + it('serializes a null list id and previous item as empty strings', () => { + const data = buildMoveFormData({ + intent: intentFixture({ listId: null, previousItemId: null }), + positionMode: 'relative', + optimistic: true, + }); + expect(data.get('list_id')).toBe(''); + expect(data.get('prev_id')).toBe(''); + }); - expect(resolvePreviousSortableItemId({ sourceItemId: '1', targetItem: target, closestEdge: 'bottom' })).toEqual('3'); + it('builds an absolute payload with a computed position', () => { + const data = buildMoveFormData({ + intent: intentFixture({ previousItemId: 'a' }), + positionMode: 'absolute', + optimistic: true, + }); + expect(data.get('target_id')).toBe('42'); + expect(data.get('position')).toBe('2'); + expect(data.get('optimistic')).toBe('true'); + expect(data.get('prev_id')).toBeNull(); }); - it('uses the previous row item when dropping on the top edge', () => { - const list = document.createElement('ul'); - const first = itemRow('1'); - const targetRow = itemRow('3'); - const target = targetRow.querySelector('article')!; + it('omits the optimistic param when the move is not optimistic', () => { + const data = buildMoveFormData({ intent: intentFixture(), positionMode: 'relative', optimistic: false }); + expect(data.get('optimistic')).toBeNull(); + }); + }); - list.append(first, targetRow); + // The drop target Pragmatic DnD reports is always the item controller's own + // registered element, which itself carries data-sortable-lists--item-id-value + // (resolveItemElement resolves self-or-descendant only, never an ancestor) — + // so fixtures below pass the item row itself rather than a nested descendant. + describe('resolvePreviousSortableItemId', () => { + it('uses the target item as previous item when dropping on the bottom edge', () => { + const target = itemRow('3'); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top' })).toEqual('1'); + expect(resolvePreviousSortableItemId({ sourceItemId: '1', targetItem: target, closestEdge: 'bottom' })).toEqual('3'); }); - it('uses the previous row item when dropping on the top edge of a row target', () => { + it('uses the previous row item when dropping on the top edge', () => { const list = document.createElement('ul'); + list.setAttribute('data-controller', 'sortable-lists--list'); const first = itemRow('1'); const targetRow = itemRow('3'); @@ -214,40 +264,40 @@ describe('sortable lists drag and drop helpers', () => { it('treats a missing closest edge as dropping before the target item', () => { const list = document.createElement('ul'); + list.setAttribute('data-controller', 'sortable-lists--list'); const first = itemRow('1'); const targetRow = itemRow('3'); - const target = targetRow.querySelector('article')!; list.append(first, targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: null })).toEqual('1'); + expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: targetRow, closestEdge: null })).toEqual('1'); }); it('uses a truncation marker when dropping before a tail item', () => { const list = document.createElement('ul'); + list.setAttribute('data-controller', 'sortable-lists--list'); const first = itemRow('1'); const targetRow = itemRow('6'); - const target = targetRow.querySelector('article')!; list.append(first, showMoreRow('5'), targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top' })).toEqual('5'); + expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: targetRow, closestEdge: 'top' })).toEqual('5'); }); it('skips the source item and uses a preceding truncation marker when resolving the previous item', () => { const list = document.createElement('ul'); + list.setAttribute('data-controller', 'sortable-lists--list'); const first = itemRow('1'); const source = itemRow('2'); const targetRow = itemRow('3'); - const target = targetRow.querySelector('article')!; list.append(first, showMoreRow(), source, targetRow); - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top' })).toEqual('hidden-item'); + expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: targetRow, closestEdge: 'top' })).toEqual('hidden-item'); }); it('returns null when dropping before the first item', () => { - const target = itemRow('1').querySelector('article')!; + const target = itemRow('1'); expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top' })).toBeNull(); }); @@ -307,6 +357,7 @@ describe('sortable lists drag and drop helpers', () => { expect(intent?.listElement).toBe(list); expect(intent?.listData).toEqual(expect.objectContaining({ type: 'backlog_bucket', listId: '7' })); + expect(intent?.rowsContainer).toBe(list); expect(intent?.previousItemId).toEqual('2'); }); @@ -356,6 +407,34 @@ describe('sortable lists drag and drop helpers', () => { expect(intent?.listElement).toBe(list); expect(intent?.listData).toEqual(expect.objectContaining({ type: 'backlog_bucket', listId: '7' })); + expect(intent?.rowsContainer).toBe(list); + expect(intent?.previousItemId).toEqual('5'); + }); + + it('resolves an end drop against the rows container rather than the list element itself', () => { + const { root, list } = buildList(); + const sourceList = document.createElement('ul'); + const source = itemRow('1'); + const header = document.createElement('header'); + const rowsContainer = document.createElement('ul'); + + list.setAttribute('data-sortable-lists--list-rows-container-selector-value', ':scope > ul'); + sourceList.setAttribute('data-controller', 'sortable-lists--list'); + sourceList.append(source); + rowsContainer.append(itemRow('4'), itemRow('5')); + list.append(header, rowsContainer); + root.append(sourceList); + + const intent = resolveDropIntent({ + location: dropLocation({ + dropTargets: [{ data: sortableListData({ type: 'backlog_bucket', listId: '7' }), element: list }], + }), + root, + sourceElement: source, + sourceData: sortableItemData({ type: 'work_package', itemId: '1' }), + }); + + expect(intent?.rowsContainer).toBe(rowsContainer); expect(intent?.previousItemId).toEqual('5'); }); @@ -379,6 +458,7 @@ describe('sortable lists drag and drop helpers', () => { }); expect(intent?.listElement).toBe(list); + expect(intent?.rowsContainer).toBe(list); expect(intent?.previousItemId).toBeNull(); }); 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 baeb539ff5f4..d4d9e21b5100 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 @@ -34,8 +34,11 @@ import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/type import { resolveItemElement, resolveItemId, + resolveItemPosition, resolveListAppendPreviousItemId, resolvePreviousItemId, + resolveRowsContainer, + resolveSourceRow, sortableListSelector, } from './list-dom'; @@ -61,22 +64,6 @@ export interface SortableListData extends Record { dropPosition:SortableListDropPosition; } -// Implemented by the sortable-lists root controller and handed to list/item -// controllers via outlet callbacks, so children read shared state through a -// typed reference instead of walking the DOM. -export interface SortableListsRoot { - readonly element:HTMLElement; - readonly moving:boolean; - readonly acceptedType:string|null; -} - -// Implemented by the list and item controllers so the root can hand them its -// reference (and revoke it) through outlet-connected callbacks. -export interface RootAwareChild { - connectRoot(root:SortableListsRoot):void; - disconnectRoot():void; -} - export function isSortableItemData(data:Record):data is SortableItemData { return data[sortableItemDataKey] === true && typeof data.type === 'string' @@ -126,47 +113,64 @@ export function sortableListData({ }; } +export type SortablePositionMode = 'relative'|'absolute'; + +// One builder for both payload shapes so call sites hand over the whole drop +// intent; the row has always already been reordered in the DOM, but the +// server should only treat that reorder as final when `optimistic` is true — +// truncated target lists opt out because the server must re-render the +// visible window (which rows show, the truncation marker's metadata) rather +// than trust the client's optimistic DOM order. export function buildMoveFormData({ - listId, - previousItemId, - type, + intent, + positionMode, + optimistic, }:{ - listId:string|null; - previousItemId:string|null; - type:string; + intent:DropIntent; + positionMode:SortablePositionMode; + optimistic:boolean; }):FormData { const data = new FormData(); + if (optimistic) { + data.append('optimistic', 'true'); + } - data.append('list_type', type); - data.append('list_id', listId ?? ''); - data.append('prev_id', previousItemId ?? ''); + if (positionMode === 'absolute') { + data.append('target_id', intent.listData.listId ?? ''); + data.append('position', String(resolveItemPosition({ + container: intent.rowsContainer, + previousItemId: intent.previousItemId, + }))); + } else { + data.append('list_type', intent.listData.type); + data.append('list_id', intent.listData.listId ?? ''); + data.append('prev_id', intent.previousItemId ?? ''); + } return data; } -export function acceptsSortableItemType({ - acceptedType, +// The shared root-scoping rule: the payload must be a sortable item created by +// an item controller wired to this exact root element. Identity (===), not +// containment — containment would wrongly accept an outer root's item over an +// inner root's surface when roots nest. +export function isItemFromRoot( + rootElement:HTMLElement|null, + data:Record, +):data is SortableItemData { + return rootElement != null + && isSortableItemData(data) + && data.rootElement === rootElement; +} + +export function listAcceptsType({ + acceptedTypes, type, }:{ - acceptedType:string|null; + acceptedTypes:string[]; type:string; }):boolean { - return acceptedType === null || acceptedType === type; -} - -// The drop rule shared by list and item drop targets: the payload must be a -// sortable item belonging to this same root and of an accepted type. Item -// targets additionally exclude themselves before calling this. -export function canAccept(root:SortableListsRoot, data:Record):boolean { - if (!isSortableItemData(data)) { - return false; - } - - if (data.rootElement == null || data.rootElement !== root.element) { - return false; - } - - return acceptsSortableItemType({ acceptedType: root.acceptedType, type: data.type }); + return acceptedTypes.includes(type); } export function isSourceListTarget({ @@ -195,7 +199,7 @@ export function resolvePreviousSortableItemId({ return targetItemId; } - const targetRow = (targetItemElement ?? targetItem).closest('li'); + const targetRow = resolveSourceRow(targetItemElement ?? targetItem); let row = targetRow?.previousElementSibling ?? null; while (row) { @@ -215,23 +219,24 @@ export function resolvePreviousSortableItemId({ // (null previous item), 'end' appends after the last. function resolveListOnlyPreviousItemId({ sourceItemId, - list, + container, dropPosition, }:{ sourceItemId:string; - list:HTMLElement; + container:HTMLElement; dropPosition:SortableListDropPosition; }):string|null { if (dropPosition === 'start') { return null; } - return resolveListAppendPreviousItemId({ sourceItemId, list }); + return resolveListAppendPreviousItemId({ sourceItemId, container }); } export interface DropIntent { listElement:HTMLElement; listData:SortableListData; + rowsContainer:HTMLElement; previousItemId:string|null; } @@ -270,6 +275,8 @@ export function resolveDropIntent({ return null; } + const rowsContainer = resolveRowsContainer(listElement); + const previousItemId = targetItem?.element instanceof HTMLElement ? resolvePreviousSortableItemId({ sourceItemId: sourceData.itemId, @@ -278,9 +285,14 @@ export function resolveDropIntent({ }) : resolveListOnlyPreviousItemId({ sourceItemId: sourceData.itemId, - list: listElement, + container: rowsContainer, dropPosition: listData.dropPosition, }); - return { listElement, listData, previousItemId }; + return { + listElement, + listData, + rowsContainer, + previousItemId, + }; } 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 41b947853648..43ae659c9742 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 @@ -31,14 +31,20 @@ import type { setCustomNativeDragPreview as setCustomNativeDragPreviewFn } from import type { preventUnhandled as preventUnhandledType } from '@atlaskit/pragmatic-drag-and-drop/prevent-unhandled'; import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; import type ItemControllerType from './item.controller'; -import type { SortableListsRoot } from './drag-and-drop'; +import type SortableListsControllerType from '../sortable-lists.controller'; +// The item controller resolves its root through a real Stimulus outlet, so +// both the root (`sortable-lists.controller.ts`) and the item controller are +// registered here and wired together through the +// `data-sortable-lists--item-sortable-lists-outlet` +// attribute, exactly as production markup does. describe('Sortable lists item controller', () => { let draggable:typeof draggableFn; let dropTargetForElements:typeof dropTargetForElementsFn; let preventUnhandled:typeof preventUnhandledType; let setCustomNativeDragPreview:typeof setCustomNativeDragPreviewFn; let ItemController:typeof ItemControllerType; + let SortableListsController:typeof SortableListsControllerType; let sortableItemData:typeof import('./drag-and-drop').sortableItemData; interface TestItemController { @@ -46,6 +52,9 @@ describe('Sortable lists item controller', () => { clearDropIndicator():void; } + let ctx:StimulusTestContext; + let fixture:HTMLElement; + beforeAll(async () => { vi.doMock('@atlaskit/pragmatic-drag-and-drop/combine', () => ({ combine: vi.fn((...cleanups:(() => void)[]) => vi.fn(() => { @@ -59,6 +68,10 @@ describe('Sortable lists item controller', () => { monitorForElements: vi.fn(() => vi.fn()), })); + vi.doMock('@atlaskit/pragmatic-drag-and-drop-auto-scroll/element', () => ({ + autoScrollForElements: vi.fn(() => vi.fn()), + })); + vi.doMock('@atlaskit/pragmatic-drag-and-drop/prevent-unhandled', () => ({ preventUnhandled: { start: vi.fn(), @@ -74,9 +87,31 @@ 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')); + ({ default: SortableListsController } = await import('../sortable-lists.controller')); ({ sortableItemData } = await import('./drag-and-drop')); }); + beforeEach(async () => { + vi.clearAllMocks(); + vi.mocked(dropTargetForElements).mockImplementation(({ element }) => { + element.setAttribute('data-drop-target-for-element', 'true'); + + return vi.fn(() => { + element.removeAttribute('data-drop-target-for-element'); + }); + }); + + ctx = await setupStimulusTest({ + controllers: { + 'sortable-lists': SortableListsController, + 'sortable-lists--item': ItemController, + }, + }); + fixture = ctx.container; + }); + + afterEach(() => ctx.dispose()); + function controllerFor(element:HTMLElement) { const controller = Object.create(ItemController.prototype) as unknown as TestItemController; @@ -87,47 +122,53 @@ describe('Sortable lists item controller', () => { return controller; } - function fakeRoot( - element = document.createElement('div'), - { moving = false, acceptedType = null as string|null } = {}, - ):SortableListsRoot { - Object.defineProperty(element, 'isConnected', { value: true, configurable: true }); - return { element, moving, acceptedType }; + async function connectedItemFor({ + id = '123', + type = 'item', + outlet = true, + innerHtml = '', + }:{ + id?:string|null; + type?:string|null; + outlet?:boolean; + innerHtml?:string; + } = {}) { + fixture.innerHTML = ` +
              +
            • ${innerHtml}
              • +
              + `; + const root = fixture.querySelector('#root')!; + const item = fixture.querySelector('[data-controller~="sortable-lists--item"]')!; + await ctx.nextFrame(); + + const controller = ctx.application.getControllerForElementAndIdentifier(item, 'sortable-lists--item') as unknown as InstanceType; + + return { root, item, controller }; } - function connectedControllerFor( - element:HTMLElement, - { handle = null, root = fakeRoot() }:{ handle?:HTMLElement|null; root?:SortableListsRoot|null } = {}, - ) { - const controller = Object.create(ItemController.prototype) as InstanceType; + function setRootMoving(root:HTMLElement, moving:boolean):void { + const rootController = ctx.application.getControllerForElementAndIdentifier(root, 'sortable-lists') as unknown as { movingFlag:boolean }; - Object.defineProperty(controller, 'element', { value: element }); - Object.defineProperty(controller, 'idValue', { value: '123' }); - Object.defineProperty(controller, 'hasIdValue', { value: true }); - Object.defineProperty(controller, 'typeValue', { value: 'item' }); - Object.defineProperty(controller, 'hasTypeValue', { value: true }); - Object.defineProperty(controller, 'hasHandleTarget', { value: handle !== null }); - if (handle) { - Object.defineProperty(controller, 'handleTarget', { value: handle }); - } + rootController.movingFlag = moving; + } - controller.connect(); - if (root) { - controller.connectRoot(root); - } + function draggableOptionsFor(element:HTMLElement) { + return vi.mocked(draggable).mock.calls.find(([options]) => options.element === element)?.[0]; + } - return controller; + function dropTargetOptionsFor(element:HTMLElement) { + return vi.mocked(dropTargetForElements).mock.calls.find(([options]) => options.element === element)?.[0]; } - function draggableArgs(element = document.createElement('article')) { - return { - dragHandle: null, - element, - input: {} as never, - }; + function dragArgs(element:HTMLElement, dragHandle:HTMLElement|null = null) { + return { element, dragHandle, input: { clientX: 0, clientY: 0 } as never }; } - function dragEventPayload(element = document.createElement('article')) { + function dragEventPayload(element:HTMLElement) { return { location: { current: { input: { clientX: 0, clientY: 0 } } } as never, source: { @@ -138,49 +179,39 @@ describe('Sortable lists item controller', () => { }; } - beforeEach(() => { - vi.clearAllMocks(); - vi.mocked(dropTargetForElements).mockImplementation(({ element }) => { - element.setAttribute('data-drop-target-for-element', 'true'); - - return vi.fn(() => { - element.removeAttribute('data-drop-target-for-element'); - }); - }); - }); - - function connectItem({ id, type }:{ id:string; type:string }) { + it('warns when the id value is missing or empty', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); - const controller = Object.create(ItemController.prototype) as InstanceType; - - Object.defineProperty(controller, 'element', { value: document.createElement('li') }); - Object.defineProperty(controller, 'idValue', { value: id }); - Object.defineProperty(controller, 'hasIdValue', { value: id !== '' }); - Object.defineProperty(controller, 'typeValue', { value: type }); - Object.defineProperty(controller, 'hasTypeValue', { value: type !== '' }); - Object.defineProperty(controller, 'hasHandleTarget', { value: false }); - controller.connect(); - - return warn; - } - - it('warns when connected without an item id', () => { - const warn = connectItem({ id: '', type: 'work_package' }); + await connectedItemFor({ id: null }); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('id'), expect.anything()); + warn.mockClear(); + await connectedItemFor({ id: '' }); expect(warn).toHaveBeenCalledWith(expect.stringContaining('id'), expect.anything()); + + warn.mockRestore(); }); - it('warns when connected without an item type', () => { - const warn = connectItem({ id: '123', type: '' }); + it('warns when the type value is missing or empty', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + await connectedItemFor({ type: null }); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('type'), expect.anything()); + warn.mockClear(); + await connectedItemFor({ type: '' }); expect(warn).toHaveBeenCalledWith(expect.stringContaining('type'), expect.anything()); + + warn.mockRestore(); }); - it('does not warn when id and type are both present', () => { - const warn = connectItem({ id: '123', type: 'work_package' }); + it('does not warn when id and type are both present', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + + await connectedItemFor({ id: '123', type: 'work_package' }); expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); }); it('marks the closest edge while dragging over an item', () => { @@ -253,13 +284,11 @@ describe('Sortable lists item controller', () => { expect(nextElement.dataset.dropPositionOwner).toEqual('2'); }); - it('keeps the item drop target active while moving through row gaps', () => { - const element = document.createElement('article'); + it('keeps the item drop target active while moving through row gaps', async () => { + const { item } = await connectedItemFor(); - connectedControllerFor(element); - - expect(vi.mocked(dropTargetForElements).mock.lastCall?.[0].getIsSticky?.({ - element, + expect(dropTargetOptionsFor(item)?.getIsSticky?.({ + element: item, input: {} as never, source: { data: {}, @@ -268,194 +297,126 @@ describe('Sortable lists item controller', () => { })).toBe(true); }); - it('does not accept itself as an item drop target', () => { - const root = document.createElement('div'); - const element = document.createElement('article'); - - connectedControllerFor(element, { root: fakeRoot(root) }); + it('accepts drops only from items of the same type', async () => { + const { root, item } = await connectedItemFor({ id: '123', type: 'item' }); - expect(vi.mocked(dropTargetForElements).mock.lastCall?.[0].canDrop?.({ - element, + expect(dropTargetOptionsFor(item)?.canDrop?.({ + element: item, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '123', rootElement: root }), - element: document.createElement('article'), + data: sortableItemData({ type: 'item', itemId: '456', rootElement: root }), + element: document.createElement('li'), } as never, - })).toBe(false); - }); - - it('does not accept drops from another sortable lists root', () => { - const targetRoot = document.createElement('div'); - const foreignRoot = document.createElement('div'); - const targetElement = document.createElement('article'); - - connectedControllerFor(targetElement, { root: fakeRoot(targetRoot) }); + })).toBe(true); - expect(vi.mocked(dropTargetForElements).mock.lastCall?.[0].canDrop?.({ - element: targetElement, + expect(dropTargetOptionsFor(item)?.canDrop?.({ + element: item, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '456', rootElement: foreignRoot }), - element: document.createElement('article'), + data: sortableItemData({ type: 'other', itemId: '456', rootElement: root }), + element: document.createElement('li'), } as never, })).toBe(false); }); - it('does not accept drops whose type is rejected by the root', () => { - const root = document.createElement('div'); - const targetElement = document.createElement('article'); - - connectedControllerFor(targetElement, { root: fakeRoot(root, { acceptedType: 'work_package' }) }); + it('rejects drops from itself', async () => { + const { root, item } = await connectedItemFor({ id: '123', type: 'item' }); - expect(vi.mocked(dropTargetForElements).mock.lastCall?.[0].canDrop?.({ - element: targetElement, + expect(dropTargetOptionsFor(item)?.canDrop?.({ + element: item, input: {} as never, source: { - data: sortableItemData({ type: 'meeting_agenda_item', itemId: '456', rootElement: root }), - element: document.createElement('article'), + data: sortableItemData({ type: 'item', itemId: '123', rootElement: root }), + element: document.createElement('li'), } as never, })).toBe(false); }); - it('does not accept drops while the root is moving another item', () => { - const root = document.createElement('div'); - const targetElement = document.createElement('article'); - - connectedControllerFor(targetElement, { root: fakeRoot(root, { moving: true }) }); + it('rejects drops from another root', async () => { + const { item } = await connectedItemFor({ id: '123', type: 'item' }); + const foreignRoot = document.createElement('div'); - expect(vi.mocked(dropTargetForElements).mock.lastCall?.[0].canDrop?.({ - element: targetElement, + expect(dropTargetOptionsFor(item)?.canDrop?.({ + element: item, input: {} as never, source: { - data: sortableItemData({ type: 'item', itemId: '456', rootElement: root }), - element: document.createElement('article'), + data: sortableItemData({ type: 'item', itemId: '456', rootElement: foreignRoot }), + element: document.createElement('li'), } as never, })).toBe(false); }); - it('does not expose native external drag data', () => { - const element = document.createElement('article'); - - connectedControllerFor(element); + it('does not expose native external drag data', async () => { + const { item } = await connectedItemFor(); - expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialDataForExternal).toBeUndefined(); + expect(draggableOptionsFor(item)?.getInitialDataForExternal).toBeUndefined(); }); - it('prevents unhandled browser drag feedback while dragging an item', () => { - const element = document.createElement('article'); + it('prevents unhandled browser drag feedback while dragging an item', async () => { + const { item } = await connectedItemFor(); - connectedControllerFor(element); - - vi.mocked(draggable).mock.lastCall?.[0].onDragStart?.(dragEventPayload(element)); + draggableOptionsFor(item)?.onDragStart?.(dragEventPayload(item)); expect(preventUnhandled.start).toHaveBeenCalledOnce(); - vi.mocked(draggable).mock.lastCall?.[0].onDrop?.(dragEventPayload(element)); + draggableOptionsFor(item)?.onDrop?.(dragEventPayload(item)); expect(preventUnhandled.stop).toHaveBeenCalledOnce(); }); - it('does not start dragging from interactive descendants', () => { - const element = document.createElement('article'); - const link = document.createElement('a'); + it('does not start dragging from interactive descendants', async () => { + const { item } = await connectedItemFor({ + innerHtml: 'Link', + }); + const link = item.querySelector('a')!; - link.href = '/work_packages/123'; - element.appendChild(link); vi.spyOn(document, 'elementFromPoint').mockReturnValue(link); - connectedControllerFor(element); - expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ - element, - dragHandle: null, - input: { clientX: 10, clientY: 10 } as never, - })).toBe(false); + expect(draggableOptionsFor(item)?.canDrag?.(dragArgs(item))).toBe(false); }); - it('starts dragging from non-interactive descendants', () => { - const element = document.createElement('article'); - const text = document.createElement('span'); + it('starts dragging from non-interactive descendants', async () => { + const { item } = await connectedItemFor({ + innerHtml: 'text', + }); + const text = item.querySelector('span')!; - element.appendChild(text); vi.spyOn(document, 'elementFromPoint').mockReturnValue(text); - connectedControllerFor(element); - expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ - element, - dragHandle: null, - input: { clientX: 10, clientY: 10 } as never, - })).toBe(true); + expect(draggableOptionsFor(item)?.canDrag?.(dragArgs(item))).toBe(true); }); - it('starts dragging from the focusable drag handle itself', () => { - const element = document.createElement('li'); - const handle = document.createElement('article'); + it('starts dragging from the focusable drag handle itself', async () => { + const { item } = await connectedItemFor({ + innerHtml: '
              ', + }); + const handle = item.querySelector('article')!; - handle.tabIndex = 0; - handle.setAttribute('data-sortable-lists--item-target', 'preview handle'); - element.appendChild(handle); - document.body.appendChild(element); vi.spyOn(document, 'elementFromPoint').mockReturnValue(handle); - connectedControllerFor(element, { handle }); - expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ - element, - dragHandle: handle, - input: { clientX: 10, clientY: 10 } as never, - })).toBe(true); - - element.remove(); + expect(draggableOptionsFor(item)?.canDrag?.(dragArgs(item, handle))).toBe(true); }); - it('does not start dragging while the root is moving another item', () => { - const element = document.createElement('article'); - const text = document.createElement('span'); - element.appendChild(text); - vi.spyOn(document, 'elementFromPoint').mockReturnValue(text); + it('refuses to start a drag while the root is moving', async () => { + const { root, item } = await connectedItemFor(); - connectedControllerFor(element, { root: fakeRoot(document.createElement('div'), { moving: true }) }); + setRootMoving(root, true); - expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ - element, dragHandle: null, input: { clientX: 10, clientY: 10 } as never, - })).toBe(false); + expect(draggableOptionsFor(item)?.canDrag?.(dragArgs(item))).toBe(false); }); - it('refuses to drag before the root reference is connected', () => { - const element = document.createElement('article'); - const text = document.createElement('span'); - element.appendChild(text); - vi.spyOn(document, 'elementFromPoint').mockReturnValue(text); - - connectedControllerFor(element, { root: null }); + it('refuses to start a drag when the root outlet is missing', async () => { + const { item } = await connectedItemFor({ outlet: false }); - expect(vi.mocked(draggable).mock.lastCall?.[0].canDrag?.({ - element, dragHandle: null, input: { clientX: 10, clientY: 10 } as never, - })).toBe(false); + expect(draggableOptionsFor(item)?.canDrag?.(dragArgs(item))).toBe(false); }); - it('includes the root element in the drag payload', () => { - const root = document.createElement('div'); - const element = document.createElement('article'); - connectedControllerFor(element, { root: fakeRoot(root) }); + it('includes the root element in the drag payload', async () => { + const { root, item } = await connectedItemFor({ id: '123', type: 'item' }); - expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(element))) + expect(draggableOptionsFor(item)?.getInitialData?.(dragArgs(item))) .toEqual(expect.objectContaining({ itemId: '123', type: 'item', rootElement: root })); }); describe('Stimulus application wiring', () => { - let ctx:StimulusTestContext; - let fixture:HTMLElement; - - beforeEach(async () => { - ctx = await setupStimulusTest({ - controllers: { - 'sortable-lists--item': ItemController, - }, - }); - fixture = ctx.container; - }); - - afterEach(() => { - ctx.dispose(); - }); - function renderBacklogsRow(itemId = '123', { boxClasses = '' } = {}) { const rowHtml = `
            • { await ctx.nextFrame(); - expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(draggableArgs(article))).toEqual(expect.objectContaining({ + expect(vi.mocked(draggable).mock.lastCall?.[0].getInitialData?.(dragArgs(article))).toEqual(expect.objectContaining({ itemId: '123', type: 'work_package', })); 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 912ea7f9785f..76c1d77ae5c8 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.ts @@ -38,13 +38,11 @@ import { setCustomNativeDragPreview } from '@atlaskit/pragmatic-drag-and-drop/el import { preventUnhandled } from '@atlaskit/pragmatic-drag-and-drop/prevent-unhandled'; import { Controller } from '@hotwired/stimulus'; import { closestInteractiveElement } from 'core-stimulus/helpers/interactive-element-helper'; +import type SortableListsController from '../sortable-lists.controller'; import { - canAccept, - isSortableItemData, + isItemFromRoot, sortableItemData, - type RootAwareChild, type SortableItemData, - type SortableListsRoot, } from './drag-and-drop'; import { sortableItemSelector } from './list-dom'; @@ -71,9 +69,11 @@ const PREVIEW_STRIPPED_ATTRIBUTES = [ // `.Box--condensed .Box-card`) would not apply to it otherwise. const BOX_DENSITY_VARIANT_CLASSES = ['Box--condensed', 'Box--spacious'] as const; -export default class ItemController extends Controller implements RootAwareChild { +export default class ItemController extends Controller { static targets = ['handle', 'preview']; + static outlets = ['sortable-lists']; + static values = { id: String, type: String, @@ -89,9 +89,11 @@ export default class ItemController extends Controller implements R declare readonly previewTarget:HTMLElement; declare readonly hasPreviewTarget:boolean; + declare readonly sortableListsOutlet:SortableListsController; + declare readonly hasSortableListsOutlet:boolean; + private cleanupFn?:CleanupFn; private dropIndicatorElement?:HTMLElement; - private root?:SortableListsRoot; connect():void { this.warnOnMissingValues(); @@ -104,27 +106,17 @@ export default class ItemController extends Controller implements R disconnect():void { this.cleanupFn?.(); this.cleanupFn = undefined; - this.disconnectRoot(); - } - - // Called by the root controller's outlet-connected callback. - connectRoot(root:SortableListsRoot):void { - this.root = root; - } - - disconnectRoot():void { - this.root = undefined; } // Both values are required: an item with an empty id can never be persisted, - // and an empty type never matches the root's accepted type, so the item would + // and an empty type never matches a list's accepted types, so the item would // appear draggable yet silently refuse every drop. Surface that wiring mistake. private warnOnMissingValues():void { - if (!this.hasIdValue) { + if (!this.hasIdValue || this.idValue === '') { console.warn('sortable-lists--item is missing its required id value (data-sortable-lists--item-id-value); it cannot be moved.', this.element); } - if (!this.hasTypeValue) { + if (!this.hasTypeValue || this.typeValue === '') { console.warn('sortable-lists--item is missing its required type value (data-sortable-lists--item-type-value); it cannot be moved.', this.element); } } @@ -134,8 +126,10 @@ export default class ItemController extends Controller implements R element: this.element, ...(this.hasHandleTarget ? { dragHandle: this.handleTarget } : {}), canDrag: ({ input }) => { - const { root } = this; - if (root == null || root.moving) { + // The single moving gate: a drop ends the drag before the persist request + // starts, so blocking new drags here is sufficient — no drop can happen + // while a move is in flight. + if (!this.hasSortableListsOutlet || this.sortableListsOutlet.moving) { return false; } return this.canDragFromPoint(input.clientX, input.clientY); @@ -171,16 +165,16 @@ export default class ItemController extends Controller implements R return dropTargetForElements({ element: this.element, canDrop: ({ source }) => { - const { root } = this; - if (root == null || root.moving) { + if (!this.hasSortableListsOutlet) { return false; } - if (isSortableItemData(source.data) && source.data.itemId === this.idValue) { + if (!isItemFromRoot(this.sortableListsOutlet.element, source.data)) { return false; } - return canAccept(root, source.data); + return source.data.itemId !== this.idValue + && source.data.type === this.typeValue; }, getData: ({ input }) => { return attachClosestEdge(this.getItemData(), { @@ -223,7 +217,7 @@ export default class ItemController extends Controller implements R return sortableItemData({ itemId: this.idValue, type: this.typeValue, - rootElement: this.root?.element ?? null, + rootElement: this.hasSortableListsOutlet ? this.sortableListsOutlet.element : null, }); } diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts index b18ca76c98ac..9398a6517c50 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts @@ -26,10 +26,17 @@ // See COPYRIGHT and LICENSE files for more details. //++ +import { vi } from 'vitest'; + import { captureRowPositions, + hasTruncationMarkerRow, reorderRows, + resolveItemElement, + resolveItemPosition, resolveListAppendPreviousItemId, + resolveRow, + resolveRowsContainer, restoreRowPositions, } from './list-dom'; @@ -67,120 +74,236 @@ describe('sortable lists DOM helpers', () => { describe('resolveListAppendPreviousItemId', () => { it('returns the last item in a list while skipping the source and truncation marker rows', () => { - const list = listElement(); + const container = listElement(); - list.append(itemRow('1'), showMoreRow(), itemRow('2'), itemRow('3')); + container.append(itemRow('1'), showMoreRow(), itemRow('2'), itemRow('3')); - expect(resolveListAppendPreviousItemId({ sourceItemId: '3', list })).toEqual('2'); + expect(resolveListAppendPreviousItemId({ sourceItemId: '3', container })).toEqual('2'); }); it('returns null when the list has no other items', () => { - const list = listElement(); + const container = listElement(); - list.append(itemRow('1')); + container.append(itemRow('1')); - expect(resolveListAppendPreviousItemId({ sourceItemId: '1', list })).toBeNull(); + expect(resolveListAppendPreviousItemId({ sourceItemId: '1', container })).toBeNull(); }); }); describe('reorderRows', () => { it('moves a row to sit immediately after the previous item anchor', () => { - const list = listElement(); + const container = listElement(); const [one, two, three] = ['1', '2', '3'].map(itemRow); - list.append(one, two, three); - reorderRows({ rows: [one], list, previousItemId: '2' }); + container.append(one, two, three); + reorderRows({ rows: [one], container, previousItemId: '2' }); - expect(itemIdOrder(list)).toEqual(['2', '1', '3']); + expect(itemIdOrder(container)).toEqual(['2', '1', '3']); }); it('moves a row to the top of the list before the first existing row', () => { - const list = listElement(); + const container = listElement(); const [one, two, three] = ['1', '2', '3'].map(itemRow); - list.append(one, two, three); - reorderRows({ rows: [three], list, previousItemId: null }); + container.append(one, two, three); + reorderRows({ rows: [three], container, previousItemId: null }); - expect(itemIdOrder(list)).toEqual(['3', '1', '2']); + expect(itemIdOrder(container)).toEqual(['3', '1', '2']); }); - it('keeps a top-of-list move inside a nested list element instead of escaping it', () => { - const list = document.createElement('div'); - const inner = document.createElement('ul'); - const [one, two, three] = ['1', '2', '3'].map(itemRow); - - list.setAttribute('data-sortable-lists-target', 'list'); - inner.append(one, two, three); - list.append(inner); - - reorderRows({ rows: [three], list, previousItemId: null }); - - expect(three.parentElement).toBe(inner); - expect(itemIdOrder(list)).toEqual(['3', '1', '2']); + it('reorders section rows in a plain div container', () => { + const container = document.createElement('div'); + container.innerHTML = ` +
              +
              `; + const [a, b] = Array.from(container.children) as HTMLElement[]; + reorderRows({ rows: [a], container, previousItemId: 'b' }); + expect(Array.from(container.children)).toEqual([b, a]); }); it('inserts a moved group after the anchor preserving their order', () => { - const list = listElement(); + const container = listElement(); const [one, two, three, four] = ['1', '2', '3', '4'].map(itemRow); - list.append(one, two, three, four); - reorderRows({ rows: [three, four], list, previousItemId: '1' }); + container.append(one, two, three, four); + reorderRows({ rows: [three, four], container, previousItemId: '1' }); - expect(itemIdOrder(list)).toEqual(['1', '3', '4', '2']); + expect(itemIdOrder(container)).toEqual(['1', '3', '4', '2']); }); it('anchors on a truncation marker row when the previous item is hidden', () => { - const list = listElement(); + const container = listElement(); const [one, two, three] = ['1', '2', '3'].map(itemRow); const marker = showMoreRow('hidden'); - list.append(three, one, marker, two); - reorderRows({ rows: [three], list, previousItemId: 'hidden' }); + container.append(three, one, marker, two); + reorderRows({ rows: [three], container, previousItemId: 'hidden' }); expect(three.previousElementSibling).toBe(marker); - expect(itemIdOrder(list)).toEqual(['1', '3', '2']); + expect(itemIdOrder(container)).toEqual(['1', '3', '2']); }); }); describe('captureRowPositions / restoreRowPositions', () => { it('restores a row to its original position after an optimistic move', () => { - const list = listElement(); + const container = listElement(); const [one, two, three] = ['1', '2', '3'].map(itemRow); - list.append(one, two, three); + container.append(one, two, three); const snapshot = captureRowPositions([three]); - reorderRows({ rows: [three], list, previousItemId: null }); - expect(itemIdOrder(list)).toEqual(['3', '1', '2']); + reorderRows({ rows: [three], container, previousItemId: null }); + expect(itemIdOrder(container)).toEqual(['3', '1', '2']); restoreRowPositions(snapshot); - expect(itemIdOrder(list)).toEqual(['1', '2', '3']); + expect(itemIdOrder(container)).toEqual(['1', '2', '3']); }); it('restores a multi-row group to its original order', () => { - const list = listElement(); + const container = listElement(); const [one, two, three, four] = ['1', '2', '3', '4'].map(itemRow); - list.append(one, two, three, four); + container.append(one, two, three, four); const snapshot = captureRowPositions([two, three]); - reorderRows({ rows: [two, three], list, previousItemId: '4' }); - expect(itemIdOrder(list)).toEqual(['1', '4', '2', '3']); + reorderRows({ rows: [two, three], container, previousItemId: '4' }); + expect(itemIdOrder(container)).toEqual(['1', '4', '2', '3']); restoreRowPositions(snapshot); - expect(itemIdOrder(list)).toEqual(['1', '2', '3', '4']); + expect(itemIdOrder(container)).toEqual(['1', '2', '3', '4']); }); it('falls back to appending when the captured next sibling is stale', () => { - const list = listElement(); + const container = listElement(); const [one, two] = ['1', '2'].map(itemRow); - list.append(one, two); + container.append(one, two); const snapshot = captureRowPositions([one]); two.remove(); expect(() => restoreRowPositions(snapshot)).not.toThrow(); - expect(itemIdOrder(list)).toEqual(['1']); + expect(itemIdOrder(container)).toEqual(['1']); + }); + }); + + describe('resolveRowsContainer', () => { + it('returns the list element itself when no selector value is set', () => { + const list = document.createElement('div'); + expect(resolveRowsContainer(list)).toBe(list); + }); + + it('returns the matched descendant when a selector value is set', () => { + const list = document.createElement('div'); + list.setAttribute('data-sortable-lists--list-rows-container-selector-value', ':scope > ul'); + const ul = document.createElement('ul'); + list.append(ul); + expect(resolveRowsContainer(list)).toBe(ul); + }); + + it('falls back to the list element when the selector matches nothing', () => { + const list = document.createElement('div'); + list.setAttribute('data-sortable-lists--list-rows-container-selector-value', ':scope > ul'); + expect(resolveRowsContainer(list)).toBe(list); + }); + + it('falls back to the list element and warns when the selector value is invalid', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const list = document.createElement('div'); + list.setAttribute('data-sortable-lists--list-rows-container-selector-value', '[['); + + expect(resolveRowsContainer(list)).toBe(list); + expect(warnSpy).toHaveBeenCalledTimes(1); + const message = warnSpy.mock.calls[0]?.[0] as string; + expect(message).toContain('data-sortable-lists--list-rows-container-selector-value'); + expect(message).toContain('[['); + + warnSpy.mockRestore(); + }); + }); + + describe('hasTruncationMarkerRow', () => { + it('returns false for a container with only item rows', () => { + const container = listElement(); + container.append(itemRow('1'), itemRow('2')); + + expect(hasTruncationMarkerRow(container)).toBe(false); + }); + + it('returns true when the container holds a truncation marker row', () => { + const container = listElement(); + container.append(itemRow('1'), showMoreRow(), itemRow('2')); + + expect(hasTruncationMarkerRow(container)).toBe(true); + }); + }); + + describe('resolveRow', () => { + it('returns the direct child containing a nested element', () => { + const container = document.createElement('ul'); + container.innerHTML = '
              • '; + const row = container.firstElementChild as HTMLElement; + expect(resolveRow(container, container.querySelector('.deep')!)).toBe(row); + }); + + it('returns the element itself when it is a direct child', () => { + const container = document.createElement('div'); + container.innerHTML = '
              '; + const section = container.querySelector('#s')!; + expect(resolveRow(container, section)).toBe(section); + }); + + it('returns null for an element outside the container', () => { + expect(resolveRow(document.createElement('div'), document.createElement('span'))).toBeNull(); + }); + }); + + describe('resolveItemElement with nested sortable items', () => { + // #23893 / AGILE-292 dual-role shape: an outer item surface (bucket) hosting + // a nested inner sortable list with its own item rows. + function dualRoleFixture() { + const outerRow = document.createElement('li'); + outerRow.innerHTML = ` +
              +
              +
              `; + return { + outerRow, + innerRow: outerRow.querySelector('ul > li')!, + }; + } + + it('resolves an inner row to its own descendant item, never an ancestor surface', () => { + // The bug this guards: closest()-based resolution walks OUT of the row and + // finds the outer bucket's item surface instead of the row's own card. + const { innerRow } = dualRoleFixture(); + expect(resolveItemElement(innerRow)?.getAttribute('data-sortable-lists--item-id-value')).toBe('inner'); + }); + + it('resolves an outer row to its own item surface, not a nested descendant', () => { + const { outerRow } = dualRoleFixture(); + expect(resolveItemElement(outerRow)?.getAttribute('data-sortable-lists--item-id-value')).toBe('outer'); + }); + }); + + describe('resolveItemPosition', () => { + function containerWith(ids:(string|null)[]):HTMLElement { + const ul = document.createElement('ul'); + ids.forEach((id) => { + const li = document.createElement('li'); + if (id) { li.setAttribute('data-sortable-lists--item-id-value', id); } + ul.append(li); + }); + return ul; + } + + it('returns 1 for a null previous item (top of list)', () => { + expect(resolveItemPosition({ container: containerWith(['a', 'b']), previousItemId: null })).toBe(1); + }); + + it('returns the position after the previous item, counting only item rows', () => { + // non-item row (e.g. blankslate / show-more marker) between a and b + expect(resolveItemPosition({ container: containerWith(['a', null, 'b']), previousItemId: 'a' })).toBe(2); + expect(resolveItemPosition({ container: containerWith(['a', null, 'b']), previousItemId: 'b' })).toBe(3); }); }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts index a78d6b638f71..1ab5fb8f7a1d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -34,21 +34,63 @@ // This module holds the drag-and-drop-agnostic half of that contract: reading // items and rows out of the DOM and moving rows around. The Pragmatic DnD // payloads built on top of it live in drag-and-drop.ts. -export const sortableListsMovingAttribute = 'data-sortable-lists-moving'; export const sortableListsRootSelector = '[data-controller~="sortable-lists"]'; export const sortableItemSelector = '[data-sortable-lists--item-id-value]'; export const sortableListSelector = '[data-controller~="sortable-lists--list"]'; export const sortablePreviousItemIdAttribute = 'data-sortable-lists-prev-item-id'; +// Stimulus value attribute of sortable-lists--list; read here so row +// resolution works from plain elements (the module is the DOM contract). +const rowsContainerSelectorAttribute = 'data-sortable-lists--list-rows-container-selector-value'; + +// The element whose direct children are the list's rows: the list element +// itself, or a descendant named by the list's rowsContainerSelector value +// (needed e.g. for Primer BorderBox, which owns its inner
                ). +export function resolveRowsContainer(list:HTMLElement):HTMLElement { + const selector = list.getAttribute(rowsContainerSelectorAttribute); + if (!selector) { + return list; + } -// Rows can sit directly under the list element or inside a nested
                  . -const listRowSelector = ':scope > li, :scope > ul > li'; + try { + return list.querySelector(selector) ?? list; + } catch (error) { + console.warn(`Invalid ${rowsContainerSelectorAttribute} selector "${selector}" on`, list, error); + return list; + } +} -function listRows(list:Element):Element[] { - return Array.from(list.querySelectorAll(listRowSelector)); +// Whether a rows container holds a truncated list's "show more" marker row +// (data-sortable-lists-prev-item-id on a non-item row). A truncated list's +// visible window (which rows show, the marker's collapsed-item metadata) is +// server-computed from the full ordering, so an optimistic client-side +// reorder cannot be the final word there — the server must re-render it. +export function hasTruncationMarkerRow(container:Element):boolean { + return container.querySelector(`[${sortablePreviousItemIdAttribute}]`) !== null; } -function firstListRow(list:Element):Element|null { - return list.querySelector(listRowSelector); +// A row is the direct child of the rows container that contains the element. +export function resolveRow(container:Element, element:Element):HTMLElement|null { + let current:Element|null = element; + + while (current && current.parentElement !== container) { + current = current.parentElement; + } + + return current instanceof HTMLElement ? current : null; +} + +// The row of a dragged item element, resolved against its own list. +export function resolveSourceRow(sourceElement:Element):HTMLElement|null { + const list = sourceElement.closest(sortableListSelector); + if (!list) { + return null; + } + + return resolveRow(resolveRowsContainer(list), sourceElement); +} + +function listRows(container:Element):Element[] { + return Array.from(container.children); } export function resolveItemId(element:Element):string|null { @@ -63,9 +105,17 @@ export function resolveClosestItemElement(element:Element):HTMLElement|null { return element.closest(sortableItemSelector); } +// A row's item is itself or a descendant — never an ancestor. Walking up via +// closest() would resolve an inner row (a card inside a dual-role bucket) to +// the outer bucket's item surface. querySelector returns the first match in +// document order, so an outer row's own surface wins over its nested list's +// items. export function resolveItemElement(element:Element):HTMLElement|null { - return resolveClosestItemElement(element) ?? - element.querySelector(sortableItemSelector); + if (element instanceof HTMLElement && element.matches(sortableItemSelector)) { + return element; + } + + return element.querySelector(sortableItemSelector); } export function resolvePreviousItemId(element:Element):string|null { @@ -81,22 +131,22 @@ export function resolvePreviousItemId(element:Element):string|null { // on data-sortable-lists-prev-item-id rather than exposing an item element. // Anchor on that marker so the row lands next to the collapsed block instead // of jumping to the top. -function resolveAnchorRow(list:HTMLElement, previousItemId:string):HTMLElement|null { +function resolveAnchorRow(container:HTMLElement, previousItemId:string):HTMLElement|null { const escaped = CSS.escape(previousItemId); - const anchor = list.querySelector(`[data-sortable-lists--item-id-value="${escaped}"]`) - ?? list.querySelector(`[${sortablePreviousItemIdAttribute}="${escaped}"]`); + const anchor = container.querySelector(`[data-sortable-lists--item-id-value="${escaped}"]`) + ?? container.querySelector(`[${sortablePreviousItemIdAttribute}="${escaped}"]`); - return anchor?.closest('li') ?? null; + return anchor ? resolveRow(container, anchor) : null; } export function resolveListAppendPreviousItemId({ sourceItemId, - list, + container, }:{ sourceItemId:string; - list:Element; + container:Element; }):string|null { - const rows = listRows(list).reverse(); + const rows = listRows(container).reverse(); for (const row of rows) { const itemId = resolvePreviousItemId(row); @@ -138,39 +188,66 @@ export function restoreRowPositions(positions:RowPlacement[]):void { } // Optimistically move rows on the client without waiting for the server. -// `rows` are the moved
                • s in order (one today, the selected set once +// `rows` are the moved rows in order (one today, the selected set once // multi-item DnD lands); `previousItemId` of null means top of list. export function reorderRows({ rows, - list, + container, previousItemId, }:{ rows:HTMLElement[]; - list:HTMLElement; + container:HTMLElement; previousItemId:string|null; }):void { - let anchor:Element|null = previousItemId ? resolveAnchorRow(list, previousItemId) : null; + let anchor:Element|null = previousItemId ? resolveAnchorRow(container, previousItemId) : null; for (const row of rows) { if (anchor) { anchor.after(row); } else { - insertAtListTop(list, row); + insertAtListTop(container, row); } anchor = row; } } -// A plain list.prepend() would drop the row before a nested
                    . Insert -// before the first existing row instead, keeping it in the same container as -// its siblings. -function insertAtListTop(list:HTMLElement, row:HTMLElement):void { - const firstRow = firstListRow(list); +function insertAtListTop(container:HTMLElement, row:HTMLElement):void { + const firstRow = container.firstElementChild; if (firstRow && firstRow !== row) { firstRow.before(row); } else if (!firstRow) { - (list.querySelector(':scope > ul') ?? list).prepend(row); + container.prepend(row); + } +} + +// 1-based position a dropped row will occupy, counting only item rows (rows +// hosting an item surface); used by the absolute position payload mode. +export function resolveItemPosition({ + container, + previousItemId, +}:{ + container:HTMLElement; + previousItemId:string|null; +}):number { + if (previousItemId === null) { + return 1; } + + let position = 1; + + for (const row of listRows(container)) { + const item = resolveItemElement(row); + if (!item) { + continue; + } + + position += 1; + if (resolveItemId(item) === previousItemId) { + return position; + } + } + + return position; } 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 f88cc4af5ce6..b785f693589b 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 @@ -24,20 +24,23 @@ // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. // // See COPYRIGHT and LICENSE files for more details. -//++ +//++ import type { dropTargetForElements as dropTargetForElementsFn } from '@atlaskit/pragmatic-drag-and-drop/element/adapter'; import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; +import type SortableListsControllerType from '../sortable-lists.controller'; import type ListControllerType from './list.controller'; -import type { sortableItemData as sortableItemDataFn, SortableListsRoot } from './drag-and-drop'; +import type { sortableItemData as sortableItemDataFn } from './drag-and-drop'; -// The list controller is tested in ISOLATION: the root drives the outlet -// hand-over in production (sortable-lists.controller.ts), so here we render only -// the list, let it connect, then call connectRoot(fakeRoot) ourselves to stand -// in for that wiring. +// The list controller resolves its root through a real Stimulus outlet, so +// both the root (`sortable-lists.controller.ts`) and the list controller are +// registered here and wired together through the +// `data-sortable-lists--list-sortable-lists-outlet` +// attribute, exactly as production markup does. describe('Sortable lists list controller', () => { let dropTargetForElements:typeof dropTargetForElementsFn; let ListController:typeof ListControllerType; + let SortableListsController:typeof SortableListsControllerType; let sortableItemData:typeof sortableItemDataFn; let ctx:StimulusTestContext; @@ -50,8 +53,13 @@ describe('Sortable lists list controller', () => { monitorForElements: vi.fn(() => vi.fn()), })); + vi.doMock('@atlaskit/pragmatic-drag-and-drop-auto-scroll/element', () => ({ + autoScrollForElements: vi.fn(() => vi.fn()), + })); + ({ dropTargetForElements } = await import('@atlaskit/pragmatic-drag-and-drop/element/adapter')); ({ default: ListController } = await import('./list.controller')); + ({ default: SortableListsController } = await import('../sortable-lists.controller')); ({ sortableItemData } = await import('./drag-and-drop')); }); @@ -59,6 +67,7 @@ describe('Sortable lists list controller', () => { vi.clearAllMocks(); ctx = await setupStimulusTest({ controllers: { + 'sortable-lists': SortableListsController, 'sortable-lists--list': ListController, }, }); @@ -67,39 +76,36 @@ describe('Sortable lists list controller', () => { afterEach(() => ctx.dispose()); - function fakeRoot( - element = document.createElement('div'), - { moving = false, acceptedType = null as string|null } = {}, - ):SortableListsRoot { - return { element, moving, acceptedType }; - } - async function connectedListFor({ type = 'sprint', id = '7', dropPosition = null, - root = fakeRoot(), + acceptedTypes = ['work_package'], + outlet = true, }:{ type?:string|null; id?:string|null; dropPosition?:string|null; - root?:SortableListsRoot|null; + acceptedTypes?:string[]|null; + outlet?:boolean; } = {}) { fixture.innerHTML = ` -
                      +
                      +
                        +
                        `; + const root = fixture.querySelector('#root')!; const list = fixture.querySelector('[data-controller~="sortable-lists--list"]')!; await ctx.nextFrame(); const controller = ctx.application.getControllerForElementAndIdentifier(list, 'sortable-lists--list') as unknown as InstanceType; - if (root) { - controller.connectRoot(root); - } - return { list, controller }; + return { root, list, controller }; } function dropTargetOptionsFor(element:HTMLElement) { @@ -123,12 +129,6 @@ describe('Sortable lists list controller', () => { expect(dropTargetForElements).toHaveBeenCalledWith(expect.objectContaining({ element: list })); }); - it('does not register without a list type value', async () => { - const { list } = await connectedListFor({ type: null, root: null }); - - expect(dropTargetForElements).not.toHaveBeenCalledWith(expect.objectContaining({ element: list })); - }); - it('exposes its list payload through getData', async () => { const { list } = await connectedListFor({ type: 'sprint', id: '7' }); @@ -157,72 +157,62 @@ describe('Sortable lists list controller', () => { .toEqual(expect.objectContaining({ dropPosition: 'end' })); }); - it('accepts a same-root item of the accepted type', async () => { - const root = document.createElement('div'); - const { list } = await connectedListFor({ root: fakeRoot(root, { acceptedType: 'work_package' }) }); + it('does not register a drop target without acceptedTypes', async () => { + const { list } = await connectedListFor({ acceptedTypes: null }); - expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(root, 'work_package') })) - .toBe(true); + expect(dropTargetForElements).not.toHaveBeenCalledWith(expect.objectContaining({ element: list })); }); - it('rejects an item whose type is not accepted', async () => { - const root = document.createElement('div'); - const { list } = await connectedListFor({ root: fakeRoot(root, { acceptedType: 'work_package' }) }); + it('does not register a drop target with an empty acceptedTypes array', async () => { + const { list } = await connectedListFor({ acceptedTypes: [] }); - expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(root, 'meeting_agenda_item') })) - .toBe(false); + expect(dropTargetForElements).not.toHaveBeenCalledWith(expect.objectContaining({ element: list })); }); - it('rejects a source from another root', async () => { - const { list } = await connectedListFor({ root: fakeRoot(document.createElement('div')) }); + it('warns and does not register when acceptedTypes is set but type is missing or empty', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); - expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(document.createElement('div'), 'work_package') })) - .toBe(false); - }); + const { list: listWithoutType } = await connectedListFor({ type: null, acceptedTypes: ['work_package'] }); + expect(warn).toHaveBeenCalledOnce(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('type'), expect.anything()); + expect(dropTargetForElements).not.toHaveBeenCalledWith(expect.objectContaining({ element: listWithoutType })); - it('rejects a source with no root reference', async () => { - const { list } = await connectedListFor({ root: fakeRoot(document.createElement('div')) }); + warn.mockClear(); - expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(null, 'work_package') })) - .toBe(false); + const { list: listWithEmptyType } = await connectedListFor({ type: '', acceptedTypes: ['work_package'] }); + expect(warn).toHaveBeenCalledOnce(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('type'), expect.anything()); + expect(dropTargetForElements).not.toHaveBeenCalledWith(expect.objectContaining({ element: listWithEmptyType })); + + warn.mockRestore(); }); - it('refuses drops until the root reference is connected', async () => { - const { list } = await connectedListFor({ root: null }); + it('rejects drops when the root outlet is not connected', async () => { + const { list } = await connectedListFor({ outlet: false }); expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(document.createElement('div'), 'work_package') })) .toBe(false); }); - it('reflects the root moving state as aria-busy on connect', async () => { - const { list } = await connectedListFor({ root: fakeRoot(document.createElement('div'), { moving: true }) }); - - expect(list.getAttribute('aria-busy')).toEqual('true'); - }); - - it('clears aria-busy when reflectMoving is turned off', async () => { - const { list, controller } = await connectedListFor({ root: fakeRoot(document.createElement('div'), { moving: true }) }); - expect(list.getAttribute('aria-busy')).toEqual('true'); + it('accepts an item of an accepted type from the same root', async () => { + const { list, root } = await connectedListFor({ acceptedTypes: ['work_package'] }); - controller.reflectMoving(false); - expect(list.hasAttribute('aria-busy')).toBe(false); + expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(root, 'work_package') })) + .toBe(true); }); - it('clears aria-busy when the root outlet disconnects mid-move', async () => { - const { list, controller } = await connectedListFor({ root: fakeRoot(document.createElement('div'), { moving: true }) }); - expect(list.getAttribute('aria-busy')).toEqual('true'); + it('rejects an item type not in acceptedTypes', async () => { + const { list, root } = await connectedListFor({ acceptedTypes: ['work_package'] }); - controller.disconnectRoot(); - expect(list.hasAttribute('aria-busy')).toBe(false); + expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(root, 'sprint') })) + .toBe(false); }); - it('clears aria-busy when the list element disconnects mid-move', async () => { - const { list } = await connectedListFor({ root: fakeRoot(document.createElement('div'), { moving: true }) }); - expect(list.getAttribute('aria-busy')).toEqual('true'); + it('rejects an item from a different root', async () => { + const { list } = await connectedListFor({ acceptedTypes: ['work_package'] }); - list.remove(); - await ctx.nextFrame(); - expect(list.hasAttribute('aria-busy')).toBe(false); + expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(document.createElement('div'), 'work_package') })) + .toBe(false); }); it('outlines the container for a list-only drop', async () => { @@ -264,12 +254,4 @@ describe('Sortable lists list controller', () => { options?.onDragLeave?.({} as never); expect(list.dataset.dropContainer).toBeUndefined(); }); - - it('rejects drops while the root is moving', async () => { - const root = document.createElement('div'); - const { list } = await connectedListFor({ root: fakeRoot(root, { acceptedType: 'work_package', moving: true }) }); - - expect(dropTargetOptionsFor(list)?.canDrop?.({ element: list, input: {} as never, source: source(root, 'work_package') })) - .toBe(false); - }); }); diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts index df790b9c7711..99661a32aec5 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.ts @@ -29,39 +29,55 @@ import { dropTargetForElements } from '@atlaskit/pragmatic-drag-and-drop/element/adapter'; import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/types'; import { Controller } from '@hotwired/stimulus'; +import type SortableListsController from '../sortable-lists.controller'; import { - canAccept, + isItemFromRoot, isSortableItemData, + listAcceptsType, sortableListData, - type RootAwareChild, type SortableListData, type SortableListDropPosition, - type SortableListsRoot, } from './drag-and-drop'; type CleanupFn = () => void; const dropPositions = new Set(['start', 'end']); -export default class ListController extends Controller implements RootAwareChild { +export default class ListController extends Controller { + static outlets = ['sortable-lists']; + static values = { type: String, id: String, dropPosition: { type: String, default: 'end' }, + acceptedTypes: Array, + // Consumed by list-dom.ts via the rendered attribute; declared here so the + // list's public API is complete in one place. + rowsContainerSelector: String, }; + declare readonly sortableListsOutlet:SortableListsController; + declare readonly hasSortableListsOutlet:boolean; + declare readonly typeValue:string; - declare readonly hasTypeValue:boolean; declare readonly idValue:string; declare readonly hasIdValue:boolean; declare readonly dropPositionValue:string; + declare readonly acceptedTypesValue:string[]; + declare readonly hasAcceptedTypesValue:boolean; - private root?:SortableListsRoot; private cleanupFn?:CleanupFn; connect():void { - // A list without a type value is not a drop target. - if (!this.hasTypeValue) { + // A list without accepted types is not a drop target (display-only list). + if (!this.hasAcceptedTypesValue || this.acceptedTypesValue.length === 0) { + return; + } + + // The type doubles as the persisted list_type: a droppable list without it + // would accept drops it cannot persist. Surface that wiring mistake. + if (this.typeValue === '') { + console.warn('sortable-lists--list has acceptedTypes but is missing its required type value (data-sortable-lists--list-type-value); it cannot accept drops.', this.element); return; } @@ -88,28 +104,6 @@ export default class ListController extends Controller implements R disconnect():void { this.cleanupFn?.(); this.cleanupFn = undefined; - this.disconnectRoot(); - } - - // Called by the root controller's outlet-connected callback. - connectRoot(root:SortableListsRoot):void { - this.root = root; - this.reflectMoving(root.moving); - } - - disconnectRoot():void { - this.root = undefined; - // The root only reaches still-connected list outlets when it ends a move, so - // a list that disconnects mid-move would otherwise keep aria-busy forever. - this.reflectMoving(false); - } - - reflectMoving(moving:boolean):void { - if (moving) { - this.element.setAttribute('aria-busy', 'true'); - } else { - this.element.removeAttribute('aria-busy'); - } } private get dropPosition():SortableListDropPosition { @@ -125,12 +119,12 @@ export default class ListController extends Controller implements R } private canDrop(data:Record):boolean { - const { root } = this; - if (root == null || root.moving) { + if (!this.hasSortableListsOutlet) { return false; } - return canAccept(root, data); + return isItemFromRoot(this.sortableListsOutlet.element, data) + && listAcceptsType({ acceptedTypes: this.acceptedTypesValue, type: data.type }); } // The list is the item targets' parent drop target, so its onDrag keeps firing diff --git a/modules/backlogs/app/components/backlogs/inbox_component.html.erb b/modules/backlogs/app/components/backlogs/inbox_component.html.erb index a15a01b8cb99..7dfe6e61a017 100644 --- a/modules/backlogs/app/components/backlogs/inbox_component.html.erb +++ b/modules/backlogs/app/components/backlogs/inbox_component.html.erb @@ -40,7 +40,10 @@ See COPYRIGHT and LICENSE files for more details. test_selector: "backlog-inbox", data: { controller: "sortable-lists--list", + sortable_lists__list_sortable_lists_outlet: "#backlogs_container", sortable_lists__list_type_value: "inbox", + sortable_lists__list_accepted_types_value: ["work_package"].to_json, + sortable_lists__list_rows_container_selector_value: ":scope > ul", sortable_lists__list_drop_position_value: "start" } ) diff --git a/modules/backlogs/app/components/backlogs/work_package_card_list_component.rb b/modules/backlogs/app/components/backlogs/work_package_card_list_component.rb index 1df92178b424..48661eb2bce0 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_list_component.rb +++ b/modules/backlogs/app/components/backlogs/work_package_card_list_component.rb @@ -117,7 +117,10 @@ def merge_drag_and_drop_data! def drag_and_drop_data data = { controller: "sortable-lists--list", - sortable_lists__list_type_value: drag_and_drop.fetch(:list_type) + sortable_lists__list_sortable_lists_outlet: "#backlogs_container", + sortable_lists__list_type_value: drag_and_drop.fetch(:list_type), + sortable_lists__list_accepted_types_value: ["work_package"].to_json, + sortable_lists__list_rows_container_selector_value: ":scope > ul" } data[:sortable_lists__list_id_value] = drag_and_drop[:list_id] if drag_and_drop[:list_id].present? data[:sortable_lists__list_drop_position_value] = drag_and_drop[:drop_position] if drag_and_drop[:drop_position].present? diff --git a/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb b/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb index d38e725e05e2..dbf0f73b15ba 100644 --- a/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb +++ b/modules/backlogs/app/components/backlogs/work_package_card_list_item_component.rb @@ -84,6 +84,7 @@ def card_data def draggable_data { controller: "sortable-lists--item", + sortable_lists__item_sortable_lists_outlet: "#backlogs_container", sortable_lists__item_id_value: work_package.id, sortable_lists__item_type_value: "work_package" } diff --git a/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb b/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb index 9e2f23c528c1..f9b6a58e9382 100644 --- a/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb +++ b/modules/backlogs/app/controllers/backlogs/work_packages_controller.rb @@ -67,9 +67,21 @@ def move_to_bucket_dialog end def move # rubocop:disable Metrics/AbcSize + source_list = [@work_package.sprint_id, @work_package.backlog_bucket_id] + call = ::Backlogs::WorkPackages::UpdateService.new(user: current_user, story: @work_package) .call(**move_service_params) + # An optimistic move (drag and drop) has already reordered the row in the + # client DOM. When the item stayed in its list there is nothing further to + # render — sprint totals and blank slates are unaffected — so an empty + # response leaves the optimistic order in place. Menu moves (no optimistic + # param) still need the frame reload to become visible at all. + if optimistic_same_list_move?(call, source_list) + head :no_content + return + end + if call.success? reload_frame_via_turbo_stream("backlogs_container") @@ -107,6 +119,16 @@ def move_params params.permit(:prev_id, :position, :direction, :list_type, :list_id) end + # NOT part of move_params: the service's keyword args do not accept it, and + # params.permit above already keeps it out of move_service_params. + def optimistic_move? + ActiveModel::Type::Boolean.new.cast(params[:optimistic]) + end + + def optimistic_same_list_move?(call, source_list) + call.success? && optimistic_move? && [call.result.sprint_id, call.result.backlog_bucket_id] == source_list + end + # A blank prev_id (drag or menu move to the top of a list) is kept so the # service inserts at the top; nil values (absent prev_id/direction) are # dropped. The service resolves list_type/list_id into the destination list. diff --git a/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb b/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb index c0302f2cfcee..ccf86105eb04 100644 --- a/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/_backlog_list.html.erb @@ -32,10 +32,7 @@ See COPYRIGHT and LICENSE files for more details. class: "op-backlogs-page", data: { controller: "backlogs--list-refresh sortable-lists", - sortable_lists_accepted_type_value: "work_package", - sortable_lists_move_url_template_value: backlogs_move_url_template(@project), - sortable_lists_sortable_lists__list_outlet: "#backlogs_container [data-controller~='sortable-lists--list']", - sortable_lists_sortable_lists__item_outlet: "#backlogs_container [data-controller~='sortable-lists--item']" + sortable_lists_move_url_templates_value: { work_package: backlogs_move_url_template(@project) }.to_json } do %>
                        diff --git a/modules/backlogs/app/views/backlogs/backlog/show.html.erb b/modules/backlogs/app/views/backlogs/backlog/show.html.erb index 2f3d31b5246a..58d2eeff1a3e 100644 --- a/modules/backlogs/app/views/backlogs/backlog/show.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/show.html.erb @@ -49,11 +49,8 @@ See COPYRIGHT and LICENSE files for more details. class: "op-backlogs-page", data: { controller: "backlogs--list-refresh backlogs--split-view-sync sortable-lists", - action: "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->backlogs--split-view-sync#onWorkPackageMoved", - sortable_lists_accepted_type_value: "work_package", - sortable_lists_move_url_template_value: backlogs_move_url_template(@project), - sortable_lists_sortable_lists__list_outlet: "#backlogs_container [data-controller~='sortable-lists--list']", - sortable_lists_sortable_lists__item_outlet: "#backlogs_container [data-controller~='sortable-lists--item']" + action: "#{Backlogs::WorkPackagesController::WORK_PACKAGE_MOVED_EVENT}@document->backlogs--split-view-sync#onWorkPackageMoved sortable-lists:moved@document->backlogs--split-view-sync#onSortableListsMoved", + sortable_lists_move_url_templates_value: { work_package: backlogs_move_url_template(@project) }.to_json } %> <% end %> diff --git a/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb b/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb index 3c183bae94be..a9e657734e60 100644 --- a/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/bucket_component_spec.rb @@ -127,6 +127,14 @@ def render_component end end + it "outlets the list controller to the backlogs root and accepts work packages" do + expect(rendered_component).to have_css(".Box") do |box| + expect(box["data-sortable-lists--list-sortable-lists-outlet"]).to eq("#backlogs_container") + expect(box["data-sortable-lists--list-accepted-types-value"]).to eq('["work_package"]') + expect(box["data-sortable-lists--list-rows-container-selector-value"]).to eq(":scope > ul") + end + end + it "renders the shared work-package row menu with inbox src" do expect(rendered_component).to have_element( "include-fragment", diff --git a/modules/backlogs/spec/components/backlogs/inbox_component_spec.rb b/modules/backlogs/spec/components/backlogs/inbox_component_spec.rb index c1d88d96edd7..7e4cefdc8ff3 100644 --- a/modules/backlogs/spec/components/backlogs/inbox_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/inbox_component_spec.rb @@ -73,6 +73,14 @@ def render_component end end + it "outlets the list controller to the backlogs root and accepts work packages" do + expect(page).to have_css(".Box#inbox_project_#{project.id}") do |box| + expect(box["data-sortable-lists--list-sortable-lists-outlet"]).to eq("#backlogs_container") + expect(box["data-sortable-lists--list-accepted-types-value"]).to eq('["work_package"]') + expect(box["data-sortable-lists--list-rows-container-selector-value"]).to eq(":scope > ul") + end + end + it "announces dynamic empty-state updates" do expect(page).to have_role(:status, aria: { live: "polite" }) end diff --git a/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb b/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb index 86ae5ab0b905..3d4414578038 100644 --- a/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/sprint_component_spec.rb @@ -110,6 +110,14 @@ def menu_items end end + it "outlets the list controller to the backlogs root and accepts work packages" do + expect(rendered_component).to have_css(".Box") do |box| + expect(box["data-sortable-lists--list-sortable-lists-outlet"]).to eq("#backlogs_container") + expect(box["data-sortable-lists--list-accepted-types-value"]).to eq('["work_package"]') + expect(box["data-sortable-lists--list-rows-container-selector-value"]).to eq(":scope > ul") + end + end + it "passes an explicit sprint test selector to the shared box" do expect(rendered_component).to have_css(".Box[data-test-selector='sprint-#{sprint.id}']") end diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb index def10b3239f7..23d69795b413 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_list_component_spec.rb @@ -294,6 +294,14 @@ def render_component(work_packages:, container:, drag_and_drop:) expect(box["data-sortable-lists--list-id-value"]).to eq(sprint.id.to_s) end end + + it "outlets the list controller to the backlogs root and accepts work packages" do + expect(rendered_component).to have_css(".Box") do |box| + expect(box["data-sortable-lists--list-sortable-lists-outlet"]).to eq("#backlogs_container") + expect(box["data-sortable-lists--list-accepted-types-value"]).to eq('["work_package"]') + expect(box["data-sortable-lists--list-rows-container-selector-value"]).to eq(":scope > ul") + end + end end end diff --git a/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb b/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb index 6b957bab192e..82a7c25ac460 100644 --- a/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb +++ b/modules/backlogs/spec/components/backlogs/work_package_card_list_item_component_spec.rb @@ -68,6 +68,7 @@ expect(item.row_args[:test_selector]).to eq("work-package-#{work_package.id}") expect(item.row_args[:data]).to include( controller: "sortable-lists--item", + sortable_lists__item_sortable_lists_outlet: "#backlogs_container", sortable_lists__item_id_value: work_package.id, sortable_lists__item_type_value: "work_package" ) diff --git a/modules/backlogs/spec/features/work_packages/drag_in_inbox_spec.rb b/modules/backlogs/spec/features/work_packages/drag_in_inbox_spec.rb index 1d523255ec98..1c3678a00292 100644 --- a/modules/backlogs/spec/features/work_packages/drag_in_inbox_spec.rb +++ b/modules/backlogs/spec/features/work_packages/drag_in_inbox_spec.rb @@ -141,6 +141,46 @@ end end + context "when the inbox is truncated" do + # tail_size = [TRUNCATE_MIDDLE / 5, 1].max = 1, so with TRUNCATE_MIDDLE + # stubbed to 2, the visible window is first(2) + last(1); the truncate + # threshold (TRUNCATE_MIDDLE + tail_size * 2 = 4) stays below the 5 work + # packages the outer example group already sets up, so the inbox truncates. + before do + stub_const("Backlogs::InboxComponent::TRUNCATE_MIDDLE", 2) + end + + it "keeps the server-rendered truncation window consistent after a same-list drag" do + backlogs_page.visit! + + backlogs_page.expect_work_packages_in_inbox_in_order(work_packages: [inbox_wp1, inbox_wp2]) + backlogs_page.expect_inbox_item(inbox_wp5) + backlogs_page.expect_no_inbox_item(inbox_wp3) + backlogs_page.expect_no_inbox_item(inbox_wp4) + backlogs_page.expect_inbox_show_more + expect(backlogs_page.inbox_truncation_marker_previous_item_id).to eq(inbox_wp4.id.to_s) + + # Same list, but the drop target (the inbox) is truncated: the server + # must recompute the visible window rather than trust the optimistic + # client-side reorder, so this move settles via a turbo-stream frame + # reload rather than the 204/sortable-lists:moved path a plain same-list + # drag takes. `cross_list: true` steers drag_work_package to wait on + # that frame reload (see sortable-lists.controller.ts's `optimistic` + # gate and Backlogs::InboxComponent's TRUNCATE_MIDDLE window). + backlogs_page + .drag_work_package(inbox_wp1, before: inbox_wp5, cross_list: true) + + # inbox_wp1 moved behind inbox_wp4 (the item the marker names), pushing + # it out of the visible head and pulling inbox_wp3 into view instead. + backlogs_page.expect_work_packages_in_inbox_in_order(work_packages: [inbox_wp2, inbox_wp3]) + backlogs_page.expect_inbox_item(inbox_wp5) + backlogs_page.expect_no_inbox_item(inbox_wp1) + backlogs_page.expect_no_inbox_item(inbox_wp4) + backlogs_page.expect_inbox_show_more + expect(backlogs_page.inbox_truncation_marker_previous_item_id).to eq(inbox_wp1.id.to_s) + end + end + context "when lacking the permission to manage sprint items" do current_user do create(:user, diff --git a/modules/backlogs/spec/features/work_packages/drag_in_sprint_spec.rb b/modules/backlogs/spec/features/work_packages/drag_in_sprint_spec.rb index 6e4f4debc5db..4ab52f8b2c1e 100644 --- a/modules/backlogs/spec/features/work_packages/drag_in_sprint_spec.rb +++ b/modules/backlogs/spec/features/work_packages/drag_in_sprint_spec.rb @@ -143,7 +143,7 @@ end it "keeps drop indicators active after moving a bucket item into the sprint" do - backlogs_page.drag_work_package(bucket_wp2, before: sprint1_wp4) + backlogs_page.drag_work_package(bucket_wp2, before: sprint1_wp4, cross_list: true) backlogs_page.expect_work_packages_in_sprint_in_order( sprint1, work_packages: [sprint1_wp1, sprint1_wp2, sprint1_wp3, bucket_wp2, sprint1_wp4] diff --git a/modules/backlogs/spec/features/work_packages/edit_in_split_view_after_move_spec.rb b/modules/backlogs/spec/features/work_packages/edit_in_split_view_after_move_spec.rb index a75104700359..61f3b5e76f60 100644 --- a/modules/backlogs/spec/features/work_packages/edit_in_split_view_after_move_spec.rb +++ b/modules/backlogs/spec/features/work_packages/edit_in_split_view_after_move_spec.rb @@ -96,4 +96,34 @@ def move_work_package(target) it_behaves_like "editing works after the move" end + + # A cross-list drag gets a server-dispatched WORK_PACKAGE_MOVED_EVENT (see + # the controller's #move), which is how the split view above learns to + # refresh its cached lock_version. A same-list drag is answered with a bare + # 204 and dispatches no such event; the split view instead relies on the + # client-side sortable-lists:moved event (see split-view-sync.controller.ts). + # This scenario exercises that client-only path. + context "when moving the work package within its own list by dragging it with the mouse", :selenium do + let!(:other_work_package_in_sprint) { create(:work_package, project:, sprint:) } + let!(:third_work_package_in_sprint) { create(:work_package, project:, sprint:) } + + it "updates the work package without a conflict error" do + backlogs_page.visit! + + split_view = backlogs_page.open_work_package_details(work_package) + + backlogs_page.drag_work_package(work_package, before: third_work_package_in_sprint) + + backlogs_page.expect_work_packages_in_sprint_in_order( + sprint, + work_packages: [other_work_package_in_sprint, work_package, third_work_package_in_sprint] + ) + split_view.expect_attributes(sprint:) + + split_view.edit_field(:subject).update("Updated after move") + + backlogs_page.expect_and_dismiss_toaster(message: "Successful update") + expect(work_package.reload.subject).to eq("Updated after move") + end + end end diff --git a/modules/backlogs/spec/requests/backlogs/backlog_spec.rb b/modules/backlogs/spec/requests/backlogs/backlog_spec.rb index 57933834ec06..e8b813422c2e 100644 --- a/modules/backlogs/spec/requests/backlogs/backlog_spec.rb +++ b/modules/backlogs/spec/requests/backlogs/backlog_spec.rb @@ -88,17 +88,9 @@ expect(response).to have_turbo_frame "backlogs_container" expect(response.body).to include('class="op-sprint-planning-container"') expect(response.body).to include('data-controller="backlogs--list-refresh sortable-lists"') - expect(response.body).to include('data-sortable-lists-accepted-type-value="work_package"') expect(response.body).to include( - %(data-sortable-lists-move-url-template-value="/projects/#{project.identifier}/backlogs/work_packages/{id}/move") - ) - expect(response.body).to include( - "data-sortable-lists-sortable-lists--list-outlet=" \ - "\"#backlogs_container [data-controller~='sortable-lists--list']\"" - ) - expect(response.body).to include( - "data-sortable-lists-sortable-lists--item-outlet=" \ - "\"#backlogs_container [data-controller~='sortable-lists--item']\"" + %(data-sortable-lists-move-url-templates-value="{"work_package":") \ + "/projects/#{project.identifier}/backlogs/work_packages/{id}/move"}\"" ) expect(response.body).to include('id="owner_backlogs_container"') expect(response.body).to include('id="sprint_backlogs_container"') @@ -115,7 +107,7 @@ move_url_template = "/projects/#{project.identifier}/backlogs/work_packages/{id}/move?all=true" expect(response.body).to include( - %(data-sortable-lists-move-url-template-value="#{move_url_template}") + %(data-sortable-lists-move-url-templates-value="{"work_package":"#{move_url_template}"}") ) end diff --git a/modules/backlogs/spec/requests/backlogs/work_packages_move_spec.rb b/modules/backlogs/spec/requests/backlogs/work_packages_move_spec.rb new file mode 100644 index 000000000000..c3ddb45f519f --- /dev/null +++ b/modules/backlogs/spec/requests/backlogs/work_packages_move_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe "Backlogs work package move", :skip_csrf, type: :rails_request do + shared_let(:type_feature) { create(:type_feature) } + shared_let(:user) { create(:admin) } + shared_let(:status) { create(:status, name: "status 1", is_default: true) } + shared_let(:project) { create(:project) } + shared_let(:sprint) { create(:sprint, project:) } + shared_let(:story_one) { create(:work_package, status:, sprint:, project:) } + shared_let(:story_two) { create(:work_package, status:, sprint:, project:) } + + current_user { user } + + describe "PUT #move" do + context "with an optimistic same-list reorder" do + it "responds 204 without a body and persists the reorder" do + put move_project_backlogs_work_package_path(project, story_one), + headers: { "Accept" => "text/vnd.turbo-stream.html" }, + params: { prev_id: story_two.id, list_type: "sprint", list_id: sprint.id, optimistic: "true" } + + expect(response).to have_http_status(:no_content) + expect(response.body).to be_empty + expect(story_one.reload.position).to be > story_two.reload.position + end + end + + context "with a same-list reorder without the optimistic param (menu move)" do + it "responds with a turbo-stream frame reload and persists the reorder" do + put move_project_backlogs_work_package_path(project, story_one), + headers: { "Accept" => "text/vnd.turbo-stream.html" }, + params: { prev_id: story_two.id, list_type: "sprint", list_id: sprint.id } + + expect(response).to have_http_status(:ok) + expect(response.media_type).to eq("text/vnd.turbo-stream.html") + expect(response.body).to include("backlogs_container") + expect(story_one.reload.position).to be > story_two.reload.position + end + end + + context "with a same-list reorder and optimistic explicitly false" do + it "responds with a turbo-stream frame reload, not a 204, and persists the reorder" do + put move_project_backlogs_work_package_path(project, story_one), + headers: { "Accept" => "text/vnd.turbo-stream.html" }, + params: { prev_id: story_two.id, list_type: "sprint", list_id: sprint.id, optimistic: "false" } + + expect(response).to have_http_status(:ok) + expect(response.media_type).to eq("text/vnd.turbo-stream.html") + expect(response.body).to include("backlogs_container") + expect(story_one.reload.position).to be > story_two.reload.position + end + end + + context "with an optimistic cross-list move" do + it "responds with a turbo-stream frame reload and persists the move" do + put move_project_backlogs_work_package_path(project, story_one), + headers: { "Accept" => "text/vnd.turbo-stream.html" }, + params: { prev_id: "", list_type: "inbox", list_id: "", optimistic: "true" } + + expect(response).to have_http_status(:ok) + expect(response.media_type).to eq("text/vnd.turbo-stream.html") + expect(response.body).to include("backlogs_container") + expect(story_one.reload.sprint_id).to be_nil + end + end + + context "with a failing move" do + it "responds with an error flash stream" do + put move_project_backlogs_work_package_path(project, story_one), + headers: { "Accept" => "text/vnd.turbo-stream.html" }, + params: { list_type: "unknown", list_id: "1" } + + expect(response).to have_http_status(:unprocessable_entity) + expect(response.media_type).to eq("text/vnd.turbo-stream.html") + expect(response.body).to include( + I18n.t(:notice_unsuccessful_update_with_reason, + reason: I18n.t("backlogs.stories.update_service.invalid_target_type")) + ) + end + end + end +end diff --git a/modules/backlogs/spec/support/pages/backlog.rb b/modules/backlogs/spec/support/pages/backlog.rb index f630d4d004ce..4317fd2db06b 100644 --- a/modules/backlogs/spec/support/pages/backlog.rb +++ b/modules/backlogs/spec/support/pages/backlog.rb @@ -167,6 +167,17 @@ def click_inbox_show_more wait_for_backlogs_network_idle end + # The truncation marker row's data-sortable-lists-prev-item-id names the + # last work package collapsed behind the "show more" row (the DOM contract + # sortable-lists relies on to anchor drops next to a hidden block). Reading + # it back lets specs assert the server recomputed the truncated window + # rather than trusting a stale client-side reorder. + def inbox_truncation_marker_previous_item_id + within_backlog_inbox do + find(".op-work-package-card-list--show-more-row", visible: :all)["data-sortable-lists-prev-item-id"] + end + end + def expect_work_packages_in_inbox_in_order(work_packages: []) within_backlog_inbox do expect_work_packages_in_order work_packages: @@ -546,17 +557,20 @@ def expect_no_filter_count(type) end end - def drag_work_package(moved, before: nil, into: nil) + # A drag within one list is optimistic: the server answers 204 and no frame + # reload happens, so the settle signal is the sortable-lists:moved event. A + # cross-list drag still reloads the backlogs_container frame. Callers of + # drag_work_package(before:) must pass cross_list: true whenever the move + # takes the frame-reload response path: crossing lists, or staying within a + # truncated list (the client omits the optimistic param there, so the + # server streams a reload). + def drag_work_package(moved, before: nil, into: nil, cross_list: into.present?) raise ArgumentError, "You must specify either before or into" unless before.present? ^ into.present? moved_element = find(draggable_work_package_selector(moved)) - target_element = if before - find(work_package_selector(before)) - else - find(sprint_selector(into)) - end + target_element = before ? find(work_package_selector(before)) : find(sprint_selector(into)) - wait_for_backlogs_turbo_stream(frame_reload: true) do + wait_for_drag_work_package(cross_list:) do drag_backlogs_item(source: moved_element, target: target_element, edge: before ? :top : nil) end rescue Capybara::Cuprite::ObsoleteNode, Selenium::WebDriver::Error::StaleElementReferenceError @@ -829,6 +843,19 @@ def wait_for_backlogs_turbo_stream(wait: Capybara.default_max_wait_time, frame_r end end + # cross_list: true waits for the backlogs_container frame reload — used for + # genuine cross-list drags and for same-list drags in truncated lists, both + # of which take the stream-reload response. A plain same-list drag is + # optimistic (204, no reload) and only ever settles via the + # sortable-lists:moved event. + def wait_for_drag_work_package(cross_list:, &) + if cross_list + wait_for_backlogs_turbo_stream(frame_reload: true, &) + else + wait_for_sortable_lists_moved(&) + end + end + def install_backlogs_dnd_probe(source:, target:, edge:) page.execute_script(<<~JS, source, target, edge&.to_s) window.__opBacklogsDndProbeAbort?.abort(); diff --git a/spec/support/capybara/wait_helpers.rb b/spec/support/capybara/wait_helpers.rb index 206152e06356..b04378fc5a70 100644 --- a/spec/support/capybara/wait_helpers.rb +++ b/spec/support/capybara/wait_helpers.rb @@ -112,6 +112,22 @@ def wait_for_turbo_frame(frame: nil, wait: Capybara.default_max_wait_time, &) wait_for_browser_event("turbo:frame-load", target_id: frame&.to_s, wait:, &) end + # Executes the given block and waits for a sortable-lists move to settle. + # + # The sortable-lists Stimulus root dispatches `sortable-lists:moved` after a + # successful move request, strictly after its moving flag has been cleared — + # so when this returns, the list accepts the next drag. Same-list optimistic + # moves answer with 204 and never reload the frame, making this event the + # only reliable settle signal for them. + # + # @param wait [Integer, true, false, nil] seconds to wait; +true+ uses + # Capybara's default wait time, a falsey value skips the wait and just runs the block + # @yield the actions that trigger the move + # @return [Object] the block's return value + def wait_for_sortable_lists_moved(wait: Capybara.default_max_wait_time, &) + wait_for_browser_event("sortable-lists:moved", wait:, &) + end + private # Shared implementation for the +wait_for_turbo*+ helpers.