[Bug Fix] Popover: set data-state/data-side, clear closeTimeout on disconnect, close on Escape

[djalmaaraujo]

Related issue

Related to #494

Description

Brings Popover up to the standard HoverCard got in #438. popover_controller.js had not been touched since the monorepo unification (#358), so PopoverContent was shipping eight Tailwind variant classes that could never match, plus a timer that outlived the controller.

Four changes, each mirroring what hover_card_controller.js already does rather than inventing a new approach:

• data-state — PopoverContent now server-renders data-state="closed" (as HoverCardContent does), and the controller sets open/closed in showPopover/hidePopover. This is what makes data-[state=open]:animate-in, fade-in-0 and zoom-in-95 actually resolve; before this they were dead CSS.
• data-side — updatePosition now reads placement back out of computePosition and writes dataset.side from it. Reading the resolved placement matters specifically because flip() is in the middleware chain, so the popover can land on the opposite side of the requested one; only the resolved value makes the data-[side=*]:slide-in-from-* classes point the right way.
• closeTimeout — cleared in disconnect(), and this.cleanup is nulled after being invoked. Without this, a disconnect+reconnect inside the 100 ms hover-close window leaves the dead instance's timeout pending; it then resolves contentTarget to the same re-attached element and hides it, closing a popover the fresh instance had just opened. No console error — the popover just snaps shut.
• Escape — closes the popover, with the document keydown listener added on open and removed on close and on disconnect. HoverCard and DropdownMenu both already do this; without it, a trigger: "click" popover has no keyboard dismissal at all.

One extra change fell out of testing the third item. disconnect() used to start with the element-listener teardown, which resolves this.triggerTarget / this.contentTarget — and resolving a target throws once that element is gone. Stimulus catches the throw and logs Error disconnecting controller, so the page survives, but every line after it in disconnect() is skipped: clearTimeout never runs and the pending close leaks anyway, which defeats the fix. disconnect() now runs the teardown that cannot fail first (timer, both document listeners, the autoUpdate cleanup) and touches targets last. The document-level click removal moved out of the element teardown for the same reason, so the method is now element-only and renamed removeElementEventListeners to say so.

Review round follow-up: the ordering rule above was applied in disconnect() but not consistently, and three related gaps were fixed on top.

• hidePopover() had the same hazard — it mutated contentTarget before releasing the keydown listener and the autoUpdate cleanup, so losing the content target while the controller stayed connected leaked both. It now releases first and guards the content mutation.
• removeElementEventListeners() dereferenced both targets unconditionally, so losing only the content target left the trigger's click listener attached and pointing at a dead controller. Each target is now guarded separately.
• openValue is stored in the DOM, so a reconnect arrives already open — but connect() never re-armed the keydown listener or the positioning, leaving a popover that looked open while Escape did nothing and the position was frozen. connect() now calls showPopover() when openValue is true.
• The autoUpdate callback also bails out instead of throwing if a target disappears before anything tears the popover down.

A second round tightened the same area further:

• updatePosition() captures the exact trigger/content pair it positions instead of re-reading the targets, and both the autoUpdate callback and the computePosition continuation check isConnected before touching them — a target detached or swapped mid-flight is skipped rather than throwing or being written to while stale.
• When a target does go away, the autoUpdate handle is now actually released through a shared stopAutoUpdate() (also used by hidePopover() and disconnect()), rather than the callback merely returning and leaving the observers firing on every scroll and resize. The release is deferred with queueMicrotask because autoUpdate invokes its callback once synchronously, before the handle has been assigned.
• openValue is no longer assigned by the callers. showPopover() sets it true only after its guard passes and hidePopover() sets it false, so a guarded early return can't leave the DOM claiming open while nothing is wired up — and a click while the content target is missing no longer wedges the popover permanently closed. As a side effect the five duplicated openValue = ...; show/hide pairs collapse into the two methods that own the transition.

connect()'s addEventListeners() is deliberately left unguarded: markup missing a trigger or content is an authoring error and should fail loudly. The guards are on the teardown and positioning paths, whose job is to release or update things and which therefore must not be able to fail.

Escape stays an imperative document keydown listener rather than a declarative keydown.esc@document-> action, for two reasons: the listener is only registered while the popover is actually open (a declarative action would register one per popover for the lifetime of the page — the docs page alone renders 14), and the gem ships this controller independently of the Ruby components, so behavior kept in the controller keeps working for consumers who write their own markup. It also matches how HoverCard and DropdownMenu already handle Escape.

Both copies of the controller are updated (gem/lib/ruby_ui/popover/ and docs/app/javascript/controllers/ruby_ui/) — they were byte-identical and letting them drift is what #490 had to repair for Accordion. mcp/data/registry.json is regenerated, since it embeds the component source and the CI freshness check fails otherwise.

Deliberately out of scope

• Exit animation. hidden (display: none) lands in the same frame as data-state="closed", so animate-out still can't be seen. HoverCard and DropdownMenu share this exact limitation; only Tooltip solves it, by deferring unmount on animationend. Fixing it properly is a cross-component change and belongs in its own issue.
• Turbo snapshot state. A popover left open when Turbo caches the page is restored open — the hidden class is already absent in the snapshot, so this predates the change and is not made worse by it. Tooltip is the only component that unmounts on turbo:before-cache.

Testing instructions

cd gem && bundle exec rake covers the server-rendered attribute (281 runs, plus standardrb over 402 files). The behavior is in the Stimulus controller, so the rest needs a browser — the docs app has no system tests, so these are manual steps.

Start the docs app (cd docs && bin/dev) and open http://localhost:3000/docs/popover.

1. Enter animation now runs (the data-state fix)

1. Hover the "Open Popover" trigger in the Example section.
2. It should fade and zoom in, not appear instantly.
3. With it open, inspect the content div in devtools: it must carry data-state="open". Move the mouse away and it flips to data-state="closed".
4. On main, the same element has no data-state attribute at all and the panel pops in with no transition.

2. data-side follows the resolved placement

1. Scroll to the Placement section and hover the top, right, left and bottom triggers in turn.
2. Each content div should gain a bare-axis data-side (top/right/left/bottom) and slide in from the opposite edge.
3. Shrink the window until a top popover has no room above it — flip() moves it below and data-side must follow to bottom, so the slide direction inverts with it. This is the case the requested-placement value would get wrong.

3. Escape closes it, and it reopens afterwards

1. In the Trigger section (options: { trigger: 'click' }), click "Click" to open.
2. Press Esc — it closes.
3. Click the trigger again — it must reopen (confirms the keydown listener bookkeeping is balanced, not left dangling or double-registered).
4. Click it once more to toggle closed, then click outside — both close paths should still behave.

4. The stale-timer bug (the reason for the disconnect change)

In the devtools console on the popover docs page:

const el = document.querySelector("[data-controller='ruby-ui--popover']");
const parent = el.parentNode;
const trigger = el.querySelector("[data-ruby-ui--popover-target='trigger']");
const content = el.querySelector("[data-ruby-ui--popover-target='content']");

trigger.dispatchEvent(new MouseEvent("mouseenter", {bubbles: true}));
setTimeout(() => {
  trigger.dispatchEvent(new MouseEvent("mouseleave", {bubbles: true})); // schedules the 100ms close
  el.remove(); parent.appendChild(el);                                  // disconnect + reconnect inside that window
  setTimeout(() => {
    el.querySelector("[data-ruby-ui--popover-target='trigger']")
      .dispatchEvent(new MouseEvent("mouseenter", {bubbles: true}));    // fresh instance opens it
    setTimeout(() => console.log("hidden:", content.classList.contains("hidden")), 400);
  }, 20);
}, 300);

Expected: hidden: false — the popover stays open. On main this logs hidden: true: the dead instance's timeout fired and closed it.

5. Teardown survives a missing target (the disconnect ordering)

Same console, fresh reload. Here the trigger is removed while the close is pending, so the element teardown throws but contentTarget still resolves — which makes it visible whether clearTimeout got to run:

const el = document.querySelector("[data-controller='ruby-ui--popover']");
const parent = el.parentNode;
const trigger = el.querySelector("[data-ruby-ui--popover-target='trigger']");
const content = el.querySelector("[data-ruby-ui--popover-target='content']");

trigger.dispatchEvent(new MouseEvent("mouseenter", {bubbles: true}));
setTimeout(() => {
  trigger.dispatchEvent(new MouseEvent("mouseleave", {bubbles: true})); // schedule the 100ms close
  trigger.remove();                                                     // trigger target disappears
  el.remove(); parent.appendChild(el);                                  // -> disconnect()
  setTimeout(() => console.log("state:", content.getAttribute("data-state")), 600);
}, 300);

Expected: state: open — the timer was cleared before the throw. With the teardown in the old order this logs state: closed, i.e. the stale close still fired. Stimulus will log Error disconnecting controller either way: the DOM really is missing a target at that point, and that part is pre-existing (addEventListeners throws the same way on connect, unguarded, on main too) — what changed is that it no longer costs us the cleanup.

6. Reconnect keeps a popover fully functional, not just visually open

const el = document.querySelector("[data-controller='ruby-ui--popover']");
const parent = el.parentNode;
const trigger = el.querySelector("[data-ruby-ui--popover-target='trigger']");
const content = el.querySelector("[data-ruby-ui--popover-target='content']");

trigger.dispatchEvent(new MouseEvent("mouseenter", {bubbles: true}));
setTimeout(() => {
  el.remove(); parent.appendChild(el);   // disconnect + reconnect while open
  setTimeout(() => {
    document.dispatchEvent(new KeyboardEvent("keydown", {key: "Escape", bubbles: true}));
    setTimeout(() => console.log("state:", content.getAttribute("data-state")), 200);
  }, 100);
}, 400);

Expected: state: closed — Escape still works on the reconnected instance. Without the connect() re-arm this logs state: open: the popover is visibly open but nothing is listening.

7. Losing only the content target does not strand the trigger

const el = Array.from(document.querySelectorAll("[data-controller='ruby-ui--popover']"))
  .find(e => e.getAttribute("data-ruby-ui--popover-trigger-value") === "click");
const parent = el.parentNode;
const trigger = el.querySelector("[data-ruby-ui--popover-target='trigger']");

el.querySelector("[data-ruby-ui--popover-target='content']").remove();
el.remove(); parent.appendChild(el);
setTimeout(() => trigger.click(), 200);   // must not raise

Expected: no Missing target element "content" error in the console from the click. Without the per-target guards the trigger keeps its listener and the click reaches a disconnected controller.

8. No regressions in the other suites

• cd docs && bundle exec standardrb && bin/rails db:test:prepare test (75 runs)
• cd mcp && bundle exec rake test (24 runs), and bundle exec exe/ruby-ui-mcp-build must leave registry.json unchanged

[cubic-dev-ai]

All reported issues were addressed across 5 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>

[cubic-dev-ai]

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

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

Re-trigger cubic</sub>