test(channels): cover ActionMenu keyboard-skip + stats ETA value; drop redundant disabledIndices
Deep-review follow-up on R13 S3. - Add a roving-keyboard-nav test proving a disabled ActionMenu item is skipped by arrow keys, and assert the stats "left" row renders the formatEta value (formatEta had no coverage anywhere). - Remove the disabledIndices plumbing from AnchoredPopover + ActionMenu: it was redundant. useListNavigation already skips items via their DOM disabled/aria-disabled attribute, and PopoverItem renders a native disabled button — verified against @floating-ui/react 0.27 isListIndexDisabled and by a mutation test. Behaviour is unchanged and now guarded by the new test.
This commit is contained in:
@@ -73,6 +73,43 @@ describe("ActionMenu", () => {
|
|||||||
expect(onSelect).not.toHaveBeenCalled();
|
expect(onSelect).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("skips a disabled item in roving keyboard nav", () => {
|
||||||
|
// items[0] disabled, items[1] + items[2] enabled. Roving keyboard focus (the `tabindex=0` item,
|
||||||
|
// which tracks useListNavigation's activeIndex) must NEVER land on the disabled index-0 — arrowing
|
||||||
|
// skips straight over it. The skip comes for free: PopoverItem renders a native `disabled` button
|
||||||
|
// and useListNavigation reads that DOM state (no explicit disabledIndices needed). Without it,
|
||||||
|
// ArrowUp/Down would strand roving focus on the inert index-0 button. (floating-ui wires this on
|
||||||
|
// real key nav, not on open, in jsdom — so we drive it with arrow keys.)
|
||||||
|
renderMenu([
|
||||||
|
{ id: "sync", label: "Sync", onSelect: vi.fn(), disabled: true },
|
||||||
|
{ id: "backfill", label: "Backfill", onSelect: vi.fn() },
|
||||||
|
{ id: "reset", label: "Reset", onSelect: vi.fn() },
|
||||||
|
]);
|
||||||
|
fireEvent.click(screen.getByRole("button", { name: "Catalog actions" }));
|
||||||
|
const [disabled, backfill, reset] = screen.getAllByRole("menuitem", { hidden: true }) as [
|
||||||
|
HTMLElement,
|
||||||
|
HTMLElement,
|
||||||
|
HTMLElement,
|
||||||
|
];
|
||||||
|
const menu = screen.getByRole("menu", { hidden: true });
|
||||||
|
const rover = () =>
|
||||||
|
[disabled, backfill, reset].find((el) => el.getAttribute("tabindex") === "0")?.textContent;
|
||||||
|
|
||||||
|
// The disabled item never becomes the roving target, whichever way we arrow.
|
||||||
|
fireEvent.keyDown(menu, { key: "ArrowDown" }); // skips disabled[0] -> first enabled
|
||||||
|
expect(rover()).toBe("Backfill");
|
||||||
|
expect(disabled.getAttribute("tabindex")).toBe("-1");
|
||||||
|
|
||||||
|
fireEvent.keyDown(menu, { key: "ArrowDown" });
|
||||||
|
expect(rover()).toBe("Reset");
|
||||||
|
|
||||||
|
fireEvent.keyDown(menu, { key: "ArrowUp" }); // back to Backfill
|
||||||
|
expect(rover()).toBe("Backfill");
|
||||||
|
fireEvent.keyDown(menu, { key: "ArrowUp" }); // wraps over disabled[0] to the last item
|
||||||
|
expect(rover()).toBe("Reset");
|
||||||
|
expect(disabled.getAttribute("tabindex")).toBe("-1"); // still never the disabled item
|
||||||
|
});
|
||||||
|
|
||||||
it("honours a custom trigger node", () => {
|
it("honours a custom trigger node", () => {
|
||||||
renderMenu([{ id: "sync", label: "Sync", onSelect: vi.fn() }]);
|
renderMenu([{ id: "sync", label: "Sync", onSelect: vi.fn() }]);
|
||||||
cleanup();
|
cleanup();
|
||||||
|
|||||||
@@ -31,15 +31,10 @@ export default function ActionMenu({
|
|||||||
trigger?: ReactNode;
|
trigger?: ReactNode;
|
||||||
}) {
|
}) {
|
||||||
const [open, setOpen] = useState(false);
|
const [open, setOpen] = useState(false);
|
||||||
// A disabled item (a mutation already in flight) can't take DOM focus, so roving keyboard nav must
|
// A disabled item (a mutation already in flight) is skipped by roving keyboard nav automatically:
|
||||||
// SKIP it — otherwise ArrowUp/Down strands the highlight on an inert button. The platform handles it.
|
// PopoverItem renders it as a native `disabled` button, which useListNavigation reads and never
|
||||||
const disabledIndices = items.flatMap((it, i) => (it.disabled ? [i] : []));
|
// focuses. No explicit disabledIndices needed. See ActionMenu.dom.test.tsx for the guard.
|
||||||
const pop = useAnchoredPopover({
|
const pop = useAnchoredPopover({ open, onOpenChange: setOpen, placement: "bottom-end" });
|
||||||
open,
|
|
||||||
onOpenChange: setOpen,
|
|
||||||
placement: "bottom-end",
|
|
||||||
disabledIndices,
|
|
||||||
});
|
|
||||||
return (
|
return (
|
||||||
<>
|
<>
|
||||||
<button
|
<button
|
||||||
|
|||||||
@@ -91,9 +91,6 @@ export interface AnchoredPopoverOptions {
|
|||||||
* lands the roving focus on it instead of the first item — the behaviour a "pick one of these
|
* lands the roving focus on it instead of the first item — the behaviour a "pick one of these
|
||||||
* modes" menu needs. Leave unset for action menus and multi-select toggle lists. */
|
* modes" menu needs. Leave unset for action menus and multi-select toggle lists. */
|
||||||
selectedIndex?: number | null;
|
selectedIndex?: number | null;
|
||||||
/** Indices of items that are disabled (e.g. an action whose mutation is already in flight), so
|
|
||||||
* roving keyboard nav SKIPS them instead of stranding focus on an unfocusable disabled button. */
|
|
||||||
disabledIndices?: number[];
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Internal: call sites use the inferred hook return, they don't name the type.
|
// Internal: call sites use the inferred hook return, they don't name the type.
|
||||||
@@ -178,7 +175,10 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) {
|
|||||||
setHoverIndex(null);
|
setHoverIndex(null);
|
||||||
},
|
},
|
||||||
selectedIndex: opts.selectedIndex ?? null,
|
selectedIndex: opts.selectedIndex ?? null,
|
||||||
disabledIndices: opts.disabledIndices,
|
// Disabled items are skipped automatically: with no `disabledIndices`, useListNavigation reads
|
||||||
|
// each item's DOM `disabled`/`aria-disabled` (PopoverItem always renders native `disabled`), so a
|
||||||
|
// pending/inert item is never a roving target. (Verified against @floating-ui/react 0.27
|
||||||
|
// `isListIndexDisabled`.) No explicit index list needed.
|
||||||
loop: true,
|
loop: true,
|
||||||
// Focus the first item on open for BOTH click and keyboard (the default 'auto' skips pointer-opens,
|
// Focus the first item on open for BOTH click and keyboard (the default 'auto' skips pointer-opens,
|
||||||
// which strands focus on the menu container — arrows then can't enter the roving list at all).
|
// which strands focus on the menu container — arrows then can't enter the roving list at all).
|
||||||
|
|||||||
@@ -19,6 +19,7 @@ vi.mock("react-i18next", async (importOriginal) => {
|
|||||||
|
|
||||||
import ChannelStatsPopover from "./ChannelStatsPopover";
|
import ChannelStatsPopover from "./ChannelStatsPopover";
|
||||||
import { type MyStatus } from "../lib/api";
|
import { type MyStatus } from "../lib/api";
|
||||||
|
import { formatEta } from "../lib/format";
|
||||||
|
|
||||||
beforeAll(() => {
|
beforeAll(() => {
|
||||||
class Stub {
|
class Stub {
|
||||||
@@ -76,12 +77,17 @@ describe("ChannelStatsPopover", () => {
|
|||||||
expect(screen.getByText((1234).toLocaleString())).toBeTruthy(); // my_videos, locale-agnostic
|
expect(screen.getByText((1234).toLocaleString())).toBeTruthy(); // my_videos, locale-agnostic
|
||||||
});
|
});
|
||||||
|
|
||||||
it("shows the 'left' ETA row only while a backfill is pending", () => {
|
it("shows the 'left' ETA row only while a backfill is pending, with the formatted ETA value", () => {
|
||||||
open(status({ deep_pending_count: 0 }));
|
open(status({ deep_pending_count: 0 }));
|
||||||
expect(screen.queryByText("channels.stats.left")).toBeNull();
|
expect(screen.queryByText("channels.stats.left")).toBeNull();
|
||||||
cleanup();
|
cleanup();
|
||||||
open(status({ deep_pending_count: 2 }));
|
open(status({ deep_pending_count: 2, deep_eta_seconds: 120 }));
|
||||||
expect(screen.getByText("channels.stats.left")).toBeTruthy();
|
expect(screen.getByText("channels.stats.left")).toBeTruthy();
|
||||||
|
// The value is the FORMATTED ETA (via formatEta), not raw seconds — asserting it guards the
|
||||||
|
// formatEta wiring, which is untested anywhere else. Using the same formatEta keeps this
|
||||||
|
// locale/impl-agnostic (expected === what the component renders).
|
||||||
|
expect(screen.getByText(formatEta(120))).toBeTruthy();
|
||||||
|
expect(screen.queryByText("120")).toBeNull(); // never the raw seconds
|
||||||
});
|
});
|
||||||
|
|
||||||
it("keeps each stat's explanation as a per-row title (hint not lost)", () => {
|
it("keeps each stat's explanation as a per-row title (hint not lost)", () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user