Skip to content

Browser: Remove duplicate border in Modern UI - #335463

Merged
Lee Murray (mrleemurray) merged 2 commits into
microsoft:mainfrom
mrleemurray:agents/fix-double-border-modern-ui-browser-editor
Sep 10, 2026
Merged

Browser: Remove duplicate border in Modern UI#335463
Lee Murray (mrleemurray) merged 2 commits into
microsoft:mainfrom
mrleemurray:agents/fix-double-border-modern-ui-browser-editor

Conversation

@mrleemurray

@mrleemurray Lee Murray (mrleemurray) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • let the Modern UI editor card own the integrated Browser's resting boundary
  • retain the Browser-specific border reservation for classic UI, modal editors, agent sharing, and device emulation
  • cover fractional-zoom rounding at the editor-card edge and add focused regression coverage

Validation

  • npm run compile
  • ./scripts/test.sh --runGlob '**/webContentsViewRendererFeature.test.js'
  • ESLint on the changed production and test files
  • targeted stylelint/design-token hygiene on Browser and Modern UI boundary styles (no errors)
  • real UI checks with Modern UI on/off; dark, light, and high-contrast themes; focused and inactive split Browser editors; zoom levels -1, 0, and +1; window resize; rounded corners; and device emulation

Fixes #327036

Before / After

Both captures use the Browser welcome page, Dark Modern, Modern UI enabled, a 1440×900 window, zoom level 0, hidden sidebars/panel, and the focused URL field.

Context Before After
Full window Before: full window showing the inset Browser boundary After: full window with the Browser content flush to the editor card
Border detail Before: close crop showing the double border After: close crop showing the redundant inner border removed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings September 10, 2026 12:28
@vs-code-engineering

Copy link
Copy Markdown
Contributor

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

Kyle Cutler (@kycutler)

Matched files:

  • src/vs/workbench/contrib/browserView/electron-browser/features/webContentsViewRendererFeature.ts
  • src/vs/workbench/contrib/browserView/test/electron-browser/features/webContentsViewRendererFeature.test.ts

Joaquín Ruales (@jruales)

Matched files:

  • src/vs/workbench/contrib/browserView/electron-browser/features/webContentsViewRendererFeature.ts
  • src/vs/workbench/contrib/browserView/test/electron-browser/features/webContentsViewRendererFeature.test.ts

Copilot AI left a comment

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.

🟡 Changes recommended

Pixel expansion must preserve zero-sized panes to prevent native-view bleed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Removes the redundant integrated Browser boundary when Modern UI’s editor card supplies it.

Changes:

  • Conditionally reserves Browser border padding.
  • Adds host-pixel snapping for fractional zoom.
  • Adds focused layout regression coverage.
File summaries
File Description
webContentsViewRendererFeature.ts Updates padding and pixel-snapping behavior.
webContentsViewRendererFeature.test.ts Tests UI modes and fractional zoom.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mrleemurray
Lee Murray (mrleemurray) merged commit b4473c5 into microsoft:main Sep 10, 2026
33 checks passed
@vs-code-engineering vs-code-engineering Bot added this to the 1.138.0 milestone Sep 10, 2026
const snappedAbsoluteTop = snapDown(absoluteTop);
return {
...current,
width: coverAvailableArea && current.width !== 0 ? snapUp(absoluteLeft + current.width) - snappedAbsoluteLeft : snapDown(current.width),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why was this rounding changed? Rounding up causes the browser to overflow the available area in certain cases (whereas rounding down leaves a bit of extra margin in those cases which is generally preferable).

Comment on lines -94 to -102
// Snap CSS-pixel values down so `v × hostZoom` is an exact integer:
// main places the WCV at `round(v × hostZoom) × systemDPR` physical
// pixels while CSS renders it at `v × hostZoom × systemDPR`, so this
// collapses main's rounding to a no-op and keeps the WebContentsView
// aligned with the placeholder screenshot. We snap the absolute
// origin (pane origin + local offset) then derive the corresponding
// local position so the DOM element and the WCV land on the same
// physical pixel. Runs late so it refines whatever sizing upstream
// contributions (e.g. device emulation) produced.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This comment gives really important context and shouldn't have been removed

@kycutler

Copy link
Copy Markdown
Collaborator

Heads-up to some issues with this:

  1. The browser will overflow the area in some cases (e.g. depending on VS Code zoom levels, screen pixel densities). It's one or two pixels so not a huge deal but for one thing it means the placeholder screenshot doesn't actually cover the same area as the view, so it will jump a bit when the browser is paused
  2. The border radius does not match that of the panels. Electron doesn't support different radii per corner so our only option would be to make the top radii larger too
capture

I'm also not a fan of the page jumping when shared, but this is more minor so could be left for feedback

@mrleemurray

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback - I think we should revert for now & revisit later #335487

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modern UI - Double border for the integrated browser

5 participants