Popover: data-state/data-side never set (animation classes are dead CSS), closeTimeout leaks past disconnect, no Escape

[djalmaaraujo]

Summary

Popover was never brought up to the standard its closest sibling HoverCard got in #438 (the Popper → Floating UI port). popover_controller.js is untouched since the monorepo unification (#358), while hover_card_controller.js gained state attributes, side-aware positioning, timer cleanup and keyboard dismissal.

The result is that PopoverContent ships eight Tailwind variant classes that can never match, plus a timer that can outlive the controller.

1. data-state is never set — the enter/exit animation classes are dead CSS

gem/lib/ruby_ui/popover/popover_content.rb declares:

"data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0",
"data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95",

But popover_controller.js only toggles the hidden class:

showPopover() {
  this.contentTarget.classList.remove("hidden");
  this.updatePosition();
}

hidePopover() {
  this.contentTarget.classList.add("hidden");
  if (this.cleanup) {
    this.cleanup();
  }
}

Nothing ever writes data-state on the content element, and unlike HoverCardContent (which server-renders data: { ..., state: :closed }), PopoverContent doesn't even carry an initial value. So the attribute simply never exists and the popover appears/disappears with no transition — visually inconsistent with HoverCard, DropdownMenu and Tooltip, which all animate.

hover_card_controller.js is the reference:

show() {
  this.openValue = true;
  this.contentTarget.classList.remove("hidden");
  this.contentTarget.dataset.state = "open";
  // ...
}

hide() {
  this.openValue = false;
  this.contentTarget.classList.add("hidden");
  this.contentTarget.dataset.state = "closed";
  // ...
}

2. data-side is never set — the directional slide classes are dead CSS too

Same root cause, one layer further. popover_content.rb also declares:

"data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2",
"data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",

updatePosition() destructures only { x, y } from computePosition and throws away the resolved placement:

computePosition(this.triggerTarget, this.contentTarget, {
  placement: this.optionsValue.placement || "bottom",
  middleware: [flip(), shift(), offset(8)],
}).then(({ x, y }) => {
  Object.assign(this.contentTarget.style, { left: `${x}px`, top: `${y}px` });
});

The resolved placement matters here precisely because flip() is in the middleware chain — the popover can end up on the opposite side of the requested one, and only the resolved value is correct. hover_card_controller.js already does this:

}).then(({ x, y, placement }) => {
  Object.assign(this.contentTarget.style, { left: `${x}px`, top: `${y}px` });
  this.contentTarget.dataset.side = placement.split("-")[0];
});

3. closeTimeout survives disconnect()

handleMouseLeave schedules a 100 ms close:

handleMouseLeave = () => {
  this.closeTimeout = setTimeout(() => {
    this.openValue = false;
    this.hidePopover();
  }, 100);
};

disconnect() never clears it:

disconnect() {
  this.removeEventListeners();
  if (this.cleanup) {
    this.cleanup();
  }
}

If the element is disconnected inside that 100 ms window and reconnected — a Turbo frame or stream replacement, a morph, an element moved in the DOM — Stimulus builds a fresh controller instance while the old instance's timeout is still pending. The stale callback resolves this.contentTarget to the same, re-attached element and hides it, so a popover the new instance has just opened snaps shut on its own. There is no console error to go on; the symptom is a popover that closes for no visible reason.

hover_card_controller.js clears its timers on disconnect (clearTimers()), and also nulls out this.cleanup after invoking it, which the popover doesn't — so a stale autoUpdate cleanup stays referenced.

4. No keyboard dismissal

HoverCard and DropdownMenu both close on Escape. Popover has no keydown handling at all, so with options: { trigger: "click" } a keyboard user can open the popover and has no way to dismiss it — the only close paths are an outside mouse click or a second click on the trigger.

Proposed fix

Bring Popover to parity with HoverCard, mirroring its implementation rather than inventing a new approach:

• server-render data-state="closed" on PopoverContent, and set dataset.state to open/closed in showPopover/hidePopover;
• read placement back out of computePosition and set dataset.side from it;
• clear closeTimeout in disconnect(), and null this.cleanup after calling it;
• close on Escape, with the document keydown listener added on open and removed on close/disconnect.

Both copies of the controller (gem/lib/ruby_ui/popover/popover_controller.js and docs/app/javascript/controllers/ruby_ui/popover_controller.js) need the change — they are currently byte-identical and drifting them apart is what #490 had to fix for Accordion.

Known limitation, deliberately out of scope

Because hidden (display: none) is applied in the same frame as data-state="closed", the animate-out exit animation still won't be visible — the element is removed from the render tree before the animation can run. HoverCard and DropdownMenu have the identical limitation; only Tooltip handles it, via an animationend listener that defers unmounting. Making the exit animation actually play is a broader change across all the hidden-toggling floating components and should be its own issue, so this one stops at the enter animation, the side-aware slide, and the correctness bugs.