From 8503b04899ee65b3c5eeba0e0f42c03f0b57d90a Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Fri, 22 Nov 2024 10:23:13 -0800 Subject: [PATCH 1/3] refactor: Remove custom hit testing in usePress --- .../@react-aria/interactions/src/usePress.ts | 33 +++++++++---------- .../interactions/test/usePress.test.js | 12 ++++++- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/packages/@react-aria/interactions/src/usePress.ts b/packages/@react-aria/interactions/src/usePress.ts index a90b602cc39..247b37139cc 100644 --- a/packages/@react-aria/interactions/src/usePress.ts +++ b/packages/@react-aria/interactions/src/usePress.ts @@ -433,7 +433,13 @@ export function usePress(props: PressHookProps): PressResult { shouldStopPropagation = triggerPressStart(e, state.pointerType); - addGlobalListener(getOwnerDocument(e.currentTarget), 'pointermove', onPointerMove, false); + // Release pointer capture so that touch interactions can leave the original target. + // This enables onPointerLeave and onPointerEnter to fire. + let target = e.target as Element; + if ('releasePointerCapture' in target) { + target.releasePointerCapture(e.pointerId); + } + addGlobalListener(getOwnerDocument(e.currentTarget), 'pointerup', onPointerUp, false); addGlobalListener(getOwnerDocument(e.currentTarget), 'pointercancel', onPointerCancel, false); } @@ -467,27 +473,20 @@ export function usePress(props: PressHookProps): PressResult { } // Only handle left clicks - // Safari on iOS sometimes fires pointerup events, even - // when the touch isn't over the target, so double check. - if (e.button === 0 && isOverTarget(e, e.currentTarget)) { + if (e.button === 0) { triggerPressUp(e, state.pointerType || e.pointerType); } }; - // Safari on iOS < 13.2 does not implement pointerenter/pointerleave events correctly. - // Use pointer move events instead to implement our own hit testing. - // See https://bugs.webkit.org/show_bug.cgi?id=199803 - let onPointerMove = (e: PointerEvent) => { - if (e.pointerId !== state.activePointerId) { - return; + pressProps.onPointerEnter = (e) => { + if (e.pointerId === state.activePointerId && state.target && !state.isOverTarget && state.pointerType != null) { + state.isOverTarget = true; + triggerPressStart(createEvent(state.target, e), state.pointerType); } + }; - if (state.target && isOverTarget(e, state.target)) { - if (!state.isOverTarget && state.pointerType != null) { - state.isOverTarget = true; - triggerPressStart(createEvent(state.target, e), state.pointerType); - } - } else if (state.target && state.isOverTarget && state.pointerType != null) { + pressProps.onPointerLeave = (e) => { + if (e.pointerId === state.activePointerId && state.target && state.isOverTarget && state.pointerType != null) { state.isOverTarget = false; triggerPressEnd(createEvent(state.target, e), state.pointerType, false); cancelOnPointerExit(e); @@ -496,7 +495,7 @@ export function usePress(props: PressHookProps): PressResult { let onPointerUp = (e: PointerEvent) => { if (e.pointerId === state.activePointerId && state.isPressed && e.button === 0 && state.target) { - if (isOverTarget(e, state.target) && state.pointerType != null) { + if (state.target.contains(e.target as Element) && state.pointerType != null) { triggerPressEnd(createEvent(state.target, e), state.pointerType); } else if (state.isOverTarget && state.pointerType != null) { triggerPressEnd(createEvent(state.target, e), state.pointerType, false); diff --git a/packages/@react-aria/interactions/test/usePress.test.js b/packages/@react-aria/interactions/test/usePress.test.js index 23273ff726f..b5485f3fc6d 100644 --- a/packages/@react-aria/interactions/test/usePress.test.js +++ b/packages/@react-aria/interactions/test/usePress.test.js @@ -141,10 +141,15 @@ describe('usePress', function () { ); let el = res.getByText('test'); + el.releasePointerCapture = jest.fn(); fireEvent(el, pointerEvent('pointerdown', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); + expect(el.releasePointerCapture).toHaveBeenCalled(); fireEvent(el, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); - fireEvent(el, pointerEvent('pointerup', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); + // react listens for pointerout and pointerover instead of pointerleave and pointerenter... + fireEvent(el, pointerEvent('pointerout', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); + fireEvent(document, pointerEvent('pointerup', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); fireEvent(el, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); + fireEvent(el, pointerEvent('pointerover', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); expect(events).toEqual([ { @@ -182,7 +187,10 @@ describe('usePress', function () { events = []; fireEvent(el, pointerEvent('pointerdown', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); fireEvent(el, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); + // react listens for pointerout and pointerover instead of pointerleave and pointerenter... + fireEvent(el, pointerEvent('pointerout', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); fireEvent(el, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); + fireEvent(el, pointerEvent('pointerover', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); fireEvent(el, pointerEvent('pointerup', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); expect(events).toEqual([ @@ -387,7 +395,9 @@ describe('usePress', function () { let el = res.getByText('test'); fireEvent(el, pointerEvent('pointerdown', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); fireEvent(el, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); + fireEvent(el, pointerEvent('pointerout', {pointerId: 1, pointerType: 'mouse', clientX: 100, clientY: 100})); fireEvent(el, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); + fireEvent(el, pointerEvent('pointerover', {pointerId: 1, pointerType: 'mouse', clientX: 0, clientY: 0})); expect(events).toEqual([ { From b776da9e66ed88189e25083f36dfa43009137ebf Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Tue, 26 Nov 2024 18:30:50 -0800 Subject: [PATCH 2/3] Only allow dragging to select an item with mouse, not touch --- packages/@react-aria/menu/src/useMenuItem.ts | 30 ++++++++++++++----- .../selection/src/useSelectableItem.ts | 10 +++++-- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/packages/@react-aria/menu/src/useMenuItem.ts b/packages/@react-aria/menu/src/useMenuItem.ts index 328985b0ac6..5c514a98d47 100644 --- a/packages/@react-aria/menu/src/useMenuItem.ts +++ b/packages/@react-aria/menu/src/useMenuItem.ts @@ -110,7 +110,7 @@ export function useMenuItem(props: AriaMenuItemProps, state: TreeState, re 'aria-haspopup': hasPopup, onPressStart: pressStartProp, onPressUp: pressUpProp, - onPress, + onPress: pressProp, onPressChange, onPressEnd, onHoverStart: hoverStartProp, @@ -196,20 +196,34 @@ export function useMenuItem(props: AriaMenuItemProps, state: TreeState, re pressStartProp?.(e); }; + let maybeClose = () => { + // Pressing a menu item should close by default in single selection mode but not multiple + // selection mode, except if overridden by the closeOnSelect prop. + if (!isTrigger && onClose && (closeOnSelect ?? (selectionManager.selectionMode !== 'multiple' || selectionManager.isLink(key)))) { + onClose(); + } + }; + let onPressUp = (e: PressEvent) => { - if (e.pointerType !== 'keyboard') { + // If interacting with mouse, allow the user to mouse down on the trigger button, + // drag, and release over an item (matching native behavior). + if (e.pointerType === 'mouse') { performAction(e); - - // Pressing a menu item should close by default in single selection mode but not multiple - // selection mode, except if overridden by the closeOnSelect prop. - if (!isTrigger && onClose && (closeOnSelect ?? (selectionManager.selectionMode !== 'multiple' || selectionManager.isLink(key)))) { - onClose(); - } + maybeClose(); } pressUpProp?.(e); }; + let onPress = (e: PressEvent) => { + if (e.pointerType !== 'keyboard' && e.pointerType !== 'mouse') { + performAction(e); + maybeClose(); + } + + pressProp?.(e); + }; + let {itemProps, isFocused} = useSelectableItem({ selectionManager: selectionManager, key, diff --git a/packages/@react-aria/selection/src/useSelectableItem.ts b/packages/@react-aria/selection/src/useSelectableItem.ts index 3a3d80c731a..650e8e72ff7 100644 --- a/packages/@react-aria/selection/src/useSelectableItem.ts +++ b/packages/@react-aria/selection/src/useSelectableItem.ts @@ -241,7 +241,7 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte } }; - // If allowsDifferentPressOrigin, make selection happen on pressUp (e.g. open menu on press down, selection on menu item happens on press up.) + // If allowsDifferentPressOrigin and interacting with mouse, make selection happen on pressUp (e.g. open menu on press down, selection on menu item happens on press up.) // Otherwise, have selection happen onPress (prevents listview row selection when clicking on interactable elements in the row) if (!allowsDifferentPressOrigin) { itemPressProps.onPress = (e) => { @@ -257,12 +257,16 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte }; } else { itemPressProps.onPressUp = hasPrimaryAction ? undefined : (e) => { - if (e.pointerType !== 'keyboard' && allowsSelection) { + if (e.pointerType === 'mouse' && allowsSelection) { onSelect(e); } }; - itemPressProps.onPress = hasPrimaryAction ? performAction : undefined; + itemPressProps.onPress = hasPrimaryAction ? performAction : (e) => { + if (e.pointerType !== 'keyboard' && e.pointerType !== 'mouse' && allowsSelection) { + onSelect(e); + } + }; } } else { itemPressProps.onPressStart = (e) => { From 8bc988039e91922c584bc6a7ec9d8ae2bb0c8b0a Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Tue, 26 Nov 2024 18:56:31 -0800 Subject: [PATCH 3/3] Don't close when dragging outside submenu --- packages/@react-spectrum/menu/src/Menu.tsx | 10 ---------- .../@react-spectrum/menu/src/MenuTrigger.tsx | 18 +++++++++++++++--- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/@react-spectrum/menu/src/Menu.tsx b/packages/@react-spectrum/menu/src/Menu.tsx index c1397a4eccf..f5cbeab087f 100644 --- a/packages/@react-spectrum/menu/src/Menu.tsx +++ b/packages/@react-spectrum/menu/src/Menu.tsx @@ -24,7 +24,6 @@ import {mergeProps, useLayoutEffect, useSlotId, useSyncRef} from '@react-aria/ut import React, {ReactElement, useContext, useEffect, useRef, useState} from 'react'; import {SpectrumMenuProps} from '@react-types/menu'; import styles from '@adobe/spectrum-css-temp/components/menu/vars.css'; -import {useInteractOutside} from '@react-aria/interactions'; import {useLocale, useLocalizedStringFormatter} from '@react-aria/i18n'; import {useMenu} from '@react-aria/menu'; import {useTreeState} from '@react-stately/tree'; @@ -70,15 +69,6 @@ export const Menu = React.forwardRef(function Menu(props: Spec let nextMenuLevel = state.collection.getItem(nextMenuLevelKey); hasOpenSubmenu = nextMenuLevel != null; } - useInteractOutside({ - ref: domRef, - onInteractOutside: (e) => { - if (!popoverContainer?.contains(e.target as Node) && !trayContainerRef.current?.contains(e.target as Node)) { - rootMenuTriggerState?.close(); - } - }, - isDisabled: isSubmenu || !hasOpenSubmenu - }); return ( diff --git a/packages/@react-spectrum/menu/src/MenuTrigger.tsx b/packages/@react-spectrum/menu/src/MenuTrigger.tsx index cacb37f3d8b..8c9e59d8cc7 100644 --- a/packages/@react-spectrum/menu/src/MenuTrigger.tsx +++ b/packages/@react-spectrum/menu/src/MenuTrigger.tsx @@ -10,12 +10,12 @@ * governing permissions and limitations under the License. */ -import {classNames, SlotProvider, useDOMRef, useIsMobileDevice} from '@react-spectrum/utils'; +import {classNames, SlotProvider, unwrapDOMRef, useDOMRef, useIsMobileDevice} from '@react-spectrum/utils'; import {DOMRef} from '@react-types/shared'; import {MenuContext} from './context'; import {Placement} from '@react-types/overlays'; import {Popover, Tray} from '@react-spectrum/overlays'; -import {PressResponder} from '@react-aria/interactions'; +import {PressResponder, useInteractOutside} from '@react-aria/interactions'; import React, {forwardRef, Fragment, useRef} from 'react'; import {SpectrumMenuTriggerProps} from '@react-types/menu'; import styles from '@adobe/spectrum-css-temp/components/menu/vars.css'; @@ -74,17 +74,29 @@ export const MenuTrigger = forwardRef(function MenuTrigger(props: SpectrumMenuTr state }; + // Close when clicking outside the root menu when a submenu is open. + let rootOverlayRef = useRef(null); + let rootOverlayDomRef = unwrapDOMRef(rootOverlayRef); + useInteractOutside({ + ref: rootOverlayDomRef, + onInteractOutside: () => { + state?.close(); + }, + isDisabled: !state.isOpen || state.expandedKeysStack.length === 0 + }); + // On small screen devices, the menu is rendered in a tray, otherwise a popover. let overlay; if (isMobile) { overlay = ( - + {menu} ); } else { overlay = (