Make annotation of iframes opt-in

docs/publishers/embedding.rst

@@ -13,3 +13,31 @@ each page that you want to have the Hypothesis client on:
 .. code-block:: html
 
    <script src="https://hypothes.is/embed.js" async></script>
+
+
+Enabling annotation of iframed content
+--------------------------------------
+
+The simplest way to support annotation of iframed content is to add the
+above script tag to the document displayed in the iframe. This will display the
+sidebar in the iframe itself.
+
+Additionally Hypothesis has limited support for enabling annotation of iframed
+content while showing the sidebar in the top-level document. To use this:
+
+1. Add the above script tag to the top-level document
+
+2. Do not add the script tag to the iframed documents themselves, the client
+   will do this itself.
+
+3. Opt iframes into annotation by adding the "enable-annotation" attribute:
+
+.. code-block:: html
+
+   <iframe enable-annotation>
+   ...
+   </iframe>
+
+This method *only* works for iframes which are same-origin with the top-level
+document. The client will watch for new iframes being added to the document and
+will automatically enable annotation for them.

scripts/gulp/live-reload-server.js

@@ -85,7 +85,7 @@ function LiveReloadServer(port, config) {
               <button id="add-test" style="padding: 0.6em; font-size: 0.75em">Toggle 2nd Frame</button>
             </div>
             <div style="margin: 10px 0 0 75px;">
-              <iframe id="iframe1" src="/document/license" style="width: 50%;height: 300px;"></iframe>
+              <iframe enable-annotation id="iframe1" src="/document/license" style="width: 50%;height: 300px;"></iframe>
             </div>
             <div id="iframe2-container" style="margin: 10px 0 0 75px;">
             </div>

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

@@ -58,6 +58,7 @@ describe('CrossFrame multi-frame scenario', function () {
   it('detects frames on page', function () {
     // Create a frame before initializing
     var validFrame = document.createElement('iframe');
+    validFrame.setAttribute('enable-annotation', '');
     container.appendChild(validFrame);
 
     // Create another that mimics the sidebar frame
@@ -90,6 +91,7 @@ describe('CrossFrame multi-frame scenario', function () {
   it('detects removed frames', function () {
     // Create a frame before initializing
     var frame = document.createElement('iframe');
+    frame.setAttribute('enable-annotation', '');
     container.appendChild(frame);
 
     // Now initialize
@@ -103,6 +105,7 @@ describe('CrossFrame multi-frame scenario', function () {
 
   it('injects embed script in frame', function () {
     var frame = document.createElement('iframe');
+    frame.setAttribute('enable-annotation', '');
     container.appendChild(frame);
 
     crossFrame.pluginInit();
@@ -120,6 +123,7 @@ describe('CrossFrame multi-frame scenario', function () {
 
   it('excludes injection from already injected frames', function () {
     var frame = document.createElement('iframe');
+    frame.setAttribute('enable-annotation', '');
     frame.srcdoc = '<script>window.__hypothesis_frame = true;</script>';
     container.appendChild(frame);
 
@@ -140,6 +144,7 @@ describe('CrossFrame multi-frame scenario', function () {
 
     // Add a frame to the DOM
     var frame = document.createElement('iframe');
+    frame.setAttribute('enable-annotation', '');
     container.appendChild(frame);
 
     return new Promise(function (resolve) {
@@ -157,6 +162,7 @@ describe('CrossFrame multi-frame scenario', function () {
   it('detects dynamically removed frames', function () {
     // Create a frame before initializing
     var frame = document.createElement('iframe');
+    frame.setAttribute('enable-annotation', '');
     container.appendChild(frame);
 
     // Now initialize
@@ -179,6 +185,7 @@ 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');
+    frame.setAttribute('enable-annotation', '');
     container.appendChild(frame);
 
     // Now initialize
@@ -219,6 +226,7 @@ describe('CrossFrame multi-frame scenario', function () {
 
     // Add a frame to the DOM
     var frame = document.createElement('iframe');
+    frame.setAttribute('enable-annotation', '');
     container.appendChild(frame);
 
     return new Promise(function (resolve) {

src/annotator/util/frame-util.js

@@ -1,18 +1,23 @@
 'use strict';
 
-// Find all iframes within this iframe only
-function findFrames (container) {
+/**
+ * Return all `<iframe>` elements under `container` which are annotate-able.
+ *
+ * @param {Element} container
+ * @return {HTMLIFrameElement[]}
+ */
+function findFrames(container) {
   const frames = Array.from(container.getElementsByTagName('iframe'));
-  return frames.filter(isValid);
+  return frames.filter(shouldEnableAnnotation);
 }
 
 // Check if the iframe has already been injected
-function hasHypothesis (iframe) {
+function hasHypothesis(iframe) {
   return iframe.contentWindow.__hypothesis_frame === true;
 }
 
 // Inject embed.js into the iframe
-function injectHypothesis (iframe, scriptUrl, config) {
+function injectHypothesis(iframe, scriptUrl, config) {
   const configElement = document.createElement('script');
   configElement.className = 'js-hypothesis-config';
   configElement.type = 'application/json';
@@ -29,50 +34,56 @@ function injectHypothesis (iframe, scriptUrl, config) {
 }
 
 // Check if we can access this iframe's document
-function isAccessible (iframe) {
+function isAccessible(iframe) {
   try {
     return !!iframe.contentDocument;
   } catch (e) {
     return false;
   }
 }
 
-
 /**
- * Check if the frame elements being considered for injection have the
- * basic heuristics for content that a user might want to annotate.
- *  Rules:
- *    - avoid our client iframe
- *    - iframe should be sizeable - to avoid the small advertisement and social plugins
+ * Return `true` if an iframe should be made annotate-able.
+ *
+ * To enable annotation, an iframe must be opted-in by adding the
+ * "enable-annotation" attribute and must be visible.
  *
  * @param  {HTMLIFrameElement} iframe the frame being checked
  * @returns {boolean}   result of our validity checks
  */
-function isValid (iframe) {
-
+function shouldEnableAnnotation(iframe) {
+  // Ignore the Hypothesis sidebar.
   const isNotClientFrame = !iframe.classList.contains('h-sidebar-iframe');
 
+  // Ignore hidden or very small iframes.
   const frameRect = iframe.getBoundingClientRect();
   const MIN_WIDTH = 150;
   const MIN_HEIGHT = 150;
-  const hasSizableContainer = frameRect.width > MIN_WIDTH && frameRect.height > MIN_HEIGHT;
+  const hasSizableContainer =
+    frameRect.width > MIN_WIDTH && frameRect.height > MIN_HEIGHT;
+
+  // Ignore frames which have not opted into annotation support.
+  // Eventually we would like iframe annotation to be enabled by default,
+  // however we need to resolve a number of issues with iframe support before we
+  // can do that. See https://github.com/hypothesis/client/issues/530
+  const enabled = iframe.hasAttribute('enable-annotation');
 
-  return isNotClientFrame && hasSizableContainer;
+  return isNotClientFrame && hasSizableContainer && enabled;
 }
 
-function isDocumentReady (iframe, callback) {
+function isDocumentReady(iframe, callback) {
   if (iframe.contentDocument.readyState === 'loading') {
-    iframe.contentDocument.addEventListener('DOMContentLoaded', function () {
+    iframe.contentDocument.addEventListener('DOMContentLoaded', function() {
       callback();
     });
   } else {
     callback();
   }
 }
 
-function isLoaded (iframe, callback) {
+function isLoaded(iframe, callback) {
   if (iframe.contentDocument.readyState !== 'complete') {
-    iframe.addEventListener('load', function () {
+    iframe.addEventListener('load', function() {
       callback();
     });
   } else {

src/annotator/util/test/frame-util-test.js

@@ -2,13 +2,14 @@
 
 const frameUtil = require('../frame-util');
 
-describe('frameUtil', function () {
+describe('annotator.util.frame-util', function () {
   describe('findFrames', function () {
 
     let container;
 
     const _addFrameToContainer = (options={})=>{
       const frame = document.createElement('iframe');
+      frame.setAttribute('enable-annotation', '');
       frame.className = options.className || '';
       frame.style.height = `${(options.height || 150)}px`;
       frame.style.width = `${(options.width || 150)}px`;
@@ -25,7 +26,7 @@ describe('frameUtil', function () {
       container.remove();
     });
 
-    it('should find valid frames', function () {
+    it('should return valid frames', function () {
 
       let foundFrames = frameUtil.findFrames(container);
 
@@ -39,8 +40,7 @@ describe('frameUtil', function () {
       assert.deepEqual(foundFrames, [frame1, frame2], 'appended frames should be found');
     });
 
-    it('should not find small frames', function () {
-
+    it('should not return small frames', function () {
       // add frames that are small in both demensions
       _addFrameToContainer({width: 140});
       _addFrameToContainer({height: 140});
@@ -50,7 +50,16 @@ describe('frameUtil', function () {
       assert.lengthOf(foundFrames, 0, 'frames with small demensions should not be found');
     });
 
-    it('should not find hypothesis frames', function () {
+    it('should not return frames that have not opted into annotation', () => {
+      const frame = _addFrameToContainer();
+
+      frame.removeAttribute('enable-annotation');
+
+      const foundFrames = frameUtil.findFrames(container);
+      assert.lengthOf(foundFrames, 0);
+    });
+
+    it('should not return the Hypothesis sidebar', function () {
 
       _addFrameToContainer({className: 'h-sidebar-iframe other-class-too'});