From 6747a36f5dec0c49fb32c68c8c980508e6ace7db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20R=C3=A4dlinger?= Date: Mon, 16 Oct 2017 04:18:17 -0700 Subject: [PATCH] VirtualizedList: fix bug where onViewableItemsChanged wouldn't trigger Summary: In the current implementation of the `VirtualizedList` the `onViewableItemsChanged` callback wouldn't trigger if the underlying list data changes. (see example snack https://snack.expo.io/Hk5703eBb) I added a method in the `ViewabilityHelper` to invalidate the cached viewableIndices, which gets triggered when the list-data changes. Closes https://github.com/facebook/react-native/pull/14922 Differential Revision: D5864537 Pulled By: sahrens fbshipit-source-id: 37f617763596244208548817d5b138dadc12c75d --- Libraries/Lists/ViewabilityHelper.js | 7 +++ Libraries/Lists/VirtualizedList.js | 6 ++ .../Lists/__tests__/ViewabilityHelper-test.js | 50 ++++++++++++++++ .../Lists/__tests__/VirtualizedList-test.js | 59 +++++++++++++++++++ 4 files changed, 122 insertions(+) diff --git a/Libraries/Lists/ViewabilityHelper.js b/Libraries/Lists/ViewabilityHelper.js index 3742e607d..05f1a1bc7 100644 --- a/Libraries/Lists/ViewabilityHelper.js +++ b/Libraries/Lists/ViewabilityHelper.js @@ -223,6 +223,13 @@ class ViewabilityHelper { } } + /** + * clean-up cached _viewableIndices to evaluate changed items on next update + */ + resetViewableIndices() { + this._viewableIndices = []; + } + /** * Records that an interaction has happened even if there has been no scroll. */ diff --git a/Libraries/Lists/VirtualizedList.js b/Libraries/Lists/VirtualizedList.js index c1f241374..ba60b6104 100644 --- a/Libraries/Lists/VirtualizedList.js +++ b/Libraries/Lists/VirtualizedList.js @@ -516,6 +516,12 @@ class VirtualizedList extends React.PureComponent { }); if (data !== this.props.data || extraData !== this.props.extraData) { this._hasDataChangedSinceEndReached = true; + + // clear the viewableIndices cache to also trigger + // the onViewableItemsChanged callback with the new data + this._viewabilityTuples.forEach(tuple => { + tuple.viewabilityHelper.resetViewableIndices(); + }); } } diff --git a/Libraries/Lists/__tests__/ViewabilityHelper-test.js b/Libraries/Lists/__tests__/ViewabilityHelper-test.js index 829408212..38948deb2 100644 --- a/Libraries/Lists/__tests__/ViewabilityHelper-test.js +++ b/Libraries/Lists/__tests__/ViewabilityHelper-test.js @@ -385,4 +385,54 @@ describe('onUpdate', function() { viewableItems: [{isViewable: true, key: 'a'}], }); }); + + it('returns the right visible row after the underlying data changed', function() { + const helper = new ViewabilityHelper(); + rowFrames = { + a: {y: 0, height: 200}, + b: {y: 200, height: 200}, + }; + data = [{key: 'a'}, {key: 'b'}]; + const onViewableItemsChanged = jest.fn(); + helper.onUpdate( + data.length, + 0, + 200, + getFrameMetrics, + createViewToken, + onViewableItemsChanged, + ); + expect(onViewableItemsChanged.mock.calls.length).toBe(1); + expect(onViewableItemsChanged.mock.calls[0][0]).toEqual({ + changed: [{isViewable: true, key: 'a'}], + viewabilityConfig: {viewAreaCoveragePercentThreshold: 0}, + viewableItems: [{isViewable: true, key: 'a'}], + }); + + // update data + rowFrames = { + c: {y: 0, height: 200}, + a: {y: 200, height: 200}, + b: {y: 400, height: 200}, + }; + data = [{key: 'c'}, {key: 'a'}, {key: 'b'}]; + + helper.resetViewableIndices(); + + helper.onUpdate( + data.length, + 0, + 200, + getFrameMetrics, + createViewToken, + onViewableItemsChanged, + ); + + expect(onViewableItemsChanged.mock.calls.length).toBe(2); + expect(onViewableItemsChanged.mock.calls[1][0]).toEqual({ + changed: [{isViewable: true, key: 'c'}, {isViewable: false, key: 'a'}], + viewabilityConfig: {viewAreaCoveragePercentThreshold: 0}, + viewableItems: [{isViewable: true, key: 'c'}], + }); + }); }); diff --git a/Libraries/Lists/__tests__/VirtualizedList-test.js b/Libraries/Lists/__tests__/VirtualizedList-test.js index 645336fb6..7499d9b0b 100644 --- a/Libraries/Lists/__tests__/VirtualizedList-test.js +++ b/Libraries/Lists/__tests__/VirtualizedList-test.js @@ -160,4 +160,63 @@ describe('VirtualizedList', () => { ); expect(component).toMatchSnapshot(); }); + + it('returns the viewableItems correctly in the onViewableItemsChanged callback after changing the data', () => { + const ITEM_HEIGHT = 800; + let data = [{key: 'i1'}, {key: 'i2'}, {key: 'i3'}]; + const nativeEvent = { + contentOffset: {y: 0, x: 0}, + layoutMeasurement: {width: 300, height: 600}, + contentSize: {width: 300, height: data.length * ITEM_HEIGHT}, + zoomScale: 1, + contentInset: {right: 0, top: 0, left: 0, bottom: 0}, + }; + const onViewableItemsChanged = jest.fn(); + const props = { + data, + renderItem: ({item}) => , + getItem: (data, index) => data[index], + getItemCount: data => data.length, + getItemLayout: (data, index) => ({ + length: ITEM_HEIGHT, + offset: ITEM_HEIGHT * index, + index, + }), + onViewableItemsChanged, + }; + + const component = ReactTestRenderer.create(); + + const instance = component.getInstance(); + + instance._onScrollBeginDrag({nativeEvent}); + instance._onScroll({ + timeStamp: 1000, + nativeEvent, + }); + + expect(onViewableItemsChanged).toHaveBeenCalledTimes(1); + expect(onViewableItemsChanged).toHaveBeenLastCalledWith( + expect.objectContaining({ + viewableItems: [expect.objectContaining({isViewable: true, key: 'i1'})], + }), + ); + data = [{key: 'i4'}, ...data]; + component.update(); + + instance._onScroll({ + timeStamp: 2000, + nativeEvent: { + ...nativeEvent, + contentOffset: {y: 100, x: 0}, + }, + }); + + expect(onViewableItemsChanged).toHaveBeenCalledTimes(2); + expect(onViewableItemsChanged).toHaveBeenLastCalledWith( + expect.objectContaining({ + viewableItems: [expect.objectContaining({isViewable: true, key: 'i4'})], + }), + ); + }); });