From b4a6569d0d3fdc1de4d86ed7fdf1e5682b063ea2 Mon Sep 17 00:00:00 2001
From: npeter83 <npeter83+claude@gmail.com>
Date: Fri, 25 Sep 2026 01:40:45 +0200
Subject: [PATCH] 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.
---
 .../components/SavedViewsPopover.dom.test.tsx | 21 +++++++++
 frontend/src/components/SavedViewsPopover.tsx |  7 +++
 frontend/src/components/SelectedViewChip.tsx  | 22 +++++-----
 .../src/components/WatchPage.dom.test.tsx     | 19 ++++++++
 frontend/src/components/WatchPage.tsx         |  3 +-
 frontend/src/components/Welcome.tsx           |  4 +-
 frontend/src/components/ui/Modal.dom.test.tsx | 43 ++++++++++++++++++-
 frontend/src/components/ui/Modal.tsx          |  4 +-
 frontend/src/components/ui/SelectTrigger.tsx  | 12 +++++-
 frontend/src/lib/keymap.test.ts               | 11 +++++
 frontend/src/lib/keymap.ts                    |  5 ++-
 frontend/src/lib/layerStack.ts                | 19 +++++---
 12 files changed, 144 insertions(+), 26 deletions(-)

diff --git a/frontend/src/components/SavedViewsPopover.dom.test.tsx b/frontend/src/components/SavedViewsPopover.dom.test.tsx
index 4de6ad92..1cbc9685 100644
--- a/frontend/src/components/SavedViewsPopover.dom.test.tsx
+++ b/frontend/src/components/SavedViewsPopover.dom.test.tsx
@@ -49,3 +49,24 @@ describe("SavedViewsPopover · Manage views…", () => {
     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");
+  });
+});
diff --git a/frontend/src/components/SavedViewsPopover.tsx b/frontend/src/components/SavedViewsPopover.tsx
index a4b15732..c372fd20 100644
--- a/frontend/src/components/SavedViewsPopover.tsx
+++ b/frontend/src/components/SavedViewsPopover.tsx
@@ -14,12 +14,17 @@ import { SelectTrigger } from "./ui/SelectTrigger";
 export default function SavedViewsPopover({
   trigger,
   toolbar = false,
+  icon,
+  trailing,
   ariaLabel,
   placement = "bottom-start",
 }: {
   trigger: ReactNode;
   /** The toolbar's view chip: the A5 dropdown trigger around `trigger`, instead of a glyph. */
   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;
   placement?: "bottom-start" | "bottom-end";
 }) {
@@ -34,6 +39,8 @@ export default function SavedViewsPopover({
         <SelectTrigger
           context="toolbar"
           open={open}
+          icon={icon}
+          trailing={trailing}
           ref={pop.refs.setReference}
           {...pop.getReferenceProps()}
           aria-label={label}
diff --git a/frontend/src/components/SelectedViewChip.tsx b/frontend/src/components/SelectedViewChip.tsx
index f8432a45..4f8ba066 100644
--- a/frontend/src/components/SelectedViewChip.tsx
+++ b/frontend/src/components/SelectedViewChip.tsx
@@ -15,19 +15,17 @@ export default function SelectedViewChip() {
     <SavedViewsPopover
       ariaLabel={t("views.saved")}
       toolbar
-      trigger={
-        <>
-          <Bookmark className="h-3.5 w-3.5 shrink-0 text-accent" aria-hidden="true" />
-          <span className="max-w-40 truncate">{displayView.name}</span>
-          {isDirty && (
-            <span
-              className="h-1.5 w-1.5 shrink-0 rounded-full bg-warn"
-              title={t("views.modified")}
-              aria-label={t("views.modified")}
-            />
-          )}
-        </>
+      icon={<Bookmark className="h-3.5 w-3.5 shrink-0 text-accent" aria-hidden="true" />}
+      trailing={
+        isDirty && (
+          <span
+            className="h-1.5 w-1.5 shrink-0 rounded-full bg-warn"
+            title={t("views.modified")}
+            aria-label={t("views.modified")}
+          />
+        )
       }
+      trigger={<span className="block max-w-40 truncate">{displayView.name}</span>}
     />
   );
 }
diff --git a/frontend/src/components/WatchPage.dom.test.tsx b/frontend/src/components/WatchPage.dom.test.tsx
index 73d3ccf5..629298e0 100644
--- a/frontend/src/components/WatchPage.dom.test.tsx
+++ b/frontend/src/components/WatchPage.dom.test.tsx
@@ -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)", () => {
   it("renders a native <audio> for an audio-only asset", async () => {
     window.history.replaceState({}, "", "/watch/tok-audio");
diff --git a/frontend/src/components/WatchPage.tsx b/frontend/src/components/WatchPage.tsx
index 7a3209a5..e2f8a488 100644
--- a/frontend/src/components/WatchPage.tsx
+++ b/frontend/src/components/WatchPage.tsx
@@ -395,8 +395,9 @@ export default function WatchPage() {
                 >
                   {T.repeat}
                 </Chip>
+                {/* The toolbar rung, level with the Repeat chip beside it (one row, one height). */}
                 {meta.allow_download && (
-                  <LinkButton href={meta.file_url} icon={<Download />}>
+                  <LinkButton href={meta.file_url} icon={<Download />} size="sm">
                     {T.download}
                   </LinkButton>
                 )}
diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx
index 5afe4077..3a082852 100644
--- a/frontend/src/components/Welcome.tsx
+++ b/frontend/src/components/Welcome.tsx
@@ -30,7 +30,7 @@ import { setLanguage, type LangCode } from "../i18n";
 import LanguageSwitcher from "./LanguageSwitcher";
 import { Button } from "./ui/Button";
 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 type { VerifyEntry } from "../lib/useVerifyEntry";
 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).
     const id = pushLayer();
     const onKey = (e: KeyboardEvent) => {
-      if (e.key === "Escape" && isTopmost(id)) {
+      if (takeEscape(e, id)) {
         e.stopPropagation();
         onCloseRef.current();
         return;
diff --git a/frontend/src/components/ui/Modal.dom.test.tsx b/frontend/src/components/ui/Modal.dom.test.tsx
index 26597680..c8a8f931 100644
--- a/frontend/src/components/ui/Modal.dom.test.tsx
+++ b/frontend/src/components/ui/Modal.dom.test.tsx
@@ -22,7 +22,8 @@ vi.mock("../../lib/history", () => ({ useBackToClose: () => {} }));
 
 import Modal from "./Modal";
 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.
 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.");
   });
 });
+
+// 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);
+  });
+});
diff --git a/frontend/src/components/ui/Modal.tsx b/frontend/src/components/ui/Modal.tsx
index 62577e45..8336db97 100644
--- a/frontend/src/components/ui/Modal.tsx
+++ b/frontend/src/components/ui/Modal.tsx
@@ -3,7 +3,7 @@ import { createPortal } from "react-dom";
 import { useTranslation } from "react-i18next";
 import { X } from "lucide-react";
 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 { trapTab, useDialogChrome } from "./dialogFocus";
 import { IconButton } from "./IconButton";
@@ -63,7 +63,7 @@ export default function Modal({
   useEffect(() => {
     const id = pushLayer();
     const onKey = (e: KeyboardEvent) => {
-      if (e.key === "Escape" && isTopmost(id)) {
+      if (takeEscape(e, id)) {
         e.stopPropagation();
         onCloseRef.current();
         return;
diff --git a/frontend/src/components/ui/SelectTrigger.tsx b/frontend/src/components/ui/SelectTrigger.tsx
index 24f5267c..f87a8fae 100644
--- a/frontend/src/components/ui/SelectTrigger.tsx
+++ b/frontend/src/components/ui/SelectTrigger.tsx
@@ -39,7 +39,17 @@ export const SelectTrigger = forwardRef<
     >
       {icon}
       {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}
       <ChevronDown
         aria-hidden="true"
diff --git a/frontend/src/lib/keymap.test.ts b/frontend/src/lib/keymap.test.ts
index d2a6f2e0..49bc8d04 100644
--- a/frontend/src/lib/keymap.test.ts
+++ b/frontend/src/lib/keymap.test.ts
@@ -11,6 +11,7 @@ import {
   isTypingTarget,
   matches,
 } from "./keymap";
+import { markEscapeHandled } from "./layerStack";
 
 /** A keydown as it arrives from a real focus target (jsdom only fills `e.target` on dispatch). */
 function press(init: KeyboardEventInit, on?: HTMLElement): KeyboardEvent {
@@ -143,6 +144,16 @@ describe("dispatch", () => {
     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
   // 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.
diff --git a/frontend/src/lib/keymap.ts b/frontend/src/lib/keymap.ts
index a66785a0..c826b97d 100644
--- a/frontend/src/lib/keymap.ts
+++ b/frontend/src/lib/keymap.ts
@@ -1,5 +1,5 @@
 import { useEffect, useRef, type RefObject } from "react";
-import { anyBlockingLayer } from "./layerStack";
+import { anyBlockingLayer, escapeHandled } from "./layerStack";
 import { createStore } from "./store";
 import type { Page } from "./urlState";
 
@@ -322,6 +322,9 @@ export function dispatch<S extends KeyScope>(
   handlers: { readonly [K in ShortcutId<S>]: (e: KeyboardEvent) => void },
 ): boolean {
   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);
   if (live && live.scope !== scope) return false;
   const quiet = isPageLevelScope(scope) && anyBlockingLayer();
diff --git a/frontend/src/lib/layerStack.ts b/frontend/src/lib/layerStack.ts
index aaea9a45..23abd042 100644
--- a/frontend/src/lib/layerStack.ts
+++ b/frontend/src/lib/layerStack.ts
@@ -85,6 +85,16 @@ export function escapeHandled(e: KeyboardEvent): boolean {
   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
  *  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()`
@@ -110,12 +120,9 @@ export function useLayer(
     let onKey: ((e: KeyboardEvent) => void) | undefined;
     if (escRef.current) {
       onKey = (e: KeyboardEvent) => {
-        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?.();
-        }
+        // takeEscape stamps BEFORE closing, so a `window` listener below (fires after this `document`
+        // one) sees the Escape as consumed even though closing us pops this layer off the stack.
+        if (takeEscape(e, id)) escRef.current?.();
       };
       document.addEventListener("keydown", onKey);
     }