[data grid] scrollToIndexes not working properly when getRowHeight set to `auto`

swati-kore commented 7 months ago

Steps to reproduce

Reproduction: https://stackblitz.com/edit/mui-mui-x-nughf3?file=src%2Fdemo.tsx

export default function ColumnAutosizingDynamicRowHeight() {
  const apiRef = useGridApiRef();

  React.useEffect(() => {
    setTimeout(() => {
      console.log('scrollToIndexes');
      apiRef.current!.scrollToIndexes({
        rowIndex: 300,
      });
    }, 1000);
  }, [apiRef]);

  return (
    <div style={{ height: 400, width: '100%' }}>
      <DataGridPro
        apiRef={apiRef}
        rows={rows}
        columns={columns}
        getRowHeight={() => 'auto'}
      />
    </div>
  );
}

https://github.com/mui/mui-x/assets/3165635/19ca9886-c922-49be-8c41-65e5472fb773

Step 2. Make sure the column has huge length data so that the height of each row will vary
Step 3. So when data is loaded we have added useEffect to scroll to the index which will not work with getRowHeight={() => 'auto' } ( Here row will have wrapped text )
Step 4. If we change getRowHeight={() => '50' } or any custom row height then scroll to index will work as expected

Current behavior

apiRef?.current?.scrollToIndexes is not working in case of Step 3 but working for step 4

Expected behavior

apiRef?.current?.scrollToIndexes should work properly for both step 3 and step 4

Context

I want to scroll to a particular row index even when row text is wrapped or unwrapped

Your environment

    "@emotion/react": "11.11.4",
    "@emotion/styled": "11.11.0",
    "@mui/icons-material": "5.15.14",
    "@mui/material": "5.15.14",
    "@mui/system": "^5.4.1",
    "@mui/x-data-grid": "6.19.8",
    "@mui/x-data-grid-generator": "6.19.8",
    "@mui/x-data-grid-pro": "6.19.8",
    "@types/react": "18.2.67",
    "@types/react-dom": "18.2.22",
    "react": "18.2.0",
    "react-dom": "18.2.0"

Search keywords: scrollToIndexes, data grid, auto row height

michelengelen commented 7 months ago

Please provide a minimal reproduction test case with the latest version. This would help a lot 👷.

A live example would be perfect. You can find a examples and guides on how to find templates in our docs with which you can provide examples for your specific use-case: Support - Bug reproduction.

Thank you! 🙇🏼

swati-kore commented 7 months ago

Updated the details with an example code, please help 🙏

michelengelen commented 7 months ago

hey @swati-kore ... could you please add some examples on your data (columns and rows)? Without them I cannot run your example code.

swati-kore commented 7 months ago

Hi @michelengelen I have added columns and row data, please check

michelengelen commented 7 months ago

Hey @swati-kore ... sorry for the late reply. I was out sick yesterday! I can confirm that this is a bug. Here is an example with our long content demo: Bug repro

The scrolling seems off by just a bit. Thanks for raising this @swati-kore 🙇🏼

swati-kore commented 7 months ago

Hey @michelengelen! No worries about the late reply, hope you're feeling better now. Thanks for confirming the bug and providing an example. I'll take a look at it. Thanks a bunch for flagging this 🙌

swati-kore commented 6 months ago

Hi @michelengelen , I encountered the same issue with the apiRef.current.scroll({ top: parseInt(storedPosition) }); method. Is this also a bug? please check following code

https://stackblitz.com/edit/mui-mui-x-ddszck?file=src%2Fdemo.tsx

romgrk commented 6 months ago

Hi @swati-kore, could you please put your code in a runnable code example such as this one: https://stackblitz.com/fork/github/mui/mui-x/tree/master/bug-reproductions/x-data-grid

And edit your comments to avoid pasting large snippets of code, it makes the issue easier to understand if we can avoid scrolling around so much. Thanks!

swati-kore commented 6 months ago

Hi @romgrk @michelengelen Here is runnable code the https://stackblitz.com/edit/mui-mui-x-ddszck?file=src%2Fdemo.tsx

Thank you!

michelengelen commented 6 months ago

It might have something to do with virtualization. If you perform the scroll action twice:

React.useEffect(() => {
  setTimeout(() => {
    apiRef?.current?.scrollToIndexes({
      rowIndex: 50,
    });
  }, 1000);
  setTimeout(() => {
    apiRef?.current?.scrollToIndexes({
      rowIndex: 50,
    });
  }, 2000);
}, [apiRef]);

with a delay it works. Or if you have a button that scrolls to a certain index and you push it twice.

@romgrk do you have any idea if we can even fix this?

romgrk commented 6 months ago

It's currently not possible to jump to an index if the rows have unknown heights, the rows need to be rendered to the DOM to be measured. On your side, you could try to get proper row measurements with something like the text-metrics package, but I've tried to play with it a bit and couldn't get reliable results: https://stackblitz.com/edit/mui-mui-x-q6qroc?file=src%2Fdemo.tsx

I think maybe we could add some logic to keep the view sticked to a row when we update row heights after they've been rendered/measured properly. The scrollbar would update but the view could be stable.

@mui/xgrid Is this a feature we'd want to implement?

cherniavskii commented 6 months ago

Let's wait for upvotes on this issue.

jbogedahl-st commented 3 months ago

I'm also hitting this with the upgrade to v7 and it's a big show-stopper for us. We have variable height rows. Scrolling from the bottom of the grid is also (still) a major problem, doesn't work well with height of 'auto'.

romgrk commented 3 months ago

This issue should also exist in v6. Do you have an example that shows a regression?

jbogedahl-st commented 3 months ago

This issue should also exist in v6. Do you have an example that shows a regression?

Yes, it is still an issue in v6, however, we were able to work around it by calling the scrollToIndexes() method twice, like others have mentioned above. However, after upgrading to v7.5, I noticed overall slower performance painting the grid, then when trying to call scrollToIndexes() now (I tried simplifying and only calling once, several different combinations of work-arounds I've found people mention to no avail), it ends up flickering several times and scrolls all the way to the bottom of the grid. Then, due to problems that seem to still be around, or reintroduced related to this issue: https://github.com/mui/mui-x/pull/6622

The user is stuck at the bottom of the grid unable to scroll up.

I just wasted two days trying to work around the issue and finally reverted the upgrade today. I will have to find time to come up with a reproducible example I can share, obviously not able to share production code here.

My main point in commenting is due to the comment above about waiting for upvotes before doing anything about it. I wanted to express that for us it is a complete show stopper and so would encourage the team to spend time on fixing the issue at least with the reproducible examples documented so far.

romgrk commented 3 months ago

The core issue with this problem is that it will require to render to the DOM every row that needs to be measured (every auto row), and that's computationally quite expensive unless your dataset is very small. It could cause large interactivity delays right after calling scrollToIndexes(), because the grid would be forced to disable virtualization for the whole region that is being scrolled. I don't think there is any good solution to that case from our side. If this case is important to you, it's probably best that you try to come up with a way to set row dimensions without auto. The best would be to find a way to have static height rows. Alternatively you could also try looking into the canvas text metrics API to get dimensions without having to go through the DOM (assuming the dynamic height comes from text content).

The user is stuck at the bottom of the grid unable to scroll up.

If you can get us a reproduction we'll look into that.

jbogedahl-st commented 3 months ago

The core issue with this problem is that it will require to render to the DOM every row that needs to be measured (every auto row), and that's computationally quite expensive unless your dataset is very small. It could cause large interactivity delays right after calling scrollToIndexes(), because the grid would be forced to disable virtualization for the whole region that is being scrolled. I don't think there is any good solution to that case from our side.

You may be right in what it would require to fix this, however, this is functionality that is advertised as part of this component. We are paying a license fee to use this component, but we are unable to do so, so we're basically paying for nothing right now. This should be built in and either a flag prop exposed to use this functionality, or this should be prevented and documented well that this functionality is not available. As of now, this is considered a bug on a piece of software that we have purchased that is a major show-stopper for us. I don't know what has changed between v6 and v7 to make the problem so much worse, but whatever it is, the grid is useless to us right now. A major use case for us is to have a selected row coming from the server, so we must be able to load the grid and scroll to that selected row. To dictate that the grid rows must be a fixed height is not a solution our clients can accept. We must have the documented capabilities of using 'auto' row heights, virtualization (we deal with very large datasets in our grids) and the ability to scroll to a given index.

DanailH commented 3 months ago

@jbogedahl-st I understand your frustration but everything has its limits when it comes to making something both performant and usable for as wide a variety of cases as possible. When it comes to what has changed between v6 and v7 we made a conscious decision to focus on the general performance of the grid and thus update the virtualization engine. What @romgrk has suggested in terms of you trying to set specific row dimensions is sound advice in the case where we don't have the full picture of your use case. I see that there is a reproduction added to the issue already but if you want to give us another one you are more than welcome (so we can get a better view of the problem). We will discuss it during our next grooming and get back to you here with future steps.

however, this is functionality that is advertised as part of this component. We are paying a license fee to use this component, but we are unable to do so, so we're basically paying for nothing right now

The grid has many functionalities and because of that combining multiple features together might result in unexpected behaviors. We do try and test as many scenarios as possible but I think that you can agree that not everything can be covered. That's why we rely on people reporting issues so we can fix those scenarios and cover them with tests so we can be sure that the issue does not resurface. Lastly, you probably also use a variety of other features of the grid so I don't think you are paying for nothing.

I hope I was able to ease your frustration.

romgrk commented 3 months ago

I think there's two issues here, the jumps and the height estimation for scrollToIndexes. We can look into solutions for both but we really need a reproduction case that shows how you're using the grid and which issues you observe. For example, I have a demo above with the text-metrics API, but that would only apply if your dynamic height is based on textual content.

oliviertassinari commented 3 months ago

I have tried to understand how the workaround of calling scrollToIndexes() changed between MUI X Data Grid v6 and v7. I can't find noticeable differences, calling them twice at 500ms of interval seems to work:

Also because I was curious AG Grid: https://codesandbox.io/s/auto-row-height-forked-2svp37?file=/src/index.js. It looks like we are missing a reproduction to understand how upgrading to v7 creates an issue.

On the root level problem, it's not clear to me that scrollToIndexes() should work with a single API call when we have getRowHeight={() => 'auto'}. At the end of the day, the grid needs to render each row, so it has to go through this layout shift phase. And from there, does this need to be a library abstraction? I would start with an end-user workaround and see if it's really that bad.

On the right end-user workaround, maybe we need an API to get the scroll of a given index, say getRowIndexScroll(). The developer would pool this API until they get their rowIndex in the viewport.

Now, pooling isn't so great. We could also turn this into an event API. We used to have onViewportRowsChange https://mui.com/x/migration/migration-data-grid-v4/#virtualization, which is now called rowsScroll. So I would be curious to try this out: listen to rowsScroll event, store that into a state. When trying to scroll to a position, call scrollToIndexes every 200ms until the desired row is stable in the viewport.

jbogedahl-st commented 3 months ago

@DanailH @romgrk @oliviertassinari - here is a short screen recording of what I'm hitting with v7.5.0 of the data grid. I am still working on creating a simple reproducible of the middle panel. Reproducing the scrollToIndexes() problem is easy, any grid with virtualization and 'auto' row heights hits the problem of not scrolling to the correct item. I'm still trying to pull all the relevant code together to show the other issues as well in a simple example on codesandbox. In the meantime, here is what I'm seeing.. In this video I have a dataset of only 83 records, so it's not even that big. But for the point of illustration, I have virtualization enabled with 'auto' row heights. Here are the steps of what I'm doing:

I'm sitting on a page with a specific item in the grid selected. When I go away and come back, this same item should be selected in the grid.
When I click away and come back, I see the following issues:
- Note how slow it takes to paint everything as it scrolls down the grid
- I am not clicking on anything here, it just scrolls to the bottom of the grid on its own... very slowly, repainting several times. State is not changing during this time, the only thing that I have done in code is call the scrollToIndexes() (twice)
Now I'm sitting at the bottom of the grid, the selected item is up more towards the middle of my data set. I try to scroll up and it takes me several scrolls to get up to where my selected item is visible. You can see the screen repaint over and over and jump back down to the bottom, little by little scrolling until my selected item is visible.
I hit this same problem any time I want to scroll to a given item in the middle of my dataset, the further illustrations include adding a new item, which is placed near the middle and removing an item and having the selection move to the next item in the list. With this system, we go back to the server in between each of these actions and must reload the grid (I don't have control of this, we are working on a long-term plan to change this, but for now it is what it is, but is irrelavant to the problem at hand).

Video 6-21 at 8.41.webm

jbogedahl-st commented 3 months ago

@DanailH @romgrk @oliviertassinari Here is a codesandbox reproducible example. I tried to strip a bunch of the customization and cruft out, but there's still quite a bit there to sift through. If I have time I will try to go back in and whittle it down, in the meantime you can see all problems happening in this example. Choppy, slow, initial load; scrolling to the bottom instead of to the selected row, unable to scroll up smoothly once in this state, etc.

https://codesandbox.io/p/sandbox/interesting-swartz-f5tfk7?file=%2Fsrc%2FDataTable.tsx%3A318%2C1

oliviertassinari commented 3 months ago

@jbogedahl-st Could you make the sandbox public? Thanks

jbogedahl-st commented 3 months ago

@jbogedahl-st Could you make the sandbox public? Thanks

Sorry, try it now

mui / mui-x