Epic-end deep-review of the branch surfaced six verified findings; fix them. a11y (medium): FilterSelect and SortControl opened their AnchoredPopover in mode:"listbox" but never passed a role, so the surface fell through to role="menu" while the rows rendered role="option"+aria-selected — options orphaned outside a listbox, so AT could not announce the selected value. Switch both to the house menu pattern (as ViewSwitcher already uses): a menu surface with menuitemradio (single) / menuitemcheckbox (multi) rows carrying aria-checked, scoped by a role=group. Trigger stays a button+haspopup=menu. a11y (low): the SortControl asc/desc direction segment signalled the active direction by colour alone. Add aria-pressed so the current direction is programmatically exposed, matching SegmentedToggle. correctness (low): PageShell converted a rem maxWidth to px with a hardcoded 16 to decide the pinned-rail overlay threshold, but index.css scales the root font by --font-scale (default 1.06 ~ 17px, slider 0.9-1.3), so the threshold was wrong at every text size on the rem pages (playlists is the only one with a pinned rail). Use the live root font-size, scale the 36rem slack the same way, and recompute when --font-scale changes (a guarded MutationObserver — the slider fires no resize). tests: cover the two gaps the review flagged — ItemCount (plural, 1M+ overflow, undefined icon-only, tooltip) and the PageShell rail-pinned-w/content-mw threshold math incl. the font-scale scaling. Updated the FilterSelect/ SortControl role queries to the new menuitemradio/checkbox roles. Verified in the running app: menu surfaces carry role=group + menuitemradio/ checkbox with 0 orphaned options, and the sort direction segment exposes aria-pressed. Dropped two review findings (max-width duplication, pill-class reuse) as refuted on adversarial verification. Gate green: tsc, 372 vitest, prettier, eslint (0 err).
161 lines
8.5 KiB
TypeScript
161 lines
8.5 KiB
TypeScript
import {
|
||
cloneElement,
|
||
createContext,
|
||
isValidElement,
|
||
useContext,
|
||
useLayoutEffect,
|
||
useState,
|
||
type ReactElement,
|
||
type ReactNode,
|
||
type Ref,
|
||
} from "react";
|
||
import { createPortal } from "react-dom";
|
||
import PageScroller from "./PageScroller";
|
||
|
||
// The DOM node a module's toolbar portals into (see PageToolbar below). PageShell provides its
|
||
// fixed band as the default target; a module can override it with its own <PageToolbarSlot.Provider>
|
||
// to redirect its toolbar somewhere else — the channel page points it at a sticky sub-header so the
|
||
// toolbar rides with the tabs instead of the shell's top band.
|
||
export const PageToolbarSlot = createContext<HTMLElement | null>(null);
|
||
|
||
// A module renders its per-module fixed band (filter chips / sort / counts) through this. The
|
||
// content lands in the shell's toolbar band — ABOVE the scroller, so it stays put while the content
|
||
// scrolls and is never touched by the scroller's edge-fade mask. It's a portal, so the toolbar's
|
||
// event handlers and closures still belong to the module's React subtree (setFilters etc. work as
|
||
// if it were rendered inline). Renders nothing until the band mounts (one tick on first mount) and
|
||
// nothing when there's no shell — so a module works with or without one.
|
||
export function PageToolbar({ children }: { children: ReactNode }) {
|
||
const slot = useContext(PageToolbarSlot);
|
||
return slot ? createPortal(children, slot) : null;
|
||
}
|
||
|
||
// The page shell: the fixed chrome around a single scrollable content region. It formalizes "what
|
||
// is fixed vs what scrolls" per module, so the chrome App used to hand-assemble around every page
|
||
// (a floating Header + a top-padded wrapper + PageScroller, plus the channel page's separate
|
||
// branch that rendered its OWN scroller) collapses to one contract and one scroller.
|
||
//
|
||
// Regions, top to bottom:
|
||
// - `header` — floats (absolute, height `var(--hdr-h)`) over the content column; the wrapper pads
|
||
// its top by that height so nothing hides under it. Omit it (channel page) → no pad
|
||
// and no fixed top band, so the module's own banner can scroll from the very top.
|
||
// - `banners` — in-flow, fixed notices (demo/version) above the scroll region.
|
||
// - toolbar band — in-flow, FIXED per-module band (filter chips / sort / counts); filled by a
|
||
// module via <PageToolbar>. Empty ⇒ zero height, so pages without one lose nothing.
|
||
// - children — the ONE scrollable region. `PageScroller` owns the scroller and its two
|
||
// invariants (`scrollbar-gutter: stable`, edge fade); see that file.
|
||
//
|
||
// The shell renders a fragment so it drops straight into App's `relative` content column — the
|
||
// floating header keeps resolving its `absolute` position against that same ancestor.
|
||
export default function PageShell({
|
||
header,
|
||
banners,
|
||
children,
|
||
scrollKey,
|
||
fadeTop = true,
|
||
maxWidth,
|
||
}: {
|
||
header?: ReactNode;
|
||
banners?: ReactNode;
|
||
children: ReactNode;
|
||
// Identifies the logical view for the scroll restore/reset policy — see PageScroller.
|
||
scrollKey?: string;
|
||
// Off when this page's scroll region starts with a sticky header — see PageScroller.
|
||
fadeTop?: boolean;
|
||
// The content column's max width (e.g. "1600px"); the header + toolbar centre on the same width via
|
||
// --content-mw. Omit for a full-width page (feed). See index.css --content-mw.
|
||
maxWidth?: string;
|
||
}) {
|
||
const [toolbarSlot, setToolbarSlot] = useState<HTMLDivElement | null>(null);
|
||
// Publish this page's content max-width so the floating header (and toolbar band) can centre on the
|
||
// same width the content column uses. Global (one page shell renders at a time). Also decide whether
|
||
// a PINNED filter rail should stay OVERLAY (keep the toolbar's fixed leading gutter, don't push the
|
||
// centred content) or go in-flow: overlay only when the centred content leaves a wide-enough left
|
||
// margin for the rail to sit in — else pinning would overlap the content, so it pushes (in-flow).
|
||
useLayoutEffect(() => {
|
||
const root = document.documentElement;
|
||
root.style.setProperty("--content-mw", maxWidth ?? "none");
|
||
// Room the pinned rail needs: its own width (~16rem) on each side of the centred content, plus the
|
||
// nav rail (~4rem) = ~36rem. Below that, or on a full-width page (no maxWidth), pin in-flow (w-67).
|
||
let lastPxPerRem = 0;
|
||
const apply = () => {
|
||
// Convert the rem maxWidth (and the 36rem slack) against the LIVE root font-size, not a hardcoded
|
||
// 16: index.css scales the root by --font-scale (default 1.06 ≈ 17px, slider 0.9–1.3), and the page's
|
||
// own content wrapper is a rem `max-w-*`, so a fixed 16 mis-sizes the threshold at every text size.
|
||
const pxPerRem = parseFloat(getComputedStyle(root).fontSize) || 16;
|
||
lastPxPerRem = pxPerRem;
|
||
const mwPx = maxWidth
|
||
? maxWidth.endsWith("rem")
|
||
? parseFloat(maxWidth) * pxPerRem
|
||
: parseFloat(maxWidth)
|
||
: null;
|
||
const roomy = mwPx != null && window.innerWidth >= mwPx + 36 * pxPerRem;
|
||
root.style.setProperty("--rail-pinned-w", roomy ? "2.25rem" : "16.75rem");
|
||
};
|
||
apply();
|
||
window.addEventListener("resize", apply);
|
||
// The text-size slider changes --font-scale (hence the root font-size) WITHOUT firing resize, so the
|
||
// rem→px threshold would go stale. Re-run when the root font actually changes; the pxPerRem guard keeps
|
||
// our own --rail-pinned-w write (a rem string, no font change) from re-triggering the observer.
|
||
const fontObserver = new MutationObserver(() => {
|
||
if ((parseFloat(getComputedStyle(root).fontSize) || 16) !== lastPxPerRem) apply();
|
||
});
|
||
fontObserver.observe(root, { attributes: true, attributeFilter: ["style"] });
|
||
return () => {
|
||
window.removeEventListener("resize", apply);
|
||
fontObserver.disconnect();
|
||
root.style.removeProperty("--content-mw");
|
||
root.style.removeProperty("--rail-pinned-w");
|
||
};
|
||
}, [maxWidth]);
|
||
// The floating header pads the content below it by `--hdr-h`. That must equal the header's REAL
|
||
// height, which grows as the flex-wrap header wraps to 2–3 rows on a narrow window — a fixed pad
|
||
// would let the content toolbar slide up under it (E5 S7). Measure the header and write its height
|
||
// straight onto the content wrapper that consumes `--hdr-h`.
|
||
//
|
||
// Both nodes are held in STATE (callback refs), NOT plain refs behind a one-shot `useLayoutEffect([])`:
|
||
// PageShell is reused — never remounted — across feed↔channel navigation (same component position in
|
||
// App.tsx), so the header DOM node is swapped out from under a stable PageShell. A one-shot effect keeps
|
||
// observing the FIRST header node; when that node detaches on channel-open, its ResizeObserver fires with
|
||
// a 0 box and rewrites --hdr-h to `0+0+4`=4px on the (still-mounted) content div. On Back the pad class
|
||
// returns with that stale 4px → the content toolbar slides up under the header, stuck until reload. Keying
|
||
// the measurement to the live node re-runs it every time the header mounts/unmounts, so it stays correct.
|
||
const [headerEl, setHeaderEl] = useState<HTMLDivElement | null>(null);
|
||
const [contentEl, setContentEl] = useState<HTMLDivElement | null>(null);
|
||
useLayoutEffect(() => {
|
||
if (!headerEl || !contentEl) {
|
||
// No header (channel page) → drop any stale pad so a leftover value can't linger.
|
||
contentEl?.style.removeProperty("--hdr-h");
|
||
return;
|
||
}
|
||
// `+ 4` = a tight gap below the floating header before the content toolbar starts.
|
||
const apply = () =>
|
||
contentEl.style.setProperty("--hdr-h", `${headerEl.offsetTop + headerEl.offsetHeight + 4}px`);
|
||
apply();
|
||
const ro = new ResizeObserver(apply);
|
||
ro.observe(headerEl);
|
||
return () => ro.disconnect();
|
||
}, [headerEl, contentEl]);
|
||
const measuredHeader = isValidElement(header)
|
||
? cloneElement(header as ReactElement<{ ref?: Ref<HTMLDivElement> }>, {
|
||
ref: setHeaderEl,
|
||
})
|
||
: header;
|
||
return (
|
||
<PageToolbarSlot.Provider value={toolbarSlot}>
|
||
{measuredHeader}
|
||
<div
|
||
ref={setContentEl}
|
||
className={`flex-1 min-w-0 min-h-0 flex flex-col ${header ? "pt-(--hdr-h)" : ""}`}
|
||
>
|
||
{banners}
|
||
{/* pr-[--sbw]: the toolbar band isn't the scroller, so without this it would run the
|
||
scrollbar-gutter's width past the scrolled cards on the right. Pad it back to card width. */}
|
||
<div ref={setToolbarSlot} className="shrink-0 pr-[var(--sbw)]" />
|
||
<PageScroller scrollKey={scrollKey} fadeTop={fadeTop}>
|
||
{children}
|
||
</PageScroller>
|
||
</div>
|
||
</PageToolbarSlot.Provider>
|
||
);
|
||
}
|