diff --git a/web/e2e/attachment-viewer-owners.spec.ts b/web/e2e/attachment-viewer-owners.spec.ts index a2a600e52..68be71f83 100644 --- a/web/e2e/attachment-viewer-owners.spec.ts +++ b/web/e2e/attachment-viewer-owners.spec.ts @@ -244,9 +244,10 @@ test.describe('attachment viewer — global key & gesture owners (TASK-2436)', ( // The root shortcuts were entirely unguarded before TASK-2430, and // `isInputFocused()` cannot help: a focused viewer BUTTON is not a text // control, so every one of them used to fire straight through the viewer. - // All six are checked, because the guard is one `return` covering all of - // them — and a test for only one of them would leave five that a - // re-ordering could resurrect. + // All five are checked, because the guard is one `return` covering all of + // them — and a test for only one of them would leave four that a + // re-ordering could resurrect. (Cmd+] was a sixth until BUG-2666 removed + // it: it toggled state nothing rendered.) await browserLogin(page); const doc = await seedDoc(fixture, request, 'Owner shortcuts'); await uploadAttachment(fixture, request, doc.id, 'shortcuts.png'); @@ -258,7 +259,6 @@ test.describe('attachment viewer — global key & gesture owners (TASK-2436)', ( palette: !!document.querySelector('.palette'), shortcutsModal: !!document.querySelector('#keyboard-shortcuts-title'), quickAdd: !!document.querySelector('.quick-add-modal'), - detailPanel: localStorage.getItem('pad-detail-panel'), sidebarCollapsed: !!document.querySelector('aside.sidebar.collapsed'), filterBar: !!document.querySelector('.filter-bar'), // Cmd+F's effect once the bar is already open is to FOCUS the @@ -273,11 +273,6 @@ test.describe('attachment viewer — global key & gesture owners (TASK-2436)', ( await page.keyboard.press('Escape'); await expect.poll(async () => (await state()).palette).toBe(false); - const detailBefore = (await state()).detailPanel; - await page.keyboard.press('Control+]'); - await expect.poll(async () => (await state()).detailPanel).not.toBe(detailBefore); - const detailAfterToggle = (await state()).detailPanel; - const sidebarBefore = (await state()).sidebarCollapsed; await page.keyboard.press('Control+\\'); await expect.poll(async () => (await state()).sidebarCollapsed).toBe(!sidebarBefore); @@ -307,15 +302,14 @@ test.describe('attachment viewer — global key & gesture owners (TASK-2436)', ( // ── WITH A VIEWER FRONTMOST: nothing moves ── await openViewer(page); const before = await state(); - for (const key of ['Control+k', 'Control+]', 'Control+\\', 'Control+n', 'Control+f', '?']) { + for (const key of ['Control+k', 'Control+\\', 'Control+n', 'Control+f', '?']) { await page.keyboard.press(key); } // Give any of them a chance to land before asserting nothing did. await page.waitForTimeout(300); expect(await state(), 'no root shortcut may act while a viewer is frontmost').toEqual(before); // Sanity: the values asserted-unchanged are the ones the baseline moved, - // so "unchanged" is a real claim about six live shortcuts. - expect(before.detailPanel).toBe(detailAfterToggle); + // so "unchanged" is a real claim about five live shortcuts. expect(before.sidebarCollapsed).toBe(sidebarAfterToggle); expect(before.filterBar).toBe(true); expect( diff --git a/web/e2e/pane-mobile-overlay.spec.ts b/web/e2e/pane-mobile-overlay.spec.ts index b4816d203..56bb48e43 100644 --- a/web/e2e/pane-mobile-overlay.spec.ts +++ b/web/e2e/pane-mobile-overlay.spec.ts @@ -15,11 +15,11 @@ import type { Page } from '@playwright/test'; * * The load-bearing correctness property is that crossing the breakpoint is * PURE PRESENTATION: it must NOT drop `?item=`. `ui.svelte.ts` has a matchMedia - * handler that force-closes the legacy `detailPanelOpen` boolean on mobile - * entry; the URL-derived pane is deliberately NOT wired to it. If a regression - * ever routes pane visibility through that boolean, entering mobile would - * silently clear the open item — this spec drives a real breakpoint crossing - * and asserts `?item=` survives it in both directions. + * handler that collapses the sidebar on mobile entry; the URL-derived pane is + * deliberately NOT wired to it. If a regression ever routes pane visibility + * through that handler, entering mobile would silently clear the open item — + * this spec drives a real breakpoint crossing and asserts `?item=` survives it + * in both directions. * * The URL assertions are placed AFTER `settleBreakpoint()`, which blocks until * the app's matchMedia `change` listeners AND the Svelte effect flush have run @@ -138,7 +138,7 @@ test.describe('mobile split-pane full-screen overlay (PLAN-2105 Phase 4 / TASK-2 await pane.evaluate((el) => ((el as HTMLElement).dataset.t2121 = 'persist')); // Enter mobile (768px): the ui.svelte.ts matchMedia handler fires here - // (detailPanelOpen = false). Assert AFTER settleBreakpoint so the async + // (sidebarOpen = false). Assert AFTER settleBreakpoint so the async // handler has definitely run — the URL-derived pane must NOT be dropped. await settleBreakpoint(page, OVERLAY); await expect.poll(() => panePosition(page)).toBe('fixed'); diff --git a/web/src/app.css b/web/src/app.css index 62b2562cd..210c3f60e 100644 --- a/web/src/app.css +++ b/web/src/app.css @@ -70,7 +70,6 @@ --bottom-nav-height: 56px; --context-bar-height: 44px; --sidebar-width: 260px; - --detail-panel-width: 300px; --content-max-width: 960px; --font-ui: -apple-system, BlinkMacSystemFont, 'Segoe UI', system-ui, sans-serif; diff --git a/web/src/lib/components/collections/PaneHost.svelte b/web/src/lib/components/collections/PaneHost.svelte index 81c03d0be..a6d7d722b 100644 --- a/web/src/lib/components/collections/PaneHost.svelte +++ b/web/src/lib/components/collections/PaneHost.svelte @@ -309,10 +309,9 @@ // No resize primitive existed in the repo, so this is built from // scratch: a draggable divider between the list column and the pane, // pointer-captured so the drag can't select text or fall through to a - // row-click. The width persists to localStorage under a GLOBAL key that - // deliberately does NOT collide with the vestigial `pad-detail-panel` - // key in ui.svelte.ts, reusing the read-on-init / write-on-change idiom - // from that store (the `pad-topbar` pattern). + // row-click. The width persists to localStorage under a GLOBAL key, + // reusing the read-on-init / write-on-change idiom from ui.svelte.ts + // (the `pad-topbar` pattern). const PANE_WIDTH_KEY = 'pad-pane-width'; const PANE_WIDTH_MIN = 360; // mirrors the CSS clamp() floor const PANE_WIDTH_MAX = 720; // generous ceiling (CSS clamp maxed at 640) @@ -909,11 +908,10 @@ (ItemDetail `.graph-drawer`: fixed, `width: 100vw`, no border). Pane visibility stays URL-derived (`?item=`); this overlay is pure - presentation keyed off the viewport, NOT the vestigial - `uiStore.detailPanelOpen` boolean — whose mobile-entry force-close - must never reach `?item=` (see the reconciliation note in - ui.svelte.ts). So crossing the 768px boundary only swaps - split ⇄ overlay; it never closes the pane or drops the open item. + presentation keyed off the viewport, and the ui.svelte.ts + mobile-crossing handler must never reach `?item=` (see the note + there). So crossing the 768px boundary only swaps split ⇄ overlay; + it never closes the pane or drops the open item. `position: fixed` is viewport-relative here because no ancestor (.collection-page / .main-content / .app-shell) establishes a diff --git a/web/src/lib/stores/ui.svelte.ts b/web/src/lib/stores/ui.svelte.ts index 50ac72663..e6266c92f 100644 --- a/web/src/lib/stores/ui.svelte.ts +++ b/web/src/lib/stores/ui.svelte.ts @@ -13,7 +13,6 @@ let isTouch = $state(browser ? 'ontouchstart' in window : false); // viewport, leaving the delta ~0. Consumers (e.g. BottomNav) hide fixed bottom // chrome so it doesn't sit stranded above the keyboard. PLAN-1694. let keyboardVisible = $state(false); -let detailPanelOpen = $state(browser ? localStorage.getItem('pad-detail-panel') !== 'closed' && !viewport.isMobile : false); let createWorkspaceOpen = $state(false); // Set when the last workspace tab is closed and the user lands on /console // (PLAN-3002 Q2): the console page highlights its create button as the way @@ -32,25 +31,21 @@ let connectAfterNavigateSlug = $state(null); if (browser) { // `isMobile` itself is owned by the shared breakpoint store (one app-wide // listener). Here we only run the layout side effects that must fire when - // the viewport crosses the mobile breakpoint: collapse the sidebar/detail - // panel entering mobile, restore the sidebar leaving it. `change` fires only - // on a crossing, so no manual before/after comparison is needed. + // the viewport crosses the mobile breakpoint: collapse the sidebar entering + // mobile, restore it leaving. `change` fires only on a crossing, so no + // manual before/after comparison is needed. // - // `detailPanelOpen` is the pre-PLAN-2105 legacy panel boolean — it is - // mutated here but no component reads it to render anything anymore, so - // this force-close is inert. The collection page's split-pane detail view - // (PLAN-2105 / TASK-2121) is deliberately NOT wired to it: pane state is - // URL-derived (`?item=`), and on mobile the pane simply restyles to a - // full-screen overlay via CSS (see the `@media (max-width: 768px)` block - // in the collection +page.svelte). Routing pane visibility through this - // boolean would silently drop `?item=` on every mobile-entry crossing — - // the exact bug TASK-2121 must avoid. If a future consumer starts reading - // `detailPanelOpen`, keep this handler's reach limited to that legacy - // panel and off the URL-derived pane. + // The collection page's split-pane detail view (PLAN-2105 / TASK-2121) is + // deliberately NOT touched here: pane state is URL-derived (`?item=`), and + // on mobile the pane simply restyles to a full-screen overlay via CSS (see + // the `@media (max-width: 768px)` block in PaneHost). Closing it on a + // crossing would silently drop `?item=` on every mobile entry, the exact + // bug TASK-2121 avoids. (The legacy `detailPanelOpen` boolean this handler + // used to force-close had no reader since the first release and was + // removed with its ⌘] shortcut, BUG-2666.) window.matchMedia(MOBILE_MEDIA_QUERY).addEventListener('change', (e) => { if (e.matches) { sidebarOpen = false; - detailPanelOpen = false; } else { sidebarOpen = true; } @@ -88,7 +83,6 @@ export const uiStore = { get isMobile() { return viewport.isMobile; }, get isTouch() { return isTouch; }, get keyboardVisible() { return keyboardVisible; }, - get detailPanelOpen() { return detailPanelOpen; }, get createWorkspaceOpen() { return createWorkspaceOpen; }, toggleSidebar() { sidebarOpen = !sidebarOpen; }, @@ -111,18 +105,6 @@ export const uiStore = { closeSearch() { searchOpen = false; }, toggleSearch() { searchOpen = !searchOpen; }, - toggleDetailPanel() { - detailPanelOpen = !detailPanelOpen; - if (browser) localStorage.setItem('pad-detail-panel', detailPanelOpen ? 'open' : 'closed'); - }, - openDetailPanel() { - detailPanelOpen = true; - if (browser) localStorage.setItem('pad-detail-panel', 'open'); - }, - closeDetailPanel() { - detailPanelOpen = false; - if (browser) localStorage.setItem('pad-detail-panel', 'closed'); - }, openCreateWorkspace() { createWorkspaceOpen = true; diff --git a/web/src/routes/+layout.svelte b/web/src/routes/+layout.svelte index ba41f6fe9..1c0caa812 100644 --- a/web/src/routes/+layout.svelte +++ b/web/src/routes/+layout.svelte @@ -302,11 +302,12 @@ uiStore.toggleTopbar(); return; } - if (isMod(e) && e.key === ']') { - e.preventDefault(); - uiStore.toggleDetailPanel(); - return; - } + // Nothing here binds Mod+[ or Mod+] (BUG-2666): they are Back/Forward in + // Chrome and Safari on macOS and in Firefox on macOS and Linux, and a + // page that prevents them takes the browser's history keys away. + // Chrome does not reserve them, so it delivers them here, and the ⌘] + // that used to sit here cancelled Forward to toggle state no component + // rendered. if (isMod(e) && e.key === 'n') { e.preventDefault(); uiStore.requestQuickAdd(); diff --git a/web/src/routes/layoutShortcuts.svelte.test.ts b/web/src/routes/layoutShortcuts.svelte.test.ts index 6fd1fdd4d..d7de3e1f9 100644 --- a/web/src/routes/layoutShortcuts.svelte.test.ts +++ b/web/src/routes/layoutShortcuts.svelte.test.ts @@ -96,6 +96,23 @@ describe('+layout app-shell shortcuts — defer to a frontmost surface (TASK-243 expect(uiStore.sidebarOpen).toBe(!before); }); + it('BUG-2666: Mod+[ and Mod+] are left to the browser (Back/Forward), with either modifier', () => { + // They are history keys in Chrome and Safari on macOS and in Firefox on + // macOS and Linux, and Chrome does not reserve them, so it delivers them + // here. A shell that prevents them takes Back/Forward away; the ⌘] that + // used to be bound here did exactly that to toggle state nothing rendered. + for (const key of ['[', ']']) { + for (const mod of ['metaKey', 'ctrlKey'] as const) { + const e = new KeyboardEvent('keydown', { key, [mod]: true, bubbles: true, cancelable: true }); + window.dispatchEvent(e); + expect({ key, mod, prevented: e.defaultPrevented }).toEqual({ key, mod, prevented: false }); + } + } + // Control: the same dispatch path DOES reach the handler, or the + // assertion above would hold with no handler at all. + expect(press('k', true).defaultPrevented).toBe(true); + }); + it('declines EVERY shortcut this handler owns while a viewer lease is frontmost', () => { // All of them, not a sample: a per-key guard (rather than the single // handler-level bail) would let whichever keys it missed straight @@ -107,7 +124,6 @@ describe('+layout app-shell shortcuts — defer to a frontmost surface (TASK-243 for (const [key, mod] of [ ['k', true], ['\\', true], - [']', true], ['n', true], ['f', true], ['?', false],