Skip to content

Commit

Permalink
fix(cdk/drag-drop): constrainPosition not working as expected (#25061)
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 #25046.

(cherry picked from commit 2c956c0)
  • Loading branch information
crisbeto committed Jun 12, 2022
1 parent 267d690 commit 59268fa
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 28 deletions.
7 changes: 6 additions & 1 deletion src/cdk/drag-drop/directives/drag.spec.ts
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
10 changes: 7 additions & 3 deletions src/cdk/drag-drop/directives/drag.ts
Expand Up @@ -132,10 +132,14 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
/**
* Function that can be used to customize the logic of how the position of the drag item
* is limited while it's being dragged. Gets called with a point containing the current position
* of the user's pointer on the page and should return a point describing where the item should
* be rendered.
* of the user's pointer on the page, a reference to the item being dragged and its dimenstions.
* 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
65 changes: 43 additions & 22 deletions src/cdk/drag-drop/drag-ref.ts
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 @@ -355,10 +358,14 @@ export class DragRef<T = any> {
/**
* Function that can be used to customize the logic of how the position of the drag item
* is limited while it's being dragged. Gets called with a point containing the current position
* of the user's pointer on the page and should return a point describing where the item should
* be rendered.
* of the user's pointer on the page, a reference to the item being dragged and its dimenstions.
* 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
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 59268fa

Please sign in to comment.