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] Blank space when scrolling horizontally #3436

Closed
2 tasks done
m4theushw opened this issue Dec 15, 2021 · 7 comments · Fixed by #4158
Closed
2 tasks done

[DataGrid] Blank space when scrolling horizontally #3436

m4theushw opened this issue Dec 15, 2021 · 7 comments · Fixed by #4158
Assignees
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine

Comments

@m4theushw
Copy link
Member

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

lMgA6hyKEC

Expected behavior 🤔

It should display the next columns.

Steps to reproduce 🕹

Steps:

  1. Open https://mui.com/components/data-grid/demo/
  2. Resize the "Commodity" column to make it larger than the grid width
  3. Scroll horizontally

Context 🔦

I noticed this in #3387, but since the logic for variable row height is the same for the column width we have the bug in both axis.

Your environment 🌎

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

Order ID 💳 (optional)

No response

@m4theushw m4theushw added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Dec 15, 2021
@m4theushw m4theushw self-assigned this Dec 15, 2021
@m4theushw
Copy link
Member Author

m4theushw commented Dec 15, 2021

Apparently this can be solved by using rowThreshold=0 and columnThreshold=0. However, we can't change the default value or remove these props now because that would be a breaking change. Also, zeroing these values causes a major performance degradation, since a bunch of styles will have to be recomputed on every render. This change will have to be postponed to v6.

Once these props were removed, we need to fix the performance regression. I had a look at the impact and only raising the specificity, as well as moving the styles to GridRootStyles, might not be sufficient. Ideally, @mui/styles should expose a way to memoize styles that don't depend on prop values.

One example of component whose styles specificity should be increased is https://github.com/mui-org/material-ui-x/blob/a5f36ce091060fcb689c7697bc9d8f571eb4bda4/packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderTitle.tsx#L26

The way it's, these styles are serialized on every render (e.g. every scroll).

@lb-kevin
Copy link

I'm having the same problem if I have a column with already a big initial width (+ datagrid having many columns).
So not only happens when resizing a column.

Example: https://codesandbox.io/s/datagrid-v5-with-mui-core-v4-forked-tdthh?file=/src/App.tsx

@m4theushw
Copy link
Member Author

@lb-kevin Thanks for the feedback. Have you tried to use columnThreshold=0 to fix it?

@lb-kevin
Copy link

@m4theushw Sorry, I don't seem to be able to replicate the problem any more. My bad. Thanks for the help anyway.

@cherniavskii
Copy link
Member

cherniavskii commented Jan 27, 2022

Similar issue with variable row height in #3758
rowThreshold={0} fixes it.

@oliviertassinari oliviertassinari added the feature: Rendering layout Related to the data grid Rendering engine label Jan 29, 2022
@alexfauquette
Copy link
Member

@m4theushw What do you think about this fix

--- a/packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
+++ b/packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx
@@ -223,17 +223,25 @@ export const useGridVirtualScroller = (props: UseGridVirtualScrollerProps) => {
       ? prevRenderContext.current
       : computeRenderContext();
 
-    const rowsScrolledSincePreviousRender = Math.abs(
+    const topRowsScrolledSincePreviousRender = Math.abs(
       nextRenderContext.firstRowIndex - prevRenderContext.current.firstRowIndex,
     );
+    const bottomRowsScrolledSincePreviousRender = Math.abs(
+      nextRenderContext.lastRowIndex - prevRenderContext.current.lastRowIndex,
+    );
 
-    const columnsScrolledSincePreviousRender = Math.abs(
+    const topColumnsScrolledSincePreviousRender = Math.abs(
       nextRenderContext.firstColumnIndex - prevRenderContext.current.firstColumnIndex,
     );
+    const bottomColumnsScrolledSincePreviousRender = Math.abs(
+      nextRenderContext.lastColumnIndex - prevRenderContext.current.lastColumnIndex,
+    );
 
     const shouldSetState =
-      rowsScrolledSincePreviousRender >= rootProps.rowThreshold ||
-      columnsScrolledSincePreviousRender >= rootProps.columnThreshold ||
+      topRowsScrolledSincePreviousRender >= rootProps.rowThreshold ||
+      bottomRowsScrolledSincePreviousRender >= rootProps.rowThreshold ||
+      topColumnsScrolledSincePreviousRender >= rootProps.columnThreshold ||
+      bottomColumnsScrolledSincePreviousRender >= rootProps.columnThreshold ||
       prevTotalWidth.current !== columnsTotalWidth;

From what I understand, the problem is that the column threshold is computed on the left column. But if these columns on the left are too large, and the columns on the right too small, we could have moved by 0 columns on the left (so no rerender) and more than the buffer on the right leading to missing columns.

By this diff, the threshold is applied to the min between left and right movement

Another solution could be to specify the threshold in px, it simplifies the scroll handler, move the conversion px to column index in the getRows() method. But matching threshold and buffer is more complicated.

@m4theushw
Copy link
Member Author

Another solution could be to specify the threshold in px, it simplifies the scroll handler, move the conversion px to column index in the getRows() method. But matching threshold and buffer is more complicated.

Maybe specify it as a percentage. The threshold in pixels will trigger too many renders if the column or rows vary a lot in size.

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! feature: Rendering layout Related to the data grid Rendering engine
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants