Fix: fix page change flickering when zoomed in (#384)

box · Sep 12, 2017 · d091704 · d091704
1 parent e5899b2
commit d091704
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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
@@ -528,26 +528,31 @@ 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', () => {
             multiImage.handlePageChangeFromScroll();
             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);
         });
     });
 });