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.

Sign up for GitHub

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

Jump to bottom

fix: "Invariant Violation: Expected targetIds to be registered." #3409

Merged
merged 3 commits into from Mar 29, 2022
Merged

fix: "Invariant Violation: Expected targetIds to be registered." #3409

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99db7b8
fix: "Invariant Violation: Expected targetIds to be registered."
joshkel Mar 22, 2022
f8ba3ee
chore: semver
darthtrevino Mar 28, 2022
c901c10
fix: cancel any raf before creating a new one
darthtrevino Mar 29, 2022
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
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
View file
Open in desktop
@@ -1,6 +1,6 @@
# Contributing to React-DnD

So you want to contibute to React-DnD? Thank you! This library is a community effort, and your contributions are greatly appreciated.
So you want to contribute to React-DnD? Thank you! This library is a community effort, and your contributions are greatly appreciated.

## FAQ

Expand Down
47 changes: 33 additions & 14 deletions packages/backend-html5/src/HTML5BackendImpl.ts
View file
Open in desktop
Expand Up @@ -347,6 +347,7 @@ export class HTML5BackendImpl implements Backend {
if (this.clearCurrentDragSourceNode() && this.monitor.isDragging()) {
this.actions.endDrag()
}
this.cancelHover()
}

private setCurrentDragSourceNode(node: Element | null) {
Expand Down Expand Up @@ -400,6 +401,33 @@ export class HTML5BackendImpl implements Backend {
return false
}

private scheduleHover = (dragOverTargetIds: string[] | null) => {
if (
this.hoverRafId === null &&
typeof requestAnimationFrame !== 'undefined'
) {
this.hoverRafId = requestAnimationFrame(() => {
if (this.monitor.isDragging()) {
this.actions.hover(dragOverTargetIds || [], {
Copy link

@vsiao vsiao Mar 28, 2022 •
edited

Choose a reason for hiding this comment

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

Thanks for trying to fix this bug! I just commented on https://github.com/react-dnd/react-dnd/pull/3313/files#r836932854 and I think it's still relevant here so I'm cross-linking it. From what I can tell, it still seems very possible to encounter bad race conditions, even with the changes that you've made.

Did you try always calling cancelHover before scheduling to ensure that we're at least capturing the most recent copy of dragOverTargetIds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback, @vsiao. So, if I'm understanding correctly:

In the original code, if the user hovers over element A, then quickly hovers over element B before the requestAnimationFrame for A can fire, then the hover will be out of date.

After my changes, I think this won't throw errors, because I think the code is structured to cancel any hover effects before anything that would invalidate a target ID. (Actually, it may be possible for an externally triggered rerender to invalidate a target ID. That may need more investigation.) But it's still a visual bug.

To address that, you're saying we should also cancel the hover effect before triggering any hover effect?

That seems correct to me (and so @darthtrevino's fix should go outside of the if, like you said).

clientOffset: this.lastClientOffset,
})
}

this.hoverRafId = null
})
}
}

private cancelHover = () => {
if (
this.hoverRafId !== null &&
typeof cancelAnimationFrame !== 'undefined'
) {
cancelAnimationFrame(this.hoverRafId)
this.hoverRafId = null
}
}

public handleTopDragStartCapture = (): void => {
this.clearCurrentDragSourceNode()
this.dragStartSourceIds = []
Expand Down Expand Up @@ -429,6 +457,7 @@ export class HTML5BackendImpl implements Backend {
// Avoid crashing if we missed a drop event or our previous drag died
if (this.monitor.isDragging()) {
this.actions.endDrag()
this.cancelHover()
}

// Don't publish the source just yet (see why below)
Expand Down Expand Up @@ -530,6 +559,7 @@ export class HTML5BackendImpl implements Backend {
// Only proceed if we have not handled it already.
this.actions.endDrag()
}
this.cancelHover()
}

public handleTopDragEnterCapture = (e: DragEvent): void => {
Expand Down Expand Up @@ -622,20 +652,7 @@ export class HTML5BackendImpl implements Backend {
this.altKeyPressed = e.altKey
this.lastClientOffset = getEventClientOffset(e)

if (
this.hoverRafId === null &&
typeof requestAnimationFrame !== 'undefined'
) {
this.hoverRafId = requestAnimationFrame(() => {
if (this.monitor.isDragging()) {
this.actions.hover(dragOverTargetIds || [], {
clientOffset: this.lastClientOffset,
})
}

this.hoverRafId = null
})
}
this.scheduleHover(dragOverTargetIds)

const canDrop = (dragOverTargetIds || []).some((targetId) =>
this.monitor.canDropOnTarget(targetId),
Expand Down Expand Up @@ -672,6 +689,7 @@ export class HTML5BackendImpl implements Backend {
if (this.isDraggingNativeItem()) {
setTimeout(() => this.endDragNativeItem(), 0)
}
this.cancelHover()
}

public handleTopDropCapture = (e: DragEvent): void => {
Expand Down Expand Up @@ -709,6 +727,7 @@ export class HTML5BackendImpl implements Backend {
} else if (this.monitor.isDragging()) {
this.actions.endDrag()
}
this.cancelHover()
}

public handleSelectStart = (e: DragEvent): void => {
Expand Down