From b116d71d3b10732f14c9904d43d2cee6096a0680 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 4 Jul 2026 19:50:57 +0100 Subject: [PATCH 1/4] Generalize sortable-lists row containers and drop intent Resolves sortable-list rows through an explicit rows container so consumers are no longer tied to ul/li-only markup. Rows are the direct children of the container named by the list's optional rowsContainerSelector value (e.g. ':scope > ul' for Primer BorderBox lists, which own their inner ul); without it, the list element itself holds the rows. Row lookup never resolves an ancestor, so an inner row of a nested dual-role surface resolves its own item rather than the outer one. resolveDropIntent computes the rows container once and carries it on the returned intent, so list-only end drops append after the true last row. buildMoveFormData receives the whole drop intent and supports both payload modes: relative (prev_id) and absolute (target_id plus a position counted over item rows only). --- .../sortable-lists/drag-and-drop.spec.ts | 112 +++++++---- .../dynamic/sortable-lists/drag-and-drop.ts | 53 +++-- .../dynamic/sortable-lists/list-dom.spec.ts | 186 +++++++++++++----- .../dynamic/sortable-lists/list-dom.ts | 116 ++++++++--- 4 files changed, 342 insertions(+), 125 deletions(-) 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..79841dae9e84 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 @@ -33,6 +33,7 @@ import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/type import { acceptsSortableItemType, buildMoveFormData, + type DropIntent, isSortableItemData, isSortableListData, resolveDropIntent, @@ -160,32 +161,56 @@ describe('sortable lists drag and drop helpers', () => { }); }); - describe('buildMoveFormData', () => { - it('serializes list data and previous item id for the move endpoint', () => { - const data = buildMoveFormData({ type: 'backlog_bucket', listId: '7', previousItemId: '12' }); + 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); + + return { + listElement, + rowsContainer: container, + previousItemId, + listData: sortableListData({ type: 'sprint', listId, dropPosition: 'end' }), + }; + } - expect(data.get('list_type')).toEqual('backlog_bucket'); - expect(data.get('list_id')).toEqual('7'); - expect(data.get('prev_id')).toEqual('12'); + describe('buildMoveFormData', () => { + it('builds a relative payload from the intent', () => { + const data = buildMoveFormData({ intent: intentFixture(), positionMode: 'relative' }); + 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('serializes a top-of-list move as an empty previous item id', () => { - const data = buildMoveFormData({ type: 'inbox', listId: null, previousItemId: null }); + it('serializes a null list id and previous item as empty strings', () => { + const data = buildMoveFormData({ + intent: intentFixture({ listId: null, previousItemId: null }), + positionMode: 'relative', + }); + expect(data.get('list_id')).toBe(''); + expect(data.get('prev_id')).toBe(''); + }); - expect(data.get('list_type')).toEqual('inbox'); - expect(data.get('list_id')).toEqual(''); - expect(data.get('prev_id')).toEqual(''); + it('builds an absolute payload with a computed position', () => { + const data = buildMoveFormData({ intent: intentFixture({ previousItemId: 'a' }), positionMode: 'absolute' }); + 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(); }); }); + // 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').querySelector('article')!; - - expect(resolvePreviousSortableItemId({ sourceItemId: '1', targetItem: target, closestEdge: 'bottom' })).toEqual('3'); - }); - - it('uses the row item as previous item when the drop target is the row', () => { const target = itemRow('3'); expect(resolvePreviousSortableItemId({ sourceItemId: '1', targetItem: target, closestEdge: 'bottom' })).toEqual('3'); @@ -193,17 +218,7 @@ describe('sortable lists drag and drop helpers', () => { 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')!; - - list.append(first, targetRow); - - expect(resolvePreviousSortableItemId({ sourceItemId: '2', targetItem: target, closestEdge: 'top' })).toEqual('1'); - }); - - it('uses the previous row item when dropping on the top edge of a row target', () => { - const list = document.createElement('ul'); + list.setAttribute('data-controller', 'sortable-lists--list'); const first = itemRow('1'); const targetRow = itemRow('3'); @@ -214,40 +229,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(); }); @@ -359,6 +374,33 @@ describe('sortable lists drag and drop helpers', () => { 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'); + }); + it('prepends to the list when its drop position is start', () => { const { root, list } = buildList(); const sourceList = document.createElement('ul'); 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..d63cb913fe27 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,10 @@ import { type DragLocationHistory } from '@atlaskit/pragmatic-drag-and-drop/type import { resolveItemElement, resolveItemId, + resolveItemPosition, resolveListAppendPreviousItemId, resolvePreviousItemId, + resolveSourceRow, sortableListSelector, } from './list-dom'; @@ -126,20 +128,32 @@ export function sortableListData({ }; } +export type SortablePositionMode = 'relative'|'absolute'; + +// One builder for both payload shapes so call sites hand over the whole drop +// intent; every sortable-lists move is optimistic (the row has already been +// reordered in the DOM), which the `optimistic` param signals to the server. export function buildMoveFormData({ - listId, - previousItemId, - type, + intent, + positionMode, }:{ - listId:string|null; - previousItemId:string|null; - type:string; + intent:DropIntent; + positionMode:SortablePositionMode; }):FormData { const data = new FormData(); - - data.append('list_type', type); - data.append('list_id', listId ?? ''); - data.append('prev_id', previousItemId ?? ''); + data.append('optimistic', 'true'); + + 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; } @@ -195,7 +209,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,18 +229,18 @@ 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 { @@ -270,6 +284,8 @@ export function resolveDropIntent({ return null; } + const rowsContainer = resolveRowsContainer(listElement); + const previousItemId = targetItem?.element instanceof HTMLElement ? resolvePreviousSortableItemId({ sourceItemId: sourceData.itemId, @@ -278,9 +294,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/list-dom.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.spec.ts index b18ca76c98ac..6e565cd535f0 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 @@ -29,7 +29,11 @@ import { captureRowPositions, reorderRows, + resolveItemElement, + resolveItemPosition, resolveListAppendPreviousItemId, + resolveRow, + resolveRowsContainer, restoreRowPositions, } from './list-dom'; @@ -67,120 +71,206 @@ 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); + }); + }); + + 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..715a3201a688 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -39,16 +39,45 @@ 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; + } + + return list.querySelector(selector) ?? list; +} + +// 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; +} -// Rows can sit directly under the list element or inside a nested
        . -const listRowSelector = ':scope > li, :scope > ul > li'; +// 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; + } -function listRows(list:Element):Element[] { - return Array.from(list.querySelectorAll(listRowSelector)); + return resolveRow(resolveRowsContainer(list), sourceElement); } -function firstListRow(list:Element):Element|null { - return list.querySelector(listRowSelector); +function listRows(container:Element):Element[] { + return Array.from(container.children); } export function resolveItemId(element:Element):string|null { @@ -63,9 +92,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 +118,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 +175,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; +} From de79f69ed835eec30b1ae9c3ae0d8878f51dc378 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 4 Jul 2026 19:52:43 +0100 Subject: [PATCH 2/4] Move sortable-lists wiring to leaf outlets Replaces the root-pushed child state (outlet callbacks, the connectRoot handshake, RootAwareChild) with leaf controllers that read the sortable-lists root through a child-declared Stimulus outlet: values act as props down, Pragmatic payloads as events up, and the outlet as the pull-based context read. Lists now declare their own acceptedTypes and item drop targets accept same-type items only; the root's acceptedType value is gone. The item's canDrag becomes the single moving gate: a drop ends the drag before the persist request starts, so the repeated checks in canDrop and canMonitor were unreachable. moving becomes a private field behind a getter, the data-sortable-lists-moving attribute disappears, and aria-busy is set on the root element only. Root turbo frames declare a single moveUrlTemplates value (item type to URL template, JSON-encoded) instead of separate accepted-type/move-url-template values and CSS-selector-based list/item outlet attributes. List components (inbox, sprint, bucket, work package card list) outlet directly to the #backlogs_container root and declare their own acceptedTypes and rowsContainerSelector values; item draggable data outlets to the same root. Updates the affected component and request specs to the new attribute contract with an explicit assertion per list component for the outlet and acceptedTypes wiring. --- .../dynamic/sortable-lists.controller.spec.ts | 214 +++++------ .../dynamic/sortable-lists.controller.ts | 158 +++----- .../sortable-lists/drag-and-drop.spec.ts | 44 ++- .../dynamic/sortable-lists/drag-and-drop.ts | 54 +-- .../sortable-lists/item.controller.spec.ts | 357 ++++++++---------- .../dynamic/sortable-lists/item.controller.ts | 46 +-- .../dynamic/sortable-lists/list-dom.ts | 1 - .../sortable-lists/list.controller.spec.ts | 142 +++---- .../dynamic/sortable-lists/list.controller.ts | 60 ++- .../backlogs/inbox_component.html.erb | 3 + .../work_package_card_list_component.rb | 5 +- .../work_package_card_list_item_component.rb | 1 + .../backlogs/backlog/_backlog_list.html.erb | 5 +- .../app/views/backlogs/backlog/show.html.erb | 5 +- .../backlogs/bucket_component_spec.rb | 8 + .../backlogs/inbox_component_spec.rb | 8 + .../backlogs/sprint_component_spec.rb | 8 + .../work_package_card_list_component_spec.rb | 8 + ...k_package_card_list_item_component_spec.rb | 1 + .../spec/requests/backlogs/backlog_spec.rb | 14 +- 20 files changed, 523 insertions(+), 619 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 3027c4935aec..13c4f6f92d15 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 = `
              @@ -151,7 +134,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 +205,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 +230,7 @@ describe('Sortable lists controller', () => {
              • @@ -258,10 +239,7 @@ describe('Sortable lists controller', () => {
                • @@ -313,9 +291,9 @@ describe('Sortable lists controller', () => { expect(options.body.get('prev_id')).toEqual('5'); }); - it('builds the move URL from the controller URI template', async () => { + 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 +305,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 +350,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 +385,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 +473,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 +587,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..bbb8880eb0db 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -40,15 +40,15 @@ import { buildMoveFormData, isSortableItemData, resolveDropIntent, - type RootAwareChild, - type SortableListData, - type SortableListsRoot, + type DropIntent, + type SortableItemData, + type SortablePositionMode, } from './sortable-lists/drag-and-drop'; import { captureRowPositions, reorderRows, + resolveSourceRow, restoreRowPositions, - sortableListsMovingAttribute, } from './sortable-lists/list-dom'; type CleanupFn = () => void; @@ -60,36 +60,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 +101,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 +121,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 +145,51 @@ 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 is answered + // with 204 and this DOM order is final; a cross-list move 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 result = await this.moveItem({ intent, moveUrl }); - 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 { @@ -243,27 +213,11 @@ export default class SortableListsController extends Controller imp return /^[a-z][a-z\d+\-.]*:/i.test(moveUrl) ? url.toString() : `${url.pathname}${url.search}${url.hash}`; } - private async moveItem({ - listData, - previousItemId, - moveUrl, - }:{ - listData:SortableListData; - previousItemId:string|null; - moveUrl:string; - }):Promise { - const request = new FetchRequest( - 'put', - moveUrl, - { - body: buildMoveFormData({ - listId: listData.listId, - previousItemId, - type: listData.type, - }), - responseKind: 'turbo-stream', - }, - ); + private async moveItem({ intent, moveUrl }:{ intent:DropIntent; moveUrl:string }):Promise { + const request = new FetchRequest('put', moveUrl, { + body: buildMoveFormData({ intent, positionMode: this.positionMode }), + responseKind: 'turbo-stream', + }); this.setMoving(true); try { @@ -284,13 +238,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 79841dae9e84..23b21ab48689 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,11 +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, @@ -147,17 +148,41 @@ 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('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 an item without a root element', () => { + expect(isItemFromRoot(root, sortableItemData({ type: 'work_package', itemId: '1' }))).toBe(false); + }); + + it('rejects a null root', () => { + expect(isItemFromRoot(null, sortableItemData({ type: 'work_package', itemId: '1', rootElement: root }))).toBe(false); + }); + + 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('allows drops when the source type matches the accepted type', () => { - expect(acceptsSortableItemType({ acceptedType: 'work_package', type: 'work_package' })).toBe(true); + it('rejects a type not contained in acceptedTypes', () => { + expect(listAcceptsType({ acceptedTypes: ['work_package'], type: 'sprint' })).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 everything for an empty acceptedTypes', () => { + expect(listAcceptsType({ acceptedTypes: [], type: 'work_package' })).toBe(false); }); }); @@ -322,6 +347,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'); }); @@ -371,6 +397,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('5'); }); @@ -421,6 +448,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 d63cb913fe27..77cac28f37ab 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 @@ -37,6 +37,7 @@ import { resolveItemPosition, resolveListAppendPreviousItemId, resolvePreviousItemId, + resolveRowsContainer, resolveSourceRow, sortableListSelector, } from './list-dom'; @@ -63,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' @@ -158,29 +143,27 @@ export function buildMoveFormData({ 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({ @@ -246,6 +229,7 @@ function resolveListOnlyPreviousItemId({ export interface DropIntent { listElement:HTMLElement; listData:SortableListData; + rowsContainer:HTMLElement; previousItemId:string|null; } 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.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts index 715a3201a688..cd923e0a8b7f 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -34,7 +34,6 @@ // 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"]'; 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/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..9f27e7c53bf5 100644 --- a/modules/backlogs/app/views/backlogs/backlog/show.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/show.html.erb @@ -50,10 +50,7 @@ See COPYRIGHT and LICENSE files for more details. 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']" + 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/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 From d6d901441807a80ba83beb2e445d60b2a6ddcbdd Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 4 Jul 2026 17:36:38 +0100 Subject: [PATCH 3/4] Refresh split view on client-side move events Extracts the shared cache-refresh logic in SplitViewSyncController into a private refreshWorkPackage helper and adds onSortableListsMoved, bound to the client-dispatched sortable-lists:moved document event. Same-list moves resolve with a 204 and never trigger the server-dispatched backlogs:work-package-moved event, so the split view's cached work package would otherwise keep a stale lock_version after such a move. Adds the second action binding to the backlogs turbo frame in show.html.erb, alongside two new spec cases covering the refresh and the no-item-id guard. --- .../split-view-sync.controller.spec.ts | 28 ++++++++++++++++- .../backlogs/split-view-sync.controller.ts | 31 ++++++++++++++----- .../app/views/backlogs/backlog/show.html.erb | 2 +- 3 files changed, 52 insertions(+), 9 deletions(-) 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/modules/backlogs/app/views/backlogs/backlog/show.html.erb b/modules/backlogs/app/views/backlogs/backlog/show.html.erb index 9f27e7c53bf5..58d2eeff1a3e 100644 --- a/modules/backlogs/app/views/backlogs/backlog/show.html.erb +++ b/modules/backlogs/app/views/backlogs/backlog/show.html.erb @@ -49,7 +49,7 @@ 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", + 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 %> From 6f399c7b0433a5193637a841edbf88b0e90c9324 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Sat, 4 Jul 2026 19:53:18 +0100 Subject: [PATCH 4/4] Enable optimistic same-list moves with stream fallbacks Makes sortable-lists mark safe moves as optimistic so Backlogs::WorkPackagesController#move can answer a same-list drag with 204 No Content: the client has already reordered the row and nothing further needs to render. Menu-driven moves carry no optimistic param and cross-list moves change lists, so both keep the turbo-stream frame reload they need for sprint totals, blank slates, and buckets that no longer match their filter. The param stays out of the service params (the update service takes keyword arguments) and is cast with ActiveModel::Type::Boolean, so optimistic=false is not treated as truthy. Truncated lists opt out of the optimistic param: their visible window (a head/tail slice plus a show-more marker row) is server-computed, so hasTruncationMarkerRow keeps them on the stream reload path while the optimistic DOM reorder still happens client-side for responsiveness. Also hardens resolveRowsContainer against invalid rowsContainerSelector values, warning and falling back to the list element instead of letting querySelector's SyntaxError break the drop. On success the root dispatches sortable-lists:moved strictly after its moving flag clears; the split view listens to it (the 204 path has no server-dispatched event) and the new wait_for_sortable_lists_moved helper makes same-list drag specs deterministic, while cross-list and truncated-list drags keep waiting on the frame reload. Request specs cover the optimistic same-list, menu same-list, cross-list, optimistic=false, and failure branches; Selenium specs cover the truncated inbox and a same-list drag with the split view open. --- .../dynamic/sortable-lists.controller.spec.ts | 29 +++++ .../dynamic/sortable-lists.controller.ts | 26 ++-- .../sortable-lists/drag-and-drop.spec.ts | 14 ++- .../dynamic/sortable-lists/drag-and-drop.ts | 13 +- .../dynamic/sortable-lists/list-dom.spec.ts | 33 ++++++ .../dynamic/sortable-lists/list-dom.ts | 16 ++- .../backlogs/work_packages_controller.rb | 22 ++++ .../work_packages/drag_in_inbox_spec.rb | 40 +++++++ .../work_packages/drag_in_sprint_spec.rb | 2 +- .../edit_in_split_view_after_move_spec.rb | 30 +++++ .../backlogs/work_packages_move_spec.rb | 111 ++++++++++++++++++ .../backlogs/spec/support/pages/backlog.rb | 41 +++++-- spec/support/capybara/wait_helpers.rb | 16 +++ 13 files changed, 372 insertions(+), 21 deletions(-) create mode 100644 modules/backlogs/spec/requests/backlogs/work_packages_move_spec.rb diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts index 13c4f6f92d15..79b1e09fce2a 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts @@ -118,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 = `
                          @@ -291,6 +297,29 @@ describe('Sortable lists controller', () => { expect(options.body.get('prev_id')).toEqual('5'); }); + 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({ moveUrlTemplates: { work_package: '/projects/demo/backlogs/work_packages/{id}/move' }, diff --git a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts index bbb8880eb0db..19a1af50f65e 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.ts @@ -46,6 +46,7 @@ import { } from './sortable-lists/drag-and-drop'; import { captureRowPositions, + hasTruncationMarkerRow, reorderRows, resolveSourceRow, restoreRowPositions, @@ -150,15 +151,18 @@ export default class SortableListsController extends Controller { return; } - // Move the row optimistically, then persist. A same-list move is answered - // with 204 and this DOM order is final; a cross-list move gets a - // turbo-stream frame reload that reconciles the list. 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, container: intent.rowsContainer, previousItemId: intent.previousItemId }); - const result = await this.moveItem({ intent, moveUrl }); + const optimistic = !hasTruncationMarkerRow(intent.rowsContainer); + const result = await this.moveItem({ intent, moveUrl, optimistic }); if (result.ok) { // After moveItem's finally: moving is false again, so listeners resumed @@ -213,9 +217,17 @@ export default class SortableListsController extends Controller { return /^[a-z][a-z\d+\-.]*:/i.test(moveUrl) ? url.toString() : `${url.pathname}${url.search}${url.hash}`; } - private async moveItem({ intent, moveUrl }:{ intent:DropIntent; moveUrl:string }):Promise { + private async moveItem({ + intent, + moveUrl, + optimistic, + }:{ + intent:DropIntent; + moveUrl:string; + optimistic:boolean; + }):Promise { const request = new FetchRequest('put', moveUrl, { - body: buildMoveFormData({ intent, positionMode: this.positionMode }), + body: buildMoveFormData({ intent, positionMode: this.positionMode, optimistic }), responseKind: 'turbo-stream', }); 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 23b21ab48689..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 @@ -204,7 +204,7 @@ describe('sortable lists drag and drop helpers', () => { describe('buildMoveFormData', () => { it('builds a relative payload from the intent', () => { - const data = buildMoveFormData({ intent: intentFixture(), positionMode: 'relative' }); + 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'); @@ -216,18 +216,28 @@ describe('sortable lists drag and drop helpers', () => { 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(''); }); it('builds an absolute payload with a computed position', () => { - const data = buildMoveFormData({ intent: intentFixture({ previousItemId: 'a' }), positionMode: 'absolute' }); + 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('omits the optimistic param when the move is not optimistic', () => { + const data = buildMoveFormData({ intent: intentFixture(), positionMode: 'relative', optimistic: false }); + expect(data.get('optimistic')).toBeNull(); + }); }); // The drop target Pragmatic DnD reports is always the item controller's own 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 77cac28f37ab..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 @@ -116,17 +116,24 @@ export function sortableListData({ export type SortablePositionMode = 'relative'|'absolute'; // One builder for both payload shapes so call sites hand over the whole drop -// intent; every sortable-lists move is optimistic (the row has already been -// reordered in the DOM), which the `optimistic` param signals to the server. +// 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({ intent, positionMode, + optimistic, }:{ intent:DropIntent; positionMode:SortablePositionMode; + optimistic:boolean; }):FormData { const data = new FormData(); - data.append('optimistic', 'true'); + if (optimistic) { + data.append('optimistic', 'true'); + } if (positionMode === 'absolute') { data.append('target_id', intent.listData.listId ?? ''); 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 6e565cd535f0..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,8 +26,11 @@ // See COPYRIGHT and LICENSE files for more details. //++ +import { vi } from 'vitest'; + import { captureRowPositions, + hasTruncationMarkerRow, reorderRows, resolveItemElement, resolveItemPosition, @@ -202,6 +205,36 @@ describe('sortable lists DOM helpers', () => { 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', () => { 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 cd923e0a8b7f..1ab5fb8f7a1d 100644 --- a/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts +++ b/frontend/src/stimulus/controllers/dynamic/sortable-lists/list-dom.ts @@ -51,7 +51,21 @@ export function resolveRowsContainer(list:HTMLElement):HTMLElement { return list; } - return list.querySelector(selector) ?? list; + try { + return list.querySelector(selector) ?? list; + } catch (error) { + console.warn(`Invalid ${rowsContainerSelectorAttribute} selector "${selector}" on`, list, error); + return list; + } +} + +// 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; } // A row is the direct child of the rows container that contains the element. 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/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/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.