-
-
Notifications
You must be signed in to change notification settings - Fork 122
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
BugFix: Global cursor style reset on unmount #313
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
return () => { | ||
resetGlobalCursorStyle(); | ||
}; | ||
}, []); |
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 this is an over-simplification. The cursor should only be updated/reset if one or more of the resize handles the user is currently interacting with get unmounted. That's something the PanelResizeHandleRegistry
has to manage.
Handles register and unregister themselves with that registry on mount:
react-resizable-panels/packages/react-resizable-panels/src/PanelResizeHandle.ts
Lines 166 to 187 in 44d3a65
return registerResizeHandle( | |
resizeHandleId, | |
element, | |
direction, | |
{ | |
// Coarse inputs (e.g. finger/touch) | |
coarse: hitAreaMargins?.coarse ?? 15, | |
// Fine inputs (e.g. mouse) | |
fine: hitAreaMargins?.fine ?? 5, | |
}, | |
setResizeHandlerState | |
); | |
}, [ | |
direction, | |
disabled, | |
hitAreaMargins, | |
registerResizeHandleWithParentGroup, | |
resizeHandleId, | |
resizeHandler, | |
startDragging, | |
stopDragging, | |
]); |
I think the fix here needs to be in the unregister function:
react-resizable-panels/packages/react-resizable-panels/src/PanelResizeHandleRegistry.ts
Lines 64 to 76 in 44d3a65
return function unregisterResizeHandle() { | |
panelConstraintFlags.delete(resizeHandleId); | |
registeredResizeHandlers.delete(data); | |
const count = ownerDocumentCounts.get(ownerDocument) ?? 1; | |
ownerDocumentCounts.set(ownerDocument, count - 1); | |
updateListeners(); | |
if (count === 1) { | |
ownerDocumentCounts.delete(ownerDocument); | |
} | |
}; |
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 the right fix might be- on unregister- if there are any current intersecting drag handles, re-compute them and then update the cursor again (using the same methods already defined in that module)
Just an update on my end - work on this has been deprioritised as the team for the other frontend have found a workaround for the issue they were experiencing, and I've had higher priority stuff come through. I'd still like to continue on this when I get a chance, though. |
Thanks for the update! |
@@ -72,6 +82,24 @@ describe("PanelResizeHandle", () => { | |||
expect(element.title).toBe("bar"); | |||
}); | |||
|
|||
it("resets the global cursor style on unmount", () => { |
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 retained this test from my original attempt at fixing this bug. I've dropped the test "resets the global cursor style on disabled", as this is not the behaviour we want.
}; | ||
} | ||
|
||
function recalculateIntersectingHandlesAfterUnregister( |
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.
These changes were written by ChatGPT after I explained the problem to it and fed it the source code for the relevant files. It seems to work as expected; I've not yet observed the issue re-appear in the project I have linked against the forked version of this library. It seems to match your recommended approach:
I think the right fix might be- on unregister- if there are any current intersecting drag handles, re-compute them and then update the cursor again (using the same methods already defined in that module)
I asked ChatGPT (and had a look myself) whether it would be possible to call the existing recalculateIntersectingHandles
function within the unregister callback, but it seems this would require more refactoring and potentially be more brittle than the solution ChatGPT proposes that involves writing these two new functions. We can discuss whether there's a way to simplify this further and re-use more of the existing code, though.
Fixes #308.
This PR updates the
PanelResizeHandle
component to callresetGlobalCursorStyle
when unmounted. This should prevent the cursor from indefinitely retaining the "resize" style if the component is unmounted as a panel is being resized.I've tested out these changes locally and they appear to resolve the issue.