fix(a11y): touch/keyboard reveal tail + single-Escape layer coordination (UX-7/UX-8)
UX-8: one Escape must close only the topmost layer, never both an open popover AND the player beneath it. The popover's Escape listener is on `document` and fires before the player's on `window`; closing the popover flushes its layer off the shared stack synchronously, so the player's `anyLayerAbove()` lookup read "nothing above" and closed too. Add a consumed-event flag to layerStack (`markEscapeHandled`/`escapeHandled`): the acting topmost layer stamps the shared KeyboardEvent, and both players check the stamp before the (now-mutated) stack. Immune to listener order and mid-dispatch stack mutation. Cover with layerStack.dom.test.ts. UX-7: the E4 `.reveal` sweep already covered the card action rows; two residual hover-only traps remained — invisible-but-tappable/Tab-focusable on touch. Add a group-agnostic `.reveal-hover` recipe (display:none under `(hover: none)`) paired with each site's own hover/focus-within reveal: - VideoCard play/Continue/Restart overlay: also reveal on group-focus-within (keyboard), and drop it on touch — the card tap already plays/resumes, so no permanent scrim over every mobile thumbnail. - Carousel prev/next arrows: gone on touch, where the strip swipes.
This commit is contained in:
@@ -77,8 +77,10 @@ export function Carousel({
|
|||||||
|
|
||||||
const label = ariaLabel ?? (typeof title === "string" ? title : undefined);
|
const label = ariaLabel ?? (typeof title === "string" ? title : undefined);
|
||||||
|
|
||||||
|
// Mouse-only: hidden until the carousel is hovered or the arrow is keyboard-focused; `reveal-hover`
|
||||||
|
// removes it on touch, where the strip scrolls by swipe — so an invisible edge button never eats a tap.
|
||||||
const arrowBtn =
|
const arrowBtn =
|
||||||
"absolute top-1/2 -translate-y-1/2 z-10 grid h-9 w-9 place-items-center rounded-full " +
|
"reveal-hover absolute top-1/2 -translate-y-1/2 z-10 grid h-9 w-9 place-items-center rounded-full " +
|
||||||
"bg-black/55 text-white backdrop-blur-xs border border-white/15 shadow-lg transition " +
|
"bg-black/55 text-white backdrop-blur-xs border border-white/15 shadow-lg transition " +
|
||||||
"opacity-0 group-hover/car:opacity-100 focus-visible:opacity-100 hover:bg-black/75";
|
"opacity-0 group-hover/car:opacity-100 focus-visible:opacity-100 hover:bg-black/75";
|
||||||
|
|
||||||
|
|||||||
@@ -31,7 +31,7 @@ import {
|
|||||||
import Avatar from "./Avatar";
|
import Avatar from "./Avatar";
|
||||||
import AddToPlaylist from "./AddToPlaylist";
|
import AddToPlaylist from "./AddToPlaylist";
|
||||||
import { PopoverSurface, useAnchoredPopover } from "./AnchoredPopover";
|
import { PopoverSurface, useAnchoredPopover } from "./AnchoredPopover";
|
||||||
import { pushLayer, removeLayer, anyLayerAbove } from "../lib/layerStack";
|
import { pushLayer, removeLayer, anyLayerAbove, escapeHandled } from "../lib/layerStack";
|
||||||
import DownloadButton from "./DownloadButton";
|
import DownloadButton from "./DownloadButton";
|
||||||
import { api, type Video, type VideoStatus } from "../lib/api";
|
import { api, type Video, type VideoStatus } from "../lib/api";
|
||||||
import type { YTPlayer } from "../lib/youtube";
|
import type { YTPlayer } from "../lib/youtube";
|
||||||
@@ -337,7 +337,10 @@ export default function PlayerModal({
|
|||||||
if (e.key === "Escape") {
|
if (e.key === "Escape") {
|
||||||
// Defer to any layer opened above the player FIRST (a Modal like the download dialog, or an
|
// Defer to any layer opened above the player FIRST (a Modal like the download dialog, or an
|
||||||
// anchored popover): it takes the Escape and we do nothing — so we don't also step out of
|
// anchored popover): it takes the Escape and we do nothing — so we don't also step out of
|
||||||
// maximise or close the player underneath it.
|
// maximise or close the player underneath it. The consumed-event flag is the reliable signal
|
||||||
|
// (UX-8): the layer's `document` listener runs before ours and its close can pop it off the
|
||||||
|
// stack synchronously, so `anyLayerAbove` alone would miss it and close the player too.
|
||||||
|
if (escapeHandled(e)) return;
|
||||||
if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) return;
|
if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) return;
|
||||||
// In fullscreen, let the browser's Esc exit fullscreen only — don't also close the modal.
|
// In fullscreen, let the browser's Esc exit fullscreen only — don't also close the modal.
|
||||||
if (fullscreenElement()) return;
|
if (fullscreenElement()) return;
|
||||||
|
|||||||
@@ -53,7 +53,7 @@ import { LS, useAccountPersistedObject } from "../lib/storage";
|
|||||||
import { useDismiss } from "../lib/useDismiss";
|
import { useDismiss } from "../lib/useDismiss";
|
||||||
import { useScrollFade } from "../lib/useScrollFade";
|
import { useScrollFade } from "../lib/useScrollFade";
|
||||||
import { useBackToClose } from "../lib/history";
|
import { useBackToClose } from "../lib/history";
|
||||||
import { pushLayer, removeLayer, anyLayerAbove } from "../lib/layerStack";
|
import { pushLayer, removeLayer, anyLayerAbove, escapeHandled } from "../lib/layerStack";
|
||||||
|
|
||||||
// The rich info overlay (poster/cast/ratings) reuses the same component as the card's info page.
|
// The rich info overlay (poster/cast/ratings) reuses the same component as the card's info page.
|
||||||
const PlexInfo = lazy(() => import("./PlexInfo"));
|
const PlexInfo = lazy(() => import("./PlexInfo"));
|
||||||
@@ -602,7 +602,10 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) {
|
|||||||
case "Escape":
|
case "Escape":
|
||||||
// Defer to any layer opened above the player (Modal or anchored popover) — let it take the
|
// Defer to any layer opened above the player (Modal or anchored popover) — let it take the
|
||||||
// Escape, not us. A player MENU registers as a layer too, so it closes first and the next
|
// Escape, not us. A player MENU registers as a layer too, so it closes first and the next
|
||||||
// Escape reaches the auto-skip-cancel below (the old blanket-stopPropagation bug).
|
// Escape reaches the auto-skip-cancel below (the old blanket-stopPropagation bug). Check the
|
||||||
|
// consumed-event flag first (UX-8): a layer's `document` listener runs before this `window`
|
||||||
|
// one and its close can pop it off the stack synchronously, so `anyLayerAbove` alone misses it.
|
||||||
|
if (escapeHandled(e)) break;
|
||||||
if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) break;
|
if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) break;
|
||||||
if (skipProgressRef.current != null) cancelAutoSkipRef.current();
|
if (skipProgressRef.current != null) cancelAutoSkipRef.current();
|
||||||
else if (menuOpenRef.current) {
|
else if (menuOpenRef.current) {
|
||||||
|
|||||||
@@ -262,9 +262,12 @@ function Thumb({
|
|||||||
<Bookmark className="w-3.5 h-3.5" />
|
<Bookmark className="w-3.5 h-3.5" />
|
||||||
</span>
|
</span>
|
||||||
)}
|
)}
|
||||||
{/* Hover overlay: Play everywhere; Continue + Restart once the video is in progress. */}
|
{/* Hover overlay: Play everywhere; Continue + Restart once the video is in progress. Mouse-only:
|
||||||
|
reveal on hover AND on keyboard focus-within (else a Tab-focused button is invisible), and
|
||||||
|
`reveal-hover` drops it entirely on touch — the card tap already plays/resumes there, so a
|
||||||
|
permanent dark scrim over every mobile thumbnail (and an invisible tap target) is not wanted. */}
|
||||||
{onOpen && (
|
{onOpen && (
|
||||||
<div className="absolute inset-0 flex items-center justify-center gap-2 bg-black/45 opacity-0 group-hover:opacity-100 transition-opacity">
|
<div className="absolute inset-0 flex items-center justify-center gap-2 bg-black/45 opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 transition-opacity reveal-hover">
|
||||||
{inProgress ? (
|
{inProgress ? (
|
||||||
<>
|
<>
|
||||||
<button
|
<button
|
||||||
|
|||||||
@@ -581,6 +581,16 @@ body {
|
|||||||
.reveal-idle {
|
.reveal-idle {
|
||||||
display: none;
|
display: none;
|
||||||
}
|
}
|
||||||
|
/* Sibling of `.reveal` for a MOUSE-ONLY hover-reveal: unlike `.reveal` (shown at rest on touch),
|
||||||
|
a `.reveal-hover` control is removed entirely on a device that can't hover, so it is never an
|
||||||
|
invisible tap/Tab target. Pair it with the site's own `opacity-0 group-hover:… group-focus-within:…`
|
||||||
|
(which still governs the hover + keyboard case). For a full-card/edge affordance whose action is
|
||||||
|
already reachable on touch another way — VideoCard's play overlay (tapping the card plays/resumes)
|
||||||
|
and the Carousel arrows (swiping scrolls the strip) — where a permanent scrim/edge button is not
|
||||||
|
wanted. Group-agnostic (encodes only the touch removal), so it works under any `.group`/`group\/…`. */
|
||||||
|
.reveal-hover {
|
||||||
|
display: none;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/* Dark mode: the ambient accent pools are faint over a near-black bg, so the docked frosted
|
/* Dark mode: the ambient accent pools are faint over a near-black bg, so the docked frosted
|
||||||
|
|||||||
@@ -0,0 +1,79 @@
|
|||||||
|
// @vitest-environment jsdom
|
||||||
|
//
|
||||||
|
// Guards the UX-8 fix: one Escape must close only the TOPMOST layer, never both an open popover AND
|
||||||
|
// the player beneath it. The bug was that the popover's Escape listener (on `document`) fires before
|
||||||
|
// the player's (on `window`), and closing the popover flushes its layer off the stack synchronously —
|
||||||
|
// so by the time the player's `anyLayerAbove()` lookup runs, the stack already reads "nothing above"
|
||||||
|
// and the player closes too. The fix stamps the shared KeyboardEvent when a layer acts; the player
|
||||||
|
// reads the stamp instead of the (now-mutated) stack. This test reproduces that exact ordering with
|
||||||
|
// the stack primitives — the layer removes itself between the two listeners, mimicking the flush.
|
||||||
|
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||||
|
import {
|
||||||
|
pushLayer,
|
||||||
|
removeLayer,
|
||||||
|
anyLayerAbove,
|
||||||
|
isTopmost,
|
||||||
|
markEscapeHandled,
|
||||||
|
escapeHandled,
|
||||||
|
} from "./layerStack";
|
||||||
|
|
||||||
|
// Keep the module-level stack clean between cases (ids never repeat, but membership must not leak).
|
||||||
|
const opened: number[] = [];
|
||||||
|
function open(): number {
|
||||||
|
const id = pushLayer();
|
||||||
|
opened.push(id);
|
||||||
|
return id;
|
||||||
|
}
|
||||||
|
afterEach(() => {
|
||||||
|
while (opened.length) removeLayer(opened.pop()!);
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("layerStack Escape coordination (UX-8)", () => {
|
||||||
|
it("a player defers even when the acting layer is popped mid-dispatch", () => {
|
||||||
|
const playerId = open();
|
||||||
|
const popoverId = open();
|
||||||
|
const playerClose = vi.fn();
|
||||||
|
|
||||||
|
// Popover: the `document` listener from useLayer — topmost acts, stamps the event, and (the
|
||||||
|
// regression) its close synchronously removes the layer before the window listener runs.
|
||||||
|
const onDocumentKey = (e: KeyboardEvent) => {
|
||||||
|
if (e.key === "Escape" && isTopmost(popoverId)) {
|
||||||
|
markEscapeHandled(e);
|
||||||
|
removeLayer(popoverId); // the synchronous React flush the real close triggers
|
||||||
|
}
|
||||||
|
};
|
||||||
|
// Player: the `window` listener — fires AFTER the document one, reading an already-mutated stack.
|
||||||
|
const onWindowKey = (e: KeyboardEvent) => {
|
||||||
|
if (e.key !== "Escape") return;
|
||||||
|
if (escapeHandled(e)) return; // the fix
|
||||||
|
if (anyLayerAbove(playerId)) return; // the old, now-insufficient guard
|
||||||
|
playerClose();
|
||||||
|
};
|
||||||
|
|
||||||
|
const e = new KeyboardEvent("keydown", { key: "Escape" });
|
||||||
|
onDocumentKey(e);
|
||||||
|
onWindowKey(e);
|
||||||
|
|
||||||
|
// The stamp holds even though the stack no longer shows a layer above the player...
|
||||||
|
expect(escapeHandled(e)).toBe(true);
|
||||||
|
expect(anyLayerAbove(playerId)).toBe(false); // ...which is exactly why anyLayerAbove alone failed
|
||||||
|
expect(playerClose).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("a plain Escape with no layer above still closes the player", () => {
|
||||||
|
const playerId = open();
|
||||||
|
const playerClose = vi.fn();
|
||||||
|
|
||||||
|
const e = new KeyboardEvent("keydown", { key: "Escape" });
|
||||||
|
if (!escapeHandled(e) && !anyLayerAbove(playerId)) playerClose();
|
||||||
|
|
||||||
|
expect(playerClose).toHaveBeenCalledOnce();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("stamps are per-event: a fresh Escape is not treated as already-handled", () => {
|
||||||
|
const first = new KeyboardEvent("keydown", { key: "Escape" });
|
||||||
|
markEscapeHandled(first);
|
||||||
|
const second = new KeyboardEvent("keydown", { key: "Escape" });
|
||||||
|
expect(escapeHandled(second)).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -38,6 +38,23 @@ export function anyLayerAbove(id: number): boolean {
|
|||||||
return i !== -1 && i < stack.length - 1;
|
return i !== -1 && i < stack.length - 1;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// UX-8 — a layer that ACTS on an Escape stamps the event here; a lower layer with its own key handler
|
||||||
|
// (the players, listening on `window`) reads the stamp instead of re-reading the stack. This closes
|
||||||
|
// the "one Escape closes BOTH the popover and the player" bug: the popover's Escape listener is on
|
||||||
|
// `document` and fires before the player's on `window` (bubble order), and closing the popover flushes
|
||||||
|
// its layer off the stack synchronously — so by the time the player's `anyLayerAbove()` runs, the stack
|
||||||
|
// already reads "nothing above" and the player ALSO closes. Tagging the one shared KeyboardEvent object
|
||||||
|
// (every listener in a single dispatch is handed the same instance) is immune to that mid-dispatch
|
||||||
|
// mutation; `anyLayerAbove` stays as the fallback for a layer above with no Escape handler of its own.
|
||||||
|
// WeakSet so a spent event is GC'd without a manual sweep.
|
||||||
|
const handledEscapes = new WeakSet<KeyboardEvent>();
|
||||||
|
export function markEscapeHandled(e: KeyboardEvent): void {
|
||||||
|
handledEscapes.add(e);
|
||||||
|
}
|
||||||
|
export function escapeHandled(e: KeyboardEvent): boolean {
|
||||||
|
return handledEscapes.has(e);
|
||||||
|
}
|
||||||
|
|
||||||
/** Register an open layer for as long as `active` is true, and get live predicates to read inside your
|
/** Register an open layer for as long as `active` is true, and get live predicates to read inside your
|
||||||
* own key handler. Pass `onEscape` to also install a topmost-only Escape listener (the common case —
|
* own key handler. Pass `onEscape` to also install a topmost-only Escape listener (the common case —
|
||||||
* modals, popovers). Omit it for a layer that owns a bigger key handler and only needs `anyAbove()`
|
* modals, popovers). Omit it for a layer that owns a bigger key handler and only needs `anyAbove()`
|
||||||
@@ -57,7 +74,12 @@ export function useLayer(
|
|||||||
let onKey: ((e: KeyboardEvent) => void) | undefined;
|
let onKey: ((e: KeyboardEvent) => void) | undefined;
|
||||||
if (escRef.current) {
|
if (escRef.current) {
|
||||||
onKey = (e: KeyboardEvent) => {
|
onKey = (e: KeyboardEvent) => {
|
||||||
if (e.key === "Escape" && isTopmost(id)) escRef.current?.();
|
if (e.key === "Escape" && isTopmost(id)) {
|
||||||
|
// Stamp BEFORE closing so a player's `window` listener (fires after this `document` one)
|
||||||
|
// sees the Escape as consumed even though closing us pops this layer off the stack.
|
||||||
|
markEscapeHandled(e);
|
||||||
|
escRef.current?.();
|
||||||
|
}
|
||||||
};
|
};
|
||||||
document.addEventListener("keydown", onKey);
|
document.addEventListener("keydown", onKey);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user