Skip to content

Make long scrolled notebook outputs resizable#13093

Merged
seeM merged 20 commits intomainfrom
feat/output-resize-handle-v2
Apr 23, 2026
Merged

Make long scrolled notebook outputs resizable#13093
seeM merged 20 commits intomainfrom
feat/output-resize-handle-v2

Conversation

@seeM
Copy link
Copy Markdown
Contributor

@seeM seeM commented Apr 20, 2026

This PR makes long scrolled notebook outputs resizable. Addresses #12600.

Also added some missing functionality from the horizontal splitter and tidied up both splitters a bit:

  • HorizontalSplitter now has a hover state with delayed enablement matching the vertical splitter and VS Code Sash and a double click handler prop e.g. to reset to the original size
  • Fixed a bug in both splitters where unhovering was delayed
  • Fixed both splitters to use --vscode-sash-hoverBorder instead of --vscode-focusBorder (although the former defaults to the latter so it probably has little reach)
  • Removed hoverDelay state, changed hoverDelayer to a ref, and directly update hoverDelayer.defaultDelay - we didn't need to rerender when hover delay changes

Release Notes

New Features

Bug Fixes

  • N/A

QA Notes

@:positron-notebooks @:web

There are e2e tests for this PR.

@seeM seeM requested review from dhruvisompura and nstrayer April 20, 2026 16:57
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 20, 2026

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:positron-notebooks @:web

readme  valid tags


// Calculate the delta.
const delta = Math.trunc(e.clientX - clientX);
const delta = e.clientX - clientX;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dhruvisompura can you please double-check if this is ok? From my reading it should be fine as a float, but wasn't sure if I was missing something perhaps performance related. Note when testing performance in this branch that you have to compare with a main dev build - release builds are noticeably faster for some reason.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure Math.trunc was very intentional so that we don't set the width of things to 123.23432px.

Copy link
Copy Markdown
Contributor

@dhruvisompura dhruvisompura Apr 22, 2026

Choose a reason for hiding this comment

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

IIRC, we ran into an issue with sub pixel rendering previously in the console pane(?) and it caused things to look blurry. Not sure if this is related to that issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@softwarenerd do you recall what the problem was that we ran into that required us to truncate the pixel value?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because e.clientX is a CSS-pixel value that can be fractional on high-DPI / fractional-scaling displays (Retina, Windows 125%/150% scaling, trackpad subpixel motion). The delta then feeds into newWidth at verticalSplitter.tsx:310, which is applied as an element width — non-integer widths cause subpixel rendering jitter (the panel edge wobbling mid-drag).

Why Math.trunc specifically (vs Math.floor / Math.round):

  • Math.trunc is symmetric around zero: trunc(1.9) = 1, trunc(-1.9) = -1. Drags left and right of equal magnitude produce equal-magnitude deltas.
  • Math.floor would bias: floor(-1.9) = -2. A right drag would move 1px per unit of motion, a left drag 2px — visible asymmetry during slow drags.
  • Math.round works but rounds at .5, which can produce inconsistent step sizes across consecutive mousemove events (sometimes rounding up, sometimes down).

So: Math.trunc gives clean, symmetric, integer deltas regardless of display scaling.

nstrayer
nstrayer previously approved these changes Apr 21, 2026
Copy link
Copy Markdown
Contributor

@nstrayer nstrayer left a comment

Choose a reason for hiding this comment

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

I like it. Is a nice QOL improvement.
I made a few comments on testing and keyboard-nav stuff.

Additionally I noticed some somewhat funky behavior when you shrink the output almost all the way. Mostly that the drag works as you would expect until a somewhat arbitrary point near fully collapsed where the shrinking stops happening from the bottom and starts happening from the top (aka the shifts in layout start happening above the cell rather than below. I captured it in a video below. The point at which this happens seems arbitrary and this video is one of the smallest versions of it. I think it's okay for now as 1. people probably wont shrink these outputs that far most of the time and 2. the output is still shrinking, just not in exactly the expected way.

Screen.Recording.2026-04-21.at.2.21.25.PM.mov

Comment thread test/e2e/pages/notebooksPositron.ts Outdated
Comment thread test/e2e/pages/notebooksPositron.ts Outdated
Comment thread src/vs/base/browser/ui/positronComponents/splitters/horizontalSplitter.tsx Outdated
Comment thread src/vs/base/browser/ui/positronComponents/splitters/horizontalSplitter.tsx Outdated
Co-authored-by: Nick Strayer <nick.strayer@posit.co>
Signed-off-by: Wasim Lorgat <mwlorgat@gmail.com>
softwarenerd
softwarenerd previously approved these changes Apr 22, 2026
Copy link
Copy Markdown
Contributor

@softwarenerd softwarenerd left a comment

Choose a reason for hiding this comment

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

I approve with a couple of notes. Has this been well-tested in Data Explorer?


// Calculate the delta.
const delta = Math.trunc(e.clientX - clientX);
const delta = e.clientX - clientX;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure Math.trunc was very intentional so that we don't set the width of things to 123.23432px.

const calculateNewHeight = (e: PointerEvent) => {
// Calculate the delta.
const delta = Math.trunc(e.clientY - clientY);
const delta = e.clientY - clientY;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure Math.trunc was very intentional so that we don't set the width of things to 123.23432px.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted in 5ef2371, but I am curious why integer width is important. It's not uncommon for elements to have non-integer widths. Is this requirement specific to the data explorer or are there other benefits?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm having a hard time recalling precisely what this fixed, but, I coded it for a reason. I think because of some subpixel rendering jitter / artifacts. I guess I'd turn this question around and ask why you felt that it should be removed? Did you see some benefit to doing so?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question! I wanted to understand if/why it was needed and perhaps learn something new about browsers in the process. I couldn't figure it out myself so thought I'd start a discussion to hear from others. We still haven't figured it out, but given that it's not an important change and not required by the feature I was working on, I'm happy to leave it as is.

Copy link
Copy Markdown
Contributor

@dhruvisompura dhruvisompura left a comment

Choose a reason for hiding this comment

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

Just tested the changes and it looks like I can drag the output so its longer than the output itself. Is that intentional? I would expect it to cap out at the height of the contents.

Screen.Recording.2026-04-22.at.12.07.10.PM.mov

Comment on lines 74 to +93
@@ -90,7 +90,7 @@ const getHoverDelay = (configurationService: IConfigurationService) =>
* @param element The element.
* @returns true, if the point is inside the specified element; otherwise, false.
*/
const isPointInsideElement = (x: number, y: number, element?: HTMLElement) => {
export const isPointInsideElement = (x: number, y: number, element?: HTMLElement) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if these should be in a helper/util file that both splitters can import since the horizontal splitter is using them now too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but this causes eslint layering errors because IConfigurationService is in platform which would require a bigger refactor that I didn't want to take on in this PR.


// Calculate the delta.
const delta = Math.trunc(e.clientX - clientX);
const delta = e.clientX - clientX;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@softwarenerd do you recall what the problem was that we ran into that required us to truncate the pixel value?

@seeM
Copy link
Copy Markdown
Contributor Author

seeM commented Apr 23, 2026

@dhruvisompura nice catch! Fixed in b056b8a

Screen.Recording.2026-04-23.at.17.33.11.mov

@seeM seeM merged commit d3b1914 into main Apr 23, 2026
19 of 20 checks passed
@seeM seeM deleted the feat/output-resize-handle-v2 branch April 23, 2026 15:36
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants