From bc138e3b375caea26fe11ade93560f00eec13c51 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:57:20 +0200 Subject: [PATCH] fix(ui): a disabled menu row says why it is off, under its label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reason for a disabled menu row lived in a `title` on a natively disabled button: no pointer events, so no tooltip, and roving focus skips the row — four translated sentences reached nobody (the Watch later rename/delete lock, "no other list to move to", the reorder gate). An aria-disabled row was tried and reverted in E10's review. ActionMenu items now take a `note`, rendered as a muted line under a disabled row's label: seen, and readable in a screen reader's browse mode. Only the icon and label dim — row-level opacity would dim the explanation too. The reorder group states its reason once, under its first row. --- frontend/src/components/Playlists.tsx | 14 +++--- frontend/src/components/PlaylistsRail.tsx | 6 +-- .../src/components/ui/ActionMenu.dom.test.tsx | 16 +++++++ frontend/src/components/ui/ActionMenu.tsx | 45 +++++++++++++------ 4 files changed, 55 insertions(+), 26 deletions(-) diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index 0722502b..0b246166 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -772,17 +772,14 @@ export default function Playlists() { })), } : // Kept visible and disabled rather than dropped: an action that vanishes teaches - // nobody why. KNOWN GAP: the `title` that carries the why currently reaches nobody - // either — see ActionMenu's `title` prop, where the two failed attempts at fixing - // that are recorded. The row is honest about being off; it is not yet honest about - // the reason. + // nobody why — and its `note` says the why, under the label. { id: "move", label: t("playlists.moveTo"), icon: , separatorBefore: !recentMoveTarget, disabled: true, - title: t("playlists.moveNoTargets"), + note: t("playlists.moveNoTargets"), onSelect: () => {}, }, ...( @@ -797,11 +794,10 @@ export default function Playlists() { label, icon, separatorBefore: i === 0, - // Same gate as the drag handle. NOTE the drag handle explains itself and this row - // does not: the handle is `aria-disabled` (focusable, tooltip-capable) while a menu - // row cannot be — see ActionMenu's `title` prop for why. + // Same gate as the drag handle. The reason goes under the FIRST of the four rows + // only — one explanation for the group, not the same line four times. disabled: !storedOrderEditable, - title: storedOrderEditable ? undefined : dragHint, + note: i === 0 ? dragHint : undefined, onSelect: () => moveInOrder(ctxRow, to), })), { diff --git a/frontend/src/components/PlaylistsRail.tsx b/frontend/src/components/PlaylistsRail.tsx index e32a0dc6..ad6ce147 100644 --- a/frontend/src/components/PlaylistsRail.tsx +++ b/frontend/src/components/PlaylistsRail.tsx @@ -199,7 +199,7 @@ export default function PlaylistsRail({ // Interpolated, not spelled out: the HU copy said "Watch later", a name the Hungarian UI never // shows for this list ("Megnézem később") — so the message named a list the reader had never seen // (E10 review round 1). Passing the localized name makes the two impossible to drift. - const lockedTitle = ctxBuiltin + const lockedNote = ctxBuiltin ? t("playlists.builtinLocked", { name: t("playlists.watchLater") }) : undefined; const railItems: ActionMenuItem[] = !ctxPl @@ -210,7 +210,7 @@ export default function PlaylistsRail({ label: t("playlists.rename"), icon: , disabled: ctxBuiltin, - title: lockedTitle, + note: lockedNote, onSelect: () => { setRenameValue(ctxPl.name); setRenamingId(ctxPl.id); @@ -224,7 +224,7 @@ export default function PlaylistsRail({ separatorBefore: true, danger: true, disabled: ctxBuiltin, - title: lockedTitle, + note: lockedNote, // The same dialog (and the same "what gets selected next" rule) as the detail header's // trash button — one implementation, two entry points. onSelect: () => void plActions.remove(ctxPl), diff --git a/frontend/src/components/ui/ActionMenu.dom.test.tsx b/frontend/src/components/ui/ActionMenu.dom.test.tsx index b5b54ace..6885da86 100644 --- a/frontend/src/components/ui/ActionMenu.dom.test.tsx +++ b/frontend/src/components/ui/ActionMenu.dom.test.tsx @@ -42,6 +42,22 @@ describe("ActionMenu", () => { // floating-ui never computes a position in jsdom, so the surface stays visibility:hidden; the items // are in the DOM but the a11y tree treats them as hidden — hence `{ hidden: true }` on role queries. + it("says why a disabled row is off, under its label — and only on a disabled row", () => { + // A `title` on a disabled row reached nobody: a disabled button fires no pointer events and + // roving focus skips it. The reason is visible text now, undimmed so it can be read. + renderMenu([ + { id: "move", label: "Move to", onSelect: vi.fn(), disabled: true, note: "No other list" }, + { id: "sync", label: "Sync", onSelect: vi.fn(), note: "never shown" }, + ]); + fireEvent.click(screen.getByRole("button", { name: "Catalog actions" })); + const [move, sync] = screen.getAllByRole("menuitem", { hidden: true }); + expect(move!.textContent).toBe("Move toNo other list"); + expect(sync!.textContent).toBe("Sync"); + const note = screen.getByText("No other list"); + expect(note.closest(".opacity-50")).toBeNull(); + expect(screen.getByText("Move to").className).toContain("opacity-50"); + }); + it("opens on click and shows one menuitem per item", () => { renderMenu([ { id: "sync", label: "Sync", onSelect: vi.fn() }, diff --git a/frontend/src/components/ui/ActionMenu.tsx b/frontend/src/components/ui/ActionMenu.tsx index 3e591d48..797a54e6 100644 --- a/frontend/src/components/ui/ActionMenu.tsx +++ b/frontend/src/components/ui/ActionMenu.tsx @@ -16,24 +16,37 @@ import { closeThenAct, useAnchoredPopover } from "./anchoredPopoverHooks"; /** Shared surface paint for every menu built on this file — the ⋯ menu, its submenus, and * `ContextMenu`. One constant so a context menu can never look like a different component. */ export const MENU_SURFACE = "glass-menu min-w-52 rounded-xl p-1.5 animate-[popIn_0.16s_ease]"; +// No `disabled:opacity-50` on the row itself: a disabled row may carry a `note`, and opacity on the +// row would dim the one line that explains it. The icon and the label dim instead (`dim`). const ROW = - "flex w-full items-center gap-2.5 rounded-lg px-2 py-1.5 text-left text-sm outline-hidden transition disabled:opacity-50"; + "flex w-full items-center gap-2.5 rounded-lg px-2 py-1.5 text-left text-sm outline-hidden transition"; +const dim = (disabled: boolean | undefined) => (disabled ? " opacity-50" : ""); + +/** The label, and under it — on a disabled row only — the reason it is off. */ +function RowLabel({ item }: { item: MenuItemBase }) { + return ( + + {item.label} + {item.disabled && item.note && ( + {item.note} + )} + + ); +} interface MenuItemBase { id: string; label: string; icon?: ReactNode; // an element (so a caller can swap in a spinning icon while pending) disabled?: boolean; - /** Optional native tooltip (carries a longer hint the label alone can't). - * - * KNOWN GAP, deliberately still open: on a `disabled` row this reaches nobody — a disabled button - * fires no pointer events, so no tooltip, and floating-ui's roving focus skips it. E10 review - * round 1 tried an `aria-disabled` "inert" row to fix that; round 2 found `useListNavigation` - * skips `aria-disabled` identically (`isListIndexDisabled`), which AnchoredPopover's own comment - * already said — so the swap fixed nothing and made the mouse worse (the row took the active - * highlight, then no-opped on click). Reverted. A real fix has to put the reason somewhere a - * disabled row is not: a visible hint line in the menu, or an `aria-describedby` note. */ + /** Optional native tooltip (carries a longer hint the label alone can't) — for an ENABLED row. */ title?: string; + /** Why a DISABLED row is off, shown as a line under its label. Not a `title`: a disabled button + * fires no pointer events (no tooltip) and roving focus skips it, so a reason there reached + * nobody. An `aria-disabled` row was tried and reverted (E10 review rounds 1–2: floating-ui skips + * it identically, and the mouse got a highlight that then no-opped). Visible text is the one + * place a disabled row's reason can be: seen, and read in a screen reader's browse mode. */ + note?: string; /** Draw a divider above this item. Ignored on the first item — a menu never opens with a rule. */ separatorBefore?: boolean; } @@ -88,11 +101,13 @@ export function MenuItemList({ className={`${ROW}${it.danger ? " text-danger" : ""}`} > {it.icon && ( - + {it.icon} )} - {it.label} + )} @@ -142,8 +157,10 @@ function SubMenu({ item, onClose }: { item: SubmenuItem; onClose: () => void }) }} className={ROW} > - {item.icon && {item.icon}} - {item.label} + {item.icon && ( + {item.icon} + )} +