Unify initialization procedure of dialogs (#1835)

Resolves #1773 This PR replaces the `overlay-toggled` event with two separate `overlay-shown`/`overlay-hidden` events and fires them into the slotted (dialog) element which allows slotted custom components to initialize themselves when the dialog is shown. For example: ```javascript connectedCallback() { // ... this.addEventListener("overlay-shown", () => { this._state = this._states.INITIALIZING; }); } ``` <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1835"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
tiny-pilot · Aug 14, 2024 · 67f1c4d · 67f1c4d
1 parent e485124
commit 67f1c4d
Show file tree

Hide file tree

Showing 10 changed files with 68 additions and 64 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -352,7 +352,7 @@ class extends HTMLElement {
 The class property `_states` can then be used in the JavaScript component code:
 
 ```javascript
-this._state = this._states.FETCH_FROM_URL;
+this._state = this._states.INITIALIZING;
 ```
 
 We then use CSS rules based on the `state` attribute to control the component's appearance:
@@ -388,6 +388,16 @@ This ensures that the elements in the `<div id="initializing">` only appear when
 
 Prefer to change a web component's appearance based on attributes and CSS rules as opposed to JavaScript that manipulates the `.style` attributes of elements within the component.
 
+We can then initialize the component when the dialog is opened by listening for the `overlay-shown` event:
+
+```javascript
+connectedCallback() {
+  this.addEventListener("overlay-shown", () => {
+    this._state = this._states.INITIALIZING);
+  };
+}
+```
+
 ### Disable closing a dialog
 
 For a component that is used within an overlay, there might be certain states that should prevent the user from closing the dialog. That’s typically the case when we are waiting for an action to complete (for example when loading something).

diff --git a/app/static/js/app.js b/app/static/js/app.js
@@ -230,8 +230,11 @@ document.onload = document.getElementById("app").focus();
 
 document.addEventListener("keydown", onKeyDown);
 document.addEventListener("keyup", onKeyUp);
-document.addEventListener("overlay-toggled", (evt) => {
-  overlayTracker.trackStatus(evt.target, evt.detail.isShown);
+document.addEventListener("overlay-shown", (evt) => {
+  overlayTracker.trackStatus(evt.detail.overlay, /*isShown=*/ true);
+});
+document.addEventListener("overlay-hidden", (evt) => {
+  overlayTracker.trackStatus(evt.detail.overlay, /*isShown=*/ false);
 });
 document.addEventListener("video-streaming-mode-changed", (evt) => {
   document.getElementById("status-bar").videoStreamIndicator.mode =
@@ -314,35 +317,21 @@ menuBar.addEventListener("update-dialog-requested", () => {
   document.getElementById("update-dialog").checkVersion();
 });
 menuBar.addEventListener("change-hostname-dialog-requested", () => {
-  // Note: we have to call `initialize()` after `show()`, to ensure that the
-  // dialog is able to focus the main input element.
-  // See https://github.com/tiny-pilot/tinypilot/issues/1770
   document.getElementById("change-hostname-overlay").show();
-  document.getElementById("change-hostname-dialog").initialize();
 });
 menuBar.addEventListener("wifi-dialog-requested", () => {
-  // Note: we have to call `initialize()` after `show()`, to ensure that the
-  // dialog is able to focus the main input element.
-  // See https://github.com/tiny-pilot/tinypilot/issues/1770
   document.getElementById("wifi-overlay").show();
-  document.getElementById("wifi-dialog").initialize();
 });
 menuBar.addEventListener("network-status-dialog-requested", () => {
-  // Note: we have to call `initialize()` after `show()`, to ensure that the
-  // dialog is able to focus the main input element.
-  // See https://github.com/tiny-pilot/tinypilot/issues/1770
   document.getElementById("network-status-overlay").show();
-  document.getElementById("network-status-dialog").initialize();
 });
 menuBar.addEventListener("fullscreen-requested", () => {
   document.getElementById("remote-screen").fullscreen = true;
 });
 menuBar.addEventListener("debug-logs-dialog-requested", () => {
-  document.getElementById("debug-dialog").retrieveLogs();
   document.getElementById("debug-overlay").show();
 });
 menuBar.addEventListener("about-dialog-requested", () => {
-  document.getElementById("about-dialog").initialize();
   document.getElementById("about-overlay").show();
 });
 menuBar.addEventListener("mass-storage-dialog-requested", () => {
@@ -355,15 +344,10 @@ menuBar.addEventListener("static-ip-dialog-requested", () => {
   document.getElementById("feature-pro-overlay").show();
 });
 menuBar.addEventListener("video-settings-dialog-requested", () => {
-  document.getElementById("video-settings-dialog").initialize();
   document.getElementById("video-settings-overlay").show();
 });
 menuBar.addEventListener("paste-dialog-requested", () => {
-  // Note: we have to call `initialize()` after `show()`, to ensure that the
-  // dialog is able to focus the main input element.
-  // See https://github.com/tiny-pilot/tinypilot/issues/1770
   document.getElementById("paste-overlay").show();
-  document.getElementById("paste-dialog").initialize();
 });
 menuBar.addEventListener("ctrl-alt-del-requested", () => {
   // Even though only the final keystroke matters, send them one at a time to

diff --git a/app/templates/custom-elements/about-dialog.html b/app/templates/custom-elements/about-dialog.html
@@ -82,14 +82,15 @@ <h3>About TinyPilot</h3>
           this.attachShadow({ mode: "open" }).appendChild(
             template.content.cloneNode(true)
           );
+          this.addEventListener("overlay-shown", () => this._initialize());
           this.shadowRoot
             .querySelector(".close-btn")
             .addEventListener("click", () =>
               this.dispatchEvent(new DialogClosedEvent())
             );
         }
 
-        initialize() {
+        _initialize() {
           this._checkVersion();
           this._populateCredits();
         }

diff --git a/app/templates/custom-elements/change-hostname-dialog.html b/app/templates/custom-elements/change-hostname-dialog.html
@@ -125,6 +125,7 @@ <h3>Changing Hostname</h3>
             futureLocation: this.shadowRoot.getElementById("future-location"),
           };
 
+          this.addEventListener("overlay-shown", () => this._initialize());
           this._elements.hostnameInput.addEventListener("input", () => {
             this._onInputChanged();
           });
@@ -162,7 +163,7 @@ <h3>Changing Hostname</h3>
           this.setAttribute("initial-hostname", initialHostname);
         }
 
-        initialize() {
+        _initialize() {
           this._elements.inputError.hide();
           this._state = this._states.INITIALIZING;
           determineHostname()

diff --git a/app/templates/custom-elements/debug-dialog.html b/app/templates/custom-elements/debug-dialog.html
@@ -82,9 +82,6 @@ <h3>Debug Logs</h3>
         constructor() {
           super();
           this.attachShadow({ mode: "open" });
-          // Ensure that these methods always refer to the correct "this",
-          // regardless of where they are called.
-          this.retrieveLogs = this.retrieveLogs.bind(this);
         }
 
         connectedCallback() {
@@ -106,6 +103,8 @@ <h3>Debug Logs</h3>
             shareLogsButton:
               this.shadowRoot.querySelector("#share-logs-button"),
           };
+
+          this.addEventListener("overlay-shown", () => this._initialize());
           this._elements.includeSensitiveData.addEventListener(
             "input",
             (event) => {
@@ -145,7 +144,7 @@ <h3>Debug Logs</h3>
           return this.hasAttribute("include-sensitive-data");
         }
 
-        retrieveLogs() {
+        _initialize() {
           this._state = this._states.LOGS_LOADING;
           getDebugLogs()
             .then((text) => {

diff --git a/app/templates/custom-elements/network-status-dialog.html b/app/templates/custom-elements/network-status-dialog.html
@@ -66,21 +66,20 @@ <h3>Network Status</h3>
           };
           this._shouldAutoUpdate = false;
           this._updateTicker = null;
+
+          this.addEventListener("overlay-shown", () => this._initialize());
+          this.addEventListener("overlay-hidden", () => {
+            // Stop the update ticker when the dialog is closed, otherwise the
+            // status requests would continue to be fired even when the dialog
+            // is not visible anymore.
+            this._shouldAutoUpdate = false;
+            clearTimeout(this._updateTicker);
+          });
           this.shadowRoot
             .querySelector("#close-button")
             .addEventListener("click", () => {
               this.dispatchEvent(new DialogClosedEvent());
             });
-
-          // For all events that terminate the dialog, make sure to stop the
-          // update ticker, otherwise the status requests would continue to be
-          // fired even when the dialog is not visible anymore.
-          ["dialog-closed", "dialog-failed"].forEach((evtName) => {
-            this.addEventListener(evtName, () => {
-              this._shouldAutoUpdate = false;
-              clearTimeout(this._updateTicker);
-            });
-          });
         }
 
         get _state() {
@@ -96,7 +95,7 @@ <h3>Network Status</h3>
           );
         }
 
-        async initialize() {
+        async _initialize() {
           this._state = this._states.INITIALIZING;
           await this._update();
           this._state = this._states.DISPLAY;

diff --git a/app/templates/custom-elements/overlay-panel.html b/app/templates/custom-elements/overlay-panel.html
@@ -66,8 +66,6 @@
 </template>
 
 <script type="module">
-  import { DialogClosedEvent } from "/js/events.js";
-
   (function () {
     const template = document.querySelector("#overlay-panel-template");
 
@@ -101,21 +99,9 @@
               }
             }
           );
-          // Dispatch dialog-closed event into slotted element(s), to allow the
-          // dialog to react to it if need be (e.g., for performing clean-ups).
-          // In theory, there could be multiple elements in the slot, so they
-          // all need to receive the close event – hence the `forEach`. In
-          // reality, we usually only slot a single dialog element, though. It
-          // shouldn’t matter anyway, since the dialog-closed event handler is
-          // idempotent.
           this.shadowRoot
             .querySelector("#close-button")
-            .addEventListener("click", () =>
-              this.shadowRoot
-                .querySelector("#dialog-slot")
-                .assignedElements()
-                .forEach((el) => el.dispatchEvent(new DialogClosedEvent()))
-            );
+            .addEventListener("click", () => this.show(false));
           // Prevent auto-close behavior of native <dialog> if the user presses
           // the ESC key.
           this._elements.dialog.addEventListener("cancel", (evt) => {
@@ -126,21 +112,40 @@
         show(isShown = true) {
           if (isShown) {
             this._elements.dialog.showModal();
+            this._injectEvent(
+              new CustomEvent("overlay-shown", {
+                detail: { overlay: this },
+                bubbles: true,
+                composed: true,
+              })
+            );
           } else {
             this._elements.dialog.close();
+            this._injectEvent(
+              new CustomEvent("overlay-hidden", {
+                detail: { overlay: this },
+                bubbles: true,
+                composed: true,
+              })
+            );
           }
-          this.dispatchEvent(
-            new CustomEvent("overlay-toggled", {
-              detail: { isShown },
-              bubbles: true,
-              composed: true,
-            })
-          );
         }
 
         isShown() {
           return this._elements.dialog.open;
         }
+
+        // Dispatch an event into the slotted element(s), to allow the dialog to
+        // react to it if need be (e.g., for performing initializations/
+        // clean-ups). In theory, there could be multiple elements in the slot,
+        // so they all need to receive the event – hence the `forEach`. In
+        // reality, we usually only slot a single dialog element, though.
+        _injectEvent(event) {
+          this.shadowRoot
+            .querySelector("#dialog-slot")
+            .assignedElements()
+            .forEach((el) => el.dispatchEvent(event));
+        }
       }
     );
   })();

diff --git a/app/templates/custom-elements/paste-dialog.html b/app/templates/custom-elements/paste-dialog.html
@@ -93,6 +93,8 @@ <h3>Paste Text</h3>
             cancelButton: this.shadowRoot.querySelector("#cancel-btn"),
             maskInputButton: this.shadowRoot.querySelector("#mask-input"),
           };
+
+          this.addEventListener("overlay-shown", () => this._initialize());
           this._elements.pasteArea.addEventListener("input", () =>
             this._elements.confirmButton.toggleAttribute(
               "disabled",
@@ -118,7 +120,7 @@ <h3>Paste Text</h3>
           });
         }
 
-        initialize() {
+        _initialize() {
           this.toggleAttribute("mask-input", isPasteAreaMasked());
           this._elements.maskInputButton.checked = isPasteAreaMasked();
           this._elements.pasteArea.value = "";

diff --git a/app/templates/custom-elements/video-settings-dialog.html b/app/templates/custom-elements/video-settings-dialog.html
@@ -273,6 +273,8 @@ <h3>Applying Video Settings</h3>
               "#stun-validation-error"
             ),
           };
+
+          this.addEventListener("overlay-shown", () => this._initialize());
           this._elements.closeButton.addEventListener("click", () =>
             this.dispatchEvent(new DialogClosedEvent())
           );
@@ -362,7 +364,7 @@ <h3>Applying Video Settings</h3>
           );
         }
 
-        initialize() {
+        _initialize() {
           this._state = this._states.LOADING;
 
           // Reset all transient view state.

diff --git a/app/templates/custom-elements/wifi-dialog.html b/app/templates/custom-elements/wifi-dialog.html
@@ -201,6 +201,7 @@ <h3>Wi-Fi Credentials Removed</h3>
             disableButton: this.shadowRoot.querySelector("#disable-button"),
           };
 
+          this.addEventListener("overlay-shown", () => this._initialize());
           [
             this._elements.ssidInput,
             this._elements.pskInput,
@@ -243,7 +244,7 @@ <h3>Wi-Fi Credentials Removed</h3>
           );
         }
 
-        async initialize() {
+        async _initialize() {
           this._state = this._states.INITIALIZING;
           let wifiSettings, networkStatus;
           try {