Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/context-menu-long-press-lift.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@astryxdesign/core': patch
---

[fix] A ContextMenu opened by a finger's long press stays open as the finger lifts: the compatibility `mousedown` the browser sends for that press as it lifts no longer reads as a press outside. Every other press outside the menu closes it, or leaves it open, as before.

@vjeux
52 changes: 49 additions & 3 deletions apps/storybook/stories/MenuPressShadowRootA11y.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

/**
* @file MenuPressShadowRootA11y.stories.tsx
* @input Uses Selector from core, React DOM's createRoot, an open shadow root
* @output The fixture the menu-press shadow-root spec drives in a real
* @input Uses Selector and ContextMenu from core, React DOM's createRoot, an
* open shadow root
* @output The fixtures the menu-press shadow-root spec drives in a real
* browser: a Selector rendered inside an open shadow root, with its value
* printed outside it.
* printed outside it, and a ContextMenu inside one whose host leaves a
* margin of its own around it, with the last item picked printed outside.
* @position The reproduction path for
* `packages/core/src/hooks/__tests__/MenuPressShadowRoot.a11y.chromium.spec.ts`.
*
Expand All @@ -21,6 +23,7 @@ import type {Meta, StoryObj} from '@storybook/react';
import {useEffect, useRef, useState} from 'react';
import {createRoot} from 'react-dom/client';
import {Selector} from '@astryxdesign/core/Selector';
import {ContextMenu} from '@astryxdesign/core/ContextMenu';

const meta: Meta = {
title: 'a11y/Menu press in a shadow root',
Expand Down Expand Up @@ -71,3 +74,46 @@ export const SelectorInShadowRoot: StoryObj = {
);
},
};

function ShadowContextMenu({onPick}: {onPick: (label: string) => void}) {
return (
<ContextMenu
items={[
{label: 'Cut', onClick: () => onPick('Cut')},
{label: 'Copy', onClick: () => onPick('Copy')},
]}>
<div style={{padding: 24, border: '1px dashed'}}>
Right-click inside the shadow root
</div>
</ContextMenu>
);
}

export const ContextMenuInShadowRoot: StoryObj = {
render: function ContextMenuInShadowRootStory() {
const hostRef = useRef<HTMLDivElement>(null);
const [picked, setPicked] = useState('');
useEffect(() => {
const host = hostRef.current;
if (host == null) {
return;
}
const shadow = host.shadowRoot ?? host.attachShadow({mode: 'open'});
const container = document.createElement('div');
shadow.append(container);
const root = createRoot(container);
root.render(<ShadowContextMenu onPick={setPicked} />);
return () => {
root.unmount();
container.remove();
};
}, []);
return (
<>
{/* The host's own padding is a press on the host, outside its tree. */}
<div ref={hostRef} data-testid="shadow-host" style={{padding: 48}} />
<output data-testid="picked-item">{picked}</output>
</>
);
},
};
3 changes: 2 additions & 1 deletion docs/architecture/layer-runtime.md
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,8 @@ independent:

- auto popovers use native light dismiss;
- native dialogs handle backdrop clicks locally;
- ContextMenu uses a local outside `mousedown` listener;
- ContextMenu uses a local outside `mousedown` listener, which skips the
compatibility mouse events of the finger press that opened the menu;
- Tooltip and HoverCard use hover/focus loss plus a touch-only outside listener;
- sheets own scrim and swipe behavior; and
- dropdown submenus encode their root-close relationship locally through
Expand Down
152 changes: 151 additions & 1 deletion packages/core/src/ContextMenu/ContextMenu.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,15 @@
import {useRef, useState} from 'react';
import {createPortal} from 'react-dom';
import {describe, it, expect, vi, beforeAll, beforeEach} from 'vitest';
import {render, screen, fireEvent, act, waitFor} from '@testing-library/react';
import {
render,
screen,
fireEvent,
act,
waitFor,
cleanup,
createEvent,
} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import {readFileSync} from 'node:fs';
import {ContextMenu} from './ContextMenu';
Expand Down Expand Up @@ -1402,3 +1410,145 @@ describe('ContextMenu inline trigger (triggerAs)', () => {
expect(screen.getByTestId('area').tagName).toBe('DIV');
});
});

describe('ContextMenu outside dismissal', () => {
const finger = (pointerId: number) => ({pointerType: 'touch', pointerId});

// Holds a finger on the trigger until the long-press opens the menu. Only
// the long-press delay is faked.
function longPress(trigger: HTMLElement, pointerId: number) {
vi.useFakeTimers({toFake: ['setTimeout', 'clearTimeout']});
try {
fireEvent.pointerDown(trigger, finger(pointerId));
fireEvent.touchStart(trigger, {touches: [{clientX: 20, clientY: 20}]});
act(() => {
vi.advanceTimersByTime(500);
});
} finally {
vi.useRealTimers();
}
}

// What a browser sends after a touch: the lift, then compatibility mouse
// events aimed at the element the finger held.
function lift(target: HTMLElement, pointerId: number) {
fireEvent.pointerUp(target, finger(pointerId));
fireEvent.touchEnd(target);
fireEvent.mouseDown(target);
fireEvent.mouseUp(target);
fireEvent.click(target);
}

function renderMenu(onOpenChange: (isOpen: boolean) => void) {
render(
<>
<ContextMenu
items={[{label: 'Cut'}, {label: 'Copy'}]}
data-testid="ctx"
onOpenChange={onOpenChange}>
<div>Long-press me</div>
</ContextMenu>
<button type="button">Outside</button>
</>,
);
return {
trigger: screen.getByTestId('ctx'),
outside: screen.getByRole('button', {name: 'Outside'}),
};
}

it('the long press that opened the menu does not close it, mid-hold or as it lifts; the next press outside does', () => {
const onOpenChange = vi.fn<(isOpen: boolean) => void>();
const {trigger, outside} = renderMenu(onOpenChange);
longPress(trigger, 1);
expect(onOpenChange).toHaveBeenLastCalledWith(true);
// iOS: a compatibility mousedown while the finger is still down.
fireEvent.mouseDown(trigger);
expect(onOpenChange).toHaveBeenLastCalledWith(true);
// Chromium: the compatibility events that follow the lift.
lift(trigger, 1);
expect(onOpenChange).not.toHaveBeenCalledWith(false);
// A new press is a new gesture: a mouse press outside closes the menu.
fireEvent.pointerDown(outside, {pointerType: 'mouse', pointerId: 9});
fireEvent.mouseDown(outside);
expect(onOpenChange).toHaveBeenLastCalledWith(false);
});

it('a finger long press that opens the menu through a pointer contextmenu (Chromium) does not close it as it lifts', () => {
const onOpenChange = vi.fn<(isOpen: boolean) => void>();
const {trigger, outside} = renderMenu(onOpenChange);
fireEvent.pointerDown(trigger, finger(8));
const contextmenu = createEvent.contextMenu(trigger, {
clientX: 20,
clientY: 15,
detail: 1,
});
Object.defineProperty(contextmenu, 'pointerType', {value: 'touch'});
fireEvent(trigger, contextmenu);
expect(onOpenChange).toHaveBeenLastCalledWith(true);
lift(trigger, 8);
expect(onOpenChange).not.toHaveBeenCalledWith(false);
// A right-click is not a finger: its gesture's next mousedown, outside,
// closes the menu.
fireEvent.pointerDown(trigger, {pointerType: 'mouse', pointerId: 1});
fireEvent.contextMenu(trigger, {clientX: 20, clientY: 15, detail: 1});
fireEvent.mouseDown(outside);
expect(onOpenChange).toHaveBeenLastCalledWith(false);
});

it('a long press that moves the open menu keeps it open as it lifts', () => {
const onOpenChange = vi.fn<(isOpen: boolean) => void>();
const {trigger} = renderMenu(onOpenChange);
fireEvent.contextMenu(trigger, {clientX: 50, clientY: 15, detail: 1});
expect(onOpenChange).toHaveBeenLastCalledWith(true);
longPress(trigger, 2);
lift(trigger, 2);
expect(onOpenChange).not.toHaveBeenCalledWith(false);
});

it('a tap outside closes the menu through its compatibility mousedown; a scroll that starts outside leaves it open', () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: this case pins today's outcome for a scroll that starts outside. spec:AST-094 FR32 (current, not yet implemented) says the menu "MUST close as a touch outside it begins to pan", so the FR32 work will have to invert it; saying so in the title or a comment keeps it from reading as the rule.

const onOpenChange = vi.fn<(isOpen: boolean) => void>();
const {trigger, outside} = renderMenu(onOpenChange);
fireEvent.contextMenu(trigger, {clientX: 50, clientY: 15, detail: 1});
// The browser takes this finger for a scroll: no compatibility events.
fireEvent.pointerDown(outside, finger(3));
fireEvent.pointerCancel(outside, finger(3));
expect(onOpenChange).toHaveBeenLastCalledWith(true);
fireEvent.pointerDown(outside, finger(4));
lift(outside, 4);
expect(onOpenChange).toHaveBeenLastCalledWith(false);
});

it('a mouse press or a pen press outside closes the menu at the press', () => {
for (const pointerType of ['mouse', 'pen']) {
const onOpenChange = vi.fn<(isOpen: boolean) => void>();
const {trigger, outside} = renderMenu(onOpenChange);
fireEvent.contextMenu(trigger, {clientX: 50, clientY: 15, detail: 1});
fireEvent.pointerDown(outside, {pointerType, pointerId: 7});
fireEvent.mouseDown(outside);
expect(onOpenChange).toHaveBeenLastCalledWith(false);
cleanup();
}
});

it('presses that send no mousedown leave the menu open, as before: a cancelled pointerdown, a finger drag, a second finger', () => {
const onOpenChange = vi.fn<(isOpen: boolean) => void>();
const {trigger, outside} = renderMenu(onOpenChange);
fireEvent.contextMenu(trigger, {clientX: 50, clientY: 15, detail: 1});
// A control that cancels its pointerdown (a NumberInput stepper, a
// resize handle): the browser sends no mousedown.
fireEvent.pointerDown(outside, {pointerType: 'mouse', pointerId: 1});
fireEvent.pointerUp(outside, {pointerType: 'mouse', pointerId: 1});
// A finger drag on a surface that does not pan (a Slider), lifted.
fireEvent.pointerDown(outside, finger(5));
fireEvent.pointerMove(outside, finger(5));
fireEvent.pointerUp(outside, finger(5));
// A second finger's tap.
fireEvent.pointerDown(outside, finger(6));
fireEvent.pointerDown(outside, finger(7));
fireEvent.pointerUp(outside, finger(7));
fireEvent.pointerCancel(outside, finger(6));
expect(onOpenChange).toHaveBeenLastCalledWith(true);
expect(onOpenChange).not.toHaveBeenCalledWith(false);
});
});
25 changes: 24 additions & 1 deletion packages/core/src/ContextMenu/ContextMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {Icon} from '../Icon';
import {useLayer} from '../Layer/useLayer';
import {LayerDepthProvider} from '../Layer/LayerDepthContext';
import {useLayerDismissal} from '../Layer/useLayerDismissal';
import {currentGesture} from '../Layer/gestureCounter';
import {MenuBottomSheetActionList} from '../DropdownMenu/MenuBottomSheetActionList';
import {renderDropdownItems} from '../DropdownMenu/renderDropdownItems';
import {
Expand Down Expand Up @@ -354,6 +355,11 @@ export function ContextMenu({
// Element focused before the menu opened, restored when it closes so focus
// does not fall to <body> after Escape or outside-click dismissal.
const triggerFocusRef = useRef<HTMLElement | null>(null);
// The gesture of the finger press that opened the menu (`Layer/
// gestureCounter`), null when a mouse or the keyboard opened it: the
// browser's compatibility mouse events for that same press are not presses
// outside.
const openingGestureRef = useRef<number | null>(null);

const [isOpen, setIsOpen] = useState(false);
const [submenuPath, setSubmenuPath] = useState<ContextMenuItemData[]>([]);
Expand Down Expand Up @@ -461,7 +467,10 @@ export function ContextMenu({
// Dismiss on any click outside the menu. We use popover="manual" (not
// "auto") because the native light-dismiss treats the mouseup from the
// opening right-click as a dismiss event. Handling it ourselves via
// mousedown avoids that race.
// mousedown avoids that race. A touch's compatibility mousedown for the
// press that opened the menu (sent mid-hold by iOS, or as a long press
// lifts by Chromium) belongs to that press's gesture, so it is not read as
// a press outside; any later press is a new gesture and closes as before.
useEffect(() => {
if (!isOpen || usesBottomSheet) {
return;
Expand All @@ -478,6 +487,9 @@ export function ContextMenu({
return;
}
judged.add(e);
if (currentGesture() === openingGestureRef.current) {
return;
}
if (menu && !menu.contains(e.target as Node)) {
closeMenu();
}
Expand Down Expand Up @@ -589,6 +601,14 @@ export function ContextMenu({
const localX = isKeyboardInvoked || !rect ? 0 : e.clientX - rect.left;
const localY =
isKeyboardInvoked || !rect ? (rect?.height ?? 0) : e.clientY - rect.top;
// A finger's long press reaches here as a pointer `contextmenu` where
// the browser sends one (Chromium); its compatibility mousedown then
// belongs to the opening press. A mouse or the keyboard opens without
// one, so every later mousedown is a press outside.
openingGestureRef.current =
(e.nativeEvent as Partial<PointerEvent>).pointerType === 'touch'
? currentGesture()
: null;
openAtLocalPoint(localX, localY, e.currentTarget as HTMLElement);
},
[isDisabled, openAtLocalPoint, getAnchorBaseRect],
Expand All @@ -610,6 +630,9 @@ export function ContextMenu({
onLongPress: useCallback(
(point: {x: number; y: number}) => {
const rect = getAnchorBaseRect();
// The finger is still down: the compatibility mousedown iOS sends
// mid-hold, or Chromium as it lifts, belongs to this press.
openingGestureRef.current = currentGesture();
openAtLocalPoint(
rect ? point.x - rect.left : point.x,
rect ? point.y - rect.top : point.y,
Expand Down
Loading
Loading