Minor improvements following PR feedback

* check that the iframe's `src` element is not 'about:blank'. * use EventTarget class and fake DOM element for tests * fix typos
hypothesis · Sep 16, 2021 · 9e4a471 · 9e4a471
1 parent 8575eb9
commit 9e4a471
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 27 deletions.
diff --git a/src/annotator/frame-observer.js b/src/annotator/frame-observer.js
@@ -119,7 +119,11 @@ export function onDocumentReady(frame) {
     // 'uninitialized' readyState on Firefox. If a blank document is detected and
     // there is a 'src' attribute, it is expected that the blank document will be
     // replaced by the final document.
-    if (location.href === 'about:blank' && frame.hasAttribute('src')) {
+    if (
+      location.href === 'about:blank' &&
+      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.
       frame.addEventListener('load', () => {
@@ -133,7 +137,7 @@ export function onDocumentReady(frame) {
       return;
     }
 
-    // state is 'interactive' or 'complete';
+    // State is 'interactive' or 'complete';
     resolve();
   });
 }
diff --git a/src/annotator/test/frame-observer-test.js b/src/annotator/test/frame-observer-test.js
@@ -4,7 +4,7 @@ import {
   onDocumentReady,
 } from '../frame-observer';
 
-describe('annotator/frame-observer.js', () => {
+describe('annotator/frame-observer', () => {
   describe('FrameObserver', () => {
     let container;
     let frameObserver;
@@ -153,42 +153,48 @@ describe('annotator/frame-observer.js', () => {
     let fakeIFrame;
     let fakeIFrameDocument;
 
-    beforeEach(() => {
-      fakeIFrameDocument = {
-        addEventListener: sinon.stub(),
-        readyState: 'loading',
-        location: {
+    class FakeIFrameDocument extends EventTarget {
+      constructor() {
+        super();
+        this.readyState = 'loading';
+        this.location = {
           href: 'about:blank',
-        },
-      };
-
-      fakeIFrame = {
-        contentWindow: {
-          document: fakeIFrameDocument,
-        },
-        addEventListener: sinon.stub(),
-        hasAttribute: sinon.stub(),
-      };
+        };
+      }
+    }
+
+    beforeEach(() => {
+      fakeIFrameDocument = new FakeIFrameDocument();
+      fakeIFrame = document.createElement('div');
+      fakeIFrame.contentWindow = { document: fakeIFrameDocument };
+      fakeIFrame.setAttribute('src', 'http://my.dummy');
     });
 
     it('waits for the iframe load event to be triggered if the document is blank', () => {
       fakeIFrameDocument.location.href = 'about:blank';
-      fakeIFrame.hasAttribute.returns(true);
-      onDocumentReady(fakeIFrame);
+      const onLoad = onDocumentReady(fakeIFrame);
+
+      fakeIFrame.dispatchEvent(new Event('load'));
 
-      assert.calledWith(fakeIFrame.addEventListener, 'load');
+      return onLoad;
     });
 
     it('waits for the iframe DOMContentLoaded event to be triggered if the document is loading', () => {
       fakeIFrameDocument.location.href = 'about:srcdoc';
-      fakeIFrame.hasAttribute.returns(false);
       fakeIFrameDocument.readyState = 'loading';
-      onDocumentReady(fakeIFrame);
+      const onDOMContentLoaded = onDocumentReady(fakeIFrame);
 
-      assert.calledWith(
-        fakeIFrameDocument.addEventListener,
-        'DOMContentLoaded'
-      );
+      fakeIFrameDocument.dispatchEvent(new Event('DOMContentLoaded'));
+
+      return onDOMContentLoaded;
+    });
+
+    it("resolves immediately if document is 'complete' or 'interactive'", () => {
+      fakeIFrameDocument.location.href = 'about:srcdoc';
+      fakeIFrameDocument.readyState = 'complete';
+      const onReady = onDocumentReady(fakeIFrame);
+
+      return onReady;
     });
   });
 });