Skip to content

fix(shortcuts): wire Cmd/Ctrl +, - and 0 to webview zoom - #193

Open
alexwbend wants to merge 1 commit into
solardev-xyz:mainfrom
alexwbend:fix/zoom-keyboard-shortcuts
Open

fix(shortcuts): wire Cmd/Ctrl +, - and 0 to webview zoom#193
alexwbend wants to merge 1 commit into
solardev-xyz:mainfrom
alexwbend:fix/zoom-keyboard-shortcuts

Conversation

@alexwbend

Copy link
Copy Markdown

What this does

Wires Cmd/Ctrl+=, Cmd/Ctrl+- and Cmd/Ctrl+0 to the active webview's zoom, closing #88. The bindings go into the shared shortcut registry, so they get a View menu group, a renderer keydown fallback, and a remappable row in Settings > Shortcuts, exactly like every other binding in the app.

All three entry points (the hamburger - / + buttons, the new menu accelerators, and the keydown fallback) now run through one applyZoomFactor helper in menus.js, so the 100% readout can no longer drift from the webview's real zoom factor.

Why

README.md lines 308 to 310 document these three shortcuts, but nothing implemented them. Before this change the only way to zoom was opening the hamburger menu and clicking the - / + buttons.

I confirmed the gap rather than assuming it:

  • No globalShortcut, no before-input-event, and no keydown branch for =, - or 0 anywhere in src/.
  • Building the real application menu template for darwin, win32 and linux produces no zoom items and no zoom roles.
  • Electron's own docs state it directly: "Cmd+/-/0 zoom shortcuts are controlled by the 'zoomIn', 'zoomOut', and 'resetZoom' MenuItem roles in the application Menu. To disable shortcuts, manually define the Menu and omit zoom roles from the definition." Freedom defines its menu manually in menu.js and omits those roles, so the shortcuts were switched off.

Why not just add Electron's zoom roles

Adding { role: 'zoomIn' } and friends would be a three line change, but it would be wrong here for three reasons:

  1. The roles step zoomLevel on the focused webContents. Freedom's pages live in a <webview> guest while the chrome is the host renderer, so with focus in the address bar the roles would scale the browser UI rather than the page.
  2. zoomLevel is logarithmic and steps by 0.5, while the hamburger buttons step zoomFactor linearly by 0.1 within [0.25, 5]. Two entry points would move by different amounts and the zoom-level readout would not track the role driven path at all.
  3. A role carries an implicit accelerator that bypasses src/shared/shortcuts.js, so it would not appear in Settings > Shortcuts and could not be remapped. That is the same class of collision that menu.test.js already guards against for explicit accelerators.

Changes

File Change
src/shared/shortcuts.js Three page.zoom* entries, category Page, context: 'both'
src/renderer/lib/shortcuts.js Same entries in the ESM mirror
src/main/menu.js View menu zoom group, placed above Full Screen, sending page:zoom-*
src/main/preload.js onZoomIn / onZoomOut / onZoomReset bridges
src/renderer/lib/menus.js Shared applyZoomFactor path plus the IPC and keydown wiring
src/main/menu.test.js, src/renderer/lib/menus.test.js Unit coverage
test-e2e/zoom.spec.js New harness E2E spec

How to test

Manually:

  1. npm start, load any page.
  2. Press Cmd/Ctrl+= a few times, then Cmd/Ctrl+-, then Cmd/Ctrl+0. The page zooms in 10% steps and resets to 100%.
  3. Open the hamburger menu. The readout matches what the keyboard did.
  4. Open Settings > Shortcuts > Page. Zoom In, Zoom Out and Actual Size are listed and remappable, and a remap takes effect without a restart.

Automated:

npm run lint
npm test
npx playwright test --project=harness test-e2e/zoom.spec.js

Verification I ran

  • npm test: 158 suites, 3070 tests passing.
  • npm run lint: clean.
  • npx playwright test --project=harness test-e2e/zoom.spec.js: 4 passing, repeated three times for stability.
  • Full harness E2E suite twice: 62 passing each time.
  • Reverted the source changes while keeping the tests, and confirmed all four new unit tests and all four new E2E tests fail. They are genuinely red before the fix and green after.
  • Checked in a real Electron 43 process that CmdOrCtrl+=, CmdOrCtrl+- and CmdOrCtrl+0 are accepted by the accelerator parser and survive a Menu round trip. A deliberately bogus accelerator throws, so the check is meaningful.
  • Swept the whole registry on all three platforms: the new bindings are valid, unreserved, and collide with nothing.

Notes

  • Two pre-existing failures showed up in the full harness suite and are unrelated to this change. settings-adblock.spec.js fails on a clean checkout too because the adblock lists are not provisioned locally, which CI does since e368a40. One other spec (tez-unverified.spec.js in the first run, permissions.spec.js in the second) flaked differently each run and passes in isolation on both a clean tree and this branch, which matches the E2E flakiness tracked in flaky: e2e-address-bar-clipboard times out on Electron launch (required check, blocks merges) #180.
  • src/main/preload.js, src/shared/shortcuts.js and src/renderer/lib/menus.test.js are already reported by npm run format:check on main. The lines this PR adds are Prettier clean, and I deliberately did not run --write on those files so the diff stays scoped.
  • I kept the channel names as literal strings to match the adjacent page:reload and page:hard-reload wiring. page:* channels are not in src/shared/ipc-channels.js, and preload.js documents that it cannot require that module from its sandboxed context. Happy to move them if you would rather centralise.
  • No numpad or Ctrl+Shift+= aliases. Each alias needs its own menu item in the current design, which would put visible duplicate rows in the View menu, so I left it at the three bindings the README promises. Users who want the numpad variants can remap in Settings.
  • While confirming this, I noticed Cmd+P / Ctrl+P on README line 311 is unwired in the same way: no menu item, no accelerator, no keydown handler, only the hamburger Print button. Happy to open a separate issue for it.
  • One thing worth knowing for future menu driven E2E work: the window fixture waits for the address bar, but that element is static markup in index.html and is present before initMenus() and initTabs() run. test-e2e/zoom.spec.js therefore probes with Actual Size, which is idempotent, until the main to renderer path is demonstrably live before asserting anything.

Closes #88

🤖 Generated with Claude Code

README documents Cmd/Ctrl+=, Cmd/Ctrl+- and Cmd/Ctrl+0 as zoom
shortcuts, but nothing implemented them: zoom existed only as click
handlers on the hamburger menu's - / + buttons. Electron's docs state
that these shortcuts come from the zoomIn/zoomOut/resetZoom MenuItem
roles, and menu.js defines the menu manually without them, so the
keystrokes never reached the app.

Add the three bindings to the shared shortcut registry so they gain a
View menu group, a renderer keydown fallback for the Linux frameless
setups where menu accelerators never arrive, and a remappable row in
Settings > Shortcuts.

The Electron roles are deliberately not used: they step zoomLevel on
the focused webContents (the chrome, when the address bar has focus)
rather than the active <webview>, they step by a different amount than
the hamburger buttons, and their implicit accelerators would bypass the
registry and could not be remapped.

All three entry points now share one applyZoomFactor path so the zoom
readout cannot drift from the webview's real factor.

Closes solardev-xyz#88

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

Cmd/Ctrl + and - keyboard shortcuts don't zoom the webview

1 participant