PaginatedCollectionView fixes/tweaks

* autoFetch option to fetch as long as the bottom of the collection is
  visible (i.e. don't wait for a scroll). we might want to make this
  default, though my aim ATM is to preserve existing functionality
* fix checkScroll check so we don't fetch if one is in progress
* fix checkScroll so that we fetch when the bottom of the collection is
  visible, rather than requiring that we be at the bottom of the
  scrollContainer (sometimes the latter has more stuff in it)

test plan:
* see specs
* also try out g/21927

Change-Id: I65c2c542edf41a4a70ddba3dc789393bcaf502ca
Reviewed-on: https://gerrit.instructure.com/22194
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Landon Wilkins <lwilkins@instructure.com>
Reviewed-by: Ryan Florence <ryanf@instructure.com>
Product-Review: Jon Jensen <jon@instructure.com>
QA-Review: Jon Jensen <jon@instructure.com>

app/coffeescripts/views/PaginatedCollectionView.coffee

@@ -40,6 +40,11 @@ define [
 
     @optionProperty 'scrollContainer'
 
+    ##
+    # Whether the collection should keep fetching pages until below the
+    # viewport. Defaults to false (i.e. just do one fetch per scroll)
+    @optionProperty 'autoFetch'
+
     template: template
 
     ##
@@ -59,7 +64,10 @@ define [
       @collection.on 'reset', @attachScroll
       @collection.on 'fetched:last', @detachScroll
       @collection.on 'beforeFetch', @showLoadingIndicator
-      @collection.on 'fetch', @hideLoadingIndicator
+      if @autoFetch
+        @collection.on 'fetch', => setTimeout @checkScroll # next tick so events don't stomp on each other
+      else
+        @collection.on 'fetch', @hideLoadingIndicator
 
     ##
     # Sets instance properties regarding the scrollContainer
@@ -69,10 +77,10 @@ define [
     initScrollContainer: ->
       @scrollContainer = $ @scrollContainer
       @heightContainer = if @scrollContainer[0] is window
-        # window has no scrollHeight
-        document.body
+        # window has no position
+        $ document.body
       else
-        @scrollContainer[0]
+        @scrollContainer
 
     ##
     # Attaches scroll event to scrollContainer
@@ -99,12 +107,17 @@ define [
     # @api public
 
     checkScroll: =>
-      return if @fetchingPage
-      distanceToBottom = @heightContainer.scrollHeight -
+      return if @collection.fetchingPage or @collection.fetchingNextPage
+      elementBottom = @$el.position().top +
+        @$el.height() -
+        @heightContainer.position().top
+      distanceToBottom = elementBottom -
         @scrollContainer.scrollTop() -
         @scrollContainer.height()
       if distanceToBottom < @options.buffer
         @collection.fetch page: 'next'
+      else
+        @hideLoadingIndicator()
 
     ##
     # Remove scroll event if view is removed

spec/coffeescripts/views/PaginatedCollectionViewSpec.coffee

@@ -6,6 +6,7 @@ define [
 ], ($, PaginatedCollection, PaginatedCollectionView, fakePage) ->
 
   server = null
+  clock = null
   collection = null
   view = null
   fixtures = $ '#fixtures'
@@ -23,15 +24,16 @@ define [
     template: ({id}) -> id
     initialize: ->
       # make some scrolly happen
-      @$el.css 'height', 100
+      @$el.css 'height', 500
 
   class TestCollection extends PaginatedCollection
     url: '/test'
 
   module 'PaginatedCollectionView',
     setup: ->
-      fixtures.css height: 100, overflow: 'auto'
+      fixtures.css height: 500, overflow: 'auto'
       createServer()
+      clock = sinon.useFakeTimers()
       collection = new TestCollection
       view = new PaginatedCollectionView
         collection: collection
@@ -42,6 +44,7 @@ define [
 
     teardown: ->
       server.restore()
+      clock.restore()
       fixtures.attr 'style', ''
       view.remove()
 
@@ -50,7 +53,11 @@ define [
     ok $match.length, 'item found'
 
   scrollToBottom = ->
-    fixtures[0].scrollTop = fixtures[0].scrollHeight
+    # scroll within 100px of the bottom of the current list (<500 triggers a fetch)
+    fixtures[0].scrollTop = view.$el.position().top +
+      view.$el.height() -
+      fixtures.position().top -
+      100
     ok fixtures[0].scrollTop > 0
 
   test 'renders items', ->
@@ -78,6 +85,34 @@ define [
     assertItemRendered '3'
     assertItemRendered '4'
 
+  test 'doesn\'t fetch if already fetching', ->
+    sinon.spy collection, 'fetch'
+    sinon.spy view, 'hideLoadingIndicator'
+    collection.fetch()
+    view.checkScroll()
+    ok collection.fetch.calledOnce, 'fetch called once'
+    ok !view.hideLoadingIndicator.called, 'hideLoadingIndicator not called'
+
+  test 'auto-fetches visible pages', ->
+    view.remove()
+    view = new PaginatedCollectionView
+      collection: collection
+      itemView: ItemView
+      scrollContainer: fixtures
+      autoFetch: true
+    view.$el.appendTo fixtures
+    view.render()
+    fixtures.css height: 1000 # it will autofetch the second page, since we're within the threshold
+
+    collection.fetch()
+    server.sendPage fakePage(), collection.url
+    assertItemRendered '1'
+    assertItemRendered '2'
+    clock.tick(0)
+    server.sendPage fakePage(2), collection.urls.next
+    assertItemRendered '3'
+    assertItemRendered '4'
+
   test 'stops fetching pages after the last page', ->
     # see later in the test why this exists
     fakeEvent = "foo.pagination:#{view.cid}"