From 0356d2031b367794d002284f22dbd9fbaf3909eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Eduardo=20Sanz=20Garc=C3=ADa?= <esanzgar@gmail.com>
Date: Wed, 15 Sep 2021 18:43:18 +0200
Subject: [PATCH] Add documentation to test cases

* add information about why to use `window.load` instead of
  `document.DOMContentLoaded` to check when a document is ready

* use an invalid local URL (http://localhost:1) to speed DNS lookup in
  the test

* include some comments about the use of `waitForFrameObserver` in the
  tests
---
 src/annotator/frame-observer.js           |  5 +++--
 src/annotator/test/frame-observer-test.js | 16 ++++++++++++----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/annotator/frame-observer.js b/src/annotator/frame-observer.js
index 686c374daed..a541d640623 100644
--- a/src/annotator/frame-observer.js
+++ b/src/annotator/frame-observer.js
@@ -124,8 +124,9 @@ export function onDocumentReady(frame) {
       frame.hasAttribute('src') &&
       frame.src !== 'about:blank'
     ) {
-      // Unfortunately, listening for 'DOMContentLoaded' on the iframeDocument
-      // doesn't work. Instead, we need to wait for a 'load' event to be triggered.
+      // Listening to `DOMContentLoaded` on the frame's document doesn't work because the
+      // document is replaced. On the other hand, listening the frame's `load`
+      // works because the frame element (as in HTMLIFrameElement) doesn't change.
       frame.addEventListener('load', () => {
         resolve();
       });
diff --git a/src/annotator/test/frame-observer-test.js b/src/annotator/test/frame-observer-test.js
index 3aa81fc4a38..c29dede1dc0 100644
--- a/src/annotator/test/frame-observer-test.js
+++ b/src/annotator/test/frame-observer-test.js
@@ -12,6 +12,7 @@ describe('annotator/frame-observer', () => {
     let onFrameRemoved;
     const sandbox = sinon.createSandbox();
 
+    // FrameObserver waits a few milliseconds (DEBOUNCE_WAIT) to detect changes in the DOM
     function waitForFrameObserver() {
       return new Promise(resolve => setTimeout(resolve, DEBOUNCE_WAIT + 10));
     }
@@ -139,8 +140,12 @@ describe('annotator/frame-observer', () => {
 
     it("doesn't trigger onFrameAdded when annotatable iframe is from a different domain", async () => {
       const iframe = createAnnotatableIFrame();
-      iframe.setAttribute('src', 'http://cross-origin.dummy');
+      iframe.setAttribute('src', 'http://localhost:1');
 
+      // In this particular case waiting for the FrameObserver to detect the new
+      // iframe may not be enough. Because the browser fetches the URL in `src`
+      // (it is not reachable) it could take longer, that's why, in addition, we
+      // wait for the iframe's document to completely load.
       await onDocumentReady(iframe);
       await waitForFrameObserver();
 
@@ -174,6 +179,11 @@ describe('annotator/frame-observer', () => {
       fakeIFrameDocument.location.href = 'about:blank';
       const onLoad = onDocumentReady(fakeIFrame);
 
+      // After the initial 'about:blank' document, a new document is loaded.
+      const newDocument = new FakeIFrameDocument();
+      newDocument.location.href = 'http://my.dummy';
+      newDocument.readyState = 'complete';
+      fakeIFrame.contentWindow = { document: newDocument };
       fakeIFrame.dispatchEvent(new Event('load'));
 
       return onLoad;
@@ -192,9 +202,7 @@ describe('annotator/frame-observer', () => {
     it("resolves immediately if document is 'complete' or 'interactive'", () => {
       fakeIFrameDocument.location.href = 'about:srcdoc';
       fakeIFrameDocument.readyState = 'complete';
-      const onReady = onDocumentReady(fakeIFrame);
-
-      return onReady;
+      return onDocumentReady(fakeIFrame);
     });
   });
 });