fix(ui): a disabled menu row says why it is off, under its label
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.
This commit is contained in:
@@ -772,17 +772,14 @@ export default function Playlists() {
|
|||||||
})),
|
})),
|
||||||
}
|
}
|
||||||
: // Kept visible and disabled rather than dropped: an action that vanishes teaches
|
: // 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
|
// nobody why — and its `note` says the why, under the label.
|
||||||
// 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.
|
|
||||||
{
|
{
|
||||||
id: "move",
|
id: "move",
|
||||||
label: t("playlists.moveTo"),
|
label: t("playlists.moveTo"),
|
||||||
icon: <FolderInput className="w-4 h-4" />,
|
icon: <FolderInput className="w-4 h-4" />,
|
||||||
separatorBefore: !recentMoveTarget,
|
separatorBefore: !recentMoveTarget,
|
||||||
disabled: true,
|
disabled: true,
|
||||||
title: t("playlists.moveNoTargets"),
|
note: t("playlists.moveNoTargets"),
|
||||||
onSelect: () => {},
|
onSelect: () => {},
|
||||||
},
|
},
|
||||||
...(
|
...(
|
||||||
@@ -797,11 +794,10 @@ export default function Playlists() {
|
|||||||
label,
|
label,
|
||||||
icon,
|
icon,
|
||||||
separatorBefore: i === 0,
|
separatorBefore: i === 0,
|
||||||
// Same gate as the drag handle. NOTE the drag handle explains itself and this row
|
// Same gate as the drag handle. The reason goes under the FIRST of the four rows
|
||||||
// does not: the handle is `aria-disabled` (focusable, tooltip-capable) while a menu
|
// only — one explanation for the group, not the same line four times.
|
||||||
// row cannot be — see ActionMenu's `title` prop for why.
|
|
||||||
disabled: !storedOrderEditable,
|
disabled: !storedOrderEditable,
|
||||||
title: storedOrderEditable ? undefined : dragHint,
|
note: i === 0 ? dragHint : undefined,
|
||||||
onSelect: () => moveInOrder(ctxRow, to),
|
onSelect: () => moveInOrder(ctxRow, to),
|
||||||
})),
|
})),
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -199,7 +199,7 @@ export default function PlaylistsRail({
|
|||||||
// Interpolated, not spelled out: the HU copy said "Watch later", a name the Hungarian UI never
|
// 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
|
// 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.
|
// (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") })
|
? t("playlists.builtinLocked", { name: t("playlists.watchLater") })
|
||||||
: undefined;
|
: undefined;
|
||||||
const railItems: ActionMenuItem[] = !ctxPl
|
const railItems: ActionMenuItem[] = !ctxPl
|
||||||
@@ -210,7 +210,7 @@ export default function PlaylistsRail({
|
|||||||
label: t("playlists.rename"),
|
label: t("playlists.rename"),
|
||||||
icon: <Pencil className="w-4 h-4" />,
|
icon: <Pencil className="w-4 h-4" />,
|
||||||
disabled: ctxBuiltin,
|
disabled: ctxBuiltin,
|
||||||
title: lockedTitle,
|
note: lockedNote,
|
||||||
onSelect: () => {
|
onSelect: () => {
|
||||||
setRenameValue(ctxPl.name);
|
setRenameValue(ctxPl.name);
|
||||||
setRenamingId(ctxPl.id);
|
setRenamingId(ctxPl.id);
|
||||||
@@ -224,7 +224,7 @@ export default function PlaylistsRail({
|
|||||||
separatorBefore: true,
|
separatorBefore: true,
|
||||||
danger: true,
|
danger: true,
|
||||||
disabled: ctxBuiltin,
|
disabled: ctxBuiltin,
|
||||||
title: lockedTitle,
|
note: lockedNote,
|
||||||
// The same dialog (and the same "what gets selected next" rule) as the detail header's
|
// The same dialog (and the same "what gets selected next" rule) as the detail header's
|
||||||
// trash button — one implementation, two entry points.
|
// trash button — one implementation, two entry points.
|
||||||
onSelect: () => void plActions.remove(ctxPl),
|
onSelect: () => void plActions.remove(ctxPl),
|
||||||
|
|||||||
@@ -42,6 +42,22 @@ describe("ActionMenu", () => {
|
|||||||
|
|
||||||
// floating-ui never computes a position in jsdom, so the surface stays visibility:hidden; the items
|
// 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.
|
// 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", () => {
|
it("opens on click and shows one menuitem per item", () => {
|
||||||
renderMenu([
|
renderMenu([
|
||||||
{ id: "sync", label: "Sync", onSelect: vi.fn() },
|
{ id: "sync", label: "Sync", onSelect: vi.fn() },
|
||||||
|
|||||||
@@ -16,24 +16,37 @@ import { closeThenAct, useAnchoredPopover } from "./anchoredPopoverHooks";
|
|||||||
/** Shared surface paint for every menu built on this file — the ⋯ menu, its submenus, and
|
/** 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. */
|
* `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]";
|
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 =
|
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 (
|
||||||
|
<span className="min-w-0 flex-1">
|
||||||
|
<span className={`block truncate${dim(item.disabled)}`}>{item.label}</span>
|
||||||
|
{item.disabled && item.note && (
|
||||||
|
<span className="block text-2xs leading-snug text-muted">{item.note}</span>
|
||||||
|
)}
|
||||||
|
</span>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
interface MenuItemBase {
|
interface MenuItemBase {
|
||||||
id: string;
|
id: string;
|
||||||
label: string;
|
label: string;
|
||||||
icon?: ReactNode; // an element (so a caller can swap in a spinning icon while pending)
|
icon?: ReactNode; // an element (so a caller can swap in a spinning icon while pending)
|
||||||
disabled?: boolean;
|
disabled?: boolean;
|
||||||
/** Optional native tooltip (carries a longer hint the label alone can't).
|
/** Optional native tooltip (carries a longer hint the label alone can't) — for an ENABLED row. */
|
||||||
*
|
|
||||||
* 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. */
|
|
||||||
title?: string;
|
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. */
|
/** Draw a divider above this item. Ignored on the first item — a menu never opens with a rule. */
|
||||||
separatorBefore?: boolean;
|
separatorBefore?: boolean;
|
||||||
}
|
}
|
||||||
@@ -88,11 +101,13 @@ export function MenuItemList({
|
|||||||
className={`${ROW}${it.danger ? " text-danger" : ""}`}
|
className={`${ROW}${it.danger ? " text-danger" : ""}`}
|
||||||
>
|
>
|
||||||
{it.icon && (
|
{it.icon && (
|
||||||
<span className={`shrink-0 ${it.danger ? "text-danger" : "text-muted"}`}>
|
<span
|
||||||
|
className={`shrink-0 ${it.danger ? "text-danger" : "text-muted"}${dim(it.disabled)}`}
|
||||||
|
>
|
||||||
{it.icon}
|
{it.icon}
|
||||||
</span>
|
</span>
|
||||||
)}
|
)}
|
||||||
<span className="flex-1 truncate">{it.label}</span>
|
<RowLabel item={it} />
|
||||||
</PopoverItem>
|
</PopoverItem>
|
||||||
)}
|
)}
|
||||||
</Fragment>
|
</Fragment>
|
||||||
@@ -142,8 +157,10 @@ function SubMenu({ item, onClose }: { item: SubmenuItem; onClose: () => void })
|
|||||||
}}
|
}}
|
||||||
className={ROW}
|
className={ROW}
|
||||||
>
|
>
|
||||||
{item.icon && <span className="shrink-0 text-muted">{item.icon}</span>}
|
{item.icon && (
|
||||||
<span className="flex-1 truncate">{item.label}</span>
|
<span className={`shrink-0 text-muted${dim(item.disabled)}`}>{item.icon}</span>
|
||||||
|
)}
|
||||||
|
<RowLabel item={item} />
|
||||||
<ChevronRight className="h-4 w-4 shrink-0 text-muted" aria-hidden="true" />
|
<ChevronRight className="h-4 w-4 shrink-0 text-muted" aria-hidden="true" />
|
||||||
</PopoverItem>
|
</PopoverItem>
|
||||||
<PopoverSurface
|
<PopoverSurface
|
||||||
|
|||||||
Reference in New Issue
Block a user