Re-implement excerpt overflow state recalculation

Previously the overflowing state of the excerpt was computed implicitly
by the overflowing() method, which was called from several other
controller functions.

There were several problems with this approach:

 1. Since this method calls Element.scrollHeight, it triggered an
    expensive synchronous layout flush.
 2. In certain situations, this could trigger a loop where the
    overflowing flag was continually flipped (see #2960)
 3  It was unpredictable when the overflowing state would be recomputed.

This commit instead opts for a different approach which is more explicit
about when the state is recomputed, based on the assumption that the
<excerpt>'s content size is a function of:

 1. The inputs to the <excerpt>'s content (eg. the text of a quote, the body
    of an annotation)
 2. The sidebar window's size
 3. The dimensions of any embedded media

The content size is then computed and updated as follows:

 1. When the <excerpt> is initially created, an async computation of
    the scrollHeight is scheduled. The calculation unfortunately needs
    to be async because the content may contain Angular directives which
    take several $digest cycles to fully process.

 2. The <excerpt> listens for changes to the content's data (via a
    'contentData' property), media 'load' events and window 'resize'
    events and recomputes the overflow state in response to any of
    these.

There are a couple of other fixes here:

 - Use an optional boolean property for the 'enabled' input,
   rather than a function. The app passed a function to this property
   but the tests passed a boolean.

Fixes #2960

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 () {
@@ -68,23 +68,105 @@ function excerpt() {
     controller: ExcerptController,
     controllerAs: 'vm',
     link: function (scope, elem, attrs, ctrl) {
-      var contentElem;
+      var pendingOverflowCheck = false;
+
+      // Return the content element of the excerpt.
+      // This changes when the enabled state of the excerpt changes.
+      function getContentElement() {
+        return elem[0].querySelector('.excerpt');
+      }
+
+      // Listen for events which might cause the size of the excerpt's
+      // content to change, even if the content data has not changed.
+
+      // This currently includes top-level window resize events and media
+      // (images, iframes) within the content loading.
+      elem[0].addEventListener('load', scheduleOverflowCheck,
+        true /* capture. 'load' events do not bubble */);
+
+      window.addEventListener('resize', scheduleOverflowCheck);
+
+      /**
+       * Recompute whether the excerpt's content is overflowing the collapsed
+       * element.
+       *
+       * This check is scheduled manually in response to changes in the inputs
+       * to this component and certain events to avoid excessive layout flushes
+       * caused by accessing the element's size.
+       */
+      function recomputeOverflowState() {
+        if (!pendingOverflowCheck) {
+          return;
+        }
+
+        pendingOverflowCheck = false;
 
-      ctrl.contentStyle = function contentStyle() {
+        var contentElem = getContentElement();
         if (!contentElem) {
+          return;
+        }
+
+        var overflowing = false;
+        if (ctrl.enabled) {
+          var hysteresisPx = ctrl.overflowHysteresis || 0;
+          overflowing = contentElem.scrollHeight >
+                 (ctrl.collapsedHeight + hysteresisPx);
+        }
+        if (overflowing === ctrl.overflowing) {
+          return;
+        }
+
+        ctrl.overflowing = overflowing;
+        if (ctrl.onCollapsibleChanged) {
+         ctrl.onCollapsibleChanged({collapsible: ctrl.overflowing});
+        }
+      }
+
+      scope.$on('$destroy', function () {
+        pendingOverflowCheck = false;
+        window.removeEventListener('resize', scheduleOverflowCheck);
+      });
+
+      // Schedule a deferred check of whether the content is collapsed.
+      function scheduleOverflowCheck() {
+        if (pendingOverflowCheck) {
+          return;
+        }
+        pendingOverflowCheck = true;
+        requestAnimationFrame(function () {
+          recomputeOverflowState();
+          scope.$digest();
+        });
+      }
+
+      ctrl.contentStyle = function () {
+        if (!ctrl.enabled) {
           return {};
         }
 
         var maxHeight = '';
-        if (ctrl.overflowing()) {
+        if (ctrl.overflowing) {
           if (ctrl.collapse) {
             maxHeight = toPx(ctrl.collapsedHeight);
           } else if (ctrl.animate) {
-            // animating the height change requires that the final
+            // Animating the height change requires that the final
             // height be specified exactly, rather than relying on
             // auto height
+            var contentElem = getContentElement();
             maxHeight = toPx(contentElem.scrollHeight);
           }
+        } else if (typeof ctrl.overflowing === 'undefined' &&
+                   ctrl.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 [ctrl.collapsedHeight, ctrl.collapsedHeight +
+          // ctrl.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(ctrl.collapsedHeight);
         }
 
         return {
@@ -92,42 +174,29 @@ function excerpt() {
         };
       };
 
-      ctrl.overflowing = function overflowing() {
-        if (!contentElem) {
-          return false;
-        }
-
-        var hysteresisPx = ctrl.overflowHysteresis | 0;
-        return contentElem.scrollHeight >
-               (ctrl.collapsedHeight + hysteresisPx);
-      };
-
-      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;
-        }
-      });
-
-      scope.$watch('vm.overflowing()', function () {
-        if (ctrl.onCollapsibleChanged) {
-          ctrl.onCollapsibleChanged({collapsible: ctrl.overflowing()});
-        }
-      });
+      // Watch properties which may affect whether the excerpt
+      // needs to be collapsed and recompute the overflow state
+      scope.$watch('vm.contentData', scheduleOverflowCheck);
+      scope.$watch('vm.enabled', scheduleOverflowCheck);
+
+      // 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.
+      scheduleOverflowCheck();
     },
     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.

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

@@ -2,10 +2,24 @@
 
 var angular = require('angular');
 
-var assign = require('core-js/modules/$.object-assign');
 var util = require('./util');
 var excerpt = require('../excerpt');
 
+/**
+ * Wait for an <excerpt> to recompute its overflowing state.
+ *
+ * This happens asynchronously after an <excerpt> is created in order to wait
+ * for Angular directives used by the <excerpt>'s content to fully resolve.
+ *
+ * @return {Promise}
+ */
+function waitForLayout(element) {
+  element.scope.$digest();
+  return new Promise(function (resolve) {
+    window.requestAnimationFrame(resolve);
+  });
+}
+
 describe('excerpt directive', function () {
   var SHORT_DIV = '<div id="foo" style="height:5px;"></div>';
   var TALL_DIV =  '<div id="foo" style="height:200px;">foo bar</div>';
@@ -19,7 +33,7 @@ describe('excerpt directive', function () {
       collapsedHeight: 40,
       inlineControls: false,
     };
-    attrs = assign(defaultAttrs, attrs);
+    attrs = Object.assign(defaultAttrs, attrs);
     return util.createDirective(document, 'excerpt', attrs, {}, content);
   }
 
@@ -27,9 +41,21 @@ describe('excerpt directive', function () {
     return el.querySelector('.excerpt').offsetHeight;
   }
 
+  var defaultRAF = window.requestAnimationFrame;
+
   before(function () {
     angular.module('app', [])
       .directive('excerpt', excerpt.directive);
+
+    // requestAnimationFrame() is used internally by <excerpt>
+    // to schedule overflow state checks
+    window.requestAnimationFrame = function (callback) {
+      setTimeout(callback, 0);
+    };
+  });
+
+  after(function () {
+    window.requestAnimationFrame = defaultRAF;
   });
 
   beforeEach(function () {
@@ -59,8 +85,9 @@ describe('excerpt directive', function () {
     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);
+      return waitForLayout(element).then(function () {
+        assert.isBelow(height(element[0]), 100);
+      });
     });
   });
 
@@ -86,10 +113,11 @@ describe('excerpt directive', function () {
     it('displays inline controls if collapsed', function () {
       var element = excerptDirective({inlineControls: true},
         TALL_DIV);
-      element.scope.$digest();
-      var expandLink = findInlineControl(element[0]);
-      assert.ok(expandLink);
-      assert.equal(expandLink.querySelector('a').textContent, 'More');
+      return waitForLayout(element).then(function () {
+        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 () {
@@ -102,12 +130,13 @@ describe('excerpt directive', function () {
     it('toggles the expanded state when clicked', function () {
       var element = excerptDirective({inlineControls: true},
         TALL_DIV);
-      element.scope.$digest();
-      var expandLink = findInlineControl(element[0]);
-      angular.element(expandLink.querySelector('a')).click();
-      element.scope.$digest();
-      var collapseLink = findInlineControl(element[0]);
-      assert.equal(collapseLink.querySelector('a').textContent, 'Less');
+      return waitForLayout(element).then(function () {
+        var expandLink = findInlineControl(element[0]);
+        angular.element(expandLink.querySelector('a')).click();
+        element.scope.$digest();
+        var collapseLink = findInlineControl(element[0]);
+        assert.equal(collapseLink.querySelector('a').textContent, 'Less');
+      });
     });
   });
 
@@ -126,12 +155,16 @@ 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);
+      return waitForLayout(element).then(function () {
+        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);
+      return waitForLayout(element).then(function () {
+        assert.isAbove(height(element[0]), 100);
+      });
     });
   });
 
@@ -144,7 +177,9 @@ describe('excerpt directive', function () {
           callback: callback,
         }
       }, TALL_DIV);
-      assert.calledWith(callback, true);
+      return waitForLayout(element).then(function () {
+        assert.calledWith(callback, true);
+      });
     });
 
     it('reports false if excerpt is short', function () {
@@ -155,7 +190,9 @@ describe('excerpt directive', function () {
           callback: callback,
         }
       }, SHORT_DIV);
-      assert.calledWith(callback, false);
+      return waitForLayout(element).then(function () {
+        assert.calledWith(callback, false);
+      });
     });
   });
 
@@ -166,8 +203,9 @@ describe('excerpt directive', function () {
         collapsedHeight: 40,
         overflowHysteresis: 10,
       }, slightlyOverflowingDiv);
-      element.scope.$digest();
-      assert.isAbove(height(element[0]), 44);
+      return waitForLayout(element).then(function () {
+        assert.isAbove(height(element[0]), 44);
+      });
     });
 
     it('does collapse if overflow exceeds hysteresis', function () {
@@ -176,8 +214,9 @@ describe('excerpt directive', function () {
         collapsedHeight: 40,
         overflowHysteresis: 10,
       }, overflowingDiv);
-      element.scope.$digest();
-      assert.isBelow(height(element[0]), 50);
+      return waitForLayout(element).then(function () {
+        assert.isBelow(height(element[0]), 50);
+      });
     });
   });
 });

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()"