From 99c5935e3bd3b744596460ed909772fbaee78cf9 Mon Sep 17 00:00:00 2001 From: Brian Lenoski Date: Thu, 23 May 2019 14:55:47 -0500 Subject: [PATCH] Issues/244 (#1) * [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 | 13 ++++++++++- src/domHelpers.js | 25 ++++++++++++++++++++ 4 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/domHelpers.js diff --git a/src/__tests__/FixedSizeList.js b/src/__tests__/FixedSizeList.js index 371b8cdb..a9f9066c 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 isVerticallyOverScrolled. + domHelpers.isVerticallyOverScrolled = () => 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 isVerticallyOverScrolled', () => { + domHelpers.isVerticallyOverScrolled = () => 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..2cf95045 --- /dev/null +++ b/src/__tests__/domHelpers.js @@ -0,0 +1,42 @@ +import { isVerticallyOverScrolled } from '../domHelpers'; + +describe('isVerticallyOverScrolled', () => { + const clientHeight = 500; + const scrollHeight = 1000; + + it('returns overscrolled when scrollTop is less than 0', () => { + const isOverScolled = isVerticallyOverScrolled({ + scrollTop: -1, + clientHeight, + scrollHeight, + }); + expect(isOverScolled).toBe(true); + }); + + it('returns not overscrolled when scrollTop is equal to 0', () => { + const isOverScolled = isVerticallyOverScrolled({ + scrollTop: 0, + clientHeight, + scrollHeight, + }); + expect(isOverScolled).toBe(false); + }); + + it('returns not overscrolled when scrollTop is equal to scrollHeight minus clientHeight', () => { + const isOverScolled = isVerticallyOverScrolled({ + scrollTop: scrollHeight - clientHeight, + clientHeight, + scrollHeight, + }); + expect(isOverScolled).toBe(false); + }); + + it('returns overscrolled when scrollTop is greater than scrollHeight minus clientHeight', () => { + const isOverScolled = isVerticallyOverScrolled({ + scrollTop: scrollHeight - clientHeight + 1, + clientHeight, + scrollHeight, + }); + expect(isOverScolled).toBe(true); + }); +}); diff --git a/src/createListComponent.js b/src/createListComponent.js index a8c9b004..9d26a6d9 100644 --- a/src/createListComponent.js +++ b/src/createListComponent.js @@ -2,6 +2,7 @@ import memoizeOne from 'memoize-one'; import { createElement, PureComponent } from 'react'; +import { isVerticallyOverScrolled } 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,15 @@ 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 ( + isVerticallyOverScrolled({ scrollTop, scrollHeight, clientHeight }) + ) { + return null; + } + return { isScrolling: true, scrollDirection: diff --git a/src/domHelpers.js b/src/domHelpers.js index 830aba68..4cbbce9f 100644 --- a/src/domHelpers.js +++ b/src/domHelpers.js @@ -20,3 +20,28 @@ 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 +type Props = { + clientHeight: number, + scrollHeight: number, + scrollTop: number, +}; + +export function isVerticallyOverScrolled({ + clientHeight, + scrollHeight, + scrollTop, +}: Props): boolean { + const isOverScrolled = + scrollTop < 0 || scrollTop > scrollHeight - clientHeight; + return isOverScrolled; +}