Skip to content
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

fix(cdk/drag-drop): not stopping drag if page is blurred #17848

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/cdk/drag-drop/directives/drag.spec.ts
Expand Up @@ -4474,6 +4474,27 @@ describe('CdkDrag', () => {
'Expected placeholder to preserve transform when dragging stops.');
}));

it('should stop dragging if the page is blurred', fakeAsync(() => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
const dragItems = fixture.componentInstance.dragItems;

expect(fixture.componentInstance.droppedSpy).not.toHaveBeenCalled();

const item = dragItems.first;
const targetRect = dragItems.toArray()[2].element.nativeElement.getBoundingClientRect();

startDraggingViaMouse(fixture, item.element.nativeElement);
dispatchMouseEvent(document, 'mousemove', targetRect.left + 1, targetRect.top + 1);
fixture.detectChanges();

dispatchFakeEvent(window, 'blur');
fixture.detectChanges();
flush();

expect(fixture.componentInstance.droppedSpy).toHaveBeenCalledTimes(1);
}));

});

describe('in a connected drop container', () => {
Expand Down
12 changes: 12 additions & 0 deletions src/cdk/drag-drop/drag-drop-registry.spec.ts
Expand Up @@ -244,6 +244,18 @@ describe('DragDropRegistry', () => {
subscription.unsubscribe();
});

it('should dispatch an event if the window is blurred while scrolling', () => {
const spy = jasmine.createSpy('blur spy');
const subscription = registry.pageBlurred.subscribe(spy);
const item = new DragItem();

registry.startDragging(item, createMouseEvent('mousedown'));
dispatchFakeEvent(window, 'blur');

expect(spy).toHaveBeenCalled();
subscription.unsubscribe();
});

class DragItem {
isDragging() { return this.shouldBeDragging; }
constructor(public shouldBeDragging = false) {
Expand Down
38 changes: 30 additions & 8 deletions src/cdk/drag-drop/drag-drop-registry.ts
Expand Up @@ -28,6 +28,7 @@ const activeCapturingEventOptions = normalizePassiveListenerOptions({
@Injectable({providedIn: 'root'})
export class DragDropRegistry<I extends {isDragging(): boolean}, C> implements OnDestroy {
private _document: Document;
private _window: Window | null;

/** Registered drop container instances. */
private _dropInstances = new Set<C>();
Expand All @@ -41,6 +42,10 @@ export class DragDropRegistry<I extends {isDragging(): boolean}, C> implements O
/** Keeps track of the event listeners that we've bound to the `document`. */
private _globalListeners = new Map<string, {
handler: (event: Event) => void,
// The target needs to be `| null` because we bind either to `window` or `document` which
// aren't available during SSR. There's an injection token for the document, but not one for
// window so we fall back to not binding events to it.
target: EventTarget | null,
options?: AddEventListenerOptions | boolean
}>();

Expand All @@ -54,13 +59,13 @@ export class DragDropRegistry<I extends {isDragging(): boolean}, C> implements O
* Emits the `touchmove` or `mousemove` events that are dispatched
* while the user is dragging a drag item instance.
*/
readonly pointerMove: Subject<TouchEvent | MouseEvent> = new Subject<TouchEvent | MouseEvent>();
pointerMove: Subject<TouchEvent | MouseEvent> = new Subject<TouchEvent | MouseEvent>();

/**
* Emits the `touchend` or `mouseup` events that are dispatched
* while the user is dragging a drag item instance.
*/
readonly pointerUp: Subject<TouchEvent | MouseEvent> = new Subject<TouchEvent | MouseEvent>();
pointerUp: Subject<TouchEvent | MouseEvent> = new Subject<TouchEvent | MouseEvent>();

/**
* Emits when the viewport has been scrolled while the user is dragging an item.
Expand All @@ -69,10 +74,14 @@ export class DragDropRegistry<I extends {isDragging(): boolean}, C> implements O
*/
readonly scroll: Subject<Event> = new Subject<Event>();

/** Emits when the page has been blurred while the user is dragging an item. */
pageBlurred: Subject<void> = new Subject<void>();

constructor(
private _ngZone: NgZone,
@Inject(DOCUMENT) _document: any) {
this._document = _document;
this._window = (typeof window !== 'undefined' && window.addEventListener) ? window : null;
}

/** Adds a drop container to the registry. */
Expand Down Expand Up @@ -137,35 +146,45 @@ export class DragDropRegistry<I extends {isDragging(): boolean}, C> implements O
this._globalListeners
.set(isTouchEvent ? 'touchend' : 'mouseup', {
handler: (e: Event) => this.pointerUp.next(e as TouchEvent | MouseEvent),
options: true
options: true,
target: this._document
})
.set('scroll', {
handler: (e: Event) => this.scroll.next(e),
// Use capturing so that we pick up scroll changes in any scrollable nodes that aren't
// the document. See https://github.com/angular/components/issues/17144.
options: true
options: true,
target: this._document
})
// Preventing the default action on `mousemove` isn't enough to disable text selection
// on Safari so we need to prevent the selection event as well. Alternatively this can
// be done by setting `user-select: none` on the `body`, however it has causes a style
// recalculation which can be expensive on pages with a lot of elements.
.set('selectstart', {
handler: this._preventDefaultWhileDragging,
options: activeCapturingEventOptions
options: activeCapturingEventOptions,
target: this._document
})
.set('blur', {
handler: () => this.pageBlurred.next(),
target: this._window // Note that this event can only be bound on the window, not document
});

// We don't have to bind a move event for touch drag sequences, because
// we already have a persistent global one bound from `registerDragItem`.
if (!isTouchEvent) {
this._globalListeners.set('mousemove', {
handler: (e: Event) => this.pointerMove.next(e as MouseEvent),
options: activeCapturingEventOptions
options: activeCapturingEventOptions,
target: this._document
});
}

this._ngZone.runOutsideAngular(() => {
this._globalListeners.forEach((config, name) => {
this._document.addEventListener(name, config.handler, config.options);
if (config.target) {
config.target.addEventListener(name, config.handler, config.options);
}
});
});
}
Expand Down Expand Up @@ -230,6 +249,7 @@ export class DragDropRegistry<I extends {isDragging(): boolean}, C> implements O
this._clearGlobalListeners();
this.pointerMove.complete();
this.pointerUp.complete();
this.pageBlurred.complete();
}

/**
Expand Down Expand Up @@ -259,7 +279,9 @@ export class DragDropRegistry<I extends {isDragging(): boolean}, C> implements O
/** Clears out the global event listeners from the `document`. */
private _clearGlobalListeners() {
this._globalListeners.forEach((config, name) => {
this._document.removeEventListener(name, config.handler, config.options);
if (config.target) {
config.target.removeEventListener(name, config.handler, config.options);
}
});

this._globalListeners.clear();
Expand Down
20 changes: 19 additions & 1 deletion src/cdk/drag-drop/drag-ref.ts
Expand Up @@ -212,13 +212,19 @@ export class DragRef<T = any> {
/** Subscription to the viewport being resized. */
private _resizeSubscription = Subscription.EMPTY;

/** Subscription to the page being blurred. */
private _blurSubscription = Subscription.EMPTY;

/**
* Time at which the last touch event occurred. Used to avoid firing the same
* events multiple times on touch devices where the browser will fire a fake
* mouse event for each touch event, after a certain time.
*/
private _lastTouchEventTime: number;

/** Last pointer move event that was captured. */
private _lastPointerMove: MouseEvent | TouchEvent | null;

/** Time at which the last dragging sequence was started. */
private _dragStartTime: number;

Expand Down Expand Up @@ -493,7 +499,7 @@ export class DragRef<T = any> {
this._resizeSubscription.unsubscribe();
this._parentPositions.clear();
this._boundaryElement = this._rootElement = this._ownerSVGElement = this._placeholderTemplate =
this._previewTemplate = this._anchor = this._parentDragRef = null!;
this._previewTemplate = this._anchor = this._parentDragRef = this._lastPointerMove = null!;
}

/** Checks whether the element is currently being dragged. */
Expand Down Expand Up @@ -588,6 +594,7 @@ export class DragRef<T = any> {
this._pointerMoveSubscription.unsubscribe();
this._pointerUpSubscription.unsubscribe();
this._scrollSubscription.unsubscribe();
this._blurSubscription.unsubscribe();
}

/** Destroys the preview element and its ViewRef. */
Expand Down Expand Up @@ -689,6 +696,7 @@ export class DragRef<T = any> {
const constrainedPointerPosition = this._getConstrainedPointerPosition(pointerPosition);
this._hasMoved = true;
this._lastKnownPointerPosition = pointerPosition;
this._lastPointerMove = event;
this._updatePointerDirectionDelta(constrainedPointerPosition);

if (this._dropContainer) {
Expand Down Expand Up @@ -879,6 +887,7 @@ export class DragRef<T = any> {
}

this._hasStartedDragging = this._hasMoved = false;
this._lastPointerMove = null;

// Avoid multiple subscriptions and memory leaks when multi touch
// (isDragging check above isn't enough because of possible temporal and/or dimensional delays)
Expand All @@ -889,6 +898,15 @@ export class DragRef<T = any> {
.scrolled(this._getShadowRoot())
.subscribe(scrollEvent => this._updateOnScroll(scrollEvent));

// If the page is blurred while dragging (e.g. there was an `alert` or the browser window was
// minimized) we won't get a mouseup/touchend so we need to use a different event to stop the
// drag sequence. Use the last known location to figure out where the element should be dropped.
this._blurSubscription = this._dragDropRegistry.pageBlurred.subscribe(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought that this would be treated as cancelling the drag sequence instead of completing it, since the dragged item might be somewhere the user doesn't want to drop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that, but it won't be very smooth, because the list could've been shuffled already and we'd have to snap it back into place. Also it would be inconsistent, because we'd have to avoid emitting the dropped event, even though we've emitted the start, move etc. events.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do users currently have a way to cancel a drag? E.g., is I want to let people press escape to abort?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They don't. I've had it as a low-priority task on my list to add an API for it, but I haven't had the time for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would want to collect more data on this before committing to the behavior (drop vs. abort)- maybe trying it out with a few people and seeing what they expect to happen

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have people that are willing to try it out? This is a bit of an edge case to begin with so I don't know how much time we want to spend on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just a twitter poll?

if (this._lastPointerMove) {
this._endDragSequence(this._lastPointerMove);
}
});

if (this._boundaryElement) {
this._boundaryRect = getMutableClientRect(this._boundaryElement);
}
Expand Down
7 changes: 4 additions & 3 deletions tools/public_api_guard/cdk/drag-drop.d.ts
Expand Up @@ -244,9 +244,10 @@ export declare class DragDropModule {
export declare class DragDropRegistry<I extends {
isDragging(): boolean;
}, C> implements OnDestroy {
readonly pointerMove: Subject<TouchEvent | MouseEvent>;
readonly pointerUp: Subject<TouchEvent | MouseEvent>;
readonly scroll: Subject<Event>;
pageBlurred: Subject<void>;
pointerMove: Subject<TouchEvent | MouseEvent>;
pointerUp: Subject<TouchEvent | MouseEvent>;
scroll: Subject<Event>;
constructor(_ngZone: NgZone, _document: any);
isDragging(drag: I): boolean;
ngOnDestroy(): void;
Expand Down