mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
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.
This commit is contained in:
parent
ed43940e09
commit
0f44abb72f
4 changed files with 239 additions and 13 deletions
28
src/app/core-ui/drop-list/drop-list.service.spec.ts
Normal file
28
src/app/core-ui/drop-list/drop-list.service.spec.ts
Normal file
|
|
@ -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);
|
||||
}));
|
||||
});
|
||||
});
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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 ? '<task id="hit"></task>' : '<div id="hit"></div>';
|
||||
const subList = `<div class="task-list-inner" data-list-id="SUB" data-id="${opts.listModelId}">${hit}</div>`;
|
||||
const wrapper = document.createElement('div');
|
||||
wrapper.innerHTML = opts.enclosingParentTask
|
||||
? `<div class="task-list-inner" data-list-id="PARENT" data-id="UNDONE"><task>${subList}</task></div>`
|
||||
: 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 =
|
||||
'<div class="task-list-inner" data-list-id="SUB"><div id="hit"></div></div>';
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<HTMLElement>('.task-list-inner');
|
||||
return dropListElement?.dataset['listId'] === 'SUB';
|
||||
const listEl = element?.closest<HTMLElement>('.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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue