Skip to content

Commit

Permalink
fix(cdk/drag-drop): constrainPosition not working as expected
Browse files Browse the repository at this point in the history
Fixes that the `constrainPosition` input wasn't constraining the position as described in the docs. This may have worked at some point, but it's definitely broken now. Our tests didn't catch it, because they were looking at the `transform` which was wrong, instead of the position the element ended up at.

Fixes angular#25046.
  • Loading branch information
crisbeto committed Jun 11, 2022
1 parent d40da65 commit 6914fe3
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 24 deletions.
7 changes: 6 additions & 1 deletion src/cdk/drag-drop/directives/drag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1081,8 +1081,12 @@ describe('CdkDrag', () => {
expect(spy).toHaveBeenCalledWith(
jasmine.objectContaining({x: 300, y: 300}),
jasmine.any(DragRef),
jasmine.anything(),
);
expect(dragElement.style.transform).toBe('translate3d(50px, 50px, 0px)');

const elementRect = dragElement.getBoundingClientRect();
expect(Math.floor(elementRect.top)).toBe(50);
expect(Math.floor(elementRect.left)).toBe(50);
}));

it('should throw if drag item is attached to an ng-container', fakeAsync(() => {
Expand Down Expand Up @@ -3680,6 +3684,7 @@ describe('CdkDrag', () => {
expect(spy).toHaveBeenCalledWith(
jasmine.objectContaining({x: 200, y: 200}),
jasmine.any(DragRef),
jasmine.anything(),
);
expect(Math.floor(previewRect.top)).toBe(50);
expect(Math.floor(previewRect.left)).toBe(50);
Expand Down
6 changes: 5 additions & 1 deletion src/cdk/drag-drop/directives/drag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,11 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
* of the user's pointer on the page and should return a point describing where the item should
* be rendered.
*/
@Input('cdkDragConstrainPosition') constrainPosition?: (point: Point, dragRef: DragRef) => Point;
@Input('cdkDragConstrainPosition') constrainPosition?: (
userPointerPosition: Point,
dragRef: DragRef,
dimensions: ClientRect,
) => Point;

/** Class to be added to the preview element. */
@Input('cdkDragPreviewClass') previewClass: string | string[];
Expand Down
61 changes: 41 additions & 20 deletions src/cdk/drag-drop/drag-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ export class DragRef<T = any> {
/** Whether the native dragging interactions have been enabled on the root element. */
private _nativeInteractionsEnabled = true;

/** Client rect of the root element when the dragging sequence has started. */
private _initialClientRect?: ClientRect;

/** Cached dimensions of the preview element. Should be read via `_getPreviewRect`. */
private _previewRect?: ClientRect;

Expand Down Expand Up @@ -358,7 +361,11 @@ export class DragRef<T = any> {
* of the user's pointer on the page and should return a point describing where the item should
* be rendered.
*/
constrainPosition?: (point: Point, dragRef: DragRef) => Point;
constrainPosition?: (
userPointerPosition: Point,
dragRef: DragRef,
dimensions: ClientRect,
) => Point;

constructor(
element: ElementRef<HTMLElement> | HTMLElement,
Expand Down Expand Up @@ -695,12 +702,12 @@ export class DragRef<T = any> {
if (this._dropContainer) {
this._updateActiveDropContainer(constrainedPointerPosition, pointerPosition);
} else {
// If there's a position constraint function, we want the element's top/left to be at the
// specific position on the page. Use the initial position as a reference if that's the case.
const offset = this.constrainPosition ? this._initialClientRect! : this._pickupPositionOnPage;
const activeTransform = this._activeTransform;
activeTransform.x =
constrainedPointerPosition.x - this._pickupPositionOnPage.x + this._passiveTransform.x;
activeTransform.y =
constrainedPointerPosition.y - this._pickupPositionOnPage.y + this._passiveTransform.y;

activeTransform.x = constrainedPointerPosition.x - offset.x + this._passiveTransform.x;
activeTransform.y = constrainedPointerPosition.y - offset.y + this._passiveTransform.y;
this._applyRootElementTransform(activeTransform.x, activeTransform.y);
}

Expand Down Expand Up @@ -886,6 +893,7 @@ export class DragRef<T = any> {
// Avoid multiple subscriptions and memory leaks when multi touch
// (isDragging check above isn't enough because of possible temporal and/or dimensional delays)
this._removeSubscriptions();
this._initialClientRect = this._rootElement.getBoundingClientRect();
this._pointerMoveSubscription = this._dragDropRegistry.pointerMove.subscribe(this._pointerMove);
this._pointerUpSubscription = this._dragDropRegistry.pointerUp.subscribe(this._pointerUp);
this._scrollSubscription = this._dragDropRegistry
Expand All @@ -903,7 +911,7 @@ export class DragRef<T = any> {
this._pickupPositionInElement =
previewTemplate && previewTemplate.template && !previewTemplate.matchSize
? {x: 0, y: 0}
: this._getPointerPositionInElement(referenceElement, event);
: this._getPointerPositionInElement(this._initialClientRect, referenceElement, event);
const pointerPosition =
(this._pickupPositionOnPage =
this._lastKnownPointerPosition =
Expand All @@ -925,7 +933,11 @@ export class DragRef<T = any> {

this._destroyPreview();
this._destroyPlaceholder();
this._boundaryRect = this._previewRect = this._initialTransform = undefined;
this._initialClientRect =
this._boundaryRect =
this._previewRect =
this._initialTransform =
undefined;

// Re-enter the NgZone since we bound `document` events on the outside.
this._ngZone.run(() => {
Expand Down Expand Up @@ -1013,10 +1025,15 @@ export class DragRef<T = any> {
if (this.isDragging()) {
this._dropContainer!._startScrollingIfNecessary(rawX, rawY);
this._dropContainer!._sortItem(this, x, y, this._pointerDirectionDelta);
this._applyPreviewTransform(
x - this._pickupPositionInElement.x,
y - this._pickupPositionInElement.y,
);

if (this.constrainPosition) {
this._applyPreviewTransform(x, y);
} else {
this._applyPreviewTransform(
x - this._pickupPositionInElement.x,
y - this._pickupPositionInElement.y,
);
}
}
}

Expand All @@ -1033,7 +1050,7 @@ export class DragRef<T = any> {
if (previewTemplate && previewConfig) {
// Measure the element before we've inserted the preview
// since the insertion could throw off the measurement.
const rootRect = previewConfig.matchSize ? this._rootElement.getBoundingClientRect() : null;
const rootRect = previewConfig.matchSize ? this._initialClientRect : null;
const viewRef = previewConfig.viewContainer.createEmbeddedView(
previewTemplate,
previewConfig.context,
Expand All @@ -1050,9 +1067,8 @@ export class DragRef<T = any> {
);
}
} else {
const element = this._rootElement;
preview = deepCloneNode(element);
matchElementSize(preview, element.getBoundingClientRect());
preview = deepCloneNode(this._rootElement);
matchElementSize(preview, this._initialClientRect!);

if (this._initialTransform) {
preview.style.transform = this._initialTransform;
Expand Down Expand Up @@ -1170,10 +1186,10 @@ export class DragRef<T = any> {
* @param event Event that initiated the dragging.
*/
private _getPointerPositionInElement(
elementRect: ClientRect,
referenceElement: HTMLElement,
event: MouseEvent | TouchEvent,
): Point {
const elementRect = this._rootElement.getBoundingClientRect();
const handleElement = referenceElement === this._rootElement ? null : referenceElement;
const referenceRect = handleElement ? handleElement.getBoundingClientRect() : elementRect;
const point = isTouchEvent(event) ? event.targetTouches[0] : event;
Expand Down Expand Up @@ -1222,7 +1238,9 @@ export class DragRef<T = any> {
/** Gets the pointer position on the page, accounting for any position constraints. */
private _getConstrainedPointerPosition(point: Point): Point {
const dropContainerLock = this._dropContainer ? this._dropContainer.lockAxis : null;
let {x, y} = this.constrainPosition ? this.constrainPosition(point, this) : point;
let {x, y} = this.constrainPosition
? this.constrainPosition(point, this, this._initialClientRect!)
: point;

if (this.lockAxis === 'x' || dropContainerLock === 'x') {
y = this._pickupPositionOnPage.y;
Expand Down Expand Up @@ -1361,8 +1379,9 @@ export class DragRef<T = any> {
return;
}

const boundaryRect = this._boundaryElement.getBoundingClientRect();
// Note: don't use `_clientRectAtStart` here, because we want the latest position.
const elementRect = this._rootElement.getBoundingClientRect();
const boundaryRect = this._boundaryElement.getBoundingClientRect();

// It's possible that the element got hidden away after dragging (e.g. by switching to a
// different tab). Don't do anything in this case so we don't clear the user's position.
Expand Down Expand Up @@ -1511,7 +1530,9 @@ export class DragRef<T = any> {
// Cache the preview element rect if we haven't cached it already or if
// we cached it too early before the element dimensions were computed.
if (!this._previewRect || (!this._previewRect.width && !this._previewRect.height)) {
this._previewRect = (this._preview || this._rootElement).getBoundingClientRect();
this._previewRect = this._preview
? this._preview.getBoundingClientRect()
: this._initialClientRect!;
}

return this._previewRect;
Expand Down
4 changes: 2 additions & 2 deletions tools/public_api_guard/cdk/drag-drop.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
dropContainer: CdkDropListInternal,
_document: any, _ngZone: NgZone, _viewContainerRef: ViewContainerRef, config: DragDropConfig, _dir: Directionality, dragDrop: DragDrop, _changeDetectorRef: ChangeDetectorRef, _selfHandle?: CdkDragHandle | undefined, _parentDrag?: CdkDrag<any> | undefined);
boundaryElement: string | ElementRef<HTMLElement> | HTMLElement;
constrainPosition?: (point: Point, dragRef: DragRef) => Point;
constrainPosition?: (userPointerPosition: Point, dragRef: DragRef, dimensions: ClientRect) => Point;
data: T;
get disabled(): boolean;
set disabled(value: BooleanInput);
Expand Down Expand Up @@ -359,7 +359,7 @@ export class DragDropRegistry<I extends {
export class DragRef<T = any> {
constructor(element: ElementRef<HTMLElement> | HTMLElement, _config: DragRefConfig, _document: Document, _ngZone: NgZone, _viewportRuler: ViewportRuler, _dragDropRegistry: DragDropRegistry<DragRef, DropListRefInternal>);
readonly beforeStarted: Subject<void>;
constrainPosition?: (point: Point, dragRef: DragRef) => Point;
constrainPosition?: (userPointerPosition: Point, dragRef: DragRef, dimensions: ClientRect) => Point;
data: T;
get disabled(): boolean;
set disabled(value: boolean);
Expand Down

0 comments on commit 6914fe3

Please sign in to comment.