Skip to content

Commit bd83e6e

Browse files
tyler-daneclaude
tyler-dane
and
claude
authored
fix(web): make the grid context menu keyboard-operable (#2172)
* fix(web): make the grid context menu keyboard-operable The event-card right-click / Shift+F10 menu opened but left focus on the card, and its items (portalled to end-of-body) were a page of tabbing away with no arrow-key support. Screen readers also entered an empty menu: the items were plain buttons behind a generic <div>, breaking menu->menuitem ownership. - role="menuitem" on the items and role="none" on their container so the menu exposes valid menu/menuitem semantics. - useListNavigation + roving tabindex for arrow-key movement between items. - Seat focus on the first item when the menu opens and return it to the opener (the card) on close. FloatingFocusManager will not seat focus here because the menu opens from a raw contextmenu handler, not a floating-ui interaction; and a synchronous focus is a no-op (the portalled menu is not laid out during the commit, and opening writes the grid draft whose re-render reclaims focus to the card once). So focus is deferred and briefly retried via setTimeout - which, unlike rAF, still fires in a background tab - re-seating only while focus has fallen out of the menu. - Drop the dead useClick(context) call (useClick only returns reference props, which this component never spreads). Verified in-browser that focus lands on and stays at the first item after open; ContextMenuKeyboard.test.tsx covers focus-on-open, return-focus, arrow navigation, and menu/menuitem semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(web): make context-menu focus tests deterministic The focus-on-open and return-focus tests polled with waitFor while the component's setTimeout focus-retry chain settled, which ran ~2.3s locally and timed out on the slower CI runner. Flush the retry window with a bounded act() delay and assert synchronously instead - ~280ms and deterministic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): query context menu items by menuitem role The grid context menu items are now role=menuitem (not plain buttons), so the read-only-event specs must locate View/Duplicate/Delete via getByRole menuitem instead of button. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 7519880 commit bd83e6e

5 files changed

Lines changed: 279 additions & 43 deletions

File tree

‎e2e/calendars/calendar-experience.spec.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -510,7 +510,7 @@ test("a read-only event opens as a read-only form", async ({ page }) => {
510510
// close-out; keyboard ("M") and this View path are the deterministic
511511
// inspection routes today.
512512
await card.click({ button: "right" });
513-
await page.getByRole("menu").getByRole("button", { name: "View" }).click();
513+
await page.getByRole("menu").getByRole("menuitem", { name: "View" }).click();
514514

515515
const form = page.getByRole("form");
516516
const titleInput = form.getByPlaceholder("Title");
@@ -535,7 +535,7 @@ test("a read-only event's context menu offers view and duplicate but not delete"
535535
await card.click({ button: "right" });
536536

537537
const menu = page.getByRole("menu");
538-
await expect(menu.getByRole("button", { name: "View" })).toBeVisible();
539-
await expect(menu.getByRole("button", { name: "Duplicate" })).toBeVisible();
540-
await expect(menu.getByRole("button", { name: "Delete" })).toHaveCount(0);
538+
await expect(menu.getByRole("menuitem", { name: "View" })).toBeVisible();
539+
await expect(menu.getByRole("menuitem", { name: "Duplicate" })).toBeVisible();
540+
await expect(menu.getByRole("menuitem", { name: "Delete" })).toHaveCount(0);
541541
});

‎packages/web/src/components/ContextMenu/ContextMenu.tsx‎

Lines changed: 86 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
import {
22
type FloatingContext,
3-
useClick,
43
useDismiss,
54
useInteractions,
5+
useListNavigation,
66
useRole,
77
} from "@floating-ui/react";
8-
import React from "react";
8+
import React, { useCallback, useEffect, useRef, useState } from "react";
99
import { darken } from "@web/common/styles/color.utils";
1010
import { type CSSVariables } from "@web/common/styles/css.types";
1111
import { EVENT_HOVER_COLOR } from "@web/common/styles/theme.util";
@@ -14,6 +14,7 @@ import {
1414
ContextMenuItems,
1515
type ContextMenuItemsActions,
1616
ContextMenuItemsView,
17+
ContextMenuNavContext,
1718
} from "./ContextMenuItems";
1819

1920
interface ContextMenuProps {
@@ -27,6 +28,63 @@ interface ContextMenuProps {
2728

2829
export const ContextMenu = React.forwardRef<HTMLUListElement, ContextMenuProps>(
2930
({ actions, event, onOutsideClick, close, style, context }, ref) => {
31+
// Seed the active index to the first item so it carries tabIndex={0} and
32+
// arrow-key navigation has a starting point.
33+
const [activeIndex, setActiveIndex] = useState<number | null>(0);
34+
const listRef = useRef<Array<HTMLElement | null>>([]);
35+
const menuRef = useRef<HTMLUListElement | null>(null);
36+
const setMenuRef = useCallback(
37+
(node: HTMLUListElement | null) => {
38+
menuRef.current = node;
39+
if (typeof ref === "function") ref(node);
40+
else if (ref) ref.current = node;
41+
},
42+
[ref],
43+
);
44+
45+
// The menu opens from a raw contextmenu handler rather than a floating-ui
46+
// interaction, so FloatingFocusManager never seats focus for us. Do it
47+
// ourselves: pull focus into the first item once the menu is open, and hand
48+
// focus back to the opener (the event card, for a Shift+F10 keyboard open)
49+
// when it closes.
50+
const openerRef = useRef<HTMLElement | null>(
51+
typeof document === "undefined"
52+
? null
53+
: (document.activeElement as HTMLElement | null),
54+
);
55+
// Depend on the boolean, not `event`: `event` is a fresh object every
56+
// render, so a `[event]` effect would re-run and its cleanup would cancel
57+
// the pending timeout before it ever fired. `hasEvent` only flips when the
58+
// menu opens or closes.
59+
const hasEvent = !!event;
60+
useEffect(() => {
61+
if (!hasEvent) return;
62+
// Seat focus on the first item, deferred and retried for a short window.
63+
// A synchronous focus is a no-op (the portalled menu isn't laid out during
64+
// the commit), and opening writes the grid draft, whose re-render reclaims
65+
// focus to the source card once, shortly after. So we can't stop at the
66+
// first success: keep re-seating focus whenever it has fallen back out of
67+
// the menu, until the draft settles. setTimeout (not rAF) keeps firing in
68+
// a background tab. The in-menu guard leaves arrow-key focus alone.
69+
let attemptsLeft = 12;
70+
let id: ReturnType<typeof setTimeout>;
71+
const tryFocus = () => {
72+
const menu = menuRef.current;
73+
if (menu && !menu.contains(document.activeElement)) {
74+
menu.querySelector<HTMLElement>('[role="menuitem"]')?.focus();
75+
}
76+
if (attemptsLeft-- > 0) id = setTimeout(tryFocus, 16);
77+
};
78+
id = setTimeout(tryFocus, 0);
79+
return () => clearTimeout(id);
80+
}, [hasEvent]);
81+
useEffect(() => {
82+
return () => {
83+
const opener = openerRef.current;
84+
if (opener && document.contains(opener)) opener.focus();
85+
};
86+
}, []);
87+
3088
const dismiss = useDismiss(context, {
3189
outsidePress: (event) => {
3290
event.preventDefault(); // Prevents clicking another UI element when dismissing
@@ -35,11 +93,20 @@ export const ContextMenu = React.forwardRef<HTMLUListElement, ContextMenuProps>(
3593
},
3694
});
3795

38-
const click = useClick(context, { enabled: true });
39-
4096
const role = useRole(context, { role: "menu" });
4197

42-
const { getFloatingProps } = useInteractions([dismiss, click, role]);
98+
const listNavigation = useListNavigation(context, {
99+
listRef,
100+
activeIndex,
101+
onNavigate: setActiveIndex,
102+
loop: true,
103+
});
104+
105+
const { getFloatingProps, getItemProps } = useInteractions([
106+
dismiss,
107+
role,
108+
listNavigation,
109+
]);
43110

44111
if (!event) return null;
45112

@@ -48,7 +115,7 @@ export const ContextMenu = React.forwardRef<HTMLUListElement, ContextMenuProps>(
48115
return (
49116
<ul
50117
className="c-context-menu"
51-
ref={ref}
118+
ref={setMenuRef}
52119
style={
53120
{
54121
...style,
@@ -58,11 +125,19 @@ export const ContextMenu = React.forwardRef<HTMLUListElement, ContextMenuProps>(
58125
}
59126
{...getFloatingProps()}
60127
>
61-
{actions ? (
62-
<ContextMenuItemsView actions={actions} close={close} event={event} />
63-
) : (
64-
<ContextMenuItems event={event} close={close} />
65-
)}
128+
<ContextMenuNavContext.Provider
129+
value={{ getItemProps, listRef, activeIndex }}
130+
>
131+
{actions ? (
132+
<ContextMenuItemsView
133+
actions={actions}
134+
close={close}
135+
event={event}
136+
/>
137+
) : (
138+
<ContextMenuItems event={event} close={close} />
139+
)}
140+
</ContextMenuNavContext.Provider>
66141
</ul>
67142
);
68143
},

‎packages/web/src/components/ContextMenu/ContextMenuItems.test.tsx‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ describe("ContextMenuItems", () => {
124124

125125
renderWithTheme(<ContextMenuItems event={event} close={mockClose} />);
126126

127-
const editButton = screen.getByRole("button", { name: "Edit" });
127+
const editButton = screen.getByRole("menuitem", { name: "Edit" });
128128
await user.click(editButton);
129129

130130
expect(mockSetDraft).toHaveBeenCalled();
@@ -143,7 +143,7 @@ describe("ContextMenuItems", () => {
143143
pendingEventIds: ["pending-event-1"],
144144
});
145145

146-
const deleteButton = screen.getByRole("button", { name: "Delete" });
146+
const deleteButton = screen.getByRole("menuitem", { name: "Delete" });
147147
expect(deleteButton).not.toBeDisabled();
148148
await user.click(deleteButton);
149149

@@ -163,7 +163,7 @@ describe("ContextMenuItems", () => {
163163
pendingEventIds: ["pending-event-1"],
164164
});
165165

166-
const editButton = screen.getByRole("button", { name: "Edit" });
166+
const editButton = screen.getByRole("menuitem", { name: "Edit" });
167167
await user.click(editButton);
168168

169169
expect(mockSetDraft).toHaveBeenCalled();
@@ -182,7 +182,7 @@ describe("ContextMenuItems", () => {
182182
pendingEventIds: ["pending-event-1"],
183183
});
184184

185-
const duplicateButton = screen.getByRole("button", { name: "Duplicate" });
185+
const duplicateButton = screen.getByRole("menuitem", { name: "Duplicate" });
186186
await user.click(duplicateButton);
187187

188188
expect(mockDuplicateEvent).toHaveBeenCalled();
@@ -199,7 +199,7 @@ describe("ContextMenuItems", () => {
199199
pendingEventIds: ["pending-event-1"],
200200
});
201201

202-
const deleteButton = screen.getByRole("button", { name: "Delete" });
202+
const deleteButton = screen.getByRole("menuitem", { name: "Delete" });
203203
expect(deleteButton).not.toBeDisabled();
204204
expect(deleteButton).not.toHaveStyle({ cursor: "wait" });
205205
});
@@ -243,15 +243,15 @@ describe("ContextMenuItems read-only gate", () => {
243243
calendars: [readOnlyCalendar],
244244
});
245245

246-
expect(screen.getByRole("button", { name: "View" })).toBeInTheDocument();
246+
expect(screen.getByRole("menuitem", { name: "View" })).toBeInTheDocument();
247247
expect(
248-
screen.queryByRole("button", { name: "Edit" }),
248+
screen.queryByRole("menuitem", { name: "Edit" }),
249249
).not.toBeInTheDocument();
250250
expect(
251-
screen.getByRole("button", { name: "Duplicate" }),
251+
screen.getByRole("menuitem", { name: "Duplicate" }),
252252
).toBeInTheDocument();
253253
expect(
254-
screen.queryByRole("button", { name: "Delete" }),
254+
screen.queryByRole("menuitem", { name: "Delete" }),
255255
).not.toBeInTheDocument();
256256
});
257257

@@ -271,9 +271,9 @@ describe("ContextMenuItems read-only gate", () => {
271271
calendars: [writableCalendar],
272272
});
273273

274-
expect(screen.getByRole("button", { name: "View" })).toBeInTheDocument();
274+
expect(screen.getByRole("menuitem", { name: "View" })).toBeInTheDocument();
275275
expect(
276-
screen.queryByRole("button", { name: "Delete" }),
276+
screen.queryByRole("menuitem", { name: "Delete" }),
277277
).not.toBeInTheDocument();
278278
});
279279

@@ -292,7 +292,9 @@ describe("ContextMenuItems read-only gate", () => {
292292
calendars: [writableCalendar],
293293
});
294294

295-
expect(screen.getByRole("button", { name: "Edit" })).toBeInTheDocument();
296-
expect(screen.getByRole("button", { name: "Delete" })).toBeInTheDocument();
295+
expect(screen.getByRole("menuitem", { name: "Edit" })).toBeInTheDocument();
296+
expect(
297+
screen.getByRole("menuitem", { name: "Delete" }),
298+
).toBeInTheDocument();
297299
});
298300
});

‎packages/web/src/components/ContextMenu/ContextMenuItems.tsx‎

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { Copy, PenNib, Trash } from "@phosphor-icons/react";
22
import type React from "react";
3+
import { createContext, useContext } from "react";
34
import {
45
isEventReadOnly,
56
useCalendarLookup,
@@ -16,6 +17,21 @@ export interface ContextMenuAction {
1617
icon: React.ReactNode;
1718
}
1819

20+
// Supplied by ContextMenu so each item can wire into floating-ui's roving-focus
21+
// list navigation (arrow keys) and register its node in the shared listRef. The
22+
// menu still renders standalone (e.g. in tests) without a provider, in which
23+
// case items fall back to plain roving-free buttons.
24+
interface ContextMenuNavContextValue {
25+
getItemProps: (
26+
userProps?: React.HTMLProps<HTMLElement>,
27+
) => Record<string, unknown>;
28+
listRef: React.MutableRefObject<Array<HTMLElement | null>>;
29+
activeIndex: number | null;
30+
}
31+
32+
export const ContextMenuNavContext =
33+
createContext<ContextMenuNavContextValue | null>(null);
34+
1935
export interface ContextMenuItemsActions {
2036
delete: () => void;
2137
duplicate: () => void;
@@ -72,22 +88,37 @@ export function ContextMenuItemsView({
7288
]),
7389
];
7490

91+
const nav = useContext(ContextMenuNavContext);
92+
7593
return (
76-
<div id={ID_CONTEXT_MENU_ITEMS}>
77-
{menuActions.map((item) => (
78-
<button
79-
className="c-context-menu-item"
80-
key={item.id}
81-
type="button"
82-
onClick={() => {
83-
item.onClick();
84-
close();
85-
}}
86-
>
87-
{item.icon}
88-
<span className="text-l">{item.label}</span>
89-
</button>
90-
))}
94+
// role="none" keeps the menu -> menuitem ownership valid across this
95+
// container (the id is also how isContextMenuOpen() detects the menu).
96+
<div id={ID_CONTEXT_MENU_ITEMS} role="none">
97+
{menuActions.map((item, index) => {
98+
const select = () => {
99+
item.onClick();
100+
close();
101+
};
102+
const itemProps = nav
103+
? nav.getItemProps({ onClick: select })
104+
: { onClick: select };
105+
return (
106+
<button
107+
className="c-context-menu-item"
108+
key={item.id}
109+
type="button"
110+
role="menuitem"
111+
ref={(node) => {
112+
if (nav) nav.listRef.current[index] = node;
113+
}}
114+
tabIndex={nav ? (nav.activeIndex === index ? 0 : -1) : undefined}
115+
{...itemProps}
116+
>
117+
{item.icon}
118+
<span className="text-l">{item.label}</span>
119+
</button>
120+
);
121+
})}
91122
</div>
92123
);
93124
}

0 commit comments

Comments
 (0)