Merge pull request #832 from PR0M3TH3AN/codex/run-unit-tests-including-video-modal-tags

Fix zap dialog close race in video modal

js/app.js

@@ -7420,11 +7420,35 @@ class Application {
       magnet: trimmedMagnet,
     });
 
-    if (
+    const modalVideoIsConnected = (() => {
+      if (!this.modalVideo) {
+        return false;
+      }
+      if (typeof this.modalVideo.isConnected === "boolean") {
+        return this.modalVideo.isConnected;
+      }
+      const ownerDocument = this.modalVideo.ownerDocument ||
+        (typeof document !== "undefined" ? document : null);
+      if (ownerDocument?.contains) {
+        try {
+          return ownerDocument.contains(this.modalVideo);
+        } catch (error) {
+          devLogger.warn(
+            "[playVideoWithFallback] Failed to determine modal video connection state",
+            error,
+          );
+        }
+      }
+      return true;
+    })();
+
+    const shouldReuseActiveSession =
+      modalVideoIsConnected &&
       this.activePlaybackSession &&
       typeof this.activePlaybackSession.matchesRequestSignature === "function" &&
-      this.activePlaybackSession.matchesRequestSignature(requestSignature)
-    ) {
+      this.activePlaybackSession.matchesRequestSignature(requestSignature);
+
+    if (shouldReuseActiveSession) {
       this.log(
         "[playVideoWithFallback] Duplicate playback request detected; reusing active session."
       );

js/ui/components/VideoModal.js

@@ -601,6 +601,7 @@ export class VideoModal {
 
         if (this.modalZapOpenPromise) {
           this.modalZapPendingToggle = "close";
+          this.closeZapDialog({ silent: true, restoreFocus: false });
           return;
         }
 
@@ -774,7 +775,10 @@ export class VideoModal {
         const { silent = false, ...rest } = options;
         const wasOpen = popover.isOpen?.() === true;
         const result = originalClose(rest);
-        if (wasOpen && result) {
+        const wasDialogMarkedOpen =
+          this.modalZapDialogOpen === true ||
+          this.modalZapDialog?.dataset?.state === "open";
+        if (wasOpen || wasDialogMarkedOpen) {
           this.modalZapPendingToggle = null;
           this.modalZapOpenPromise = null;
           this.modalZapDialog.dataset.state = "closed";
@@ -784,7 +788,7 @@ export class VideoModal {
           if (this.modalZapBtn) {
             this.modalZapBtn.setAttribute("aria-expanded", "false");
           }
-          if (!silent) {
+          if (!silent && (wasOpen || wasDialogMarkedOpen)) {
             this.dispatch("zap:close", { video: this.activeVideo });
           }
         }
@@ -1610,7 +1614,21 @@ export class VideoModal {
             this.modalZapDialog.setAttribute("aria-hidden", "false");
           }
           this.focusZapAmount();
+          return true;
         }
+
+        if (this.modalZapDialog) {
+          this.modalZapDialog.hidden = false;
+          this.modalZapDialog.dataset.state = "open";
+          this.modalZapDialog.setAttribute("aria-hidden", "false");
+          this.modalZapDialogOpen = true;
+          if (this.modalZapBtn) {
+            this.modalZapBtn.setAttribute("aria-expanded", "true");
+          }
+          this.focusZapAmount();
+          return true;
+        }
+
         return opened;
       }
 
@@ -1647,6 +1665,13 @@ export class VideoModal {
   }
 
   closeZapDialog({ silent = false, restoreFocus } = {}) {
+    const dialogAppearsOpen = this.isZapDialogOpen();
+
+    if (this.modalZapOpenPromise && !dialogAppearsOpen) {
+      this.modalZapPendingToggle = "close";
+      return;
+    }
+
     this.modalZapPendingToggle = null;
     if (this.modalZapPopover?.close) {
       const options = { silent };

js/ui/components/tagPillList.js

@@ -37,7 +37,7 @@ function createTagButton({ doc, tag, onTagActivate }) {
   const icon = doc.createElement("span");
   icon.className = "video-tag-pill__icon";
   icon.setAttribute("aria-hidden", "true");
-  icon.textContent = "+";
+  icon.dataset.icon = "plus";
 
   button.append(label, icon);
 

js/ui/overlay/popoverEngine.js

@@ -266,7 +266,11 @@ export function createPopover(trigger, render, options = {}) {
     }
 
     const restoreScroll = () => {
-      view.scrollTo(scrollX, scrollY);
+      try {
+        view.scrollTo(scrollX, scrollY);
+      } catch (error) {
+        logger.dev?.warn?.("[popover] scroll restoration failed", error);
+      }
     };
 
     restoreScroll();

tests/ui/tag-pill-list.test.mjs

@@ -32,7 +32,7 @@ test("renderTagPillStrip builds buttons with labels and icons", () => {
 
     const icon = button.querySelector(".video-tag-pill__icon");
     assert(icon, "icon span should be present");
-    assert.equal(icon.textContent, "+");
+    assert.equal(icon.dataset.icon, "plus");
     assert.equal(icon.getAttribute("aria-hidden"), "true");
   }
 });

tests/video-modal-accessibility.test.mjs

@@ -37,10 +37,83 @@ export async function setupModal({ lazyLoad = false } = {}) {
   globalThis.KeyboardEvent = window.KeyboardEvent;
   globalThis.MouseEvent = window.MouseEvent;
   const hadWebSocket = typeof globalThis.WebSocket !== "undefined";
-  if (typeof window.scrollTo !== "function") {
-    window.scrollTo = () => {};
+  const noopScrollTo = () => {};
+  const windowPrototype = Object.getPrototypeOf(window) || null;
+  const prototypeDescriptor = windowPrototype
+    ? Object.getOwnPropertyDescriptor(windowPrototype, "scrollTo")
+    : null;
+  const hadPrototypeScrollTo = !!prototypeDescriptor;
+  let restorePrototypeScrollTo = () => {};
+  if (windowPrototype) {
+    try {
+      Object.defineProperty(windowPrototype, "scrollTo", {
+        configurable: true,
+        enumerable: false,
+        writable: true,
+        value: noopScrollTo,
+      });
+      restorePrototypeScrollTo = () => {
+        if (hadPrototypeScrollTo) {
+          Object.defineProperty(windowPrototype, "scrollTo", prototypeDescriptor);
+        } else {
+          delete windowPrototype.scrollTo;
+        }
+      };
+    } catch (error) {
+      restorePrototypeScrollTo = () => {
+        if (hadPrototypeScrollTo) {
+          try {
+            Object.defineProperty(windowPrototype, "scrollTo", prototypeDescriptor);
+          } catch (restoreError) {
+            windowPrototype.scrollTo = prototypeDescriptor?.value;
+          }
+        } else {
+          delete windowPrototype.scrollTo;
+        }
+      };
+      try {
+        windowPrototype.scrollTo = noopScrollTo;
+      } catch (assignError) {
+        // ignore inability to override prototype; relying on own property override
+      }
+    }
   }
-  globalThis.scrollTo = window.scrollTo;
+  const hadOwnScrollTo = Object.prototype.hasOwnProperty.call(window, "scrollTo");
+  const originalWindowScrollTo = window.scrollTo;
+  try {
+    Object.defineProperty(window, "scrollTo", {
+      configurable: true,
+      enumerable: false,
+      writable: true,
+      value: noopScrollTo,
+    });
+  } catch (error) {
+    window.scrollTo = noopScrollTo;
+  }
+  const originalGlobalScrollTo = globalThis.scrollTo;
+  globalThis.scrollTo = noopScrollTo;
+  const restoreScrollTo = () => {
+    restorePrototypeScrollTo();
+    if (hadOwnScrollTo) {
+      try {
+        Object.defineProperty(window, "scrollTo", {
+          configurable: true,
+          enumerable: false,
+          writable: true,
+          value: originalWindowScrollTo,
+        });
+      } catch (error) {
+        window.scrollTo = originalWindowScrollTo;
+      }
+    } else {
+      delete window.scrollTo;
+    }
+    if (originalGlobalScrollTo !== undefined) {
+      globalThis.scrollTo = originalGlobalScrollTo;
+    } else {
+      delete globalThis.scrollTo;
+    }
+  };
   if (!hadWebSocket) {
     globalThis.WebSocket = class {
       constructor() {}
@@ -133,7 +206,7 @@ export async function setupModal({ lazyLoad = false } = {}) {
     delete globalThis.location;
     delete globalThis.KeyboardEvent;
     delete globalThis.MouseEvent;
-    delete globalThis.scrollTo;
+    restoreScrollTo();
     if (!hadWebSocket) {
       delete globalThis.WebSocket;
     }
@@ -321,10 +394,83 @@ async function setupPlaybackHarness() {
   globalThis.MouseEvent = window.MouseEvent;
   globalThis.self = window;
   const hadHarnessWebSocket = typeof globalThis.WebSocket !== "undefined";
-  if (typeof window.scrollTo !== "function") {
-    window.scrollTo = () => {};
+  const noopScrollTo = () => {};
+  const windowPrototype = Object.getPrototypeOf(window) || null;
+  const prototypeDescriptor = windowPrototype
+    ? Object.getOwnPropertyDescriptor(windowPrototype, "scrollTo")
+    : null;
+  const hadPrototypeScrollTo = !!prototypeDescriptor;
+  let restorePrototypeScrollTo = () => {};
+  if (windowPrototype) {
+    try {
+      Object.defineProperty(windowPrototype, "scrollTo", {
+        configurable: true,
+        enumerable: false,
+        writable: true,
+        value: noopScrollTo,
+      });
+      restorePrototypeScrollTo = () => {
+        if (hadPrototypeScrollTo) {
+          Object.defineProperty(windowPrototype, "scrollTo", prototypeDescriptor);
+        } else {
+          delete windowPrototype.scrollTo;
+        }
+      };
+    } catch (error) {
+      restorePrototypeScrollTo = () => {
+        if (hadPrototypeScrollTo) {
+          try {
+            Object.defineProperty(windowPrototype, "scrollTo", prototypeDescriptor);
+          } catch (restoreError) {
+            windowPrototype.scrollTo = prototypeDescriptor?.value;
+          }
+        } else {
+          delete windowPrototype.scrollTo;
+        }
+      };
+      try {
+        windowPrototype.scrollTo = noopScrollTo;
+      } catch (assignError) {
+        // ignore inability to override prototype; relying on own property override
+      }
+    }
   }
-  globalThis.scrollTo = window.scrollTo;
+  const hadOwnScrollTo = Object.prototype.hasOwnProperty.call(window, "scrollTo");
+  const originalWindowScrollTo = window.scrollTo;
+  try {
+    Object.defineProperty(window, "scrollTo", {
+      configurable: true,
+      enumerable: false,
+      writable: true,
+      value: noopScrollTo,
+    });
+  } catch (error) {
+    window.scrollTo = noopScrollTo;
+  }
+  const originalGlobalScrollTo = globalThis.scrollTo;
+  globalThis.scrollTo = noopScrollTo;
+  const restoreScrollTo = () => {
+    restorePrototypeScrollTo();
+    if (hadOwnScrollTo) {
+      try {
+        Object.defineProperty(window, "scrollTo", {
+          configurable: true,
+          enumerable: false,
+          writable: true,
+          value: originalWindowScrollTo,
+        });
+      } catch (error) {
+        window.scrollTo = originalWindowScrollTo;
+      }
+    } else {
+      delete window.scrollTo;
+    }
+    if (originalGlobalScrollTo !== undefined) {
+      globalThis.scrollTo = originalGlobalScrollTo;
+    } else {
+      delete globalThis.scrollTo;
+    }
+  };
   if (!hadHarnessWebSocket) {
     globalThis.WebSocket = class {
       constructor() {}
@@ -415,7 +561,7 @@ async function setupPlaybackHarness() {
     delete globalThis.KeyboardEvent;
     delete globalThis.MouseEvent;
     delete globalThis.self;
-    delete globalThis.scrollTo;
+    restoreScrollTo();
     if (!hadHarnessWebSocket) {
       delete globalThis.WebSocket;
     }