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

docs/app/javascript/controllers/ruby_ui/popover_controller.js

@@ -19,13 +19,20 @@ export default class extends Controller {
     this.closeTimeout = null;
     this.cleanup = null;
     this.addEventListeners();
+    // openValue lives in the DOM, so a reconnect (frame swap, morph, moved element)
+    // arrives already open. Re-arm the parts that live on the controller instead of
+    // the markup — the keydown listener and the autoUpdate positioning.
+    if (this.openValue) this.showPopover();
   }
 
+  // Teardown that cannot fail comes first: resolving a target throws once the
+  // element is gone, and Stimulus swallows that, skipping the rest of disconnect.
   disconnect() {
-    this.removeEventListeners();
-    if (this.cleanup) {
-      this.cleanup();
-    }
+    clearTimeout(this.closeTimeout);
+    document.removeEventListener("keydown", this.handleKeydown);
+    document.removeEventListener("click", this.handleOutsideClick);
+    this.stopAutoUpdate();
+    this.removeElementEventListeners();
   }
 
   addEventListeners() {
@@ -40,68 +47,124 @@ export default class extends Controller {
     }
   }
 
-  removeEventListeners() {
-    this.triggerTarget.removeEventListener("mouseenter", this.handleMouseEnter);
-    this.triggerTarget.removeEventListener("mouseleave", this.handleMouseLeave);
-    this.contentTarget.removeEventListener("mouseenter", this.handleMouseEnter);
-    this.contentTarget.removeEventListener("mouseleave", this.handleMouseLeave);
-    this.triggerTarget.removeEventListener("click", this.handleClick);
-    document.removeEventListener("click", this.handleOutsideClick);
+  // Each target is guarded on its own: losing one of them must not strand the
+  // listeners attached to the other.
+  removeElementEventListeners() {
+    if (this.hasTriggerTarget) {
+      this.triggerTarget.removeEventListener("mouseenter", this.handleMouseEnter);
+      this.triggerTarget.removeEventListener("mouseleave", this.handleMouseLeave);
+      this.triggerTarget.removeEventListener("click", this.handleClick);
+    }
+
+    if (this.hasContentTarget) {
+      this.contentTarget.removeEventListener("mouseenter", this.handleMouseEnter);
+      this.contentTarget.removeEventListener("mouseleave", this.handleMouseLeave);
+    }
   }
 
   handleMouseEnter = () => {
     clearTimeout(this.closeTimeout);
-    this.openValue = true;
     this.showPopover();
   };
 
   handleMouseLeave = () => {
-    this.closeTimeout = setTimeout(() => {
-      this.openValue = false;
-      this.hidePopover();
-    }, 100);
+    this.closeTimeout = setTimeout(() => this.hidePopover(), 100);
   };
 
   handleClick = (event) => {
     event.stopPropagation();
-    this.openValue = !this.openValue;
-    this.openValue ? this.showPopover() : this.hidePopover();
+    this.openValue ? this.hidePopover() : this.showPopover();
   };
 
   handleOutsideClick = (event) => {
-    if (!this.element.contains(event.target) && this.openValue) {
-      this.openValue = false;
-      this.hidePopover();
-    }
+    if (this.element.contains(event.target)) return;
+    if (!this.openValue) return;
+
+    this.hidePopover();
   };
 
+  handleKeydown = (event) => {
+    if (event.key !== "Escape") return;
+    if (!this.openValue) return;
+
+    clearTimeout(this.closeTimeout);
+    this.hidePopover();
+  };
+
+  // openValue is set here rather than by the callers, so a guarded early return can
+  // never leave the DOM claiming the popover is open while nothing is wired up.
   showPopover() {
+    if (!this.hasTriggerTarget || !this.hasContentTarget) return;
+
+    this.openValue = true;
     this.contentTarget.classList.remove("hidden");
+    this.contentTarget.dataset.state = "open";
+    document.addEventListener("keydown", this.handleKeydown);
     this.updatePosition();
   }
 
+  // Same rule as disconnect(): release what is held outside the element first, so a
+  // missing content target cannot leave the keydown listener or autoUpdate running.
   hidePopover() {
+    this.openValue = false;
+    document.removeEventListener("keydown", this.handleKeydown);
+    this.stopAutoUpdate();
+
+    if (!this.hasContentTarget) return;
+
     this.contentTarget.classList.add("hidden");
-    if (this.cleanup) {
-      this.cleanup();
-    }
+    this.contentTarget.dataset.state = "closed";
   }
 
   updatePosition() {
-    if (this.cleanup) {
-      this.cleanup();
-    }
+    this.stopAutoUpdate();
+
+    // Hold the exact pair this run positions. A target can be detached or swapped
+    // while the controller stays connected, and the stale element must not be
+    // written to by an observer callback or an in-flight computePosition.
+    const trigger = this.triggerTarget;
+    const content = this.contentTarget;
+
+    // Deferred teardown is bound to this run's own handle, so a newer positioning
+    // run installed before the microtask drains is never torn down by an older one.
+    let stop = null;
+    const releaseThisRun = () => {
+      stop?.();
+      if (this.cleanup === stop) this.cleanup = null;
+    };
 
-    this.cleanup = autoUpdate(this.triggerTarget, this.contentTarget, () => {
-      computePosition(this.triggerTarget, this.contentTarget, {
+    stop = autoUpdate(trigger, content, () => {
+      if (!trigger.isConnected || !content.isConnected) {
+        // Release the observers instead of throwing on every scroll and resize.
+        // Deferred because autoUpdate runs this once synchronously, before the
+        // handle below has been assigned.
+        queueMicrotask(releaseThisRun);
+        return;
+      }
+
+      computePosition(trigger, content, {
         placement: this.optionsValue.placement || "bottom",
         middleware: [flip(), shift(), offset(8)],
-      }).then(({ x, y }) => {
-        Object.assign(this.contentTarget.style, {
+      }).then(({ x, y, placement }) => {
+        if (!content.isConnected) return;
+
+        Object.assign(content.style, {
           left: `${x}px`,
           top: `${y}px`,
         });
+        // flip() can resolve to the opposite side of the requested placement,
+        // so the directional slide-in classes must follow the resolved value.
+        content.dataset.side = placement.split("-")[0];
       });
     });
+
+    this.cleanup = stop;
+  }
+
+  stopAutoUpdate() {
+    if (!this.cleanup) return;
+
+    this.cleanup();
+    this.cleanup = null;
   }
 }

gem/lib/ruby_ui/popover/popover_content.rb

@@ -11,7 +11,8 @@ def view_template(&)
     def default_attrs
       {
         data: {
-          ruby_ui__popover_target: "content"
+          ruby_ui__popover_target: "content",
+          state: :closed
         },
         class: [
           "hidden z-50 rounded-md border bg-background p-1 text-foreground shadow-md outline-none",

gem/lib/ruby_ui/popover/popover_controller.js

@@ -19,13 +19,20 @@ export default class extends Controller {
     this.closeTimeout = null;
     this.cleanup = null;
     this.addEventListeners();
+    // openValue lives in the DOM, so a reconnect (frame swap, morph, moved element)
+    // arrives already open. Re-arm the parts that live on the controller instead of
+    // the markup — the keydown listener and the autoUpdate positioning.
+    if (this.openValue) this.showPopover();
   }
 
+  // Teardown that cannot fail comes first: resolving a target throws once the
+  // element is gone, and Stimulus swallows that, skipping the rest of disconnect.
   disconnect() {
-    this.removeEventListeners();
-    if (this.cleanup) {
-      this.cleanup();
-    }
+    clearTimeout(this.closeTimeout);
+    document.removeEventListener("keydown", this.handleKeydown);
+    document.removeEventListener("click", this.handleOutsideClick);
+    this.stopAutoUpdate();
+    this.removeElementEventListeners();
   }
 
   addEventListeners() {
@@ -40,68 +47,124 @@ export default class extends Controller {
     }
   }
 
-  removeEventListeners() {
-    this.triggerTarget.removeEventListener("mouseenter", this.handleMouseEnter);
-    this.triggerTarget.removeEventListener("mouseleave", this.handleMouseLeave);
-    this.contentTarget.removeEventListener("mouseenter", this.handleMouseEnter);
-    this.contentTarget.removeEventListener("mouseleave", this.handleMouseLeave);
-    this.triggerTarget.removeEventListener("click", this.handleClick);
-    document.removeEventListener("click", this.handleOutsideClick);
+  // Each target is guarded on its own: losing one of them must not strand the
+  // listeners attached to the other.
+  removeElementEventListeners() {
+    if (this.hasTriggerTarget) {
+      this.triggerTarget.removeEventListener("mouseenter", this.handleMouseEnter);
+      this.triggerTarget.removeEventListener("mouseleave", this.handleMouseLeave);
+      this.triggerTarget.removeEventListener("click", this.handleClick);
+    }
+
+    if (this.hasContentTarget) {
+      this.contentTarget.removeEventListener("mouseenter", this.handleMouseEnter);
+      this.contentTarget.removeEventListener("mouseleave", this.handleMouseLeave);
+    }
   }
 
   handleMouseEnter = () => {
     clearTimeout(this.closeTimeout);
-    this.openValue = true;
     this.showPopover();
   };
 
   handleMouseLeave = () => {
-    this.closeTimeout = setTimeout(() => {
-      this.openValue = false;
-      this.hidePopover();
-    }, 100);
+    this.closeTimeout = setTimeout(() => this.hidePopover(), 100);
   };
 
   handleClick = (event) => {
     event.stopPropagation();
-    this.openValue = !this.openValue;
-    this.openValue ? this.showPopover() : this.hidePopover();
+    this.openValue ? this.hidePopover() : this.showPopover();
   };
 
   handleOutsideClick = (event) => {
-    if (!this.element.contains(event.target) && this.openValue) {
-      this.openValue = false;
-      this.hidePopover();
-    }
+    if (this.element.contains(event.target)) return;
+    if (!this.openValue) return;
+
+    this.hidePopover();
   };
 
+  handleKeydown = (event) => {
+    if (event.key !== "Escape") return;
+    if (!this.openValue) return;
+
+    clearTimeout(this.closeTimeout);
+    this.hidePopover();
+  };
+
+  // openValue is set here rather than by the callers, so a guarded early return can
+  // never leave the DOM claiming the popover is open while nothing is wired up.
   showPopover() {
+    if (!this.hasTriggerTarget || !this.hasContentTarget) return;
+
+    this.openValue = true;
     this.contentTarget.classList.remove("hidden");
+    this.contentTarget.dataset.state = "open";
+    document.addEventListener("keydown", this.handleKeydown);
     this.updatePosition();
   }
 
+  // Same rule as disconnect(): release what is held outside the element first, so a
+  // missing content target cannot leave the keydown listener or autoUpdate running.
   hidePopover() {
+    this.openValue = false;
+    document.removeEventListener("keydown", this.handleKeydown);
+    this.stopAutoUpdate();
+
+    if (!this.hasContentTarget) return;
+
     this.contentTarget.classList.add("hidden");
-    if (this.cleanup) {
-      this.cleanup();
-    }
+    this.contentTarget.dataset.state = "closed";
   }
 
   updatePosition() {
-    if (this.cleanup) {
-      this.cleanup();
-    }
+    this.stopAutoUpdate();
+
+    // Hold the exact pair this run positions. A target can be detached or swapped
+    // while the controller stays connected, and the stale element must not be
+    // written to by an observer callback or an in-flight computePosition.
+    const trigger = this.triggerTarget;
+    const content = this.contentTarget;
+
+    // Deferred teardown is bound to this run's own handle, so a newer positioning
+    // run installed before the microtask drains is never torn down by an older one.
+    let stop = null;
+    const releaseThisRun = () => {
+      stop?.();
+      if (this.cleanup === stop) this.cleanup = null;
+    };
 
-    this.cleanup = autoUpdate(this.triggerTarget, this.contentTarget, () => {
-      computePosition(this.triggerTarget, this.contentTarget, {
+    stop = autoUpdate(trigger, content, () => {
+      if (!trigger.isConnected || !content.isConnected) {
+        // Release the observers instead of throwing on every scroll and resize.
+        // Deferred because autoUpdate runs this once synchronously, before the
+        // handle below has been assigned.
+        queueMicrotask(releaseThisRun);
+        return;
+      }
+
+      computePosition(trigger, content, {
         placement: this.optionsValue.placement || "bottom",
         middleware: [flip(), shift(), offset(8)],
-      }).then(({ x, y }) => {
-        Object.assign(this.contentTarget.style, {
+      }).then(({ x, y, placement }) => {
+        if (!content.isConnected) return;
+
+        Object.assign(content.style, {
           left: `${x}px`,
           top: `${y}px`,
         });
+        // flip() can resolve to the opposite side of the requested placement,
+        // so the directional slide-in classes must follow the resolved value.
+        content.dataset.side = placement.split("-")[0];
       });
     });
+
+    this.cleanup = stop;
+  }
+
+  stopAutoUpdate() {
+    if (!this.cleanup) return;
+
+    this.cleanup();
+    this.cleanup = null;
   }
 }

gem/test/ruby_ui/popover_test.rb

@@ -25,4 +25,17 @@ def test_render_with_all_items
 
     assert_match(/Profile/, output)
   end
+
+  # The animation classes on the content are keyed on data-state, so the
+  # attribute has to be present before the Stimulus controller ever runs.
+  def test_content_renders_closed_state_by_default
+    output = phlex do
+      RubyUI.PopoverContent { "popover body" }
+    end
+
+    assert_match(/data-state="closed"/, output)
+    assert_match(/hidden/, output)
+    assert_match(/absolute/, output)
+    assert_match(/popover body/, output)
+  end
 end