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

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

Open
swati-kore opened this issue Feb 28, 2024 · 24 comments
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! discussion waiting for 👍 Waiting for upvotes

Comments

@swati-kore
Copy link

swati-kore commented Feb 28, 2024

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>
  );
}
Screen.Recording.2024-06-21.at.13.03.11.mov
  • 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

@swati-kore swati-kore added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 28, 2024
@michelengelen

This comment was marked as resolved.

@michelengelen michelengelen added support: question Community support but can be turned into an improvement component: data grid This is the name of the generic UI component, not the React module! status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 28, 2024
@michelengelen michelengelen changed the title scrollToIndexes not working properly when getRowHeight set to auto [data grid] scrollToIndexes not working properly when getRowHeight set to auto Feb 28, 2024
@swati-kore

This comment was marked as resolved.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Feb 28, 2024
@michelengelen

This comment was marked as resolved.

@michelengelen michelengelen added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 28, 2024
@swati-kore

This comment was marked as resolved.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Feb 29, 2024
@michelengelen

This comment was marked as resolved.

@michelengelen michelengelen added bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Mar 1, 2024
@swati-kore
Copy link
Author

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
Copy link
Author

swati-kore commented Mar 19, 2024

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
Copy link
Contributor

romgrk commented Mar 19, 2024

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!

@romgrk romgrk added the status: waiting for author Issue with insufficient information label Mar 19, 2024
@swati-kore
Copy link
Author

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

Thank you!

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Mar 21, 2024
@michelengelen
Copy link
Member

michelengelen commented Mar 25, 2024

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
Copy link
Contributor

romgrk commented Mar 25, 2024

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?

@michelengelen michelengelen removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 26, 2024
@cherniavskii
Copy link
Member

Let's wait for upvotes on this issue.

@cherniavskii cherniavskii added the waiting for 👍 Waiting for upvotes label Mar 26, 2024
@jbogedahl-st
Copy link

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
Copy link
Contributor

romgrk commented Jun 18, 2024

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

@jbogedahl-st
Copy link

jbogedahl-st commented Jun 19, 2024

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:
#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
Copy link
Contributor

romgrk commented Jun 19, 2024

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
Copy link

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
Copy link
Member

DanailH commented Jun 20, 2024

@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
Copy link
Contributor

romgrk commented Jun 20, 2024

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
Copy link
Member

oliviertassinari commented Jun 21, 2024

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
Copy link

@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:

  1. 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.
  2. 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)
  1. 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.
  2. 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
Copy link

@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

This comment was marked as resolved.

@jbogedahl-st

This comment was marked as resolved.

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! discussion waiting for 👍 Waiting for upvotes
Projects
None yet
Development

No branches or pull requests

7 participants