fix(e4): address deep-review findings — bookmark popstate, focus trap, touch a11y
Deep-review (6 dimensions, 3-verifier adversarial pass) surfaced 7 findings, all verified; fixed here. - NavigationProvider + history.ts (medium, correctness): the popstate handlers restored the open channel / Plex sub-view from history.state ONLY, but a fresh ?chan / ?v bookmark lives in the URL with no marker in state — so any overlay's programmatic history.back() (closing a modal/player) silently closed the bookmarked channel or collapsed the sub-view to the grid. Give onPop the same URL fallback the initializers already use (history.ts extracts a shared readView so seed and popstate can't diverge again). - plexCards SeasonCard (medium, a11y): the "In progress" season badge was a reveal-idle swap-badge → display:none on touch, and a season has no progress bar, so an in-progress season was indistinguishable from unwatched on touch. Moved it to the top-left (the toggle owns top-right) and made it always visible. - plexCards watched toggle (low, a11y / WCAG 1.4.1): watched state was color-only on touch (green vs white CheckCircle2). New WatchedIcon distinguishes by SHAPE — CheckCircle2 (watched) vs an empty Circle (not) — used by all three card toggles. - Modal.tsx (low, a11y): the focus trap leaked on Shift+Tab when focus rested on the panel container itself (Node.contains(self) is true, so the pull-back branch never ran). Guard now also checks `active === panel`. - index.css (low, a11y): .reveal-idle faded only on .group:hover though .reveal is revealed on hover AND focus-within → the badge overlapped the control on keyboard focus. Added the focus-within fade. - PlexBrowse.tsx (low, correctness): the ?v=playlist decoder accepted a non-integer id (Number.isFinite); tightened to Number.isInteger(n) && n > 0, matching the ?pl guard. Gate green (tsc, knip, lint 0-err, 247 vitest). Self-E2E (localdev): ?v=show and ?chan bookmarks survive a modal open+close (were collapsing/closing before); Modal Shift+Tab from the panel stays inside; in-progress season badge visible on touch; watched toggle shows an empty-circle-vs-check shape difference; 0 console errors.
This commit is contained in:
@@ -73,8 +73,10 @@ export default function Modal({
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const active = document.activeElement;
|
const active = document.activeElement;
|
||||||
if (!panel.contains(active)) {
|
if (!panel.contains(active) || active === panel) {
|
||||||
// Focus escaped the dialog (or sits on the panel itself) — pull it back to an edge.
|
// Focus escaped the dialog, OR sits on the panel container itself (tabIndex=-1, reached by
|
||||||
|
// clicking dialog dead space) — Node.contains(self) is true, so `active === panel` must be
|
||||||
|
// checked explicitly or Shift+Tab from the panel would leak to the background. Pull it back.
|
||||||
e.preventDefault();
|
e.preventDefault();
|
||||||
(e.shiftKey ? last : first).focus();
|
(e.shiftKey ? last : first).focus();
|
||||||
} else if (e.shiftKey && active === first) {
|
} else if (e.shiftKey && active === first) {
|
||||||
|
|||||||
@@ -244,7 +244,14 @@ export function NavigationProvider({ children }: { children: ReactNode }) {
|
|||||||
// The YouTube-search sub-view and the channel page ride in history.state, so Back/Forward
|
// The YouTube-search sub-view and the channel page ride in history.state, so Back/Forward
|
||||||
// restores or clears them to match the entry we landed on.
|
// restores or clears them to match the entry we landed on.
|
||||||
setYtSearch((e.state?._yt as string) ?? null);
|
setYtSearch((e.state?._yt as string) ?? null);
|
||||||
const chan = e.state?._chan as string | undefined;
|
// Fall back to the ?chan URL param (the browser restored the landed entry's URL) when the entry
|
||||||
|
// has no _chan — a fresh ?chan bookmark's entry is URL-only, and reading e.state alone would
|
||||||
|
// wrongly close the channel when a popstate lands here (e.g. after an overlay's programmatic
|
||||||
|
// history.back()). Mirrors the channelView initializer's URL fallback.
|
||||||
|
const chan =
|
||||||
|
(e.state?._chan as string | undefined) ??
|
||||||
|
new URLSearchParams(window.location.search).get("chan") ??
|
||||||
|
undefined;
|
||||||
setChannelView(chan ? { id: chan, name: (e.state?._chanName as string) ?? undefined } : null);
|
setChannelView(chan ? { id: chan, name: (e.state?._chanName as string) ?? undefined } : null);
|
||||||
}
|
}
|
||||||
window.addEventListener("popstate", onPop);
|
window.addEventListener("popstate", onPop);
|
||||||
|
|||||||
@@ -77,8 +77,12 @@ const PLEX_SUB_URL: SubviewUrl<Sub> = {
|
|||||||
return arg ? { kind: "info", id: arg } : null;
|
return arg ? { kind: "info", id: arg } : null;
|
||||||
case "person":
|
case "person":
|
||||||
return arg ? { kind: "person", name: arg } : null;
|
return arg ? { kind: "person", name: arg } : null;
|
||||||
case "playlist":
|
case "playlist": {
|
||||||
return arg && Number.isFinite(Number(arg)) ? { kind: "playlist", id: Number(arg) } : null;
|
// Positive-integer id only — a fractional/zero/negative ?v=playlist:<arg> would open a
|
||||||
|
// sub-view for a non-existent id (matches the ?pl guard in PlaylistsProvider.readSelectedId).
|
||||||
|
const id = arg ? Number(arg) : NaN;
|
||||||
|
return Number.isInteger(id) && id > 0 ? { kind: "playlist", id } : null;
|
||||||
|
}
|
||||||
case "season": {
|
case "season": {
|
||||||
const [showId, seasonId] = arg.split(":");
|
const [showId, seasonId] = arg.split(":");
|
||||||
return showId && seasonId ? { kind: "season", showId, seasonId } : null;
|
return showId && seasonId ? { kind: "season", showId, seasonId } : null;
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { useTranslation } from "react-i18next";
|
import { useTranslation } from "react-i18next";
|
||||||
import { CheckCircle2, Film, Info, ListPlus, Play, Tv2 } from "lucide-react";
|
import { CheckCircle2, Circle, Film, Info, ListPlus, Play, Tv2 } from "lucide-react";
|
||||||
import { type CSSProperties } from "react";
|
import { type CSSProperties } from "react";
|
||||||
import { formatRuntime } from "../lib/format";
|
import { formatRuntime } from "../lib/format";
|
||||||
import type { PlexCard, PlexCastMember, PlexFilters, PlexSeasonDetail } from "../lib/api";
|
import type { PlexCard, PlexCastMember, PlexFilters, PlexSeasonDetail } from "../lib/api";
|
||||||
@@ -24,6 +24,18 @@ const PLAYABLE_TINT: Record<string, string> = {
|
|||||||
const swapBadgeReveal = (hasControl: boolean): string =>
|
const swapBadgeReveal = (hasControl: boolean): string =>
|
||||||
hasControl ? "reveal-idle" : "group-hover:opacity-0";
|
hasControl ? "reveal-idle" : "group-hover:opacity-0";
|
||||||
|
|
||||||
|
// Watched-toggle icon. Distinguishes state by SHAPE (a filled check vs an empty circle), not color
|
||||||
|
// alone — on touch the toggle is the only surviving watched indicator (the text pill is hidden), so a
|
||||||
|
// green-vs-white CheckCircle2 would be a use-of-color (WCAG 1.4.1) gap. Watched = CheckCircle2 (green),
|
||||||
|
// not-watched = Circle (empty, white).
|
||||||
|
function WatchedIcon({ watched }: { watched: boolean }) {
|
||||||
|
return watched ? (
|
||||||
|
<CheckCircle2 className="w-4 h-4 text-success" />
|
||||||
|
) : (
|
||||||
|
<Circle className="w-4 h-4 text-white" />
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
export function PlexPosterCard({
|
export function PlexPosterCard({
|
||||||
card,
|
card,
|
||||||
onOpen,
|
onOpen,
|
||||||
@@ -90,9 +102,7 @@ export function PlexPosterCard({
|
|||||||
title={card.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")}
|
title={card.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")}
|
||||||
className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition"
|
className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition"
|
||||||
>
|
>
|
||||||
<CheckCircle2
|
<WatchedIcon watched={card.status === "watched"} />
|
||||||
className={`w-4 h-4 ${card.status === "watched" ? "text-success" : "text-white"}`}
|
|
||||||
/>
|
|
||||||
</button>
|
</button>
|
||||||
)}
|
)}
|
||||||
{/* Media info page (movies/episodes). */}
|
{/* Media info page (movies/episodes). */}
|
||||||
@@ -219,7 +229,7 @@ export function SeasonCard({
|
|||||||
}
|
}
|
||||||
className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition"
|
className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition"
|
||||||
>
|
>
|
||||||
<CheckCircle2 className={`w-4 h-4 ${watched ? "text-success" : "text-white"}`} />
|
<WatchedIcon watched={watched} />
|
||||||
</button>
|
</button>
|
||||||
)}
|
)}
|
||||||
{/* Add whole season to a playlist, on hover. */}
|
{/* Add whole season to a playlist, on hover. */}
|
||||||
@@ -244,12 +254,12 @@ export function SeasonCard({
|
|||||||
{t("plex.watched")}
|
{t("plex.watched")}
|
||||||
</span>
|
</span>
|
||||||
)}
|
)}
|
||||||
|
{/* In-progress lives at the top-LEFT (the watched-toggle owns the top-right) and stays visible
|
||||||
|
on touch: unlike movie/episode cards, a season has no progress bar, and the always-shown
|
||||||
|
toggle reads as unwatched for a partial season — so this badge is the sole in-progress cue
|
||||||
|
and must not be a reveal-idle swap-badge (which display:none-s on touch). */}
|
||||||
{se.status === "in_progress" && (
|
{se.status === "in_progress" && (
|
||||||
<span
|
<span className="pointer-events-none absolute top-1.5 left-1.5 text-3xs bg-black/75 text-accent px-1.5 py-0.5 rounded-sm">
|
||||||
className={`pointer-events-none absolute top-1.5 right-1.5 text-3xs bg-black/75 text-accent px-1.5 py-0.5 rounded-sm ${swapBadgeReveal(
|
|
||||||
!!onToggleWatched,
|
|
||||||
)}`}
|
|
||||||
>
|
|
||||||
{t("plex.inProgress")}
|
{t("plex.inProgress")}
|
||||||
</span>
|
</span>
|
||||||
)}
|
)}
|
||||||
@@ -430,9 +440,7 @@ export function EpisodeCard({
|
|||||||
title={ep.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")}
|
title={ep.status === "watched" ? t("plex.markUnwatched") : t("plex.markWatched")}
|
||||||
className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition"
|
className="absolute top-1.5 right-1.5 p-1 rounded-full bg-black/60 reveal hover:bg-black/80 transition"
|
||||||
>
|
>
|
||||||
<CheckCircle2
|
<WatchedIcon watched={ep.status === "watched"} />
|
||||||
className={`w-4 h-4 ${ep.status === "watched" ? "text-success" : "text-white"}`}
|
|
||||||
/>
|
|
||||||
</button>
|
</button>
|
||||||
)}
|
)}
|
||||||
{ep.status === "watched" && (
|
{ep.status === "watched" && (
|
||||||
|
|||||||
@@ -558,7 +558,11 @@ body {
|
|||||||
.reveal:focus-visible {
|
.reveal:focus-visible {
|
||||||
opacity: 1;
|
opacity: 1;
|
||||||
}
|
}
|
||||||
.group:hover .reveal-idle {
|
/* Fade the idle badge whenever its companion control is revealed — on hover AND on keyboard
|
||||||
|
focus-within (the `.reveal` trigger set above lists both), else the two overlap at the same
|
||||||
|
corner for a keyboard user. */
|
||||||
|
.group:hover .reveal-idle,
|
||||||
|
.group:focus-within .reveal-idle {
|
||||||
opacity: 0;
|
opacity: 0;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -32,8 +32,12 @@ export function useHistorySubview<T>(
|
|||||||
open: (next: T) => void;
|
open: (next: T) => void;
|
||||||
back: () => void;
|
back: () => void;
|
||||||
} {
|
} {
|
||||||
const [view, setView] = useState<T>(() => {
|
// Resolve the current entry's sub-view: history.state._sub for an in-app entry, else decode the
|
||||||
// Reload of an in-app entry: history.state._sub. Fresh bookmark: decode the ?<key> param.
|
// ?<key> URL param for a fresh bookmark (whose entry carries the param but no _sub), else root.
|
||||||
|
// ⚠️ The seed AND the popstate handler must use THIS same fallback: a popstate can land on a
|
||||||
|
// bookmark entry (URL-only, no _sub) — e.g. after an overlay's programmatic history.back() — and
|
||||||
|
// reading history.state alone would wrongly collapse the bookmarked sub-view to root.
|
||||||
|
const readView = (): T => {
|
||||||
const fromState = window.history.state?._sub as T | undefined;
|
const fromState = window.history.state?._sub as T | undefined;
|
||||||
if (fromState != null) return fromState;
|
if (fromState != null) return fromState;
|
||||||
if (url) {
|
if (url) {
|
||||||
@@ -42,13 +46,15 @@ export function useHistorySubview<T>(
|
|||||||
if (decoded != null) return decoded;
|
if (decoded != null) return decoded;
|
||||||
}
|
}
|
||||||
return root;
|
return root;
|
||||||
});
|
};
|
||||||
|
const readViewRef = useRef(readView);
|
||||||
|
readViewRef.current = readView;
|
||||||
|
const [view, setView] = useState<T>(readView);
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
// Back/Forward restores the sub-view from history.state (the browser restores each entry's URL too).
|
// Back/Forward restores the sub-view from the landed entry (state OR its restored ?<key> URL).
|
||||||
const onPop = () => setView((window.history.state?._sub as T) ?? root);
|
const onPop = () => setView(readViewRef.current());
|
||||||
window.addEventListener("popstate", onPop);
|
window.addEventListener("popstate", onPop);
|
||||||
return () => window.removeEventListener("popstate", onPop);
|
return () => window.removeEventListener("popstate", onPop);
|
||||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
|
||||||
}, []);
|
}, []);
|
||||||
return {
|
return {
|
||||||
view,
|
view,
|
||||||
|
|||||||
Reference in New Issue
Block a user