mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
fix(web): drop the ⌘] binding that cancelled browser Forward for nothing (BUG-2666) (#1849)
⌘] toggled uiStore.detailPanelOpen, which no component has read since the first release; the real detail pane is URL-derived (?item=). Chrome does not reserve Back/Forward (IsReservedCommandOrKey in Chromium's browser_command_controller.cc), so it delivered the key, and the handler's preventDefault took Forward away in Chrome, Safari and Firefox on macOS and Firefox on Linux while changing nothing visible. Removes the binding, the dead state (detailPanelOpen, its toggle/open/ close, the pad-detail-panel key, the mobile-crossing force-close, the unused --detail-panel-width var) and the e2e probes that measured it. A layout test pins that Mod+[ / Mod+] are never prevented, with either modifier; it is red on main. Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS
This commit is contained in:
xarmian
@@ -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(
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<string | null>(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;
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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],
|
||||
|
||||
Reference in New Issue
Block a user