diff --git a/src/main.js b/src/main.js
index 639eed8..b1701ae 100644
--- a/src/main.js
+++ b/src/main.js
@@ -36,7 +36,8 @@ const { getClientId: getGithubClientId, requestDeviceCode, pollForToken, fetchVi
const { openPullRequest, buildPullRequestBody, testMode: githubTestMode } = require('./github-pr.cjs');
const { buildPullRequestEntries } = require('./pr-files.cjs');
const { openAndScrape, fetchAttachment } = require('./trac-view');
-const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url');
+const { openExternalUrl, describeRefusedUrl, ALLOWED_URL_SCHEMES } = require('./external-url');
+const { openLinksExternally } = require('./window-links');
const { deleteRegisteredSite, revealRegisteredSite, clearRegisteredSiteLog } = require('./site-registry');
const { removeTree } = require('./remove-tree');
const { createSetupTracker } = require('./setup-tracker');
@@ -55,6 +56,15 @@ const {
const { createProgressThrottle, describeSwitchProgress } = require('./switch-progress.cjs');
const { getStore } = require('./settings-store');
+// How an address leaves this app, shared by the renderer's `url:open` and the
+// link handling in window-links.js. A refusal is logged rather than dropped, so
+// a caller that trips the guard shows up in the log instead of doing nothing.
+const externalUrlDeps = {
+ openExternal: (target) => shell.openExternal(target),
+ onRefused: (description) => logEvent('url', `refused to open ${description} — only ${ALLOWED_URL_SCHEMES.join(', ')} are allowed`),
+ onFailed: (url, error) => logEvent('url', `could not open ${describeRefusedUrl(url)}: ${error && error.message}`)
+};
+
// One name for the send-only progress channel (#173), shared with preload.js
// through the tests rather than by import — the renderer bundle and the main
// process do not share a module graph, and a rename that only lands on one side
@@ -405,6 +415,10 @@ function createWindow() {
}
});
+ // Links belong in the contributor's browser, not in a window of this app
+ // (#284). Set before the page loads so the first click is covered too.
+ openLinksExternally(mainWindow.webContents, externalUrlDeps);
+
mainWindow.loadFile(path.join(__dirname, 'renderer', 'index.html'));
}
function buildPatchHtml(content) {
@@ -2191,12 +2205,8 @@ ipcMain.handle('branches:delete', async (_e, sitePath, targetRef) => withRegiste
}));
// Only the schemes the app actually uses reach the OS — see external-url.js for
-// why. A refusal is logged rather than dropped so a future caller that trips the
-// guard shows up in the log file instead of just doing nothing.
-ipcMain.handle('url:open', async (_e, url) => openExternalUrl(url, {
- openExternal: (target) => shell.openExternal(target),
- onRefused: (description) => logEvent('url', `refused to open ${description} — only ${ALLOWED_URL_SCHEMES.join(', ')} are allowed`)
-}));
+// why.
+ipcMain.handle('url:open', async (_e, url) => openExternalUrl(url, externalUrlDeps));
// --- opening a site's code -----------------------------------------------
//
diff --git a/src/window-links.js b/src/window-links.js
new file mode 100644
index 0000000..b163e23
--- /dev/null
+++ b/src/window-links.js
@@ -0,0 +1,55 @@
+// Keeps links in the app window opening in the contributor's browser (#284).
+//
+// Each link cancels its own navigation in its onClick handler. That covers a
+// plain click, but a middle click fires `auxclick`, which a click handler never
+// sees — so Chromium's "open in a new window" default ran and the site loaded
+// inside a bare app window with no address bar. Cmd/Ctrl+click was fine, since
+// that does arrive as a click the handler can cancel.
+//
+// Refusing once for the whole window fixes it for every link, including ones
+// added later. The address goes out through external-url.js, the same gate the
+// renderer's own openExternal calls use.
+
+const { isAllowedExternalUrl, openExternalUrl } = require('./external-url');
+
+/**
+ * Keeps a window on its own page and sends any link it opens to the browser.
+ *
+ * @param {import('electron').WebContents} wc
+ * @param {Object} [deps]
+ * @param {Function} [deps.openExternal] `shell.openExternal` in the app, a stub in tests.
+ * @param {Function} [deps.onRefused] Called with a description of a refused address.
+ * @param {Function} [deps.onFailed] Called with the address and error when opening fails.
+ */
+function openLinksExternally(wc, { openExternal, onRefused, onFailed } = {}) {
+ // These events are synchronous and ignore what the handler returns, so the
+ // hand-off cannot be awaited. The failure is reported rather than dropped:
+ // openExternal rejects when the OS has no handler for the address, and from
+ // the contributor's chair that is a link that did nothing.
+ const handOff = (url) => {
+ Promise.resolve(openExternalUrl(url, { openExternal, onRefused })).catch((error) => {
+ if (typeof onFailed === 'function') onFailed(url, error);
+ });
+ };
+
+ // Middle click, Cmd/Ctrl+click, target="_blank", window.open. A child window
+ // is never this app's UI, so it is denied whatever the address is.
+ wc.setWindowOpenHandler(({ url }) => {
+ handOff(url);
+ return { action: 'deny' };
+ });
+
+ // A click no handler cancelled, or a script navigation. Only http/https is
+ // taken over: the app's own page is a file: URL and has to stay loadable.
+ const sendToBrowser = (event, url) => {
+ if (!isAllowedExternalUrl(url)) return;
+ event.preventDefault();
+ handOff(url);
+ };
+ // will-navigate is the click. will-redirect is the 3xx or
+ // that does not fire it, and would otherwise move the window.
+ wc.on('will-navigate', sendToBrowser);
+ wc.on('will-redirect', sendToBrowser);
+}
+
+module.exports = { openLinksExternally };
diff --git a/tests/unit/window-links.test.cjs b/tests/unit/window-links.test.cjs
new file mode 100644
index 0000000..1a30f3c
--- /dev/null
+++ b/tests/unit/window-links.test.cjs
@@ -0,0 +1,130 @@
+const test = require('node:test');
+const assert = require('node:assert/strict');
+
+const { openLinksExternally } = require('../../src/window-links.js');
+
+// Stands in for a window's webContents, so the gestures can be replayed without
+// an Electron process - the same approach external-url.test.cjs takes with the
+// shell.
+function fakeWebContents() {
+ const listeners = {};
+ return {
+ windowOpenHandler: null,
+ setWindowOpenHandler(handler) { this.windowOpenHandler = handler; },
+ on(event, listener) { (listeners[event] ||= []).push(listener); },
+ // Replays a gesture; returns whether the navigation was cancelled.
+ emit(event, url) {
+ let prevented = false;
+ const fakeEvent = { preventDefault() { prevented = true; } };
+ for (const listener of listeners[event] || []) listener(fakeEvent, url);
+ return prevented;
+ }
+ };
+}
+
+function recorder() {
+ const opened = [];
+ const refused = [];
+ return {
+ opened,
+ refused,
+ deps: {
+ openExternal: async (url) => { opened.push(url); },
+ onRefused: (description) => { refused.push(description); }
+ }
+ };
+}
+
+// The hand-off is a promise the synchronous handlers cannot await.
+const settled = () => new Promise((resolve) => setImmediate(resolve));
+
+test('a middle click opens the browser instead of a window (#284)', async () => {
+ const wc = fakeWebContents();
+ const rec = recorder();
+ openLinksExternally(wc, rec.deps);
+
+ const result = wc.windowOpenHandler({ url: 'http://127.0.0.1:39372/wp-admin/' });
+ await settled();
+
+ assert.deepEqual(result, { action: 'deny' });
+ assert.deepEqual(rec.opened, ['http://127.0.0.1:39372/wp-admin/']);
+});
+
+test('a click no handler cancelled still opens the browser', async () => {
+ const wc = fakeWebContents();
+ const rec = recorder();
+ openLinksExternally(wc, rec.deps);
+
+ assert.equal(wc.emit('will-navigate', 'https://core.trac.wordpress.org/ticket/284'), true);
+ await settled();
+
+ assert.deepEqual(rec.opened, ['https://core.trac.wordpress.org/ticket/284']);
+});
+
+test('a redirect cannot move the app window either', async () => {
+ // will-redirect covers the 3xx and that never fire
+ // will-navigate - a login redirect on the site would land here.
+ const wc = fakeWebContents();
+ const rec = recorder();
+ openLinksExternally(wc, rec.deps);
+
+ assert.equal(wc.emit('will-redirect', 'https://wordpress.org/'), true);
+ await settled();
+
+ assert.deepEqual(rec.opened, ['https://wordpress.org/']);
+});
+
+test("the app's own page is left alone", async () => {
+ // Cancelling this would stop the app window reloading, and a file: address is
+ // not one to hand to the browser.
+ const wc = fakeWebContents();
+ const rec = recorder();
+ openLinksExternally(wc, rec.deps);
+
+ assert.equal(wc.emit('will-navigate', 'file:///Applications/toolkit/renderer/index.html'), false);
+ await settled();
+
+ assert.deepEqual(rec.opened, []);
+ assert.deepEqual(rec.refused, []);
+});
+
+test('a scheme the app does not open is refused, and opens no window', async () => {
+ const wc = fakeWebContents();
+ const rec = recorder();
+ openLinksExternally(wc, rec.deps);
+
+ assert.deepEqual(wc.windowOpenHandler({ url: 'file:///etc/passwd' }), { action: 'deny' });
+ await settled();
+
+ assert.deepEqual(rec.opened, []);
+ assert.equal(rec.refused.length, 1);
+});
+
+test('a failed open is reported, not swallowed', async () => {
+ // shell.openExternal rejects when the OS has nothing registered. Nothing else
+ // is in a position to catch it, and an unreported failure is a link that did
+ // nothing.
+ const wc = fakeWebContents();
+ const failures = [];
+ openLinksExternally(wc, {
+ openExternal: async () => { throw new Error('no handler'); },
+ onFailed: (url, error) => { failures.push([url, error.message]); }
+ });
+
+ wc.emit('will-navigate', 'https://example.com/');
+ wc.windowOpenHandler({ url: 'https://example.com/' });
+ await settled();
+
+ assert.deepEqual(failures, [
+ ['https://example.com/', 'no handler'],
+ ['https://example.com/', 'no handler']
+ ]);
+});
+
+test('a failure with no reporter still does not crash the app', async () => {
+ const wc = fakeWebContents();
+ openLinksExternally(wc, { openExternal: async () => { throw new Error('no handler'); } });
+
+ wc.emit('will-navigate', 'https://example.com/');
+ await settled();
+});