-
-
Notifications
You must be signed in to change notification settings - Fork 674
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: use fixed event listeners to address race conditions #1566
base: experimental
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@dnd-kit/dom': patch | ||
--- | ||
|
||
Use fixed event listeners in PointerSensor to address race conditions preventing `pointermove` and `pointerup` events from firing when document changes during a drag. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,8 @@ export class PointerSensor extends Sensor< | |
|
||
protected initialCoordinates: Coordinates | undefined; | ||
|
||
protected source: Draggable | undefined = undefined; | ||
|
||
#clearTimeout: CleanupFunction | undefined; | ||
|
||
constructor( | ||
|
@@ -67,6 +69,14 @@ export class PointerSensor extends Sensor< | |
this.handleCancel = this.handleCancel.bind(this); | ||
this.handlePointerUp = this.handlePointerUp.bind(this); | ||
this.handleKeyDown = this.handleKeyDown.bind(this); | ||
|
||
effect(() => { | ||
const unbindGlobal = this.bindGlobal(options ?? {}); | ||
|
||
return () => { | ||
unbindGlobal(); | ||
}; | ||
}); | ||
} | ||
|
||
public bind(source: Draggable, options = this.options) { | ||
|
@@ -92,6 +102,48 @@ export class PointerSensor extends Sensor< | |
return unbind; | ||
} | ||
|
||
protected bindGlobal(options: PointerSensorOptions) { | ||
const documents = new Set<Document>(); | ||
|
||
for (const draggable of this.manager.registry.draggables.value) { | ||
if (draggable.element) { | ||
documents.add(getDocument(draggable.element)); | ||
} | ||
} | ||
|
||
for (const droppable of this.manager.registry.droppables.value) { | ||
if (droppable.element) { | ||
documents.add(getDocument(droppable.element)); | ||
} | ||
} | ||
|
||
const unbindFns = Array.from(documents).map((doc) => | ||
this.listeners.bind(doc, [ | ||
{ | ||
type: 'pointermove', | ||
listener: (event: PointerEvent) => | ||
this.handlePointerMove(event, doc, options), | ||
}, | ||
{ | ||
type: 'pointerup', | ||
listener: this.handlePointerUp, | ||
options: { | ||
capture: true, | ||
}, | ||
}, | ||
{ | ||
// Cancel activation if there is a competing Drag and Drop interaction | ||
type: 'dragstart', | ||
listener: this.handleDragStart, | ||
}, | ||
]) | ||
); | ||
|
||
return () => { | ||
unbindFns.forEach((unbind) => unbind()); | ||
}; | ||
} | ||
|
||
protected handlePointerDown( | ||
event: PointerEvent, | ||
source: Draggable, | ||
|
@@ -106,11 +158,6 @@ export class PointerSensor extends Sensor< | |
) { | ||
return; | ||
} | ||
const {target} = event; | ||
const isNativeDraggable = | ||
isHTMLElement(target) && | ||
target.draggable && | ||
target.getAttribute('draggable') === 'true'; | ||
|
||
const offset = getFrameTransform(source.element); | ||
|
||
|
@@ -119,6 +166,8 @@ export class PointerSensor extends Sensor< | |
y: event.clientY * offset.scaleY + offset.y, | ||
}; | ||
|
||
this.source = source; | ||
|
||
const {activationConstraints} = options; | ||
const constraints = | ||
typeof activationConstraints === 'function' | ||
|
@@ -145,48 +194,38 @@ export class PointerSensor extends Sensor< | |
} | ||
} | ||
|
||
const ownerDocument = getDocument(event.target); | ||
|
||
const unbindListeners = this.listeners.bind(ownerDocument, [ | ||
{ | ||
type: 'pointermove', | ||
listener: (event: PointerEvent) => | ||
this.handlePointerMove(event, source, options), | ||
}, | ||
{ | ||
type: 'pointerup', | ||
listener: this.handlePointerUp, | ||
options: { | ||
capture: true, | ||
}, | ||
}, | ||
{ | ||
// Cancel activation if there is a competing Drag and Drop interaction | ||
type: 'dragstart', | ||
listener: isNativeDraggable ? this.handleCancel : preventDefault, | ||
}, | ||
]); | ||
|
||
const cleanup = () => { | ||
setTimeout(unbindListeners); | ||
this.#clearTimeout?.(); | ||
this.initialCoordinates = undefined; | ||
this.source = undefined; | ||
}; | ||
|
||
this.cleanup.add(cleanup); | ||
} | ||
|
||
protected handlePointerMove( | ||
event: PointerEvent, | ||
source: Draggable, | ||
doc: Document, | ||
options: PointerSensorOptions | ||
) { | ||
if (!this.source) { | ||
return; | ||
} | ||
|
||
const ownerDocument = | ||
this.source.element && getDocument(this.source.element); | ||
|
||
// Event may have duplicated between documents if user is bubbling events | ||
if (doc !== ownerDocument) { | ||
return; | ||
} | ||
Comment on lines
+218
to
+221
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. Note, new safe-guard to account for an edge-case where the user bubbles the I can address this on my end, but a safe-guard seemed sensible. |
||
|
||
const coordinates = { | ||
x: event.clientX, | ||
y: event.clientY, | ||
}; | ||
|
||
const offset = getFrameTransform(source.element); | ||
const offset = getFrameTransform(this.source.element); | ||
|
||
coordinates.x = coordinates.x * offset.scaleX + offset.x; | ||
coordinates.y = coordinates.y * offset.scaleY + offset.y; | ||
|
@@ -210,7 +249,7 @@ export class PointerSensor extends Sensor< | |
const {activationConstraints} = options; | ||
const constraints = | ||
typeof activationConstraints === 'function' | ||
? activationConstraints(event, source) | ||
? activationConstraints(event, this.source) | ||
: activationConstraints; | ||
const {distance, delay} = constraints ?? {}; | ||
|
||
|
@@ -222,7 +261,7 @@ export class PointerSensor extends Sensor< | |
return this.handleCancel(); | ||
} | ||
if (exceedsDistance(delta, distance.value)) { | ||
return this.handleStart(source, event); | ||
return this.handleStart(this.source, event); | ||
} | ||
} | ||
|
||
|
@@ -234,6 +273,10 @@ export class PointerSensor extends Sensor< | |
} | ||
|
||
private handlePointerUp(event: PointerEvent) { | ||
if (!this.source) { | ||
return; | ||
} | ||
Comment on lines
+276
to
+278
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. Now we're using global event listeners, we need to check if the user is dragging before stopping propagation (see measuredco/puck#769). |
||
|
||
// Prevent the default behaviour of the event | ||
event.preventDefault(); | ||
event.stopPropagation(); | ||
|
@@ -304,6 +347,25 @@ export class PointerSensor extends Sensor< | |
this.cleanup.add(unbind); | ||
} | ||
|
||
protected handleDragStart(event: DragEvent) { | ||
const {target} = event; | ||
|
||
if (!isElement(target)) { | ||
return; | ||
} | ||
|
||
const isNativeDraggable = | ||
isHTMLElement(target) && | ||
target.draggable && | ||
target.getAttribute('draggable') === 'true'; | ||
|
||
if (isNativeDraggable) { | ||
this.handleCancel(); | ||
} else { | ||
preventDefault(event); | ||
} | ||
} | ||
|
||
protected handleCancel() { | ||
const {dragOperation} = this.manager; | ||
|
||
|
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.
Noting that this may be better elsewhere for perf reasons