From 0f44abb72ff2f92580fea30d1f4e4204e22e1b0d Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Mon, 1 Jun 2026 21:52:22 +0200 Subject: [PATCH] fix(tasks): reliably convert subtasks dragged to the top-level list Two issues prevented dragging a subtask out to the top-level list to convert it into a main task: - CDK only caches a sibling drop-list's geometry when its enterPredicate passes at drag start (_startReceiving). The pointer is always over the source subtask list then, so the top-level list was never cached and conversion silently failed until an unrelated parent drag warmed it. Open a one-microtask accept window at subtask drag start so CDK caches the top-level lists' geometry; the pointer guard resumes afterwards. - An expanded neighbour's subtask list "caught" the drag in the dead-band just above the next parent (sibling order resolves subtask lists before the top-level list), silently re-parenting the subtask instead of converting it, and growing/sticking once entered. Treat only an actual subtask row as "inside" a foreign subtask list; its trailing padding now falls through to the top-level list for conversion. The source list still blocks anywhere, so in-list sorting is unaffected. --- .../drop-list/drop-list.service.spec.ts | 28 ++++ .../core-ui/drop-list/drop-list.service.ts | 31 +++++ .../task-list/task-list.component.spec.ts | 125 +++++++++++++++++- .../tasks/task-list/task-list.component.ts | 68 ++++++++-- 4 files changed, 239 insertions(+), 13 deletions(-) create mode 100644 src/app/core-ui/drop-list/drop-list.service.spec.ts diff --git a/src/app/core-ui/drop-list/drop-list.service.spec.ts b/src/app/core-ui/drop-list/drop-list.service.spec.ts new file mode 100644 index 0000000000..5dc4900690 --- /dev/null +++ b/src/app/core-ui/drop-list/drop-list.service.spec.ts @@ -0,0 +1,28 @@ +import { fakeAsync, flushMicrotasks, TestBed } from '@angular/core/testing'; +import { DropListService } from './drop-list.service'; + +describe('DropListService', () => { + let service: DropListService; + + beforeEach(() => { + TestBed.configureTestingModule({ providers: [DropListService] }); + service = TestBed.inject(DropListService); + }); + + describe('subtask drag-start window', () => { + it('is closed by default', () => { + expect(service.isSubTaskDragStarting()).toBe(false); + }); + + it('opens synchronously and closes on the next microtask', fakeAsync(() => { + service.markSubTaskDragStarting(); + // Must stay open through CDK's synchronous `_startReceiving` pass so the + // top-level lists get their geometry cached. + expect(service.isSubTaskDragStarting()).toBe(true); + + flushMicrotasks(); + // Closed again before the first pointer move so the pointer guard resumes. + expect(service.isSubTaskDragStarting()).toBe(false); + })); + }); +}); diff --git a/src/app/core-ui/drop-list/drop-list.service.ts b/src/app/core-ui/drop-list/drop-list.service.ts index 62c8df0269..8e0ea7b193 100644 --- a/src/app/core-ui/drop-list/drop-list.service.ts +++ b/src/app/core-ui/drop-list/drop-list.service.ts @@ -28,6 +28,7 @@ export class DropListService { private _list: CdkDropList[] = []; private _flushScheduled = false; private _activeDragPointer: DragPointer | null = null; + private _isSubTaskDragStarting = false; activeDragPointer(): DragPointer | null { return this._activeDragPointer; @@ -37,6 +38,36 @@ export class DropListService { this._activeDragPointer = pointer; } + /** + * True only for the microtask in which a subtask drag begins. + * + * CDK caches a sibling drop-list's geometry (`DropListRef._domRect`) lazily, + * and for a non-source list only when its `enterPredicate` passes at drag + * start (`_startReceiving`). An uncached list can never be entered, so it can + * never receive the item. The parent DONE/UNDONE list normally rejects a + * subtask drag while the pointer is over the source subtask list (so in-list + * sorting keeps working) — but at the *instant* a subtask drag starts the + * pointer is always over that subtask list, so that guard would block the + * parent list from ever being cached, and converting a subtask back to a main + * task would silently fail until some unrelated parent drag warmed the cache. + * + * This flag opens a one-microtask window at drag start during which the + * top-level lists accept the drag (letting CDK cache their geometry); the + * pointer guard then resumes for the rest of the drag. + */ + isSubTaskDragStarting(): boolean { + return this._isSubTaskDragStarting; + } + + markSubTaskDragStarting(): void { + this._isSubTaskDragStarting = true; + // Clear after CDK's synchronous `_startReceiving` pass (which runs right + // after the `cdkDragStarted` output) but before the first pointer move. + queueMicrotask(() => { + this._isSubTaskDragStarting = false; + }); + } + registerDropList(dropList: CdkDropList, isSubTaskList = false): void { if (isSubTaskList) { this._list.unshift(dropList); diff --git a/src/app/features/tasks/task-list/task-list.component.spec.ts b/src/app/features/tasks/task-list/task-list.component.spec.ts index db6c881124..1eabdd3b33 100644 --- a/src/app/features/tasks/task-list/task-list.component.spec.ts +++ b/src/app/features/tasks/task-list/task-list.component.spec.ts @@ -26,6 +26,8 @@ describe('TaskListComponent', () => { unregisterDropList: jasmine.Spy; activeDragPointer: jasmine.Spy; setActiveDragPointer: jasmine.Spy; + isSubTaskDragStarting: jasmine.Spy; + markSubTaskDragStarting: jasmine.Spy; blockAniTrigger$: { next: jasmine.Spy }; }; let store: MockStore; @@ -62,6 +64,10 @@ describe('TaskListComponent', () => { unregisterDropList: jasmine.createSpy('unregisterDropList'), activeDragPointer: jasmine.createSpy('activeDragPointer').and.returnValue(null), setActiveDragPointer: jasmine.createSpy('setActiveDragPointer'), + isSubTaskDragStarting: jasmine + .createSpy('isSubTaskDragStarting') + .and.returnValue(false), + markSubTaskDragStarting: jasmine.createSpy('markSubTaskDragStarting'), blockAniTrigger$: { next: jasmine.createSpy('next') }, }; @@ -157,7 +163,94 @@ describe('TaskListComponent', () => { expect(component.enterPredicate(drag, drop)).toBe(true); }); - it('should block the enclosing parent list while pointer is over a subtask list', () => { + // Mounts a detached `.task-list-inner[data-list-id=SUB]` and points + // `elementFromPoint` at either a real subtask row or the list's empty + // padding, so enterPredicate exercises the live pointer hit-test. + const withPointerOverSubList = ( + opts: { listModelId: string; overRow: boolean; enclosingParentTask?: boolean }, + run: () => void, + ): void => { + const hit = opts.overRow ? '' : '
'; + const subList = `
${hit}
`; + const wrapper = document.createElement('div'); + wrapper.innerHTML = opts.enclosingParentTask + ? `
${subList}
` + : subList; + document.body.appendChild(wrapper); + const hitEl = wrapper.querySelector('#hit') as Element; + spyOn(document, 'elementFromPoint').and.returnValue(hitEl); + dropListServiceMock.activeDragPointer.and.returnValue({ x: 10, y: 20 }); + try { + run(); + } finally { + wrapper.remove(); + } + }; + + const subtaskDragFrom = (sourceModelId: string): CdkDrag => { + const drag = createMockDrag({ id: 'sub1', parentId: 'parent1' }); + Object.assign(drag, { + dropContainer: { data: { listId: 'SUB', listModelId: sourceModelId } }, + }); + return drag; + }; + + it('should block the top-level list while the pointer is over the SOURCE subtask list (even its padding) so in-list sorting keeps working', () => { + withPointerOverSubList({ listModelId: 'parent1', overRow: false }, () => { + const drag = subtaskDragFrom('parent1'); + const drop = createMockDrop('UNDONE', [{ id: 'parent1' }], 'PARENT'); + expect(component.enterPredicate(drag, drop)).toBe(false); + }); + }); + + it('should block the top-level list while the pointer is over a foreign subtask ROW (re-parent intent)', () => { + withPointerOverSubList({ listModelId: 'parent2', overRow: true }, () => { + const drag = subtaskDragFrom('parent1'); + const drop = createMockDrop('UNDONE', [{ id: 'parent2' }], 'PARENT'); + expect(component.enterPredicate(drag, drop)).toBe(false); + }); + }); + + it('should accept the top-level list over a foreign subtask list trailing padding — the dead-band above the next parent (regression: #7905)', () => { + withPointerOverSubList({ listModelId: 'parent2', overRow: false }, () => { + const drag = subtaskDragFrom('parent1'); + const drop = createMockDrop('UNDONE', [{ id: 'parent2' }], 'PARENT'); + expect(component.enterPredicate(drag, drop)).toBe(true); + }); + }); + + it('should not mistake the enclosing parent task for a row when over a foreign subtask list padding', () => { + withPointerOverSubList( + { listModelId: 'parent2', overRow: false, enclosingParentTask: true }, + () => { + const drag = subtaskDragFrom('parent1'); + const drop = createMockDrop('UNDONE', [{ id: 'parent2' }], 'PARENT'); + expect(component.enterPredicate(drag, drop)).toBe(true); + }, + ); + }); + + it('should reject a foreign subtask list as drop target over its trailing padding (so the drag converts instead of re-parenting)', () => { + withPointerOverSubList({ listModelId: 'parent2', overRow: false }, () => { + const drag = subtaskDragFrom('parent1'); + const drop = createMockDrop('parent2', [{ id: 'sub3' }], 'SUB'); + expect(component.enterPredicate(drag, drop)).toBe(false); + }); + }); + + it('should accept a foreign subtask list as drop target over one of its rows (re-parent)', () => { + withPointerOverSubList({ listModelId: 'parent2', overRow: true }, () => { + const drag = subtaskDragFrom('parent1'); + const drop = createMockDrop('parent2', [{ id: 'sub3' }], 'SUB'); + expect(component.enterPredicate(drag, drop)).toBe(true); + }); + }); + + it('should accept the enclosing parent list during the drag-start window even while the pointer is over a subtask list', () => { + // At drag start the pointer is always over the source subtask list, so + // the pointer guard alone would keep CDK from ever caching the parent + // list geometry, leaving subtask -> main-task conversion broken until an + // unrelated parent drag warmed the cache (regression: #7905). const wrapper = document.createElement('div'); wrapper.innerHTML = '
'; @@ -165,6 +258,7 @@ describe('TaskListComponent', () => { const hitEl = wrapper.querySelector('#hit') as Element; spyOn(document, 'elementFromPoint').and.returnValue(hitEl); dropListServiceMock.activeDragPointer.and.returnValue({ x: 10, y: 20 }); + dropListServiceMock.isSubTaskDragStarting.and.returnValue(true); const drag = createMockDrag({ id: 'sub1', parentId: 'parent1' }); Object.assign(drag, { dropContainer: { data: { listId: 'SUB', listModelId: 'parent1' } }, @@ -172,7 +266,7 @@ describe('TaskListComponent', () => { const drop = createMockDrop('UNDONE', [{ id: 'parent1' }], 'PARENT'); try { - expect(component.enterPredicate(drag, drop)).toBe(false); + expect(component.enterPredicate(drag, drop)).toBe(true); } finally { wrapper.remove(); } @@ -416,6 +510,33 @@ describe('TaskListComponent', () => { }); }); + describe('onDragStarted', () => { + const createStartEvent = (): { source: { _dragRef: unknown } } => ({ + source: { _dragRef: {} }, + }); + + afterEach(() => { + // Tear down the window pointermove listener registered for subtask drags. + component.onDragEnded(); + }); + + it('opens the drag-start window for a subtask drag', () => { + component.onDragStarted( + { id: 'sub1', parentId: 'parent1' } as TaskWithSubTasks, + createStartEvent() as never, + ); + expect(dropListServiceMock.markSubTaskDragStarting).toHaveBeenCalledTimes(1); + }); + + it('does NOT open the drag-start window for a top-level task drag', () => { + component.onDragStarted( + { id: 'top1' } as TaskWithSubTasks, + createStartEvent() as never, + ); + expect(dropListServiceMock.markSubTaskDragStarting).not.toHaveBeenCalled(); + }); + }); + // _move() routes drag drops to the correct dispatch path. The crux of the // section feature: a non-reserved listModelId must only be treated as a // section when listId === 'PARENT'; subtask drop-lists ('SUB') also use diff --git a/src/app/features/tasks/task-list/task-list.component.ts b/src/app/features/tasks/task-list/task-list.component.ts index f916c875f5..db72f33627 100644 --- a/src/app/features/tasks/task-list/task-list.component.ts +++ b/src/app/features/tasks/task-list/task-list.component.ts @@ -176,7 +176,7 @@ export class TaskListComponent implements OnDestroy, AfterViewInit { onDragPointerDown(task: TaskWithSubTasks, event: PointerEvent): void { // Seed the pointer position so subtask -> parent-list drags can hit-test // the source subtask list before the first pointermove (see - // _isPointerOverSubTaskList). + // _pointerSubTaskList). if (task.parentId) { this.dropListService.setActiveDragPointer({ x: event.clientX, y: event.clientY }); } @@ -185,6 +185,10 @@ export class TaskListComponent implements OnDestroy, AfterViewInit { onDragStarted(task: TaskWithSubTasks, event: CdkDragStart): void { this._scheduleExternalDragService.setActiveTask(task, event.source._dragRef); if (task.parentId) { + // Runs synchronously before CDK's `_startReceiving` pass, so the + // top-level lists get their geometry cached even though the pointer is + // still over the source subtask list (see markSubTaskDragStarting). + this.dropListService.markSubTaskDragStarting(); this._startDragPointerTracking(); } } @@ -228,11 +232,26 @@ export class TaskListComponent implements OnDestroy, AfterViewInit { const isToTopLevelList = targetModelId === 'DONE' || targetModelId === 'UNDONE'; if (isToTopLevelList) { + // Accept during the drag-start window so CDK caches this list's + // geometry (see markSubTaskDragStarting). if ( drag.dropContainer?.data?.listId === 'SUB' && - this._isPointerOverSubTaskList() + !this.dropListService.isSubTaskDragStarting() ) { - return false; + const overList = this._pointerSubTaskList(); + const sourceModelId = drag.dropContainer?.data?.listModelId; + // Keep the drag inside a subtask list (reject this top-level list) + // while the pointer is over the SOURCE list — anywhere, so in-list + // sorting keeps routing to the subtask list — or over an actual row + // of a foreign list (the user is re-parenting). Over a foreign list's + // trailing padding (the dead-band just above the next parent task), + // fall through so the subtask converts to a main task there. + if ( + overList && + (overList.listModelId === sourceModelId || overList.isOverRow) + ) { + return false; + } } return true; } @@ -241,6 +260,14 @@ export class TaskListComponent implements OnDestroy, AfterViewInit { // task id as listModelId). Reject section drop-lists (listId === 'PARENT' // with a non-reserved id) — section.taskIds is parent-only. if (targetListId === 'SUB' && !PARENT_ALLOWED_LISTS.includes(targetModelId)) { + // Only claim the drop while the pointer is over an actual row of THIS + // list. Over its trailing padding, fall through so the enclosing + // top-level list can convert the subtask to a main task instead of this + // list greedily re-parenting it (see _pointerSubTaskList). + const overList = this._pointerSubTaskList(); + if (overList && overList.listModelId === targetModelId && !overList.isOverRow) { + return false; + } return true; } return false; @@ -275,18 +302,37 @@ export class TaskListComponent implements OnDestroy, AfterViewInit { return true; }; - private _isPointerOverSubTaskList(): boolean { - // CDK intentionally excludes the source list from normal sibling enter - // resolution. For subtask -> parent-list drags we still need to know when - // the pointer is physically over the source subtask list, otherwise the - // parent DONE/UNDONE list accepts the drag and prevents in-list sorting. + /** + * Resolves which subtask list (if any) the drag pointer is currently over, + * and whether it sits over an actual subtask *row* rather than the list's + * empty trailing padding. + * + * CDK excludes the source list from normal sibling enter-resolution, so we + * hit-test the live pointer ourselves to keep the enclosing top-level list + * from stealing in-list sorting. The row distinction matters for the + * dead-band just above a parent task: an expanded neighbour's subtask-list + * box (and its host padding) overshoots a few px below its last row, and + * because subtask lists are resolved before the top-level list (sibling + * order), a pointer aimed at "the slot above the next parent" would be + * greedily claimed by that neighbour and re-parent the subtask. Treating only + * a real row as "inside" a list lets that trailing padding convert to a main + * task instead. + */ + private _pointerSubTaskList(): { listModelId: string; isOverRow: boolean } | null { const pointer = this.dropListService.activeDragPointer(); if (!pointer) { - return false; + return null; } const element = document.elementFromPoint(pointer.x, pointer.y); - const dropListElement = element?.closest('.task-list-inner'); - return dropListElement?.dataset['listId'] === 'SUB'; + const listEl = element?.closest('.task-list-inner'); + if (listEl?.dataset['listId'] !== 'SUB') { + return null; + } + // A `task` ancestor only counts as a row of *this* list — the enclosing + // parent task is also a `task`, but its nearest list is the top-level one. + const rowEl = element?.closest('task'); + const isOverRow = !!rowEl && rowEl.closest('.task-list-inner') === listEl; + return { listModelId: listEl.dataset['id'] ?? '', isOverRow }; } async drop(