From 804f14d03df72c18619309f0cb014836cb01e206 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 14 Oct 2019 12:01:23 +1100 Subject: [PATCH 1/5] fix: don't save the scroll position between transitionHook and updateScroll --- src/index.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/index.js b/src/index.js index 6acc108..87192b7 100644 --- a/src/index.js +++ b/src/index.js @@ -53,6 +53,7 @@ export default class ScrollBehavior { this._checkWindowScrollHandle = null; this._windowScrollTarget = null; this._numWindowScrollAttempts = 0; + this._isTransitioning = false; this._scrollElements = {}; @@ -62,6 +63,7 @@ export default class ScrollBehavior { on(window, 'scroll', this._onWindowScroll); this._removeTransitionHook = addTransitionHook(() => { + this.isTransitioning = true; requestAnimationFrame.cancel(this._saveWindowPositionHandle); this._saveWindowPositionHandle = null; @@ -78,6 +80,7 @@ export default class ScrollBehavior { } registerElement(key, element, shouldUpdateScroll, context) { + const self = this; invariant( !this._scrollElements[key], 'ScrollBehavior: There is already an element registered for `%s`.', @@ -94,6 +97,9 @@ export default class ScrollBehavior { savePositionHandle: null, onScroll() { + if (self.isTransitioning) { + return; // Don't save the scroll position until the transition is complete + } if (!scrollElement.savePositionHandle) { scrollElement.savePositionHandle = requestAnimationFrame( saveElementPosition, @@ -133,6 +139,8 @@ export default class ScrollBehavior { } updateScroll(prevContext, context) { + this.isTransitioning = false; + this._updateWindowScroll(prevContext, context).then(() => { // Save the position immediately after a transition so that if no // scrolling occurs, there is still a saved position @@ -169,6 +177,11 @@ export default class ScrollBehavior { } _onWindowScroll = () => { + if (this.isTransitioning) { + // Don't save the scroll position unil the transition is complete + return; + } + // It's possible that this scroll operation was triggered by what will be a // `POP` transition. Instead of updating the saved location immediately, we // have to enqueue the update, then potentially cancel it if we observe a From 31f3ee656857839262b736591326b2e8e2a4a630 Mon Sep 17 00:00:00 2001 From: Jimmy Jia Date: Wed, 20 Nov 2019 19:13:38 -0500 Subject: [PATCH 2/5] fixes --- src/index.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/index.js b/src/index.js index 87192b7..c333000 100644 --- a/src/index.js +++ b/src/index.js @@ -53,6 +53,7 @@ export default class ScrollBehavior { this._checkWindowScrollHandle = null; this._windowScrollTarget = null; this._numWindowScrollAttempts = 0; + this._isTransitioning = false; this._scrollElements = {}; @@ -63,7 +64,8 @@ export default class ScrollBehavior { on(window, 'scroll', this._onWindowScroll); this._removeTransitionHook = addTransitionHook(() => { - this.isTransitioning = true; + this._isTransitioning = true; + requestAnimationFrame.cancel(this._saveWindowPositionHandle); this._saveWindowPositionHandle = null; @@ -80,7 +82,6 @@ export default class ScrollBehavior { } registerElement(key, element, shouldUpdateScroll, context) { - const self = this; invariant( !this._scrollElements[key], 'ScrollBehavior: There is already an element registered for `%s`.', @@ -96,10 +97,12 @@ export default class ScrollBehavior { shouldUpdateScroll, savePositionHandle: null, - onScroll() { - if (self.isTransitioning) { - return; // Don't save the scroll position until the transition is complete + onScroll: () => { + if (this._isTransitioning) { + // Don't save the scroll position until the transition is complete. + return; } + if (!scrollElement.savePositionHandle) { scrollElement.savePositionHandle = requestAnimationFrame( saveElementPosition, @@ -139,7 +142,7 @@ export default class ScrollBehavior { } updateScroll(prevContext, context) { - this.isTransitioning = false; + this._isTransitioning = false; this._updateWindowScroll(prevContext, context).then(() => { // Save the position immediately after a transition so that if no @@ -177,8 +180,8 @@ export default class ScrollBehavior { } _onWindowScroll = () => { - if (this.isTransitioning) { - // Don't save the scroll position unil the transition is complete + if (this._isTransitioning) { + // Don't save the scroll position until the transition is complete. return; } From a43f77c4f9afa0136ef9c22f278dc8c89c44d31a Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Thu, 19 Dec 2019 06:41:22 +1100 Subject: [PATCH 3/5] refactor: use explicit API for ignoring scroll events --- src/index.js | 23 +++++++++++++---------- types/index.d.ts | 4 ++++ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/index.js b/src/index.js index c333000..6e945ad 100644 --- a/src/index.js +++ b/src/index.js @@ -53,8 +53,7 @@ export default class ScrollBehavior { this._checkWindowScrollHandle = null; this._windowScrollTarget = null; this._numWindowScrollAttempts = 0; - - this._isTransitioning = false; + this._ignoreScrollEvents = false; this._scrollElements = {}; @@ -64,8 +63,6 @@ export default class ScrollBehavior { on(window, 'scroll', this._onWindowScroll); this._removeTransitionHook = addTransitionHook(() => { - this._isTransitioning = true; - requestAnimationFrame.cancel(this._saveWindowPositionHandle); this._saveWindowPositionHandle = null; @@ -97,8 +94,8 @@ export default class ScrollBehavior { shouldUpdateScroll, savePositionHandle: null, - onScroll: () => { - if (this._isTransitioning) { + onScroll() { + if (this._ignoreScrollEvents) { // Don't save the scroll position until the transition is complete. return; } @@ -142,8 +139,6 @@ export default class ScrollBehavior { } updateScroll(prevContext, context) { - this._isTransitioning = false; - this._updateWindowScroll(prevContext, context).then(() => { // Save the position immediately after a transition so that if no // scrolling occurs, there is still a saved position @@ -179,9 +174,17 @@ export default class ScrollBehavior { this._removeTransitionHook(); } + startIgnoringScrollEvents() { + this._ignoreScrollEvents = true; + } + + stopIgnoringScrollEvents() { + this._ignoreScrollEvents = false; + } + _onWindowScroll = () => { - if (this._isTransitioning) { - // Don't save the scroll position until the transition is complete. + if (this._ignoreScrollEvents) { + // Don't save the scroll position until the transition is complete return; } diff --git a/types/index.d.ts b/types/index.d.ts index 298ad9c..7bb569e 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -56,5 +56,9 @@ declare module 'scroll-behavior' { element: HTMLElement, target: ScrollPosition | string, ) => void; + + startIgnoringScrollEvents(): void; + + stopIgnoringScrollEvents(): void; } } From 715bb312c9e59c42c13ef9704b12965cbcb46033 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Thu, 19 Dec 2019 06:41:55 +1100 Subject: [PATCH 4/5] tests: add test for ignoring scroll events --- test/ScrollBehavior.test.js | 28 ++++++++++++++++++++++++++++ test/withScroll.js | 10 ++++++++++ 2 files changed, 38 insertions(+) diff --git a/test/ScrollBehavior.test.js b/test/ScrollBehavior.test.js index 98eff09..d088caf 100644 --- a/test/ScrollBehavior.test.js +++ b/test/ScrollBehavior.test.js @@ -122,6 +122,34 @@ describe('ScrollBehavior', () => { ]); }); + it('should ignore scroll events when startIgnoringScrollEvents is used', done => { + const history = withRoutes(withScroll(createHistory())); + + unlisten = run(history, [ + () => { + history.startIgnoringScrollEvents(); + scrollTop(window, 5000); + delay(() => history.push('/detail')); + }, + () => { + delay(() => history.goBack()); + }, + () => { + expect(scrollTop(window)).to.equal(0); + history.stopIgnoringScrollEvents(); + scrollTop(window, 2000); + delay(() => history.push('/detail')); + }, + () => { + delay(() => history.goBack()); + }, + () => { + expect(scrollTop(window)).to.equal(2000); + done(); + }, + ]); + }); + it('should allow custom position', done => { const history = withRoutes( withScroll(createHistory(), () => [10, 20]), diff --git a/test/withScroll.js b/test/withScroll.js index 214e583..ee30bcd 100644 --- a/test/withScroll.js +++ b/test/withScroll.js @@ -86,9 +86,19 @@ export default function withScroll(history, shouldUpdateScroll) { }; } + function startIgnoringScrollEvents() { + scrollBehavior.startIgnoringScrollEvents(); + } + + function stopIgnoringScrollEvents() { + scrollBehavior.stopIgnoringScrollEvents(); + } + return { ...history, listen, registerScrollElement, + startIgnoringScrollEvents, + stopIgnoringScrollEvents, }; } From 8c5f37584fe96d5df847c6b7dc5d110afc58d377 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Thu, 19 Dec 2019 07:21:11 +1100 Subject: [PATCH 5/5] fix: ignore scroll events for custom elements + tests --- src/index.js | 15 ++++++--------- test/ScrollBehavior.test.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/index.js b/src/index.js index 6e945ad..3c1b85a 100644 --- a/src/index.js +++ b/src/index.js @@ -73,7 +73,9 @@ export default class ScrollBehavior { // It's fine to save element scroll positions here, though; the browser // won't modify them. - this._saveElementPosition(key); + if (!this._ignoreScrollEvents) { + this._saveElementPosition(key); + } }); }); } @@ -94,13 +96,8 @@ export default class ScrollBehavior { shouldUpdateScroll, savePositionHandle: null, - onScroll() { - if (this._ignoreScrollEvents) { - // Don't save the scroll position until the transition is complete. - return; - } - - if (!scrollElement.savePositionHandle) { + onScroll: () => { + if (!scrollElement.savePositionHandle && !this._ignoreScrollEvents) { scrollElement.savePositionHandle = requestAnimationFrame( saveElementPosition, ); @@ -109,7 +106,7 @@ export default class ScrollBehavior { }; // In case no scrolling occurs, save the initial position - if (!scrollElement.savePositionHandle) { + if (!scrollElement.savePositionHandle && !this._ignoreScrollEvents) { scrollElement.savePositionHandle = requestAnimationFrame( saveElementPosition, ); diff --git a/test/ScrollBehavior.test.js b/test/ScrollBehavior.test.js index d088caf..df2d8f3 100644 --- a/test/ScrollBehavior.test.js +++ b/test/ScrollBehavior.test.js @@ -278,6 +278,36 @@ describe('ScrollBehavior', () => { }, ]); }); + + it('should ignore scroll events when startIgnoringScrollEvents is used', done => { + const history = withScrollElement( + withRoutes(withScroll(createHistory())), + ); + + unlisten = run(history, [ + () => { + history.startIgnoringScrollEvents(); + scrollTop(history.container, 5432); + delay(() => history.push('/detail')); + }, + () => { + delay(() => history.goBack()); + }, + () => { + expect(scrollTop(history.container)).to.equal(0); + history.stopIgnoringScrollEvents(); + scrollTop(history.container, 2000); + delay(() => history.push('/detail')); + }, + () => { + delay(() => history.goBack()); + }, + () => { + expect(scrollTop(history.container)).to.equal(2000); + done(); + }, + ]); + }); }); }); });