Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions web/src/app.css
Original file line number Diff line number Diff line change
Expand Up @@ -727,8 +727,11 @@ dialog.attachment-image-lightbox .attachment-image-lightbox-close:hover {
/* Modals / dialogs / tooltips. Most KeyboardShortcuts-style overlays
carry role="dialog" on the backdrop. The shared Modal primitive uses a
native <dialog> (implicit dialog role, no literal role attribute), so it
is covered by the `dialog[open]` selector above. */
[role="dialog"],
is covered by the `dialog[open]` selector above.
`:not(.item-pane)` keeps the detail pane printable: on mobile the pane
carries role="dialog" (TASK-2131) but IS the content being printed, not a
transient overlay to strip. */
[role="dialog"]:not(.item-pane),
[role="tooltip"],
[role="menu"] {
display: none !important;
Expand Down
42 changes: 42 additions & 0 deletions web/src/lib/collections/paneFocus.svelte.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
paneFocusables,
nextTrapTarget,
resolvePaneReturnTarget,
inExemptSurface,
} from './paneFocus';

// jsdom has no layout engine, so `offsetParent` / `getClientRects` can't gate
Expand Down Expand Up @@ -64,6 +65,47 @@ describe('paneFocusables', () => {
});
});

describe('inExemptSurface', () => {
it('does NOT treat the pane region itself as exempt (TASK-2131)', () => {
// The mobile overlay carries role="dialog"; an in-pane control must not be
// seen as an exempt foreign surface (that killed the mobile Tab trap and
// confused the focus-follows classifier). Excluded via `:not(.item-pane)`.
const root = mount(`
<aside class="item-pane" role="dialog" aria-modal="true">
<button id="in-pane">edit</button>
</aside>
`);
expect(inExemptSurface(root.querySelector('#in-pane'))).toBe(false);
});

it('treats a nested dialog/menu opened FROM the pane as exempt', () => {
// A real overlay (e.g. a BottomSheet or field-select menu) that portals in
// while the pane is open still owns its own focus/keyboard.
const root = mount(`
<aside class="item-pane" role="dialog">
<div role="menu"><button id="menu-item">move to…</button></div>
</aside>
<div role="dialog" id="sheet"><button id="sheet-btn">confirm</button></div>
`);
expect(inExemptSurface(root.querySelector('#menu-item'))).toBe(true);
expect(inExemptSurface(root.querySelector('#sheet-btn'))).toBe(true);
});

it('treats a native <dialog> and standalone overlays as exempt; nothing else', () => {
const root = mount(`
<dialog open><button id="native">ok</button></dialog>
<div role="listbox"><div id="opt">option</div></div>
<div class="block-context-menu"><button id="ctx">turn into</button></div>
<button id="plain">plain</button>
`);
expect(inExemptSurface(root.querySelector('#native'))).toBe(true);
expect(inExemptSurface(root.querySelector('#opt'))).toBe(true);
expect(inExemptSurface(root.querySelector('#ctx'))).toBe(true);
expect(inExemptSurface(root.querySelector('#plain'))).toBe(false);
expect(inExemptSurface(null)).toBe(false);
});
});

describe('nextTrapTarget', () => {
function setup() {
const container = mount(`<a href="/1">1</a><a href="/2">2</a><a href="/3">3</a>`);
Expand Down
9 changes: 8 additions & 1 deletion web/src/lib/collections/paneFocus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,16 @@ export function paneFocusables(
* must not read a focus / pointerdown on them as a master↔pane switch.
* ONE definition shared by both call sites (PaneHost's mobile trap + the host
* route's activePane classifier) so the two can't drift.
*
* `[role="dialog"]:not(.item-pane)` EXCLUDES the pane region itself (TASK-2131):
* the mobile overlay now carries `role="dialog"` (a true modal), but it is the
* region these consumers operate ON, not a foreign overlay to defer to — without
* the `:not`, `closest()` from any in-pane element would match the pane and
* wrongly mark the whole pane exempt (killing the mobile Tab trap and confusing
* the classifier). A genuinely nested dialog opened FROM the pane still matches.
*/
export const PANE_EXEMPT_SURFACE_SELECTOR =
'dialog, [role="dialog"], [role="menu"], [role="listbox"], .block-context-menu';
'dialog, [role="dialog"]:not(.item-pane), [role="menu"], [role="listbox"], .block-context-menu';

/** True when `el` (or an ancestor) is one of the {@link PANE_EXEMPT_SURFACE_SELECTOR} overlays. */
export function inExemptSurface(el: Element | null | undefined): boolean {
Expand Down
54 changes: 42 additions & 12 deletions web/src/lib/components/collections/PaneHost.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import { onDestroy, untrack } from 'svelte';
import ItemDetail from '$lib/components/items/ItemDetail.svelte';
import { viewport } from '$lib/stores/breakpoint.svelte';
import { paneOverlay } from '$lib/stores/paneOverlay.svelte';
import { paneFocusables, nextTrapTarget, inExemptSurface } from '$lib/collections/paneFocus';
import type { PaneTarget } from '$lib/types';

Expand Down Expand Up @@ -330,10 +331,11 @@
// can't see these).
// Both DEFER to a native modal <dialog> (Share / Edit Collection / the
// Open-Children confirm — `Modal.svelte` uses `showModal()`), which renders
// in the top layer above the overlay and owns its own focus cycle. `<aside>`
// isn't a dialog, so this never exempts the pane itself; role="dialog" sheets
// (BottomSheet) sit BELOW the pane and stay behind it, so focus belongs on
// the pane, not them.
// in the top layer above the overlay and owns its own focus cycle. The pane
// itself now carries `role="dialog"` on mobile (TASK-2131), so the exempt
// selector excludes it via `:not(.item-pane)` — the trap must NOT treat the
// region it's guarding as an exempt surface; role="dialog" sheets (BottomSheet)
// sit BELOW the pane and stay behind it, so focus belongs on the pane, not them.
$effect(() => {
if (!browser) return;
if (!paneEl || !viewport.isMobile) return;
Expand Down Expand Up @@ -377,6 +379,25 @@
};
});

// App-shell isolation for the mobile modal (PLAN-2105 / TASK-2131). The
// focus trap + `aria-modal` above keep KEYBOARD focus in the overlay, but a
// JS trap can't constrain a screen-reader virtual cursor and `aria-modal`
// alone is unevenly honored — so the app-shell chrome the layout renders
// (`MobileContextBar` + `BottomNav`) must physically leave the a11y tree
// while the overlay is up. That chrome lives in `+layout.svelte`, ABOVE this
// host, so we can't `inert` it by prop; instead register the active overlay
// in a shared store the layout reads (the list column, in-DOM here, is
// already inerted directly). Gated on the SAME condition as the trap
// (`viewport.isMobile && openItemRef`); the cleanup releases the ref on
// unmount and on any dep change (breakpoint crossing, `openItemRef`
// clearing), so desktop and closed states never inert the chrome.
$effect(() => {
if (!browser) return;
if (!openItemRef || !viewport.isMobile) return;
paneOverlay.enter();
return () => paneOverlay.leave();
});

// Desktop focusin backstop (PLAN-2154 Architecture C / R1, TASK-2162) — the
// NARROWED desktop mirror of the mobile trap's focusin catch-all above.
// `focusPaneRegion` (fired synchronously at each hop) is the primary R1
Expand Down Expand Up @@ -545,21 +566,30 @@
first paint when there's no stored value.
-->
<!--
`aside` gives the pane complementary-landmark semantics; `aria-label`
names it so a screen reader announces the region on entry (TASK-2122).
`tabindex="-1"` makes it a programmatic focus target (not a Tab stop):
Tab from the list moves focus here, and the mobile overlay moves focus
here on open (then traps it). Desktop keeps focus on the list so j/k
navigation is uninterrupted. Focus-in (mobile), the Tab bridge, ESC
return-to-list, and the mobile focus trap are wired in the effects +
the host's keydown handler.
`aria-label` names the pane so a screen reader announces the region on
entry (TASK-2122). `tabindex="-1"` makes it a programmatic focus target
(not a Tab stop): Tab from the list moves focus here, and the mobile
overlay moves focus here on open (then traps it). Desktop keeps focus on
the list so j/k navigation is uninterrupted. Focus-in (mobile), the Tab
bridge, ESC return-to-list, and the mobile focus trap are wired in the
effects + the host's keydown handler.

Role is viewport-dependent (PLAN-2105 / TASK-2131). On the DESKTOP split
the pane is a non-modal companion to the list, so it stays a bare `<aside>`
(implicit `complementary` landmark) with no `aria-modal`. On MOBILE it's a
full-screen overlay covering everything, so it becomes a `role="dialog"`
`aria-modal="true"` modal — paired with the app-shell chrome being inerted
(the overlay effect above) and the mobile focus trap for a complete modal.
`aria-label` supplies the required accessible name in the dialog case.
-->
<!-- svelte-ignore a11y_no_noninteractive_tabindex -->
<aside
class="item-pane"
bind:this={paneEl}
tabindex="-1"
aria-label="Item detail"
role={viewport.isMobile ? 'dialog' : undefined}
aria-modal={viewport.isMobile ? 'true' : undefined}
style={paneWidth != null ? `--pane-width: ${paneWidth}px` : undefined}
>
{#if paneMintForRoute}
Expand Down
51 changes: 51 additions & 0 deletions web/src/lib/stores/paneOverlay.svelte.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Runs in the jsdom vitest project (filename ends `.svelte.test.ts`), which
// compiles the `.svelte.ts` rune module. `paneOverlay` is a ref-counted signal
// (TASK-2131): PaneHost calls enter()/leave() per active mobile overlay and the
// workspace layout reads `mobileOverlayActive` to inert the app-shell chrome.
// The module holds process-wide `$state`, so each test re-imports a fresh copy.
import { describe, it, expect, beforeEach, vi } from 'vitest';

beforeEach(() => {
// Fresh module (and its process-wide $state count) per test.
vi.resetModules();
});

describe('paneOverlay', () => {
it('starts inactive', async () => {
const { paneOverlay } = await import('./paneOverlay.svelte');
expect(paneOverlay.mobileOverlayActive).toBe(false);
});

it('is active after a single enter() and inactive after the matching leave()', async () => {
const { paneOverlay } = await import('./paneOverlay.svelte');
paneOverlay.enter();
expect(paneOverlay.mobileOverlayActive).toBe(true);
paneOverlay.leave();
expect(paneOverlay.mobileOverlayActive).toBe(false);
});

it('stays active while overlapping overlays are open (ref-counted)', async () => {
// Models a route change where the incoming PaneHost enters before the
// outgoing one's effect cleanup leaves — the signal must not flicker off.
const { paneOverlay } = await import('./paneOverlay.svelte');
paneOverlay.enter(); // outgoing host
paneOverlay.enter(); // incoming host mounts before...
expect(paneOverlay.mobileOverlayActive).toBe(true);
paneOverlay.leave(); // ...outgoing host's cleanup runs
expect(paneOverlay.mobileOverlayActive).toBe(true);
paneOverlay.leave();
expect(paneOverlay.mobileOverlayActive).toBe(false);
});

it('never goes negative on an unmatched leave()', async () => {
// A defensive floor: a stray leave() (double-cleanup) can't drive the
// count below zero and wedge a later enter() into staying inactive.
const { paneOverlay } = await import('./paneOverlay.svelte');
paneOverlay.leave();
expect(paneOverlay.mobileOverlayActive).toBe(false);
paneOverlay.enter();
expect(paneOverlay.mobileOverlayActive).toBe(true);
paneOverlay.leave();
expect(paneOverlay.mobileOverlayActive).toBe(false);
});
});
35 changes: 35 additions & 0 deletions web/src/lib/stores/paneOverlay.svelte.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Ref-counted signal for "a mobile full-screen detail-pane overlay is active"
// (PLAN-2105 / TASK-2131). At ≤768px the detail pane (PaneHost's `.item-pane`)
// restyles to a full-viewport modal overlay covering the app-shell chrome.
// While it's up, the workspace layout marks the chrome siblings it renders —
// `MobileContextBar` + `BottomNav` — `inert` so they leave the focus order AND
// the screen-reader tree. A JS focus trap plus `aria-modal="true"` on the pane
// can't fully constrain an SR virtual cursor, so the background chrome must
// physically drop out of the a11y tree — the same treatment the collection
// host already gives its list column (`inert={viewport.isMobile && openItemRef}`).
// The chrome lives in `+layout.svelte`, ABOVE the pane host, so the host can't
// reach it by prop; it hoists the fact into this store instead.
//
// Ref-counted rather than a bare boolean so a brief overlap of two mounted
// overlays — e.g. a client route change from the collection host to the
// full-page item host where the incoming PaneHost's effect runs before the
// outgoing one's cleanup — can't clear the signal early. PaneHost is the only
// writer (one `enter`/`leave` per active overlay via an `$effect`); the layout
// is the only reader. This one-way split satisfies CONVE-1688 — a store must
// never read state it also writes.
let overlayCount = $state(0);

export const paneOverlay = {
/** True while ≥1 mobile detail-pane overlay is active. Read by the workspace layout. */
get mobileOverlayActive() {
return overlayCount > 0;
},
/** Register a newly-active mobile overlay. Pair every call with `leave()`. */
enter() {
overlayCount += 1;
},
/** Release a mobile overlay that stopped being active (unmount / breakpoint / close). */
leave() {
overlayCount = Math.max(0, overlayCount - 1);
},
};
35 changes: 27 additions & 8 deletions web/src/routes/[username]/[workspace]/+layout.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import { titleStore } from '$lib/stores/title.svelte';
import { authStore } from '$lib/stores/auth.svelte';
import { registerWorkspaceTools, type WebMcpHandle } from '$lib/webmcp/register';
import { paneOverlay } from '$lib/stores/paneOverlay.svelte';
import ConnectBanner from '$lib/components/ConnectBanner.svelte';
import VerifyEmailBanner from '$lib/components/VerifyEmailBanner.svelte';
import BottomNav from '$lib/components/layout/BottomNav.svelte';
Expand Down Expand Up @@ -246,16 +247,34 @@
}
</script>

<VerifyEmailBanner />
<!--
App-shell chrome (PLAN-2105 / TASK-2131). While a mobile detail-pane overlay
is up (`paneOverlay.mobileOverlayActive`, set by PaneHost) every app-shell
sibling the layout renders sits BEHIND the full-screen modal, so all of them
are marked `inert` — out of the focus order and the screen-reader tree.
`aria-modal` on the pane isn't reliably honored on its own, so the background
must physically leave the a11y tree; that has to cover the banners
(VerifyEmailBanner's Resend, ConnectBanner's action) as well as the context
bar / bottom nav, or their controls stay reachable behind the modal. Only
`{@render children()}` — which contains the pane itself — is left interactive.
`display: contents` wrappers carry the `inert` (which cascades to descendants)
without adding a box, so the fixed chrome renders exactly as before. Off mobile
/ pane closed the signal is false and the attribute is absent — no desktop change.
-->
<div style="display: contents" inert={paneOverlay.mobileOverlayActive}>
<VerifyEmailBanner />

<ConnectBanner
{wsSlug}
serverUrl={typeof window !== 'undefined' ? window.location.origin : ''}
workspaceName={workspaceStore.current?.name ?? ''}
/>
<ConnectBanner
{wsSlug}
serverUrl={typeof window !== 'undefined' ? window.location.origin : ''}
workspaceName={workspaceStore.current?.name ?? ''}
/>

<MobileContextBar />
<MobileContextBar />
</div>

{@render children()}

<BottomNav />
<div style="display: contents" inert={paneOverlay.mobileOverlayActive}>
<BottomNav />
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -2189,9 +2189,12 @@
// (not bare `dialog`) because `Modal.svelte`'s native <dialog> is
// ALWAYS mounted and toggled via `showModal()`/`close()` — an
// existence check without `[open]` would false-positive on every page
// that merely HAS a Modal instance, open or not. The pane (<aside>)
// and graph drawer aren't dialogs, so this never blocks the chain.
if (document.querySelector('dialog[open], [role="dialog"]')) return;
// that merely HAS a Modal instance, open or not. The pane itself now
// carries `role="dialog"` on the mobile overlay (TASK-2131), so it is
// EXCLUDED via `:not(.item-pane)` — its ESC is owned by its own escape-
// stack handler, and counting it here would swallow ESC-to-close / pop.
// The graph drawer isn't a dialog, so the chain is otherwise unblocked.
if (document.querySelector('dialog[open], [role="dialog"]:not(.item-pane)')) return;
// A HELD key fires many auto-repeat keydowns (`e.repeat === true`).
// Consumed here, BEFORE any layer-close/pop decision, so a hold can
// never cascade through the chain — only the initial physical press
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,8 +430,11 @@
const target = e.target as HTMLElement | null;
// Text-editing targets own ESC locally — don't hijack into a layer-close.
if (isTextEntryTarget(target)) return;
// A native <dialog> / role="dialog" sheet owns its own ESC.
if (document.querySelector('dialog[open], [role="dialog"]')) return;
// A native <dialog> / role="dialog" sheet owns its own ESC. The pane's own
// mobile overlay is `role="dialog"` too (TASK-2131) but is EXCLUDED via
// `:not(.item-pane)` — it's the layer this ESC is meant to close, handled
// through the shared escape stack, not a foreign modal to defer to.
if (document.querySelector('dialog[open], [role="dialog"]:not(.item-pane)')) return;
// A HELD key auto-repeats; only the initial physical press acts.
if (e.repeat) {
e.preventDefault();
Expand Down
Loading