Skip to content
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

[DataGrid] Sorting in a DataGrid with many columns can shift rows in relation to the column headers #3890

Closed
2 tasks done
DomenHocevar opened this issue Feb 6, 2022 · 7 comments · Fixed by #4058
Closed
2 tasks done
Assignees
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse

Comments

@DomenHocevar
Copy link

DomenHocevar commented Feb 6, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

Take a look here https://codesandbox.io/s/datagriddemo-material-demo-forked-l2hp1?file=/demo.tsx

Screencast.2022-02-06.22.45.47.mp4

As you can see in the video, when scrolling somewhere to the middle and then scrolling a column, the rows seem to shift in relation to the column headers.

Expected behavior 🤔

Rows should not shift in relation to the column headers.

Steps to reproduce 🕹

https://codesandbox.io/s/datagriddemo-material-demo-forked-l2hp1?file=/demo.tsx

Context 🔦

I'm trying to support showing items, that have many properties, in the data grid.

Your environment 🌎

`npx @mui/envinfo`
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.
@DomenHocevar
Copy link
Author

I have found the culprit: setting columnBuffer property to a high enough number (number of all columns) fixes this issue. Thus the problem must lie in the column virtualization.

@michaldudak michaldudak transferred this issue from mui/material-ui Feb 8, 2022
@cherniavskii cherniavskii added status: waiting for maintainer These issues haven't been looked at yet by a maintainer bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 8, 2022
@cherniavskii
Copy link
Member

cherniavskii commented Feb 8, 2022

It's a regression introduced in v5.0.0-beta.5 (Release notes). Virtualization was reworked in #2673
I cannot reproduce the issue using v5.0.0-beta.4: https://codesandbox.io/s/datagriddemo-material-demo-forked-88yi5?file=/demo.tsx

@cherniavskii cherniavskii added the regression A bug, but worse label Feb 8, 2022
@zromick
Copy link

zromick commented Feb 8, 2022

@DomenHocevar Thanks for opening a ticket on this, I just ran into the same issue :))

@ceoshikhar
Copy link

I have found the culprit: setting columnBuffer property to a high enough number (number of all columns) fixes this issue. Thus the problem must lie in the column virtualization.

For me, setting it to around 55% of total columns worked fine.

@alexfauquette
Copy link
Member

alexfauquette commented Feb 25, 2022

I investigated a bit on this bug. Here is what seems to append:

When sorting the columns, the virtualization is modified. The first column to be rendered is updated.
The rows are updated to (some cells at the beginning are removed and other ones on the right are added)
But the rowContainer is not updated, because the number of column impacted is below columnThreshold

A workaround is to set columnThreshold={0}

Here is a demo to experiment with the bug and the fix. As you can see the column "D" content move when sorting except if we set columnThreshold to 0

https://codesandbox.io/s/datagriddemo-material-demo-forked-9ch1rg?file=/demo.tsx

@alexfauquette
Copy link
Member

To fix this, I propose to move the updateRenderZonePosition from handleScroll to getRows in order to keep rows and their container sync

In getRows, we would add this, where instead of checking the renderContext, we verify that firstRowToRender and firstColumnToRender did not changed

const renderedAreaMoved = (firstRowToRender !== prevRenderBuffer.current.firstRowToRender) 
	|| (firstColumnToRender !== prevRenderBuffer.current.firstColumnToRender)

if (renderedAreaMoved) {
  updateRenderZonePosition(nextRenderContext);
  prevRenderBuffer.current = {
    firstRowToRender,
    firstColumnToRender
  }
}

@m4theushw
Copy link
Member

m4theushw commented Feb 25, 2022

@alexfauquette How about the following diff?

diff --git a/packages/grid/x-data-grid/src/internals/hooks/features/virtualization/useGridVirtualScroller.tsx b/packages/grid/x-data-grid/src/internals/hooks/features/virtualization/useGridVirtualScroller.tsx
index 6b81921da..1b7c759fe 100644
--- a/packages/grid/x-data-grid/src/internals/hooks/features/virtualization/useGridVirtualScroller.tsx
+++ b/packages/grid/x-data-grid/src/internals/hooks/features/virtualization/useGridVirtualScroller.tsx
@@ -141,6 +141,7 @@ export const useGridVirtualScroller = (props: UseGridVirtualScrollerProps) => {
     const initialRenderContext = computeRenderContext();
     prevRenderContext.current = initialRenderContext;
     setRenderContext(initialRenderContext);
+    updateRenderZonePosition(initialRenderContext);
 
     const { top, left } = scrollPosition.current!;
     const params = { top, left, renderContext: initialRenderContext };

With #3218, computeRenderContext now depends on rowsMeta.positions which changes when sorting a column. This triggers the effect where the render context is updated but we don't re-position the render zone. Initially the effect was supposed to only be called in the first render but it's triggered more than once now.

To fix this, I propose to move the updateRenderZonePosition from handleScroll to getRows in order to keep rows and their container sync

Note that column pinning also calls getRows but it doesn't need to update the render zone. updateRenderZonePosition only needs to be called when the render context changes. I think with the effect is simpler to achieve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants