[Bug Fix] Overlays: play the exit animation before hiding

[tvq]

Related issue

No existing issue — happy to open one if you'd prefer to track it separately.

Description

Every overlay that ships data-[state=closed]:animate-out cut it: each controller set the closed state and applied hidden (display: none) — or called element.remove() — in the same frame, so the enter animation ran and the exit was a hard cut. Three of them never set data-state at all.

Component	Before
Popover, HoverCard, ContextMenu	hidden in the same tick as data-state="closed"
DropdownMenu, ClipboardPopover	hidden on the wrapper; data-state never set
Select	hidden on the wrapper; exit keyed on the root's open value
Sheet, CommandDialog	element.remove() outright
Tooltip	already correct — the pattern this follows

The fix defers the hide to the end of the exit animation with one block that is byte-identical across the eight controllers, plus a one-line afterExit() per controller (hidden on the wrapper, or element.remove()):

• data-state="closed" first, hide on animationend — the classes get their frames.
• animationcancel too — reopening mid-exit must not strand the pending hide.
• Two guards on the event — animationend bubbles, so an animated child must not hide its container; and closing during the opening animation cancels enter, so only the captured exit run settles it. Both mirror @radix-ui/react-presence.
• No timeout fallback — the element's own animations are read up front with getAnimations(). If there is nothing to wait for (no tw-animate-css, overridden classes, a consumer's reduced-motion rule, or no box to run in) it hides at once instead of hanging on an event that never fires.
• settleExit on disconnect — every other listener in these controllers is torn down there.
• data-[state=closed]:fill-mode-forwards on every affected content component — without it the element repaints at full opacity between the last keyframe and the hide, which flashes. TooltipContent already carries it.
• Explicit panel target (and backdrop for Sheet/CommandDialog) for the animated element where it differs from the hidden wrapper.

Behaviour notes: DropdownMenu#toggle now reads openValue (hidden no longer distinguishes the states) and releases its z-index on settle; CommandDialog reopened while dismissing brings the same instance back rather than stacking a second one; Sheet reopened mid-exit inserts a new instance while the old one finishes, as it does today on a double click. Clipboard's empty-source branch called a method that did not exist (showErrorPopover) — fixed while restructuring it.

Kept as copies rather than a shared module: the gem ships no shared JS today, and one import would have to resolve under both importmap and bundlers — see the thread on the duplication comment.

Testing instructions

cd gem && bundle exec rake covers the rendered fill-mode-forwards class for all eight. The lifecycle is JS — cd docs && bin/dev, then for each of /docs/popover, /docs/hover_card, /docs/context_menu, /docs/dropdown_menu, /docs/clipboard, /docs/select, /docs/sheet, /docs/command:

1. Open, then close (outside click, Esc, item, close button). It should fade/slide out, not vanish.
2. Reopen mid-fade: it should come back without flicker or a stuck panel.
3. Close mid-open (trigger twice in quick succession): the exit should still play in full.

I also drove all eight through those three scenarios in headless Chrome against the built docs bundle — every one settled (hidden applied / element removed / same instance reopened).

Note on reduced motion: unchanged — the library ships no prefers-reduced-motion rules today, so animate-in/animate-out run regardless. Happy to open a separate issue for library-wide support.

[cubic-dev-ai]

All reported issues were addressed across 9 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 3 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[djalmaaraujo]

Este bloco (hideAfterExitAnimation + handleExitAnimationEnd + settleExit, ~30 linhas) foi copiado byte-a-byte para os três controllers (popover, hover_card, context_menu). Extraia para um helper/mixin compartilhado — hoje qualquer correção precisa ser replicada em 3 lugares (e dropdown/tooltip ficaram de fora justamente por isso).

[tvq]

Agreed it's copies, and deliberately so — see the "Kept as copies" paragraph in the description. As of 685f37c it is the same byte-identical block in all eight overlay controllers; the per-controller part is a one-line afterExit(). The gem ships no shared JS today: every controller imports only npm packages, and the generator copies *.js from a single component folder (component_generator.rb). A shared module needs one import that resolves under both importmap (bare specifier via pin_all_from) and bundlers (docs is esbuild, where that bare specifier doesn't resolve without consumer config), plus a new dependency kind for the generator. That's a distribution change, and I'd rather not smuggle it in under a bug fix.

Happy to do it as the follow-up if you want it — would you prefer the generator to grow a "shared JS" kind, or another resolution strategy? Once maintainers pick one, the eight copies collapse into one import.

[djalmaaraujo]

O settleExit novo (que remove os listeners de animationend/animationcancel) roda por último no disconnect, depois de this.removeEventListeners() na linha 31, que acessa this.triggerTarget/this.contentTarget sem guarda. Se o elemento já foi destacado, essa chamada lança, o Stimulus engole a exceção e o resto do disconnect — inclusive este settleExit — não executa, vazando os listeners. É exatamente a falha que o próprio popover_controller.js documenta ("Teardown that cannot fail comes first"). Mesma questão no context_menu (this.hide() antes do settleExit).

[tvq]

Good catch — fixed in ad5daf7. hover_card now follows the same rule as popover: disconnect() runs the teardown that cannot fail first (timers, the document listener, autoUpdate), and removeEventListeners() guards each target on its own, so settleExit is always reached. hide() got the same ordering — it runs from a timer, and a missing content target would have left the keydown listener and autoUpdate behind.

context_menu is a different shape: disconnect() goes through hide(), which arms the handlers, so settleExit has to run after it. What could fail there was hide() touching contentTarget before releasing the document listeners — reordered so those go first and the target is guarded. That access predates this PR, but it sits on the path this PR added, so it belongs here.

[djalmaaraujo]

Não há timeout de fallback: o hidden só é aplicado via animationend/animationcancel. Se nenhum dos dois disparar — animação pausada em aba em background, animate-out interrompido por uma troca de estilo que não gera animationcancel, ancestral em display:none — o hidden nunca entra e o elemento fica presente em opacity:0. No context_menu o content é pointer-events-auto, então ele passa a interceptar cliques de forma invisível. Um setTimeout de segurança (duração da animação + folga) chamando settleExit resolveria.

[tvq]

Partly agreed — there was one real gap, fixed in ad5daf7. Went through the three cases:

• Ancestor already display:none when hide() runs — real. getComputedStyle reports the element's own display (block) and still resolves animation-name: exit, so the guard armed a listener for an animation with no box to run in. The check now uses content.getAnimations(), which returns [] for an element without a box (and for animation: none, and for hidden), so all of those settle synchronously. Verified in Chrome on the same element: getComputedStyle → {animationName: "exit", display: "block"}, getAnimations() → [].
• Background tab — you're right that timers fire (throttled) while animation events don't, so a timeout would win that race. But the animation isn't lost: it sits at currentTime: 0 and completes on the first rendering opportunity, where animationend fires and hidden lands — checked that too. Nobody can click a tab they can't see, so I don't think that difference is worth a duration constant.
• Style swap without animationcancel — couldn't construct one. Removing the animation from the element (name change, display:none on it or an ancestor) fires animationcancel; detaching from the DOM fires nothing, but the listeners stay on the element and the animation restarts on reinsertion, so it still settles.

Left the timeout out: it's the piece @radix-ui/react-presence deliberately doesn't carry, the repo's own Stimulus guardrails say the same ("no fixed timeouts as a proxy for completion — drive UI off real signals", .claude/skills/ruby-ui-stimulus/SKILL.md), and I'd rather not guess a duration for classes a consumer may override. If you have a rendered-element case where neither event arrives, I'll add it.

[djalmaaraujo]

getComputedStyle(content) no caminho de hide() força um recálculo de estilo síncrono a cada fechamento (hot path de interação). Menor: dá pra evitar lendo só quando necessário ou cacheando o nome da animação de saída.

[tvq]

Fair, though it's one style flush per close on a path that has just changed data-state and needs that flush at the next frame anyway — so it's moved, not added. As of ad5daf7 it's getAnimations() rather than getComputedStyle, which flushes the same way; @radix-ui/react-presence does the equivalent per close. Caching the name would break the moment a consumer overrides the classes at runtime, which is the case the check exists for.

[djalmaaraujo]

exitAnimationNames acopla ao nome literal do keyframe do tailwindcss-animate exposto em animation-name (exit). Se o utilitário de animação mudar de nome (ou o projeto trocar a animação de saída por uma com outro nome), handleExitAnimationEnd nunca casa e o overlay nunca assenta o hidden. Comparar por animationName conhecido é frágil; considere não filtrar por nome (o guard de event.target === currentTarget + estado já basta para o caso do filho).

[tvq]

I think this reads the code as pinning "exit", but nothing here names a keyframe: the list is taken from the element's own animations after data-state="closed" is set (previously animation-name from computed style, now getAnimations() in ad5daf7), so whatever the exit is called — tw-animate-css's or a consumer's replacement — that's the name being matched. The only hard-coded "exit" in the library is tooltip_controller.js:54, which this PR doesn't touch.

The filter itself can't go: closing during the open animation cancels enter, and that animationcancel lands on the freshly armed listener with animationName: "enter". Without the name check it settles immediately and hides the element before the exit plays — that's the bug cubic caught on the first revision and 204845e fixed. target === currentTarget only covers bubbling from children, not the cancelled run on the element itself. It's the same guard @radix-ui/react-presence carries as isCurrentAnimation.

[djalmaaraujo]

Correção incompleta no design system (fora do diff, por isso comento aqui): o mesmo bug de "flash" no fechamento que este PR corrige em popover/hover_card/context_menu continua em gem/lib/ruby_ui/dropdown_menu/dropdown_menu_content.rb + dropdown_menu_controller.js. O content tem data-[state=closed]:animate-out e o close() ainda faz this.contentTarget.classList.add("hidden") na hora (cortando a saída), sem ganhar data-[state=closed]:fill-mode-forwards nem o hideAfterExitAnimation. O tooltip tem estrutura parecida. Como o mecanismo foi implementado como três cópias especiais em vez de compartilhado, os irmãos ficaram com o defeito idêntico — vale generalizar e cobrir todos os overlays com animate-out.

[tvq]

Done in 685f37c — every overlay with animate-out now plays its exit in this PR: DropdownMenu, ClipboardPopover, Select (which wasn't even on my follow-up list — it keyed its exit on the root's open value rather than data-state), Sheet and CommandDialog. Tooltip was already correct: tooltip_controller.js waits on animationend and checks the exit name before unmounting (:17, :53-58) — it's the pattern this PR follows, not one it left out.

None of the five was a copy-paste of the block: DropdownMenu/Clipboard never set data-state and animate an inner element while hidden sits on the wrapper; Select keyed on the open value; Sheet and CommandDialog remove() the element. The block now takes the animated element and hands the outcome to a one-line afterExit() per controller, so it stays byte-identical across all eight. Details per component are in the commit message; the description is updated to match. Extraction into a shared module is the one thing left open — see the thread above.

[cubic-dev-ai]

5 issues found across 19 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="gem/lib/ruby_ui/popover/popover_controller.js">

<violation number="1" location="gem/lib/ruby_ui/popover/popover_controller.js:125">
P2: The identical exit-animation block (afterExit, exitAnimationNames WeakMap, hideAfterExitAnimation, handleExitAnimationEnd, settleExit) is duplicated across 8 controllers in this batch, each carrying the comment "keep them in sync". Any future fix (e.g. the exit-detection or name-filter behavior) must be applied to all 8 copies or some will drift. Extract the shared logic into a small helper/module (parameterized with an afterExit callback) once this batch merges.</violation>

<violation number="2" location="gem/lib/ruby_ui/popover/popover_controller.js:139">
P2: When the content has more than one active CSS animation, the first matching `animationend` or `animationcancel` hides it, even if another exit animation is still running. Track the animations introduced by the closed-state transition and settle only after the exit run has completed.</violation>
</file>

<file name="gem/lib/ruby_ui/dropdown_menu/dropdown_menu_controller.js">

<violation number="1" location="gem/lib/ruby_ui/dropdown_menu/dropdown_menu_controller.js:111">
P2: When another CSS animation is active on the panel during close, its end or cancellation can call `settleExit` and cut off the actual exit animation. Track only the animation started by the closed state, or wait until that specific exit animation completes.</violation>
</file>

<file name="gem/lib/ruby_ui/hover_card/hover_card_controller.js">

<violation number="1" location="gem/lib/ruby_ui/hover_card/hover_card_controller.js:127">
P1: When the runtime lacks `Element.getAnimations()` or `CSSAnimation`, closing the hover card throws before `settleExit`, so the content never receives `hidden`. Feature-detect both APIs and fall back to immediate hiding when animation inspection is unavailable.</violation>
</file>

<file name="gem/lib/ruby_ui/clipboard/clipboard_controller.js">

<violation number="1" location="gem/lib/ruby_ui/clipboard/clipboard_controller.js:23">
P2: When the clipboard has no source child, this synchronous fallback shows the error during the trigger click, but the same click bubbles to the window handler and immediately starts closing it. Defer the fallback until after event propagation or make the outside handler ignore clicks inside the controller so the error remains visible.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: When the runtime lacks Element.getAnimations() or CSSAnimation, closing the hover card throws before settleExit, so the content never receives hidden. Feature-detect both APIs and fall back to immediate hiding when animation inspection is unavailable.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/hover_card/hover_card_controller.js, line 127:

<comment>When the runtime lacks `Element.getAnimations()` or `CSSAnimation`, closing the hover card throws before `settleExit`, so the content never receives `hidden`. Feature-detect both APIs and fall back to immediate hiding when animation inspection is unavailable.</comment>

<file context>
@@ -97,47 +103,58 @@ export default class extends Controller {
+  exitAnimationNames = new WeakMap();
+
+  hideAfterExitAnimation(animated) {
+    const exitAnimations = animated
+      .getAnimations()
+      .filter((animation) => animation instanceof CSSAnimation);
</file context>

Suggested change

	const exitAnimations = animated
	.getAnimations()
	.filter((animation) => animation instanceof CSSAnimation);
	const exitAnimations =
	typeof animated.getAnimations === "function" && typeof CSSAnimation !== "undefined"
	? animated.getAnimations().filter((animation) => animation instanceof CSSAnimation)
	: [];

[cubic-dev-ai]

P2: When the content has more than one active CSS animation, the first matching animationend or animationcancel hides it, even if another exit animation is still running. Track the animations introduced by the closed-state transition and settle only after the exit run has completed.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/popover/popover_controller.js, line 139:

<comment>When the content has more than one active CSS animation, the first matching `animationend` or `animationcancel` hides it, even if another exit animation is still running. Track the animations introduced by the closed-state transition and settle only after the exit run has completed.</comment>

<file context>
@@ -115,40 +115,48 @@ export default class extends Controller {
-    this.exitAnimationNames = styles.animationName.split(",").map((name) => name.trim());
-    content.addEventListener("animationend", this.handleExitAnimationEnd);
-    content.addEventListener("animationcancel", this.handleExitAnimationEnd);
+    this.exitAnimationNames.set(animated, exitAnimations.map((animation) => animation.animationName));
+    animated.addEventListener("animationend", this.handleExitAnimationEnd);
+    animated.addEventListener("animationcancel", this.handleExitAnimationEnd);
</file context>

[cubic-dev-ai]

P2: When another CSS animation is active on the panel during close, its end or cancellation can call settleExit and cut off the actual exit animation. Track only the animation started by the closed state, or wait until that specific exit animation completes.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/dropdown_menu/dropdown_menu_controller.js, line 111:

<comment>When another CSS animation is active on the panel during close, its end or cancellation can call `settleExit` and cut off the actual exit animation. Track only the animation started by the closed state, or wait until that specific exit animation completes.</comment>

<file context>
@@ -77,13 +78,57 @@ export default class extends Controller {
+      return;
+    }
+
+    this.exitAnimationNames.set(animated, exitAnimations.map((animation) => animation.animationName));
+    animated.addEventListener("animationend", this.handleExitAnimationEnd);
+    animated.addEventListener("animationcancel", this.handleExitAnimationEnd);
</file context>

[cubic-dev-ai]

P2: When the clipboard has no source child, this synchronous fallback shows the error during the trigger click, but the same click bubbles to the window handler and immediately starts closing it. Defer the fallback until after event propagation or make the outside handler ignore clicks inside the controller so the error remains visible.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/clipboard/clipboard_controller.js, line 23:

<comment>When the clipboard has no source child, this synchronous fallback shows the error during the trigger click, but the same click bubbles to the window handler and immediately starts closing it. Defer the fallback until after event propagation or make the outside handler ignore clicks inside the controller so the error remains visible.</comment>

<file context>
@@ -3,18 +3,24 @@ import { computePosition, flip, shift } from "@floating-ui/dom";
     let sourceElement = this.sourceTarget.children[0];
     if (!sourceElement) {
-      this.showErrorPopover();
+      this.#showErrorPopover();
       return;
     }
</file context>

Suggested change

	this.#showErrorPopover();
	queueMicrotask(() => this.#showErrorPopover());

[cubic-dev-ai]

P2: The identical exit-animation block (afterExit, exitAnimationNames WeakMap, hideAfterExitAnimation, handleExitAnimationEnd, settleExit) is duplicated across 8 controllers in this batch, each carrying the comment "keep them in sync". Any future fix (e.g. the exit-detection or name-filter behavior) must be applied to all 8 copies or some will drift. Extract the shared logic into a small helper/module (parameterized with an afterExit callback) once this batch merges.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/popover/popover_controller.js, line 125:

<comment>The identical exit-animation block (afterExit, exitAnimationNames WeakMap, hideAfterExitAnimation, handleExitAnimationEnd, settleExit) is duplicated across 8 controllers in this batch, each carrying the comment "keep them in sync". Any future fix (e.g. the exit-detection or name-filter behavior) must be applied to all 8 copies or some will drift. Extract the shared logic into a small helper/module (parameterized with an afterExit callback) once this batch merges.</comment>

<file context>
@@ -115,40 +115,48 @@ export default class extends Controller {
-    // An element with no exit animation never fires animationend.
-    if (styles.animationName === "none" || styles.display === "none") {
-      this.settleExit(content);
+  // Overlay exit — the same block in every overlay controller, so keep them in sync.
+  exitAnimationNames = new WeakMap();
+
</file context>