Fix login flow triggering popup blocker in Firefox and IE

hypothesis/client#603 broke the login popup window in Firefox and IE
because the call to `window.open` no longer happens in the same turn of
the event loop as the user's click on the "Login" link. It is therefore
no longer considered in FF to have happened "in response to a user
gesture".

This PR fixes the issue by separating creation and use of the popup
window into separate functions and moving creation to happen earlier, in
the same event loop turn as the "Login" button click.

Fixes #534

src/sidebar/oauth-auth.js

@@ -255,8 +255,9 @@ function auth($http, $rootScope, $window, OAuthClient,
    * then exchange for access and refresh tokens.
    */
   function login() {
+    var authWindow = OAuthClient.createLoginPopupWindow($window);
     return oauthClient().then(client => {
-      return client.authorize($window);
+      return client.authorize($window, authWindow);
     }).then(code => {
       // Save the auth code. It will be exchanged for an access token when the
       // next API request is made.

src/sidebar/test/oauth-auth-test.js

@@ -12,6 +12,7 @@ var TOKEN_KEY = 'hypothesis.oauth.hypothes%2Eis.token';
 describe('sidebar.oauth-auth', function () {
 
   var $rootScope;
+  var FakeOAuthClient;
   var auth;
   var nowStub;
   var fakeApiRoutes;
@@ -83,6 +84,13 @@ describe('sidebar.oauth-auth', function () {
       authorize: sinon.stub().returns(Promise.resolve(null)),
     };
 
+    FakeOAuthClient = ($http, config) => {
+      fakeClient.$http = $http;
+      fakeClient.config = config;
+      return fakeClient;
+    };
+    FakeOAuthClient.createLoginPopupWindow = sinon.stub();
+
     fakeWindow = new FakeWindow;
 
     fakeHttp = {};
@@ -94,11 +102,7 @@ describe('sidebar.oauth-auth', function () {
       flash: fakeFlash,
       localStorage: fakeLocalStorage,
       settings: fakeSettings,
-      OAuthClient: ($http, config) => {
-        fakeClient.$http = $http;
-        fakeClient.config = config;
-        return fakeClient;
-      },
+      OAuthClient: FakeOAuthClient,
     });
 
     angular.mock.inject((_auth_, _$rootScope_) => {
@@ -499,8 +503,10 @@ describe('sidebar.oauth-auth', function () {
     });
 
     it('calls OAuthClient#authorize', () => {
+      var fakePopup = {};
+      FakeOAuthClient.createLoginPopupWindow.returns(fakePopup);
       return auth.login().then(() => {
-        assert.calledWith(fakeClient.authorize, fakeWindow);
+        assert.calledWith(fakeClient.authorize, fakeWindow, fakePopup);
       });
     });
 

src/sidebar/util/oauth-client.js

@@ -151,10 +151,12 @@ class OAuthClient {
    *
    * Returns an authorization code which can be passed to `exchangeAuthCode`.
    *
-   * @param {Window} $window
+   * @param {Window} $window - Window which will receive the auth response.
+   * @param {Window} authWindow - Popup window where the login prompt will be shown.
+   *   This should be created using `createLoginPopupWindow`.
    * @return {Promise<string>}
    */
-  authorize($window) {
+  authorize($window, authWindow) {
     // Random state string used to check that auth messages came from the popup
     // window that we opened.
     var state = this.generateState();
@@ -184,41 +186,6 @@ class OAuthClient {
     });
 
     // Authorize user and retrieve grant token
-
-    // In Chrome & Firefox the sizes passed to `window.open` are used for the
-    // viewport size. In Safari the size is used for the window size including
-    // title bar etc. There is enough vertical space at the bottom to allow for
-    // this.
-    //
-    // See https://bugs.webkit.org/show_bug.cgi?id=143678
-    var width  = 475;
-    var height = 430;
-    var left   = $window.screen.width / 2 - width / 2;
-    var top    = $window.screen.height /2 - height / 2;
-
-    // Generate settings for `window.open` in the required comma-separated
-    // key=value format.
-    var authWindowSettings = queryString.stringify({
-      left: left,
-      top: top,
-      width: width,
-      height: height,
-    }).replace(/&/g, ',');
-
-    // Open the auth window before fetching the `oauth.authorize` URL to ensure
-    // that the `window.open` call happens in the same turn of the event loop
-    // that was initiated by the user clicking the "Log in" link.
-    //
-    // Otherwise the `window.open` call is not deemed to be in response to a
-    // user gesture in Firefox & IE 11 and their popup blocking heuristics will
-    // prevent the window being opened. See
-    // https://github.com/hypothesis/client/issues/534 and
-    // https://github.com/hypothesis/client/issues/535.
-    //
-    // Chrome, Safari & Edge have different heuristics and are not affected by
-    // this problem.
-    var authWindow = $window.open('about:blank', 'Login to Hypothesis', authWindowSettings);
-
     var authUrl = this.authorizationEndpoint;
     authUrl += '?' + queryString.stringify({
       client_id: this.clientId,
@@ -245,6 +212,41 @@ class OAuthClient {
     };
     return this.$http.post(url, data, requestConfig);
   }
+
+  /**
+   * Create a pop-up window for use with `OAuthClient#authorize`.
+   *
+   * This function _must_ be called in the same turn of the event loop as the
+   * button or link which initiates login to avoid triggering the popup blocker
+   * in certain browsers. See https://github.com/hypothesis/client/issues/534
+   * and https://github.com/hypothesis/client/issues/535.
+   *
+   * @param {Window} $window - The parent of the popup window.
+   * @return {Window}
+   */
+  static createLoginPopupWindow($window) {
+    // In Chrome & Firefox the sizes passed to `window.open` are used for the
+    // viewport size. In Safari the size is used for the window size including
+    // title bar etc. There is enough vertical space at the bottom to allow for
+    // this.
+    //
+    // See https://bugs.webkit.org/show_bug.cgi?id=143678
+    var width  = 475;
+    var height = 430;
+    var left   = $window.screen.width / 2 - width / 2;
+    var top    = $window.screen.height /2 - height / 2;
+
+    // Generate settings for `window.open` in the required comma-separated
+    // key=value format.
+    var authWindowSettings = queryString.stringify({
+      left: left,
+      top: top,
+      width: width,
+      height: height,
+    }).replace(/&/g, ',');
+
+    return $window.open('about:blank', 'Login to Hypothesis', authWindowSettings);
+  }
 }
 
 module.exports = OAuthClient;

src/sidebar/util/test/oauth-client-test.js

@@ -162,6 +162,20 @@ describe('sidebar.util.oauth-client', () => {
     });
   });
 
+  describe('.createLoginPopupWindow', () => {
+    it('creates and returns the popup window', () => {
+      var fakeWindow = new FakeWindow;
+      var popupWindow = OAuthClient.createLoginPopupWindow(fakeWindow);
+      assert.equal(popupWindow, fakeWindow.open.returnValues[0]);
+      assert.calledWith(
+        fakeWindow.open,
+        'about:blank',
+        'Login to Hypothesis',
+        'height=430,left=274.5,top=169,width=475'
+      );
+    });
+  });
+
   describe('#authorize', () => {
     var fakeWindow;
 
@@ -169,8 +183,14 @@ describe('sidebar.util.oauth-client', () => {
       fakeWindow = new FakeWindow;
     });
 
-    it('opens a popup window at the authorization URL', () => {
-      var authorized = client.authorize(fakeWindow);
+    function authorize() {
+      var popupWindow = OAuthClient.createLoginPopupWindow(fakeWindow);
+      var authorized = client.authorize(fakeWindow, popupWindow);
+      return { authorized, popupWindow };
+    }
+
+    it('navigates the popup window to the authorization URL', () => {
+      var { authorized, popupWindow } = authorize();
 
       fakeWindow.sendMessage({
         type: 'authorization_response',
@@ -187,22 +207,12 @@ describe('sidebar.util.oauth-client', () => {
           state: 'notrandom',
         };
         var expectedAuthUrl = `${config.authorizationEndpoint}?${stringify(params)}`;
-        // Check that the auth window was opened and then set to the expected
-        // location. The final URL is not passed to `window.open` to work around
-        // a pop-up blocker issue.
-        assert.calledWith(
-          fakeWindow.open,
-          'about:blank',
-          'Login to Hypothesis',
-          'height=430,left=274.5,top=169,width=475'
-        );
-        var authPopup = fakeWindow.open.returnValues[0];
-        assert.equal(authPopup.location.href, expectedAuthUrl);
+        assert.equal(popupWindow.location.href, expectedAuthUrl);
       });
     });
 
     it('resolves with an auth code if successful', () => {
-      var authorized = client.authorize(fakeWindow);
+      var { authorized } = authorize();
 
       fakeWindow.sendMessage({
         type: 'authorization_response',
@@ -216,7 +226,7 @@ describe('sidebar.util.oauth-client', () => {
     });
 
     it('rejects with an error if canceled', () => {
-      var authorized = client.authorize(fakeWindow);
+      var { authorized } = authorize();
 
       fakeWindow.sendMessage({
         type: 'authorization_canceled',
@@ -229,7 +239,7 @@ describe('sidebar.util.oauth-client', () => {
     });
 
     it('ignores responses with incorrect "state" values', () => {
-      var authorized = client.authorize(fakeWindow);
+      var { authorized } = authorize();
 
       fakeWindow.sendMessage({
         type: 'authorization_response',