diff --git a/docs/menus.md b/docs/menus.md index 4193be54f..580762d17 100644 --- a/docs/menus.md +++ b/docs/menus.md @@ -26,6 +26,17 @@ when you have actually looked at it on a phone. `ContextMenu` is the exception: it defaults to `compactMode="sheet"` and enables native long press. Disable mobile triggering explicitly on draggable rows, where long press belongs to drag instead. +## Selecting an item on iOS + +An item that closes the menu runs its action after a fixed grace period on iOS, not immediately: +a native presenter launched while UIKit is still tearing down the surface can hang. Both surfaces +unmount the moment the menu closes, so the wait is a timer rather than the surface's own dismissal +callback — a callback fired from inside a subtree that is being removed is racing its own removal, +and it loses. Selections used to be dropped entirely for exactly that reason. + +The consequence for callers: an `onSelect` on iOS runs a beat after the press. Don't add a second +delay on top of it, and don't read state that the same press mutated. + ## Pages A submenu is a page, declared as data on the surface and reached by a `MenuSubTrigger` whose `id` diff --git a/packages/app/src/components/ui/menu/menu-context.test.tsx b/packages/app/src/components/ui/menu/menu-context.test.tsx new file mode 100644 index 000000000..40b45b934 --- /dev/null +++ b/packages/app/src/components/ui/menu/menu-context.test.tsx @@ -0,0 +1,63 @@ +/** + * @vitest-environment jsdom + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { Platform } from "react-native"; +import { IOS_TEARDOWN_GRACE_MS, useMenuState } from "./menu-context"; + +/** + * `Platform.OS` is a plain property on the react-native-web double, so the iOS branch is + * reachable by writing to it. The grace period only exists on iOS, which is exactly the platform + * whose selections used to be dropped. + */ +const originalPlatform = Platform.OS; + +function setPlatform(os: string) { + Object.assign(Platform, { OS: os }); +} + +beforeEach(() => vi.useFakeTimers()); + +afterEach(() => { + vi.useRealTimers(); + setPlatform(originalPlatform); +}); + +describe("useMenuState selectItem on iOS", () => { + beforeEach(() => setPlatform("ios")); + + it("runs a closing item's action once the surface has had time to go away", () => { + const onSelect = vi.fn(); + const { result } = renderHook(() => useMenuState({ defaultOpen: true })); + + act(() => result.current.selectItem(onSelect, true)); + expect(result.current.open).toBe(false); + expect(onSelect).not.toHaveBeenCalled(); + + act(() => vi.advanceTimersByTime(IOS_TEARDOWN_GRACE_MS)); + expect(onSelect).toHaveBeenCalledTimes(1); + }); + + it("leaves the menu open for an item that does not close it", () => { + const onSelect = vi.fn(); + const { result } = renderHook(() => useMenuState({ defaultOpen: true })); + + act(() => result.current.selectItem(onSelect, false)); + expect(result.current.open).toBe(true); + expect(onSelect).toHaveBeenCalledTimes(1); + }); +}); + +describe("useMenuState selectItem elsewhere", () => { + beforeEach(() => setPlatform("android")); + + it("runs a closing item's action immediately", () => { + const onSelect = vi.fn(); + const { result } = renderHook(() => useMenuState({ defaultOpen: true })); + + act(() => result.current.selectItem(onSelect, true)); + expect(result.current.open).toBe(false); + expect(onSelect).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/app/src/components/ui/menu/menu-context.ts b/packages/app/src/components/ui/menu/menu-context.ts index ebc34fe9d..8dceb70ec 100644 --- a/packages/app/src/components/ui/menu/menu-context.ts +++ b/packages/app/src/components/ui/menu/menu-context.ts @@ -24,12 +24,14 @@ export type MenuPresentation = "popover" | "sheet"; /** What a menu does on a compact screen. Wide screens are always a popover. */ export type MenuCompactMode = "popover" | "sheet"; +/** How long UIKit is given to finish removing the menu surface before an action runs. */ +export const IOS_TEARDOWN_GRACE_MS = 250; + export interface MenuContextValue { open: boolean; setOpen: (open: boolean) => void; /** Runs an item's action, closing the menu first when the item asks for it. */ selectItem: (onSelect: (() => void) | undefined, closeOnSelect: boolean) => void; - flushPendingSelect: () => void; triggerRef: React.RefObject; /** Point anchor for menus opened from a gesture rather than a trigger box. */ anchorRect: Rect | null; @@ -104,7 +106,6 @@ export function useMenuState({ compactMode?: MenuCompactMode; }): MenuContextValue { const triggerRef = useRef(null); - const pendingSelectRef = useRef<(() => void) | null>(null); const [anchorRect, setAnchorRect] = useState(null); const [path, setPath] = useState(MENU_ROOT_PATH); const isCompact = useIsCompactFormFactor(); @@ -126,21 +127,6 @@ export function useMenuState({ [setIsOpenState], ); - const flushPendingSelect = useCallback(() => { - const pendingSelect = pendingSelectRef.current; - pendingSelectRef.current = null; - if (!pendingSelect) return; - - if (Platform.OS === "ios") { - // Native presenters such as PHPicker can hang if launched while an RN - // Modal is still completing dismissal on UIKit's side. - setTimeout(pendingSelect, 250); - return; - } - - pendingSelect(); - }, []); - const selectItem = useCallback( (onSelect: (() => void) | undefined, closeOnSelect: boolean) => { if (!closeOnSelect) { @@ -148,14 +134,19 @@ export function useMenuState({ return; } + setOpen(false); + if (!onSelect) return; + if (Platform.OS === "ios") { - pendingSelectRef.current = onSelect ?? null; - setOpen(false); + // Native presenters such as PHPicker can hang if launched while UIKit is still + // tearing down the surface this menu was drawn in. The wait is timed rather than + // driven by the surface's own dismissal callback: both surfaces unmount as soon as + // the menu closes, so any such callback is racing its own removal and usually loses. + setTimeout(onSelect, IOS_TEARDOWN_GRACE_MS); return; } - setOpen(false); - onSelect?.(); + onSelect(); }, [setOpen], ); @@ -177,7 +168,6 @@ export function useMenuState({ open: isOpen, setOpen, selectItem, - flushPendingSelect, triggerRef, anchorRect, setAnchorRect, @@ -187,17 +177,6 @@ export function useMenuState({ closeSub, goBack, }), - [ - isOpen, - setOpen, - selectItem, - flushPendingSelect, - anchorRect, - presentation, - path, - openSub, - closeSub, - goBack, - ], + [isOpen, setOpen, selectItem, anchorRect, presentation, path, openSub, closeSub, goBack], ); } diff --git a/packages/app/src/components/ui/menu/menu-overlay.tsx b/packages/app/src/components/ui/menu/menu-overlay.tsx index 460e7c49e..ba5f3cf01 100644 --- a/packages/app/src/components/ui/menu/menu-overlay.tsx +++ b/packages/app/src/components/ui/menu/menu-overlay.tsx @@ -384,12 +384,10 @@ export function AnchoredSurface({ export function MenuOverlay({ visible, onClose, - onDismiss, children, }: { visible: boolean; onClose: () => void; - onDismiss?: () => void; children: ReactElement | null; }): ReactElement | null { const floatingLayer = useOverlayLayer("floating"); @@ -438,7 +436,6 @@ export function MenuOverlay({ transparent animationType="none" statusBarTranslucent={Platform.OS === "android"} - onDismiss={onDismiss} onRequestClose={onClose} > {overlay} diff --git a/packages/app/src/components/ui/menu/menu-surface.tsx b/packages/app/src/components/ui/menu/menu-surface.tsx index 0daa7f1f2..3ee52a7ec 100644 --- a/packages/app/src/components/ui/menu/menu-surface.tsx +++ b/packages/app/src/components/ui/menu/menu-surface.tsx @@ -206,7 +206,7 @@ function MenuPopoverSurface({ return ( - + <>