Review 5018

#5018 — fix(core): stop popup triggers from fighting their own light dismiss (#5004)

cixzhang · open · view on GitHub

Verdict: request changes · 3 rounds

These records predate the Review Presentation format for the most part, so the problem/solution/API/theme-target/breaking slots are only filled where the review itself carried them. Reviews from 2026-08-23 on use the full template.

Round 1 — comment · 2026-08-16

Thanks — the gesture counter is the right primitive, and having one place that knows a dismissal just happened is a good direction.

Needs a merge with main; #4769 landed today and this will conflict.

Three things:

The guard only reaches callers that go through toggle(). Anything calling show()/hide() directly bypasses it — ComplexSelector is the example after the merge, and its new test passes off its own leftover 50ms timer rather than this change. Guarding in show() would make it hold for every caller instead of being something each one has to opt into.

dismissedByGestureRef is never cleared (useLayer.tsx:485), so a click with no pointerdown before it — AT activation, .click() — is read as part of the dismissing gesture indefinitely. Make it one-shot.

keepOpenProps isn't finished. KeepLayerOpenProps is unexported, InputClearButton grew two ad-hoc handler props, and five clearable inputs are unwired — let's keep it internal until Typeahead and the date/time inputs are done rather than ship a half-built API.

[Reviewed by Robohands and the maintainer]

Round 2 — comment · 2026-08-22

Merged main and rewrote the guard against your three points. Pushed to this branch (4947e62).

The guard moved into show(). Every caller lands there, so a trigger no longer opts in — wasJustDismissed() is checked before the layer opens, whatever route the caller took. ComplexSelector's private 50 ms timer is deleted with it. Its two tests now fail when I remove the show() guard and pass with it, so they are testing this change and not the timer.

The dismissal is one-shot. It is spent by the click that ends the press it came from — a one-shot bubble-phase click listener on the document, so every guard reading it has already run. A click with no press of its own (AT activation, element.click()) therefore acts. New test: acts on a synthesized click with no press of its own, red without the clear.

One more defect the guard exposed. hide() called hidePopover() while isOpenRef still read true, so a toggle event delivered synchronously was read as a browser dismissal — a programmatic hide stamped the gesture. The ref is cleared before the call now, which is what the comment above the reconciler already claimed. DropdownMenu's old race test was passing off exactly that bug (three clicks, no pointerdown, no light dismiss); it is rewritten to drive the losing order, plus a second test that a press of its own re-opens.

Real Chromium

Playwright, trusted presses, pnpm storybook. Chromium delivers click before toggle on both mouse and touch (pointerdown → click → toggle:closed), so it wins the race on its own — as you found. To test the losing order I hand React the dismissal first, from a real press: a bubble-phase pointerdown listener dispatches toggle: closed on the open popover, then the press's own click arrives. Before/after is the same browser, same driver, main's files swapped in for "before":

trigger press while open	before	after
DropdownMenu	closes (its own 50 ms timer)	closes
Popover	re-opens	closes
Selector	re-opens	closes
MultiSelector	re-opens	closes
ComplexSelector	closes (its own 50 ms timer)	closes

Also in Chromium, with the fix: third press re-opens everywhere; a synthesized .click() after a dismissing press opens the menu; an outside press dismisses; MultiSelector's clear ✕ clears the value with the menu still open.

Tests

Layer, Popover, DropdownMenu, Selector, MultiSelector, ComplexSelector, Field, DateInput, Typeahead, Tooltip, HoverCard, Dialog, CommandPalette — 1011 passing. Full packages/core is green apart from DateInputTouch timeouts under parallel load, which reproduce identically on the merge commit before this change (they pass in isolation, 133/133). Typecheck clean, lint 0 errors.

Against the shared dismissal stack

Merging this branch into #4881 auto-merges usePopover.tsx cleanly — the one file both touch. That merge does conflict in Layer/index.ts, Tooltip/useTooltip.tsx and useHoverCard.tsx, but those conflicts are #4881 against today's main and appear with this branch left out. Nothing here is subsumed or duplicated either: #4881 routes Escape and the close-watcher through a stack of layers, and pointer light dismiss stays the browser's, so the toggle this guard counts on still fires. Order can be whichever suits you.

Not done

keepOpenProps is untouched — your third point (unexported type, two ad-hoc props on InputClearButton, five unwired inputs) is an API-scope call, and I did not want to make it on your behalf inside a correctness fix.

[Pushed by Robohands]

Round 3 — request changes · 2026-08-24 (gate on our own PR)

Full artifact: {review-root}/review-artifacts/pr-5018/gate-r1.md on the M5. Head reviewed 4947e624, re-verified unmoved immediately before posting.

slot
problem	the button of an open menu closes it and it comes straight back — the dismissal and the click are one press, and which half React sees first is a race. Second, stated problem: the clear ✕ and status ⚠ sit on the trigger but outside the popover, so pressing one dismissed the popup
solution	2 decisions: (1) count gestures and absorb the dismissing press inside show(), deleting three private 50 ms timers, with the one-shot clear and the hide()-before-hidePopover() correction riding along; (2) keepOpenProps names a control on the trigger an invoker for the press
API	keepOpenProps + wasJustDismissed on ContextLayerReturn, FixedLayerReturn and UsePopoverReturn — all public through the core barrel; KeepLayerOpenProps exported from the module but not from Layer/index.ts or the barrel; two ad-hoc handler props on InputClearButton; neither new member in useLayer.doc.mjs or usePopover.doc.mjs
theme targets	none — structural diff, greps for colour/style and for aria/role/strings both zero in runtime source
ossification	no other hook in core hands out a "do not dismiss me" props bag, so this is a new concept, and two of seven clearable inputs are wired. [Needs human judgement]
breaking	API no · visual no · theme no. Behaviour: one change I could not reach from a shipped configuration — a controlled Popover whose consumer holds isOpen true through a light dismiss now stays closed until the next render (Popover.tsx:489-497 against the new show() guard). Recorded, not filed
evidence	real Chromium, both sides in one session, warm main b0a09a630f5 on :6100 vs the head on storybook dev :6337. The re-open reproduces on main and is gone here; clear ✕ and status ⚠ keep the menu open where main closes it; frames published to assets/pr-5018 on the fork. Probes banked: probe-5018-trigger-clear.cjs, probe-5018-hit-ownership.cjs, probe-5018-below-placement.cjs. WebKit could not be driven — it will not open a page on this host
R31c	main moved: #4881 landed after the merge-base. It leaves useLayer.tsx alone and owns Escape only, so the fix survives — verified by trial merge, 550 green tests on the merged tree, and re-running the Chromium check there

The block. keepOpenProps.onPointerDown stamps popovertarget and removes it from a pointerup listener only (useLayer.tsx:691). A press that ends in pointercancel — a touch the browser takes over for a scroll, a long-press — leaves it, and Chrome's AX tree then reports the clear button expanded=true: the exact state the PR body rules out. All four other press cleanups in packages/core pair pointerup with pointercancel; this is the fifth and does not. grep -c pointercancel over the whole diff is 0.

Held for the maintainer. Whether keepOpenProps ships as public surface now or stays internal until the other five clearable inputs are wired — our own ask from round 1, which the author deliberately declined to answer inside a correctness fix. The loop's recommendation: keep it internal, and land the guard half alone.

As posted

https://github.com/facebook/astryx/pull/5018#issuecomment-5404962367

Thanks — the two you took from last round hold up. In Chromium, with the dismissal handed to React before the click, main re-opens the menu and this closes it:

	before	after
press the trigger of an open menu, dismissal first
press the clear ✕ with the menu open
press the status ⚠ with the menu open

One to fix before it lands: after a touch the browser cancels — a press turned into a scroll, a long-press — Chrome's a11y tree has the clear ✕ as an expanded pop-up button, the state your note rules out. keepOpenProps takes the popovertarget off on pointerup only (useLayer.tsx:691), and a press has two ends. Listen for pointercancel too; the four other press cleanups in core do (ResizeHandle.tsx:439, useSheetGestures.ts).

keepOpenProps is still your call from last round; I'd keep it internal until the date/time inputs are wired. Send the guard on its own once pointercancel is covered and it lands today — does the invoker half want to wait for them?

[Reviewed by Robohands]

What changed before posting

Nothing — posted as the second draft stood. The first draft was cut by the loop's own critic for two claims wider than its evidence (it credited itself with the author's Popover/Selector rows and with an AT-activation check it had not driven) and for 167 words against a 150-word cap; deleting the two unearned claims fixed both.