Merge pull request #3240 from hypothesis/remove-timeouts-when-scope-destroyed

Clear pending timeouts when AppController is destroyed

h/static/scripts/app-controller.js

@@ -6,6 +6,7 @@ var scrollIntoView = require('scroll-into-view');
 var annotationMetadata = require('./annotation-metadata');
 var events = require('./events');
 var parseAccountID = require('./filter/persona').parseAccountID;
+var scopeTimeout = require('./util/scope-timeout');
 
 function authStateFromUserID(userid) {
   if (userid) {
@@ -107,7 +108,7 @@ module.exports = function AppController(
   function scrollToView(selector) {
     // Add a timeout so that if the element has just been shown (eg. via ngIf)
     // it is added to the DOM before we try to locate and scroll to it.
-    setTimeout(function () {
+    scopeTimeout($scope, function () {
       scrollIntoView($document[0].querySelector(selector));
     }, 0);
   }

h/static/scripts/directive/annotation-share-dialog.js

@@ -2,6 +2,8 @@
 
 var angular = require('angular');
 
+var scopeTimeout = require('../util/scope-timeout');
+
 module.exports = function () {
   return {
     bindToController: true,
@@ -15,7 +17,7 @@ module.exports = function () {
       $scope.$watch('vm.isOpen', function (isOpen) {
         if (isOpen) {
           // Focus the input and select it once the dialog has become visible
-          setTimeout(function () {
+          scopeTimeout($scope, function () {
             shareLinkInput.focus();
             shareLinkInput.select();
           });
@@ -32,11 +34,9 @@ module.exports = function () {
           // Stop listening for clicks outside the dialog once it is closed.
           // The setTimeout() here is to ignore the initial click that opens
           // the dialog.
-          setTimeout(function () {
-              document.addEventListener('click', hideListener);
-            },
-            0
-          );
+          scopeTimeout($scope, function () {
+            document.addEventListener('click', hideListener);
+          }, 0);
         }
       }.bind(this));
 

h/static/scripts/directive/markdown.js

@@ -6,6 +6,7 @@ var debounce = require('lodash.debounce');
 var commands = require('../markdown-commands');
 var mediaEmbedder = require('../media-embedder');
 var renderMarkdown = require('../render-markdown');
+var scopeTimeout = require('../util/scope-timeout');
 
 /**
  * @ngdoc directive
@@ -49,7 +50,7 @@ module.exports = function($sanitize) {
         // A timeout is used so that focus() is not called until
         // the visibility change has been applied (by adding or removing
         // the relevant CSS classes)
-        setTimeout(function () {
+        scopeTimeout(scope, function () {
           input.focus();
         }, 0);
       }

h/static/scripts/util/scope-timeout.js

+'use strict';
+
+/**
+ * Sets a timeout which is linked to the lifetime of an Angular scope.
+ *
+ * When the scope is destroyed, the timeout will be cleared if it has
+ * not already fired.
+ *
+ * The callback is not invoked within a $scope.$apply() context. It is up
+ * to the caller to do that if necessary.
+ *
+ * @param {Scope} $scope - An Angular scope
+ * @param {Function} fn - Callback to invoke with setTimeout
+ * @param {number} delay - Delay argument to pass to setTimeout
+ */
+module.exports = function ($scope, fn, delay, setTimeoutFn, clearTimeoutFn) {
+  setTimeoutFn = setTimeoutFn || setTimeout;
+  clearTimeoutFn = clearTimeoutFn || clearTimeout;
+
+  var removeDestroyHandler;
+  var id = setTimeoutFn(function () {
+    removeDestroyHandler();
+    fn();
+  }, delay);
+  removeDestroyHandler = $scope.$on('$destroy', function () {
+    clearTimeoutFn(id);
+  });
+};

h/static/scripts/util/test/scope-timeout-test.js

+'use strict';
+
+var scopeTimeout = require('../scope-timeout');
+
+function FakeScope() {
+  this.listeners = {};
+  this.$on = function (event, fn) {
+    this.listeners[event] = this.listeners[event] || [];
+    this.listeners[event].push(fn);
+    return function () {
+      this.listeners[event] = this.listeners[event].filter(function (otherFn) {
+        return otherFn !== fn;
+      });
+    }.bind(this);
+  };
+}
+
+describe('scope-timeout', function () {
+  var fakeSetTimeout;
+  var fakeClearTimeout;
+
+  beforeEach(function () {
+    fakeSetTimeout = sinon.stub().returns(42);
+    fakeClearTimeout = sinon.stub();
+  });
+
+  it('schedules a timeout', function () {
+    var $scope = new FakeScope();
+    var callback = sinon.stub();
+    scopeTimeout($scope, callback, 0, fakeSetTimeout, fakeClearTimeout);
+    assert.calledOnce(fakeSetTimeout);
+    var timeoutFn = fakeSetTimeout.args[0][0];
+    timeoutFn();
+    assert.called(callback);
+  });
+
+  it('removes the scope listener when the timeout fires', function () {
+    var $scope = new FakeScope();
+    var callback = sinon.stub();
+    scopeTimeout($scope, callback, 0, fakeSetTimeout, fakeClearTimeout);
+    assert.equal($scope.listeners.$destroy.length, 1);
+    var timeoutFn = fakeSetTimeout.args[0][0];
+    timeoutFn();
+    assert.equal($scope.listeners.$destroy.length, 0);
+  });
+
+  it('clears the timeout when the scope is destroyed', function () {
+    var $scope = new FakeScope();
+    var callback = sinon.stub();
+    scopeTimeout($scope, callback, 0, fakeSetTimeout, fakeClearTimeout);
+    var destroyFn = $scope.listeners.$destroy[0];
+    destroyFn();
+    assert.calledWith(fakeClearTimeout, 42);
+  });
+});