Remove fallback for anchor positioning#312044
Conversation
The stable safari and firefox release should both support this now This lets up clean up a fair amount of manual layout logic Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes legacy fallback logic for positioning overlay webviews and switches callers to an anchor-positioning-based API, simplifying layout/resize workarounds now that stable Safari/Firefox support CSS anchor positioning.
Changes:
- Replace
layoutWebviewOverElement(...)withsetAnchorElement(...)across webview consumers (editors, views, notebooks, etc.). - Remove manual resize/scroll/timeout-based repositioning logic that was previously needed for legacy/fallback layout.
- Simplify
OverlayLayoutElementto rely solely on CSS anchor positioning (removing dimension/position fallbacks and feature detection).
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/webviewView/browser/webviewViewPane.ts | Drops ResizeObserver/timeout repositioning and anchors the webview to the view container. |
| src/vs/workbench/contrib/webviewPanel/browser/webviewEditor.ts | Updates webview panel positioning to use setAnchorElement on scroll/layout events. |
| src/vs/workbench/contrib/webview/browser/webview.ts | Renames the overlay positioning API in IOverlayWebview to setAnchorElement. |
| src/vs/workbench/contrib/webview/browser/overlayWebview.ts | Implements setAnchorElement and preserves anchor state across claim() calls. |
| src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts | Switches notebook overlay layout to anchor-based positioning (no fallback dimension/position). |
| src/vs/workbench/contrib/mcp/browser/mcpServerEditor.ts | Updates embedded webview anchoring calls to the new API. |
| src/vs/workbench/contrib/extensions/browser/extensionEditor.ts | Updates embedded webview anchoring calls to the new API. |
| src/vs/workbench/contrib/chat/browser/agentPluginEditor/agentPluginEditor.ts | Updates embedded webview anchoring calls to the new API. |
| src/vs/base/browser/overlayLayoutElement.ts | Removes fallback positioning paths and exposes setAnchorElement as the primary mechanism. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 2
| // Force the overlay layout to be created if it doesn't exist | ||
| this.overlayLayout.setAnchorElement(anchorElement, { clippingContainer }); |
There was a problem hiding this comment.
setAnchorElement forces overlayLayout creation even when the webview has not been claimed yet. When _windowId is still undefined, this will attach the overlay layout to the main window container and then claim() will immediately dispose/recreate it when assigning the actual target window. Consider only caching _anchorState here and applying it only if _overlayLayout already exists (and rely on the existing claim() re-apply) to avoid unnecessary DOM churn / wrong-window attachment.
| // Force the overlay layout to be created if it doesn't exist | |
| this.overlayLayout.setAnchorElement(anchorElement, { clippingContainer }); | |
| this._overlayLayout?.setAnchorElement(anchorElement, { clippingContainer }); |
| * Call {@link setAnchorElement} each time the layout is recalculated. When the | ||
| * same anchor element is passed again the call is a no-op (the browser keeps them in sync). |
There was a problem hiding this comment.
The class-level doc says to call setAnchorElement “each time the layout is recalculated”, but the method-level doc below says it only needs to be called when the anchor/clipping container changes. Please reconcile these comments so the guidance is consistent (and matches the new anchor-positioning-only behavior).
| * Call {@link setAnchorElement} each time the layout is recalculated. When the | |
| * same anchor element is passed again the call is a no-op (the browser keeps them in sync). | |
| * Call {@link setAnchorElement} when the anchor element or clipping container changes. | |
| * Passing the same anchor element again is a no-op because the browser keeps them in sync. |
The stable safari and firefox release should both support this now
This lets up clean up a fair amount of manual layout logic and workarounds we needed previously