Skip to content

Fix VirtualizedSectionList.scrollToLocation off-by-one and sticky header offset (#58329) #58329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
amroaltah wants to merge 1 commit into
base: main
Choose a base branch
from
+175 −29
Open

Fix VirtualizedSectionList.scrollToLocation off-by-one and sticky header offset (#58329) #58329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/virtualized-lists/Lists/VirtualizedSectionList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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: {
Expand Down
198 changes: 172 additions & 26 deletions packages/virtualized-lists/Lists/__tests__/VirtualizedSectionList-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,35 +219,61 @@ describe('VirtualizedSectionList', () => {
const ITEM_HEIGHT = 100;

const createVirtualizedSectionList = async (props?: {
stickySectionHeadersEnabled: boolean,
stickySectionHeadersEnabled?: boolean,
sections?: Array<SectionBase<{key: string}>>,
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<SectionBase<{key: string}>>;

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(
<VirtualizedSectionList
sections={
[
// $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<SectionBase<{key: string}>>
}
sections={sections}
renderItem={({item}) => <item value={item.key} />}
getItem={(data, key) => data[key]}
getItemCount={data => data.length}
getItemLayout={(data, index) => ({
length: ITEM_HEIGHT,
offset: ITEM_HEIGHT * index,
index,
})}
{...props}
getItemLayout={getItemLayout}
{...restProps}
/>,
);
});
Expand All @@ -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,
});
Expand All @@ -279,7 +305,7 @@ describe('VirtualizedSectionList', () => {
viewOffset,
});
expect(spy).toHaveBeenCalledWith({
index: 1,
index: 2,
itemIndex: 1,
sectionIndex: 0,
viewOffset: viewOffset + ITEM_HEIGHT,
Expand All @@ -291,7 +317,7 @@ describe('VirtualizedSectionList', () => {
// prevents #18098
{sectionIndex: 0, itemIndex: 0},
{
index: 0,
index: 1,
itemIndex: 0,
sectionIndex: 0,
viewOffset: 0,
Expand All @@ -300,7 +326,7 @@ describe('VirtualizedSectionList', () => {
[
{sectionIndex: 2, itemIndex: 1},
{
index: 11,
index: 12,
itemIndex: 1,
sectionIndex: 2,
viewOffset: 0,
Expand All @@ -313,7 +339,7 @@ describe('VirtualizedSectionList', () => {
viewOffset: 25,
},
{
index: 1,
index: 2,
itemIndex: 1,
sectionIndex: 0,
viewOffset: 25,
Expand All @@ -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<SectionBase<{key: string}>>,
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,
});
});
});
});
Loading