From 609664a8e266f2b5b5aeef2f501936142b4ff3fd Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Wed, 13 Apr 2016 22:18:04 -0400 Subject: [PATCH] More robust scrolling and less flaky tests (#56) --- modules/__tests__/config.js | 3 + modules/__tests__/delay.js | 6 ++ .../__tests__/describeShouldUpdateScroll.js | 11 +- modules/__tests__/fixtures.js | 4 +- modules/__tests__/run.js | 6 +- modules/__tests__/useScrollToTop-test.js | 9 +- modules/__tests__/useSimpleScroll-test.js | 9 +- modules/__tests__/useStandardScroll-test.js | 18 ++-- modules/useScrollToTop.js | 3 +- modules/utils/createUseScroll.js | 101 ++++++++++++++++-- modules/utils/scrollTo.js | 3 - 11 files changed, 142 insertions(+), 31 deletions(-) create mode 100644 modules/__tests__/delay.js delete mode 100644 modules/utils/scrollTo.js diff --git a/modules/__tests__/config.js b/modules/__tests__/config.js index 505e943..5db065d 100644 --- a/modules/__tests__/config.js +++ b/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 diff --git a/modules/__tests__/delay.js b/modules/__tests__/delay.js new file mode 100644 index 0000000..66807f7 --- /dev/null +++ b/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)) +} diff --git a/modules/__tests__/describeShouldUpdateScroll.js b/modules/__tests__/describeShouldUpdateScroll.js index a7a9f04..390e1d1 100644 --- a/modules/__tests__/describeShouldUpdateScroll.js +++ b/modules/__tests__/describeShouldUpdateScroll.js @@ -2,6 +2,7 @@ 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' @@ -9,10 +10,12 @@ 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') diff --git a/modules/__tests__/fixtures.js b/modules/__tests__/fixtures.js index b4603aa..edffa09 100644 --- a/modules/__tests__/fixtures.js +++ b/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) } diff --git a/modules/__tests__/run.js b/modules/__tests__/run.js index d312923..e5efdbc 100644 --- a/modules/__tests__/run.js +++ b/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) }) } diff --git a/modules/__tests__/useScrollToTop-test.js b/modules/__tests__/useScrollToTop-test.js index e8e6e5a..b714030 100644 --- a/modules/__tests__/useScrollToTop-test.js +++ b/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() diff --git a/modules/__tests__/useSimpleScroll-test.js b/modules/__tests__/useSimpleScroll-test.js index 3d0e57a..9860a32 100644 --- a/modules/__tests__/useSimpleScroll-test.js +++ b/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() diff --git a/modules/__tests__/useStandardScroll-test.js b/modules/__tests__/useStandardScroll-test.js index 738ac44..3f1fe2d 100644 --- a/modules/__tests__/useStandardScroll-test.js +++ b/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) diff --git a/modules/useScrollToTop.js b/modules/useScrollToTop.js index c9703c3..67d118a 100644 --- a/modules/useScrollToTop.js +++ b/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 } diff --git a/modules/utils/createUseScroll.js b/modules/utils/createUseScroll.js index 8e0cd89..ecc9998 100644 --- a/modules/utils/createUseScroll.js +++ b/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) { diff --git a/modules/utils/scrollTo.js b/modules/utils/scrollTo.js deleted file mode 100644 index d1a1937..0000000 --- a/modules/utils/scrollTo.js +++ /dev/null @@ -1,3 +0,0 @@ -export default function scrollTo([ x, y ]) { - window.scrollTo(x, y) -}