Skip to content

Commit

Permalink
Add documentation to test cases
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
esanzgar committed Oct 8, 2021
1 parent ba20635 commit 0356d20
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 6 deletions.
5 changes: 3 additions & 2 deletions src/annotator/frame-observer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
16 changes: 12 additions & 4 deletions src/annotator/test/frame-observer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
Expand All @@ -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);
});
});
});

0 comments on commit 0356d20

Please sign in to comment.