Merge af39487 into 654bd8f

gulpTasks/test.js

@@ -55,7 +55,7 @@ gulp.task('testDev', ['watchTest'], function (done) {
 gulp.task('remapCoverage', function (done) {
   return gulp.src(`${COVERAGE_DIR}/coverage-es5.json`)
     .pipe(remapIstanbul({
-      exclude: /(webpack|~\/d3\/|~\/es6-promise\/dist\/|~\/process\/|~\/underscore\/|vertx|~\/coveomagicbox\/|~\/d3-.*\/|~\/modal-box\/|~\/moment\/|~\/pikaday\/|test\/|lib\/|es6-promise|analytics|jstimezonedetect|latinize)/
+      exclude: /(webpack|~\/d3\/|~\/es6-promise\/dist\/|~\/process\/|~\/underscore\/|vertx|~\/coveomagicbox\/|~\/d3-.*\/|~\/modal-box\/|~\/moment\/|~\/pikaday\/|test\/|lib\/|es6-promise|analytics|jstimezonedetect|latinize|whatwg)/
     }))
     .pipe(rename('coverage.json'))
     .pipe(gulp.dest(COVERAGE_DIR));

src/ui/ResultList/ResultList.ts

@@ -119,6 +119,10 @@ export class ResultList extends Component {
      * See also {@link ResultList.options.infiniteScrollPageSize}, {@link ResultList.options.infiniteScrollContainer}
      * and {@link ResultList.options.enableInfiniteScrollWaitingAnimation}.
      *
+     * It is important to specify the {@link ResultList.options.infiniteScrollContainer} manually if you want the scrolling
+     * element to be something else than the default `window` element.
+     * Otherwise, you might get in a weird state where the framework will rapidly trigger multiple successive query.
+     *
      * Default value is `false`.
      */
     enableInfiniteScroll: ComponentOptions.buildBooleanOption({ defaultValue: false }),
@@ -144,6 +148,11 @@ export class ResultList extends Component {
      *
      * This implies that if the framework can find no scrollable parent, it uses the window itself as a scrollable
      * container.
+     *
+     * This heuristic is not perfect, for technical reasons. There are always some corner case CSS combination which the framework will
+     * not be able to detect correctly as 'scrollable'.
+     *
+     * It is highly recommended that you manually set this option if you wish to have something else than `window` be the scrollable element.
      */
     infiniteScrollContainer: ComponentOptions.buildChildHtmlElementOption({ depend: 'enableInfiniteScroll', defaultFunction: (element) => ComponentOptions.findParentScrolling(element) }),
 
@@ -207,6 +216,15 @@ export class ResultList extends Component {
   private fetchingMoreResults: Promise<IQueryResults>;
   private reachedTheEndOfResults = false;
 
+  // This variable serves to block some setup where the framework fails to correctly identify the "real" scrolling container.
+  // Since it's not technically feasible to correctly identify the scrolling container in every possible scenario without some very complex logic, we instead try to add some kind of mechanism to
+  // block runaway requests where UI will keep asking more results in the index, eventually bringing the browser to it's knee.
+  // Those successive request are needed in "displayMoreResults" to ensure we fill the scrolling container correctly.
+  // Since the container is not identified correctly, it is never "full", so we keep asking for more.
+  // It is reset every time the user actually scroll the container manually.
+  private successiveScrollCount = 0;
+  private static MAX_AMOUNT_OF_SUCESSIVE_REQUESTS = 5;
+
   /**
    * Creates a new ResultList component. Binds various event related to queries (e.g., on querySuccess ->
    * renderResults). Binds scroll event if {@link ResultList.options.enableInfiniteScroll} is `true`.
@@ -241,7 +259,10 @@ export class ResultList extends Component {
 
     if (this.options.enableInfiniteScroll) {
       this.handlePageChanged();
-      this.bind.on(<HTMLElement>this.options.infiniteScrollContainer, 'scroll', (e: Event) => this.handleScrollOfResultList());
+      this.bind.on(<HTMLElement>this.options.infiniteScrollContainer, 'scroll', (e: Event) => {
+        this.successiveScrollCount = 0;
+        this.handleScrollOfResultList();
+      });
     }
     this.bind.onQueryState(MODEL_EVENTS.CHANGE_ONE, QUERY_STATE_ATTRIBUTES.FIRST, () => this.handlePageChanged());
 
@@ -339,7 +360,7 @@ export class ResultList extends Component {
   /**
    * Executes a query to fetch new results. After the query returns, renders the new results.
    *
-   * Asserts that there are more results to display by verifying whether t3he last query has returned as many results as
+   * Asserts that there are more results to display by verifying whether the last query has returned as many results as
    * requested.
    *
    * Asserts that the ResultList is not currently fetching results.
@@ -377,7 +398,15 @@ export class ResultList extends Component {
     this.fetchingMoreResults.then(() => {
       this.hideWaitingAnimationForInfiniteScrolling();
       this.fetchingMoreResults = undefined;
-      Defer.defer(() => this.handleScrollOfResultList());
+      Defer.defer(() => {
+        this.successiveScrollCount++;
+        if (this.successiveScrollCount <= ResultList.MAX_AMOUNT_OF_SUCESSIVE_REQUESTS) {
+          this.handleScrollOfResultList();
+        } else {
+          this.logger.info(`Result list has triggered 5 consecutive queries to try and fill up the scrolling container, but it is still unable to do so`);
+          this.logger.info(`Try explicitly setting the 'data-infinite-scroll-container-selector' option on the result list. See : https://coveo.github.io/search-ui/components/resultlist.html#options.infinitescrollcontainer`);
+        }
+      });
     });
   }
 

test/ui/FacetTest.ts

@@ -334,7 +334,7 @@ export function FacetTest() {
         expect(test.cmp.facetSort.activeSort.name.toLowerCase()).toBe('chisquare');
       });
 
-      it('available sort with only no sorts should still work', ()=> {
+      it('available sort with only no sorts should still work', () => {
         test = Mock.optionsComponentSetup<Facet, IFacetOptions>(Facet, {
           field: '@field',
           availableSorts: ['nosort']

test/ui/ResultListTest.ts

@@ -11,6 +11,8 @@ import { ResultLayoutEvents } from '../../src/events/ResultLayoutEvents';
 import { AdvancedComponentSetupOptions } from '../MockEnvironment';
 import { TemplateList } from '../../src/ui/Templates/TemplateList';
 import { QueryBuilder } from '../../src/ui/Base/QueryBuilder';
+import { analyticsActionCauseList } from '../../src/ui/Analytics/AnalyticsActionListMeta';
+import { IQueryResults } from '../../src/rest/QueryResults';
 
 export function ResultListTest() {
   describe('ResultList', () => {
@@ -25,6 +27,108 @@ export function ResultListTest() {
       test = null;
     });
 
+    describe('displayMoreResults', () => {
+
+      beforeEach(() => {
+        // Fill the result list one time first, so we can have more results.
+        Simulate.query(test.env);
+      });
+
+      describe('when returning less than 10 results', () => {
+        let promiseResults: Promise<IQueryResults>;
+        beforeEach(() => {
+          promiseResults = new Promise((resolve, reject) => {
+            resolve(FakeResults.createFakeResults(5));
+          });
+
+          (<jasmine.Spy>test.env.queryController.fetchMore).and.returnValue(promiseResults);
+        });
+
+        it('should stop asking for more results if consecutive calls are queued', () => {
+          test.cmp.displayMoreResults(10);
+          test.cmp.displayMoreResults(10);
+          test.cmp.displayMoreResults(10);
+          expect(test.env.queryController.fetchMore).toHaveBeenCalledTimes(1);
+        });
+
+        it('should stop asking for more results if less results than requested are returned', (done) => {
+          test.cmp.displayMoreResults(10);
+          promiseResults.then(() => {
+            test.cmp.displayMoreResults(10);
+            expect(test.env.queryController.fetchMore).toHaveBeenCalledTimes(1);
+            done();
+          });
+        });
+      });
+
+      describe('when returning 10 or more results', () => {
+        let promiseResults: Promise<IQueryResults>;
+        beforeEach(() => {
+          promiseResults = new Promise((resolve, reject) => {
+            resolve(FakeResults.createFakeResults(10));
+          });
+
+          (<jasmine.Spy>test.env.queryController.fetchMore).and.returnValue(promiseResults);
+        });
+
+        afterEach(() => {
+          promiseResults = null;
+        });
+
+        it('should trigger 10 new result displayed event when fetching more results', (done) => {
+          test.cmp.displayMoreResults(10);
+          let newResultSpy = jasmine.createSpy('newresultspy');
+          $$(test.cmp.element).on(ResultListEvents.newResultDisplayed, newResultSpy);
+          promiseResults.then(() => {
+            expect(newResultSpy).toHaveBeenCalledTimes(10);
+            done();
+          });
+        });
+
+        it('should trigger a single new results displayed event when fetching more results', (done) => {
+          test.cmp.displayMoreResults(10);
+          let newResultsSpy = jasmine.createSpy('newresultsspy');
+          $$(test.cmp.element).on(ResultListEvents.newResultsDisplayed, newResultsSpy);
+          promiseResults.then(() => {
+            // Once when filling the initial result list, another time when displaying more results
+            expect(newResultsSpy).toHaveBeenCalledTimes(2);
+            done();
+          });
+        });
+
+        it('should log an analytics event when more results are returned', (done) => {
+          test.cmp.displayMoreResults(10);
+          promiseResults.then(() => {
+            expect(test.env.usageAnalytics.logCustomEvent).toHaveBeenCalledWith(analyticsActionCauseList.pagerScrolling, jasmine.any(Object), test.cmp.element);
+            done();
+          });
+        });
+
+        it('should queue up another scroll when it receives results to fill up the container, if infinite scrolling is enabled', (done) => {
+          test.cmp.options.enableInfiniteScroll = true;
+          test.cmp.displayMoreResults(10);
+          promiseResults.then(() => {
+            setTimeout(() => {
+              expect(test.env.queryController.fetchMore).toHaveBeenCalled();
+              done();
+            }, 1000);
+          });
+        });
+
+        it('should not queue up infinite amount of request if it is trying to fill up the scrolling container', (done) => {
+          test.cmp.options.enableInfiniteScroll = true;
+          test.cmp.displayMoreResults(10);
+          promiseResults.then(() => {
+            setTimeout(() => {
+              // Once at the initial request, + 5 (ResultList.MAX_AMOUNT_OF_SUCESSIVE_REQUESTS)
+              expect(test.env.queryController.fetchMore).toHaveBeenCalledTimes(6);
+              done();
+            }, 1000);
+          });
+        });
+      });
+    });
+
     it('should allow to return the currently displayed result', () => {
       expect(ResultList.resultCurrentlyBeingRendered).toBeNull();
       let data = FakeResults.createFakeResult();