diff --git a/electron/applicationMenu.js b/electron/applicationMenu.js new file mode 100644 index 00000000..6673c584 --- /dev/null +++ b/electron/applicationMenu.js @@ -0,0 +1,81 @@ +// Cmd+C works in every window, not just the first (ENG-289). +// +// The app never called setApplicationMenu, so macOS was left with Electron's implicit default and +// the Edit accelerators were whatever that happened to bind. The menu is process-global while its +// items act on whichever webContents holds focus, so "which window" is decided at keypress time by +// role dispatch, not by whatever window the menu was built next to. Declaring the menu ourselves +// makes that dispatch explicit and identical in the first window and the tenth. +// +// ROLES ONLY, deliberately. A hand-written copy handler has to answer "copy from where", and the +// answer is a focused-webContents lookup that is wrong for a guest exactly when the user +// is inside one. Chromium already resolves roles against the focused contents, including guests, +// so the correct implementation is the one where we write no logic at all. +// +// macOS only: on Windows and Linux setting a menu paints a visible menu bar this app has never had. + +const APP_NAME = 'OpenSwarm'; + +/** The template, separated from the Electron call so it can be asserted without an app instance. */ +function applicationMenuTemplate() { + return [ + { + label: APP_NAME, + submenu: [ + { role: 'about' }, + { type: 'separator' }, + { role: 'services' }, + { type: 'separator' }, + { role: 'hide' }, + { role: 'hideOthers' }, + { role: 'unhide' }, + { type: 'separator' }, + { role: 'quit' }, + ], + }, + { + label: 'Edit', + submenu: [ + { role: 'undo' }, + { role: 'redo' }, + { type: 'separator' }, + { role: 'cut' }, + { role: 'copy' }, + { role: 'paste' }, + { role: 'pasteAndMatchStyle' }, + { role: 'delete' }, + { role: 'selectAll' }, + ], + }, + { + label: 'View', + submenu: [ + { role: 'reload' }, + { role: 'forceReload' }, + { type: 'separator' }, + { role: 'resetZoom' }, + { role: 'zoomIn' }, + { role: 'zoomOut' }, + { type: 'separator' }, + { role: 'togglefullscreen' }, + ], + }, + { + label: 'Window', + submenu: [ + { role: 'minimize' }, + { role: 'zoom' }, + { type: 'separator' }, + { role: 'front' }, + ], + }, + ]; +} + +function installApplicationMenu(electron) { + if (process.platform !== 'darwin') return false; + const { Menu } = electron; + Menu.setApplicationMenu(Menu.buildFromTemplate(applicationMenuTemplate())); + return true; +} + +module.exports = { applicationMenuTemplate, installApplicationMenu, APP_NAME }; diff --git a/electron/applicationMenu.test.js b/electron/applicationMenu.test.js new file mode 100644 index 00000000..8023e257 --- /dev/null +++ b/electron/applicationMenu.test.js @@ -0,0 +1,65 @@ +// Run: node --test electron/applicationMenu.test.js +// +// ENG-289: "open a second window, select text, Cmd+C, nothing is copied; the same steps in the +// first window work." The app never declared an application menu, so the Edit accelerators were +// whatever Electron's implicit default bound. The menu is process-global and its items act on the +// FOCUSED webContents, so declaring it ourselves is what makes the second window behave like the +// first. +// +// These assert the two properties that decide whether the bug can come back: the Edit accelerators +// exist at all, and they are ROLES (Chromium resolves those against the focused contents, including +// a guest) rather than hand-written handlers that have to guess at a target. +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { applicationMenuTemplate, installApplicationMenu } = require('./applicationMenu'); + +function editSubmenu() { + const edit = applicationMenuTemplate().find((m) => m.label === 'Edit'); + assert.ok(edit, 'there must be an Edit menu; it is what binds Cmd+C on macOS'); + return edit.submenu; +} + +test('the clipboard accelerators are all present', () => { + const roles = editSubmenu().map((i) => i.role).filter(Boolean); + for (const needed of ['copy', 'cut', 'paste', 'selectAll', 'undo', 'redo']) { + assert.ok(roles.includes(needed), `Edit menu is missing the ${needed} role`); + } +}); + +test('clipboard items are roles, never hand-written click handlers', () => { + // A click handler would have to resolve "copy from where" itself, and that lookup is wrong for a + // webview guest exactly when the user is typing inside one. + for (const item of editSubmenu()) { + if (item.type === 'separator') continue; + assert.ok(item.role, `Edit item ${JSON.stringify(item)} must be a role`); + assert.equal(item.click, undefined, `Edit item ${item.role} must not carry a click handler`); + } +}); + +test('no item pins itself to a specific window', () => { + const json = JSON.stringify(applicationMenuTemplate()); + assert.ok(!json.includes('mainWindow'), 'a menu bound to one window is exactly the reported bug'); + assert.ok(!json.includes('webContents'), 'roles resolve the target themselves; do not capture one'); +}); + +test('the menu is installed on macOS and skipped elsewhere', () => { + const calls = []; + const fake = { Menu: { setApplicationMenu: (m) => calls.push(m), buildFromTemplate: (t) => t } }; + const real = process.platform; + try { + Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true }); + assert.equal(installApplicationMenu(fake), true); + assert.equal(calls.length, 1, 'macOS must get an explicit menu'); + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + assert.equal(installApplicationMenu(fake), false, 'Windows would paint a menu bar this app never had'); + assert.equal(calls.length, 1); + } finally { + Object.defineProperty(process, 'platform', { value: real, configurable: true }); + } +}); + +test('main.js actually installs it', () => { + const src = require('fs').readFileSync(require('path').join(__dirname, 'main.js'), 'utf8'); + assert.match(src, /installApplicationMenu/, 'a menu module nothing calls fixes nothing'); +}); diff --git a/electron/main.js b/electron/main.js index 16495c53..8e95e706 100644 --- a/electron/main.js +++ b/electron/main.js @@ -2267,6 +2267,9 @@ ipcMain.handle('haptic:perform', (event, pattern) => { }); app.whenReady().then(async () => { + // Declare the Edit accelerators instead of inheriting whatever the implicit default bound, so + // Cmd+C acts on the focused window rather than only the first one (ENG-289). macOS only. + try { require('./applicationMenu').installApplicationMenu(require('electron')); } catch (_) {} // We made it here, so any prior update swap finished. Drop a stale updating.lock // (the watchdog never deletes it) so a real crash later isn't silently swallowed. try { fs.unlinkSync(CRASH_WATCHDOG_UPDATING_LOCK); } catch (_) {}