Merge pull request #457 from evidentpoint/multi-frame-fixes

Multiple frame detection improvements

src/annotator/frame-observer.js

+'use strict';
+
+let FrameUtil = require('./util/frame-util');
+let debounce = require('lodash.debounce');
+
+// Find difference of two arrays
+let difference = (arrayA, arrayB) => {
+  return arrayA.filter(x => !arrayB.includes(x));
+};
+
+const DEBOUNCE_WAIT = 40;
+
+class FrameObserver {
+
+  constructor (target) {
+    this._target = target;
+    this._handledFrames = [];
+
+    this._mutationObserver = new MutationObserver(debounce(() => {
+      this._discoverFrames();
+    }, DEBOUNCE_WAIT));
+  }
+
+  observe (onFrameAddedCallback, onFrameRemovedCallback) {
+    this._onFrameAdded = onFrameAddedCallback;
+    this._onFrameRemoved = onFrameRemovedCallback;
+
+    this._discoverFrames();
+    this._mutationObserver.observe(this._target, {
+      childList: true,
+      subtree: true,
+    });
+  }
+
+  disconnect () {
+    this._mutationObserver.disconnect();
+  }
+
+  _addFrame (frame) {
+    if (FrameUtil.isAccessible(frame)) {
+      FrameUtil.isDocumentReady(frame, () => {
+        frame.contentWindow.addEventListener('unload', () => {
+          this._removeFrame(frame);
+        });
+        this._handledFrames.push(frame);
+        this._onFrameAdded(frame);
+      });
+    } else {
+      // Could warn here that frame was not cross origin accessible
+    }
+  }
+
+  _removeFrame (frame) {
+    this._onFrameRemoved(frame);
+
+    // Remove the frame from our list
+    this._handledFrames = this._handledFrames.filter(x => x !== frame);
+  }
+
+  _discoverFrames () {
+    let frames = FrameUtil.findFrames(this._target);
+
+    for (let frame of frames) {
+      if (!this._handledFrames.includes(frame)) {
+        this._addFrame(frame);
+      }
+    }
+
+    for (let frame of difference(this._handledFrames, frames)) {
+      this._removeFrame(frame);
+    }
+  }
+}
+FrameObserver.DEBOUNCE_WAIT = DEBOUNCE_WAIT;
+
+module.exports = FrameObserver;
\ No newline at end of file

src/annotator/plugin/cross-frame.coffee

@@ -4,8 +4,7 @@ AnnotationSync = require('../annotation-sync')
 Bridge = require('../../shared/bridge')
 Discovery = require('../../shared/discovery')
 FrameUtil = require('../util/frame-util')
-
-debounce = require('lodash.debounce')
+FrameObserver = require('../frame-observer')
 
 # Extracts individual keys from an object and returns a new one.
 extract = extract = (obj, keys...) ->
@@ -13,11 +12,6 @@ extract = extract = (obj, keys...) ->
   ret[key] = obj[key] for key in keys when obj.hasOwnProperty(key)
   ret
 
-
-# Find difference of two arrays
-difference = (arrayA, arrayB) ->
-  arrayA.filter (x) -> !arrayB.includes(x)
-
 # Class for establishing a messaging connection to the parent sidebar as well
 # as keeping the annotation state in sync with the sidebar application, this
 # frame acts as the bridge client, the sidebar is the server. This plugin
@@ -35,8 +29,7 @@ module.exports = class CrossFrame extends Plugin
 
     opts = extract(options, 'on', 'emit')
     annotationSync = new AnnotationSync(bridge, opts)
-
-    handledFrames = []
+    frameObserver = new FrameObserver(elem)
 
     this.pluginInit = ->
       onDiscoveryCallback = (source, origin, token) ->
@@ -44,13 +37,14 @@ module.exports = class CrossFrame extends Plugin
       discovery.startDiscovery(onDiscoveryCallback)
 
       if options.enableMultiFrameSupport
-        _setupFrameDetection()
+        frameObserver.observe(_injectToFrame, _iframeUnloaded);
 
     this.destroy = ->
       # super doesnt work here :(
       Plugin::destroy.apply(this, arguments)
       bridge.destroy()
       discovery.stopDiscovery()
+      frameObserver.disconnect()
 
     this.sync = (annotations, cb) ->
       annotationSync.sync(annotations, cb)
@@ -64,34 +58,10 @@ module.exports = class CrossFrame extends Plugin
     this.onConnect = (fn) ->
       bridge.onConnect(fn)
 
-    _setupFrameDetection = ->
-      _discoverOwnFrames()
-
-      # Listen for DOM mutations, to know when frames are added / removed
-      observer = new MutationObserver(debounce(_discoverOwnFrames, 300, leading: true))
-      observer.observe(elem, {childList: true, subtree: true});
-
-    _discoverOwnFrames = ->
-      frames = FrameUtil.findFrames(elem)
-      for frame in frames
-        if frame not in handledFrames
-          _handleFrame(frame)
-          handledFrames.push(frame)
-
-      for frame, i in difference(handledFrames, frames)
-        _iframeUnloaded(frame)
-        delete handledFrames[i]
-
     _injectToFrame = (frame) ->
       if !FrameUtil.hasHypothesis(frame)
-        FrameUtil.injectHypothesis(frame, options.embedScriptUrl)
-        frame.contentWindow.addEventListener 'unload', ->
-          _iframeUnloaded(frame)
-
-    _handleFrame = (frame) ->
-      if !FrameUtil.isAccessible(frame) then return
-      FrameUtil.isLoaded frame, () ->
-        _injectToFrame(frame)
+        FrameUtil.isLoaded frame, () ->
+          FrameUtil.injectHypothesis(frame, options.embedScriptUrl)
 
     _iframeUnloaded = (frame) ->
       # TODO: Bridge call here not yet implemented, placeholder for now

src/annotator/test/integration/multi-frame-test.js

 'use strict';
 
 var proxyquire = require('proxyquire');
-var isLoaded = require('../../util/frame-util.js').isLoaded;
+var isLoaded = require('../../util/frame-util').isLoaded;
+
+var FRAME_ADD_WAIT = require('../../frame-observer').DEBOUNCE_WAIT + 10;
 
 describe('CrossFrame multi-frame scenario', function () {
   var fakeAnnotationSync;
@@ -143,7 +145,7 @@ describe('CrossFrame multi-frame scenario', function () {
             'expected dynamically added frame to be modified');
           resolve();
         });
-      }, 0);
+      }, FRAME_ADD_WAIT);
     });
   });
 
@@ -169,4 +171,75 @@ describe('CrossFrame multi-frame scenario', function () {
     });
   });
 
+  it('detects a frame dynamically removed, and added again', function () {
+    // Create a frame before initializing
+    var frame = document.createElement('iframe');
+    container.appendChild(frame);
+
+    // Now initialize
+    crossFrame.pluginInit();
+
+    return new Promise(function (resolve) {
+
+      isLoaded(frame, function () {
+        assert(frame.contentDocument.body.hasChildNodes(),
+          'expected initial frame to be modified');
+
+        frame.remove();
+
+        // Yield to let the DOM and CrossFrame catch up
+        setTimeout(function () {
+          // Add the frame again
+          container.appendChild(frame);
+
+          // Yield again
+          setTimeout(function () {
+            isLoaded(frame, function () {
+              assert(frame.contentDocument.body.hasChildNodes(),
+                'expected dynamically added frame to be modified');
+              resolve();
+            });
+          }, FRAME_ADD_WAIT);
+        }, 0);
+      });
+    });
+  });
+
+  it('detects a frame dynamically added, removed, and added again', function () {
+
+    // Initialize with no initial frame
+    crossFrame.pluginInit();
+
+    // Add a frame to the DOM
+    var frame = document.createElement('iframe');
+    container.appendChild(frame);
+
+    return new Promise(function (resolve) {
+      // Yield to let the DOM and CrossFrame catch up
+      setTimeout(function () {
+        isLoaded(frame, function () {
+          assert(frame.contentDocument.body.hasChildNodes(),
+            'expected dynamically added frame to be modified');
+
+          frame.remove();
+
+          // Yield again
+          setTimeout(function () {
+            // Add the frame again
+            container.appendChild(frame);
+
+            // Yield
+            setTimeout(function () {
+              isLoaded(frame, function () {
+                assert(frame.contentDocument.body.hasChildNodes(),
+                  'expected dynamically added frame to be modified');
+                resolve();
+              });
+            }, FRAME_ADD_WAIT);
+          }, 0);
+        });
+      }, FRAME_ADD_WAIT);
+    });
+  });
+
 });
\ No newline at end of file

src/annotator/util/frame-util.js

@@ -43,6 +43,16 @@ function isValid (iframe) {
   return iframe.className !== 'h-sidebar-iframe';
 }
 
+function isDocumentReady (iframe, callback) {
+  if (iframe.contentDocument.readyState === 'loading') {
+    iframe.contentDocument.addEventListener('DOMContentLoaded', function () {
+      callback();
+    });
+  } else {
+    callback();
+  }
+}
+
 function isLoaded (iframe, callback) {
   if (iframe.contentDocument.readyState !== 'complete') {
     iframe.addEventListener('load', function () {
@@ -60,4 +70,5 @@ module.exports = {
   isAccessible: isAccessible,
   isValid: isValid,
   isLoaded: isLoaded,
+  isDocumentReady: isDocumentReady,
 };