Skip to content

fix(drag-drop): fix drag start delay behavior to allow scrolling (#16224) #16537

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/cdk/drag-drop/directives/drag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,24 @@ describe('CdkDrag', () => {
expect(styles.touchAction || (styles as any).webkitUserDrag).toBe('none');
}));

it('should reenable native drag interactions after dragging', fakeAsync(() => {
const fixture = createComponent(StandaloneDraggable);
fixture.detectChanges();
const dragElement = fixture.componentInstance.dragElement.nativeElement;
const styles = dragElement.style;

expect(styles.touchAction || (styles as any).webkitUserDrag).toBeFalsy();

startDraggingViaMouse(fixture, dragElement);
dispatchMouseEvent(document, 'mousemove', 50, 100);
fixture.detectChanges();
expect(styles.touchAction || (styles as any).webkitUserDrag).toBe('none');

dispatchMouseEvent(document, 'mouseup', 50, 100);
fixture.detectChanges();
expect(styles.touchAction || (styles as any).webkitUserDrag).toBeFalsy();
}));

it('should stop propagation for the drag sequence start event', fakeAsync(() => {
const fixture = createComponent(StandaloneDraggable);
fixture.detectChanges();
Expand Down
44 changes: 29 additions & 15 deletions src/cdk/drag-drop/drag-drop-registry.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ describe('DragDropRegistry', () => {
const firstItem = testComponent.dragItems.first;

expect(registry.isDragging(firstItem)).toBe(false);
registry.startDragging(firstItem, createMouseEvent('mousedown'));
registry.initializeDragging(firstItem, createMouseEvent('mousedown'));
expect(registry.isDragging(firstItem)).toBe(true);
});

it('should be able to stop dragging an item', () => {
const firstItem = testComponent.dragItems.first;

registry.startDragging(firstItem, createMouseEvent('mousedown'));
registry.initializeDragging(firstItem, createMouseEvent('mousedown'));
expect(registry.isDragging(firstItem)).toBe(true);

registry.stopDragging(firstItem);
Expand All @@ -57,7 +57,7 @@ describe('DragDropRegistry', () => {
it('should stop dragging an item if it is removed', () => {
const firstItem = testComponent.dragItems.first;

registry.startDragging(firstItem, createMouseEvent('mousedown'));
registry.initializeDragging(firstItem, createMouseEvent('mousedown'));
expect(registry.isDragging(firstItem)).toBe(true);

registry.removeDragItem(firstItem);
Expand All @@ -68,7 +68,7 @@ describe('DragDropRegistry', () => {
const spy = jasmine.createSpy('pointerMove spy');
const subscription = registry.pointerMove.subscribe(spy);

registry.startDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
registry.initializeDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
dispatchMouseEvent(document, 'mousemove');

expect(spy).toHaveBeenCalled();
Expand All @@ -80,7 +80,7 @@ describe('DragDropRegistry', () => {
const spy = jasmine.createSpy('pointerMove spy');
const subscription = registry.pointerMove.subscribe(spy);

registry.startDragging(testComponent.dragItems.first,
registry.initializeDragging(testComponent.dragItems.first,
createTouchEvent('touchstart') as TouchEvent);
dispatchTouchEvent(document, 'touchmove');

Expand All @@ -94,7 +94,7 @@ describe('DragDropRegistry', () => {
const subscription = registry.pointerMove.subscribe(spy);

fixture.nativeElement.addEventListener('mousemove', (e: MouseEvent) => e.stopPropagation());
registry.startDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
registry.initializeDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
dispatchMouseEvent(fixture.nativeElement.querySelector('div'), 'mousemove');

expect(spy).toHaveBeenCalled();
Expand All @@ -106,7 +106,7 @@ describe('DragDropRegistry', () => {
const spy = jasmine.createSpy('pointerUp spy');
const subscription = registry.pointerUp.subscribe(spy);

registry.startDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
registry.initializeDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
dispatchMouseEvent(document, 'mouseup');

expect(spy).toHaveBeenCalled();
Expand All @@ -118,7 +118,7 @@ describe('DragDropRegistry', () => {
const spy = jasmine.createSpy('pointerUp spy');
const subscription = registry.pointerUp.subscribe(spy);

registry.startDragging(testComponent.dragItems.first,
registry.initializeDragging(testComponent.dragItems.first,
createTouchEvent('touchstart') as TouchEvent);
dispatchTouchEvent(document, 'touchend');

Expand All @@ -132,7 +132,7 @@ describe('DragDropRegistry', () => {
const subscription = registry.pointerUp.subscribe(spy);

fixture.nativeElement.addEventListener('mouseup', (e: MouseEvent) => e.stopPropagation());
registry.startDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
registry.initializeDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
dispatchMouseEvent(fixture.nativeElement.querySelector('div'), 'mouseup');

expect(spy).toHaveBeenCalled();
Expand All @@ -159,9 +159,9 @@ describe('DragDropRegistry', () => {
const firstItem = testComponent.dragItems.first;

// First finger down
registry.startDragging(firstItem, createTouchEvent('touchstart') as TouchEvent);
registry.initializeDragging(firstItem, createTouchEvent('touchstart') as TouchEvent);
// Second finger down
registry.startDragging(firstItem, createTouchEvent('touchstart') as TouchEvent);
registry.initializeDragging(firstItem, createTouchEvent('touchstart') as TouchEvent);
// First finger up
registry.stopDragging(firstItem);

Expand Down Expand Up @@ -194,15 +194,23 @@ describe('DragDropRegistry', () => {
expect(dispatchTouchEvent(document, 'touchmove').defaultPrevented).toBe(false);
});

it('should not revent the default `touchmove` action when an item is only initialized', () => {
registry.initializeDragging(testComponent.dragItems.first,
createTouchEvent('touchstart') as TouchEvent);
expect(dispatchTouchEvent(document, 'touchmove').defaultPrevented).toBe(false);
});

it('should prevent the default `touchmove` action when an item is being dragged', () => {
registry.startDragging(testComponent.dragItems.first,
registry.initializeDragging(testComponent.dragItems.first,
createTouchEvent('touchstart') as TouchEvent);
registry.startDragging(testComponent.dragItems.first);
expect(dispatchTouchEvent(document, 'touchmove').defaultPrevented).toBe(true);
});

it('should prevent the default `touchmove` if event propagation is stopped', () => {
registry.startDragging(testComponent.dragItems.first,
registry.initializeDragging(testComponent.dragItems.first,
createTouchEvent('touchstart') as TouchEvent);
registry.startDragging(testComponent.dragItems.first);

fixture.nativeElement.addEventListener('touchmove', (e: TouchEvent) => e.stopPropagation());

Expand All @@ -215,16 +223,22 @@ describe('DragDropRegistry', () => {
expect(dispatchFakeEvent(document, 'selectstart').defaultPrevented).toBe(false);
});

it('should not prevent the default `selectstart` action when an item is initialized', () => {
registry.initializeDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
expect(dispatchFakeEvent(document, 'selectstart').defaultPrevented).toBe(false);
});

it('should prevent the default `selectstart` action when an item is being dragged', () => {
registry.startDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
registry.initializeDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
registry.startDragging(testComponent.dragItems.first);
expect(dispatchFakeEvent(document, 'selectstart').defaultPrevented).toBe(true);
});

it('should dispatch `scroll` events if the viewport is scrolled while dragging', () => {
const spy = jasmine.createSpy('scroll spy');
const subscription = registry.scroll.subscribe(spy);

registry.startDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
registry.initializeDragging(testComponent.dragItems.first, createMouseEvent('mousedown'));
dispatchFakeEvent(document, 'scroll');

expect(spy).toHaveBeenCalled();
Expand Down
31 changes: 22 additions & 9 deletions src/cdk/drag-drop/drag-drop-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,11 @@ export class DragDropRegistry<I, C extends {id: string}> implements OnDestroy {
/** Registered drag item instances. */
private _dragInstances = new Set<I>();

/** Drag item instances for which a drag sequence has been initialized. */
private _initializedDragSequences = new Set<I>();

/** Drag item instances that are currently being dragged. */
private _activeDragInstances = new Set<I>();
private _startedDragSequences = new Set<I>();

/** Keeps track of the event listeners that we've bound to the `document`. */
private _globalListeners = new Map<string, {
Expand Down Expand Up @@ -114,15 +117,15 @@ export class DragDropRegistry<I, C extends {id: string}> implements OnDestroy {
* @param drag Drag instance which is being dragged.
* @param event Event that initiated the dragging.
*/
startDragging(drag: I, event: TouchEvent | MouseEvent) {
initializeDragging(drag: I, event: TouchEvent | MouseEvent) {
// Do not process the same drag twice to avoid memory leaks and redundant listeners
if (this._activeDragInstances.has(drag)) {
if (this._initializedDragSequences.has(drag)) {
return;
}

this._activeDragInstances.add(drag);
this._initializedDragSequences.add(drag);

if (this._activeDragInstances.size === 1) {
if (this._initializedDragSequences.size === 1) {
const isTouchEvent = event.type.startsWith('touch');
const moveEvent = isTouchEvent ? 'touchmove' : 'mousemove';
const upEvent = isTouchEvent ? 'touchend' : 'mouseup';
Expand Down Expand Up @@ -159,18 +162,28 @@ export class DragDropRegistry<I, C extends {id: string}> implements OnDestroy {
}
}

startDragging(drag: I) {
if (this._startedDragSequences.has(drag)) {
return;
}

this._startedDragSequences.add(drag);
}

/** Stops dragging a drag item instance. */
stopDragging(drag: I) {
this._activeDragInstances.delete(drag);

if (this._activeDragInstances.size === 0) {
this._startedDragSequences.delete(drag);
this._initializedDragSequences.delete(drag);

if (this._initializedDragSequences.size === 0) {
this._clearGlobalListeners();
}
}

/** Gets whether a drag item instance is currently being dragged. */
isDragging(drag: I) {
return this._activeDragInstances.has(drag);
return this._initializedDragSequences.has(drag);
}

/**
Expand All @@ -195,7 +208,7 @@ export class DragDropRegistry<I, C extends {id: string}> implements OnDestroy {
* @param event Event whose default action should be prevented.
*/
private _preventDefaultWhileDragging = (event: Event) => {
if (this._activeDragInstances.size) {
if (this._startedDragSequences.size) {
event.preventDefault();
}
}
Expand Down
11 changes: 10 additions & 1 deletion src/cdk/drag-drop/drag-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,11 @@ export class DragRef<T = any> {
return;
}

// We need to prevent default here in case the pointer move starts a scroll sequence.
// If we do not prevent the scroll from starting, then we won't be able to prevent future
// touchemove events.
event.preventDefault();

// Prevent other drag sequences from starting while something in the container is still
// being dragged. This can happen while we're waiting for the drop animation to finish
// and can cause errors, because some elements might still be moving around.
Expand Down Expand Up @@ -600,6 +605,8 @@ export class DragRef<T = any> {
* @param event Browser event object that ended the sequence.
*/
private _endDragSequence(event: MouseEvent | TouchEvent) {
toggleNativeDragInteractions(this._rootElement, true);

// Note that here we use `isDragging` from the service, rather than from `this`.
// The difference is that the one from the service reflects whether a dragging sequence
// has been initiated, whereas the one on `this` includes whether the user has passed
Expand Down Expand Up @@ -646,6 +653,8 @@ export class DragRef<T = any> {

/** Starts the dragging sequence. */
private _startDragSequence(event: MouseEvent | TouchEvent) {
this._dragDropRegistry.startDragging(this);

// Emit the event on the item before the one on the container.
this.started.next({source: this});

Expand Down Expand Up @@ -742,7 +751,7 @@ export class DragRef<T = any> {
this._pointerDirectionDelta = {x: 0, y: 0};
this._pointerPositionAtLastDirectionChange = {x: pointerPosition.x, y: pointerPosition.y};
this._dragStartTime = Date.now();
this._dragDropRegistry.startDragging(this, event);
this._dragDropRegistry.initializeDragging(this, event);
}

/** Cleans up the DOM artifacts that were added to facilitate the element being dragged. */
Expand Down
3 changes: 2 additions & 1 deletion tools/public_api_guard/cdk/drag-drop.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,14 @@ export declare class DragDropRegistry<I, C extends {
readonly scroll: Subject<Event>;
constructor(_ngZone: NgZone, _document: any);
getDropContainer(id: string): C | undefined;
initializeDragging(drag: I, event: TouchEvent | MouseEvent): void;
isDragging(drag: I): boolean;
ngOnDestroy(): void;
registerDragItem(drag: I): void;
registerDropContainer(drop: C): void;
removeDragItem(drag: I): void;
removeDropContainer(drop: C): void;
startDragging(drag: I, event: TouchEvent | MouseEvent): void;
startDragging(drag: I): void;
stopDragging(drag: I): void;
}

Expand Down