Merge pull request #327 from hypothesis/mobile-scrolling

Fix Mobile Safari Scrolling

src/sidebar/components/test/thread-list-test.js

@@ -66,6 +66,8 @@ function FakeVirtualThreadList($scope, $window, rootThread) {
 inherits(FakeVirtualThreadList, EventEmitter);
 
 describe('threadList', function () {
+  var threadListContainers;
+
   function createThreadList(inputs) {
     var defaultInputs = {
       thread: threadFixtures.thread,
@@ -76,8 +78,33 @@ describe('threadList', function () {
       onSetCollapsed: sinon.stub(),
     };
 
-    var element = util.createDirective(document, 'threadList',
-      Object.assign({}, defaultInputs, inputs));
+    // Create a scrollable container for the `<thread-list>` so that scrolling
+    // can be tested.
+    var parentEl = document.createElement('div');
+    parentEl.style.overflow = 'scroll';
+    parentEl.style.height = '100px';
+
+    // Add an element inside the scrollable container which is much taller than
+    // the container, so that it actually becomes scrollable.
+    var tallDiv = document.createElement('div');
+    tallDiv.style.height = '1000px';
+    parentEl.appendChild(tallDiv);
+
+    document.body.appendChild(parentEl);
+
+    // Create the `<thread-list>` instance
+    var element = util.createDirective(
+      document,
+      'threadList',
+      Object.assign({}, defaultInputs, inputs),
+      {}, // initialScope
+      '', // initialHtml
+      { parentElement: parentEl }
+    );
+
+    element.parentEl = parentEl;
+
+    threadListContainers.push(parentEl);
 
     return element;
   }
@@ -91,6 +118,13 @@ describe('threadList', function () {
     angular.mock.module('app', {
       VirtualThreadList: FakeVirtualThreadList,
     });
+    threadListContainers = [];
+  });
+
+  afterEach(function () {
+    threadListContainers.forEach(function (el) {
+      el.remove();
+    });
   });
 
   it('displays the children of the root thread', function () {
@@ -102,34 +136,38 @@ describe('threadList', function () {
   });
 
   describe('when a new annotation is created', function () {
-    var scrollSpy;
-    beforeEach(function () {
-      scrollSpy = sinon.stub(window, 'scroll');
-    });
-
-    afterEach(function () {
-      scrollSpy.restore();
-    });
-
     it('scrolls the annotation into view', function () {
       var element = createThreadList();
+      element.parentEl.scrollTop = 500;
+
       var annot = annotFixtures.annotation;
       element.scope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annot);
-      assert.called(scrollSpy);
+
+      // Check that the thread list was scrolled up to make the new annotation
+      // visible.
+      assert.isBelow(element.parentEl.scrollTop, 100);
     });
 
     it('does not scroll the annotation into view if it is a reply', function () {
       var element = createThreadList();
+      element.parentEl.scrollTop = 500;
+
       var reply = annotFixtures.reply;
       element.scope.$broadcast(events.BEFORE_ANNOTATION_CREATED, reply);
-      assert.notCalled(scrollSpy);
+
+      // Check that the thread list was not scrolled
+      assert.equal(element.parentEl.scrollTop, 500);
     });
 
     it('does not scroll the annotation into view if it is a highlight', function () {
       var element = createThreadList();
+      element.parentEl.scrollTop = 500;
+
       var highlight = annotFixtures.highlight;
       element.scope.$broadcast(events.BEFORE_ANNOTATION_CREATED, highlight);
-      assert.notCalled(scrollSpy);
+
+      // Check that the thread list was not scrolled
+      assert.equal(element.parentEl.scrollTop, 500);
     });
 
     it('clears the selection', function () {

src/sidebar/components/thread-list.js

@@ -43,14 +43,40 @@ var virtualThreadOptions = {
   },
 };
 
+/**
+ * Return the closest ancestor of `el` which is scrollable.
+ *
+ * @param {Element} el
+ */
+function closestScrollableAncestor(el) {
+  var parentEl = el;
+  while (parentEl !== document.body) {
+    var computedStyle = window.getComputedStyle(parentEl);
+    if (computedStyle.overflowY === 'scroll') {
+      return parentEl;
+    }
+    parentEl = parentEl.parentElement;
+  }
+  return parentEl;
+}
+
 // @ngInject
-function ThreadListController($scope, VirtualThreadList) {
+function ThreadListController($element, $scope, VirtualThreadList) {
   // `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 self = this;
-  var visibleThreads = new VirtualThreadList($scope, window, this.thread, virtualThreadOptions);
+
+  // `scrollRoot` is the `Element` to scroll when scrolling a given thread into
+  // view.
+  this.scrollRoot = closestScrollableAncestor($element[0]);
+
+  var options = Object.assign({
+    scrollRoot: this.scrollRoot,
+  }, virtualThreadOptions);
+
+  var visibleThreads = new VirtualThreadList($scope, window, this.thread, options);
   visibleThreads.on('changed', function (state) {
     self.virtualThreadList = {
       visibleThreads: state.visibleThreads,
@@ -76,17 +102,14 @@ function ThreadListController($scope, VirtualThreadList) {
    * of the view.
    */
   function scrollOffset(id) {
-    // Note: This assumes that the element occupies the entire height of the
-    // containing document. It would be preferable if only the contents of the
-    // <thread-list> itself scrolled.
-    var maxYOffset = document.body.scrollHeight - window.innerHeight;
+    var maxYOffset = self.scrollRoot.scrollHeight - self.scrollRoot.clientHeight;
     return Math.min(maxYOffset, visibleThreads.yOffsetOf(id));
   }
 
   /** Scroll the annotation with a given ID or $tag into view. */
   function scrollIntoView(id) {
     var estimatedYOffset = scrollOffset(id);
-    window.scroll(0, estimatedYOffset);
+    self.scrollRoot.scrollTop = estimatedYOffset;
 
     // As a result of scrolling the sidebar, the target scroll offset for
     // annotation `id` might have changed as a result of:

src/sidebar/test/virtual-thread-list-test.js

@@ -19,6 +19,7 @@ describe('VirtualThreadList', function () {
   };
 
   var fakeScope;
+  var fakeScrollRoot;
   var fakeWindow;
 
   function idRange(start, end) {
@@ -45,6 +46,25 @@ describe('VirtualThreadList', function () {
   beforeEach(function () {
     fakeScope = {$digest: sinon.stub()};
 
+    fakeScrollRoot = {
+      scrollTop: 0,
+      listeners: {},
+      addEventListener: function (event, listener) {
+        this.listeners[event] = this.listeners[event] || [];
+        this.listeners[event].push(listener);
+      },
+      removeEventListener: function (event, listener) {
+        this.listeners[event] = this.listeners[event].filter(function (fn) {
+          return fn !== listener;
+        });
+      },
+      trigger: function (event) {
+        (this.listeners[event] || []).forEach(function (cb) {
+          cb();
+        });
+      },
+    };
+
     fakeWindow = {
       listeners: {},
       addEventListener: function (event, listener) {
@@ -57,15 +77,15 @@ describe('VirtualThreadList', function () {
         });
       },
       trigger: function (event) {
-        this.listeners[event].forEach(function (cb) {
+        (this.listeners[event] || []).forEach(function (cb) {
           cb();
         });
       },
       innerHeight: 100,
-      pageYOffset: 0,
     };
 
     threadOptions.invisibleThreadFilter = sinon.stub().returns(false);
+    threadOptions.scrollRoot = fakeScrollRoot;
 
     var rootThread = {annotation: undefined, children: []};
     threadList = new VirtualThreadList(fakeScope, fakeWindow, rootThread, threadOptions);
@@ -77,7 +97,7 @@ describe('VirtualThreadList', function () {
   unroll('generates expected state when #when', function (testCase) {
     var thread = generateRootThread(testCase.threads);
 
-    fakeWindow.pageYOffset = testCase.scrollOffset;
+    fakeScrollRoot.scrollTop = testCase.scrollOffset;
     fakeWindow.innerHeight = testCase.windowHeight;
 
     // make sure for everything that is not being presented in the
@@ -93,7 +113,7 @@ describe('VirtualThreadList', function () {
     assert.equal(lastState.offscreenUpperHeight, testCase.expectedHeightAbove);
     assert.equal(lastState.offscreenLowerHeight, testCase.expectedHeightBelow);
   },[{
-    when: 'window is scrolled to top of list',
+    when: 'scrollRoot is scrolled to top of list',
     threads: 100,
     scrollOffset: 0,
     windowHeight: 300,
@@ -101,7 +121,7 @@ describe('VirtualThreadList', function () {
     expectedHeightAbove: 0,
     expectedHeightBelow: 18800,
   },{
-    when: 'window is scrolled to middle of list',
+    when: 'scrollRoot is scrolled to middle of list',
     threads: 100,
     scrollOffset: 2000,
     windowHeight: 300,
@@ -109,7 +129,7 @@ describe('VirtualThreadList', function () {
     expectedHeightAbove: 1000,
     expectedHeightBelow: 16800,
   },{
-    when: 'window is scrolled to bottom of list',
+    when: 'scrollRoot is scrolled to bottom of list',
     threads: 100,
     scrollOffset: 18800,
     windowHeight: 300,
@@ -118,15 +138,17 @@ describe('VirtualThreadList', function () {
     expectedHeightBelow: 0,
   }]);
 
-  unroll('recalculates when a window.#event occurs', function (testCase) {
+  it('recalculates when a window.resize occurs', function () {
     lastState = null;
-    fakeWindow.trigger(testCase.event);
+    fakeWindow.trigger('resize');
     assert.ok(lastState);
-  },[{
-    event: 'resize',
-  },{
-    event: 'scroll',
-  }]);
+  });
+
+  it('recalculates when a scrollRoot.scroll occurs', function () {
+    lastState = null;
+    fakeScrollRoot.trigger('scroll');
+    assert.ok(lastState);
+  });
 
   it('recalculates when root thread changes', function () {
     threadList.setRootThread({annotation: undefined, children: []});
@@ -137,7 +159,7 @@ describe('VirtualThreadList', function () {
     unroll('affects visible threads', function (testCase) {
       var thread = generateRootThread(10);
       fakeWindow.innerHeight = 500;
-      fakeWindow.pageYOffset = 0;
+      fakeScrollRoot.scrollTop = 0;
       idRange(0,10).forEach(function (id) {
         threadList.setThreadHeight(id, testCase.threadHeight);
       });
@@ -154,16 +176,18 @@ describe('VirtualThreadList', function () {
   });
 
   describe('#detach', function () {
-    unroll('stops listening to window.#event events', function (testCase) {
+    it('stops listening to window.resize events', function () {
       threadList.detach();
       lastState = null;
-      fakeWindow.trigger(testCase.event);
+      fakeWindow.trigger('resize');
       assert.isNull(lastState);
-    },[{
-      event: 'resize',
-    },{
-      event: 'scroll',
-    }]);
+    });
+    it('stops listening to scrollRoot.scroll events', function () {
+      threadList.detach();
+      lastState = null;
+      fakeScrollRoot.trigger('scroll');
+      assert.isNull(lastState);
+    });
   });
 
   describe('#yOffsetOf', function () {

src/sidebar/virtual-thread-list.js

@@ -4,6 +4,16 @@ var EventEmitter = require('tiny-emitter');
 var debounce = require('lodash.debounce');
 var inherits = require('inherits');
 
+/**
+ * @typedef Options
+ * @property {Function} [invisibleThreadFilter] - Function used to determine
+ *   whether an off-screen thread should be rendered or not.  Called with a
+ *   `Thread` and if it returns `true`, the thread is rendered even if offscreen.
+ * @property {Element} [scrollRoot] - The scrollable Element which contains the
+ *   thread list. The set of on-screen threads is determined based on the scroll
+ *   position and height of this element.
+ */
+
 /**
  * VirtualThreadList is a helper for virtualizing the annotation thread list.
  *
@@ -20,10 +30,7 @@ var inherits = require('inherits');
  * @param {Window} container - The Window displaying the list of annotation threads.
  * @param {Thread} rootThread - The initial Thread object for the top-level
  *        threads.
- * @param {Object} options - The render-time options to help make final adjustments
- *        to what is and is not rendered.
- *        options.invisibleThreadFilter allows integrator to tell us what should be
- *          rerendered but not visible to the user yet.
+ * @param {Options} options
  */
 function VirtualThreadList($scope, window_, rootThread, options) {
   var self = this;
@@ -36,16 +43,17 @@ function VirtualThreadList($scope, window_, rootThread, options) {
   this._heights = {};
 
   this.window = window_;
+  this.scrollRoot = options.scrollRoot || document.body;
 
   var debouncedUpdate = debounce(function () {
     self._updateVisibleThreads();
     $scope.$digest();
   }, 20);
-  this.window.addEventListener('scroll', debouncedUpdate);
+  this.scrollRoot.addEventListener('scroll', debouncedUpdate);
   this.window.addEventListener('resize', debouncedUpdate);
 
   this._detach = function () {
-    this.window.removeEventListener('scroll', debouncedUpdate);
+    this.scrollRoot.removeEventListener('scroll', debouncedUpdate);
     this.window.removeEventListener('resize', debouncedUpdate);
   };
 }
@@ -153,17 +161,20 @@ VirtualThreadList.prototype._updateVisibleThreads = function () {
   for (var i = 0; i < allThreads.length; i++) {
     thread = allThreads[i];
     var threadHeight = this._height(thread.id);
+
     var added = false;
 
-    if (usedHeight + threadHeight < this.window.pageYOffset - MARGIN_ABOVE) {
+    if (usedHeight + threadHeight < this.scrollRoot.scrollTop - MARGIN_ABOVE) {
       // Thread is above viewport
       offscreenUpperHeight += threadHeight;
     } else if (usedHeight <
-      this.window.pageYOffset + visibleHeight + MARGIN_BELOW) {
+      this.scrollRoot.scrollTop + visibleHeight + MARGIN_BELOW) {
+
       // Thread is either in or close to the viewport
       visibleThreads.push(thread);
       added = true;
     } else {
+
       // Thread is below viewport
       offscreenLowerHeight += threadHeight;
     }

src/styles/app.scss

@@ -57,6 +57,9 @@ hypothesis-app {
   @include grey-background;
 
   min-height: 100%;
+  height: 100%;
+  overflow: scroll;
+  -webkit-overflow-scrolling: touch;
   padding: $sidebar-h-padding;
   padding-top: $sidebar-h-padding + $top-bar-height;
 

src/styles/top-bar.scss

@@ -6,7 +6,7 @@
   border-bottom: solid 1px $gray-lighter;
   height: $top-bar-height;
   font-size: 15px;
-  position: fixed;
+  position: absolute;
   left: 0;
   right: 0;
   top: 0;