Skip to content

Commit cd3e231

Browse files
authored
Merge pull request #1520 from AR307/codex/upstream-browser-capture-bounds
fix(browser): keep preview placement responsive during screenshot capture
2 parents ecd5799 + ea4aaa8 commit cd3e231

7 files changed

Lines changed: 473 additions & 42 deletions

File tree

‎apps/desktop/electron/main/browser-view.ts‎

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { shell, WebContentsView, type BrowserWindow, type WebContents } from "electron";
1+
import { shell, View, WebContentsView, type BrowserWindow, type WebContents } from "electron";
22
import { realpathSync, statSync, watch, type FSWatcher } from "node:fs";
33
import { dirname, isAbsolute, resolve, sep } from "node:path";
44
import { fileURLToPath, pathToFileURL } from "node:url";
@@ -8,8 +8,8 @@ import { isAllowedHttpUrl, parseAllowedExternalUrl } from "./safe-open-external"
88
/**
99
* Work panel embedded preview browser (D100, ADR 0019).
1010
*
11-
* A single WebContentsView owned by the main process, attached to the main
12-
* window and positioned from renderer-measured bounds. The renderer is the
11+
* A single WebContentsView owned by the main process, clipped by a native
12+
* presentation View positioned from renderer-measured bounds. The renderer is the
1313
* visibility authority: it hides the view whenever the browser tab is not
1414
* the active panel surface or a blocking overlay opens (the view always
1515
* composites above renderer content).
@@ -92,6 +92,7 @@ export function resolveLocalFile(raw: string, root: string | null): string | nul
9292

9393
export class BrowserPane {
9494
private view: WebContentsView | null = null;
95+
private surface: View | null = null;
9596
private window: BrowserWindow | null = null;
9697
private visible = false;
9798
private bounds = { x: 0, y: 0, width: 0, height: 0 };
@@ -309,23 +310,26 @@ export class BrowserPane {
309310
}
310311

311312
private applyBounds(): void {
312-
// Chromium restores capture-time viewport metrics on completion. Keep
313-
// native bounds unchanged meanwhile, then apply the latest requested size.
314-
if (this.view && this.visible && !this.capturing && !this.view.webContents.isDestroyed()) {
315-
this.view.setBounds(this.bounds);
313+
// Move and clip the presentation surface immediately, even while Chromium
314+
// pins its capture-time viewport. A pending screenshot must not leave the
315+
// guest over the conversation when the window or work panel moves.
316+
this.surface?.setBounds(this.bounds);
317+
if (this.view && this.visible && !this.capturing && this.getWebContents()) {
318+
this.view.setBounds({ x: 0, y: 0, width: this.bounds.width, height: this.bounds.height });
316319
}
317320
}
318321

319322
setVisible(visible: boolean): void {
320323
this.visible = visible;
321324
if (!this.view) return;
325+
this.surface?.setVisible(visible);
322326
this.view.setVisible?.(visible);
323327
if (visible) this.attach();
324328
else this.detach();
325329
}
326330

327331
openExternal(): void {
328-
const url = this.view?.webContents.getURL();
332+
const url = this.getWebContents()?.getURL();
329333
if (!url) return;
330334
const allowed = parseAllowedExternalUrl(url);
331335
if (allowed) {
@@ -346,30 +350,33 @@ export class BrowserPane {
346350
this.clearLiveReload();
347351
this.detach();
348352
if (this.view) {
349-
this.view.webContents.close();
353+
const wc = this.getWebContents();
354+
this.surface?.removeChildView(this.view);
350355
this.view = null;
356+
this.surface = null;
357+
wc?.close();
351358
}
352359
}
353360

354361
private attach(): void {
355-
if (!this.window || this.window.isDestroyed() || !this.view) return;
362+
if (!this.window || this.window.isDestroyed() || !this.surface) return;
356363
const children = this.window.contentView.children;
357364
// The guest hole sits on top of plugin chrome. Re-adding a plugin view
358365
// after this pane is attached would cover the guest unless we keep it last.
359-
if (children.includes(this.view) && children[children.length - 1] !== this.view) {
360-
this.window.contentView.removeChildView(this.view);
366+
if (children.includes(this.surface) && children[children.length - 1] !== this.surface) {
367+
this.window.contentView.removeChildView(this.surface);
361368
}
362-
if (!this.window.contentView.children.includes(this.view)) {
363-
this.window.contentView.addChildView(this.view);
369+
if (!this.window.contentView.children.includes(this.surface)) {
370+
this.window.contentView.addChildView(this.surface);
364371
}
365372
this.applyBounds();
366373
}
367374

368375
private detach(): void {
369-
if (!this.window || this.window.isDestroyed() || !this.view) return;
376+
if (!this.window || this.window.isDestroyed() || !this.surface) return;
370377
const children = this.window.contentView.children;
371-
if (children.includes(this.view)) {
372-
this.window.contentView.removeChildView(this.view);
378+
if (children.includes(this.surface)) {
379+
this.window.contentView.removeChildView(this.surface);
373380
}
374381
}
375382

@@ -417,7 +424,9 @@ export class BrowserPane {
417424
}
418425

419426
private ensureView(): WebContentsView {
420-
if (this.view && !this.view.webContents.isDestroyed()) return this.view;
427+
if (this.view && this.getWebContents()) return this.view;
428+
this.detach();
429+
if (this.view) this.surface?.removeChildView(this.view);
421430
const view = new WebContentsView({
422431
webPreferences: {
423432
sandbox: true,
@@ -427,6 +436,11 @@ export class BrowserPane {
427436
partition: PARTITION,
428437
},
429438
});
439+
const surface = new View();
440+
surface.addChildView(view);
441+
surface.setVisible(this.visible);
442+
surface.setBounds(this.bounds);
443+
this.surface = surface;
430444
const wc = view.webContents;
431445
wc.setWindowOpenHandler(({ url }) => {
432446
const allowed = parseAllowedExternalUrl(url);

‎apps/desktop/test/browser-capture-resize.test.mjs‎

Lines changed: 110 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,21 @@ import test from "node:test";
99
const electron = `data:text/javascript,${encodeURIComponent(`
1010
import { EventEmitter } from "node:events";
1111
export const shell = {};
12-
export class WebContentsView {
12+
export class View {
13+
children = [];
14+
parent = null;
15+
visible = true;
16+
bounds = { x: 0, y: 0, width: 0, height: 0 };
17+
addChildView(view) { this.children.push(view); view.parent = this; }
18+
removeChildView(view) { this.children.splice(this.children.indexOf(view), 1); view.parent = null; }
19+
setBounds(bounds) { this.bounds = { ...bounds }; }
20+
getBounds() { return { ...this.bounds }; }
21+
setVisible(visible) { this.visible = visible; }
22+
}
23+
export class WebContentsView extends View {
1324
static instances = [];
1425
constructor() {
26+
super();
1527
const wc = Object.assign(new EventEmitter(), {
1628
id: WebContentsView.instances.length + 1,
1729
url: "", destroyed: false, viewport: { width: 0, height: 0 },
@@ -24,7 +36,11 @@ const electron = `data:text/javascript,${encodeURIComponent(`
2436
getURL() { return this.url; },
2537
getTitle: () => "fixture", isLoading: () => false,
2638
isDestroyed() { return this.destroyed; },
27-
close() { this.destroyed = true; },
39+
close() {
40+
if (this.destroyed) throw new Error("guest already destroyed");
41+
this.destroyed = true;
42+
this.emit("destroyed");
43+
},
2844
navigationHistory: { canGoBack: () => false, canGoForward: () => false },
2945
setWindowOpenHandler: () => {},
3046
session: { setPermissionRequestHandler: () => {} },
@@ -51,13 +67,13 @@ const electron = `data:text/javascript,${encodeURIComponent(`
5167
},
5268
});
5369
this.webContents = wc;
70+
wc.once("destroyed", () => { this.webContents = undefined; });
5471
WebContentsView.instances.push(this);
5572
}
5673
setBounds(bounds) {
5774
this.bounds = { ...bounds };
5875
this.webContents.viewport = { width: bounds.width, height: bounds.height };
5976
}
60-
setVisible() {}
6177
}
6278
`)}`;
6379
registerHooks({ resolve(specifier, context, next) {
@@ -86,7 +102,41 @@ async function harness(t) {
86102
host.setChromeSession("session", "first");
87103
await host.navigate({ url: "https://fixture.invalid/first" }, "session");
88104
t.after(() => host.dispose());
89-
return { host, resize, wc: WebContentsView.instances.at(-1).webContents };
105+
return { host, resize, children, view: WebContentsView.instances.at(-1), wc: WebContentsView.instances.at(-1).webContents };
106+
}
107+
108+
function paintedBounds(view) {
109+
const own = view.getBounds();
110+
if (!view.parent) return own;
111+
const parent = view.parent.getBounds();
112+
return {
113+
x: parent.x + own.x, y: parent.y + own.y,
114+
width: Math.min(own.width, parent.width - own.x),
115+
height: Math.min(own.height, parent.height - own.y),
116+
};
117+
}
118+
119+
for (const raw of [false, true]) {
120+
test(`${raw ? "raw CDP" : "browser screenshot"} stays inside the relocated panel before capture finishes`, async (t) => {
121+
const { host, wc, view } = await harness(t);
122+
const started = once(wc, "capture-start");
123+
const capture = raw
124+
? host.cdpCommand("Page.captureScreenshot", { format: "png" })
125+
: host.screenshot({ fullPage: true });
126+
await started;
127+
// A maximize/restore can move the panel while the tool's screenshot is
128+
// still pending. The content must never remain above the conversation.
129+
const relocated = { x: 1100, y: 42, width: 620, height: 430 };
130+
host.setChromeSurface({ pluginId: "pi.browser", viewId: "browser", visible: true, bounds: relocated });
131+
host.setGuestHole("pi.browser", { x: 0, y: 0, width: 620, height: 430 });
132+
const during = paintedBounds(view);
133+
assert.deepEqual(wc.viewport, { width: 800, height: 500 }, "capture viewport remains stable");
134+
wc.captures[0].resolve();
135+
await capture;
136+
assert.deepEqual(during, relocated, "move and clip the native guest before awaiting capture");
137+
assert.deepEqual(paintedBounds(view), relocated);
138+
assert.deepEqual(wc.viewport, { width: 620, height: 430 });
139+
});
90140
}
91141

92142
for (const raw of [false, true]) {
@@ -165,3 +215,59 @@ test("closing a tab rejects its queued capture without capturing a replacement",
165215
assert.equal(wc.captures.length, 1);
166216
assert.equal(WebContentsView.instances.at(-1).webContents.captures.length, 0);
167217
});
218+
219+
test("hiding and restoring a captured page keeps its presentation isolated from sibling tabs", async (t) => {
220+
const { host, children, view, wc } = await harness(t);
221+
const surface = view.parent;
222+
const started = once(wc, "capture-start");
223+
const capture = host.screenshot();
224+
await started;
225+
host.setChromeSession("session", "second");
226+
await host.navigate({ url: "https://fixture.invalid/second" }, "session");
227+
assert(!children.includes(surface));
228+
assert.equal(surface.visible, false);
229+
const relocated = { x: 960, y: 70, width: 530, height: 400 };
230+
host.setChromeSurface({ pluginId: "pi.browser", viewId: "browser", visible: true, bounds: relocated });
231+
host.setGuestHole("pi.browser", { x: 0, y: 0, width: 530, height: 400 });
232+
host.setChromeSession("session", "first");
233+
assert.deepEqual(paintedBounds(view), relocated);
234+
assert.equal(surface.visible, true);
235+
assert.equal(children.at(-1), surface);
236+
wc.captures[0].resolve();
237+
await capture;
238+
assert.deepEqual(wc.viewport, { width: 530, height: 400 });
239+
host.closeTab("session", "first");
240+
assert(!children.includes(surface));
241+
assert.equal(surface.children.length, 0);
242+
assert.equal(wc.destroyed, true);
243+
});
244+
245+
test("a destroyed native guest can be resized, replaced and disposed safely", async (t) => {
246+
const { host, resize, wc, view, children } = await harness(t);
247+
const previousSurface = view.parent;
248+
wc.close();
249+
assert.equal(view.webContents, undefined, "Electron clears a destroyed guest handle");
250+
resize(930, 620);
251+
await host.navigate({ url: "https://fixture.invalid/recreated" }, "session");
252+
const replacement = WebContentsView.instances.at(-1);
253+
assert.notEqual(replacement, view);
254+
assert(!children.includes(previousSurface));
255+
assert.equal(children.length, 1);
256+
assert.deepEqual(replacement.webContents.viewport, { width: 930, height: 620 });
257+
replacement.webContents.close();
258+
host.dispose();
259+
host.dispose();
260+
assert.equal(children.length, 0);
261+
});
262+
263+
test("capture completion after native guest destruction preserves the original capture result", async (t) => {
264+
const { host, resize, wc } = await harness(t);
265+
const started = once(wc, "capture-start");
266+
const capture = host.screenshot();
267+
await started;
268+
wc.close();
269+
resize(900, 620);
270+
wc.captures[0].resolve();
271+
assert.equal((await capture).data, "fixture-image");
272+
host.dispose();
273+
});

‎apps/desktop/test/browser-pane-navigation.test.mjs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ import { pathToFileURL } from "node:url";
1111
const electron = `data:text/javascript,${encodeURIComponent(`
1212
import { EventEmitter } from "node:events";
1313
export const shell = {};
14+
export class View {
15+
children = [];
16+
addChildView(view) { this.children.push(view); }
17+
removeChildView(view) { this.children.splice(this.children.indexOf(view), 1); }
18+
setVisible() {}
19+
setBounds() {}
20+
}
1421
export class WebContentsView {
1522
static instances = [];
1623
constructor() {

‎docs/spec/06-delivery/04-e2e-test-plan.md‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4481,23 +4481,31 @@ window; opening a normal panel afterward must still work.
44814481
- **Milestone**: M5
44824482
- **Status**: Draft (manual)
44834483

4484-
#### E2E-BROWSER-capture-resize: Capture completion preserves the latest viewport
4484+
#### E2E-BROWSER-capture-resize: Capture preserves live placement and restores the latest viewport
44854485

44864486
- **Preconditions**: Isolated Electron profile and a local responsive page
44874487
taller than the visible browser viewport. No provider account is needed.
4488-
- **Steps**: Start a full-page screenshot, then resize the browser hole twice
4489-
before Chromium completes it. Repeat while alternating larger and smaller
4490-
sizes and through raw `Page.captureScreenshot`. Queue overlapping captures,
4491-
change resource tabs, fail a capture, and close a tab with a queued capture.
4492-
- **Expected**: The completed capture does not restore a stale viewport. The
4493-
page's `innerWidth`/`innerHeight` match the latest requested bounds. Captures
4494-
on one page serialize without blocking a sibling page. Failed capture
4495-
releases resize handling. Closing a page cannot redirect its queued capture
4496-
to another page. No screenshot is repeated to repair layout.
4497-
- **Status**: Native Electron capture/resize path automated by
4488+
- **Steps**: Start a full-page screenshot and keep it pending while resizing
4489+
the browser hole twice. Repeat with raw `Page.captureScreenshot`. While a
4490+
capture is pending, maximize, restore, enter fullscreen, and leave fullscreen
4491+
so the panel moves or changes size. Also queue overlapping captures, switch
4492+
resource tabs, hide and restore a captured page, fail a capture, close a tab
4493+
with a queued capture, and recreate a destroyed guest.
4494+
- **Expected**: During capture, the native guest follows the panel's current
4495+
position and remains clipped within its bounds; it never covers the
4496+
conversation. Its capture-time viewport stays stable until completion, then
4497+
`innerWidth`/`innerHeight` match the latest requested bounds. Captures on one
4498+
page serialize without blocking a sibling page. Failed capture releases
4499+
resize handling. Hiding or closing a page cannot expose it or redirect its
4500+
queued capture to another page. A destroyed guest leaves no orphan native
4501+
view when it is recreated or disposed. No screenshot is repeated to repair
4502+
layout.
4503+
- **Status**: Native Electron capture/resize and pending window-transition
4504+
placement path automated by
44984505
`node scripts/e2e-browser-capture-resize.mjs` (artifacts retained). The
4499-
production Host/Pane/CDP service paths for failure, queueing and tab closure
4500-
are covered by `apps/desktop/test/browser-capture-resize.test.mjs`.
4506+
production Host/Pane/CDP service paths for failure, queueing, tab closure,
4507+
guest recreation, and relocated-panel clipping are covered by
4508+
`apps/desktop/test/browser-capture-resize.test.mjs`.
45014509

45024510
#### E2E-BROWSER-session-preview-race: Session switching does not expose a stale preview
45034511

‎docs/zh-CN/spec/06-delivery/04-e2e-test-plan.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2460,6 +2460,19 @@ MainChat 弥补了缺口。 Maximized/fullscreen 调用保留最新的
24602460
- **里程碑**:M5
24612461
- **状态**:草案(手动)
24622462

2463+
#### E2E-BROWSER-capture-resize:截图期间保持预览位置并恢复最新视口
2464+
2465+
- **前提条件**:隔离的 Electron 配置;本地响应式网页高于浏览器可见视口。无需配置模型服务。
2466+
- **步骤**:开始整页截图并保持其等待状态,期间两次调整浏览器占位区域;通过原始
2467+
`Page.captureScreenshot` 重复。截图等待期间依次最大化、还原、进入和退出全屏,使面板移动或改变尺寸。
2468+
同时覆盖:排队中的截图、切换资源标签、隐藏并恢复截图页、截图失败、带排队任务的标签关闭,以及销毁后的 guest 重建。
2469+
- **预期**:截图等待期间,原生 guest 跟随面板当前位置,并裁剪在面板边界内,不覆盖聊天区域。
2470+
截图视口在完成前保持稳定,完成后 `innerWidth`/`innerHeight` 与最新请求尺寸一致。同一页面的截图串行执行,
2471+
不阻塞其他标签。失败截图会解除尺寸锁定;隐藏或关闭页面不会意外显示页面,也不会把排队截图转发到其他页面。
2472+
重建或释放销毁的 guest 后不残留孤立原生视图。无需重复截图来修复布局。
2473+
- **关联规范**:英文源规格中的 [E2E-BROWSER-capture-resize](../../../spec/06-delivery/04-e2e-test-plan.md#e2e-browser-capture-resize-capture-preserves-live-placement-and-restores-the-latest-viewport)。
2474+
- **状态**:原生 Electron 截图/尺寸及窗口切换期间的位置验证由 `node scripts/e2e-browser-capture-resize.mjs` 自动执行(保留产物)。`apps/desktop/test/browser-capture-resize.test.mjs` 覆盖生产 Host/Pane/CDP 路径、失败、排队、标签关闭、guest 重建及面板裁剪。
2475+
24632476
#### E2E-BROWSER-session-preview-race:切换会话不显示过期预览
24642477

24652478
- **前置条件**:两个会话具有不同 HTML 预览,浏览器面板上下文分别保留。夹具可以独立

0 commit comments

Comments
 (0)