Skip to content

Commit b0fee21

Browse files
committed
fix(firefox): harden global shortcuts against AltGr, IME, and pickers
Address review P1s on #2608: ignore Alt/AltGraph chords so AltGr slash still types into the composer, skip Escape abort during IME composition, and let open provider/language pickers consume Escape before abortRun.
1 parent f63d350 commit b0fee21

2 files changed

Lines changed: 22 additions & 0 deletions

File tree

src/firefox/src/ui/sidepanel.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9319,8 +9319,16 @@ async function handleGlobalKeydown(e) {
93199319
const isOtherFormField = e.target !== inputEl && (tag === 'INPUT' || tag === 'TEXTAREA' || tag === 'SELECT');
93209320

93219321
if (e.key === 'Escape') {
9322+
// IME Escape cancels a composition candidate — never abort the run for that.
9323+
if (e.isComposing) return;
93229324
const slashMenuOpen = !!slashCommandMenuEl && !slashCommandMenuEl.classList.contains('hidden');
93239325
if (slashMenuOpen) return;
9326+
// Provider/language pickers close on Escape in bubble/target handlers; do not
9327+
// abort the active run while those listboxes are open.
9328+
const providerPickerOpen = !!providerPickerMenu && !providerPickerMenu.classList.contains('hidden');
9329+
if (providerPickerOpen) return;
9330+
const languagePickerOpen = !!languagePickerMenu && !languagePickerMenu.classList.contains('hidden');
9331+
if (languagePickerOpen) return;
93249332
if (isProcessing) {
93259333
e.preventDefault();
93269334
abortRun();
@@ -9330,6 +9338,9 @@ async function handleGlobalKeydown(e) {
93309338
}
93319339

93329340
if (isOtherFormField) return;
9341+
// AltGr (Ctrl+Alt) and Option chords type international characters — never
9342+
// treat them as Ctrl/Cmd shortcuts.
9343+
if (e.altKey || e.getModifierState?.('AltGraph')) return;
93339344

93349345
const mod = e.ctrlKey || e.metaKey;
93359346

test/run.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16614,6 +16614,17 @@ test('firefox sidepanel implements documented global keyboard shortcuts', () =>
1661416614
assert.match(panel, /mod && e\.shiftKey && e\.key === 'D'/, 'firefox: Dev mode shortcut missing');
1661516615
assert.match(panel, /await ensureActMode\(\)/, 'firefox: Act shortcut should call ensureActMode');
1661616616
assert.match(panel, /await ensureDevMode\(\)/, 'firefox: Dev shortcut should call ensureDevMode');
16617+
// Review P1s: AltGr / IME / open pickers must not steal Escape or slash entry
16618+
assert.match(panel, /e\.altKey \|\| e\.getModifierState\?\.\('AltGraph'\)/, 'firefox: AltGr/Option chords must not trigger Ctrl shortcuts');
16619+
assert.match(panel, /if \(e\.isComposing\) return;/, 'firefox: IME Escape must not abort the active run');
16620+
assert.match(panel, /providerPickerMenu && !providerPickerMenu\.classList\.contains\('hidden'\)/, 'firefox: open provider picker Escape must not abort');
16621+
assert.match(panel, /languagePickerMenu && !languagePickerMenu\.classList\.contains\('hidden'\)/, 'firefox: open language picker Escape must not abort');
16622+
const composingGuard = panel.indexOf('if (e.isComposing) return;', globalHandlerStart);
16623+
const providerPickerGuard = panel.indexOf("providerPickerMenu && !providerPickerMenu.classList.contains('hidden')", globalHandlerStart);
16624+
const languagePickerGuard = panel.indexOf("languagePickerMenu && !languagePickerMenu.classList.contains('hidden')", globalHandlerStart);
16625+
assert.equal(composingGuard !== -1 && composingGuard < abortCall, true, 'firefox: IME guard must precede abortRun');
16626+
assert.equal(providerPickerGuard !== -1 && providerPickerGuard < abortCall, true, 'firefox: provider picker guard must precede abortRun');
16627+
assert.equal(languagePickerGuard !== -1 && languagePickerGuard < abortCall, true, 'firefox: language picker guard must precede abortRun');
1661716628

1661816629
for (const shortcut of ['Ctrl/Cmd+/', 'Ctrl/Cmd+Shift+A', 'Ctrl/Cmd+Shift+X', 'Ctrl/Cmd+Shift+D', 'Escape']) {
1661916630
assert.match(locale, new RegExp(escapeRegExpLiteral(shortcut)), `firefox: /help should mention ${shortcut}`);

0 commit comments

Comments
 (0)