mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.11k stars 1.27k forks source link

[data grid] `scrollToIndex` method is not working #4744

Open mwesterhoff opened 2 years ago

mwesterhoff commented 2 years ago

Order ID 💳

41240

The problem in depth 🔍

We are trying to use Data Grid Pro for a CRUD-like application, which includes adding records. When adding a new empty record, we need to scroll to the corresponding location.

We have used your "Full featured CRUD" demo as a starting point: https://mui.com/x/react-data-grid/editing/#full-featured-crud but it does not correctly scroll for us. The demo only has 5 records, but if you increase that to e.g. 10, it shows the described issue. Clicking "Add Record" does not make the newly added row visible: https://0mlcy8.csb.app/

Also looking at the code, we wonder if scrolling to apiRef.current.getRowsCount() - 1 is really the correct thing. Depending on sorting, the new (empty) row might not be the last - right? Does one not need to scroll to apiRef.current.getRowIndex(id) or apiRef.current.getRowIndexRelativeToVisibleRows(id)?

Your environment 🌎

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

You don't need to use scrollToIndexes. startRowEditMode has a fieldToFocus param that can be used to pass which field should receive focus. The browser will scroll to it by default. In the next release, the docs will be updated with new demos not using scrollToIndexes. Until there, I created a CodeSandbox showing how to use the new param: https://codesandbox.io/s/fullfeaturedcrudgrid-material-demo-forked-gi06jl?file=/demo.tsx

If you still need to use scrollToIndexes, then getRowIndexRelativeToVisibleRows should be used.

danielrenninghoff commented 2 years ago

Hi, I work on the same project as the author of this issue. I have looked into using the fieldToFocus property, however it does not seem to work correctly. The issue only happens when there are more rows than space available in the data grid. In your example, there are only 5 rows, however when using 15 rows, the scrolling to index in your example does not work reliably anymore.

I have edited your example CodeSandbox to include 15 rows: https://codesandbox.io/s/fullfeaturedcrudgrid-material-demo-forked-ke0nvq?file=/demo.tsx

After loading the page, stay on top and don't scroll to the bottom. Then click the "Add Record" button. The browser will not scroll to the bottom. Happens on both Chrome 101 and Firefox 100 on Windows 10.

The issue seems to be related to the virtualization of the data grid. When disabling virtualization using the disableVirtualization property, it is possible to reliably scroll to the bottom with the following code:

setTimeout(() => {
  apiRef.current.scrollToIndexes({
    rowIndex: apiRef.current.getRowsCount() - 1
  });
  apiRef.current.setCellFocus(id, "name");
});

This CodeSandbox contains the default CRUD example code, however I have increased the row count to 15 and disabled virtualization: https://codesandbox.io/s/fullfeaturedcrudgrid-material-demo-forked-0tuslg?file=/demo.tsx

The "Add Record" button scrolls reliably, however when I remove the disableVirtualization option, it does not work anymore.

Since disabling virtualization is not really a good option, could you please look into this issue?

m4theushw commented 2 years ago

@danielrenninghoff Yeah, it doesn't scroll because the call to element.scroll() only happens when the cell is rendered, but with virtualization enabled you need to scroll first to render it. I'm investigating how to automatically scroll when the focus is updated. The main problem is that React.useLayoutEffect is not firing at the correct moment.

cherniavskii commented 2 years ago

Hey @danielrenninghoff @mwesterhoff I've looked into this issue and scrollToIndexes methods seems to work just fine. I've commented setCellFocus method call in your demo and you can see that scrolling works as expected: https://codesandbox.io/s/fullfeaturedcrudgrid-material-demo-forked-1y3cny

The problem starts when you do both scrollToIndexes and setCellFocus one after another. I suspect when setCellFocus is called, the cell is not rendered yet and scroll gets messed up. I was able to workaround it wrapping setCellFocus into another setTimeout: https://codesandbox.io/s/fullfeaturedcrudgrid-material-demo-forked-7vrrtn?file=/demo.tsx

toddsmith-adsk commented 3 months ago

This is what ended up working for me when trying to add and then edit a new row with a large dataset and virtualization:

  useEffect(() => {
    apiRef.current.scrollToIndexes(coordinates);
    apiRef.current.setCellFocus(coordinates.rowIndex, "name");
  }, [apiRef, coordinates]);

  const handleAddRow = () => {
     ...snip

    apiRef.current.startRowEditMode({ id: -1 });
    setTimeout(() => {
      setCoordinates({ rowIndex: rows.length, colIndex: 0 });
    }, 1000);
  }

Without the 1000 msec delay the cell will not be rendered by the time scrollToIndex is called and thus targetOffsetHeight is computed as NaN in scrollToIndexes().