Fix browser shortcut review regressions

This commit is contained in:
Mohamed Boudra
2026-07-14 14:53:19 +00:00
parent 6aa0daaf22
commit 4165841030
10 changed files with 218 additions and 99 deletions

View File

@@ -17,9 +17,10 @@ It validates the compositor behavior that unit tests cannot see:
- the real-Electron host-composer sentinel proves guest Enter cannot submit a focused
host composer;
- the automation group loads the compiled production keyboard boundary and guest
preload, then proves that a page window handler gets first refusal, unhandled
shortcuts cross from both ordinary and editable page targets, digit wildcard
shortcuts cross, and background automation stays in the guest.
preload, then proves that page window handlers get first refusal even when they
register after preload initialization, unhandled shortcuts cross from both ordinary
and editable page targets, digit wildcard shortcuts cross, and background automation
stays in the guest.
Run it with the repo Electron:

View File

@@ -15,7 +15,7 @@ import {
} from "@/keyboard/keyboard-shortcuts";
import { resolveKeyboardFocusScope } from "@/keyboard/focus-scope";
import {
buildBrowserShortcutPolicy,
buildBrowserKeyboardPolicy,
parseBrowserShortcutInput,
shouldPublishBrowserShortcutPolicy,
} from "@/keyboard/browser-shortcuts";
@@ -68,16 +68,16 @@ export function useKeyboardShortcuts({
const publishBrowserShortcutPolicy = useCallback(
(chordState?: ChordState) => {
const prefixes =
const policy =
enabled && !isMobile
? buildBrowserShortcutPolicy({
? buildBrowserKeyboardPolicy({
bindings,
chordState,
isMac,
isDesktop: isDesktopApp,
})
: [];
void getDesktopHost()?.browser?.setShortcutPolicy?.({ prefixes });
: { menuPrefixes: [], prefixes: [] };
void getDesktopHost()?.browser?.setShortcutPolicy?.(policy);
},
[bindings, enabled, isDesktopApp, isMac, isMobile],
);
@@ -348,29 +348,6 @@ export function useKeyboardShortcuts({
});
})
: null;
const browserReservedShortcutSubscription = isElectronRuntime()
? getDesktopHost()?.events?.on?.("browser-shortcut", (payload) => {
if (typeof payload !== "object" || payload === null || Array.isArray(payload)) {
return;
}
if (!("action" in payload) || payload.action !== "new-tab") {
return;
}
if (
!("browserId" in payload) ||
typeof payload.browserId !== "string" ||
payload.browserId.length === 0
) {
return;
}
routeAndPerformShortcut({
action: "workspace.tab.new",
payload: null,
domEvent: null,
});
})
: null;
return () => {
if (chordStateRef.current.timeoutId !== null) {
clearTimeout(chordStateRef.current.timeoutId);
@@ -389,11 +366,6 @@ export function useKeyboardShortcuts({
} else {
void browserShortcutSubscription?.then((dispose) => dispose());
}
if (typeof browserReservedShortcutSubscription === "function") {
browserReservedShortcutSubscription();
} else {
void browserReservedShortcutSubscription?.then((dispose) => dispose());
}
};
}, [
bindings,

View File

@@ -1,21 +1,21 @@
import { describe, expect, it } from "vitest";
import {
buildBrowserShortcutPolicy,
buildBrowserKeyboardPolicy,
parseBrowserShortcutInput,
shouldPublishBrowserShortcutPolicy,
} from "./browser-shortcuts";
import { buildEffectiveBindings, resolveKeyboardShortcut } from "./keyboard-shortcuts";
describe("buildBrowserShortcutPolicy", () => {
describe("buildBrowserKeyboardPolicy", () => {
it("publishes only chord starts while no browser chord is pending", () => {
const bindings = buildEffectiveBindings({
"workspace-tab-new-ctrl-t-non-mac": "Ctrl+Y",
"workspace-terminal-new-ctrl-shift-t-non-mac": "Ctrl+F12 Ctrl+F11",
});
const policy = buildBrowserShortcutPolicy({ bindings, isMac: false, isDesktop: true });
const policy = buildBrowserKeyboardPolicy({ bindings, isMac: false, isDesktop: true });
expect(policy).toContainEqual({
expect(policy.prefixes).toContainEqual({
alt: false,
code: "KeyY",
control: true,
@@ -23,14 +23,14 @@ describe("buildBrowserShortcutPolicy", () => {
meta: false,
shift: false,
});
expect(policy).toContainEqual({
expect(policy.prefixes).toContainEqual({
alt: false,
code: "F12",
control: true,
meta: false,
shift: false,
});
expect(policy).not.toContainEqual({
expect(policy.prefixes).not.toContainEqual({
alt: false,
code: "F11",
control: true,
@@ -47,14 +47,14 @@ describe("buildBrowserShortcutPolicy", () => {
(binding) => binding.id === "workspace-terminal-new-ctrl-shift-t-non-mac",
);
const policy = buildBrowserShortcutPolicy({
const policy = buildBrowserKeyboardPolicy({
bindings,
chordState: { candidateIndices: [chordIndex], step: 1, timeoutId: null },
isMac: false,
isDesktop: true,
});
expect(policy).toEqual([
expect(policy.prefixes).toEqual([
{
alt: false,
code: "F11",
@@ -63,6 +63,14 @@ describe("buildBrowserShortcutPolicy", () => {
shift: false,
},
]);
expect(policy.menuPrefixes).toContainEqual({
alt: false,
code: "KeyW",
control: true,
key: "w",
meta: false,
shift: false,
});
const result = resolveKeyboardShortcut({
event: {
@@ -93,16 +101,16 @@ describe("buildBrowserShortcutPolicy", () => {
"settings-toggle-ctrl-comma-non-mac": "Ctrl+F10 F9",
});
const policy = buildBrowserShortcutPolicy({ bindings, isMac: false, isDesktop: true });
const policy = buildBrowserKeyboardPolicy({ bindings, isMac: false, isDesktop: true });
expect(policy).not.toContainEqual({
expect(policy.prefixes).not.toContainEqual({
alt: false,
code: "F10",
control: true,
meta: false,
shift: false,
});
expect(policy).not.toContainEqual({
expect(policy.prefixes).not.toContainEqual({
alt: false,
code: "F9",
control: false,
@@ -116,7 +124,9 @@ describe("buildBrowserShortcutPolicy", () => {
"workspace-tab-new-cmd-t-mac": "Mod+Y",
});
expect(buildBrowserShortcutPolicy({ bindings, isMac: true, isDesktop: true })).toContainEqual({
expect(
buildBrowserKeyboardPolicy({ bindings, isMac: true, isDesktop: true }).prefixes,
).toContainEqual({
alt: false,
code: "KeyY",
control: false,
@@ -128,16 +138,16 @@ describe("buildBrowserShortcutPolicy", () => {
it("does not publish plain browser keys", () => {
const bindings = buildEffectiveBindings({});
const policy = buildBrowserShortcutPolicy({ bindings, isMac: false, isDesktop: true });
const policy = buildBrowserKeyboardPolicy({ bindings, isMac: false, isDesktop: true });
expect(policy).not.toContainEqual({
expect(policy.prefixes).not.toContainEqual({
alt: false,
code: "Enter",
control: false,
meta: false,
shift: false,
});
expect(policy).not.toContainEqual({
expect(policy.prefixes).not.toContainEqual({
alt: false,
code: "Slash",
control: false,
@@ -148,9 +158,9 @@ describe("buildBrowserShortcutPolicy", () => {
it("publishes Cmd+B with its logical key for non-QWERTY layouts", () => {
const bindings = buildEffectiveBindings({});
const policy = buildBrowserShortcutPolicy({ bindings, isMac: true, isDesktop: true });
const policy = buildBrowserKeyboardPolicy({ bindings, isMac: true, isDesktop: true });
expect(policy).toContainEqual({
expect(policy.prefixes).toContainEqual({
alt: false,
code: "KeyB",
control: false,
@@ -162,9 +172,9 @@ describe("buildBrowserShortcutPolicy", () => {
it("publishes the physical code needed for macOS Option shortcuts", () => {
const bindings = buildEffectiveBindings({});
const policy = buildBrowserShortcutPolicy({ bindings, isMac: true, isDesktop: true });
const policy = buildBrowserKeyboardPolicy({ bindings, isMac: true, isDesktop: true });
expect(policy).toContainEqual({
expect(policy.prefixes).toContainEqual({
alt: true,
code: "KeyT",
control: false,

View File

@@ -22,6 +22,11 @@ export interface BrowserShortcutInput extends KeyboardShortcutInput {
browserId: string;
}
export interface BrowserKeyboardPolicy {
menuPrefixes: BrowserShortcutPrefix[];
prefixes: BrowserShortcutPrefix[];
}
interface BrowserShortcutPolicyInput {
bindings: readonly ParsedShortcutBinding[];
chordState?: ChordState;
@@ -117,9 +122,7 @@ function prefixKey(prefix: BrowserShortcutPrefix): string {
].join(":");
}
export function buildBrowserShortcutPolicy(
input: BrowserShortcutPolicyInput,
): BrowserShortcutPrefix[] {
function buildBrowserShortcutPrefixes(input: BrowserShortcutPolicyInput): BrowserShortcutPrefix[] {
const prefixes = new Map<string, BrowserShortcutPrefix>();
const context = {
isMac: input.isMac,
@@ -158,3 +161,14 @@ export function buildBrowserShortcutPolicy(
return [...prefixes.values()];
}
export function buildBrowserKeyboardPolicy(
input: BrowserShortcutPolicyInput,
): BrowserKeyboardPolicy {
const menuPrefixes = buildBrowserShortcutPrefixes({ ...input, chordState: undefined });
const prefixes =
input.chordState && input.chordState.step > 0
? buildBrowserShortcutPrefixes(input)
: menuPrefixes;
return { menuPrefixes, prefixes };
}

View File

@@ -1774,6 +1774,46 @@ function installBrowserKeyboardSentinels() {
};
}
async function verifyLateBrowserShortcutFirstRefusal({ guest, sentinel }) {
const { shortcutInputs } = sentinel;
await guest.executeJavaScript(
`window.addEventListener("keydown", function preventLateBrowserShortcut(event) {
if (
(event.metaKey || event.ctrlKey) &&
!event.altKey &&
!event.shiftKey &&
event.key.toLowerCase() === "b"
) {
window.removeEventListener("keydown", preventLateBrowserShortcut);
event.preventDefault();
window.fixtureLog.push({
event: "late-shortcut-b",
defaultPrevented: event.defaultPrevented,
trusted: event.isTrusted,
});
}
});`,
true,
);
automationBrowserShortcut(guest);
await waitForAutomationLog(
guest,
(entry) =>
entry.event === "late-shortcut-b" &&
entry.defaultPrevented === true &&
entry.trusted === true,
"late page-prevented trusted browser shortcut",
);
await delay(100);
if (shortcutInputs.length !== 0 || sentinel.applicationMenuShortcutHits !== 0) {
fail(
`late page-prevented browser shortcut escaped guest: inputs=${JSON.stringify(shortcutInputs)} menu=${sentinel.applicationMenuShortcutHits}`,
);
}
pass("automation late page preventDefault keeps browser shortcut in the guest");
return { group: "automation", check: "browser-shortcut-late-page-prevented", pass: true };
}
async function verifyBrowserKeyboardIsolation({ guest, win, browserId, usesMeta, sentinel }) {
const checks = [];
const { shortcutInputs } = sentinel;
@@ -1812,8 +1852,11 @@ async function verifyBrowserKeyboardIsolation({ guest, win, browserId, usesMeta,
pass("automation page preventDefault keeps browser shortcut in the guest");
checks.push({ group: "automation", check: "browser-shortcut-page-prevented", pass: true });
await guest.executeJavaScript("window.preventBrowserShortcut = false", true);
checks.push(await verifyLateBrowserShortcutFirstRefusal({ guest, sentinel }));
const unhandledTarget = await guest.executeJavaScript(
"window.preventBrowserShortcut = false; document.getElementById('save').focus(); document.activeElement.id",
"document.getElementById('save').focus(); document.activeElement.id",
true,
);
if (unhandledTarget !== "save") {
@@ -2011,6 +2054,17 @@ async function runAutomationGroup() {
});
browserKeyboard.attach({ contents: guest, hostContents: win.webContents });
browserKeyboard.publish(win.webContents.id, {
menuPrefixes: [
{
alt: false,
code: "KeyB",
control: !usesMeta,
key: "b",
meta: usesMeta,
repeat: false,
shift: false,
},
],
prefixes: [
{
alt: false,

View File

@@ -53,18 +53,23 @@ function installKeydownListener(): void {
if (!event.isTrusted || event.defaultPrevented || !browserId || !matchesPolicy(event)) {
return;
}
event.preventDefault();
event.stopPropagation();
ipcRenderer.send(SHORTCUT_INPUT_CHANNEL, {
alt: event.altKey,
browserId,
code: event.code,
control: event.ctrlKey,
key: event.key,
meta: event.metaKey,
repeat: event.repeat,
shift: event.shiftKey,
});
const shortcutBrowserId = browserId;
setTimeout(() => {
if (event.defaultPrevented) {
return;
}
event.preventDefault();
ipcRenderer.send(SHORTCUT_INPUT_CHANNEL, {
alt: event.altKey,
browserId: shortcutBrowserId,
code: event.code,
control: event.ctrlKey,
key: event.key,
meta: event.metaKey,
repeat: event.repeat,
shift: event.shiftKey,
});
}, 0);
});
}

View File

@@ -193,6 +193,16 @@ describe("BrowserKeyboard", () => {
const guest = new FakeBrowserContents(61);
const host = new FakeBrowserContents(62);
const initialPolicy = {
menuPrefixes: [
{
alt: false,
code: "KeyB",
control: true,
meta: false,
repeat: false as const,
shift: false,
},
],
prefixes: [
{
alt: false,
@@ -204,7 +214,7 @@ describe("BrowserKeyboard", () => {
},
],
};
const latestPolicy = { prefixes: [] };
const latestPolicy = { menuPrefixes: [], prefixes: [] };
keyboard.publish(host.id, initialPolicy);
attach({ browserId: "browser-a", contents: guest, hostContents: host });
keyboard.publish(host.id, latestPolicy);
@@ -235,7 +245,7 @@ describe("BrowserKeyboard", () => {
const guest = new FakeBrowserContents(71);
const host = new FakeBrowserContents(72);
attach({ browserId: "browser-a", contents: guest, hostContents: host });
keyboard.publish(host.id, { prefixes: [] });
keyboard.publish(host.id, { menuPrefixes: [], prefixes: [] });
host.destroy();
guest.domReady();
@@ -243,28 +253,32 @@ describe("BrowserKeyboard", () => {
expect(guest.sent).toEqual([
{
channel: "paseo:browser-keyboard-policy",
payload: { browserId: "browser-a", prefixes: [] },
payload: { browserId: "browser-a", menuPrefixes: [], prefixes: [] },
},
]);
});
test("owns reserved shortcuts and leaves plain guest input contained", () => {
test("owns browser chrome shortcuts and leaves customizable shortcuts to policy", () => {
const { attach } = createBrowserKeyboard();
const guest = new FakeBrowserContents(81);
const host = new FakeBrowserContents(82);
attach({ browserId: "browser-a", contents: guest, hostContents: host });
const command = process.platform === "darwin" ? { meta: true } : { control: true };
const reservedWasPrevented = guest.input(electronInput({ ...command, code: "KeyT", key: "t" }));
const reservedWasPrevented = guest.input(electronInput({ ...command, code: "KeyL", key: "l" }));
const customizableWasPrevented = guest.input(
electronInput({ ...command, code: "KeyT", key: "t" }),
);
const enterWasPrevented = guest.input(electronInput({ code: "Enter", key: "Enter" }));
expect(reservedWasPrevented).toBe(true);
expect(customizableWasPrevented).toBe(false);
expect(enterWasPrevented).toBe(false);
expect(guest.ignoredMenuShortcuts).toEqual([false, true]);
expect(guest.ignoredMenuShortcuts).toEqual([false, false, true]);
expect(host.sent).toEqual([
{
channel: "paseo:event:browser-shortcut",
payload: { action: "new-tab", browserId: "browser-a" },
payload: { action: "focus-url", browserId: "browser-a" },
},
]);
});
@@ -274,6 +288,9 @@ describe("BrowserKeyboard", () => {
const guest = new FakeBrowserContents(91);
const host = new FakeBrowserContents(92);
keyboard.publish(host.id, {
menuPrefixes: [
{ alt: false, code: "KeyW", control: true, meta: false, repeat: false, shift: false },
],
prefixes: [
{ alt: false, code: "KeyW", control: true, meta: false, repeat: false, shift: false },
],
@@ -285,4 +302,24 @@ describe("BrowserKeyboard", () => {
expect(wasPrevented).toBe(false);
expect(guest.ignoredMenuShortcuts).toEqual([true]);
});
test("keeps idle policy shortcuts out of the application menu while a chord is pending", () => {
const { attach, keyboard } = createBrowserKeyboard();
const guest = new FakeBrowserContents(101);
const host = new FakeBrowserContents(102);
keyboard.publish(host.id, {
menuPrefixes: [
{ alt: false, code: "KeyW", control: true, meta: false, repeat: false, shift: false },
],
prefixes: [
{ alt: false, code: "F11", control: true, meta: false, repeat: false, shift: false },
],
});
attach({ browserId: "browser-a", contents: guest, hostContents: host });
const wasPrevented = guest.input(electronInput({ code: "KeyW", control: true, key: "w" }));
expect(wasPrevented).toBe(false);
expect(guest.ignoredMenuShortcuts).toEqual([true]);
});
});

View File

@@ -3,6 +3,7 @@ import { PaseoBrowserWebviewRegistry } from "../browser-webviews/registry.js";
import {
type BrowserKeyboardPolicy,
classifyBrowserReservedShortcut,
matchesBrowserShortcutPrefixes,
matchesBrowserShortcutPolicy,
parseBrowserKeyboardPolicy,
parseBrowserShortcutInput,
@@ -150,19 +151,21 @@ export class BrowserKeyboard {
input: Electron.Input,
): void {
const policy = this.policiesByHostWebContentsId.get(registration.hostWebContentsId);
const matchInput = {
alt: input.alt,
code: input.code,
control: input.control,
key: input.key,
meta: input.meta,
repeat: input.isAutoRepeat,
shift: input.shift,
};
const belongsToBrowserPolicy =
policy !== undefined &&
matchesBrowserShortcutPolicy(policy, {
alt: input.alt,
code: input.code,
control: input.control,
key: input.key,
meta: input.meta,
repeat: input.isAutoRepeat,
shift: input.shift,
});
policy !== undefined && matchesBrowserShortcutPolicy(policy, matchInput);
const belongsToMenuPolicy =
policy !== undefined && matchesBrowserShortcutPrefixes(policy.menuPrefixes, matchInput);
guest.contents.setIgnoreMenuShortcuts(
(!input.control && !input.meta) || belongsToBrowserPolicy,
(!input.control && !input.meta) || belongsToBrowserPolicy || belongsToMenuPolicy,
);
const reservedShortcut = classifyBrowserReservedShortcut(input, {
isMac: process.platform === "darwin",
@@ -182,7 +185,6 @@ export class BrowserKeyboard {
}
return;
case "focus-url":
case "new-tab":
event.preventDefault();
if (!guest.hostContents.isDestroyed()) {
guest.hostContents.send(RESERVED_SHORTCUT_OUTPUT_CHANNEL, {

View File

@@ -22,10 +22,10 @@ describe("browser keyboard policy", () => {
expect(
macInputs.map((input) => classifyBrowserReservedShortcut(input, { isMac: true })),
).toEqual(["new-tab", "focus-url", "reload", "force-reload"]);
).toEqual([null, "focus-url", "reload", "force-reload"]);
expect(
nonMacInputs.map((input) => classifyBrowserReservedShortcut(input, { isMac: false })),
).toEqual(["new-tab", "focus-url", "reload", "force-reload"]);
).toEqual([null, "focus-url", "reload", "force-reload"]);
});
test("rejects the wrong or ambiguous command modifier for reserved shortcuts", () => {
@@ -65,11 +65,17 @@ describe("browser keyboard policy", () => {
test("accepts only complete modifier prefixes from the host renderer", () => {
expect(
parseBrowserKeyboardPolicy({
menuPrefixes: [
{ code: "KeyB", control: true, meta: false, alt: false, repeat: false, shift: false },
],
prefixes: [
{ code: "KeyB", control: true, meta: false, alt: false, repeat: false, shift: false },
],
}),
).toEqual({
menuPrefixes: [
{ code: "KeyB", control: true, meta: false, alt: false, repeat: false, shift: false },
],
prefixes: [
{ code: "KeyB", control: true, meta: false, alt: false, repeat: false, shift: false },
],
@@ -80,6 +86,7 @@ describe("browser keyboard policy", () => {
test("rejects a false code fallback instead of treating it as absent", () => {
expect(
parseBrowserKeyboardPolicy({
menuPrefixes: [],
prefixes: [
{
alt: false,
@@ -110,6 +117,7 @@ describe("browser keyboard policy", () => {
test("matches digit shortcuts for the top row and numeric keypad", () => {
const policy = parseBrowserKeyboardPolicy({
menuPrefixes: [],
prefixes: [
{ alt: false, code: "Digit", control: true, meta: false, repeat: false, shift: false },
],

View File

@@ -11,6 +11,7 @@ export interface BrowserShortcutPrefix {
}
export interface BrowserKeyboardPolicy {
menuPrefixes: BrowserShortcutPrefix[];
prefixes: BrowserShortcutPrefix[];
}
@@ -35,7 +36,7 @@ export interface BrowserShortcutMatchInput {
shift: boolean;
}
export type BrowserReservedShortcut = "new-tab" | "focus-url" | "reload" | "force-reload";
export type BrowserReservedShortcut = "focus-url" | "reload" | "force-reload";
function isRecord(value: unknown): value is Record<string, unknown> {
return typeof value === "object" && value !== null && !Array.isArray(value);
@@ -72,19 +73,28 @@ function parsePrefix(value: unknown): BrowserShortcutPrefix | null {
};
}
export function parseBrowserKeyboardPolicy(value: unknown): BrowserKeyboardPolicy | null {
if (!isRecord(value) || !Array.isArray(value.prefixes)) {
function parsePrefixes(value: unknown): BrowserShortcutPrefix[] | null {
if (!Array.isArray(value)) {
return null;
}
const prefixes: BrowserShortcutPrefix[] = [];
for (const entry of value.prefixes) {
for (const entry of value) {
const prefix = parsePrefix(entry);
if (!prefix) {
return null;
}
prefixes.push(prefix);
}
return { prefixes };
return prefixes;
}
export function parseBrowserKeyboardPolicy(value: unknown): BrowserKeyboardPolicy | null {
if (!isRecord(value)) {
return null;
}
const menuPrefixes = parsePrefixes(value.menuPrefixes);
const prefixes = parsePrefixes(value.prefixes);
return menuPrefixes && prefixes ? { menuPrefixes, prefixes } : null;
}
export function parseBrowserShortcutInput(value: unknown): BrowserShortcutInput | null {
@@ -145,11 +155,18 @@ function matchesPrefix(prefix: BrowserShortcutPrefix, input: BrowserShortcutMatc
return (prefix.alt || prefix.codeFallback === true) && matchesCode(prefix.code, input.code);
}
export function matchesBrowserShortcutPrefixes(
prefixes: BrowserShortcutPrefix[],
input: BrowserShortcutMatchInput,
): boolean {
return prefixes.some((prefix) => matchesPrefix(prefix, input));
}
export function matchesBrowserShortcutPolicy(
policy: BrowserKeyboardPolicy,
input: BrowserShortcutMatchInput,
): boolean {
return policy.prefixes.some((prefix) => matchesPrefix(prefix, input));
return matchesBrowserShortcutPrefixes(policy.prefixes, input);
}
export function classifyBrowserReservedShortcut(
@@ -172,7 +189,6 @@ export function classifyBrowserReservedShortcut(
return null;
}
const key = input.key.toLowerCase();
if (!input.shift && key === "t") return "new-tab";
if (!input.shift && key === "l") return "focus-url";
if (key !== "r") return null;
return input.shift ? "force-reload" : "reload";