Merge pull request #447 from hypothesis/fix-openLoginForm-and-openSidebar

Enable reading openLoginForm and openSidebar from the host page

src/annotator/config/index.js

@@ -14,8 +14,8 @@ function configFrom(window_) {
     app: settings.app,
     query: settings.query,
     annotations: settings.annotations,
-    openLoginForm: settings.hostPageSetting('openLoginForm'),
-    openSidebar: settings.hostPageSetting('openSidebar'),
+    openLoginForm: settings.hostPageSetting('openLoginForm', {allowInBrowserExt: true}),
+    openSidebar: settings.hostPageSetting('openSidebar', {allowInBrowserExt: true}),
     showHighlights: settings.hostPageSetting('showHighlights'),
     branding: settings.hostPageSetting('branding'),
     assetRoot: settings.hostPageSetting('assetRoot'),

src/annotator/config/settings.js

@@ -92,8 +92,10 @@ function settingsFrom(window_) {
     return jsonConfigs.query || queryFromURL();
   }
 
-  function hostPageSetting(name) {
-    if (isBrowserExtension(app())) {
+  function hostPageSetting(name, options = {}) {
+    var allowInBrowserExt = options.allowInBrowserExt || false;
+
+    if (!allowInBrowserExt && isBrowserExtension(app())) {
       return null;
     }
 

src/annotator/config/test/index-test.js

@@ -61,18 +61,40 @@ describe('annotator.config.index', function() {
   [
     'openLoginForm',
     'openSidebar',
+  ].forEach(function(settingName) {
+    it('reads ' + settingName + ' from the host page, even when in a browser extension', function() {
+      configFrom('WINDOW');
+
+      assert.calledWithExactly(
+        fakeSettingsFrom().hostPageSetting,
+        settingName, {allowInBrowserExt: true}
+      );
+    });
+  });
+
+  [
     'showHighlights',
     'branding',
     'assetRoot',
     'sidebarAppUrl',
     'services',
   ].forEach(function(settingName) {
-    it('makes a hostPageSetting for ' + settingName, function() {
+    it('reads ' + settingName + ' from the host page only when in an embedded client', function() {
       configFrom('WINDOW');
 
       assert.calledWithExactly(fakeSettingsFrom().hostPageSetting, settingName);
     });
+  });
 
+  [
+    'openLoginForm',
+    'openSidebar',
+    'showHighlights',
+    'branding',
+    'assetRoot',
+    'sidebarAppUrl',
+    'services',
+  ].forEach(function(settingName) {
     it('returns the ' + settingName + ' value from the host page', function() {
       var settings = {
         'openLoginForm': 'OPEN_LOGIN_FORM_SETTING',

src/annotator/config/test/settings-test.js

@@ -267,69 +267,97 @@ describe('annotator.config.settingsFrom', function() {
   });
 
   describe('#hostPageSetting', function() {
-    context('when the client is from a browser extension', function() {
-      beforeEach('configure a browser extension client', function() {
-        fakeIsBrowserExtension.returns(true);
-      });
-
-      it('always returns null', function() {
-        // These settings in the host page should be ignored.
-        fakeConfigFuncSettingsFrom.returns({foo: 'bar'});
-        fakeSharedSettings.jsonConfigsFrom.returns({foo: 'bar'});
-
-        assert.isNull(settingsFrom(fakeWindow()).hostPageSetting('foo'));
-      });
-    });
-
-    context('when the client is embedded in a web page', function() {
-      beforeEach('configure an embedded client', function() {
-        fakeIsBrowserExtension.returns(false);
-      });
-
-      it('returns setting values from window.hypothesisConfig()', function() {
-        fakeConfigFuncSettingsFrom.returns({foo: 'bar'});
-
-        assert.equal(settingsFrom(fakeWindow()).hostPageSetting('foo'), 'bar');
-      });
-
-      it('returns setting values from js-hypothesis-config scripts', function() {
-        fakeSharedSettings.jsonConfigsFrom.returns({foo: 'bar'});
-
-        assert.equal(settingsFrom(fakeWindow()).hostPageSetting('foo'), 'bar');
-      });
-
-      specify('hypothesisConfig() overrides js-hypothesis-config', function() {
-        fakeConfigFuncSettingsFrom.returns({
-          foo: 'fooFromHypothesisConfig',
-        });
-        fakeSharedSettings.jsonConfigsFrom.returns({
-          foo: 'fooFromJsHypothesisConfigScript',
-        });
-
-        assert.equal(
-          settingsFrom(fakeWindow()).hostPageSetting('foo'),
-          'fooFromHypothesisConfig'
-        );
-      });
-
-      [
-        null,
-        undefined,
-      ].forEach(function(returnValue) {
-        specify('even a ' + returnValue + ' from hypothesisConfig() overrides js-hypothesis-configs', function() {
-          fakeConfigFuncSettingsFrom.returns({foo: returnValue});
-          fakeSharedSettings.jsonConfigsFrom.returns({foo: 'bar'});
-
-          assert.equal(settingsFrom(fakeWindow()).hostPageSetting('foo'), returnValue);
+    [
+      {
+        when: 'the client is embedded in a web page',
+        specify: 'it returns setting values from window.hypothesisConfig()',
+        isBrowserExtension: false,
+        configFuncSettings: {foo: 'configFuncValue'},
+        jsonSettings: {},
+        expected: 'configFuncValue',
+      },
+      {
+        when: 'the client is embedded in a web page',
+        specify: 'it returns setting values from js-hypothesis-config objects',
+        isBrowserExtension: false,
+        configFuncSettings: {},
+        jsonSettings: {foo: 'jsonValue'},
+        expected: 'jsonValue',
+      },
+      {
+        when: 'the client is embedded in a web page',
+        specify: 'hypothesisConfig() settings override js-hypothesis-config ones',
+        isBrowserExtension: false,
+        configFuncSettings: {foo: 'configFuncValue'},
+        jsonSettings: {foo: 'jsonValue'},
+        expected: 'configFuncValue',
+      },
+      {
+        when: 'the client is embedded in a web page',
+        specify: 'even a null from hypothesisConfig() overrides js-hypothesis-config',
+        isBrowserExtension: false,
+        configFuncSettings: {foo: null},
+        jsonSettings: {foo: 'jsonValue'},
+        expected: null,
+      },
+      {
+        when: 'the client is embedded in a web page',
+        specify: 'even an undefined from hypothesisConfig() overrides js-hypothesis-config',
+        isBrowserExtension: false,
+        configFuncSettings: {foo: undefined},
+        jsonSettings: {foo: 'jsonValue'},
+        expected: undefined,
+      },
+      {
+        when: 'the client is embedded in a web page',
+        specify: "it returns undefined if the setting isn't defined anywhere",
+        isBrowserExtension: false,
+        configFuncSettings: {},
+        jsonSettings: {},
+        expected: undefined,
+      },
+      {
+        when: 'the client is in a browser extension',
+        specify: 'it always returns null',
+        isBrowserExtension: true,
+        configFuncSettings: {foo: 'configFuncValue'},
+        jsonSettings: {foo: 'jsonValue'},
+        expected: null,
+      },
+      {
+        when: 'the client is in a browser extension and allowInBrowserExt: true is given',
+        specify: 'it returns settings from window.hypothesisConfig()',
+        isBrowserExtension: true,
+        allowInBrowserExt: true,
+        configFuncSettings: {foo: 'configFuncValue'},
+        jsonSettings: {},
+        expected: 'configFuncValue',
+      },
+      {
+        when: 'the client is in a browser extension and allowInBrowserExt: true is given',
+        specify: 'it returns settings from js-hypothesis-configs',
+        isBrowserExtension: true,
+        allowInBrowserExt: true,
+        configFuncSettings: {},
+        jsonSettings: {foo: 'jsonValue'},
+        expected: 'jsonValue',
+      },
+    ].forEach(function(test) {
+      context(test.when, function() {
+        specify(test.specify, function() {
+          fakeIsBrowserExtension.returns(test.isBrowserExtension);
+          fakeConfigFuncSettingsFrom.returns(test.configFuncSettings);
+          fakeSharedSettings.jsonConfigsFrom.returns(test.jsonSettings);
+          var settings = settingsFrom(fakeWindow());
+
+          var setting = settings.hostPageSetting(
+            'foo',
+            {allowInBrowserExt: test.allowInBrowserExt || false}
+          );
+
+          assert.equal(setting, test.expected);
         });
       });
-
-      it("returns undefined if the setting isn't defined anywhere", function() {
-        fakeConfigFuncSettingsFrom.returns({});
-        fakeSharedSettings.jsonConfigsFrom.returns({});
-
-        assert.isUndefined(settingsFrom(fakeWindow()).hostPageSetting('foo'));
-      });
     });
   });
 });