From d091704fd162c78a0e7af2b9c4b7718159297737 Mon Sep 17 00:00:00 2001 From: Jeremy Press Date: Tue, 12 Sep 2017 15:45:22 -0700 Subject: [PATCH] Fix: fix page change flickering when zoomed in (#384) --- src/lib/__tests__/util-test.js | 66 +++++++++++++++++++ src/lib/util.js | 18 +++-- src/lib/viewers/image/MultiImageViewer.js | 4 ++ .../image/__tests__/MultiImageViewer-test.js | 11 +++- 4 files changed, 89 insertions(+), 10 deletions(-) diff --git a/src/lib/__tests__/util-test.js b/src/lib/__tests__/util-test.js index 43a534ba7..a6b9024ec 100644 --- a/src/lib/__tests__/util-test.js +++ b/src/lib/__tests__/util-test.js @@ -561,4 +561,70 @@ describe('lib/util', () => { expect(element.style.height).to.equal(`${height}px`); }); }); + + describe('pageNumberFromScroll()', () => { + it('should incrememt the page if scrolling down and scroll top has passed the midpoint of page', () => { + const currentPageNum = 1; + const previousScrollTop = 0; + const currentPageEl = { + offsetTop: 0, + clientHeight: 200 + }; + const wrapperEl = { + scrollTop: 101, + offsetHeight: 500 + }; + + const result = util.pageNumberFromScroll(currentPageNum, previousScrollTop, currentPageEl, wrapperEl); + expect(result).to.equal(2); + }); + + it('should not change the page if scrolling down and scroll top has not passed the midpoint of page', () => { + const currentPageNum = 1; + const previousScrollTop = 0; + const currentPageEl = { + offsetTop: 0, + clientHeight: 200 + }; + const wrapperEl = { + scrollTop: 99, + offsetHeight: 500 + }; + + const result = util.pageNumberFromScroll(currentPageNum, previousScrollTop, currentPageEl, wrapperEl); + expect(result).to.equal(1); + }); + + it('should decrement the page if scrolling up and scroll bottom has passed the midpoint of page', () => { + const currentPageNum = 2; + const previousScrollTop = 500; + const currentPageEl = { + offsetTop: 100, + clientHeight: 200 + }; + const wrapperEl = { + scrollTop: 0, + offsetHeight: 100 + }; + + const result = util.pageNumberFromScroll(currentPageNum, previousScrollTop, currentPageEl, wrapperEl); + expect(result).to.equal(1); + }); + + it('should not change the page if scrolling up and scroll bottom has not passed the midpoint of page', () => { + const currentPageNum = 2; + const previousScrollTop = 500; + const currentPageEl = { + offsetTop: 0, + clientHeight: 200 + }; + const wrapperEl = { + scrollTop: 10, + offsetHeight: 100 + }; + + const result = util.pageNumberFromScroll(currentPageNum, previousScrollTop, currentPageEl, wrapperEl); + expect(result).to.equal(2); + }); + }); }); diff --git a/src/lib/util.js b/src/lib/util.js index aaad9b3ba..64dc3e492 100644 --- a/src/lib/util.js +++ b/src/lib/util.js @@ -730,20 +730,24 @@ export function removeActivationListener(element, handler) { * * @public * @param {number} currentPageNum - The current page + * @param {number} previousScrollTop - The last recorded Y scrolling position * @param {HTMLElement} currentPageEl - The current page element - * @param {HTMLElement} wrapperEl - the content wrapper element + * @param {HTMLElement} wrapperEl - The content wrapper element * @return {number} the resulting page number */ -export function pageNumberFromScroll(currentPageNum, currentPageEl, wrapperEl) { +export function pageNumberFromScroll(currentPageNum, previousScrollTop, currentPageEl, wrapperEl) { + let pageNum = currentPageNum; const currentScrollTop = wrapperEl.scrollTop; const currentScrollBottom = wrapperEl.scrollTop + wrapperEl.offsetHeight; const currentPageMiddleY = currentPageEl.offsetTop + currentPageEl.clientHeight / 2; - if (currentScrollTop > currentPageMiddleY) { - return currentPageNum + 1; - } else if (currentScrollBottom < currentPageMiddleY) { - return currentPageNum - 1; + if (currentScrollTop > previousScrollTop) { + // Scrolling down + pageNum = currentScrollTop > currentPageMiddleY ? currentPageNum + 1 : currentPageNum; + } else if (currentScrollTop < previousScrollTop) { + // Scrolling up + pageNum = currentScrollBottom < currentPageMiddleY ? currentPageNum - 1 : currentPageNum; } - return currentPageNum; + return pageNum; } diff --git a/src/lib/viewers/image/MultiImageViewer.js b/src/lib/viewers/image/MultiImageViewer.js index 79870d648..5ef2aa6ae 100644 --- a/src/lib/viewers/image/MultiImageViewer.js +++ b/src/lib/viewers/image/MultiImageViewer.js @@ -33,6 +33,7 @@ class MultiImageViewer extends ImageBaseViewer { // Defaults the current page number to 1 this.currentPageNumber = 1; + this.previousScrollTop = 0; } /** @@ -338,12 +339,15 @@ class MultiImageViewer extends ImageBaseViewer { handlePageChangeFromScroll() { const pageChange = pageNumberFromScroll( this.currentPageNumber, + this.previousScrollTop, this.singleImageEls[this.currentPageNumber - 1], this.wrapperEl ); this.updateCurrentPage(pageChange); + this.scrollCheckHandler = null; + this.previousScrollTop = this.wrapperEl.scrollTop; } } diff --git a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js index ebfffffe3..3ee8dac30 100644 --- a/src/lib/viewers/image/__tests__/MultiImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/MultiImageViewer-test.js @@ -528,14 +528,18 @@ describe('lib/viewers/image/MultiImageViewer', () => { multiImage.singleImageEls = [document.createElement('div')]; stubs.singleImageEls = multiImage.singleImageEls; - multiImage.wrapperEl = document.createElement('div'); + multiImage.wrapperEl = { + scrollTop: 100 + } stubs.wrapperEl = multiImage.wrapperEl; + multiImage.previousScrollTop = 0; + }) it('should determine the current page number based on scroll', () => { multiImage.handlePageChangeFromScroll(); - expect(stubs.pageNumberFromScroll).to.be.calledWith(1, stubs.singleImageEls[0], stubs.wrapperEl); + expect(stubs.pageNumberFromScroll).to.be.calledWith(1, 0, stubs.singleImageEls[0], stubs.wrapperEl); }); it('should attempt to update the current page number', () => { @@ -543,11 +547,12 @@ describe('lib/viewers/image/MultiImageViewer', () => { expect(stubs.updateCurrentPage).to.be.called; }); - it('reset the scroll check handler', () => { + it('reset the scroll check handler and update the previous scroll top position', () => { multiImage.scrollCheckHandler = true; multiImage.handlePageChangeFromScroll(); expect(multiImage.scrollCheckHandler).to.equal(null); + expect(multiImage.previousScrollTop).to.equal(100); }); }); });