-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(cdk/drag-drop): support immediate drag with preview snapped to cursor on mousedown #30728
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -677,7 +677,7 @@ export class DragRef<T = any> { | |
}; | ||
|
||
/** Handler that is invoked when the user moves their pointer after they've initiated a drag. */ | ||
private _pointerMove = (event: MouseEvent | TouchEvent) => { | ||
private _pointerMove = (event: MouseEvent | TouchEvent, mouseDown: boolean = false) => { | ||
const pointerPosition = this._getPointerPositionOnPage(event); | ||
|
||
if (!this._hasStartedDragging()) { | ||
|
@@ -711,8 +711,9 @@ export class DragRef<T = any> { | |
this._ngZone.run(() => this._startDragSequence(event)); | ||
} | ||
} | ||
|
||
return; | ||
if (!mouseDown) { | ||
return; | ||
} | ||
} | ||
|
||
// We prevent the default action down here so that we know that dragging has started. This is | ||
|
@@ -977,6 +978,16 @@ export class DragRef<T = any> { | |
this._pointerPositionAtLastDirectionChange = {x: pointerPosition.x, y: pointerPosition.y}; | ||
this._dragStartTime = Date.now(); | ||
this._dragDropRegistry.startDragging(this, event); | ||
|
||
// when pixel threshold = 0 and dragStartDelay = 0 and a preview container/position exists we immediately drag | ||
if ( | ||
(event.type == 'mousedown' || event.type == 'touchstart') && | ||
previewTemplate && | ||
this._config.dragStartThreshold === 0 && | ||
this._getDragStartDelay(event) === 0 | ||
) { | ||
this._pointerMove(event, true); | ||
} | ||
} | ||
|
||
/** Cleans up the DOM artifacts that were added to facilitate the element being dragged. */ | ||
|
@@ -1086,6 +1097,9 @@ export class DragRef<T = any> { | |
|
||
if (this.constrainPosition) { | ||
this._applyPreviewTransform(x, y); | ||
} else if (this._previewTemplate?.snapToCursor) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if centering the element under the cursor is common enough that it should be a default behavior. Also couldn't you get the same result by offsetting it using a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @crisbeto -- this won't affect default behavior, it's fenced off to only occur when snapToCursor input is set on cdkDragPreview. Margin/etc. won't work because the transform is applied internally. As I wrote in the PR notes, the CDK dragdrop transform is fixed and automatically calculated transform on _pointerMove. So there are several issues with margin -- one is that it would have to be calculated in an attempt to override the transform by implementor using (cdkDragMoved) event which then defeats the ability to start preview on mousedown (since cdkDragMoved isn't fired until movement), and secondarily the shift in the container position would change location calculations that are used internally on CDK dragdrop for determining entered/exited events, lastly you cannot access the preview containers boundaries until it is rendered and it's all private members so even then requires a class DOM selector to get to it from handler in the first place. I explored a lot of different options to get around the limitations of the CDK Dragdrop (DOM access to the preview inside a template, controlling/recalc position of the preview dynamically externally that messed up the entered/exited, and the separate issue of having drag behavior correctly computed immediately on mousedown/touchstart). This PR elegantly handles those scenarios while maintaining existing behavior of the dragdrop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure the behavior is fenced off, but it's still a public API that we have to support and document. We also have to ensure that it works correctly with other APIs (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @crisbeto I'm not opposed to that & happy to do some more coding around that. I'm not sure what the additional use-case requirements would be though -- did you have a specific alternate scenarios in mind? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Off the top of my head, it could be a function that lets you determine the offset of the preview. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @crisbeto having the offset of the preview doesn't change anything b/c you'd then have to be trying to replace the transform: property again on every mouse move event. Not only would that be jumpy...it would also mean end users subscribing to mousemove which is expensive and warned against in the public API docs. Not sure if I'm missing what you're saying but would be happy to get on a Zoom call to go through it. LMK - just inbox me if so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @crisbeto since you were the original implementer of constrainPosition logic -- do you have specific knowledge about how this behavior might be meaningfully accomplished while providing the broadest possible flexibility? I reviewed again today and don't have a clear direction to investigate other possibilities. |
||
const previewRect = this._getPreviewRect(); | ||
this._applyPreviewTransform(rawX - previewRect.width / 2, rawY - previewRect.height / 2); | ||
} else { | ||
this._applyPreviewTransform( | ||
x - this._pickupPositionInElement.x, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow this. So the only behavior difference here is that the preview will show up on the
mousedown
rather than the nextmousemove
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this chessboard for example:
Chess.com Explorer
Click on any piece to move it and you will see the drag immediately starts. This is the effect of the snapToCursor combined with immediate preview rendering (on mousedown/touchstart).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in that case a better way to approach it would be to check that threshold/delay are at 0 and then call
_startDragSequence
from the "down" handler. It might be necessary to move some logic out of_pointerMove
so it can be reused.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crisbeto I had looked at that -- however the entirety of the
_pointerMove
function must be executed in this case especially around the constrainPosition and other logic executed in that method in addition to_startDragSequence
as a setup step being called.I had tried to limit it to just a few calls but that either breaks functionality or results in excess redundant code where this version cleanly was backwards-compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that we can move the whole block that's guarded by
!this._hasStartedDragging()
out into a separate function. The rest shouldn't be executed before dragging has started anyways.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisbeto unfortunately that doesn't work since the dragging must start when the mouse goes down...so the entirety of the pointerMove function is executed (note the return is skipped when it's a mouse down).