From 78d2ddb9b771d1110ba78a02a4e3cd14325230fd Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 22 Nov 2023 14:59:18 +1100
Subject: [PATCH] Close popout when focus outside

I'm starting to think that these stimulus tests are worthless. The environment is not the same as a browser, which creates extra work to deal with the inconsistencies. And it means we're not testing real world behaviour.

So these are just unit tests, but they take extra effort to put together due to the inter-relatedness with the DOM. Hmm.
---
 .../controllers/popout_controller.js          | 20 +++++++++++
 .../stimulus/popout_controller_test.js        | 33 ++++++++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/app/webpacker/controllers/popout_controller.js b/app/webpacker/controllers/popout_controller.js
index 06c5156ce4..a2fac4e58e 100644
--- a/app/webpacker/controllers/popout_controller.js
+++ b/app/webpacker/controllers/popout_controller.js
@@ -10,6 +10,20 @@ export default class PopoutController extends Controller {
     // Show when click or down-arrow on button
     this.buttonTarget.addEventListener("click", this.show.bind(this));
     this.buttonTarget.addEventListener("keydown", this.showIfDownArrow.bind(this));
+
+    // Close when click or tab outside of dialog. Run async (don't block primary event handlers).
+    this.closeIfOutsideBound = this.closeIfOutside.bind(this); // Store reference for removing listeners later.
+    document.addEventListener("click", this.closeIfOutsideBound, { passive: true });
+    document.addEventListener("focusin", this.closeIfOutsideBound, { passive: true });
+  }
+
+  disconnect() {
+    // Clean up handlers registered outside the controller element.
+    // (jest cleans up document too early)
+    if (document) {
+      document.removeEventListener("click", this.closeIfOutsideBound);
+      document.removeEventListener("focusin", this.closeIfOutsideBound);
+    }
   }
 
   show(e) {
@@ -23,4 +37,10 @@ export default class PopoutController extends Controller {
       this.show(e);
     }
   }
+
+  closeIfOutside(e) {
+    if (!this.dialogTarget.contains(e.target)) {
+      this.dialogTarget.style.display = "none";
+    }
+  }
 }
diff --git a/spec/javascripts/stimulus/popout_controller_test.js b/spec/javascripts/stimulus/popout_controller_test.js
index f3f332143d..59708a8e46 100644
--- a/spec/javascripts/stimulus/popout_controller_test.js
+++ b/spec/javascripts/stimulus/popout_controller_test.js
@@ -20,16 +20,19 @@ describe("PopoutController", () => {
           <input id="input2">
         </div>
       </div>
+      <input id="input3">
     `;
 
     const button = document.getElementById("button");
     const input1 = document.getElementById("input1");
     const input2 = document.getElementById("input2");
+    const input3 = document.getElementById("input3");
   });
 
   describe("Show", () => {
     it("shows the dialog on click", () => {
-      button.click();
+      // button.click(); // For some reason this fails due to passive: true, but works in real life.
+      button.dispatchEvent(new Event("click"));
 
       expect(dialog.style.display).toBe("block"); // visible
     });
@@ -46,4 +49,32 @@ describe("PopoutController", () => {
       expect(dialog.style.display).toBe("none"); // not visible
     });
   });
+
+  describe("Close", () => {
+    beforeEach(() => {
+      button.dispatchEvent(new Event("click")); // Dialog is open
+    })
+
+    it("closes the dialog when click outside", () => {
+      input3.click();
+
+      expect(dialog.style.display).toBe("none"); // not visible
+    });
+
+    it("closes the dialog when focusing another field (eg with tab)", () => {
+      input3.focus();
+
+      expect(dialog.style.display).toBe("none"); // not visible
+    });
+
+    it("doesn't close the dialog when focusing internal field", () => {
+      input2.focus();
+
+      expect(dialog.style.display).toBe("block"); // visible
+    });
+  });
+
+  describe("Cleaning up", () => {
+    // unable to test disconnect
+  });
 });