Skip to content

[DataGrid] Fix scroll jump with dynamic row height #16763

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
} from '../dimensions/gridDimensionsSelectors';
import { getValidRowHeight, getRowHeightWarning } from './gridRowsUtils';
import type { HeightEntry } from './gridRowsMetaInterfaces';

import { gridFocusedVirtualCellSelector } from '../virtualization/gridFocusedVirtualCellSelector';
/* eslint-disable no-underscore-dangle */

export const rowsMetaStateInitializer: GridStateInitializer = (state, props, apiRef) => {
Expand Down Expand Up @@ -261,6 +261,13 @@ export const useGridRowsMeta = (
? entry.borderBoxSize[0].blockSize
: entry.contentRect.height;
const rowId = (entry.target as any).__mui_id;
const focusedVirtualRowId = gridFocusedVirtualCellSelector(apiRef)?.id;
if (focusedVirtualRowId === rowId && height === 0) {
// Focused virtual row has 0 height.
// We don't want to store it to avoid scroll jumping.
// https://github.com/mui/mui-x/issues/14726
return;
}
apiRef.current.unstable_storeRowHeightMeasurement(rowId, height);
}
if (!isHeightMetaValid.current) {
Expand Down
34 changes: 34 additions & 0 deletions test/e2e/fixtures/DataGrid/DynamicRowHeight.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import * as React from 'react';
import { DataGrid } from '@mui/x-data-grid';

const rows = [
{ id: 0 },
{ id: 1 },
{ id: 2 },
{ id: 3 },
{ id: 4 },
{ id: 5 },
{ id: 6 },
{ id: 7 },
{ id: 8 },
{ id: 9 },
{ id: 10 },
{ id: 11 },
{ id: 12 },
{ id: 13 },
{ id: 14 },
{ id: 15 },
{ id: 16 },
{ id: 17 },
{ id: 18 },
{ id: 19 },
{ id: 20 },
];

export default function DynamicRowHeight() {
return (
<div style={{ height: 300, width: 400 }}>
<DataGrid rows={rows} columns={[{ field: 'id' }]} getRowHeight={() => 'auto'} />
</div>
);
}
33 changes: 33 additions & 0 deletions test/e2e/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,39 @@ async function initializeEnvironment(
});
expect(scrollTop).not.to.equal(0);
});

// https://github.com/mui/mui-x/issues/14726
it('should not cause scroll jumping when the focused cell is outside of the viewport', async () => {
await renderFixture('DataGrid/DynamicRowHeight');

await page.click('[role="row"][data-id="2"] [role="gridcell"][data-field="id"]');

const scrollPositions: number[] = [];
await page.exposeFunction('storeScrollPosition', async (scrollTop: number) => {
scrollPositions.push(scrollTop);
});
await page.evaluate(() => {
const virtualScroller = document.querySelector('.MuiDataGrid-virtualScroller')!;
virtualScroller.addEventListener('scroll', () => {
// @ts-ignore
window.storeScrollPosition(virtualScroller.scrollTop);
});
});

await page.mouse.wheel(0, 150);
await page.waitForTimeout(500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use waitForSelector here?

In the description of waitForTimeout it says

Note that page.waitForTimeout() should only be used for debugging. Tests using the timer in production are going to be flaky. Use signals such as network events, selectors becoming visible and others instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a timeout, the assertion will happen before the potential scroll jump, and the test will pass without the fix applied.
I can't see waitForSelector as a replacement here, but I'm open for suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test it out, but what I had in mind is to wait for the last row to become attached or first row detached because of the virtualization and assert then
could that work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the last row to become attached or first row detached

Both happen before the scroll jump, so the test will pass without the fix applied.

Perhaps it's possible to make an assertion that will wait for the first row to appear again after scroll with a timeout, and throw if it does appear. But it's not that different from the current solution.
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that would also just spend time waiting for nothing to happen 🙂
in that case, I am fine leave it as is for now


const hadScrollJump = scrollPositions.some((scrollTop, index) => {
if (index === 0) {
return false;
}
// When scrolling down, the scrollTop should be always decreasing
const prevScrollTop = scrollPositions[index - 1];
return scrollTop < prevScrollTop;
});

expect(hadScrollJump).to.equal(false, `Scroll jumped, scrollPositions: ${scrollPositions}`);
});
});

describe('<DatePicker />', () => {
Expand Down