History

fix(desktop): app zoom no longer zooms the preview browser (#6649)

Julius Marminge committed 2 months ago

Commit d8a6dfd1 parent e9ae1344 files · +219 −38Browse files

Unified · Split

4 files
  1. apps/desktop/src/preview/Manager.test.ts +119 −15
  2. apps/desktop/src/preview/Manager.ts +49 −23
  3. apps/desktop/src/window/DesktopWindow.test.ts +47 −0
  4. apps/desktop/src/window/DesktopWindow.ts +4 −0
apps/desktop/src/preview/Manager.test.ts+119−15
@@ -979,7 +979,10 @@ describe("PreviewManager", () => { · 978 unmodified lines
979979 ),
980980 );
981981
982− effectIt.effect("mirrors Electron's effective zoom across registration and navigation", () =>
982+ // The guest reports whatever zoom level Chromium handed it from the app
983+ // window, so the tab's own zoom is the source of truth in both directions:
984+ // asserted onto every guest, never read back off one.
985+ effectIt.effect("keeps the tab's own zoom instead of the guest's reported zoom", () =>
983986 withManager((manager) =>
984987 Effect.gen(function* () {
985988 let effectiveZoom = 0.9;
@@ -1025,18 +1028,13 @@ describe("PreviewManager", () => { · 39 unmodified lines
10251028 yield* manager.createTab("tab_zoom");
10261029 yield* manager.registerWebview("tab_zoom", 42);
10271030
1028− expect(states.at(-1)?.zoomFactor).toBe(0.9);
1029− expect(setZoomFactor).not.toHaveBeenCalled();
1031+ expect(states.at(-1)?.zoomFactor).toBe(1);
1032+ expect(setZoomFactor).toHaveBeenCalledWith(1);
10301033
1031− effectiveZoom = 1.25;
1032− listeners.get("did-navigate")?.();
1033− yield* Effect.yieldNow;
1034−
1035− expect(states.at(-1)?.zoomFactor).toBe(1.25);
1036− expect(setZoomFactor).not.toHaveBeenCalled();
1037−
1038− zoomReadable = false;
1039− url = "https://example.com/after-zoom-read-failed";
1034+ // An app zoom leaves the guest reporting the inherited level. Navigating
1035+ // must not adopt it as the preview's zoom.
1036+ effectiveZoom = 0.8;
1037+ url = "https://example.com/after-app-zoom";
10401038 listeners.get("did-navigate")?.();
10411039 yield* Effect.yieldNow;
10421040
@@ -1045,7 +1043,18 @@ describe("PreviewManager", () => { · 2 unmodified lines
10451043 url,
10461044 title: "Example",
10471045 });
1048− expect(states.at(-1)?.zoomFactor).toBe(1.25);
1046+ expect(states.at(-1)?.zoomFactor).toBe(1);
1047+
1048+ // Only the preview's own zoom controls move it.
1049+ yield* manager.zoomIn("tab_zoom");
1050+ expect(setZoomFactor).toHaveBeenCalledWith(1.1);
1051+ expect(states.at(-1)?.zoomFactor).toBe(1.1);
1052+
1053+ zoomReadable = false;
1054+ listeners.get("did-navigate")?.();
1055+ yield* Effect.yieldNow;
1056+
1057+ expect(states.at(-1)?.zoomFactor).toBe(1.1);
10491058
10501059 const replacementSetZoomFactor = vi.fn();
10511060 fromId.mockReturnValue({
@@ -1074,8 +1083,103 @@ describe("PreviewManager", () => { · 22 unmodified lines
10741083
10751084 yield* manager.registerWebview("tab_zoom", 43);
10761085
1077− expect(replacementSetZoomFactor).toHaveBeenCalledWith(1.25);
1078− expect(states.at(-1)?.zoomFactor).toBe(1.25);
1086+ expect(replacementSetZoomFactor).toHaveBeenCalledWith(1.1);
1087+ expect(states.at(-1)?.zoomFactor).toBe(1.1);
1088+ }),
1089+ ),
1090+ );
1091+
1092+ // Zooming the app UI pushes the window's zoom level onto every guest, so the
1093+ // preview has to be put back at the zoom the user gave it.
1094+ effectIt.effect("re-applies each tab's own zoom when the app window zooms", () =>
1095+ withManager((manager) =>
1096+ Effect.gen(function* () {
1097+ const setZoomFactor = vi.fn();
1098+ fromId.mockReturnValue({
1099+ id: 42,
1100+ isDestroyed: () => false,
1101+ getType: () => "webview",
1102+ getURL: () => "https://example.com",
1103+ getTitle: () => "Example",
1104+ isLoading: () => false,
1105+ getZoomFactor: () => 1,
1106+ setZoomFactor,
1107+ on: vi.fn(),
1108+ off: vi.fn(),
1109+ ipc: { on: vi.fn(), off: vi.fn() },
1110+ send: webviewSend,
1111+ navigationHistory: { canGoBack: () => false, canGoForward: () => false },
1112+ setWindowOpenHandler: vi.fn(),
1113+ debugger: {
1114+ isAttached: () => false,
1115+ attach: vi.fn(),
1116+ sendCommand: vi.fn(async () => undefined),
1117+ on: vi.fn(),
1118+ off: vi.fn(),
1119+ },
1120+ } as never);
1121+
1122+ yield* manager.createTab("tab_reapply");
1123+ yield* manager.registerWebview("tab_reapply", 42);
1124+ yield* manager.zoomIn("tab_reapply");
1125+ setZoomFactor.mockClear();
1126+
1127+ yield* manager.reapplyZoom();
1128+
1129+ expect(setZoomFactor).toHaveBeenCalledTimes(1);
1130+ expect(setZoomFactor).toHaveBeenCalledWith(1.1);
1131+ }),
1132+ ),
1133+ );
1134+
1135+ // did-attach and dom-ready both re-register the guest that is already
1136+ // attached, and a guest that just inherited the app window's zoom needs its
1137+ // own back — without that round trip republishing tab state.
1138+ effectIt.effect("re-asserts the tab's zoom when the active guest registers again", () =>
1139+ withManager((manager) =>
1140+ Effect.gen(function* () {
1141+ const setZoomFactor = vi.fn();
1142+ fromId.mockReturnValue({
1143+ id: 42,
1144+ isDestroyed: () => false,
1145+ getType: () => "webview",
1146+ getURL: () => "https://example.com",
1147+ getTitle: () => "Example",
1148+ isLoading: () => false,
1149+ getZoomFactor: () => 1,
1150+ setZoomFactor,
1151+ on: vi.fn(),
1152+ off: vi.fn(),
1153+ ipc: { on: vi.fn(), off: vi.fn() },
1154+ send: webviewSend,
1155+ navigationHistory: { canGoBack: () => false, canGoForward: () => false },
1156+ setWindowOpenHandler: vi.fn(),
1157+ debugger: {
1158+ isAttached: () => false,
1159+ attach: vi.fn(),
1160+ sendCommand: vi.fn(async () => undefined),
1161+ on: vi.fn(),
1162+ off: vi.fn(),
1163+ },
1164+ } as never);
1165+ const states: PreviewManager.PreviewTabState[] = [];
1166+ yield* manager.subscribeStateChanges((_tabId, state) =>
1167+ Effect.sync(() => {
1168+ states.push(state);
1169+ }),
1170+ );
1171+
1172+ yield* manager.createTab("tab_reregister_zoom");
1173+ yield* manager.registerWebview("tab_reregister_zoom", 42);
1174+ yield* manager.zoomIn("tab_reregister_zoom");
1175+ setZoomFactor.mockClear();
1176+ const publishedBefore = states.length;
1177+
1178+ yield* manager.registerWebview("tab_reregister_zoom", 42);
1179+
1180+ expect(setZoomFactor).toHaveBeenCalledWith(1.1);
1181+ expect(states.length).toBe(publishedBefore);
1182+ expect(states.at(-1)?.zoomFactor).toBe(1.1);
10791183 }),
10801184 ),
10811185 );
apps/desktop/src/preview/Manager.ts+49−23
@@ -647,6 +647,22 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 646 unmodified lines
647647 if (Option.isSome(next)) yield* emit(tabId, next.value);
648648 });
649649
650+ /**
651+ * Pushes a tab's zoom factor onto whichever guest it currently owns, reading
652+ * both at call time. Anything that applies zoom after an await goes through
653+ * here: a snapshot taken before the await can be older than a zoom action that
654+ * landed in between, and re-applying it would roll that action back.
655+ */
656+ const assertTabZoom = Effect.fn("PreviewManager.assertTabZoom")(function* (tabId: string) {
657+ const tab = (yield* SynchronizedRef.get(tabsRef)).get(tabId);
658+ if (!tab || tab.webContentsId == null) return;
659+ const wc = webContents.fromId(tab.webContentsId);
660+ if (!wc || wc.isDestroyed()) return;
661+ yield* attempt({ operation: "assertTabZoom", tabId, webContentsId: wc.id }, () =>
662+ wc.setZoomFactor(tab.zoomFactor),
663+ ).pipe(Effect.ignore);
664+ });
665+
650666 const requireWebContents = Effect.fn("PreviewManager.requireWebContents")(function* (
651667 tabId: string,
652668 ) {
@@ -1305,10 +1321,6 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 652 unmodified lines
13051321 confirmedNavigation = false,
13061322 ) {
13071323 if (wc.isDestroyed()) return;
1308− const zoomFactor = yield* attempt(
1309− { operation: "syncWebContentsState.getZoomFactor", tabId, webContentsId: wc.id },
1310− () => wc.getZoomFactor(),
1311− ).pipe(Effect.option);
13121324 const computedNavStatus = computeNavStatus(wc);
13131325 const canGoBack = wc.navigationHistory.canGoBack();
13141326 const canGoForward = wc.navigationHistory.canGoForward();
@@ -1338,7 +1350,9 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 23 unmodified lines
13381350 navStatus,
13391351 canGoBack,
13401352 canGoForward,
1341− ...(Option.isSome(zoomFactor) ? { zoomFactor: zoomFactor.value } : {}),
1353+ // zoomFactor is deliberately not read back from the guest: Chromium
1354+ // reports the level it inherited from the app window, so mirroring it
1355+ // would turn an app zoom into the preview's own zoom.
13421356 updatedAt,
13431357 };
13441358 return [
@@ -1716,11 +1730,10 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 371 unmodified lines
17161730 const annotationTheme = yield* Ref.get(annotationThemeRef);
17171731 const currentAttachment = attached.get(webContentsId);
17181732 if (tab.webContentsId === webContentsId && currentAttachment?.webContents === wc) {
1719− const zoomFactor = yield* attempt(
1720− { operation: "registerWebview.getZoomFactor", tabId, webContentsId },
1721− () => wc.getZoomFactor(),
1722− );
1723− yield* update(tabId, { zoomFactor });
1733+ // The guest we already own re-announced itself, so nothing about the tab
1734+ // changed. Only push its zoom back down — Chromium may have just handed
1735+ // this guest the app window's zoom level.
1736+ yield* assertTabZoom(tabId);
17241737 yield* attempt({ operation: "registerWebview.sendTheme", tabId, webContentsId }, () =>
17251738 wc.send(ANNOTATION_THEME_CHANNEL, annotationTheme),
17261739 );
@@ -1749,18 +1762,13 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 22 unmodified lines
17491762 ) {
17501763 return yield* new PreviewTabNotFoundError({ tabId });
17511764 }
1752− const zoomFactor =
1753− replacedWebContentsId !== null
1754− ? yield* attempt(
1755− { operation: "registerWebview.restoreZoomFactor", tabId, webContentsId },
1756− () => {
1757− wc.setZoomFactor(currentTab.zoomFactor);
1758− return currentTab.zoomFactor;
1759− },
1760− )
1761− : yield* attempt({ operation: "registerWebview.getZoomFactor", tabId, webContentsId }, () =>
1762− wc.getZoomFactor(),
1763− );
1765+ // Always assert the tab's own zoom rather than reading the guest's: a guest
1766+ // attaching while the app UI is zoomed starts at the embedder's inherited
1767+ // zoom level, which is not the preview's zoom. Done before the guest is
1768+ // published so it never paints a frame at the inherited zoom.
1769+ yield* attempt({ operation: "registerWebview.restoreZoomFactor", tabId, webContentsId }, () =>
1770+ wc.setZoomFactor(currentTab.zoomFactor),
1771+ );
17641772 yield* attachListeners(tabId, wc);
17651773 const registeredAt = yield* currentIso;
17661774 const registration = yield* SynchronizedRef.modifyEffect(tabsRef, (tabs) =>
@@ -1784,7 +1792,6 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 17 unmodified lines
17841792 navStatus: pendingUrl === null ? computeNavStatus(wc) : current.navStatus,
17851793 canGoBack: wc.navigationHistory.canGoBack(),
17861794 canGoForward: wc.navigationHistory.canGoForward(),
1787− zoomFactor,
17881795 updatedAt: registeredAt,
17891796 };
17901797 return [
@@ -1806,6 +1813,9 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 15 unmodified lines
18061813 return yield* new PreviewTabNotFoundError({ tabId });
18071814 }
18081815 const { state: registered, pendingUrl } = registration.value;
1816+ // A zoom action that landed while this attach was in flight addressed the
1817+ // guest this one replaced, so settle the new guest on the committed factor.
1818+ yield* assertTabZoom(tabId);
18091819 runFork(restoreControlSession(tabId, wc));
18101820 yield* emit(tabId, registered);
18111821 yield* attempt({ operation: "registerWebview.sendTheme", tabId, webContentsId }, () =>
@@ -2099,6 +2109,17 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 287 unmodified lines
20992109 );
21002110 });
21012111
2112+ /**
2113+ * Chromium hands every guest `<webview>` the embedder's zoom level, so zooming
2114+ * the app UI drags the previewed page along with it. The preview browser owns
2115+ * its own zoom factor, so re-assert it on each attached guest whenever the main
2116+ * window's zoom changes (see DesktopWindow.zoomMain).
2117+ */
2118+ const reapplyZoom = Effect.fn("PreviewManager.reapplyZoom")(function* () {
2119+ const tabIds = Array.from((yield* SynchronizedRef.get(tabsRef)).keys());
2120+ yield* Effect.forEach(tabIds, assertTabZoom, { discard: true });
2121+ });
2122+
21022123 const applyZoom = Effect.fn("PreviewManager.applyZoom")(function* (
21032124 tabId: string,
21042125 transform: (current: number) => number,
@@ -3476,6 +3497,7 @@ const makeNativeOperations = Effect.fn("PreviewManager.makeOperations")(function · 1371 unmodified lines
34763497 openPictureInPicture,
34773498 openDevTools,
34783499 pickElement,
3500+ reapplyZoom,
34793501 refresh,
34803502 registerWebview,
34813503 resetZoom: (tabId: string) => applyZoom(tabId, () => DEFAULT_ZOOM_FACTOR),
@@ -3774,6 +3796,9 @@ export class PreviewManager extends Context.Service< · 292 unmodified lines
37743796 readonly zoomIn: (tabId: string) => Effect.Effect<void, PreviewManagerError>;
37753797 readonly zoomOut: (tabId: string) => Effect.Effect<void, PreviewManagerError>;
37763798 readonly resetZoom: (tabId: string) => Effect.Effect<void, PreviewManagerError>;
3799+ // Re-applies every attached guest's own zoom factor, undoing the zoom level
3800+ // Chromium inherits from the embedder when the app UI zooms.
3801+ readonly reapplyZoom: () => Effect.Effect<void>;
37773802 readonly hardReload: (tabId: string) => Effect.Effect<void, PreviewManagerError>;
37783803 readonly setColorScheme: (
37793804 tabId: string,
@@ -3874,6 +3899,7 @@ export const make = Effect.gen(function* PreviewManagerMake() { · 94 unmodified lines
38743899 zoomIn: operations.zoomIn,
38753900 zoomOut: operations.zoomOut,
38763901 resetZoom: operations.resetZoom,
3902+ reapplyZoom: operations.reapplyZoom,
38773903 hardReload: operations.hardReload,
38783904 setColorScheme: operations.setColorScheme,
38793905 openDevTools: operations.openDevTools,
apps/desktop/src/window/DesktopWindow.test.ts+47−0
@@ -61,9 +61,14 @@ const environmentInput = { · 60 unmodified lines
6161 function makeFakeBrowserWindow() {
6262 const windowListeners = new Map<string, (...args: readonly unknown[]) => void>();
6363 const webContentsListeners = new Map<string, (...args: readonly unknown[]) => void>();
64+ let zoomLevel = 0;
6465 const webContents = {
6566 copyImageAt: vi.fn(),
6667 getURL: vi.fn(() => "t3code-dev://app/"),
68+ getZoomLevel: vi.fn(() => zoomLevel),
69+ setZoomLevel: vi.fn((level: number) => {
70+ zoomLevel = level;
71+ }),
6772 isLoadingMainFrame: vi.fn(() => false),
6873 on: vi.fn((eventName: string, listener: (...args: readonly unknown[]) => void) => {
6974 webContentsListeners.set(eventName, listener);
@@ -116,6 +121,7 @@ function makeFakeBrowserWindow() { · 46 unmodified lines
116121 openDevTools: webContents.openDevTools,
117122 reload: webContents.reload,
118123 send: webContents.send,
124+ setZoomLevel: webContents.setZoomLevel,
119125 setAutoHideCursor: window.setAutoHideCursor,
120126 webContentsListeners,
121127 windowListeners,
@@ -186,6 +192,7 @@ function makeTestLayer(input: { · 64 unmodified lines
186192 bounds: DesktopAppSettings.DesktopWindowBounds,
187193 ) => Effect.Effect<void>;
188194 readonly openedExternalUrls?: unknown[];
195+ readonly previewZoomReapplies?: number[];
189196 }) {
190197 let desktopSettings = input.desktopSettings ?? DesktopAppSettings.DEFAULT_DESKTOP_SETTINGS;
191198 const desktopAppSettingsLayer = Layer.succeed(DesktopAppSettings.DesktopAppSettings, {
@@ -264,6 +271,10 @@ function makeTestLayer(input: { · 72 unmodified lines
264271 setMainWindow: () => Effect.void,
265272 isBrowserPartition: (partition) => partition.startsWith("persist:t3code-preview-"),
266273 getBrowserPartition: () => Effect.succeed("persist:t3code-preview-test"),
274+ reapplyZoom: () =>
275+ Effect.sync(() => {
276+ input.previewZoomReapplies?.push(input.window.webContents.getZoomLevel());
277+ }),
267278 }),
268279 ),
269280 ),
@@ -483,6 +494,42 @@ describe("DesktopWindow", () => { · 213 unmodified lines
483494 }),
484495 );
485496
497+ // Chromium hands the main window's zoom level down to embedded preview
498+ // guests, so every app zoom has to put the preview browser back at its own
499+ // zoom or zooming the UI drags the previewed page with it.
500+ it.effect("restores the preview browser's own zoom after zooming the app", () =>
501+ Effect.gen(function* () {
502+ const fakeWindow = makeFakeBrowserWindow();
503+ const createCount = yield* Ref.make(0);
504+ const mainWindow = yield* Ref.make<Option.Option<Electron.BrowserWindow>>(Option.none());
505+ const previewZoomReapplies: number[] = [];
506+ const layer = makeTestLayer({
507+ window: fakeWindow.window,
508+ createCount,
509+ mainWindow,
510+ previewZoomReapplies,
511+ });
512+
513+ yield* Effect.gen(function* () {
514+ const desktopWindow = yield* DesktopWindow.DesktopWindow;
515+ yield* desktopWindow.handleBackendReady(new URL("http://127.0.0.1:3773"));
516+
517+ yield* desktopWindow.zoomMain("out");
518+ yield* desktopWindow.zoomMain("out");
519+ yield* desktopWindow.zoomMain("in");
520+ yield* desktopWindow.zoomMain("reset");
521+
522+ assert.deepEqual(
523+ fakeWindow.setZoomLevel.mock.calls.map(([level]) => level),
524+ [-0.5, -1, -0.5, 0],
525+ );
526+ // Recorded after the window level moved, so the preview is put back at
527+ // its own zoom on every step rather than left on the inherited one.
528+ assert.deepEqual(previewZoomReapplies, [-0.5, -1, -0.5, 0]);
529+ }).pipe(Effect.provide(layer));
530+ }),
531+ );
532+
486533 it.effect("uses the persisted main window bounds when opening the window", () =>
487534 Effect.gen(function* () {
488535 const fakeWindow = makeFakeBrowserWindow();
apps/desktop/src/window/DesktopWindow.ts+4−0
@@ -855,6 +855,10 @@ export const make = Effect.gen(function* () { · 854 unmodified lines
855855 webContents.setZoomLevel(
856856 direction === "reset" ? 0 : webContents.getZoomLevel() + (direction === "in" ? 0.5 : -0.5),
857857 );
858+ // Chromium pushes the new level down to embedded guests, which would zoom
859+ // the previewed page along with the app UI. The preview browser keeps its
860+ // own zoom, so put each guest back where the preview left it.
861+ yield* previewManager.reapplyZoom();
858862 }),
859863 syncAppearance: Effect.gen(function* () {
860864 const shouldUseDarkColors = yield* electronTheme.shouldUseDarkColors;