Hover: restore scrolling for long content

[ktsoator (ktsoator)]

Fixes #215972
Fixes #316535
Fixes #319825
Fixes #324038
Fixes #325545
Fixes #327517

Summary

Long workbench hover content, including Source Control Graph commit details and Terminal environment information, could exceed the hover height without making the content node scrollable. The outer hover clipped the overflow, which made metadata and action rows unreachable.

HoverWidget.layout() clears the content node's previous maxHeight before recalculating the hover dimensions. The layout previously restored a height constraint only on the outer hover container, so the DomScrollableElement content kept its full height and did not expose an internal scroll range.

This change:

• clears the outer hover's previous height constraint before measuring its natural layout;
• reserves space for borders and fixed status, action, and hint rows;
• constrains the scrollable content to the remaining available hover height;
• preserves the existing vertical scrollbar padding behavior once content overflows; and
• keeps short hover content non-scrollable.

The fix is implemented in the shared hover widget so all workbench hover consumers receive consistent overflow behavior.

Verification

• HoverService unit suite on current main with this change: 37 passing
• Pre-commit hygiene: passed for both changed files
• Manually verified with a 40-line commit body in Source Control Graph:
  • content viewport: 400 px
  • content scroll height: 931 px
  • mouse-wheel input increased scrollTop from 0 to 50
  • metadata and the final commit-message lines are reachable through the hover scrollbar

Screenshot

Before

After

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds explicit max-height constraints to hover content and introduces a regression test to validate the sizing behavior.

Changes:

• Set maxHeight on the hover contents node to match the hover container.
• Add a browser test asserting the hover container and contents receive the expected maxHeight.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/vs/platform/hover/test/browser/hoverService.test.ts	Adds a new test for max-height constraints on hover and hover content.
src/vs/platform/hover/browser/hoverWidget.ts	Applies max-height styling to the hover’s content element in addition to the container.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ktsoator (ktsoator)]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/vs/platform/hover/browser/hoverWidget.ts:618

• Setting the same maxHeight on both the container and the contents can be incorrect if containerDomNode includes vertical padding/borders or other elements besides contentsDomNode (e.g., header/status parts). In that case, the contents may be over-constrained (or cause unnecessary overflow). Consider deriving the contents max height from the actual available inner height (e.g., from containerDomNode.clientHeight after applying container maxHeight, minus any non-content vertical space), or ensuring the contents uses box-sizing: border-box and accounts for padding consistently.

		this._hover.containerDomNode.style.maxHeight = `${maxHeight}px`;
		this._hover.contentsDomNode.style.maxHeight = `${maxHeight}px`;
		if (this._hover.contentsDomNode.clientHeight < this._hover.contentsDomNode.scrollHeight) {
			// Add padding for a vertical scrollbar
			const extraRightPadding = `${this._hover.scrollbar.options.verticalScrollbarSize}px`;

src/vs/platform/hover/test/browser/hoverService.test.ts:258

• This assertion hard-codes an exact px string derived from mainWindow.innerHeight, which can make the test brittle if the implementation rounds maxHeight (or if the environment applies normalization). A more robust check is to compare numeric values (e.g., parseFloat(...)) and/or assert the max height is within an expected bound (<= innerHeight * ratio) while still verifying both nodes are constrained consistently.

			const expectedMaxHeight = `${mainWindow.innerHeight * 0.25}px`;

			assert.deepStrictEqual({
				hoverMaxHeight: hoverWidget.domNode.style.maxHeight,
				contentsMaxHeight: contentsDomNode.style.maxHeight,
				contentOverflows: contentsDomNode.scrollHeight > contentsDomNode.clientHeight
			}, {
				hoverMaxHeight: expectedMaxHeight,
				contentsMaxHeight: expectedMaxHeight,
				contentOverflows: true
			});

[ktsoator (ktsoator)]

Hi Ladislau Szomoru (@lszomoru), thanks for taking a look at this.

VS Code has been my primary editor for a long time, and I really appreciate the quality and attention to detail that goes into it. Since I use the Source Control Graph regularly, I had been running into this behavior for some time, and I could still reproduce it after updating to the latest VS Code version available to me.

Initially, I was not sure whether the truncation was intentional behavior or a bug. After spending some time tracing the hover layout code, it appeared that the maxHeight of the scrollable content was cleared during layout but was not restored along with the outer hover constraint.

I submitted this PR based on that finding and included a regression test, manual verification, and before/after screenshots. Please let me know if I have misunderstood the intended hover behavior or if you would prefer a different approach.

Thank you for your time and for maintaining a tool that I use every day.

[ktsoator (ktsoator)]

Hi Dmitriy Vasyura (@dmitrivMS), apologies for the direct ping. I noticed that you have recently worked on the shared hover code, so I thought this change might be relevant to you.

I updated to the latest VS Code version available to me today and can still reproduce the truncation described in #316535. This PR restores the height constraint on the scrollable hover content and includes a regression test, manual verification, and before/after screenshots.

When you have a chance, could you please take a look or let me know whether this approach is consistent with the intended hover layout behavior? Thank you for your time.