Merge pull request #3188 from hypothesis/274-excerpt-overflow-checking

Re-implement collapsible excerpt overflow state checking

h/static/scripts/app.js

@@ -204,6 +204,7 @@ module.exports = angular.module('h', [
   .value('AnnotationSync', require('./annotation-sync'))
   .value('AnnotationUISync', require('./annotation-ui-sync'))
   .value('Discovery', require('./discovery'))
+  .value('ExcerptOverflowMonitor', require('./directive/excerpt-overflow-monitor'))
   .value('raven', require('./raven'))
   .value('settings', settings)
   .value('time', require('./time'))

h/static/scripts/directive/annotation.js

@@ -698,12 +698,19 @@ function AnnotationController(
     return persona.username(domainModel.user);
   };
 
-  /** Sets whether or not the controls for
-   * expanding/collapsing the body of lengthy annotations
-   * should be shown.
+  /**
+   * Sets whether or not the controls for expanding/collapsing the body of
+   * lengthy annotations should be shown.
    */
-  vm.setBodyCollapsible = function(canCollapse) {
+  vm.setBodyCollapsible = function (canCollapse) {
+    if (canCollapse === vm.canCollapseBody) {
+      return;
+    }
     vm.canCollapseBody = canCollapse;
+
+    // This event handler is called from outside the digest cycle, so
+    // explicitly trigger a digest.
+    $scope.$digest();
   };
 
   init();
@@ -768,7 +775,7 @@ function link(scope, elem, attrs, controllers) {
   *
   */
 // @ngInject
-function annotation($document) {
+function annotation() {
   return {
     restrict: 'E',
     bindToController: true,

h/static/scripts/directive/excerpt-overflow-monitor.js

+'use strict';
+
+function toPx(val) {
+  return val.toString() + 'px';
+}
+
+/**
+ * Interface used by ExcerptOverflowMonitor to retrieve the state of the
+ * <excerpt> and report when the state changes.
+ *
+ * interface Excerpt {
+ *   getState(): State;
+ *   contentHeight(): number | undefined;
+ *   onOverflowChanged(): void;
+ * }
+ */
+
+/**
+ * A helper for the <excerpt> component which handles determinination of the
+ * overflow state and content styling given the current state of the component
+ * and the height of its contents.
+ *
+ * When the state of the excerpt or its content changes, the component should
+ * call check() to schedule an async update of the overflow state.
+ *
+ * @param {Excerpt} excerpt - Interface used to query the current state of the
+ *        excerpt and notify it when the overflow state changes.
+ * @param {(callback) => number} requestAnimationFrame -
+ *        Function called to schedule an async recalculation of the overflow
+ *        state.
+ */
+function ExcerptOverflowMonitor(excerpt, requestAnimationFrame) {
+  var pendingUpdate = false;
+
+  // Last-calculated overflow state
+  var prevOverflowing;
+
+  function update() {
+    var state = excerpt.getState();
+
+    if (!pendingUpdate) {
+      return;
+    }
+
+    pendingUpdate = false;
+
+    var overflowing = false;
+    if (state.enabled) {
+      var hysteresisPx = state.overflowHysteresis || 0;
+      overflowing = excerpt.contentHeight() >
+        (state.collapsedHeight + hysteresisPx);
+    }
+    if (overflowing === prevOverflowing) {
+      return;
+    }
+
+    prevOverflowing = overflowing;
+    excerpt.onOverflowChanged(overflowing);
+  }
+
+  /**
+   * Schedule a deferred check of whether the content is collapsed.
+   */
+  function check() {
+    if (pendingUpdate) {
+      return;
+    }
+    pendingUpdate = true;
+    requestAnimationFrame(update);
+  }
+
+  /**
+   * Returns an object mapping CSS properties to values that should be applied
+   * to an excerpt's content element in order to truncate it based on the
+   * current overflow state.
+   */
+  function contentStyle() {
+    var state = excerpt.getState();
+    if (!state.enabled) {
+      return {};
+    }
+
+    var maxHeight = '';
+    if (prevOverflowing) {
+      if (state.collapse) {
+        maxHeight = toPx(state.collapsedHeight);
+      } else if (state.animate) {
+        // Animating the height change requires that the final
+        // height be specified exactly, rather than relying on
+        // auto height
+        maxHeight = toPx(excerpt.contentHeight());
+      }
+    } else if (typeof prevOverflowing === 'undefined' &&
+               state.collapse) {
+      // If the excerpt is collapsed but the overflowing state has not yet
+      // been computed then the exact max height is unknown, but it will be
+      // in the range [state.collapsedHeight, state.collapsedHeight +
+      // state.overflowHysteresis]
+      //
+      // Here we guess that the final content height is most likely to be
+      // either less than `collapsedHeight` or more than `collapsedHeight` +
+      // `overflowHysteresis`, in which case it will be truncated to
+      // `collapsedHeight`.
+      maxHeight = toPx(state.collapsedHeight);
+    }
+
+    return {
+      'max-height': maxHeight,
+    };
+  }
+
+  this.contentStyle = contentStyle;
+  this.check = check;
+}
+
+module.exports = ExcerptOverflowMonitor;

h/static/scripts/directive/excerpt.js

@@ -9,16 +9,16 @@ function ExcerptController() {
     this.animate = true;
   }
 
-  this.enabled = this.enabled || function () {
-    return true;
-  };
+  if (this.enabled === undefined) {
+    this.enabled = true;
+  }
 
   this.isExpandable = function () {
-    return this.overflowing() && this.collapse;
+    return this.overflowing && this.collapse;
   };
 
   this.isCollapsible = function () {
-    return this.overflowing() && !this.collapse;
+    return this.overflowing && !this.collapse;
   };
 
   this.toggle = function (event) {
@@ -37,7 +37,7 @@ function ExcerptController() {
   };
 
   this.showInlineControls = function () {
-    return this.overflowing() && this.inlineControls;
+    return this.overflowing && this.inlineControls;
   };
 
   this.bottomShadowStyles = function () {
@@ -49,10 +49,6 @@ function ExcerptController() {
   };
 }
 
-function toPx(val) {
-  return val.toString() + 'px';
-}
-
 /**
  * @ngdoc directive
  * @name excerpt
@@ -62,72 +58,97 @@ function toPx(val) {
  *              and collapsing the resulting truncated element.
  */
 // @ngInject
-function excerpt() {
+function excerpt(ExcerptOverflowMonitor) {
   return {
     bindToController: true,
     controller: ExcerptController,
     controllerAs: 'vm',
     link: function (scope, elem, attrs, ctrl) {
-      var contentElem;
-
-      ctrl.contentStyle = function contentStyle() {
-        if (!contentElem) {
-          return {};
+      // Test if the element or any of its parents have been hidden by
+      // an 'ng-show' directive
+      function isElementHidden() {
+        var el = elem[0];
+        while (el) {
+          if (el.classList.contains('ng-hide')) {
+            return true;
           }
-
-        var maxHeight = '';
-        if (ctrl.overflowing()) {
-          if (ctrl.collapse) {
-            maxHeight = toPx(ctrl.collapsedHeight);
-          } else if (ctrl.animate) {
-            // animating the height change requires that the final
-            // height be specified exactly, rather than relying on
-            // auto height
-            maxHeight = toPx(contentElem.scrollHeight);
+          el = el.parentElement;
         }
+        return false;
       }
 
+      var overflowMonitor = new ExcerptOverflowMonitor({
+        getState: function () {
           return {
-          'max-height': maxHeight,
+            enabled: ctrl.enabled,
+            animate: ctrl.animate,
+            collapsedHeight: ctrl.collapsedHeight,
+            collapse: ctrl.collapse,
+            overflowHysteresis: ctrl.overflowHysteresis,
           };
-      };
-
-      ctrl.overflowing = function overflowing() {
+        },
+        contentHeight: function () {
+          var contentElem = elem[0].querySelector('.excerpt');
           if (!contentElem) {
-          return false;
+            return;
+          }
+          return contentElem.scrollHeight;
+        },
+        onOverflowChanged: function (overflowing) {
+          ctrl.overflowing = overflowing;
+          if (ctrl.onCollapsibleChanged) {
+            ctrl.onCollapsibleChanged({collapsible: overflowing});
           }
+          // Even though this change happens outside the framework, we use
+          // $digest() rather than $apply() here to avoid a large number of full
+          // digest cycles if many excerpts update their overflow state at the
+          // same time. The onCollapsibleChanged() handler, if any, is
+          // responsible for triggering any necessary digests in parent scopes.
+          scope.$digest();
+        },
+      }, window.requestAnimationFrame);
 
-        var hysteresisPx = ctrl.overflowHysteresis | 0;
-        return contentElem.scrollHeight >
-               (ctrl.collapsedHeight + hysteresisPx);
-      };
+      ctrl.contentStyle = overflowMonitor.contentStyle;
 
-      scope.$watch('vm.enabled()', function (isEnabled) {
-        if (isEnabled) {
-          contentElem = elem[0].querySelector('.excerpt');
-
-          // trigger an update of the excerpt when events happen
-          // outside of Angular's knowledge that might affect the content
-          // size. For now, the only event we handle is loading of
-          // embedded media or frames
-          contentElem.addEventListener('load', scope.$digest.bind(scope),
-            true /* capture. 'load' events do not bubble */);
-        } else {
-          contentElem = undefined;
-        }
+      // Listen for document events which might affect whether the excerpt
+      // is overflowing, even if its content has not changed.
+      elem[0].addEventListener('load', overflowMonitor.check, false /* capture */);
+      window.addEventListener('resize', overflowMonitor.check);
+      scope.$on('$destroy', function () {
+        window.removeEventListener('resize', overflowMonitor.check);
       });
 
-      scope.$watch('vm.overflowing()', function () {
-        if (ctrl.onCollapsibleChanged) {
-          ctrl.onCollapsibleChanged({collapsible: ctrl.overflowing()});
+      // Watch for changes to the visibility of the excerpt.
+      // Unfortunately there is no DOM API for this, so we rely on a digest
+      // being triggered after the visibility changes.
+      scope.$watch(isElementHidden, function (hidden) {
+        if (!hidden) {
+          overflowMonitor.check();
         }
       });
+
+      // Watch input properties which may affect the overflow state
+      scope.$watch('vm.contentData', overflowMonitor.check);
+      scope.$watch('vm.enabled', overflowMonitor.check);
+
+      // Trigger an initial calculation of the overflow state.
+      //
+      // This is performed asynchronously so that the content of the <excerpt>
+      // has settled - ie. all Angular directives have been fully applied and
+      // the DOM has stopped changing. This may take several $digest cycles.
+      overflowMonitor.check();
     },
     scope: {
       /** Whether or not expansion should be animated. Defaults to true. */
       animate: '<?',
+      /**
+       * The data which is used to generate the excerpt's content.
+       * When this changes, the excerpt will recompute whether the content
+       * is overflowing.
+       */
+      contentData: '<',
       /** Whether or not truncation should be enabled */
-      enabled: '&?',
+      enabled: '<?',
       /**
        * Specifies whether controls to expand and collapse
        * the excerpt should be shown inside the <excerpt> component.
@@ -137,8 +158,12 @@ function excerpt() {
       inlineControls: '<',
       /** Sets whether or not the excerpt is collapsed. */
       collapse: '=?',
-      /** Called when the collapsibility of the excerpt (that is, whether or
+      /**
+       * Called when the collapsibility of the excerpt (that is, whether or
        * not the content height exceeds the collapsed height), changes.
+       *
+       * Note: This function is *not* called from inside a digest cycle,
+       * the caller is responsible for triggering any necessary digests.
        */
       onCollapsibleChanged: '&?',
       /** The height of this container in pixels when collapsed.

h/static/scripts/directive/test/excerpt-overflow-monitor-test.js

+'use strict';
+
+var ExcerptOverflowMonitor = require('../excerpt-overflow-monitor');
+
+describe('ExcerptOverflowMonitor', function () {
+  var contentHeight;
+  var ctrl;
+  var monitor;
+  var state;
+
+  beforeEach(function () {
+    contentHeight = 0;
+
+    state = {
+      enabled: true,
+      animate: true,
+      collapsedHeight: 100,
+      collapse: true,
+      overflowHysteresis: 20,
+    };
+
+    ctrl = {
+      getState: function () { return state; },
+      contentHeight: function () { return contentHeight; },
+      onOverflowChanged: sinon.stub(),
+    };
+
+    monitor = new ExcerptOverflowMonitor(ctrl, function (callback) {
+      callback();
+    });
+  });
+
+  context('when enabled', function () {
+    it('overflows if height > collaped height + hysteresis', function () {
+      contentHeight = 200;
+      monitor.check();
+      assert.calledWith(ctrl.onOverflowChanged, true);
+    });
+
+    it('does not overflow if height < collapsed height', function () {
+      contentHeight = 80;
+      monitor.check();
+      assert.calledWith(ctrl.onOverflowChanged, false);
+    });
+
+    it('does not overflow if height is in [collapsed height, collapsed height + hysteresis]', function () {
+      contentHeight = 110;
+      monitor.check();
+      assert.calledWith(ctrl.onOverflowChanged, false);
+    });
+  });
+
+  context('when not enabled', function () {
+    beforeEach(function () {
+      state.enabled = false;
+    });
+
+    it('does not overflow if height > collapsed height + hysteresis', function () {
+      contentHeight = 200;
+      monitor.check();
+      assert.calledWith(ctrl.onOverflowChanged, false);
+    });
+  });
+
+  context('#contentStyle', function () {
+    it('sets max-height if collapsed and overflowing', function () {
+      contentHeight = 200;
+      monitor.check();
+      assert.deepEqual(monitor.contentStyle(), {'max-height': '100px'});
+    });
+
+    it('sets max height to empty if not overflowing', function () {
+      contentHeight = 80;
+      monitor.check();
+      assert.deepEqual(monitor.contentStyle(), {'max-height': ''});
+    });
+
+    it('sets max-height if overflow state is unknown', function () {
+      // Before the initial overflow check, the state is unknown
+      assert.deepEqual(monitor.contentStyle(), {'max-height': '100px'});
+    });
+  });
+
+  context('#check', function () {
+    it('calls onOverflowChanged() if state changed', function () {
+      contentHeight = 200;
+      monitor.check();
+
+      ctrl.onOverflowChanged = sinon.stub();
+      contentHeight = 250;
+      monitor.check();
+      assert.notCalled(ctrl.onOverflowChanged);
+    });
+  });
+});

h/static/scripts/directive/test/excerpt-test.js

@@ -2,38 +2,136 @@
 
 var angular = require('angular');
 
-var assign = require('core-js/modules/$.object-assign');
 var util = require('./util');
 var excerpt = require('../excerpt');
 
 describe('excerpt directive', function () {
+  // ExcerptOverflowMonitor fake instance created by the current test
+  var fakeOverflowMonitor;
+
   var SHORT_DIV = '<div id="foo" style="height:5px;"></div>';
   var TALL_DIV =  '<div id="foo" style="height:200px;">foo bar</div>';
 
   function excerptDirective(attrs, content) {
     var defaultAttrs = {
-      // disable animation so that expansion/collapse happens immediately
-      // when the controls are toggled in tests
-      animate: false,
       enabled: true,
+      contentData: 'the content',
       collapsedHeight: 40,
       inlineControls: false,
     };
-    attrs = assign(defaultAttrs, attrs);
+    attrs = Object.assign(defaultAttrs, attrs);
     return util.createDirective(document, 'excerpt', attrs, {}, content);
   }
 
-  function height(el) {
-    return el.querySelector('.excerpt').offsetHeight;
-  }
-
   before(function () {
     angular.module('app', [])
       .directive('excerpt', excerpt.directive);
   });
 
   beforeEach(function () {
+    function FakeOverflowMonitor(ctrl) {
+      fakeOverflowMonitor = this;
+
+      this.ctrl = ctrl;
+      this.check = sinon.stub();
+      this.contentStyle = sinon.stub().returns({});
+    }
+
     angular.mock.module('app');
+    angular.mock.module(function ($provide) {
+      $provide.value('ExcerptOverflowMonitor', FakeOverflowMonitor);
+    });
+  });
+
+  context('when created', function () {
+    it('schedules an overflow state recalculation', function () {
+      excerptDirective({}, '<span id="foo"></span>');
+      assert.called(fakeOverflowMonitor.check);
+    });
+
+    it('passes input properties to overflow state recalc', function () {
+      var attrs = {
+        animate: false,
+        enabled: true,
+        collapsedHeight: 40,
+        inlineControls: false,
+        overflowHysteresis: 20,
+      };
+      excerptDirective(attrs, '<span></span>');
+      assert.deepEqual(fakeOverflowMonitor.ctrl.getState(), {
+        animate: attrs.animate,
+        enabled: attrs.enabled,
+        collapsedHeight: attrs.collapsedHeight,
+        collapse: true,
+        overflowHysteresis: attrs.overflowHysteresis,
+      });
+    });
+
+    it('reports the content height to ExcerptOverflowMonitor', function () {
+      excerptDirective({}, TALL_DIV);
+      assert.deepEqual(fakeOverflowMonitor.ctrl.contentHeight(), 200);
+    });
+  });
+
+  context('input changes', function () {
+    it('schedules an overflow state check when inputs change', function () {
+      var element = excerptDirective({}, '<span></span>');
+      fakeOverflowMonitor.check.reset();
+      element.scope.contentData = 'new-content';
+      element.scope.$digest();
+      assert.calledOnce(fakeOverflowMonitor.check);
+    });
+
+    it('does not schedule a state check if inputs are unchanged', function () {
+      var element = excerptDirective({}, '<span></span>');
+      fakeOverflowMonitor.check.reset();
+      element.scope.$digest();
+      assert.notCalled(fakeOverflowMonitor.check);
+    });
+  });
+
+  context('document events', function () {
+    it('schedules an overflow check when media loads', function () {
+      var element = excerptDirective({}, '<img src="https://example.com/foo.jpg">');
+      fakeOverflowMonitor.check.reset();
+      util.sendEvent(element[0], 'load');
+      assert.called(fakeOverflowMonitor.check);
+    });
+
+    it('schedules an overflow check when the window is resized', function () {
+      var element = excerptDirective({}, '<span></span>');
+      fakeOverflowMonitor.check.reset();
+      util.sendEvent(element[0].ownerDocument.defaultView, 'resize');
+      assert.called(fakeOverflowMonitor.check);
+    });
+  });
+
+  context('visibility changes', function () {
+    it('schedules an overflow check when shown', function () {
+      var element = excerptDirective({}, '<span></span>');
+      fakeOverflowMonitor.check.reset();
+
+      // ng-hide is the class used by the ngShow and ngHide directives
+      // to show or hide elements. For now, this is the only way of hiding
+      // or showing excerpts that we need to support.
+      element[0].classList.add('ng-hide');
+      element.scope.$digest();
+      assert.notCalled(fakeOverflowMonitor.check);
+
+      element[0].classList.remove('ng-hide');
+      element.scope.$digest();
+      assert.called(fakeOverflowMonitor.check);
+    });
+  });
+
+  context('excerpt content style', function () {
+    it('sets the content style using ExcerptOverflowMonitor#contentStyle()', function () {
+      var element = excerptDirective({}, '<span></span>');
+      fakeOverflowMonitor.contentStyle.returns({'max-height': '52px'});
+      element.scope.$digest();
+      var content = element[0].querySelector('.excerpt');
+      assert.equal(content.style.cssText.trim(), 'max-height: 52px;');
+    });
   });
 
   describe('enabled state', function () {
@@ -55,13 +153,6 @@ describe('excerpt directive', function () {
       assert.equal(element.find('.excerpt #foo').length, 0);
       assert.equal(element.find('#foo').length, 1);
     });
-
-    it('truncates long contents when enabled', function () {
-      var element = excerptDirective({enabled: false}, TALL_DIV);
-      element.scope.enabled = true;
-      element.scope.$digest();
-      assert.isBelow(height(element[0]), 100);
-    });
   });
 
   function isHidden(el) {
@@ -86,23 +177,21 @@ describe('excerpt directive', function () {
     it('displays inline controls if collapsed', function () {
       var element = excerptDirective({inlineControls: true},
         TALL_DIV);
-      element.scope.$digest();
+      fakeOverflowMonitor.ctrl.onOverflowChanged(true);
       var expandLink = findInlineControl(element[0]);
       assert.ok(expandLink);
       assert.equal(expandLink.querySelector('a').textContent, 'More');
     });
 
     it('does not display inline controls if not collapsed', function () {
-      var element = excerptDirective({inlineControls: true},
-        SHORT_DIV);
+      var element = excerptDirective({inlineControls: true}, SHORT_DIV);
       var expandLink = findInlineControl(element[0]);
       assert.notOk(expandLink);
     });
 
     it('toggles the expanded state when clicked', function () {
-      var element = excerptDirective({inlineControls: true},
-        TALL_DIV);
-      element.scope.$digest();
+      var element = excerptDirective({inlineControls: true}, TALL_DIV);
+      fakeOverflowMonitor.ctrl.onOverflowChanged(true);
       var expandLink = findInlineControl(element[0]);
       angular.element(expandLink.querySelector('a')).click();
       element.scope.$digest();
@@ -123,61 +212,19 @@ describe('excerpt directive', function () {
     });
   });
 
-  describe('.collapse', function () {
-    it('collapses the body if collapse is true', function () {
-      var element = excerptDirective({collapse: true}, TALL_DIV);
-      assert.isBelow(height(element[0]), 100);
-    });
-
-    it('does not collapse the body if collapse is false', function () {
-      var element = excerptDirective({collapse: false}, TALL_DIV);
-      assert.isAbove(height(element[0]), 100);
-    });
-  });
-
-  describe('.onCollapsibleChanged', function () {
-    it('reports true if excerpt is tall', function () {
+  describe('#onCollapsibleChanged', function () {
+    it('is called when overflow state changes', function () {
       var callback = sinon.stub();
-      var element = excerptDirective({
+      excerptDirective({
         onCollapsibleChanged: {
           args: ['collapsible'],
           callback: callback,
         }
-      }, TALL_DIV);
+      }, '<span></span>');
+      fakeOverflowMonitor.ctrl.onOverflowChanged(true);
       assert.calledWith(callback, true);
-    });
-
-    it('reports false if excerpt is short', function () {
-      var callback = sinon.stub();
-      var element = excerptDirective({
-        onCollapsibleChanged: {
-          args: ['collapsible'],
-          callback: callback,
-        }
-      }, SHORT_DIV);
+      fakeOverflowMonitor.ctrl.onOverflowChanged(false);
       assert.calledWith(callback, false);
     });
   });
-
-  describe('overflowHysteresis', function () {
-    it('does not collapse if overflow is less than hysteresis', function () {
-      var slightlyOverflowingDiv = '<div class="foo" style="height:45px;"></div>';
-      var element = excerptDirective({
-        collapsedHeight: 40,
-        overflowHysteresis: 10,
-      }, slightlyOverflowingDiv);
-      element.scope.$digest();
-      assert.isAbove(height(element[0]), 44);
-    });
-
-    it('does collapse if overflow exceeds hysteresis', function () {
-      var overflowingDiv = '<div style="height:60px;"></div>';
-      var element = excerptDirective({
-        collapsedHeight: 40,
-        overflowHysteresis: 10,
-      }, overflowingDiv);
-      element.scope.$digest();
-      assert.isBelow(height(element[0]), 50);
-    });
-  });
 });

h/static/scripts/karma-phantomjs-polyfill.js

@@ -11,6 +11,9 @@
 // by all browsers we support (IE >= 10)
 require('core-js/es5');
 
+// PhantomJS 1.x does not support rAF.
+require('raf').polyfill();
+
 // Additional polyfills for newer features.
 // Be careful that any polyfills used here match what is used in the
 // app itself.

h/templates/client/annotation.html

@@ -59,7 +59,8 @@
            ng-if="vm.hasQuotes()">
     <excerpt collapsed-height="40"
              inline-controls="true"
-             overflow-hysteresis="20">
+             overflow-hysteresis="20"
+             content-data="selector.exact">
       <blockquote class="annotation-quote"
                   ng-bind-html="selector.exact"
                   ng-repeat="selector in target.selector
@@ -77,7 +78,8 @@
              on-collapsible-changed="vm.setBodyCollapsible(collapsible)"
              collapse="vm.collapseBody"
              collapsed-height="400"
-             overflow-hysteresis="20">
+             overflow-hysteresis="20"
+             content-data="vm.form.text">
       <markdown ng-model="vm.form.text"
                 read-only="!vm.editing()">
       </markdown>

h/templates/client/excerpt.html

-<div ng-transclude ng-if="!vm.enabled()"></div>
-<div class="excerpt__container" ng-if="vm.enabled()">
+<div ng-transclude ng-if="!vm.enabled"></div>
+<div class="excerpt__container" ng-if="vm.enabled">
   <div class="excerpt" ng-style="vm.contentStyle()">
     <div ng-transclude></div>
     <div ng-click="vm.expand()"