Review 4881

#4881 — feat(core): one shared dismissal stack for every overlay layer

cixzhang · open · view on GitHub

Verdict: comment

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.

The review · 2026-08-16

Thanks for taking this on — one Escape owner for the whole app is the right direction, and the depth-aware stack is a real improvement over every layer racing the same keypress.

The thing I'd want to sort out first: we preventDefault every Escape we handle (layerStack.ts:224), but only five families are registered. Lightbox and MobileNav close via the native cancel event only, so once a required Dialog is on the stack, Escape stops reaching them. Same shape at Dialog.tsx:533 — cancel also fires for the Android back gesture, and that path no longer closes anything. Could the stack fall through when nothing registered claims the press?

hasActiveFocusTrapEscape went public in 0.4.2, and the shim changes what it means from "a trap is above me" to "any layer is present" — BottomSheetSwitcher.tsx:581 gates dismissal on it, so the sheet stops closing whenever a tooltip is up. Ideally I'd like to keep the shim trap-only and migrate the sheet properly in a follow-up.

Sizing: this is a lot at once. The stack + Dialog + useFocusTrap would carry Popover, DropdownMenu, PowerSearch and Typeahead on their own — Tooltip and HoverCard bring the presence-at-press-time machinery and the WCAG argument, which feels like its own PR.

One sequencing note: #5018 is smaller, non-draft, and on a different channel (pointer, not keydown) — no conflict between the two heads. Worth landing that first and rebasing this on top. What do you think?

[Reviewed by Robohands and the maintainer]

Round 2 · 2026-08-23 — first full loop pass, and the fixes

Head reviewed: b8b78f0132238b01c65c50d79c9c503233ec3fdf (main merged in, plus two review commits). Read-only was suspended for this run: the brief asked the loop to fix what it found and re-review.

What the measurement settled. The IME half had been measured; the dismissal stack itself never had. Thirteen scenarios in Chromium 149, two-sided — main in the same worktree with packages/core/src reverted, because the stories only exist on the branch, so warm main on 6100 cannot be the baseline. Six rows improve, none regress: modal-in-modal (both closed on main), hover tip in a modal (both), a required dialog inside a host modal (the host closed, the required one stayed), Lightbox and MobileNav over a required dialog (keyboard-unclosable on main), and requestClose() on a layer that is not on top (closed the wrong one).

What the loop changed.

• Cut UseLayerDismissalReturn.isTopmost and useLayerDepth — both public from @astryxdesign/core/Layer with no caller anywhere. isTopmost's own doc said it was "for dismissal channels it does not model yet", which is the confession R1g-surface names.
• Added Layer/layerDismissalInvariants.test.tsx — 15 tests asserting which layers are still on screen after one press, not which handler ran. Covers what nothing covered: three deep, a required Dialog that must not block a layer above it, teardown leaving the stack clean, and presence asked at press time.
• Added ThreeDeep and PurposeFlipsWhileOpen stories, and made SheetWithHoverTip say that the standalone sheet is off the stack and still takes two layers on one press.

The test-quality finding worth keeping. useLayerDismissal.test.tsx's synthetic Layer helper supplies both a LayerDepthProvider and a getContainer, so its nesting tests pass on DOM containment alone — deleting the depth key from compareEntries leaves every one of them green. Only tests that mount real overlays (which portal out of containment) can see a depth regression. Eight deliberate breakages, each caught: review-artifacts/pr4881/break-matrix.log.

Left open, for her: the sizing question she raised on 16 August — do Tooltip and HoverCard stay in? They are the only two needing the presence-at-press-time machinery. Everything verifiable is green; that is the only thing left.

Not verified: second engine. WebKit exits immediately and Firefox Nightly is SIGKILLed on this Mac, so the author's Chromium-only gap stands.