Fix: Rename CloseWatcher.signalClose to CloseWatched.requestClose (#4…

…0213) * 📦 Update dependency chromedriver to v131 * switch to realWin to resolve scheme-less url to the correct hostname something changed recently with Url Parsing in Chrome or something in the FakeWin implementation is causign it to not resolve properly. Quick fix seems to be to switch it to a real window * try out the disable feature * rename to requestClose * undo skip to test which tests are still failing * add back skip * try w/o closewatcher * Update test/unit/utils/test-close-watcher-impl.js Co-authored-by: Daniel Rozenberg <me@danielrozenberg.com> * Update test/unit/utils/test-close-watcher-impl.js Co-authored-by: Daniel Rozenberg <me@danielrozenberg.com> * Update test/unit/utils/test-close-watcher-impl.js Co-authored-by: Daniel Rozenberg <me@danielrozenberg.com> * Update test/unit/utils/test-close-watcher-impl.js Co-authored-by: Daniel Rozenberg <me@danielrozenberg.com> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Daniel Rozenberg <me@danielrozenberg.com>
ampproject · Jan 7, 2025 · 37ffbde · 37ffbde
1 parent 585c0da
commit 37ffbde
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 45 deletions.
diff --git a/extensions/amp-lightbox/0.1/test/test-amp-lightbox.js b/extensions/amp-lightbox/0.1/test/test-amp-lightbox.js
@@ -114,7 +114,7 @@ describes.realWin(
       const setupCloseSpy = env.sandbox.spy(impl, 'close');
 
       await impl.open_({caller: sourceElement});
-      impl.closeWatcher_.signalClosed();
+      impl.closeWatcher_.requestClose();
       expect(setupCloseSpy).to.be.called;
     });
 

diff --git a/src/utils/close-watcher-impl.js b/src/utils/close-watcher-impl.js
@@ -67,11 +67,11 @@ export class CloseWatcherImpl {
 
   /**
    * Signals to the close watcher to close the modal.
-   * See `CloseWatcher.signalClosed`.
+   * See `CloseWatcher.requestClose`.
    */
-  signalClosed() {
+  requestClose() {
     if (this.watcher_) {
-      this.watcher_.signalClosed();
+      this.watcher_.requestClose();
     } else if (this.handler_) {
       const handler = this.handler_;
       handler();
@@ -106,7 +106,7 @@ export class CloseWatcherImpl {
   closeOnEscape_(event) {
     if (event.key == Keys_Enum.ESCAPE) {
       event.preventDefault();
-      this.signalClosed();
+      this.requestClose();
     }
   }
 }
diff --git a/src/utils/close-watcher.extern.js b/src/utils/close-watcher.extern.js
@@ -12,7 +12,7 @@ function CloseWatcher() {}
 
 CloseWatcher.prototype.destroy = function () {};
 
-CloseWatcher.prototype.signalClosed = function () {};
+CloseWatcher.prototype.requestClose = function () {};
 
 /** @type {?function(!Event)} */
 CloseWatcher.prototype.onclose;

diff --git a/test/unit/utils/test-close-watcher-impl.js b/test/unit/utils/test-close-watcher-impl.js
@@ -24,46 +24,57 @@ describes.realWin('#CloseWatcherImpl', {amp: true}, (env) => {
     historyMock.verify();
   });
 
-  // TODO(#40214): fix flaky test.
-  it.skip('should push and pop history state', async () => {
-    historyMock.expects('push').resolves('H1').once();
-    historyMock.expects('pop').withArgs('H1').once();
-    const watcher = new CloseWatcherImpl(ampdoc, handler);
-    await Promise.resolve('H1');
-    watcher.signalClosed();
-    expect(handler).to.be.calledOnce;
-  });
+  it.configure()
+    .if(() => 'CloseWatcher' in window)
+    .run('should call the handler on requestClose', async () => {
+      const watcher = new CloseWatcherImpl(ampdoc, handler);
+      watcher.requestClose();
+      expect(handler).to.be.calledOnce;
+    });
 
-  // TODO(#40214): fix flaky test.
-  it.skip('should trigger on history pop', async () => {
-    let popHandler;
-    historyMock
-      .expects('push')
-      .withArgs(
-        env.sandbox.match((a) => {
-          popHandler = a;
-          return true;
-        })
-      )
-      .resolves('H1')
-      .once();
-    new CloseWatcherImpl(ampdoc, handler);
-    await Promise.resolve('H1');
-    expect(popHandler).to.exist;
-    popHandler();
-    expect(handler).to.be.calledOnce;
-  });
+  it.configure()
+    .if(() => !('CloseWatcher' in window))
+    .run('should push and pop history state', async () => {
+      historyMock.expects('push').resolves('H1').once();
+      historyMock.expects('pop').withArgs('H1').once();
+      const watcher = new CloseWatcherImpl(ampdoc, handler);
+      await Promise.resolve('H1');
+      watcher.requestClose();
+      expect(handler).to.be.calledOnce;
+    });
 
-  // TODO(#40214): fix flaky test.
-  it.skip('should trigger on ESC key', async () => {
-    historyMock.expects('push').resolves('H1').once();
-    historyMock.expects('pop').withArgs('H1').once();
-    new CloseWatcherImpl(ampdoc, handler);
-    await Promise.resolve('H1');
+  it.configure()
+    .if(() => !('CloseWatcher' in window))
+    .run('should trigger on history pop', async () => {
+      let popHandler;
+      historyMock
+        .expects('push')
+        .withArgs(
+          env.sandbox.match((a) => {
+            popHandler = a;
+            return true;
+          })
+        )
+        .resolves('H1')
+        .once();
+      new CloseWatcherImpl(ampdoc, handler);
+      await Promise.resolve('H1');
+      expect(popHandler).to.exist;
+      popHandler();
+      expect(handler).to.be.calledOnce;
+    });
 
-    doc.documentElement.dispatchEvent(
-      new KeyboardEvent('keydown', {key: Keys_Enum.ESCAPE})
-    );
-    expect(handler).to.be.calledOnce;
-  });
+  it.configure()
+    .if(() => !('CloseWatcher' in window))
+    .run('should trigger on ESC key', async () => {
+      historyMock.expects('push').resolves('H1').once();
+      historyMock.expects('pop').withArgs('H1').once();
+      new CloseWatcherImpl(ampdoc, handler);
+      await Promise.resolve('H1');
+
+      doc.documentElement.dispatchEvent(
+        new KeyboardEvent('keydown', {key: Keys_Enum.ESCAPE})
+      );
+      expect(handler).to.be.calledOnce;
+    });
 });