Merge pull request #3161 from hypothesis/t310-annotation-fragment-capture

Make client injection robust to pages that replace the URL fragment as they load

h/static/scripts/annotator/config.js

 'use strict';
 
+var annotationIDs = require('../util/annotation-ids');
+var settings = require('../settings');
+
 var docs = 'https://h.readthedocs.org/en/latest/hacking/customized-embedding.html';
 
 /**
@@ -13,6 +16,15 @@ function config(window_) {
       document.querySelector('link[type="application/annotator+html"]').href,
   };
 
+  // Parse config from `<script class="js-hypothesis-config">` tags
+  try {
+    Object.assign(options, settings(window_.document, 'js-hypothesis-config'));
+  } catch (err) {
+    console.warn('Could not parse settings from js-hypothesis-config tags',
+      err);
+  }
+
+  // Parse config from `window.hypothesisConfig` function
   if (window_.hasOwnProperty('hypothesisConfig')) {
     if (typeof window_.hypothesisConfig === 'function') {
       Object.assign(options, window_.hypothesisConfig());
@@ -21,9 +33,17 @@ function config(window_) {
     }
   }
 
-  var annotFragmentMatch = window_.location.hash.match(/^#annotations:(.*)/);
-  if (annotFragmentMatch) {
-    options.annotations = annotFragmentMatch[1];
+  // Extract the direct linked ID from the URL.
+  //
+  // The Chrome extension or proxy may already have provided this config
+  // via a tag injected into the DOM, which avoids the problem where the page's
+  // JS rewrites the URL before Hypothesis loads.
+  //
+  // In environments where the config has not been injected into the DOM,
+  // we try to retrieve it from the URL here.
+  var directLinkedID = annotationIDs.extractIDFromURL(window_.location.href);
+  if (directLinkedID) {
+    options.annotations = directLinkedID;
   }
   return options;
 }

h/static/scripts/annotator/test/config-test.js

@@ -3,13 +3,34 @@
 var config = require('../config');
 
 describe('annotator configuration', function () {
+  var fakeScriptConfig;
+
+  function fakeQuerySelector(selector) {
+    if (selector === 'link[type="application/annotator+html"]') {
+      return {href: 'app.html'};
+    } else if (selector === 'script.js-hypothesis-config' &&
+               fakeScriptConfig) {
+      return {textContent: fakeScriptConfig};
+    } else {
+      return null;
+    }
+  }
+
   var fakeWindowBase = {
     document: {
-      querySelector: sinon.stub().returns({href: 'app.html'}),
+      querySelector: fakeQuerySelector,
+      querySelectorAll: function (selector) {
+        var match = fakeQuerySelector(selector);
+        return match ? [match] : [];
+      },
     },
     location: {hash: ''},
   };
 
+  beforeEach(function () {
+    fakeScriptConfig = '';
+  });
+
   it('reads the app src from the link tag', function () {
     var linkEl = document.createElement('link');
     linkEl.type = 'application/annotator+html';
@@ -23,7 +44,7 @@ describe('annotator configuration', function () {
 
   it('reads the #annotation query fragment', function () {
     var fakeWindow = Object.assign({}, fakeWindowBase, {
-      location: {hash:'#annotations:456'},
+      location: {href:'https://foo.com/#annotations:456'},
     });
     assert.deepEqual(config(fakeWindow), {
       app: 'app.html',
@@ -42,4 +63,12 @@ describe('annotator configuration', function () {
       firstRun: true,
     });
   });
+
+  it('merges the config from the "js-hypothesis-config" <script> tag', function () {
+    fakeScriptConfig = '{"annotations":"456"}';
+    assert.deepEqual(config(fakeWindowBase), {
+      app: 'app.html',
+      annotations: '456',
+    });
+  });
 });

h/static/scripts/settings.js

@@ -6,16 +6,20 @@ require('core-js/fn/object/assign');
  * Return application configuration information from the host page.
  *
  * Exposes shared application settings, read from script tags with the
- * class 'js-hypothesis-settings' which contain JSON content.
+ * class `settingsClass` which contain JSON content.
  *
  * If there are multiple such tags, the configuration from each is merged.
  *
  * @param {Document|Element} document - The root element to search for
  *                                      <script> settings tags.
+ * @param {string} settingsClass - The class name to match on <script> tags.
  */
-function settings(document) {
+function settings(document, settingsClass) {
+  if (!settingsClass) {
+    settingsClass = 'js-hypothesis-settings';
+  }
   var settingsElements =
-    document.querySelectorAll('script.js-hypothesis-settings');
+    document.querySelectorAll('script.' + settingsClass);
 
   var config = {};
   for (var i=0; i < settingsElements.length; i++) {

h/static/scripts/test/settings-test.js

+'use strict';
+
 var settings = require('../settings');
 
-function createJSONScriptTag(obj) {
+function createJSONScriptTag(obj, className) {
   var el = document.createElement('script');
   el.type = 'application/json';
   el.textContent = JSON.stringify(obj);
-  el.classList.add('js-hypothesis-settings');
+  el.classList.add(className);
+  el.classList.add('js-settings-test');
   return el;
 }
 
-describe('settings', function () {
-  afterEach(function () {
-    var elements = document.querySelectorAll('.js-hypothesis-settings');
+function removeJSONScriptTags() {
+  var elements = document.querySelectorAll('.js-settings-test');
   for (var i=0; i < elements.length; i++) {
     elements[i].parentNode.removeChild(elements[i]);
   }
+}
+
+describe('settings', function () {
+  afterEach(removeJSONScriptTags);
+
+  it('reads config from .js-hypothesis-settings <script> tags', function () {
+    document.body.appendChild(createJSONScriptTag({key:'value'},
+      'js-hypothesis-settings'));
+    assert.deepEqual(settings(document), {key:'value'});
+  });
+
+  it('reads config from <script> tags with the specified class name', function () {
+    document.body.appendChild(createJSONScriptTag({foo:'bar'},
+      'js-custom-settings'));
+    assert.deepEqual(settings(document), {});
+    assert.deepEqual(settings(document, 'js-custom-settings'), {foo:'bar'});
   });
 
-  it('should merge settings', function () {
-    document.body.appendChild(createJSONScriptTag({ a: 1 }));
-    document.body.appendChild(createJSONScriptTag({ b: 2 }));
-    assert.deepEqual(settings(document), { a: 1, b: 2 });
+  it('merges settings from all config <script> tags', function () {
+    document.body.appendChild(createJSONScriptTag({a: 1}, 'settings'));
+    document.body.appendChild(createJSONScriptTag({b: 2}, 'settings'));
+    assert.deepEqual(settings(document, 'settings'), {a: 1, b: 2});
   });
 });

h/static/scripts/util/annotation-ids.js

+'use strict';
+
+/**
+ * Extracts a direct-linked annotation ID from the fragment of a URL.
+ *
+ * @param {string} url - The URL which may contain a '#annotations:<ID>'
+ *        fragment.
+ * @return {string?} The annotation ID if present
+ */
+function extractIDFromURL(url) {
+  try {
+    // Annotation IDs are url-safe-base64 identifiers
+    // See https://tools.ietf.org/html/rfc4648#page-7
+    var annotFragmentMatch = url.match(/#annotations:([A-Za-z0-9_-]+)$/);
+    if (annotFragmentMatch) {
+      return annotFragmentMatch[1];
+    } else {
+      return null;
+    }
+  } catch (err) {
+    return null;
+  }
+}
+
+module.exports = {
+  extractIDFromURL: extractIDFromURL,
+};