diff --git a/.changeset/layer-dismiss-gesture.md b/.changeset/layer-dismiss-gesture.md new file mode 100644 index 0000000000000..eb0fdda9f2ca8 --- /dev/null +++ b/.changeset/layer-dismiss-gesture.md @@ -0,0 +1,7 @@ +--- +'@astryxdesign/core': patch +--- + +[fix] Popup triggers no longer fight the browser's own light dismiss: pressing the button of an open Selector, MultiSelector, ComplexSelector, DropdownMenu or Popover closes it once instead of closing and reopening, and a clear or status button sitting on the trigger no longer dismisses the popup it belongs to + +@cixzhang diff --git a/packages/core/src/ComplexSelector/ComplexSelector.test.tsx b/packages/core/src/ComplexSelector/ComplexSelector.test.tsx index 079a23451458f..3a6f2cd06d104 100644 --- a/packages/core/src/ComplexSelector/ComplexSelector.test.tsx +++ b/packages/core/src/ComplexSelector/ComplexSelector.test.tsx @@ -417,4 +417,34 @@ describe('ComplexSelector popup theme target', () => { document.querySelector('.astryx-complex-selector-popup'), ).not.toBeNull(); }); + + it('stays closed when the trigger click follows its own light dismiss (#5004)', async () => { + const user = userEvent.setup(); + render( + + {() => } + , + ); + const trigger = screen.getByRole('button', {name: 'Fruit blend'}); + await user.click(trigger); + expect(trigger).toHaveAttribute('aria-expanded', 'true'); + + // The browser dismissed the popup on pointerup and queued the toggle. When + // that event lands before the click — WebKit, or any engine under load — + // the click used to read a closed popup and reopen it. + const popover = document.querySelector('[popover]') as HTMLElement; + act(() => { + popover.dispatchEvent( + Object.assign(new Event('toggle'), { + oldState: 'open', + newState: 'closed', + }), + ); + }); + // Synchronously: the click falls inside the one gesture the guard covers, + // as it does in a browser a few milliseconds behind the dismissal. + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + }); }); diff --git a/packages/core/src/ComplexSelector/ComplexSelector.tsx b/packages/core/src/ComplexSelector/ComplexSelector.tsx index c6f27f27778f3..2ab4af58d4a95 100644 --- a/packages/core/src/ComplexSelector/ComplexSelector.tsx +++ b/packages/core/src/ComplexSelector/ComplexSelector.tsx @@ -373,14 +373,12 @@ export function ComplexSelector({ .join(' ') || undefined; const triggerRef = useRef(null); - const lastHideTimeRef = useRef(0); const [isPending, startTransition] = useTransition(); const [optimisticValue, setOptimisticValue] = useOptimistic(value); const isBusy = isLoading || isPending; const handlePopoverHide = useCallback(() => { - lastHideTimeRef.current = Date.now(); triggerRef.current?.focus(); }, []); @@ -395,7 +393,7 @@ export function ComplexSelector({ const isOpen = popover.isOpen; const handleTriggerClick = useCallback(() => { - if (isDisabled || Date.now() - lastHideTimeRef.current < 50) { + if (isDisabled) { return; } if (popover.isOpen) { diff --git a/packages/core/src/DropdownMenu/DropdownMenu.test.tsx b/packages/core/src/DropdownMenu/DropdownMenu.test.tsx index c876adf252c0e..34cd910839962 100644 --- a/packages/core/src/DropdownMenu/DropdownMenu.test.tsx +++ b/packages/core/src/DropdownMenu/DropdownMenu.test.tsx @@ -10,7 +10,7 @@ */ import {describe, it, expect, vi, beforeEach} from 'vitest'; -import {render, screen, fireEvent, waitFor} from '@testing-library/react'; +import {render, screen, fireEvent, waitFor, act} from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import {useState} from 'react'; import {DropdownMenu} from './DropdownMenu'; @@ -315,10 +315,7 @@ describe('DropdownMenu', () => { }); describe('DropdownMenu light-dismiss race', () => { - it('does not re-open the menu when a click follows a hide within the guard window', () => { - // Reproduces the iOS Safari race: pointerdown fires light-dismiss before - // the subsequent click on the trigger; without the guard, the click would - // immediately re-open the menu in the same tap. + function openMenu() { render( { data-testid="astryx-dropdown-menu" />, ); - const trigger = screen.getByTestId('astryx-dropdown-menu'); - fireEvent.click(trigger); // open - fireEvent.click(trigger); // close (stamps guard) - fireEvent.click(trigger); // would re-open without guard + fireEvent.pointerDown(trigger); + fireEvent.click(trigger); expect(HTMLElement.prototype.showPopover).toHaveBeenCalledTimes(1); - expect(HTMLElement.prototype.hidePopover).toHaveBeenCalledTimes(1); + return trigger; + } + + /** + * The browser dismisses the menu on pointerup and queues the `toggle` event; + * on the engines that lose the race it reaches React before the trigger's + * own click, which then reads a closed menu. + */ + function lightDismiss() { + const popover = document.querySelector('[popover]') as HTMLElement; + act(() => { + popover.dispatchEvent( + Object.assign(new Event('toggle'), { + oldState: 'open', + newState: 'closed', + }), + ); + }); + } + + it('does not re-open when the trigger click follows its own light dismiss', () => { + const trigger = openMenu(); + + lightDismiss(); + fireEvent.click(trigger); + + expect(HTMLElement.prototype.showPopover).toHaveBeenCalledTimes(1); + }); + + it('re-opens on a press of its own after a light dismiss', () => { + const trigger = openMenu(); + + lightDismiss(); + fireEvent.click(trigger); + fireEvent.pointerDown(trigger); + fireEvent.click(trigger); + + expect(HTMLElement.prototype.showPopover).toHaveBeenCalledTimes(2); }); }); diff --git a/packages/core/src/DropdownMenu/DropdownMenu.tsx b/packages/core/src/DropdownMenu/DropdownMenu.tsx index 78600c22d563e..4a54260c6b862 100644 --- a/packages/core/src/DropdownMenu/DropdownMenu.tsx +++ b/packages/core/src/DropdownMenu/DropdownMenu.tsx @@ -261,14 +261,8 @@ export function DropdownMenu({ const isControlled = controlledIsOpen !== undefined; const isOpen = isControlled ? controlledIsOpen : internalIsOpen; - // Track when the menu was last hidden so a near-simultaneous trigger - // click — e.g. on iOS Safari where pointerdown fires light-dismiss - // before the trigger's click event — can't immediately re-open it. - const lastHideTimeRef = useRef(0); - // Close menu + return focus to trigger const handleLayerHide = useCallback(() => { - lastHideTimeRef.current = Date.now(); onOpenChange?.(false); if (!isControlled) { setInternalIsOpen(false); @@ -422,11 +416,8 @@ export function DropdownMenu({ const handleButtonClick = useCallback( (e: React.MouseEvent) => { - // If the menu was just closed by light dismiss (e.g. iOS Safari fires - // pointerdown → hide before the trigger's click), the click would - // otherwise immediately re-open it. Short-circuit within the guard - // window. - if (Date.now() - lastHideTimeRef.current < 50) { + // The click that light-dismissed the menu is not a request to reopen it. + if (popover.wasJustDismissed()) { return; } onClick?.(); diff --git a/packages/core/src/Field/InputClearButton.tsx b/packages/core/src/Field/InputClearButton.tsx index 448d252a057e6..6cac959e98632 100644 --- a/packages/core/src/Field/InputClearButton.tsx +++ b/packages/core/src/Field/InputClearButton.tsx @@ -69,6 +69,13 @@ const styles = stylex.create({ export interface InputClearButtonProps { label: string; onClick: (e: React.MouseEvent) => void; + /** + * Pointer and capture-phase click handlers, for inputs that render this + * button beside an open layer: spread `keepOpenProps` so pressing the clear + * button does not light-dismiss the layer it sits next to. + */ + onPointerDown?: React.PointerEventHandler; + onClickCapture?: React.MouseEventHandler; xstyle?: stylex.StyleXStyles; /** * Extra class(es) for the clear glyph itself, merged onto the shared @@ -83,6 +90,8 @@ export interface InputClearButtonProps { export function InputClearButton({ label, onClick, + onPointerDown, + onClickCapture, xstyle, iconClassName, }: InputClearButtonProps): ReactNode { @@ -107,6 +116,8 @@ export function InputClearButton({ /> } onClick={onClick} + onPointerDown={onPointerDown} + onClickCapture={onClickCapture} isIconOnly xstyle={[styles.button, xstyle]} /> diff --git a/packages/core/src/Layer/gestureCounter.ts b/packages/core/src/Layer/gestureCounter.ts new file mode 100644 index 0000000000000..7fa77ef99e5dc --- /dev/null +++ b/packages/core/src/Layer/gestureCounter.ts @@ -0,0 +1,45 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +'use client'; + +/** + * @file gestureCounter.ts + * @input Listens for pointerdown and keydown on the document + * @output Exports currentGesture, a counter identifying the user gesture in + * flight + * @position Internal to Layer; used by useLayer to tell a click that belongs + * to a dismissing press from a fresh one + * + * A browser light-dismiss and the trigger's own click come from ONE press, and + * which of them React sees first is a race. Comparing timestamps against a + * window guesses; counting gestures does not. The counter advances on every + * new press or keystroke, so "the click from the gesture that dismissed the + * layer" is exactly "the click while the counter still reads what it read at + * the dismissal", no matter how long the main thread was blocked in between. + */ + +let gesture = 0; +let isListening = false; + +function advance() { + gesture += 1; +} + +function listen() { + if (isListening || typeof document === 'undefined') { + return; + } + isListening = true; + // Capture phase: the count must advance before any handler reads it. + document.addEventListener('pointerdown', advance, true); + document.addEventListener('keydown', advance, true); +} + +/** + * Identifies the user gesture in flight. Two reads returning the same value + * happened within one press (or one keystroke). + */ +export function currentGesture(): number { + listen(); + return gesture; +} diff --git a/packages/core/src/Layer/useLayer.test.tsx b/packages/core/src/Layer/useLayer.test.tsx index f44e9b1935401..6f5d478497550 100644 --- a/packages/core/src/Layer/useLayer.test.tsx +++ b/packages/core/src/Layer/useLayer.test.tsx @@ -873,3 +873,226 @@ describe('useLayer context positioning', () => { expect(style).not.toContain('position-anchor'); }); }); + +describe('keepOpenProps (controls on the trigger, #5004)', () => { + function ClearableTriggerHarness() { + const layer = useLayer({mode: 'context'}); + return ( + <> + + + {layer.render(Layer content, {placement: 'below'})} + + ); + } + + it('names the control an invoker for the duration of the press', async () => { + const user = userEvent.setup(); + const {container, getByRole} = render(); + const clear = getByRole('button', {name: 'Clear'}); + + await user.click(getByRole('button', {name: 'Trigger'})); + const popover = container.querySelector('[popover]') as HTMLElement; + + await user.pointer({keys: '[MouseLeft>]', target: clear}); + expect(clear.getAttribute('popovertarget')).toBe(popover.id); + + await user.pointer({keys: '[/MouseLeft]', target: clear}); + expect(clear).not.toHaveAttribute('popovertarget'); + }); + + it('takes the invoker off when the press ends in pointercancel', async () => { + const user = userEvent.setup(); + const {container, getByRole} = render(); + const clear = getByRole('button', {name: 'Clear'}); + + await user.click(getByRole('button', {name: 'Trigger'})); + const popover = container.querySelector('[popover]') as HTMLElement; + + await user.pointer({keys: '[MouseLeft>]', target: clear}); + expect(clear.getAttribute('popovertarget')).toBe(popover.id); + + // A touch the browser claims — for a scroll, for the platform's long-press + // menu — ends here, and no pointerup ever arrives. + await act(async () => { + document.dispatchEvent(new Event('pointercancel')); + await new Promise(resolve => { + window.setTimeout(resolve, 0); + }); + }); + + expect(clear).not.toHaveAttribute('popovertarget'); + }); + + it('leaves the control alone while the layer is closed', async () => { + const user = userEvent.setup(); + const {getByRole} = render(); + const clear = getByRole('button', {name: 'Clear'}); + + await user.click(clear); + + expect(clear).not.toHaveAttribute('popovertarget'); + }); + + it("cancels the invoker's own toggle so the press cannot close the layer", async () => { + const user = userEvent.setup(); + const {container, getByRole} = render(); + await user.click(getByRole('button', {name: 'Trigger'})); + const popover = container.querySelector('[popover]') as HTMLElement; + const clear = getByRole('button', {name: 'Clear'}); + + const click = new MouseEvent('click', {bubbles: true, cancelable: true}); + await act(async () => { + clear.setAttribute('popovertarget', popover.id); + clear.dispatchEvent(click); + }); + + expect(click.defaultPrevented).toBe(true); + expect(clear).not.toHaveAttribute('popovertarget'); + }); +}); + +describe('wasJustDismissed (light dismiss vs. the trigger click, #5004)', () => { + function GuardedTriggerHarness() { + const layer = useLayer({mode: 'context'}); + return ( + <> + + {layer.isOpen ? 'open' : 'closed'} + {layer.render(Layer content, {placement: 'below'})} + + ); + } + + /** + * The browser closing the layer itself: it hides the element and queues a + * `toggle` event, which is what reaches React — before the click, on the + * engines that lose the race. + */ + function lightDismiss(container: HTMLElement) { + const popover = container.querySelector('[popover]') as HTMLElement; + act(() => { + popover.dispatchEvent( + Object.assign(new Event('toggle'), { + oldState: 'open', + newState: 'closed', + }), + ); + }); + } + + it('absorbs the trigger click that follows a light dismiss', async () => { + const user = userEvent.setup(); + const {container, getByRole, getByTestId} = render( + , + ); + const trigger = getByRole('button', {name: 'Trigger'}); + + await user.click(trigger); + expect(getByTestId('state')).toHaveTextContent('open'); + + // Dismissal and click within one press: no pointerdown in between. + lightDismiss(container); + fireEvent.click(trigger); + + expect(getByTestId('state')).toHaveTextContent('closed'); + }); + + it('acts on a deliberate second press', async () => { + const user = userEvent.setup(); + const {container, getByRole, getByTestId} = render( + , + ); + const trigger = getByRole('button', {name: 'Trigger'}); + + await user.click(trigger); + lightDismiss(container); + // A press of its own — a new gesture, however soon it lands. + await user.click(trigger); + + expect(getByTestId('state')).toHaveTextContent('open'); + }); + + it('leaves a programmatic hide unguarded', async () => { + const user = userEvent.setup(); + const {getByRole, getByTestId} = render(); + const trigger = getByRole('button', {name: 'Trigger'}); + + // Three presses with no browser-initiated close between them. + await user.click(trigger); + await user.click(trigger); + await user.click(trigger); + + expect(getByTestId('state')).toHaveTextContent('open'); + }); + + it('acts on a synthesized click with no press of its own', async () => { + const user = userEvent.setup(); + const {container, getByRole, getByTestId} = render( + , + ); + const trigger = getByRole('button', {name: 'Trigger'}); + + await user.click(trigger); + lightDismiss(container); + fireEvent.click(trigger); + expect(getByTestId('state')).toHaveTextContent('closed'); + + // AT activation reaches the trigger as a bare click: no pointerdown, so + // the gesture counter still reads what it read at the dismissal. + act(() => { + trigger.click(); + }); + + expect(getByTestId('state')).toHaveTextContent('open'); + }); + + /** A trigger that calls show()/hide() directly, checking nothing. */ + function PlainTriggerHarness() { + const layer = useLayer({mode: 'context'}); + return ( + <> + + {layer.isOpen ? 'open' : 'closed'} + {layer.render(Layer content, {placement: 'below'})} + + ); + } + + it('absorbs the click for a trigger that never calls wasJustDismissed', async () => { + const user = userEvent.setup(); + const {container, getByRole, getByTestId} = render(); + const trigger = getByRole('button', {name: 'Trigger'}); + + await user.click(trigger); + expect(getByTestId('state')).toHaveTextContent('open'); + + lightDismiss(container); + fireEvent.click(trigger); + + expect(getByTestId('state')).toHaveTextContent('closed'); + }); +}); diff --git a/packages/core/src/Layer/useLayer.tsx b/packages/core/src/Layer/useLayer.tsx index 20b50aa82ec95..b94aa2a2a895a 100644 --- a/packages/core/src/Layer/useLayer.tsx +++ b/packages/core/src/Layer/useLayer.tsx @@ -18,6 +18,7 @@ import React, { useCallback, useEffect, useId, + useMemo, useRef, useState, type ReactNode, @@ -27,6 +28,7 @@ import * as stylex from '@stylexjs/stylex'; import type {StyleXStyles} from '@stylexjs/stylex'; import {createPortal} from 'react-dom'; import {addAnchorName, removeAnchorName} from './anchorName'; +import {currentGesture} from './gestureCounter'; import {resolveLayerPortalTarget} from './layerHost'; import {typeScaleVars, typographyVars} from '../theme/tokens.stylex'; import {overlayPaddingReset} from '../Layout/padding.stylex'; @@ -72,6 +74,18 @@ const styles = stylex.create({ }), }); +/** + * Props for a control that sits on the trigger but must not dismiss the layer. + */ +export interface KeepLayerOpenProps { + onPointerDown: React.PointerEventHandler; + /** + * Capture phase, so spreading these props never collides with the control's + * own `onClick`. + */ + onClickCapture: React.MouseEventHandler; +} + /** * Position placement relative to anchor. * Logical: start/end resolve against the popover's own inherited direction @@ -278,6 +292,31 @@ export interface ContextLayerReturn { */ isOpen: boolean; + /** + * Props for a control that lives on the trigger — a clear button, a status + * button — and must not dismiss this layer when pressed. + * + * Such a control sits OUTSIDE the popover, so the browser light-dismisses + * the layer as soon as it is pressed and the affordance is unusable while + * the layer it belongs to is open. Spreading these props names the control + * an invoker of this popover, which puts it inside the layer for that + * decision. Merge them with the control's own handlers. + */ + keepOpenProps: KeepLayerOpenProps; + + /** + * Whether the browser itself closed this layer — light dismiss, or popover + * stack eviction — during the gesture still in flight, rather than a call + * to `hide()`. + * + * A trigger checks this before acting on a click: a click from that same + * press is the tail of the dismissal, and toggling on it would reopen what + * the user just closed. The browser dismisses on pointerup and the click + * follows a beat later, so which one React sees first is a race that varies + * by engine and by load — this does not depend on winning it. + */ + wasJustDismissed: () => boolean; + /** * Unique ID for aria-describedby */ @@ -314,6 +353,28 @@ export interface FixedLayerReturn { */ isOpen: boolean; + /** + * Props for a control that lives on the trigger — a clear button, a status + * button — and must not dismiss this layer when pressed. + * + * Such a control sits OUTSIDE the popover, so the browser light-dismisses + * the layer as soon as it is pressed and the affordance is unusable while + * the layer it belongs to is open. Spreading these props names the control + * an invoker of this popover, which puts it inside the layer for that + * decision. Merge them with the control's own handlers. + */ + keepOpenProps: KeepLayerOpenProps; + + /** + * Whether the browser itself just closed this layer — light dismiss or + * popover stack eviction — rather than a call to `hide()`. + * + * A trigger checks this before acting on a click: within the guard window + * the click belongs to the gesture that dismissed the layer, so toggling on + * it would reopen what the user just closed. + */ + wasJustDismissed: () => boolean; + /** * Unique ID for aria-describedby */ @@ -485,6 +546,19 @@ export function useLayer( // stale-closure reads of the previous isOpen value. const isOpenRef = useRef(false); + // The gesture during which the browser last closed this layer on its own. + // Read through wasJustDismissed by triggers deciding whether a click is + // theirs to act on. + const dismissedByGestureRef = useRef(null); + const forgetDismissalRef = useRef<(() => void) | null>(null); + + const wasJustDismissed = useCallback( + () => + dismissedByGestureRef.current !== null && + dismissedByGestureRef.current === currentGesture(), + [], + ); + const showPopoverElement = useCallback((popover: HTMLElement) => { // Finding infra-4: the Popover API is unsupported on Safari <17 and // Firefox <125. On those browsers `showPopover` does not exist, so fall @@ -548,6 +622,11 @@ export function useLayer( }, [mode, lazyMount]); const show = useCallback(() => { + // Every caller lands here, so this is where the dismissing press is + // absorbed: opening now would reopen the popup that same press closed. + if (wasJustDismissed()) { + return; + } // A context popover left over until React commits a previous hide must not // be reopened. The synchronous mount ref is the source of truth. const candidate = popoverRef.current; @@ -569,12 +648,18 @@ export function useLayer( requestContextMount, showPopoverElement, isCurrentContextPopover, + wasJustDismissed, ]); const hide = useCallback(() => { pendingShowRef.current = false; if (isOpenRef.current) { const el = popoverRef.current; + // Clear the open state BEFORE hiding: hidePopover fires `toggle`, and + // the reconciler below reads this ref to tell the browser's own + // dismissals from ours. + openedPopoverRef.current = null; + isOpenRef.current = false; // See finding infra-4 note in `show`: mirror the same guard on hide so // unsupported browsers degrade gracefully instead of throwing. if (el) { @@ -584,14 +669,54 @@ export function useLayer( el.style.display = 'none'; } } - openedPopoverRef.current = null; - isOpenRef.current = false; setIsOpen(false); onHide?.(); } clearContextMount(); }, [onHide, clearContextMount]); + const keepOpenProps: KeepLayerOpenProps = useMemo( + () => ({ + onPointerDown: (event: React.PointerEvent) => { + if (!isOpenRef.current) { + return; + } + // The browser reads the invoker relationship twice — once when the + // press starts and once when it ends — and only then decides to + // light-dismiss, so the attribute has to span the whole gesture. It + // comes off afterwards because a permanent invoker reports itself as + // `expanded` to assistive tech, which a clear button is not. + const control = event.currentTarget; + control.setAttribute('popovertarget', id); + // A press has two ends: `pointercancel` fires instead of `pointerup` + // when the browser claims the gesture — a touch turned into a scroll, + // a long press opening the platform menu. Listening for `pointerup` + // alone leaves the attribute on, and a permanent invoker reports + // itself `expanded` to assistive tech. + const onPressEnd = () => { + document.removeEventListener('pointerup', onPressEnd); + document.removeEventListener('pointercancel', onPressEnd); + // A task, not a microtask: the dismissal runs as the pointerup + // default action, after this listener. + window.setTimeout(() => { + control.removeAttribute('popovertarget'); + }, 0); + }; + document.addEventListener('pointerup', onPressEnd); + document.addEventListener('pointercancel', onPressEnd); + }, + onClickCapture: (event: React.MouseEvent) => { + const control = event.currentTarget; + if (control.hasAttribute('popovertarget')) { + // Being an invoker would otherwise toggle the layer shut. + event.preventDefault(); + control.removeAttribute('popovertarget'); + } + }, + }), + [id], + ); + // Ref for trigger element (context mode only) const ref: RefCallback | undefined = mode === 'context' @@ -610,6 +735,24 @@ export function useLayer( } : undefined; + // A dismissal is spent by the click that ends the press it came from. Left + // standing it would also swallow a click that arrives with no press of its + // own — AT activation, element.click() — which never advances the counter. + // Bubble phase on the document: every guard reading the dismissal has run. + const rememberDismissal = useCallback((doc: Document) => { + dismissedByGestureRef.current = currentGesture(); + forgetDismissalRef.current?.(); + const forget = () => { + dismissedByGestureRef.current = null; + doc.removeEventListener('click', forget); + forgetDismissalRef.current = null; + }; + doc.addEventListener('click', forget); + forgetDismissalRef.current = forget; + }, []); + + useEffect(() => () => forgetDismissalRef.current?.(), []); + // Reconcile browser-initiated closes (light-dismiss, popover="auto" stack // eviction). These are the only cases where the DOM mutates without going // through our show/hide — we sync React state back to match. @@ -626,12 +769,15 @@ export function useLayer( if (toggleEvent.newState === 'closed' && isOpenRef.current) { openedPopoverRef.current = null; isOpenRef.current = false; + rememberDismissal( + (e.currentTarget as HTMLElement | null)?.ownerDocument ?? document, + ); setIsOpen(false); onHide?.(); clearContextMount(); } }, - [onHide, clearContextMount], + [onHide, clearContextMount, rememberDismissal], ); // Ref callback for popover element — sets up the `toggle` listener. @@ -872,6 +1018,8 @@ export function useLayer( show, hide, isOpen, + keepOpenProps, + wasJustDismissed, id, render: renderContext, }; @@ -882,6 +1030,8 @@ export function useLayer( show, hide, isOpen, + keepOpenProps, + wasJustDismissed, id, render: renderFixed, }; diff --git a/packages/core/src/MultiSelector/MultiSelector.test.tsx b/packages/core/src/MultiSelector/MultiSelector.test.tsx index 1718dc64811c0..23cf18e69e90e 100644 --- a/packages/core/src/MultiSelector/MultiSelector.test.tsx +++ b/packages/core/src/MultiSelector/MultiSelector.test.tsx @@ -2660,4 +2660,37 @@ describe('MultiSelector popup theme target', () => { expect(popup).not.toBe(layer); expect(layer.contains(popup)).toBe(true); }); + + it('stays closed when the trigger click follows its own light dismiss (#5004)', async () => { + const user = userEvent.setup(); + render( + {}} + />, + ); + const trigger = screen.getByRole('combobox'); + await user.click(trigger); + expect(trigger).toHaveAttribute('aria-expanded', 'true'); + + // The browser dismissed the popup on pointerup and queued the toggle. When + // that event lands before the click — WebKit, or any engine under load — + // the click used to read a closed popup and reopen it. + const popover = document.querySelector('[popover]') as HTMLElement; + act(() => { + popover.dispatchEvent( + Object.assign(new Event('toggle'), { + oldState: 'open', + newState: 'closed', + }), + ); + }); + // Synchronously: the click falls inside the one gesture the guard covers, + // as it does in a browser a few milliseconds behind the dismissal. + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + }); }); diff --git a/packages/core/src/MultiSelector/MultiSelector.tsx b/packages/core/src/MultiSelector/MultiSelector.tsx index 579c4d4377dfd..666eed4c82c60 100644 --- a/packages/core/src/MultiSelector/MultiSelector.tsx +++ b/packages/core/src/MultiSelector/MultiSelector.tsx @@ -1162,6 +1162,7 @@ export function MultiSelector({ onKeyDown, onItemMouseEnter, } = useMultiCombobox({ + wasJustDismissed: popover.wasJustDismissed, selectableItems: sortedItems, isDisabled, isOpen: popover.isOpen, @@ -1705,6 +1706,7 @@ export function MultiSelector({ {isBusy && } {hasClear && value.length > 0 && !isDisabled && ( ({ type="button" aria-label={t(STATUS_BUTTON_LABEL_KEY[status.type])} aria-describedby={statusTooltip.describedBy} + {...popover.keepOpenProps} onClick={e => e.stopPropagation()} {...stylex.props( focusOutlineStyles.focusVisible, diff --git a/packages/core/src/MultiSelector/hooks.ts b/packages/core/src/MultiSelector/hooks.ts index 64977c5f1f5e6..cd0644b2f993f 100644 --- a/packages/core/src/MultiSelector/hooks.ts +++ b/packages/core/src/MultiSelector/hooks.ts @@ -34,6 +34,12 @@ interface UseMultiComboboxOptions { * The Delete/Backspace clear path is skipped when false. */ hasValue?: boolean; + /** + * Whether the browser's light dismiss just closed the popup. The trigger + * click that follows belongs to that same press, so acting on it would + * reopen the popup the user just closed. + */ + wasJustDismissed?: () => boolean; listboxId: string; } @@ -62,6 +68,7 @@ export function useMultiCombobox({ onToggle, onClear, hasValue = false, + wasJustDismissed, listboxId, }: UseMultiComboboxOptions): UseMultiComboboxResult { const [highlightedIndex, setHighlightedIndex] = useState(-1); @@ -87,7 +94,7 @@ export function useMultiCombobox({ }, [onClose]); const onTriggerClick = useCallback(() => { - if (isDisabled) { + if (isDisabled || wasJustDismissed?.()) { return; } if (isOpen) { @@ -98,7 +105,7 @@ export function useMultiCombobox({ setHighlightedIndex(0); } } - }, [isDisabled, isOpen, onOpen, closeAndReset, hasSearch]); + }, [isDisabled, wasJustDismissed, isOpen, onOpen, closeAndReset, hasSearch]); const onItemMouseEnter = useCallback( (item: MultiSelectorOptionData, index: number) => { diff --git a/packages/core/src/Popover/Popover.tsx b/packages/core/src/Popover/Popover.tsx index e676dd23041db..ba082561fd77a 100644 --- a/packages/core/src/Popover/Popover.tsx +++ b/packages/core/src/Popover/Popover.tsx @@ -322,16 +322,12 @@ export function Popover({ }: PopoverProps): ReactElement { const wrapperRef = useRef(null); const isControlled = isOpen !== undefined; - // Track when the popover was last hidden by light dismiss to prevent - // the trigger click from immediately re-opening it. - const lastHideTimeRef = useRef(0); const handlePopoverShow = useCallback(() => { onOpenChange?.(true); }, [onOpenChange]); const handlePopoverHide = useCallback(() => { - lastHideTimeRef.current = Date.now(); onOpenChange?.(false); }, [onOpenChange]); @@ -353,11 +349,7 @@ export function Popover({ if (!isEnabled) { return; } - // If the popover was just closed by light dismiss (clicking outside), - // the trigger click fires in the same event — skip re-opening. - if (Date.now() - lastHideTimeRef.current < 50) { - return; - } + // `toggle` absorbs a click that belongs to its own light dismiss. popover.toggle(); }, [isEnabled, popover]); diff --git a/packages/core/src/Popover/usePopover.tsx b/packages/core/src/Popover/usePopover.tsx index 00ce13d9b0ccf..a9d08d7189a95 100644 --- a/packages/core/src/Popover/usePopover.tsx +++ b/packages/core/src/Popover/usePopover.tsx @@ -17,7 +17,11 @@ import React, {useCallback, useEffect, useRef, type ReactNode} from 'react'; import * as stylex from '@stylexjs/stylex'; -import {useLayer, type ContextRenderProps} from '../Layer/useLayer'; +import { + useLayer, + type ContextRenderProps, + type KeepLayerOpenProps, +} from '../Layer/useLayer'; import {useFocusTrap} from '../hooks/useFocusTrap'; import {LayerDepthProvider} from '../Layer/LayerDepthContext'; import type {StyleXStyles} from '@stylexjs/stylex'; @@ -249,6 +253,22 @@ export interface UsePopoverReturn { */ toggle: () => void; + /** + * Props for a control that sits on the trigger — a clear button, a status + * button — and must not dismiss this popover when pressed. Merge them with + * the control's own handlers. + */ + keepOpenProps: KeepLayerOpenProps; + + /** + * Whether the browser's own light dismiss just closed this popover. + * + * Triggers that do not route through `toggle` — a combobox that also seeds a + * highlight, say — check this first and do nothing when it is true: the click + * is the tail of the gesture that already closed the popup. + */ + wasJustDismissed: () => boolean; + /** * Whether the popover is currently open */ @@ -400,6 +420,9 @@ export function usePopover(options: UsePopoverOptions = {}): UsePopoverReturn { // Toggle function const toggle = useCallback(() => { + if (layer.wasJustDismissed()) { + return; + } if (layer.isOpen) { layer.hide(); } else { @@ -491,6 +514,8 @@ export function usePopover(options: UsePopoverOptions = {}): UsePopoverReturn { show, hide: layer.hide, toggle, + keepOpenProps: layer.keepOpenProps, + wasJustDismissed: layer.wasJustDismissed, isOpen: layer.isOpen, id: layer.id, render, diff --git a/packages/core/src/Selector/Selector.test.tsx b/packages/core/src/Selector/Selector.test.tsx index 1e09eea49ff3d..2e60813a60725 100644 --- a/packages/core/src/Selector/Selector.test.tsx +++ b/packages/core/src/Selector/Selector.test.tsx @@ -3256,6 +3256,40 @@ describe('Selector popup theme target', () => { expect(popup?.querySelector('[role="listbox"]')).not.toBeNull(); }, ); + + it('stays closed when the trigger click follows its own light dismiss (#5004)', async () => { + const user = userEvent.setup(); + render( + {}} + placement="below" + />, + ); + const trigger = screen.getByRole('combobox'); + await user.click(trigger); + expect(trigger).toHaveAttribute('aria-expanded', 'true'); + + // The browser dismissed the popup on pointerup and queued the toggle. When + // that event lands before the click — WebKit, or any engine under load — + // the click used to read a closed popup and reopen it. + const popover = document.querySelector('[popover]') as HTMLElement; + act(() => { + popover.dispatchEvent( + Object.assign(new Event('toggle'), { + oldState: 'open', + newState: 'closed', + }), + ); + }); + // Synchronously: the click falls inside the one gesture the guard covers, + // as it does in a browser a few milliseconds behind the dismissal. + fireEvent.click(trigger); + + expect(trigger).toHaveAttribute('aria-expanded', 'false'); + }); }); describe('Selector option-row theme target', () => { diff --git a/packages/core/src/Selector/Selector.tsx b/packages/core/src/Selector/Selector.tsx index c60813d0cdebb..bcf14f3e1fa60 100644 --- a/packages/core/src/Selector/Selector.tsx +++ b/packages/core/src/Selector/Selector.tsx @@ -1118,6 +1118,7 @@ export function Selector( onItemMouseEnter, } = useCombobox({ selectableItems: filteredItems, + wasJustDismissed: popover.wasJustDismissed, // The optimistic value, not the raw prop: with a pending changeAction the // prop still holds the old selection, so the popup would open with the // highlight on it and Delete/Backspace could clear a value the action has @@ -1649,6 +1650,7 @@ export function Selector( {isBusy && } {hasClear && value != null && !isDisabled && ( ( type="button" aria-label={t(STATUS_BUTTON_LABEL_KEY[status.type])} aria-describedby={statusTooltip.describedBy} + {...popover.keepOpenProps} onClick={e => e.stopPropagation()} {...stylex.props( focusOutlineStyles.focusVisible, diff --git a/packages/core/src/Selector/hooks.ts b/packages/core/src/Selector/hooks.ts index f5f467c489a85..648ff1e0c38d4 100644 --- a/packages/core/src/Selector/hooks.ts +++ b/packages/core/src/Selector/hooks.ts @@ -176,6 +176,12 @@ interface UseComboboxOptions { * lands in the search input, which then owns its own typing. */ onSearchSeed?: (char: string) => void; + /** + * Whether the browser's light dismiss just closed the popup. The trigger + * click that follows belongs to that same press, so acting on it would + * reopen the popup the user just closed. + */ + wasJustDismissed?: () => boolean; listboxId: string; } @@ -208,6 +214,7 @@ export function useCombobox({ onSelect, onClear, onSearchSeed, + wasJustDismissed, listboxId, }: UseComboboxOptions): UseComboboxResult { const [highlightedIndex, setHighlightedIndex] = useState(-1); @@ -244,7 +251,7 @@ export function useCombobox({ ); const onTriggerClick = useCallback(() => { - if (isDisabled) { + if (isDisabled || wasJustDismissed?.()) { return; } if (isOpen) { @@ -256,7 +263,15 @@ export function useCombobox({ setHighlightedIndex(selectedIndex >= 0 ? selectedIndex : 0); } } - }, [isDisabled, isOpen, onOpen, closeAndReset, findSelectedIndex, hasSearch]); + }, [ + isDisabled, + wasJustDismissed, + isOpen, + onOpen, + closeAndReset, + findSelectedIndex, + hasSearch, + ]); const onItemMouseEnter = useCallback( (item: SelectorOptionData, index: number) => {