Improve logic for scrolling sidebar to annotation with a given ID (#3360)

The previous logic contained several errors and omissions which could
result in the card not appearing at the top of the viewport after
scrolling.

 1. If the set of visible threads changed multiple times before the timeout
    that triggered re-measuring of visible thread heights changed, then
    a measured height could be replaced with 0.

    Add checks to catch this.

 2. Account for the target scroll offset changing as a result of
    the document height increasing after the actual heights of
    cards become known and consequently the maximum possible
    scroll offset changes.

 3. Correct calculation of the position that we need to scroll the
    window to in order to have a card positioned at the top
    of the window. For the first card in the list, the position
    should be 0 (as returned by visibleThreads.yOffsetOf(...)),
    and for the Nth card it should be the total height of the N-1
    previous cards.

h/static/scripts/test/widget-controller-test.js

@@ -286,18 +286,14 @@ describe('WidgetController', function () {
 
   describe('when a new annotation is created', function () {
     var windowScroll;
-    var cardListTopEl;
 
     beforeEach(function () {
       $scope.clearSelection = sinon.stub();
       windowScroll = sinon.stub(window, 'scroll');
-      cardListTopEl = $('<div class="js-thread-list-top"></div>');
-      cardListTopEl.appendTo(document.body);
     });
 
     afterEach(function () {
       windowScroll.restore();
-      cardListTopEl.remove();
     });
 
     /**

h/static/scripts/virtual-thread-list.js

@@ -75,10 +75,12 @@ VirtualThreadList.prototype.setRootThread = function (thread) {
  * is used.
  *
  * @param {string} id - The annotation ID or $$tag
- * @param {number?} height - The height of the annotation or undefined to
- *        revert to the default height for this thread.
+ * @param {number} height - The height of the annotation thread.
  */
 VirtualThreadList.prototype.setThreadHeight = function (id, height) {
+  if (isNaN(height) || height <= 0) {
+    throw new Error('Invalid thread height %d', height);
+  }
   this._heights[id] = height;
 };
 

h/static/scripts/widget-controller.js

@@ -36,6 +36,11 @@ module.exports = function WidgetController(
   drafts, groups, rootThread, settings, streamer, streamFilter, store,
   VirtualThreadList
 ) {
+
+  /**
+   * Returns the height of the thread for an annotation if it exists in the view
+   * or undefined otherwise.
+   */
   function getThreadHeight(id) {
     var threadElement = document.getElementById(id);
     if (!threadElement) {
@@ -56,6 +61,10 @@ module.exports = function WidgetController(
     return elementHeight + marginHeight;
   }
 
+  // `visibleThreads` keeps track of the subset of all threads matching the
+  // current filters which are in or near the viewport and the view then renders
+  // only those threads, using placeholders above and below the visible threads
+  // to reserve space for threads which are not actually rendered.
   var visibleThreads = new VirtualThreadList($scope, window, rootThread.thread());
   visibleThreads.on('changed', function (state) {
     $scope.virtualThreadList = {
@@ -66,7 +75,11 @@ module.exports = function WidgetController(
 
     scopeTimeout($scope, function () {
       state.visibleThreads.forEach(function (thread) {
-        visibleThreads.setThreadHeight(thread.id, getThreadHeight(thread.id));
+        var height = getThreadHeight(thread.id);
+        if (!height) {
+          return;
+        }
+        visibleThreads.setThreadHeight(thread.id, height);
       });
     }, 50);
   });
@@ -334,29 +347,33 @@ module.exports = function WidgetController(
   };
 
   /**
-   * Return the offset between the top of the window and the top of the
-   * first annotation card.
+   * Return the vertical scroll offset for the document in order to position the
+   * annotation thread with a given `id` or $$tag at the top-left corner
+   * of the view.
    */
-  function cardListYOffset() {
-    var cardListTopEl = document.querySelector('.js-thread-list-top');
-    return cardListTopEl.getBoundingClientRect().top + window.pageYOffset;
+  function scrollOffset(id) {
+    var maxYOffset = document.body.clientHeight - window.innerHeight;
+    return Math.min(maxYOffset, visibleThreads.yOffsetOf(id));
   }
 
   /** Scroll the annotation with a given ID or $$tag into view. */
   function scrollIntoView(id) {
-    var estimatedYOffset = visibleThreads.yOffsetOf(id);
-    var estimatedPos = estimatedYOffset - cardListYOffset();
-
-    window.scroll(0, estimatedPos);
+    var estimatedYOffset = scrollOffset(id);
+    window.scroll(0, estimatedYOffset);
 
-    // As a result of scrolling the sidebar, the heights of some of the cards
-    // above `id` might change because the initial estimate will be replaced by
-    // the actual known height after a card is rendered.
+    // As a result of scrolling the sidebar, the target scroll offset for
+    // annotation `id` might have changed as a result of:
+    //
+    // 1. Heights of some cards above `id` changing from an initial estimate to
+    //    an actual measured height after the card is rendered.
+    // 2. The height of the document changing as a result of any cards heights'
+    //    changing. This may affect the scroll offset if the original target
+    //    was near to the bottom of the list.
     //
     // So we wait briefly after the view is scrolled then check whether the
     // estimated Y offset changed and if so, trigger scrolling again.
     scopeTimeout($scope, function () {
-      var newYOffset = visibleThreads.yOffsetOf(id);
+      var newYOffset = scrollOffset(id);
       if (newYOffset !== estimatedYOffset) {
         scrollIntoView(id);
       }