Skip to content

Commit b9e3ef9

Browse files
Fix title bar maximize/restore desync and cross-window focus theft (#1038)
* Fix title bar maximize desync and cross-window focus theft The maximize and restore buttons only toggled themselves inside their own onclick handlers, so maximizing through a double click on the drag region or through the OS window controls left the buttons showing the wrong action. Route the button visibility through a new SetMaximized renderer event driven by the window's own maximize, unmaximize and restore events, mirroring the existing SetActive and ShowServerStatus path, so the real window state is the single source of truth. The window and titlebar focus handlers only forwarded focus to the lab view, which is null while the Welcome view is showing, so the welcome content never received keyboard focus. Forward to the active content view instead, guarding against a destroyed webContents so an env switch cannot focus a disposed view. The lab view also grabbed focus unconditionally on did-finish-load, letting a background second window steal focus whenever its lab reloaded. Only focus it when its window is already focused. I worked through these fixes with Claude Code and verified the build with tsc and the unit suite locally. * Guard the title bar drag region with an e2e test The title bar renders inside its own WebContentsView and window dragging depends on the app-region CSS staying put: a drag surface on the title and a no-drag opt-out on every control. A refactor that drops either rule breaks window moving or the buttons with no signal from the type checker or the unit suite. Pin the computed declarations so that regression fails loudly. The real OS window move cannot be driven from Playwright since synthetic mouse events do not trigger the app-region hit-test, so this stays a CSS guard and the actual drag plus the layered-view no-drag behavior remain manual per OS. The focus fixes in this branch (welcome view focus in Welcome mode, background window not stealing focus) resist a deterministic e2e assertion too: the app never becomes OS-frontmost under the test harness, so webContents.isFocused() always reads false and any assertion would be tautological. Those stay manual and are written up in the PR. I worked these tests and the harness probing out with Claude Code and confirmed the guard fails when the drag rule is removed. --------- Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
1 parent 66d31c3 commit b9e3ef9

8 files changed

Lines changed: 152 additions & 7 deletions

File tree

src/main/eventtypes.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ export enum EventTypeRenderer {
9696
SetRunningServerList = 'set-running-server-list',
9797
SetTitle = 'set-title',
9898
SetActive = 'set-active',
99+
SetMaximized = 'set-maximized',
99100
ShowServerStatus = 'show-server-status',
100101
ShowServerNotificationBadge = 'show-server-notification-badge',
101102
SetRecentSessionList = 'set-recent-session-list',

src/main/sessionwindow/sessionwindow.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,10 +193,16 @@ export class SessionWindow implements IDisposable {
193193
// this._labView freshly means an env switch that recreates the labView cannot
194194
// leave a stale, disposed webContents behind, nor accumulate a handler per switch.
195195
this._window.webContents.on('focus', () => {
196-
this._labView?.view?.webContents?.focus();
196+
const wc = this.contentView?.webContents;
197+
if (wc && !wc.isDestroyed()) {
198+
wc.focus();
199+
}
197200
});
198201
this._titleBarView.view.webContents.on('focus', () => {
199-
this._labView?.view?.webContents?.focus();
202+
const wc = this.contentView?.webContents;
203+
if (wc && !wc.isDestroyed()) {
204+
wc.focus();
205+
}
200206
});
201207

202208
if (this._contentViewType === ContentViewType.Lab) {
@@ -241,12 +247,15 @@ export class SessionWindow implements IDisposable {
241247
this._resizeViewsDelayed();
242248
});
243249
this._window.on('maximize', () => {
250+
this._titleBarView.setMaximized(this._window.isMaximized());
244251
this._resizeViewsDelayed();
245252
});
246253
this._window.on('unmaximize', () => {
254+
this._titleBarView.setMaximized(this._window.isMaximized());
247255
this._resizeViewsDelayed();
248256
});
249257
this._window.on('restore', () => {
258+
this._titleBarView.setMaximized(this._window.isMaximized());
250259
this._resizeViewsDelayed();
251260
});
252261
this._window.on('move', () => {
@@ -375,7 +384,9 @@ export class SessionWindow implements IDisposable {
375384
this._window.contentView.addChildView(labView.view);
376385

377386
labView.view.webContents.on('did-finish-load', () => {
378-
labView.view.webContents.focus();
387+
if (this._window.isFocused()) {
388+
labView.view.webContents.focus();
389+
}
379390
});
380391

381392
labView.load((errorCode: number, errorDescription: string) => {

src/main/titlebarview/preload.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,13 @@ const { contextBridge, ipcRenderer } = electron;
55

66
type SetTitleListener = (title: string) => void;
77
type SetActiveListener = (active: boolean) => void;
8+
type SetMaximizedListener = (isMaximized: boolean) => void;
89
type ShowServerStatusListener = (show: boolean) => void;
910
type ShowServerNotificationBadgeListener = (show: boolean) => void;
1011

1112
let onSetTitleListener: SetTitleListener;
1213
let onSetActiveListener: SetActiveListener;
14+
let onSetMaximizedListener: SetMaximizedListener;
1315
let onShowServerStatusListener: ShowServerStatusListener;
1416
let onShowServerNotificationBadgeListener: ShowServerNotificationBadgeListener;
1517

@@ -52,6 +54,9 @@ contextBridge.exposeInMainWorld('electronAPI', {
5254
onSetActive: (callback: SetActiveListener) => {
5355
onSetActiveListener = callback;
5456
},
57+
onSetMaximized: (callback: SetMaximizedListener) => {
58+
onSetMaximizedListener = callback;
59+
},
5560
onShowServerStatus: (callback: ShowServerStatusListener) => {
5661
onShowServerStatusListener = callback;
5762
},
@@ -74,6 +79,12 @@ ipcRenderer.on(EventTypeRenderer.SetActive, (event, active) => {
7479
}
7580
});
7681

82+
ipcRenderer.on(EventTypeRenderer.SetMaximized, (event, isMaximized) => {
83+
if (onSetMaximizedListener) {
84+
onSetMaximizedListener(isMaximized);
85+
}
86+
});
87+
7788
ipcRenderer.on(EventTypeRenderer.ShowServerStatus, (event, show) => {
7889
if (onShowServerStatusListener) {
7990
onShowServerStatusListener(show);

src/main/titlebarview/titlebar.html

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,11 @@
230230
}
231231
});
232232

233+
window.electronAPI.onSetMaximized((isMaximized) => {
234+
maximizeButton.style.display = isMaximized ? 'none' : 'block';
235+
restoreButton.style.display = isMaximized ? 'block' : 'none';
236+
});
237+
233238
window.electronAPI.onShowServerStatus((show) => {
234239
serverButton.style.display = show ? 'flex' : 'none';
235240

@@ -255,15 +260,11 @@
255260
maximizeButton.onclick = (ev) => {
256261
ev.stopPropagation();
257262
window.electronAPI.maximizeWindow();
258-
maximizeButton.style.display = 'none';
259-
restoreButton.style.display = 'block';
260263
};
261264

262265
restoreButton.onclick = (ev) => {
263266
ev.stopPropagation();
264267
window.electronAPI.restoreWindow();
265-
restoreButton.style.display = 'none';
266-
maximizeButton.style.display = 'block';
267268
};
268269

269270
closeButton.onclick = (ev) => {

src/main/titlebarview/titlebarview.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,10 @@ export class TitleBarView {
6262
);
6363
}
6464

65+
setMaximized(isMaximized: boolean) {
66+
this._view.webContents.send(EventTypeRenderer.SetMaximized, isMaximized);
67+
}
68+
6569
showServerStatus(show: boolean) {
6670
this._view.webContents.send(EventTypeRenderer.ShowServerStatus, show);
6771
}
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { expect, test } from '@playwright/test';
2+
import { cleanup, launchApp } from './helpers';
3+
4+
// The title bar is drawn inside its own WebContentsView. Window dragging relies
5+
// on `-webkit-app-region: drag` on the `.app-title` element, and every control
6+
// button opts back out with `-webkit-app-region: no-drag`. Dropping either rule
7+
// in a refactor silently breaks window moving or makes the buttons undraggable.
8+
//
9+
// This is only a guard on the computed CSS declaration. It does NOT exercise a
10+
// real window move: synthetic Playwright/CDP mouse events do not drive the OS
11+
// drag on the app-region hit-test, so an actual drag cannot be asserted here.
12+
// The real window move, and the layered-WebContentsView no-drag hit-testing
13+
// (electron/electron#43320), still have to be verified by hand per OS.
14+
test('the title bar keeps its drag region and no-drag controls', async () => {
15+
const { app, userDataDir, jupyterDir } = await launchApp();
16+
try {
17+
// Arrange: find the titlebar page. It has no window title, so it is located
18+
// by the presence of its `#app-title` element, the same element-probe
19+
// pattern the dialog-titlebar test uses.
20+
const deadline = Date.now() + 20_000;
21+
let titlebar;
22+
while (!titlebar && Date.now() < deadline) {
23+
for (const page of app.windows()) {
24+
const hasTitle = await page
25+
.evaluate(() => !!document.getElementById('app-title'))
26+
.catch(() => false);
27+
if (hasTitle) {
28+
titlebar = page;
29+
break;
30+
}
31+
}
32+
if (!titlebar) {
33+
await new Promise(resolve => setTimeout(resolve, 200));
34+
}
35+
}
36+
if (!titlebar) {
37+
throw new Error('title bar page with #app-title never appeared');
38+
}
39+
await titlebar.waitForLoadState('load');
40+
41+
// Act: read the computed app-region of the drag surface and of two control
42+
// buttons. The JS-side property is `webkitAppRegion`; its value is the
43+
// keyword ('drag' / 'no-drag'), not a length.
44+
const regions = await titlebar.evaluate(() => {
45+
const regionOf = (id: string) => {
46+
const el = document.getElementById(id);
47+
if (!el) {
48+
return null;
49+
}
50+
return (getComputedStyle(el) as CSSStyleDeclaration & {
51+
webkitAppRegion?: string;
52+
}).webkitAppRegion;
53+
};
54+
return {
55+
appTitle: regionOf('app-title'),
56+
closeButton: regionOf('close-button'),
57+
serverButton: regionOf('server-button')
58+
};
59+
});
60+
61+
// Assert: the title is draggable and the controls explicitly opt out. If a
62+
// refactor drops the `-webkit-app-region` rules these flip to the inherited
63+
// default ('none') and the test fails.
64+
expect(regions.appTitle).toBe('drag');
65+
expect(regions.closeButton).toBe('no-drag');
66+
expect(regions.serverButton).toBe('no-drag');
67+
} finally {
68+
await app.close();
69+
cleanup(userDataDir, jupyterDir);
70+
}
71+
});

test/unit/preload/titlebarview.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ describe('titlebarview preload', () => {
2929
'sendMouseEvent',
3030
'onSetTitle',
3131
'onSetActive',
32+
'onSetMaximized',
3233
'onShowServerStatus',
3334
'onShowServerNotificationBadge'
3435
].sort()
@@ -89,6 +90,14 @@ describe('titlebarview preload', () => {
8990
expect(cb).toHaveBeenCalledWith(true);
9091
});
9192

93+
it('onSetMaximized relays SetMaximized to the callback', async () => {
94+
const api = await load();
95+
const cb = vi.fn();
96+
api.onSetMaximized(cb);
97+
rendererHandler(EventTypeRenderer.SetMaximized)({}, true);
98+
expect(cb).toHaveBeenCalledWith(true);
99+
});
100+
92101
it('onShowServerStatus relays ShowServerStatus to the callback', async () => {
93102
const api = await load();
94103
const cb = vi.fn();
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import { describe, expect, it, vi } from 'vitest';
2+
import { TitleBarView } from '../../src/main/titlebarview/titlebarview';
3+
import { EventTypeRenderer } from '../../src/main/eventtypes';
4+
5+
// Drive setMaximized without the heavy WebContentsView constructor: create an
6+
// instance linked to the prototype and stub only the webContents.send it uses.
7+
function makeTitleBarView(send: ReturnType<typeof vi.fn>): TitleBarView {
8+
const view = Object.create(TitleBarView.prototype);
9+
view._view = { webContents: { send } };
10+
return view;
11+
}
12+
13+
describe('TitleBarView.setMaximized', () => {
14+
it('sends SetMaximized with true when the window is maximized', () => {
15+
// Arrange
16+
const send = vi.fn();
17+
const titleBarView = makeTitleBarView(send);
18+
19+
// Act
20+
titleBarView.setMaximized(true);
21+
22+
// Assert
23+
expect(send).toHaveBeenCalledWith(EventTypeRenderer.SetMaximized, true);
24+
});
25+
26+
it('sends SetMaximized with false when the window is restored', () => {
27+
// Arrange
28+
const send = vi.fn();
29+
const titleBarView = makeTitleBarView(send);
30+
31+
// Act
32+
titleBarView.setMaximized(false);
33+
34+
// Assert
35+
expect(send).toHaveBeenCalledWith(EventTypeRenderer.SetMaximized, false);
36+
});
37+
});

0 commit comments

Comments
 (0)