fix: add top gap for Modern UI floating panels when title bar is hidden - #328688
Conversation
…en (microsoft#328681) When the Modern UI experiment (`workbench.experimental.modernUI`) is enabled, the floating-card layout relies on the title bar to provide visual spacing at the top. When the title bar is hidden (e.g. native fullscreen on macOS) the content island becomes flush against the screen edge while left, right, and bottom gaps are preserved. Add a `notitlebar` layout class (mirroring the existing `nostatusbar` pattern for the bottom edge) that is toggled on `mainContainer` whenever the title bar grid view is not visible. This class is set based on the actual title bar visibility (`isVisible(Parts.TITLEBAR_PART)`), not the fullscreen state, so it correctly handles platforms where the title bar remains visible in fullscreen (e.g. Windows with its window controls and hamburger menu). CSS: add a doubled outer gutter (`--vscode-spacing-size40 * 2`, 8px) to the top of all floating parts (sidebar, auxiliary bar, editor, panel, activity bar) when `.notitlebar` is present. Also inset vertical sash highlights to match. JS: update the layout insets in `paneCompositePart.ts` and `editorPart.ts` to deduct the top margin from content dimensions when `isVisible(Parts.TITLEBAR_PART)` returns false. All new CSS selectors require `.floating-panels` (only set when Modern UI is enabled) and the JS changes are guarded by `isFloatingPanelsEnabled()`, so there is no effect when the experiment is disabled. Fixes microsoft#328681
|
@microsoft-github-policy-service agree |
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Benjamin Christopher Simmonds (@benibenj)Matched files:
|
There was a problem hiding this comment.
🟡 Not ready to approve
Activity bar sizing, banner visibility, and top-panel sash alignment currently produce incorrect layouts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a top outer gutter to Modern UI floating panels when the title bar is hidden.
Changes:
- Tracks hidden title bar state with
notitlebar. - Adds top margins and matching content insets.
- Adjusts sash highlights for the new gutter.
File summaries
| File | Description |
|---|---|
paneCompositePart.ts |
Reserves top space for floating pane composites. |
editorPart.ts |
Reserves the editor’s top gutter. |
floatingPanels.css |
Styles top gutters and sash insets. |
layout.ts |
Maintains the notitlebar layout class. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| .monaco-workbench.floating-panels.notitlebar .part.activitybar { | ||
| margin-top: calc(var(--vscode-spacing-size40) * 2); | ||
| } |
| /* When the title bar is hidden (.notitlebar, e.g. native fullscreen on macOS) the top | ||
| * becomes a window edge; double the top margin to match the outer gutter on other | ||
| * window edges. Mirrors the .nostatusbar pattern for the bottom edge. */ | ||
| .monaco-workbench.floating-panels.notitlebar .part.sidebar, | ||
| .monaco-workbench.floating-panels.notitlebar .part.auxiliarybar { | ||
| margin-top: calc(var(--vscode-spacing-size40) * 2); |
| .monaco-workbench.floating-panels.notitlebar.panel-position-top:not(.nopanel) .monaco-sash.vertical:not(.part .monaco-sash)::before { | ||
| top: calc(var(--vscode-spacing-size40) * 2); | ||
| } |
| // When the title bar is hidden (fullscreen), the editor top faces the window edge; | ||
| // use a doubled outer gutter. When a top panel is visible the editor faces the panel | ||
| // card instead (inter-card gap). | ||
| const titleBarHidden = !this.layoutService.isVisible(Parts.TITLEBAR_PART, mainWindow); | ||
| const topMargin = panelAtTop ? FLOATING_PANEL_MARGIN | ||
| : titleBarHidden ? FLOATING_PANEL_MARGIN * 2 : 0; |
|
Please add automated regression coverage for this layout change. The CSS margins and TypeScript content insets must remain synchronized across title-bar visibility, banner visibility, panel positions, and the maximized-panel case. In particular, the existing activity bar tests in |
The activity bar's layout() only reserved the bottom floating-panels gutter. With the new doubled top margin when the title bar is hidden, its content ended up 8px too tall, so it could overlap or clip at the bottom. It now reserves both gutters. The notitlebar class (and everything that reads it — layout.ts, EditorPart, AbstractPaneCompositePart, ActivitybarPart) only checked title bar visibility. A visible banner still gives the floating cards a top edge to sit against, same as a visible title bar, so it shouldn't get the doubled gutter either. Added a shared isFloatingTopEdgeHidden(layoutService, targetWindow) in layoutService.ts that checks both, and made setBannerHidden() update the class when banner visibility changes — it didn't before. Also dropped a redundant sash selector in floatingPanels.css (.notitlebar.panel-position-top:not(.nopanel)) that doubled the sash inset to 8px even though the editor/sidebar margin stays at the normal 4px when a top panel is visible. CSS specificity alone resolves this correctly once the override is gone. Shortened an inline comment in editorPart.ts down to one line to match the inline-comment guideline. Added tests: a suite for isFloatingTopEdgeHidden across title-bar/ banner combinations in layoutService.test.ts, and focused layout() tests in activitybarPart.test.ts covering the content-height reservation across title-bar visibility, banner visibility, and floating-panels-disabled.
There was a problem hiding this comment.
🟡 Not ready to approve
The activity bar still under-reserves its doubled bottom gutter when the status bar is hidden.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
src/vs/workbench/test/browser/parts/activitybar/activitybarPart.test.ts:356
- This disables the lint rule to install a fake through an
anycast, which the repository's test guidance explicitly prohibits. Please provide an injectable/test-visible seam for the composite-bar layout dependency or exercise a realPaneCompositeBarinstead of mutating the private disposable.
// eslint-disable-next-line local/code-no-any-casts
(part as any).compositeBar.value = {
src/vs/workbench/browser/parts/activitybar/activitybarPart.ts:277
- When the status bar is hidden, the CSS gives the activity bar a doubled bottom gutter (
floatingPanels.css:292-293), but this still subtracts only onegutter. The composite bar is therefore laid out 4px too tall and can clip at the bottom—especially in the test-plan case where both top and bottom gutters are doubled. Compute a status-aware bottom gutter here and update the new tests, which currently encode the single-bottom-gutter result.
const contentHeight = Math.max(0, height - gutter - topGutter);
src/vs/workbench/test/browser/parts/activitybar/activitybarPart.test.ts:351
- This four-line inline comment narrates the test and exceeds the repository rule that inline comments stay to one line. The test name and a short note are sufficient.
This issue also appears on line 355 of the same file.
// Regression coverage for the hidden-title-bar transition: `.floating-panels.notitlebar
// .part.activitybar` (floatingPanels.css) adds a doubled top margin when the title bar
// (and banner) are hidden, so `layout()` must reserve that same space in `contentHeight`
// or the composite bar is laid out too tall and its content clips/overlaps at the bottom.
src/vs/workbench/services/layout/browser/layoutService.ts:132
- This JSDoc exceeds the repository's 1–2 short-sentence limit and enumerates implementation consumers. Keep the helper's contract concise; callers are already discoverable from references.
/**
* Whether the top of the floating cards faces the window edge directly and should
* receive the doubled outer gutter (mirrors the doubled bottom gutter used when the
* status bar is hidden). The title bar and the banner are the two grid rows that can
* sit above the middle section, so the cards are only flush with the window edge when
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Tony (@Bosphoramus) can you address the Copilot comments then I'll re-review |
…est the margins The class was called notitlebar, but it only gets set when the title bar and the banner are both hidden. Anyone reading the CSS would guess wrong. It is now top-window-edge, which says what actually happens: the floating cards are sitting against the top of the window. The helper is isFloatingTopEdgeExposed to match. Six places were updating that class by hand. layout.ts already has an event for exactly this, and it covers both the title bar and the banner, so one subscription does the job. Seven places flip those two views today, and doing it by hand works right up until someone adds an eighth and forgets. The activity bar's layout() now checks this.content instead of the composite bar, and only calls the bar if it exists. Both this.content and partLayout are set in Part.create(), so nothing breaks, and the content area gets sized whether or not the bar is there. That means a test can read the height back instead of poking at private fields. The margin formulas for the pane composites and the editor had no tests at all, which left the panel positions and the maximized panel unprotected. They are plain functions of the layout service, so they moved to layoutService.ts next to the other floating helpers, the same way isSidebarSiblingToEditor already delegated there. paneCompositePart and editorPart lose about ninety lines between them, and the logic is unchanged. Tests now cover the panel at top, bottom and left, centered and justified alignment, the panel maximized with and without something above it, the status bar hidden, and the experiment switched off. The activity bar ones drive layout() for real and read the height back. No casts, no lint suppressions.
There was a problem hiding this comment.
🟢 Ready to approve
The CSS and layout calculations are consistently gated and covered across the relevant visibility and panel configurations.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
I'll resolve merge conflicts asap Lee Murray (@mrleemurray) |
There was a problem hiding this comment.
🟢 Ready to approve
The behavior is consistently implemented and tested; remaining feedback is non-blocking comment cleanup.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
src/vs/workbench/services/layout/browser/layoutService.ts:292
- Repository guidance limits method-body comments to one line. Please condense this explanation while retaining the non-obvious edge relationship.
// A top panel faces the editor below it, and a sibling bar faces a bottom panel card,
// so in neither case does the bottom reach the window edge.
src/vs/workbench/browser/parts/activitybar/activitybarPart.ts:270
- This comment only narrates the immediately preceding guard, which the repository guidance explicitly asks method-body comments to avoid. Removing it keeps the control flow self-explanatory.
return; // not created yet
src/vs/workbench/services/layout/browser/layoutService.ts:281
- Repository guidance limits method-body comments to one line. This constraint can be stated concisely without splitting the explanation across two lines.
This issue also appears on line 291 of the same file.
// A visible editor sits above the panel, so the gap is between two cards. When the
// panel is maximized the editor is gone and the panel takes over that row instead.
src/vs/workbench/browser/layout.ts:1704
- Repository guidance limits method-body comments to one line. This explanation can be reduced to a single concise statement.
// The floating cards abut the top window edge only while neither of the two grid rows
// above the middle section is showing, so track both rather than the title bar alone.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Lee Murray (mrleemurray)
left a comment
Lee Murray (mrleemurray)
left a comment
There was a problem hiding this comment.
Thanks!
|
Lee Murray (@mrleemurray) There is a failing test: First it failed on Linux, now its failing on macOS But i think its unrelated to my changes |
b9ca85e
into
microsoft:main
Fixes #328681
Problem
When Modern UI (
workbench.experimental.modernUI) is enabled and the title bar is hidden (e.g. native fullscreen on macOS), the floating-card content island is flush against the screen top while left, right, and bottom gaps are preserved.Fix
notitlebarlayout class (mirroringnostatusbar) toggled onmainContainerbased onisVisible(Parts.TITLEBAR_PART)— whether the title bar is visible in fullscreen depends on configuration (window.commandCenter, activity bar position, etc.), not just platform, so the fix is gated on actual visibility rather than fullscreen state8px) on all floating parts when.notitlebaris presentpaneCompositePart.tsandeditorPart.tsAll changes are gated behind
.floating-panels/isFloatingPanelsEnabled()— no effect when Modern UI is disabled.Test plan
window.commandCenter: false: title bar hides, top gap appearswindow.commandCenter: true(default): title bar visible, no extra gap