Fix vertical splitter in variables pane #5391
Conversation
The vertical splitter component would re-render itself while being resized. The renders would cause the React component to be re-created and the move event listeners to be detached from the original element the resize was happening on. Event handling for the resizing operation is now done directly on the DOM elements and not as part of React state to preserve a link to the element that is not possible due to the nature of React component lifecycle.
| * VerticalSplitterResizeParams interface. This defines the parameters of a resize operation. When | ||
| * invert is true, the mouse delta is subtracted from the starting width instead of being added to | ||
| * it, which inverts the resize operation. | ||
| * VerticalSplitterResizeParams interface. This defines the parameters of a resize operation. |
There was a problem hiding this comment.
Updating the comment here since it looked to be incorrect.
| /** | ||
| * ResizeParams interface. Represents a resize that's in progress. | ||
| */ | ||
| interface ResizeParams { |
There was a problem hiding this comment.
We no longer require this interface because the react state variable for the resize params is no longer needed for the new approach to handle resizing.
| onPointerDown={sashPointerDownHandler} | ||
| onPointerMove={sashPointerEventHandler} | ||
| onLostPointerCapture={sashPointerEventHandler} | ||
| onPointerDown={pointerDownHandler} |
There was a problem hiding this comment.
Using the same approach for resizing that we have in <HorizontalSplitter>. The pointermove and lostpointercapture events are no longer being managed via React. The event listeners for these are added directly to the element in the DOM.
| * Sash onPointerDown handler. | ||
| * @param e A PointerEvent that describes a user interaction with the pointer. | ||
| */ | ||
| const sashPointerDownHandler = (e: PointerEvent<HTMLDivElement>) => { |
There was a problem hiding this comment.
This function has been replaced by pointerDownHandler - most of the logic is the same and just been moved over. I may end up renaming pointerDownHandler to sashPointerDownHandler if that's the preference.
There was a problem hiding this comment.
Let's rename pointerDownHandler to sashPointerDownHandler for consistency.
| target.setPointerCapture(e.pointerId); | ||
| target.addEventListener('pointermove', pointerMoveHandler); | ||
| target.addEventListener('lostpointercapture', lostPointerCaptureHandler); |
There was a problem hiding this comment.
This is the secret sauce that fixed the bug. This event listener has been moved to the window body. When the event listener was on the component, a re-render of the component meant that we stopped listening to the original event that was happening.
There was a problem hiding this comment.
If I'm understanding correctly, this relies on the lostpointercapture event to fire. I guess there may be some convoluted situation where that doesn't happen like using a keyboard shortcut to toggle the sidebar while actively dragging the sash. Testing this though the callback for lost pointer will still eventually fire and clean stuff up. So I think it's fine!
There was a problem hiding this comment.
Oh that's a great edge case that you caught! I hadn't tested that so its good to know it works 😅!
| * Sash onPointerDown handler. | ||
| * @param e A PointerEvent that describes a user interaction with the pointer. | ||
| */ | ||
| const sashPointerDownHandler = (e: PointerEvent<HTMLDivElement>) => { |
There was a problem hiding this comment.
Let's rename pointerDownHandler to sashPointerDownHandler for consistency.
There was a problem hiding this comment.
This is great!
I poked at it and it works as advertised. The only large-scale stylistic thing I might suggest is moving the hook-related logic to a custom hook ala useVerticalSplitter() or something. But there's enough variables that would need to be returned it's not that much cleaner so take-it-or-leave-it.
Great work!
src/vs/base/browser/ui/positronComponents/splitters/verticalSplitter.tsx
Outdated
Show resolved
Hide resolved
| // Setup the resize state. | ||
| const resizeParams = onBeginResize(); | ||
| const startingWidth = collapsed ? sashWidth : resizeParams.columnsWidth; | ||
| const target = DOM.getWindow(e.currentTarget).document.body; |
There was a problem hiding this comment.
My first inclination is that the whole window body is too broad, but you're adding the events in a way that they don't clobber anything else listening... so this seems good to me!
There was a problem hiding this comment.
Hah, great point! 😄 I had the same exact inclination initially as well and I chatted about this exact thing with @softwarenerd. My understanding is that we need to know when a user does a drag and a release that ends outside of the application window. I'm a little new to handling drag events and followed the existing pattern we have in <HorizontalSplitter>.
| target.setPointerCapture(e.pointerId); | ||
| target.addEventListener('pointermove', pointerMoveHandler); | ||
| target.addEventListener('lostpointercapture', lostPointerCaptureHandler); |
There was a problem hiding this comment.
If I'm understanding correctly, this relies on the lostpointercapture event to fire. I guess there may be some convoluted situation where that doesn't happen like using a keyboard shortcut to toggle the sidebar while actively dragging the sash. Testing this though the callback for lost pointer will still eventually fire and clean stuff up. So I think it's fine!
Description
Addresses #4720
Fixes a bug where users were unable to smoothly resize the columns in the variables pane. This PR changes how the vertical splitter handles the events used to resize columns. The vertical splitter component would re-render itself upon a resize which would cause relevant state information to be reset. We now directly interact with the DOM element to avoid this issue.
QA Notes
Screenshots
Screen.Recording.2024-11-15.at.7.18.31.PM.mov