From e998820c0ad39d644c829bdcd14bf2b9603c851f Mon Sep 17 00:00:00 2001 From: Amro Altahtamouni Date: Tue, 15 Sep 2026 13:39:58 -0700 Subject: [PATCH] Fix VirtualizedSectionList.scrollToLocation off-by-one and sticky header offset (#58329) Summary: `VirtualizedSectionList.scrollToLocation`, and `SectionList` through its wrapper, mapped `itemIndex` to the underlying flat list without skipping the section header. As a result, `itemIndex: 0` targeted the header and every other item was one row early. Sticky-header compensation was also skipped for the first item, allowing the header to obscure it. This change adds the header row to the flattened target index and always applies the sticky-header offset using the current section header metrics. The public API remains zero-based, but callers that compensated for the old behavior must remove that compensation. Migration: - Replace `itemIndex: n + 1` workarounds with `itemIndex: n`. - Replace `itemIndex: data.length` last-item workarounds with `itemIndex: data.length - 1`. Thanks to Marc Rousavy (mrousavy) for the diagnosis: https://github.com/react/react-native/issues/50143 Changelog: [General][Breaking] - Fix `SectionList` and `VirtualizedSectionList` `scrollToLocation` to account for section headers and sticky headers correctly. Reviewed By: javache, ryanfrawley Differential Revision: D118547738 --- .../Lists/VirtualizedSectionList.js | 6 +- .../__tests__/VirtualizedSectionList-test.js | 198 +++++++++++++++--- 2 files changed, 175 insertions(+), 29 deletions(-) diff --git a/packages/virtualized-lists/Lists/VirtualizedSectionList.js b/packages/virtualized-lists/Lists/VirtualizedSectionList.js index b9551b655421..f2d257d377d0 100644 --- a/packages/virtualized-lists/Lists/VirtualizedSectionList.js +++ b/packages/virtualized-lists/Lists/VirtualizedSectionList.js @@ -138,7 +138,7 @@ class VirtualizedSectionList< State, > { scrollToLocation(params: ScrollToLocationParamsType) { - let index = params.itemIndex; + let index = params.itemIndex + 1; for (let i = 0; i < params.sectionIndex; i++) { index += this.props.getItemCount(this.props.sections[i].data) + 2; } @@ -147,10 +147,10 @@ class VirtualizedSectionList< return; } const listRef = this._listRef; - if (params.itemIndex > 0 && this.props.stickySectionHeadersEnabled) { + if (this.props.stickySectionHeadersEnabled) { const frame = listRef .__getListMetrics() - .getCellMetricsApprox(index - params.itemIndex, listRef.props); + .getCellMetricsApprox(index - params.itemIndex - 1, listRef.props); viewOffset += frame.length; } const toIndexParams: { diff --git a/packages/virtualized-lists/Lists/__tests__/VirtualizedSectionList-test.js b/packages/virtualized-lists/Lists/__tests__/VirtualizedSectionList-test.js index 24bb057cc04e..71a3f9cf7e16 100644 --- a/packages/virtualized-lists/Lists/__tests__/VirtualizedSectionList-test.js +++ b/packages/virtualized-lists/Lists/__tests__/VirtualizedSectionList-test.js @@ -219,35 +219,61 @@ describe('VirtualizedSectionList', () => { const ITEM_HEIGHT = 100; const createVirtualizedSectionList = async (props?: { - stickySectionHeadersEnabled: boolean, + stickySectionHeadersEnabled?: boolean, + sections?: Array>, + getItemLayout?: ( + data: unknown, + index: number, + ) => { + length: number, + offset: number, + index: number, + }, }) => { + const defaultSections = [ + // $FlowFixMe[incompatible-type] + { + title: 's1', + data: [{key: 'i1.1'}, {key: 'i1.2'}, {key: 'i1.3'}], + }, + // $FlowFixMe[incompatible-type] + { + title: 's2', + data: [{key: 'i2.1'}, {key: 'i2.2'}, {key: 'i2.3'}], + }, + ] as Array>; + + const sections = props?.sections ?? defaultSections; + let getItemLayout; + // Use `in` check to allow explicitly passing `getItemLayout: undefined` + // to disable the default layout (distinct from not passing the prop at all). + if (props != null && 'getItemLayout' in props) { + getItemLayout = props.getItemLayout; + } else { + getItemLayout = (data: unknown, index: number) => ({ + length: ITEM_HEIGHT, + offset: ITEM_HEIGHT * index, + index, + }); + } + const { + sections: _sections, + getItemLayout: _getItemLayout, + ...restProps + } = props ?? {}; + void _sections; + void _getItemLayout; + let component; await ReactTestRenderer.act(() => { component = ReactTestRenderer.create( > - } + sections={sections} renderItem={({item}) => } getItem={(data, key) => data[key]} getItemCount={data => data.length} - getItemLayout={(data, index) => ({ - length: ITEM_HEIGHT, - offset: ITEM_HEIGHT * index, - index, - })} - {...props} + getItemLayout={getItemLayout} + {...restProps} />, ); }); @@ -265,7 +291,7 @@ describe('VirtualizedSectionList', () => { }; }; - it('when sticky stickySectionHeadersEnabled={true}, header height is added to the developer-provided viewOffset', async () => { + it('when sticky headers enabled and itemIndex is 1, header height is added to viewOffset', async () => { const {instance, spy} = await createVirtualizedSectionList({ stickySectionHeadersEnabled: true, }); @@ -279,7 +305,7 @@ describe('VirtualizedSectionList', () => { viewOffset, }); expect(spy).toHaveBeenCalledWith({ - index: 1, + index: 2, itemIndex: 1, sectionIndex: 0, viewOffset: viewOffset + ITEM_HEIGHT, @@ -291,7 +317,7 @@ describe('VirtualizedSectionList', () => { // prevents #18098 {sectionIndex: 0, itemIndex: 0}, { - index: 0, + index: 1, itemIndex: 0, sectionIndex: 0, viewOffset: 0, @@ -300,7 +326,7 @@ describe('VirtualizedSectionList', () => { [ {sectionIndex: 2, itemIndex: 1}, { - index: 11, + index: 12, itemIndex: 1, sectionIndex: 2, viewOffset: 0, @@ -313,7 +339,7 @@ describe('VirtualizedSectionList', () => { viewOffset: 25, }, { - index: 1, + index: 2, itemIndex: 1, sectionIndex: 0, viewOffset: 25, @@ -328,5 +354,125 @@ describe('VirtualizedSectionList', () => { expect(spy).toHaveBeenCalledWith(expected); }, ); + + it('scrolls to first item of first section', async () => { + const {instance, spy} = await createVirtualizedSectionList(); + // $FlowFixMe[prop-missing] scrollToLocation not on instance + instance?.scrollToLocation({sectionIndex: 0, itemIndex: 0}); + expect(spy).toHaveBeenCalledWith({ + index: 1, + itemIndex: 0, + sectionIndex: 0, + viewOffset: 0, + }); + }); + + it('scrolls to first item of a later section', async () => { + const {instance, spy} = await createVirtualizedSectionList(); + // $FlowFixMe[prop-missing] scrollToLocation not on instance + instance?.scrollToLocation({sectionIndex: 1, itemIndex: 0}); + expect(spy).toHaveBeenCalledWith({ + index: 6, + itemIndex: 0, + sectionIndex: 1, + viewOffset: 0, + }); + }); + + it('when sticky headers enabled and itemIndex is 0, header height is added to viewOffset (was previously skipped)', async () => { + // Use distinct heights per index so only the correct header's height can satisfy the assertion. + // Header at flat index 0 has height 37, item at index 1 has height 41 — an off-by-one + // in the header lookup would produce 41 and fail. + const HEADER_HEIGHT = 37; + const ITEM_HEIGHT_DISTINCT = 41; + const getItemLayout = (data: unknown, index: number) => ({ + length: index === 0 ? HEADER_HEIGHT : ITEM_HEIGHT_DISTINCT + index, + offset: 0, + index, + }); + const {instance, spy} = await createVirtualizedSectionList({ + stickySectionHeadersEnabled: true, + getItemLayout, + }); + // $FlowFixMe[prop-missing] scrollToLocation not on instance + instance?.scrollToLocation({sectionIndex: 0, itemIndex: 0}); + expect(spy).toHaveBeenCalledWith({ + index: 1, + itemIndex: 0, + sectionIndex: 0, + viewOffset: HEADER_HEIGHT, + }); + }); + + it('preserves caller-supplied viewOffset and adds header height when sticky', async () => { + const {instance, spy} = await createVirtualizedSectionList({ + stickySectionHeadersEnabled: true, + }); + // $FlowFixMe[prop-missing] scrollToLocation not on instance + instance?.scrollToLocation({ + sectionIndex: 1, + itemIndex: 0, + viewOffset: 10, + }); + expect(spy).toHaveBeenCalledWith({ + index: 6, + itemIndex: 0, + sectionIndex: 1, + viewOffset: 10 + ITEM_HEIGHT, + }); + }); + + it('preserves caller-supplied viewOffset without sticky headers', async () => { + const {instance, spy} = await createVirtualizedSectionList(); + // $FlowFixMe[prop-missing] scrollToLocation not on instance + instance?.scrollToLocation({ + sectionIndex: 1, + itemIndex: 2, + viewOffset: 15, + }); + expect(spy).toHaveBeenCalledWith({ + index: 8, + itemIndex: 2, + sectionIndex: 1, + viewOffset: 15, + }); + }); + + it('handles out-of-range itemIndex', async () => { + const {instance, spy} = await createVirtualizedSectionList(); + // $FlowFixMe[prop-missing] scrollToLocation not on instance + instance?.scrollToLocation({sectionIndex: 1, itemIndex: 10}); + // 10 + 1 + (3 + 2) = 16, out of range for 10-item list but still forwarded + expect(spy).toHaveBeenCalledWith({ + index: 16, + itemIndex: 10, + sectionIndex: 1, + viewOffset: 0, + }); + }); + + it('works with varying item heights and no getItemLayout', async () => { + const {instance, spy} = await createVirtualizedSectionList({ + sections: [ + // $FlowFixMe[incompatible-type] + {title: 's1', data: [{key: 'a1'}, {key: 'a2'}]}, + // $FlowFixMe[incompatible-type] + { + title: 's2', + data: [{key: 'b1'}, {key: 'b2'}, {key: 'b3'}, {key: 'b4'}], + }, + ] as Array>, + getItemLayout: undefined, + }); + // $FlowFixMe[prop-missing] scrollToLocation not on instance + instance?.scrollToLocation({sectionIndex: 1, itemIndex: 0}); + // section 0: 2 items + header/footer = 4, so first item of section 1 is at 1 + 4 = 5 + expect(spy).toHaveBeenCalledWith({ + index: 5, + itemIndex: 0, + sectionIndex: 1, + viewOffset: 0, + }); + }); }); });