From 8f1725da55a1576a18c684a9f07c85581a1f0008 Mon Sep 17 00:00:00 2001 From: blenoski Date: Thu, 23 May 2019 10:22:08 -0500 Subject: [PATCH 1/3] [Issue 244] List flickers on iOS during rubber band overscrolling Added logic to determine whether or not we have overscrolled vertical scrolling boundaries of container. Updated list vertical scrolling handler to skip rerenders if we have overscrolled. --- src/__tests__/FixedSizeList.js | 18 +++++++++++++++ src/__tests__/domHelpers.js | 42 ++++++++++++++++++++++++++++++++++ src/createListComponent.js | 11 ++++++++- src/domHelpers.js | 14 ++++++++++++ 4 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/domHelpers.js diff --git a/src/__tests__/FixedSizeList.js b/src/__tests__/FixedSizeList.js index 371b8cdb..adc0ac6d 100644 --- a/src/__tests__/FixedSizeList.js +++ b/src/__tests__/FixedSizeList.js @@ -3,6 +3,7 @@ import ReactDOM from 'react-dom'; import ReactTestRenderer from 'react-test-renderer'; import ReactTestUtils from 'react-dom/test-utils'; import { FixedSizeList } from '..'; +import * as domHelpers from '../domHelpers'; const simulateScroll = (instance, scrollOffset, direction = 'vertical') => { if (direction === 'horizontal') { @@ -42,6 +43,10 @@ describe('FixedSizeList', () => { onItemsRendered, width: 50, }; + + // Test renders will set clientHeight and scrollHeight to zero + // so we need to manually mock the response of isVerticallyOverScolled. + domHelpers.isVerticallyOverScolled = () => false; }); it('should render an empty list', () => { @@ -552,6 +557,19 @@ describe('FixedSizeList', () => { simulateScroll(instance, 200); expect(onScroll.mock.calls[0][0].scrollUpdateWasRequested).toBe(false); }); + + it('should not call onScroll if isVerticallyOverScolled', () => { + domHelpers.isVerticallyOverScolled = () => true; + const onScroll = jest.fn(); + // Use ReactDOM renderer so the container ref and "onScroll" event work correctly. + const instance = ReactDOM.render( + , + document.createElement('div') + ); + onScroll.mockClear(); + simulateScroll(instance, 200); + expect(onScroll).not.toHaveBeenCalled(); + }); }); describe('itemKey', () => { diff --git a/src/__tests__/domHelpers.js b/src/__tests__/domHelpers.js new file mode 100644 index 00000000..76749245 --- /dev/null +++ b/src/__tests__/domHelpers.js @@ -0,0 +1,42 @@ +import { isVerticallyOverScolled } from '../domHelpers'; + +describe('isVerticallyOverScolled', () => { + const clientHeight = 500; + const scrollHeight = 1000; + + it('returns overscrolled when scrollTop is less than 0', () => { + const isOverScolled = isVerticallyOverScolled({ + scrollTop: -1, + clientHeight, + scrollHeight + }); + expect(isOverScolled).toBe(true); + }); + + it('returns not overscrolled when scrollTop is equal to 0', () => { + const isOverScolled = isVerticallyOverScolled({ + scrollTop: 0, + clientHeight, + scrollHeight + }); + expect(isOverScolled).toBe(false); + }); + + it('returns not overscrolled when scrollTop is equal to scrollHeight minus clientHeight', () => { + const isOverScolled = isVerticallyOverScolled({ + scrollTop: (scrollHeight - clientHeight), + clientHeight, + scrollHeight + }); + expect(isOverScolled).toBe(false); + }); + + it('returns overscrolled when scrollTop is greater than scrollHeight minus clientHeight', () => { + const isOverScolled = isVerticallyOverScolled({ + scrollTop: (scrollHeight - clientHeight) + 1, + clientHeight, + scrollHeight + }); + expect(isOverScolled).toBe(true); + }); +}); diff --git a/src/createListComponent.js b/src/createListComponent.js index a8c9b004..077e1f01 100644 --- a/src/createListComponent.js +++ b/src/createListComponent.js @@ -2,6 +2,7 @@ import memoizeOne from 'memoize-one'; import { createElement, PureComponent } from 'react'; +import { isVerticallyOverScolled } from './domHelpers' import { cancelTimeout, requestTimeout } from './timer'; import type { TimeoutID } from './timer'; @@ -519,7 +520,8 @@ export default function createListComponent({ }; _onScrollVertical = (event: ScrollEvent): void => { - const { scrollTop } = event.currentTarget; + const { scrollTop, scrollHeight, clientHeight } = event.currentTarget; + this.setState(prevState => { if (prevState.scrollOffset === scrollTop) { // Scroll position may have been updated by cDM/cDU, @@ -528,6 +530,13 @@ export default function createListComponent({ return null; } + // On iOS, we can arrive at negative offsets by swiping past the + // start or past the end which activates the rubber band overscrolling feature. + // When this happens, we're scrolling outside the constraints and don't need rerenders. + if (isVerticallyOverScolled({ scrollTop, scrollHeight, clientHeight })) { + return null; + } + return { isScrolling: true, scrollDirection: diff --git a/src/domHelpers.js b/src/domHelpers.js index 830aba68..a5c71b0c 100644 --- a/src/domHelpers.js +++ b/src/domHelpers.js @@ -20,3 +20,17 @@ export function getScrollbarSize(recalculate?: boolean = false): number { return size; } + +// Determines whether or not we have scrolled outside of the +// container boundaries. This occurs frequently on iOS +// with the rubber band overscrolling feature. This current +// implementation is focused specifically on vertical scrolling +// for Lists. A similar strategy for horizontal scrolling may +// need extra consideration due to rtl vs ltr concerns. +// +// MDN determine if an element has been totally scrolled: +// https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollHeight#Problems_and_solutions +export const isVerticallyOverScolled = ({ scrollTop, scrollHeight, clientHeight }) => { + const isOverScrolled = (scrollTop < 0) || (scrollTop > (scrollHeight - clientHeight)); + return isOverScrolled; +} From cb0432f9c00cdde08961400783efd18b1e06fc22 Mon Sep 17 00:00:00 2001 From: blenoski Date: Thu, 23 May 2019 10:44:54 -0500 Subject: [PATCH 2/3] lint + flow --- src/__tests__/domHelpers.js | 12 ++++++------ src/createListComponent.js | 6 ++++-- src/domHelpers.js | 15 +++++++++++++-- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/__tests__/domHelpers.js b/src/__tests__/domHelpers.js index 76749245..f0970df8 100644 --- a/src/__tests__/domHelpers.js +++ b/src/__tests__/domHelpers.js @@ -8,7 +8,7 @@ describe('isVerticallyOverScolled', () => { const isOverScolled = isVerticallyOverScolled({ scrollTop: -1, clientHeight, - scrollHeight + scrollHeight, }); expect(isOverScolled).toBe(true); }); @@ -17,25 +17,25 @@ describe('isVerticallyOverScolled', () => { const isOverScolled = isVerticallyOverScolled({ scrollTop: 0, clientHeight, - scrollHeight + scrollHeight, }); expect(isOverScolled).toBe(false); }); it('returns not overscrolled when scrollTop is equal to scrollHeight minus clientHeight', () => { const isOverScolled = isVerticallyOverScolled({ - scrollTop: (scrollHeight - clientHeight), + scrollTop: scrollHeight - clientHeight, clientHeight, - scrollHeight + scrollHeight, }); expect(isOverScolled).toBe(false); }); it('returns overscrolled when scrollTop is greater than scrollHeight minus clientHeight', () => { const isOverScolled = isVerticallyOverScolled({ - scrollTop: (scrollHeight - clientHeight) + 1, + scrollTop: scrollHeight - clientHeight + 1, clientHeight, - scrollHeight + scrollHeight, }); expect(isOverScolled).toBe(true); }); diff --git a/src/createListComponent.js b/src/createListComponent.js index 077e1f01..d28d6e2f 100644 --- a/src/createListComponent.js +++ b/src/createListComponent.js @@ -2,7 +2,7 @@ import memoizeOne from 'memoize-one'; import { createElement, PureComponent } from 'react'; -import { isVerticallyOverScolled } from './domHelpers' +import { isVerticallyOverScolled } from './domHelpers'; import { cancelTimeout, requestTimeout } from './timer'; import type { TimeoutID } from './timer'; @@ -533,7 +533,9 @@ export default function createListComponent({ // On iOS, we can arrive at negative offsets by swiping past the // start or past the end which activates the rubber band overscrolling feature. // When this happens, we're scrolling outside the constraints and don't need rerenders. - if (isVerticallyOverScolled({ scrollTop, scrollHeight, clientHeight })) { + if ( + isVerticallyOverScolled({ scrollTop, scrollHeight, clientHeight }) + ) { return null; } diff --git a/src/domHelpers.js b/src/domHelpers.js index a5c71b0c..913f9b0a 100644 --- a/src/domHelpers.js +++ b/src/domHelpers.js @@ -30,7 +30,18 @@ export function getScrollbarSize(recalculate?: boolean = false): number { // // MDN determine if an element has been totally scrolled: // https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollHeight#Problems_and_solutions -export const isVerticallyOverScolled = ({ scrollTop, scrollHeight, clientHeight }) => { - const isOverScrolled = (scrollTop < 0) || (scrollTop > (scrollHeight - clientHeight)); +type Props = { + clientHeight: number, + scrollHeight: number, + scrollTop: number, +}; + +export function isVerticallyOverScolled({ + clientHeight, + scrollHeight, + scrollTop, +}: Props): boolean { + const isOverScrolled = + scrollTop < 0 || scrollTop > scrollHeight - clientHeight; return isOverScrolled; } From a315c1cd0b01120eb1c2f0cf33ebc598a6d88df9 Mon Sep 17 00:00:00 2001 From: blenoski Date: Thu, 23 May 2019 10:54:36 -0500 Subject: [PATCH 3/3] fix spelling error --- src/__tests__/FixedSizeList.js | 8 ++++---- src/__tests__/domHelpers.js | 12 ++++++------ src/createListComponent.js | 4 ++-- src/domHelpers.js | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/__tests__/FixedSizeList.js b/src/__tests__/FixedSizeList.js index adc0ac6d..a9f9066c 100644 --- a/src/__tests__/FixedSizeList.js +++ b/src/__tests__/FixedSizeList.js @@ -45,8 +45,8 @@ describe('FixedSizeList', () => { }; // Test renders will set clientHeight and scrollHeight to zero - // so we need to manually mock the response of isVerticallyOverScolled. - domHelpers.isVerticallyOverScolled = () => false; + // so we need to manually mock the response of isVerticallyOverScrolled. + domHelpers.isVerticallyOverScrolled = () => false; }); it('should render an empty list', () => { @@ -558,8 +558,8 @@ describe('FixedSizeList', () => { expect(onScroll.mock.calls[0][0].scrollUpdateWasRequested).toBe(false); }); - it('should not call onScroll if isVerticallyOverScolled', () => { - domHelpers.isVerticallyOverScolled = () => true; + it('should not call onScroll if isVerticallyOverScrolled', () => { + domHelpers.isVerticallyOverScrolled = () => true; const onScroll = jest.fn(); // Use ReactDOM renderer so the container ref and "onScroll" event work correctly. const instance = ReactDOM.render( diff --git a/src/__tests__/domHelpers.js b/src/__tests__/domHelpers.js index f0970df8..2cf95045 100644 --- a/src/__tests__/domHelpers.js +++ b/src/__tests__/domHelpers.js @@ -1,11 +1,11 @@ -import { isVerticallyOverScolled } from '../domHelpers'; +import { isVerticallyOverScrolled } from '../domHelpers'; -describe('isVerticallyOverScolled', () => { +describe('isVerticallyOverScrolled', () => { const clientHeight = 500; const scrollHeight = 1000; it('returns overscrolled when scrollTop is less than 0', () => { - const isOverScolled = isVerticallyOverScolled({ + const isOverScolled = isVerticallyOverScrolled({ scrollTop: -1, clientHeight, scrollHeight, @@ -14,7 +14,7 @@ describe('isVerticallyOverScolled', () => { }); it('returns not overscrolled when scrollTop is equal to 0', () => { - const isOverScolled = isVerticallyOverScolled({ + const isOverScolled = isVerticallyOverScrolled({ scrollTop: 0, clientHeight, scrollHeight, @@ -23,7 +23,7 @@ describe('isVerticallyOverScolled', () => { }); it('returns not overscrolled when scrollTop is equal to scrollHeight minus clientHeight', () => { - const isOverScolled = isVerticallyOverScolled({ + const isOverScolled = isVerticallyOverScrolled({ scrollTop: scrollHeight - clientHeight, clientHeight, scrollHeight, @@ -32,7 +32,7 @@ describe('isVerticallyOverScolled', () => { }); it('returns overscrolled when scrollTop is greater than scrollHeight minus clientHeight', () => { - const isOverScolled = isVerticallyOverScolled({ + const isOverScolled = isVerticallyOverScrolled({ scrollTop: scrollHeight - clientHeight + 1, clientHeight, scrollHeight, diff --git a/src/createListComponent.js b/src/createListComponent.js index d28d6e2f..9d26a6d9 100644 --- a/src/createListComponent.js +++ b/src/createListComponent.js @@ -2,7 +2,7 @@ import memoizeOne from 'memoize-one'; import { createElement, PureComponent } from 'react'; -import { isVerticallyOverScolled } from './domHelpers'; +import { isVerticallyOverScrolled } from './domHelpers'; import { cancelTimeout, requestTimeout } from './timer'; import type { TimeoutID } from './timer'; @@ -534,7 +534,7 @@ export default function createListComponent({ // start or past the end which activates the rubber band overscrolling feature. // When this happens, we're scrolling outside the constraints and don't need rerenders. if ( - isVerticallyOverScolled({ scrollTop, scrollHeight, clientHeight }) + isVerticallyOverScrolled({ scrollTop, scrollHeight, clientHeight }) ) { return null; } diff --git a/src/domHelpers.js b/src/domHelpers.js index 913f9b0a..4cbbce9f 100644 --- a/src/domHelpers.js +++ b/src/domHelpers.js @@ -36,7 +36,7 @@ type Props = { scrollTop: number, }; -export function isVerticallyOverScolled({ +export function isVerticallyOverScrolled({ clientHeight, scrollHeight, scrollTop,