diff --git a/static/js/editor/keyboard-shortcuts.js b/static/js/editor/keyboard-shortcuts.js index 0f83ac1..2f9ed74 100644 --- a/static/js/editor/keyboard-shortcuts.js +++ b/static/js/editor/keyboard-shortcuts.js @@ -50,6 +50,7 @@ * }} deps */ import { state } from './state.js'; +import { isAltGrEvent } from '../platform.js'; export function wireKeyboardShortcuts(deps) { const { @@ -79,7 +80,11 @@ export function wireKeyboardShortcuts(deps) { return; } if (e.key === 'Escape') return; - if (e.ctrlKey || e.metaKey) { + // Skip the Ctrl+Alt editor chords for an AltGr keystroke (see platform.js); + // only the chord block is skipped, so the layout-character handlers below + // still act — AltGr+5 / AltGr+8 stay as the [ ] brush-size shortcut on + // AZERTY / QWERTZ. + if ((e.ctrlKey || e.metaKey) && !isAltGrEvent(e)) { if (e.key === 'z') { e.preventDefault(); if (e.shiftKey) redo(); else undo(); } // Ctrl+Shift+D = Deselect: clears the wand selection (and // lasso if active) without affecting layers. diff --git a/static/js/keyboard-shortcuts.js b/static/js/keyboard-shortcuts.js index 2252017..6599ed4 100644 --- a/static/js/keyboard-shortcuts.js +++ b/static/js/keyboard-shortcuts.js @@ -2,6 +2,8 @@ // Keyboard Shortcuts — dynamic keybinds // ============================================ +import { IS_MAC, isAltGrEvent } from './platform.js'; + const _defaultKeybinds = { search: 'ctrl+k', toggle_sidebar: 'ctrl+alt+b', new_session: 'ctrl+alt+n', fav_session: 'ctrl+alt+f', delete_session: 'ctrl+alt+d', @@ -13,8 +15,11 @@ const _defaultKeybinds = { open_notes: '', open_tasks: '', open_theme: '', }; -function _matchesCombo(e, combo) { +export function _matchesCombo(e, combo, isMac = IS_MAC) { if (!combo) return false; + // Drop AltGr keystrokes so typing characters on non-US layouts can't fire a + // Ctrl+Alt shortcut — e.g. the destructive delete_session. See platform.js. + if (isAltGrEvent(e, isMac)) return false; const parts = combo.split('+'); const needCtrl = parts.includes('ctrl'); const needAlt = parts.includes('alt'); diff --git a/static/js/platform.js b/static/js/platform.js new file mode 100644 index 0000000..e0d7747 --- /dev/null +++ b/static/js/platform.js @@ -0,0 +1,47 @@ +// ============================================ +// Platform detection + AltGr-keystroke helper +// ============================================ +// Shared by the keybind code: root keyboard-shortcuts.js, the editor's +// keyboard-shortcuts.js, and settings.js. Single source of truth so the three +// guards can't drift. + +// AltGr (right Alt on AZERTY/QWERTZ and most non-US layouts, used to type +// @ # { } [ ] | \ and €) is reported by browsers as Ctrl+Alt. macOS is the +// exception: there the Option key — a normal part of Mac shortcuts — also sets +// the AltGraph modifier state, so it must NOT be treated as AltGr. +// +// IS_MAC covers all Apple platforms, iPad/iPhone included: a Magic Keyboard's +// Option key sets AltGraph exactly like a Mac's, so they need the same carve-out +// — narrowing to macOS-only would re-break them. The name and the +// /Mac|iPhone|iPad/ test deliberately mirror the existing isMac checks in +// calendar.js and sessions.js; this is their single shared source of truth. +export const IS_MAC = + /Mac|iPhone|iPad/.test((typeof navigator !== 'undefined' && navigator.platform) || '') || + /Mac/.test((typeof navigator !== 'undefined' && navigator.userAgent) || ''); + +// True when `e` is an AltGr keystroke we should ignore for Ctrl+Alt shortcut +// purposes. getModifierState('AltGraph') is true for AltGr but false for a +// genuine left Ctrl+Alt, so real shortcuts still work. Always false on macOS, +// where Option legitimately sets AltGraph. +// +// We also require ctrlKey+altKey: the collision we defend against is precisely +// "AltGr reported AS Ctrl+Alt", so an event that asserts AltGraph WITHOUT +// presenting as Ctrl+Alt (a Linux ISO_Level3_Shift layout, a stray modifier +// state) is left alone instead of being swallowed. +// +// Trade-off: on Windows AltGr *is* Ctrl+right-Alt, so a deliberate +// Ctrl+Alt+ shortcut typed via AltGr is unreachable too — accepted; use +// the left Ctrl+Alt. +// +// NOTE: the AltGr -> AltGraph mapping is taken from the UI Events spec / MDN, +// not proven by our tests. Older Firefox and some Linux setups historically did +// not report AltGraph; where a browser sets ctrlKey+altKey without it this +// guard is simply a no-op (the pre-fix behaviour) rather than a regression. +export function isAltGrEvent(e, isMac = IS_MAC) { + return ( + !isMac && + !!e.ctrlKey && + !!e.altKey && + !!(e.getModifierState && e.getModifierState('AltGraph')) + ); +} diff --git a/static/js/settings.js b/static/js/settings.js index 6f04140..0a63dd2 100644 --- a/static/js/settings.js +++ b/static/js/settings.js @@ -6,6 +6,7 @@ import searchModule from './search.js'; import { makeWindowDraggable } from './windowDrag.js'; import { clearDockSide } from './modalSnap.js'; import { sortModelIds } from './modelSort.js'; +import { isAltGrEvent } from './platform.js'; let initialized = false; let modalEl = null; @@ -1710,6 +1711,10 @@ function _formatKeyCaps(combo) { } function _comboFromEvent(e) { + // Drop a stray AltGr keystroke (e.g. AltGr+E to type €) so it isn't recorded + // as a bogus ctrl+alt+ binding — onKey ignores empty combos. See + // platform.js for the macOS carve-out and Windows trade-off. + if (isAltGrEvent(e)) return ''; const parts = []; if (e.ctrlKey || e.metaKey) parts.push('ctrl'); if (e.altKey) parts.push('alt'); diff --git a/tests/test_keybind_altgr_js.py b/tests/test_keybind_altgr_js.py new file mode 100644 index 0000000..a93538d --- /dev/null +++ b/tests/test_keybind_altgr_js.py @@ -0,0 +1,183 @@ +"""Pin the AltGr-safety of the shared keybind predicate and the matcher. + +Driven through `node --input-type=module` so we exercise the real JS without a +full Vitest/Jest setup (same approach as test_compare_js.py / +test_reply_recipients_js.py). Skips when `node` is not installed rather than +failing. + +Bug: browsers report the AltGr key (right Alt, essential on AZERTY/QWERTZ and +many non-US layouts to type @ # { } [ ] | \\ and €) as ctrlKey=true AND +altKey=true, so a user on a non-US layout typing a special character could +silently fire a destructive ctrl+alt+ default (new_session, +delete_session, incognito, open_calendar). getModifierState('AltGraph') is true +for AltGr but false for a genuine left Ctrl+Alt — except on macOS, where the +Option key also sets it. + +The guard now lives in ONE place — `isAltGrEvent` in static/js/platform.js — and +all three call sites (editor keyboard-shortcuts.js, root keyboard-shortcuts.js, +settings.js) route through it. So these tests pin the shared *predicate* +directly (both the isMac arg and the navigator-derived IS_MAC default), plus the +`_matchesCombo` integration. They do NOT prove that real browsers actually set +AltGraph for AltGr — that mapping is taken from the UI Events spec / MDN; older +Firefox and some Linux setups historically did not report it (the guard is a +no-op there, i.e. pre-fix behaviour, not a regression). +""" +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parent.parent +_HELPER = _REPO / "static" / "js" / "keyboard-shortcuts.js" +_PLATFORM = _REPO / "static" / "js" / "platform.js" +_HAS_NODE = shutil.which("node") is not None + +# Every test here shells out to `node`; skip the whole module when it is absent +# rather than repeating the mark per test (same convention as test_compare_js.py +# / test_reply_recipients_js.py). +pytestmark = pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") + + +def _run(js: str) -> str: + proc = subprocess.run( + ["node", "--input-type=module"], + input=js, capture_output=True, text=True, cwd=str(_REPO), timeout=30, + ) + assert proc.returncode == 0, proc.stderr + return proc.stdout.strip() + + +def _is_altgr( + altgraph: bool, + is_mac: bool = False, + has_modifier_state: bool = True, + ctrl: bool = True, + alt: bool = True, +) -> bool: + """Return isAltGrEvent(ev, is_mac) — the predicate every guard routes through.""" + modifier = ( + f"ev.getModifierState = (m) => m === 'AltGraph' ? {json.dumps(altgraph)} : false;" + if has_modifier_state else "") + js = f""" + import {{ isAltGrEvent }} from '{_PLATFORM.as_uri()}'; + const ev = {{ ctrlKey: {json.dumps(ctrl)}, altKey: {json.dumps(alt)} }}; + {modifier} + console.log(JSON.stringify(isAltGrEvent(ev, {json.dumps(is_mac)}))); + """ + return json.loads(_run(js)) + + +def _is_mac_default(platform: str = "", user_agent: str = "") -> bool: + """Return platform.js IS_MAC as derived from a stubbed navigator at import time.""" + # Node >=21 exposes a read-only global `navigator`, so assignment throws; + # defineProperty (configurable) overrides it for the import-time read. + js = f""" + Object.defineProperty(globalThis, 'navigator', {{ + value: {{ platform: {json.dumps(platform)}, userAgent: {json.dumps(user_agent)} }}, + configurable: true, + }}); + const {{ IS_MAC }} = await import('{_PLATFORM.as_uri()}'); + console.log(JSON.stringify(IS_MAC)); + """ + return json.loads(_run(js)) + + +def _matches(event: dict, combo: str, altgraph: bool, is_mac: bool = False) -> bool: + """Return _matchesCombo(event, combo, is_mac) with AltGraph active or not.""" + js = f""" + import {{ _matchesCombo }} from '{_HELPER.as_uri()}'; + const ev = {json.dumps(event)}; + ev.getModifierState = (m) => m === 'AltGraph' ? {json.dumps(altgraph)} : false; + console.log(JSON.stringify(_matchesCombo(ev, {json.dumps(combo)}, {json.dumps(is_mac)}))); + """ + return json.loads(_run(js)) + + +# --- The shared predicate (covers all three guards) -------------------------- + +def test_isaltgr_true_for_altgr_keystroke_off_mac(): + # AZERTY/QWERTZ user holds AltGr: browser sets ctrlKey+altKey+AltGraph. + assert _is_altgr(altgraph=True, is_mac=False) is True + + +def test_isaltgr_false_for_genuine_ctrl_alt(): + # A real left Ctrl+Alt press leaves AltGraph unset. + assert _is_altgr(altgraph=False, is_mac=False) is False + + +def test_isaltgr_false_when_altgraph_set_but_not_ctrl_alt(): + # The collision we defend against is specifically "AltGr reported AS + # Ctrl+Alt". An event that asserts AltGraph WITHOUT presenting as Ctrl+Alt + # (e.g. a Linux ISO_Level3_Shift layout, or a stray modifier state) must NOT + # be swallowed — only a genuine Ctrl+Alt-presenting AltGr keystroke is. + assert _is_altgr(altgraph=True, ctrl=False, alt=False) is False + assert _is_altgr(altgraph=True, ctrl=True, alt=False) is False + assert _is_altgr(altgraph=True, ctrl=False, alt=True) is False + + +def test_isaltgr_false_on_mac_even_with_altgraph(): + # macOS reports AltGraph=true for the Option key, but Ctrl+Option / Cmd+Option + # are legitimate Mac shortcuts, so the predicate must never swallow them. + assert _is_altgr(altgraph=True, is_mac=True) is False + + +def test_isaltgr_false_when_getmodifierstate_missing(): + # Defensive: an event without getModifierState must not throw or report AltGr. + assert _is_altgr(altgraph=False, is_mac=False, has_modifier_state=False) is False + + +# --- The navigator-derived IS_MAC default (dead in node without a stub) ------- + +def test_is_mac_from_navigator_platform(): + # navigator.platform reports "MacIntel" on EVERY Mac — Apple Silicon + # (M1/M2/M3...) included; the string was frozen for compatibility, so there + # is no "MacARM". The regex matches the "Mac" substring, not "Intel". + assert _is_mac_default(platform="MacIntel") is True + + +def test_is_mac_apple_silicon_reports_macintel(): + # Pin the quirk explicitly: an Apple Silicon Mac's UA still says Macintosh + # and its platform still says MacIntel, so the carve-out protects it too. + assert _is_mac_default( + platform="MacIntel", + user_agent="Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15", + ) is True + + +def test_is_mac_from_user_agent_when_platform_blank(): + # iPadOS / some browsers report a Mac userAgent with an unhelpful platform. + assert _is_mac_default(platform="", user_agent="Mozilla/5.0 (Macintosh; ...)") is True + + +def test_is_not_mac_on_windows(): + assert _is_mac_default(platform="Win32", user_agent="Mozilla/5.0 (Windows NT 10.0)") is False + + +# --- _matchesCombo integration (the matcher predicate, end to end) ----------- + +def test_altgr_keystroke_does_not_trigger_ctrl_alt_shortcut(): + # AZERTY/QWERTZ user holds AltGr over a key that yields 'n'. This must NOT + # fire the destructive new_session combo. + ev = {"ctrlKey": True, "altKey": True, "shiftKey": False, "key": "n"} + assert _matches(ev, "ctrl+alt+n", altgraph=True, is_mac=False) is False + + +def test_genuine_ctrl_alt_still_matches(): + # A real left Ctrl+Alt press (AltGraph not set) must still work. + ev = {"ctrlKey": True, "altKey": True, "shiftKey": False, "key": "n"} + assert _matches(ev, "ctrl+alt+n", altgraph=False, is_mac=False) is True + + +def test_mac_option_combo_still_matches(): + # macOS reports AltGraph=true for the Option key, but Ctrl+Option / Cmd+Option + # are legitimate Mac shortcuts. On macOS the guard must NOT swallow them. + ev = {"ctrlKey": True, "altKey": True, "shiftKey": False, "key": "n"} + assert _matches(ev, "ctrl+alt+n", altgraph=True, is_mac=True) is True + + +def test_plain_ctrl_shortcut_unaffected(): + # Non-alt combos were never AltGr-ambiguous and must keep matching. + ev = {"ctrlKey": True, "altKey": False, "shiftKey": False, "key": "k"} + assert _matches(ev, "ctrl+k", altgraph=False, is_mac=False) is True