fix(design): the saved-view chip on one line, one Escape per layer, the /watch action row (E28 S3d)
- SelectTrigger lays its toolbar value out as a row; the saved-view chip passes its glyph and modified dot through the icon/trailing slots (an <svg> is display:block and broke the chip in two). - One Escape in a dialog's open list closed the list AND the dialog: the list's close flushes synchronously and pops its layer before the dialog's window listener runs. takeEscape() is the one close-on-Escape test (topmost AND not yet taken); Modal, the Welcome lightbox and useLayer use it, and keymap.dispatch ignores a taken Escape. - /watch: the Download sits on the toolbar rung, level with the Repeat chip.
This commit is contained in:
@@ -49,3 +49,24 @@ describe("SavedViewsPopover · Manage views…", () => {
|
|||||||
await vi.waitFor(() => expect(openModal).toHaveBeenCalledTimes(1));
|
await vi.waitFor(() => expect(openModal).toHaveBeenCalledTimes(1));
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// E28: the toolbar chip's glyph sat INSIDE the value text, and an <svg> is display:block under the
|
||||||
|
// preflight, so the chip broke onto two lines (the bookmark above "gaming ▾"). The glyph and the
|
||||||
|
// modified dot ride in the trigger's own slots; the value is a row.
|
||||||
|
describe("SavedViewsPopover · toolbar chip", () => {
|
||||||
|
it("puts the glyph in the trigger's icon slot, not inside the value", () => {
|
||||||
|
render(
|
||||||
|
<FloatingTree>
|
||||||
|
<SavedViewsPopover
|
||||||
|
toolbar
|
||||||
|
icon={<svg data-testid="glyph" />}
|
||||||
|
trigger={<span>gaming</span>}
|
||||||
|
ariaLabel="View chip"
|
||||||
|
/>
|
||||||
|
</FloatingTree>,
|
||||||
|
);
|
||||||
|
const button = screen.getByRole("button", { name: "View chip" });
|
||||||
|
expect(screen.getByTestId("glyph").parentElement).toBe(button);
|
||||||
|
expect(screen.getByText("gaming").parentElement!.className).toContain("flex");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -14,12 +14,17 @@ import { SelectTrigger } from "./ui/SelectTrigger";
|
|||||||
export default function SavedViewsPopover({
|
export default function SavedViewsPopover({
|
||||||
trigger,
|
trigger,
|
||||||
toolbar = false,
|
toolbar = false,
|
||||||
|
icon,
|
||||||
|
trailing,
|
||||||
ariaLabel,
|
ariaLabel,
|
||||||
placement = "bottom-start",
|
placement = "bottom-start",
|
||||||
}: {
|
}: {
|
||||||
trigger: ReactNode;
|
trigger: ReactNode;
|
||||||
/** The toolbar's view chip: the A5 dropdown trigger around `trigger`, instead of a glyph. */
|
/** The toolbar's view chip: the A5 dropdown trigger around `trigger`, instead of a glyph. */
|
||||||
toolbar?: boolean;
|
toolbar?: boolean;
|
||||||
|
/** Toolbar only: the trigger's leading glyph and its after-the-value mark, in their own slots. */
|
||||||
|
icon?: ReactNode;
|
||||||
|
trailing?: ReactNode;
|
||||||
ariaLabel?: string;
|
ariaLabel?: string;
|
||||||
placement?: "bottom-start" | "bottom-end";
|
placement?: "bottom-start" | "bottom-end";
|
||||||
}) {
|
}) {
|
||||||
@@ -34,6 +39,8 @@ export default function SavedViewsPopover({
|
|||||||
<SelectTrigger
|
<SelectTrigger
|
||||||
context="toolbar"
|
context="toolbar"
|
||||||
open={open}
|
open={open}
|
||||||
|
icon={icon}
|
||||||
|
trailing={trailing}
|
||||||
ref={pop.refs.setReference}
|
ref={pop.refs.setReference}
|
||||||
{...pop.getReferenceProps()}
|
{...pop.getReferenceProps()}
|
||||||
aria-label={label}
|
aria-label={label}
|
||||||
|
|||||||
@@ -15,19 +15,17 @@ export default function SelectedViewChip() {
|
|||||||
<SavedViewsPopover
|
<SavedViewsPopover
|
||||||
ariaLabel={t("views.saved")}
|
ariaLabel={t("views.saved")}
|
||||||
toolbar
|
toolbar
|
||||||
trigger={
|
icon={<Bookmark className="h-3.5 w-3.5 shrink-0 text-accent" aria-hidden="true" />}
|
||||||
<>
|
trailing={
|
||||||
<Bookmark className="h-3.5 w-3.5 shrink-0 text-accent" aria-hidden="true" />
|
isDirty && (
|
||||||
<span className="max-w-40 truncate">{displayView.name}</span>
|
<span
|
||||||
{isDirty && (
|
className="h-1.5 w-1.5 shrink-0 rounded-full bg-warn"
|
||||||
<span
|
title={t("views.modified")}
|
||||||
className="h-1.5 w-1.5 shrink-0 rounded-full bg-warn"
|
aria-label={t("views.modified")}
|
||||||
title={t("views.modified")}
|
/>
|
||||||
aria-label={t("views.modified")}
|
)
|
||||||
/>
|
|
||||||
)}
|
|
||||||
</>
|
|
||||||
}
|
}
|
||||||
|
trigger={<span className="block max-w-40 truncate">{displayView.name}</span>}
|
||||||
/>
|
/>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -67,6 +67,25 @@ describe("WatchPage repeat", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// E28: the Download link sat at the button's 32 beside the Repeat chip's 28 — one row, two heights.
|
||||||
|
describe("WatchPage action row", () => {
|
||||||
|
it("puts the Download on the toolbar rung, level with the Repeat chip", async () => {
|
||||||
|
window.history.replaceState({}, "", "/watch/tok-row");
|
||||||
|
mockMeta({
|
||||||
|
needs_password: false,
|
||||||
|
audio: false,
|
||||||
|
allow_download: true,
|
||||||
|
file_url: "/f.mp4",
|
||||||
|
title: "A clip",
|
||||||
|
});
|
||||||
|
const view = render(<WatchPage />);
|
||||||
|
const repeat = await view.findByRole("button", { name: "Repeat" });
|
||||||
|
const download = view.container.querySelector('a[href="/f.mp4"]')!;
|
||||||
|
expect(repeat.className).toContain("h-7");
|
||||||
|
expect(download.className).toContain("h-7");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("WatchPage media element (S3 · 5a)", () => {
|
describe("WatchPage media element (S3 · 5a)", () => {
|
||||||
it("renders a native <audio> for an audio-only asset", async () => {
|
it("renders a native <audio> for an audio-only asset", async () => {
|
||||||
window.history.replaceState({}, "", "/watch/tok-audio");
|
window.history.replaceState({}, "", "/watch/tok-audio");
|
||||||
|
|||||||
@@ -395,8 +395,9 @@ export default function WatchPage() {
|
|||||||
>
|
>
|
||||||
{T.repeat}
|
{T.repeat}
|
||||||
</Chip>
|
</Chip>
|
||||||
|
{/* The toolbar rung, level with the Repeat chip beside it (one row, one height). */}
|
||||||
{meta.allow_download && (
|
{meta.allow_download && (
|
||||||
<LinkButton href={meta.file_url} icon={<Download />}>
|
<LinkButton href={meta.file_url} icon={<Download />} size="sm">
|
||||||
{T.download}
|
{T.download}
|
||||||
</LinkButton>
|
</LinkButton>
|
||||||
)}
|
)}
|
||||||
|
|||||||
@@ -30,7 +30,7 @@ import { setLanguage, type LangCode } from "../i18n";
|
|||||||
import LanguageSwitcher from "./LanguageSwitcher";
|
import LanguageSwitcher from "./LanguageSwitcher";
|
||||||
import { Button } from "./ui/Button";
|
import { Button } from "./ui/Button";
|
||||||
import { trapTab, useDialogChrome } from "./ui/dialogFocus";
|
import { trapTab, useDialogChrome } from "./ui/dialogFocus";
|
||||||
import { anyBlockingLayerAbove, isTopmost, pushLayer, removeLayer } from "../lib/layerStack";
|
import { anyBlockingLayerAbove, pushLayer, removeLayer, takeEscape } from "../lib/layerStack";
|
||||||
import { entryHash, entrySearch } from "../lib/entryUrl";
|
import { entryHash, entrySearch } from "../lib/entryUrl";
|
||||||
import type { VerifyEntry } from "../lib/useVerifyEntry";
|
import type { VerifyEntry } from "../lib/useVerifyEntry";
|
||||||
import { MODE_URL, type Mode } from "./welcomeMode";
|
import { MODE_URL, type Mode } from "./welcomeMode";
|
||||||
@@ -283,7 +283,7 @@ function Lightbox({ src, label, onClose }: { src: string; label: string; onClose
|
|||||||
// landing page grows (deep-review round 7).
|
// landing page grows (deep-review round 7).
|
||||||
const id = pushLayer();
|
const id = pushLayer();
|
||||||
const onKey = (e: KeyboardEvent) => {
|
const onKey = (e: KeyboardEvent) => {
|
||||||
if (e.key === "Escape" && isTopmost(id)) {
|
if (takeEscape(e, id)) {
|
||||||
e.stopPropagation();
|
e.stopPropagation();
|
||||||
onCloseRef.current();
|
onCloseRef.current();
|
||||||
return;
|
return;
|
||||||
|
|||||||
@@ -22,7 +22,8 @@ vi.mock("../../lib/history", () => ({ useBackToClose: () => {} }));
|
|||||||
|
|
||||||
import Modal from "./Modal";
|
import Modal from "./Modal";
|
||||||
import { clearAll, notify } from "../../lib/notifications";
|
import { clearAll, notify } from "../../lib/notifications";
|
||||||
import { pushLayer, removeLayer } from "../../lib/layerStack";
|
import { pushLayer, removeLayer, useLayer } from "../../lib/layerStack";
|
||||||
|
import { flushSync } from "react-dom";
|
||||||
|
|
||||||
// Fire a keydown on window (where Modal installs its handler), inside act.
|
// Fire a keydown on window (where Modal installs its handler), inside act.
|
||||||
const press = (key: string, shiftKey = false) =>
|
const press = (key: string, shiftKey = false) =>
|
||||||
@@ -364,3 +365,43 @@ describe("Modal — toasts raised while it is open", () => {
|
|||||||
expect(screen.getByRole("dialog").textContent).toContain("Couldn't share it.");
|
expect(screen.getByRole("dialog").textContent).toContain("Couldn't share it.");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// E28: one Escape in a dialog's open list closed the list AND the dialog (the share dialog's user
|
||||||
|
// picker). The list's `document` listener closes it with a synchronous flush, which pops its layer
|
||||||
|
// before the dialog's `window` listener runs — so "am I topmost?" reads yes. The dialog must honour
|
||||||
|
// the list's stamp, as the players already do (UX-8).
|
||||||
|
describe("Modal — an Escape a layer above it already took", () => {
|
||||||
|
// Opened by a click after the dialog is up, as a user opens it: a child's effect runs before its
|
||||||
|
// parent's, so a list open from the first render would sit BELOW the dialog on the stack.
|
||||||
|
function List({ onClosed }: { onClosed: () => void }) {
|
||||||
|
const [open, setOpen] = useState(false);
|
||||||
|
useLayer(open, () => {
|
||||||
|
flushSync(() => setOpen(false));
|
||||||
|
onClosed();
|
||||||
|
});
|
||||||
|
return open ? (
|
||||||
|
<div data-testid="list" />
|
||||||
|
) : (
|
||||||
|
<button onClick={() => setOpen(true)}>open list</button>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
it("closes the list only; the next Escape closes the dialog", () => {
|
||||||
|
const onClose = vi.fn();
|
||||||
|
const listClosed = vi.fn();
|
||||||
|
render(
|
||||||
|
<Modal title="Share" onClose={onClose}>
|
||||||
|
<List onClosed={listClosed} />
|
||||||
|
</Modal>,
|
||||||
|
);
|
||||||
|
act(() => screen.getByRole("button", { name: "open list" }).click());
|
||||||
|
act(() => {
|
||||||
|
document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true }));
|
||||||
|
});
|
||||||
|
expect(listClosed).toHaveBeenCalledTimes(1);
|
||||||
|
expect(screen.queryByTestId("list")).toBeNull();
|
||||||
|
expect(onClose).not.toHaveBeenCalled();
|
||||||
|
press("Escape");
|
||||||
|
expect(onClose).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -3,7 +3,7 @@ import { createPortal } from "react-dom";
|
|||||||
import { useTranslation } from "react-i18next";
|
import { useTranslation } from "react-i18next";
|
||||||
import { X } from "lucide-react";
|
import { X } from "lucide-react";
|
||||||
import { useBackToClose } from "../../lib/history";
|
import { useBackToClose } from "../../lib/history";
|
||||||
import { anyBlockingLayerAbove, pushLayer, removeLayer, isTopmost } from "../../lib/layerStack";
|
import { anyBlockingLayerAbove, pushLayer, removeLayer, takeEscape } from "../../lib/layerStack";
|
||||||
import { useFullscreenPortalHost } from "../../lib/fullscreen";
|
import { useFullscreenPortalHost } from "../../lib/fullscreen";
|
||||||
import { trapTab, useDialogChrome } from "./dialogFocus";
|
import { trapTab, useDialogChrome } from "./dialogFocus";
|
||||||
import { IconButton } from "./IconButton";
|
import { IconButton } from "./IconButton";
|
||||||
@@ -63,7 +63,7 @@ export default function Modal({
|
|||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
const id = pushLayer();
|
const id = pushLayer();
|
||||||
const onKey = (e: KeyboardEvent) => {
|
const onKey = (e: KeyboardEvent) => {
|
||||||
if (e.key === "Escape" && isTopmost(id)) {
|
if (takeEscape(e, id)) {
|
||||||
e.stopPropagation();
|
e.stopPropagation();
|
||||||
onCloseRef.current();
|
onCloseRef.current();
|
||||||
return;
|
return;
|
||||||
|
|||||||
@@ -39,7 +39,17 @@ export const SelectTrigger = forwardRef<
|
|||||||
>
|
>
|
||||||
{icon}
|
{icon}
|
||||||
{label != null && <span className="text-muted">{label}:</span>}
|
{label != null && <span className="text-muted">{label}:</span>}
|
||||||
<span className={clsx("text-fg", context === "form" && "flex-1 truncate")}>{children}</span>
|
{/* The toolbar value is a row, whatever the caller puts in it: an <svg> is display:block under the
|
||||||
|
preflight, and in a plain span it broke the value onto its own line (the saved-view chip,
|
||||||
|
E28). The form value is text that truncates, which a flex box would not do. */}
|
||||||
|
<span
|
||||||
|
className={clsx(
|
||||||
|
"text-fg",
|
||||||
|
context === "form" ? "flex-1 truncate" : "flex min-w-0 items-center gap-1.5",
|
||||||
|
)}
|
||||||
|
>
|
||||||
|
{children}
|
||||||
|
</span>
|
||||||
{trailing}
|
{trailing}
|
||||||
<ChevronDown
|
<ChevronDown
|
||||||
aria-hidden="true"
|
aria-hidden="true"
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ import {
|
|||||||
isTypingTarget,
|
isTypingTarget,
|
||||||
matches,
|
matches,
|
||||||
} from "./keymap";
|
} from "./keymap";
|
||||||
|
import { markEscapeHandled } from "./layerStack";
|
||||||
|
|
||||||
/** A keydown as it arrives from a real focus target (jsdom only fills `e.target` on dispatch). */
|
/** A keydown as it arrives from a real focus target (jsdom only fills `e.target` on dispatch). */
|
||||||
function press(init: KeyboardEventInit, on?: HTMLElement): KeyboardEvent {
|
function press(init: KeyboardEventInit, on?: HTMLElement): KeyboardEvent {
|
||||||
@@ -143,6 +144,16 @@ describe("dispatch", () => {
|
|||||||
input.remove();
|
input.remove();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// E28: an Escape a menu above already took is spent — a player or page acting on it too is the
|
||||||
|
// double close the layer stack exists to prevent.
|
||||||
|
it("ignores an Escape a layer already took", () => {
|
||||||
|
const calls: string[] = [];
|
||||||
|
const e = press({ key: "Escape" });
|
||||||
|
markEscapeHandled(e);
|
||||||
|
expect(dispatch(e, "plexPlayer", plexHandlers(calls))).toBe(false);
|
||||||
|
expect(calls).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
// A `handledLocally` row is documentation + a shared chord, matched on the focused element by its
|
// A `handledLocally` row is documentation + a shared chord, matched on the focused element by its
|
||||||
// own component (Shift+F10 belongs to whichever row has focus). `dispatch` must not claim it, and
|
// own component (Shift+F10 belongs to whichever row has focus). `dispatch` must not claim it, and
|
||||||
// the mapped handler type must not demand one — this test is the runtime half of that.
|
// the mapped handler type must not demand one — this test is the runtime half of that.
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { useEffect, useRef, type RefObject } from "react";
|
import { useEffect, useRef, type RefObject } from "react";
|
||||||
import { anyBlockingLayer } from "./layerStack";
|
import { anyBlockingLayer, escapeHandled } from "./layerStack";
|
||||||
import { createStore } from "./store";
|
import { createStore } from "./store";
|
||||||
import type { Page } from "./urlState";
|
import type { Page } from "./urlState";
|
||||||
|
|
||||||
@@ -322,6 +322,9 @@ export function dispatch<S extends KeyScope>(
|
|||||||
handlers: { readonly [K in ShortcutId<S>]: (e: KeyboardEvent) => void },
|
handlers: { readonly [K in ShortcutId<S>]: (e: KeyboardEvent) => void },
|
||||||
): boolean {
|
): boolean {
|
||||||
if (sequenceEvents.has(e)) return false;
|
if (sequenceEvents.has(e)) return false;
|
||||||
|
// An Escape a layer already took (a menu that just closed) is spent: a page or a player reacting to
|
||||||
|
// it too is the double close the layer stack exists to prevent.
|
||||||
|
if (e.key === "Escape" && escapeHandled(e)) return false;
|
||||||
const live = liveSequence(e);
|
const live = liveSequence(e);
|
||||||
if (live && live.scope !== scope) return false;
|
if (live && live.scope !== scope) return false;
|
||||||
const quiet = isPageLevelScope(scope) && anyBlockingLayer();
|
const quiet = isPageLevelScope(scope) && anyBlockingLayer();
|
||||||
|
|||||||
@@ -85,6 +85,16 @@ export function escapeHandled(e: KeyboardEvent): boolean {
|
|||||||
return handledEscapes.has(e);
|
return handledEscapes.has(e);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** THE close-on-Escape test for layer `id`: an Escape no layer above has taken yet, while `id` is on
|
||||||
|
* top — and it claims the event for `id`. Both halves are needed: a list inside a dialog closes with a
|
||||||
|
* synchronous flush that pops its layer before the dialog's `window` listener runs, so topmost alone
|
||||||
|
* read "yes" and one Escape closed the list AND the dialog (E28, the share dialog's user picker). */
|
||||||
|
export function takeEscape(e: KeyboardEvent, id: number): boolean {
|
||||||
|
if (e.key !== "Escape" || escapeHandled(e) || !isTopmost(id)) return false;
|
||||||
|
markEscapeHandled(e);
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
/** 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()`
|
||||||
@@ -110,12 +120,9 @@ 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)) {
|
// takeEscape stamps BEFORE closing, so a `window` listener below (fires after this `document`
|
||||||
// Stamp BEFORE closing so a player's `window` listener (fires after this `document` one)
|
// one) sees the Escape as consumed even though closing us pops this layer off the stack.
|
||||||
// sees the Escape as consumed even though closing us pops this layer off the stack.
|
if (takeEscape(e, id)) escRef.current?.();
|
||||||
markEscapeHandled(e);
|
|
||||||
escRef.current?.();
|
|
||||||
}
|
|
||||||
};
|
};
|
||||||
document.addEventListener("keydown", onKey);
|
document.addEventListener("keydown", onKey);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user