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

Author: djalmaaraujo

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

@@ -23,8 +23,11 @@ export default class extends Controller {
 
   disconnect() {
     this.removeEventListeners();
+    clearTimeout(this.closeTimeout);
+    document.removeEventListener("keydown", this.handleKeydown);
     if (this.cleanup) {
       this.cleanup();
+      this.cleanup = null;
     }
   }
 
@@ -75,15 +78,29 @@ export default class extends Controller {
     }
   };
 
+  handleKeydown = (event) => {
+    if (event.key !== "Escape") return;
+    if (!this.openValue) return;
+
+    clearTimeout(this.closeTimeout);
+    this.openValue = false;
+    this.hidePopover();
+  };
+
   showPopover() {
     this.contentTarget.classList.remove("hidden");
+    this.contentTarget.dataset.state = "open";
+    document.addEventListener("keydown", this.handleKeydown);
     this.updatePosition();
   }
 
   hidePopover() {
     this.contentTarget.classList.add("hidden");
+    this.contentTarget.dataset.state = "closed";
+    document.removeEventListener("keydown", this.handleKeydown);
     if (this.cleanup) {
       this.cleanup();
+      this.cleanup = null;
     }
   }
 
@@ -96,11 +113,14 @@ export default class extends Controller {
       computePosition(this.triggerTarget, this.contentTarget, {
         placement: this.optionsValue.placement || "bottom",
         middleware: [flip(), shift(), offset(8)],
-      }).then(({ x, y }) => {
+      }).then(({ x, y, placement }) => {
         Object.assign(this.contentTarget.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.
+        this.contentTarget.dataset.side = placement.split("-")[0];
       });
     });
   }

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

@@ -23,8 +23,11 @@ export default class extends Controller {
 
   disconnect() {
     this.removeEventListeners();
+    clearTimeout(this.closeTimeout);
+    document.removeEventListener("keydown", this.handleKeydown);
     if (this.cleanup) {
       this.cleanup();
+      this.cleanup = null;
     }
   }
 
@@ -75,15 +78,29 @@ export default class extends Controller {
     }
   };
 
+  handleKeydown = (event) => {
+    if (event.key !== "Escape") return;
+    if (!this.openValue) return;
+
+    clearTimeout(this.closeTimeout);
+    this.openValue = false;
+    this.hidePopover();
+  };
+
   showPopover() {
     this.contentTarget.classList.remove("hidden");
+    this.contentTarget.dataset.state = "open";
+    document.addEventListener("keydown", this.handleKeydown);
     this.updatePosition();
   }
 
   hidePopover() {
     this.contentTarget.classList.add("hidden");
+    this.contentTarget.dataset.state = "closed";
+    document.removeEventListener("keydown", this.handleKeydown);
     if (this.cleanup) {
       this.cleanup();
+      this.cleanup = null;
     }
   }
 
@@ -96,11 +113,14 @@ export default class extends Controller {
       computePosition(this.triggerTarget, this.contentTarget, {
         placement: this.optionsValue.placement || "bottom",
         middleware: [flip(), shift(), offset(8)],
-      }).then(({ x, y }) => {
+      }).then(({ x, y, placement }) => {
         Object.assign(this.contentTarget.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.
+        this.contentTarget.dataset.side = placement.split("-")[0];
       });
     });
   }

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

mcp/data/registry.json

@@ -2053,11 +2053,11 @@
         },
         {
           "path": "popover_content.rb",
-          "content": "# frozen_string_literal: true\n\nmodule RubyUI\n  class PopoverContent < Base\n    def view_template(&)\n      div(**attrs, &)\n    end\n\n    private\n\n    def default_attrs\n      {\n        data: {\n          ruby_ui__popover_target: \"content\"\n        },\n        class: [\n          \"hidden z-50 rounded-md border bg-background p-1 text-foreground shadow-md outline-none\",\n          \"data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0\",\n          \"data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95\",\n          \"data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2\",\n          \"data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2\",\n          \"absolute\"\n        ]\n      }\n    end\n  end\nend\n"
+          "content": "# frozen_string_literal: true\n\nmodule RubyUI\n  class PopoverContent < Base\n    def view_template(&)\n      div(**attrs, &)\n    end\n\n    private\n\n    def default_attrs\n      {\n        data: {\n          ruby_ui__popover_target: \"content\",\n          state: :closed\n        },\n        class: [\n          \"hidden z-50 rounded-md border bg-background p-1 text-foreground shadow-md outline-none\",\n          \"data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0\",\n          \"data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95\",\n          \"data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2\",\n          \"data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2\",\n          \"absolute\"\n        ]\n      }\n    end\n  end\nend\n"
         },
         {
           "path": "popover_controller.js",
-          "content": "import { Controller } from \"@hotwired/stimulus\";\nimport {\n  computePosition,\n  flip,\n  shift,\n  offset,\n  autoUpdate,\n} from \"@floating-ui/dom\";\n\nexport default class extends Controller {\n  static targets = [\"trigger\", \"content\"];\n  static values = {\n    open: { type: Boolean, default: false },\n    options: { type: Object, default: {} },\n    trigger: { type: String, default: \"hover\" },\n  };\n\n  connect() {\n    this.closeTimeout = null;\n    this.cleanup = null;\n    this.addEventListeners();\n  }\n\n  disconnect() {\n    this.removeEventListeners();\n    if (this.cleanup) {\n      this.cleanup();\n    }\n  }\n\n  addEventListeners() {\n    if (this.triggerValue === \"hover\") {\n      this.triggerTarget.addEventListener(\"mouseenter\", this.handleMouseEnter);\n      this.triggerTarget.addEventListener(\"mouseleave\", this.handleMouseLeave);\n      this.contentTarget.addEventListener(\"mouseenter\", this.handleMouseEnter);\n      this.contentTarget.addEventListener(\"mouseleave\", this.handleMouseLeave);\n    } else if (this.triggerValue === \"click\") {\n      this.triggerTarget.addEventListener(\"click\", this.handleClick);\n      document.addEventListener(\"click\", this.handleOutsideClick);\n    }\n  }\n\n  removeEventListeners() {\n    this.triggerTarget.removeEventListener(\"mouseenter\", this.handleMouseEnter);\n    this.triggerTarget.removeEventListener(\"mouseleave\", this.handleMouseLeave);\n    this.contentTarget.removeEventListener(\"mouseenter\", this.handleMouseEnter);\n    this.contentTarget.removeEventListener(\"mouseleave\", this.handleMouseLeave);\n    this.triggerTarget.removeEventListener(\"click\", this.handleClick);\n    document.removeEventListener(\"click\", this.handleOutsideClick);\n  }\n\n  handleMouseEnter = () => {\n    clearTimeout(this.closeTimeout);\n    this.openValue = true;\n    this.showPopover();\n  };\n\n  handleMouseLeave = () => {\n    this.closeTimeout = setTimeout(() => {\n      this.openValue = false;\n      this.hidePopover();\n    }, 100);\n  };\n\n  handleClick = (event) => {\n    event.stopPropagation();\n    this.openValue = !this.openValue;\n    this.openValue ? this.showPopover() : this.hidePopover();\n  };\n\n  handleOutsideClick = (event) => {\n    if (!this.element.contains(event.target) && this.openValue) {\n      this.openValue = false;\n      this.hidePopover();\n    }\n  };\n\n  showPopover() {\n    this.contentTarget.classList.remove(\"hidden\");\n    this.updatePosition();\n  }\n\n  hidePopover() {\n    this.contentTarget.classList.add(\"hidden\");\n    if (this.cleanup) {\n      this.cleanup();\n    }\n  }\n\n  updatePosition() {\n    if (this.cleanup) {\n      this.cleanup();\n    }\n\n    this.cleanup = autoUpdate(this.triggerTarget, this.contentTarget, () => {\n      computePosition(this.triggerTarget, this.contentTarget, {\n        placement: this.optionsValue.placement || \"bottom\",\n        middleware: [flip(), shift(), offset(8)],\n      }).then(({ x, y }) => {\n        Object.assign(this.contentTarget.style, {\n          left: `${x}px`,\n          top: `${y}px`,\n        });\n      });\n    });\n  }\n}\n"
+          "content": "import { Controller } from \"@hotwired/stimulus\";\nimport {\n  computePosition,\n  flip,\n  shift,\n  offset,\n  autoUpdate,\n} from \"@floating-ui/dom\";\n\nexport default class extends Controller {\n  static targets = [\"trigger\", \"content\"];\n  static values = {\n    open: { type: Boolean, default: false },\n    options: { type: Object, default: {} },\n    trigger: { type: String, default: \"hover\" },\n  };\n\n  connect() {\n    this.closeTimeout = null;\n    this.cleanup = null;\n    this.addEventListeners();\n  }\n\n  disconnect() {\n    this.removeEventListeners();\n    clearTimeout(this.closeTimeout);\n    document.removeEventListener(\"keydown\", this.handleKeydown);\n    if (this.cleanup) {\n      this.cleanup();\n      this.cleanup = null;\n    }\n  }\n\n  addEventListeners() {\n    if (this.triggerValue === \"hover\") {\n      this.triggerTarget.addEventListener(\"mouseenter\", this.handleMouseEnter);\n      this.triggerTarget.addEventListener(\"mouseleave\", this.handleMouseLeave);\n      this.contentTarget.addEventListener(\"mouseenter\", this.handleMouseEnter);\n      this.contentTarget.addEventListener(\"mouseleave\", this.handleMouseLeave);\n    } else if (this.triggerValue === \"click\") {\n      this.triggerTarget.addEventListener(\"click\", this.handleClick);\n      document.addEventListener(\"click\", this.handleOutsideClick);\n    }\n  }\n\n  removeEventListeners() {\n    this.triggerTarget.removeEventListener(\"mouseenter\", this.handleMouseEnter);\n    this.triggerTarget.removeEventListener(\"mouseleave\", this.handleMouseLeave);\n    this.contentTarget.removeEventListener(\"mouseenter\", this.handleMouseEnter);\n    this.contentTarget.removeEventListener(\"mouseleave\", this.handleMouseLeave);\n    this.triggerTarget.removeEventListener(\"click\", this.handleClick);\n    document.removeEventListener(\"click\", this.handleOutsideClick);\n  }\n\n  handleMouseEnter = () => {\n    clearTimeout(this.closeTimeout);\n    this.openValue = true;\n    this.showPopover();\n  };\n\n  handleMouseLeave = () => {\n    this.closeTimeout = setTimeout(() => {\n      this.openValue = false;\n      this.hidePopover();\n    }, 100);\n  };\n\n  handleClick = (event) => {\n    event.stopPropagation();\n    this.openValue = !this.openValue;\n    this.openValue ? this.showPopover() : this.hidePopover();\n  };\n\n  handleOutsideClick = (event) => {\n    if (!this.element.contains(event.target) && this.openValue) {\n      this.openValue = false;\n      this.hidePopover();\n    }\n  };\n\n  handleKeydown = (event) => {\n    if (event.key !== \"Escape\") return;\n    if (!this.openValue) return;\n\n    clearTimeout(this.closeTimeout);\n    this.openValue = false;\n    this.hidePopover();\n  };\n\n  showPopover() {\n    this.contentTarget.classList.remove(\"hidden\");\n    this.contentTarget.dataset.state = \"open\";\n    document.addEventListener(\"keydown\", this.handleKeydown);\n    this.updatePosition();\n  }\n\n  hidePopover() {\n    this.contentTarget.classList.add(\"hidden\");\n    this.contentTarget.dataset.state = \"closed\";\n    document.removeEventListener(\"keydown\", this.handleKeydown);\n    if (this.cleanup) {\n      this.cleanup();\n      this.cleanup = null;\n    }\n  }\n\n  updatePosition() {\n    if (this.cleanup) {\n      this.cleanup();\n    }\n\n    this.cleanup = autoUpdate(this.triggerTarget, this.contentTarget, () => {\n      computePosition(this.triggerTarget, this.contentTarget, {\n        placement: this.optionsValue.placement || \"bottom\",\n        middleware: [flip(), shift(), offset(8)],\n      }).then(({ x, y, placement }) => {\n        Object.assign(this.contentTarget.style, {\n          left: `${x}px`,\n          top: `${y}px`,\n        });\n        // flip() can resolve to the opposite side of the requested placement,\n        // so the directional slide-in classes must follow the resolved value.\n        this.contentTarget.dataset.side = placement.split(\"-\")[0];\n      });\n    });\n  }\n}\n"
         },
         {
           "path": "popover_trigger.rb",