More robust scrolling and less flaky tests (#56)

modules/__tests__/config.js

@@ -1,6 +1,9 @@
 import createBrowserHistory from 'history/lib/createBrowserHistory'
 import createHashHistory from 'history/lib/createHashHistory'
 
+// Use a delay between steps to let things settle.
+export const DELAY = 20
+
 export const HISTORIES = [
   createBrowserHistory,
   createHashHistory

modules/__tests__/delay.js

@@ -0,0 +1,6 @@
+import { DELAY } from './config'
+
+export default function delay(cb) {
+  // Give throttled scroll listeners time to settle down.
+  requestAnimationFrame(() => setTimeout(cb, DELAY))
+}

modules/__tests__/describeShouldUpdateScroll.js

@@ -2,17 +2,20 @@ import expect from 'expect'
 import scrollLeft from 'dom-helpers/query/scrollLeft'
 import scrollTop from 'dom-helpers/query/scrollTop'
 
+import delay from './delay'
 import { useRoutes } from './fixtures'
 import run from './run'
 
 export default function describeShouldUpdateScroll(useScroll, createHistory) {
   describe('shouldUpdateScroll', () => {
     let unlisten
 
-    afterEach(() => {
+    afterEach(done => {
       if (unlisten) {
         unlisten()
       }
+
+      delay(done)
     })
 
     it('should allow scroll suppression', done => {
@@ -24,13 +27,16 @@ export default function describeShouldUpdateScroll(useScroll, createHistory) {
           !oldLoc || oldLoc.pathname !== newLoc.pathname
         )
       })
+
       unlisten = run(history, [
         () => {
           history.push('/oldpath')
         },
         () => {
           scrollTop(window, 5000)
-          history.push({ pathname: '/oldpath', query: { key: 'value' } })
+          delay(() => history.push({
+            pathname: '/oldpath', query: { key: 'value' }
+          }))
         },
         () => {
           expect(scrollTop(window)).toBe(5000)
@@ -49,6 +55,7 @@ export default function describeShouldUpdateScroll(useScroll, createHistory) {
           [ 10 , 20 ]
         )
       })
+
       unlisten = run(history, [
         () => {
           history.push('/oldpath')

modules/__tests__/fixtures.js

@@ -3,7 +3,7 @@ import createUseScroll from '../utils/createUseScroll'
 export function useRoutes(createHistory) {
   let element
 
-  function updateScroll({ pathname }) {
+  function getScrollPosition({ pathname }) {
     if (pathname === '/') {
       element.style.height = '20000px'
       element.style.width = '20000px'
@@ -25,5 +25,5 @@ export function useRoutes(createHistory) {
     document.body.removeChild(element)
   }
 
-  return createUseScroll(updateScroll, start, stop)(createHistory)
+  return createUseScroll(getScrollPosition, start, stop)(createHistory)
 }

modules/__tests__/run.js

@@ -1,4 +1,6 @@
-export default function run(history, steps, delay) {
+import { DELAY } from './config'
+
+export default function run(history, steps) {
   window.history.replaceState(null, null, '/')
 
   let i = 0
@@ -10,6 +12,6 @@ export default function run(history, steps, delay) {
 
     // First wait a extra tick for all the scroll callbacks to fire before
     // position, even if we don't need an extra delay.
-    setTimeout(() => setTimeout(steps[i++], delay))
+    setTimeout(steps[i++], DELAY)
   })
 }

modules/__tests__/useScrollToTop-test.js

@@ -4,6 +4,7 @@ import scrollTop from 'dom-helpers/query/scrollTop'
 import useScrollToTop from '../useScrollToTop'
 
 import { HISTORIES } from './config'
+import delay from './delay'
 import describeShouldUpdateScroll from './describeShouldUpdateScroll'
 import { useRoutes } from './fixtures'
 import run from './run'
@@ -17,17 +18,19 @@ describe('useScrollToTop', () => {
         history = useRoutes(useScrollToTop(createHistory))()
       })
 
-      afterEach(() => {
+      afterEach(done => {
         if (unlisten) {
           unlisten()
         }
+
+        delay(done)
       })
 
       it('should scroll to top on PUSH', done => {
         unlisten = run(history, [
           () => {
             scrollTop(window, 15000)
-            history.push('/detail')
+            delay(() => history.push('/detail'))
           },
           () => {
             expect(scrollTop(window)).toBe(0)
@@ -40,7 +43,7 @@ describe('useScrollToTop', () => {
         unlisten = run(history, [
           () => {
             scrollTop(window, 15000)
-            history.push('/detail')
+            delay(() => history.push('/detail'))
           },
           () => {
             history.goBack()

modules/__tests__/useSimpleScroll-test.js

@@ -4,6 +4,7 @@ import scrollTop from 'dom-helpers/query/scrollTop'
 import useSimpleScroll from '../useSimpleScroll'
 
 import { HISTORIES } from './config'
+import delay from './delay'
 import describeShouldUpdateScroll from './describeShouldUpdateScroll'
 import { useRoutes } from './fixtures'
 import run from './run'
@@ -17,17 +18,19 @@ describe('useSimpleScroll', () => {
         history = useRoutes(useSimpleScroll(createHistory))()
       })
 
-      afterEach(() => {
+      afterEach(done => {
         if (unlisten) {
           unlisten()
         }
+
+        delay(done)
       })
 
       it('should scroll to top on PUSH', done => {
         unlisten = run(history, [
           () => {
             scrollTop(window, 15000)
-            history.push('/detail')
+            delay(() => history.push('/detail'))
           },
           () => {
             expect(scrollTop(window)).toBe(0)
@@ -40,7 +43,7 @@ describe('useSimpleScroll', () => {
         unlisten = run(history, [
           () => {
             scrollTop(window, 15000)
-            history.push('/detail')
+            delay(() => history.push('/detail'))
           },
           () => {
             history.goBack()

modules/__tests__/useStandardScroll-test.js

@@ -4,6 +4,7 @@ import scrollTop from 'dom-helpers/query/scrollTop'
 import useStandardScroll from '../useStandardScroll'
 
 import { HISTORIES } from './config'
+import delay from './delay'
 import describeShouldUpdateScroll from './describeShouldUpdateScroll'
 import { useRoutes } from './fixtures'
 import run from './run'
@@ -20,20 +21,22 @@ describe('useStandardScroll', () => {
         unlistenBefore = history.listenBefore(listenBeforeSpy)
       })
 
-      afterEach(() => {
+      afterEach(done => {
         if (unlisten) {
           unlisten()
         }
         if (unlistenBefore) {
           unlistenBefore()
         }
+
+        delay(done)
       })
 
       it('should scroll to top on PUSH', done => {
         unlisten = run(history, [
           () => {
             scrollTop(window, 15000)
-            history.push('/detail')
+            delay(() => history.push('/detail'))
           },
           () => {
             expect(listenBeforeSpy.calls.length).toBe(1)
@@ -46,12 +49,13 @@ describe('useStandardScroll', () => {
       it('should restore scroll on POP', done => {
         unlisten = run(history, [
           () => {
-            scrollTop(window, 15000)
+            // This will be ignored, but will exercise the throttle logic.
+            scrollTop(window, 10000)
 
-            // Delay this to let the scroll position actually save.
-            requestAnimationFrame(() => setTimeout(() => {
-              history.push('/detail')
-            }))
+            setTimeout(() => {
+              scrollTop(window, 15000)
+              delay(() => history.push('/detail'))
+            })
           },
           () => {
             expect(listenBeforeSpy.calls.length).toBe(1)

modules/useScrollToTop.js

@@ -1,7 +1,6 @@
 import { POP } from 'history/lib/Actions'
 
 import createUseScroll from './utils/createUseScroll'
-import scrollTo from './utils/scrollTo'
 import setScrollRestoration from './utils/setScrollRestoration'
 
 /**
@@ -18,7 +17,7 @@ export default function useScrollToTop(createHistory) {
     // then let the browser update to its remembered scroll position first,
     // before we set the actual correct scroll position.
     if (action === POP && !unsetScrollRestoration) {
-      setTimeout(() => scrollTo([ 0, 0 ]))
+      setTimeout(() => window.scrollTo(0, 0))
       return null
     }
 

modules/utils/createUseScroll.js

@@ -1,4 +1,11 @@
-import scrollTo from './scrollTo'
+import off from 'dom-helpers/events/off'
+import on from 'dom-helpers/events/on'
+import scrollLeft from 'dom-helpers/query/scrollLeft'
+import scrollTop from 'dom-helpers/query/scrollTop'
+import requestAnimationFrame from 'dom-helpers/util/requestAnimationFrame'
+
+// Try at most this many times to scroll, to avoid getting stuck.
+const MAX_SCROLL_ATTEMPTS = 2
 
 export default function createUseScroll(
   getScrollPosition, start, stop, updateLocation
@@ -9,17 +16,62 @@ export default function createUseScroll(
 
       const history = createHistory(historyOptions)
 
+      let scrollTarget
+      let numScrollAttempts
+      let checkScrollHandle
+
+      function cancelCheckScroll() {
+        if (checkScrollHandle !== null) {
+          requestAnimationFrame.cancel(checkScrollHandle)
+          checkScrollHandle = null
+        }
+      }
+
+      function onScroll() {
+        if (!scrollTarget) {
+          return
+        }
+
+        const [ xTarget, yTarget ] = scrollTarget
+        const x = scrollLeft(window)
+        const y = scrollTop(window)
+
+        if (x === xTarget && y === yTarget) {
+          scrollTarget = null
+          cancelCheckScroll()
+        }
+      }
+
+      // FIXME: This is not the right way to track whether there are listeners.
       let numListeners = 0
 
       function checkStart() {
-        if (++numListeners === 1 && start) {
-          start(history)
+        if (numListeners === 0) {
+          if (start) {
+            start(history)
+          }
+
+          scrollTarget = null
+          numScrollAttempts = 0
+          checkScrollHandle = null
+
+          on(window, 'scroll', onScroll)
         }
+
+        ++numListeners
       }
 
       function checkStop() {
-        if (--numListeners === 0 && stop) {
-          stop()
+        --numListeners
+
+        if (numListeners === 0) {
+          if (stop) {
+            stop()
+          }
+
+          off(window, 'scroll', onScroll)
+
+          cancelCheckScroll()
         }
       }
 
@@ -33,6 +85,31 @@ export default function createUseScroll(
         }
       }
 
+      function checkScrollPosition() {
+        checkScrollHandle = null
+
+        // We can only get here if scrollTarget is set. Every code path that
+        // unsets scroll target also cancels the handle to avoid calling this
+        // handler. Still, check anyway just in case.
+        /* istanbul ignore if: paranoid guard */
+        if (!scrollTarget) {
+          return
+        }
+
+        const [ x, y ] = scrollTarget
+        window.scrollTo(x, y)
+
+        ++numScrollAttempts
+
+        /* istanbul ignore if: paranoid guard */
+        if (numScrollAttempts >= MAX_SCROLL_ATTEMPTS) {
+          scrollTarget = null
+          return
+        }
+
+        checkScrollHandle = requestAnimationFrame(checkScrollPosition)
+      }
+
       let oldLocation
       let listeners = [], currentLocation, unlisten
 
@@ -42,6 +119,9 @@ export default function createUseScroll(
 
         listeners.forEach(listener => listener(location))
 
+        // Whatever we were doing before isn't relevant any more.
+        cancelCheckScroll()
+
         // useStandardScroll needs the new location even when not updating the
         // scroll position, to update the current key.
         if (updateLocation) {
@@ -59,9 +139,16 @@ export default function createUseScroll(
           scrollPosition = getScrollPosition(currentLocation)
         }
 
-        if (scrollPosition) {
-          scrollTo(scrollPosition)
+        scrollTarget = scrollPosition
+
+        // Check the scroll position to see if we even need to scroll.
+        onScroll()
+        if (!scrollTarget) {
+          return
         }
+
+        numScrollAttempts = 0
+        checkScrollPosition()
       }
 
       function listen(listener) {