ag-grid / ag-grid

The best JavaScript Data Table for building Enterprise Applications. Supports React / Angular / Vue / Plain JavaScript.
http://www.ag-grid.com
Other
12.62k stars 1.86k forks source link

ensureIndexVisible(rowNode.rowIndex, 'top'); not working in V30 #6794

Closed sonicviz closed 1 year ago

sonicviz commented 1 year ago

I'm submitting a ... (check one with "x")

[X] bug report => see 'Providing a Reproducible Scenario'
[] feature request => do not use Github for feature requests, see 'Customers of AG Grid'
[] support request => see 'Requesting Community Support'

Current behavior

I just updated from V28.2.1 to V30 All looked ok and then I noticed that my rows were no longer scrolling back to the top with this.gridApi.ensureIndexVisible(rowNode.rowIndex, 'top');

I have paginated scrolling that is triggered when a reset Grid button is clicked, to find the right page, and then scroll to the correct current selected and highlighted row index.

        if (rowNode) {
            rowNode.setDataValue('selected', true)
            var page = Math.floor(rowNode.rowIndex / this.pageSize);
            this.gridApi.paginationGoToPage(page);
            this.gridApi.ensureIndexVisible(rowNode.rowIndex, 'top');
        }

In V30, I can navigate to another page, scroll down, hit my reset grid button to trigger the above code, and it will gp back to the correct page but it will not scroll the selected highlighted row into view.

Expected behavior

Without the bug the selected highlighted row is scrolled to the top of the page when I reset the grid that triggers the above code. I can page to another page, scroll down, hit "reset grid" to tigger the above code, and the grid will navigate to the correct page (80 rows per page) and then show the selected highlighted row at the top.

            this.gridApi.ensureIndexVisible(rowNode.rowIndex, 'top');

I reverted back to V28.2.1 and the expected behavior is back. In V30 the code shown in current behavior correctly goes back to the right page, but no longer scrolls the selected row into view at the top.

ensureIndexVisible as shown under scrolling https://www.ag-grid.com/javascript-data-grid/grid-api/

Please tell us about your environment:

Win 11 X64

"Mozilla/5.0 (Windows NT 6.2; Win64; x64) AppleWebKit/604.1.38 (KHTML, like Gecko) Chrome/49.0.2623 Safari/604.1.38 CoherentGT/2.0"

Javascript

kiril-matev commented 1 year ago

Hello @sonicviz,

We're keen to look into this but we can't reproduce the issue you've reported based on the context you provided.

Please send us a live reproduction example (plunker sample) we can use to investigate.

Looking forward to your response.

sonicviz commented 1 year ago

Not everything can be reproduced in a plunker, libraries etc tend to only show certain issues when used in context of an application in situ. I know reproduction is key to resolving bugs (I do that for a living too) but in this case I can't provide a simple plunker repro, I can only explain. If you're not seeing this with simple plunker examples then I guess it's deeper down, possibly a timing issue that's killing the scroll back to top maybe.

Would a video demo work, showing V28.2.1 vs V30 behavior just to prove I'm not making it up? Is there any ability to instrument the API to give out more detailed internal info on what might be causing it to hiccup?

kiril-matev commented 1 year ago

Hi @sonicviz,

Thanks for this response.

We don't doubt your report that there is an issue on your end so a video would really help. We are keen to investigate and we can't investigate what we can't reproduce locally.

In case you can't reproduce this in a plunker sample, please build a separate small project to reproduce it, then ZIP the source code and send over to us.

Looking forward to your response.

sonicviz commented 1 year ago

Hi Kiril,

Here's a video (unlisted) that demonstrates V28.2.1 (expected behavior, first half of video) with V30 (not working as expected, second half, no code changes just swapped the import): https://youtu.be/XzSEAWghYe8

I did a Plunker with the basic functionality that the video demonstrates, with both versions, but the error doesn't reproduce: https://plnkr.co/plunk/SaPbDchbcHvkQRYp

As explained above in other message, and as you can see in the video, libraries etc tend to only show certain issues when used in context of an application in situ. In this case you can certainly see the behavior is different. In particular, note the strange double flash on the selected row of the ATR plane test in V30 when it should be scrolling to the top as it does in V28.2.1

My gut feel is a timing related issue perhaps, that's been introduced via some changes post V28.2.1 perhaps? These can be tricky to resolve.

In case you can't reproduce this in a plunker sample, please build a separate small project to reproduce it, then ZIP the source code and send over to us.

This is difficult, as you can see it's a fairly specialised implementation of AG-Grid and you need to be familiar with MSFS and the SDK to debug it. That said, it's all HTML and JS, but it uses an older version of browser tech via the Coherent GT UI framework: "Mozilla/5.0 (Windows NT 6.2; Win64; x64) AppleWebKit/604.1.38 (KHTML, like Gecko) Chrome/49.0.2623 Safari/604.1.38 CoherentGT/2.0"

It's a pretty complex series of addons, but AG-Grid has been solid so far. The only major issue I had, which was almost a showstopper, was a memory issue that glitched out the grid when it was in infinite scroll. After some experimentation, I found using pagination resolved that, and settled on 80 planes per page. I could load more, I think it was up to ~120 or so before it started glitching, but I set it at 80 for a good safety and performance buffer. Infinite scroll would be nicer, but this works ok.

But ensureIndexVisible is pretty key for the UX, popping the user back to their selected aircraft so it really needs to work seemlessly, as it does in V28.2.1

Not sure what else to try here. No code on my side has changed, just the library.

Ag-Grid is used in both: https://sonicviz.com/project/aircraft-manager/ and https://sonicviz.com/project/location-manager/

For more background, here are some blog posts on the development of them: https://sonicviz.com/2023/01/29/aircraft-manager-for-msfs/ https://sonicviz.com/project/location-manager/

I'd love to upgrade to the Enterprise version at some point, but as an indie the budget is tight at the moment. Response from users is excellent, and AG-Grid has been performing great so far!

kiril-matev commented 1 year ago

Hi @sonicviz,

Thank you for this response.

Thank you for the plunker sample. However, it's not useful because it's not clear what series of steps you're following to cause the issue. This is why when providing a reproduction sample, please include a list of steps to execute (a test case) with the sample and a description of the actual vs expected behavior.

I agree the correct operation of ensureIndexVisible API is key for a good user experience. However, we still don't have the necessary information needed to reproduce this issue.

I understand that some issues are only reproducible as part of a larger application and this makes them harder to reproduce separately. However, as we don't have the larger application you're working with, it's impossible for us to know what the circumstances of this issue are, so we can reproduce it and investigate it.

Our bug submission process requires submitting a sample and you are best-positioned to build one as you can see the issue in action and you can go through a process of narrowing it down to identify the circumstances of it occurring. For example:

  1. You can use different versions of AG Grid in your application to see the exact version where this behavior changed
  2. You can try out different configurations of AG Grid (for example, hiding some of the columns or modifying the size of row buffer or turning off row virtualization altogether) to see if the issue is resolved or not
  3. You can update the way application supplies data or makes API calls to the grid to see if the issue is resolved or not

We cannot do any of this and we won't be productive doing any investigation of an issue we cannot reproduce. By following this process you can identify the minimal reproduction scenario and can build a separate sample to reproduce this which you can send to us.

sonicviz commented 1 year ago

Trying to make a reproducable demo in Plunker would be a time sink in itself, and might not even be possible, as explained.

I will try testing with all versions post V28.2.1 to detect which version the aberrant behavior kicks in at though, which should narrow down what might be the cause.

kiril-matev commented 1 year ago

Hi @sonicviz,

You're right - trying to make a reproducible demo in plunker/stackblitz would be time consuming for you, but it is still possible because you have the application where this is reproducible. As we don't have this application, it is practically impossible for us or anyone else to build a reproduction example.

I think narrowing down the cause in terms of which version introduced the issue may be of little use, so I wouldn't recommend spending time on this or on investigating the particular line of code in AG Grid that causes this. This is because unless we have a test case to reproduce the issue locally we can't investigate it. Unless we have a test case we can execute locally, we can't validate the issue was fixed after we make a change.

This is why the best use of time is to build a standalone example which reproduces the issue.

I would recommend an iterative approach: 1/ Progressively reduce the configuration applied to AG Grid to identify a minimum configuration where this occurs. Here are examples of reducing the configuration of AG Grid: 1.1 If you're using 10 columns in AG Grid and it's reproducible with just one, remove the 9 columns) 1.2 If you're using row/range selection and it's reproducible without, remove row/range selection 1.3. If you're using custom cell renderers and it's reproducible without, remove the custom cell renderers 1.4 Any additional configuration that can be removed while still reproducing the issue, remove it.

This way you'll identify the minimal configuration that causes this issue.

I understand your application is complex and uses external APIs to get data, etc. This is why, when building the reproduction example: 2/ Use a mock backend of your application, providing static dummy data 3/ Use buttons in the UI to call AG Grid API functions that your application calls automatically to cause the issue.

We appreciate your time and we're looking forward to your response so we can investigate this further.

sonicviz commented 1 year ago

Thanks for the info.

It was definitely introduced in V29, which I just tested. Other suggested tests in your initial reply above fail to have any impact.

Question is, what changes did you introduce in V29 that cause this to happen?

Version issue appears at: ○ OK § V28.2.1 ○ Fails § V29 • Modifying the size of row buffer ○ See [Row Buffer](onenote: ○ rowBuffer: 0 FAIL ○ rowBuffer: 50 FAIL ○ rwoBuffer: 150 FAIL (just for kicks) • Turning off row virtualization altogether ○ See [Row Buffer](onenote: ○ suppressRowVirtualisation: true, ○ FAIL • suppressMaxRenderedRowRestriction: true, ○ FAIL

Update: Removing columns has no effect, even with a minimal layout it won't exhibi the expected ensureIndexVisibl

On the plus side, I don't get the memory glitch error now so it works ok without pagination. I adjusted the code but ensureIndexVisible still doesn't work.

After confirming the issue was introduced with V29 I've gone to V30 for testing. Not sure what else I can do here.

sonicviz commented 1 year ago

I'm seeing this in the log, with debug on:

[Log] PC2 UpdateSelectedPlane resetSelectedPlane moved to page: 0 rowNode.rowIndex: 63 (PlaneSelection.js, line 2344) [Log] AG Grid.selectionService: reset (ag-grid-community.min.js, line 8) [Log] AG Grid.selectionService: reset (ag-grid-community.min.js, line 8) [Log] PC2 UpdateSelectedPlane index: 63 Name: Late-631 (PlaneSelection.js, line 2311) [Log] PC2 UpdateSelectedPlane this.currentSelectedAircraft: 63 (PlaneSelection.js, line 2323) [Log] PC2 UpdateSelectedPlane rowNode: (PlaneSelection.js, line 2330) [Log] e {rowIndex: 63, key: null, childrenMapped: {}, displayed: true, rowTop: 2898, …} (PlaneSelection.js, line 2331) [Log] PC2 UpdateSelectedPlane resetSelectedPlane moved to page: 0 rowNode.rowIndex: 63 (PlaneSelection.js, line 2344)

Is "[Log] AG Grid.selectionService: reset (ag-grid-community.min.js, line 8)" relevant at all?

kiril-matev commented 1 year ago

Hi @sonicviz,

Thanks for this response.

Logs and event stack traces aren't helpful in reproducing an issue. We need to have a reproduction scenario that we can use to reproduce the issue and validate a fix of the issue.

As you can see, AG Grid v29 introduced a great number of changes listed here: https://ag-grid.com/changelog/?fixVersion=29.0.0

We cannot pinpoint a particular change that's causing this based on the context you've provided.

As I mentioned earlier, the only way to report this issue to us is with a reproduction example, especially in this case as even you can't reproduce the issue outside of your application. You need to iteratively strip out parts of your application backend and replace it with a mock backend if that's needed to reproduce the issue in a standalone project you can send to us.

Building an isolated reproduction example takes time but in our experience it's the quickest way to work on an issue.

We are ready to investigate this potential issue once we receive a reproduction example.

sonicviz commented 1 year ago

Logs and event stack traces aren't helpful in reproducing an issue.

I have to disagree with that. Logs and tracing are most definitely useful when you can't physically reproduce an issue. I had a user in the UK recently who's 10 hardware controllers were not switching, the only way I could debug it was to send a custom instrumented build to trace out what was happening. That information resulted in a couple of key UX and performance changes, and was only doable through old skool logging output to a text field in AGGrid, which I then had him copy and send it to me to trawl through. It worked. With enough information from debugging and logging you can deduce where the issue lies, if the system is instrumented to output useful information like that. Unless you understand what the code is doing you can't fix the issue, and there are multiple ways to get that understanding, logs and debug traces being a key one. Sometimes oldskool still works.

Doesn't debug provide anything useful?

re: https://www.ag-grid.com/changelog/?fixVersion=29.0.0&ref=blog.ag-grid.com I'm guessing it has something to do with:

Selection

selectIndex, deselectIndex - removed, use rowNode.setSelected(isSelected) instead. selectNode, deselectNode - removed, use rowNode.setSelected(isSelected) instead. getSelectedNodesById - removed, use getSelectedNodes instead. isNodeSelected - removed, use rowNode.isSelected instead. getRangeSelections - removed, use getCellRanges instead. addRangeSelection - removed, use addCellRange instead.

or

Display

ensureColIndexVisible - removed, use ensureColumnVisible instead. doLayout - removed, no longer required as grid now responds to size changes. checkGridSize - removed as was legacy and no longer required. getFirstRenderedRow, getLastRenderedRow - removed, use getFirstDisplayedRow, getLastDisplayedRow instead. addVirtualRowListener - removed, use addRenderedRowListener instead.

But I'm not using any of those. It has to be related to selection and scrolling, somehow. Logically, if something was working fine, then it breaks, and nothing else was changed but the library then the issue is with the library, not the application. Unless it's a documented breaking change that you can update, but I'm not seeing anything in the release notes related to this apart from the above.

Are you supposed to be using rowNode.setSelected(isSelected) for ensureIndexVisible to work now?

You need to iteratively strip out parts of your application backend and replace it with a mock backend if that's needed to reproduce the issue in a standalone project you can send to us.

That's not feasible. There's also no guarantee it will work. I understand having a reproducable demo is the easiest solution to work with, but that's not always possible. You also need alternative ways to debug, such as instrumented builds with useful tracing/logging info at key points to get insight into why something isn't working the way it should.

sonicviz commented 1 year ago

I tried debugging using the unminified version but it would not load it, so reduced to debugging with the minified version. Stepping through the relevant code in ensureIndexVisible shows that V30.0.2 is coming up with the wrong numbers. Also, added from V29 on was gridBodyComp\fakeVScrollComp.ts , which didn't exist in V28.2.1

The following was doing the same test, entering the aircraft selection screen where it should automatically scroll the row 103 into the top position. It does it in V28.2.1, but not in V30.0.2 (and V29+). The local variables tell the story of what's calculated at the completion of ensureIndexVisible. It's a little tricky to translate as some of the variable names are switched in the different minified versions, but by matching the forumulas you can see which is supposed to be which.

image

V28.2.1 Debug = Success! image

V30.0.2 Debug = Fail. image

Unless this is some kind of CSS issue, with the fake vertical scroll, maybe.

I guess I'll just stick to V28.2.1

annaclatworthy commented 1 year ago

Hi - I have been having similar problems with ensureIndexVisible(top) where it is not producing the result I'm expecting, and I am currently working off the car example data.

My environment: Angular 14, and Ag-Grid is the latest at 30.0.2.

Say I call i = this.agGrid.api.getFirstDisplayedRow() and then later on call this.agGrid.api.ensureIndexVisible(i, 'top'), There is a 10 row difference in the results. If I call this.agGrid.api.ensureIndexVisible(i +10 , 'top'), I get the result I would expect.

Why is this?

kiril-matev commented 1 year ago

Hi @annaclatworthy,

Thanks for reaching out.

It would be helpful if you provide examples to illustrate the specific configuration you're using. For example, you've not mentioned if you're using server-side row model, or whether you're using row auto-height, whether you're setting rowBuffer or any other setting.

Settings in AG Grid can modify behavior to a great extent and it's very difficult to speculate which of the many (thousands) of configurations you're using. This is why we ask for sample projects to avoid delays due to multiple rounds of communication until we identify your actual configuration.

See below:

  1. Open https://plnkr.co/edit/9OdiN65EgQq9cOyR?open=main.js
  2. Open console
  3. Scroll down to row 40
  4. Click SAVE VERTICAL SCROLL POSITION button above grid Actual & Expected: Console shows first displayed row index as 29.
  5. Scroll down a few rows
  6. Click SCROLL SAVED ROW INTO VIEW button above grid Actual & Expected: Row 29 scrolled into view This is because the grid has a default rowBuffer value of 10 - this means 10 rows are rendered above and below the visible rows. The purpose is to have rendered rows to display when the user is scrolling instead of rendering them on the spot. See this documented here: https://www.ag-grid.com/javascript-data-grid/dom-virtualisation/#row-buffer

This is why the first rendered row is 10 rows above the top row shown in the grid, and the last row rendered is 10 rows below the last row shown in the grid.

It seems to me you're using this approach to save and restore scrolling position. You have three options at this time: 1/ Using the approach you have now, you can add the default rowBuffer value to the first visible row index. This way you get the index of the actual first visible row on screen and then call ensureIndexVisible with this index. https://plnkr.co/edit/BP711tKVT1tbuuqT?open=main.js

2/ Using the approach you have now, you can set rowBuffer=0. In this case the first displayed row is the top row shown in the grid. This way you can get the first displayed row index and provide it to the ensureIndexVisible method. Read the documentation above for the effect of setting rowBuffer. https://plnkr.co/edit/IHKQNmUHVcP8feeC?open=main.js

3/ Using the bodyScroll event. You can save/restore the vertical and horizontal scroll positions using internal API members (as internal members they are subject to change):

  1. Open Example (JS): https://plnkr.co/edit/STPMHNk8wW2gl6S4  Example (Angular): https://plnkr.co/edit/QrDOQiw4Zs2OuW11
  2. Scroll the grid down
  3. Scroll the grid right
  4. Refresh the plunker preview using the small refresh button in the plunker preview pane (not the entire browser window) Actual & Expected: Grid reloads, earlier scrollbar positions restored

Note in this sample the scroll position gets saved to local storage inside the onBodyScroll event handler. Also, onBodyScroll event handler gets called when the grid first initializes so we ignore the first call. The scroll positions get restored inside the firstDataRendered event handler which gets fired when the grid is rendered for the first time.

AG-Zoheil commented 1 year ago

As this has been open without any further interaction for a while, I will be closing this issue.

If the issue has already been resolved by other means, you can ignore this. If however, you are still in need of support you can choose to raise the issue again for the AG Grid community to support.

Kind regards, Zoheil

sonicviz commented 1 year ago

Closing issues that are not resolved only to have them reopened? Why?

AG-Zoheil commented 1 year ago

Hi @sonicviz,

Thank you for your response. Leaving older issues open will only clog up the view and make it more difficult for both AG-Grid and the wider community to provide support. Also at times, our users find workarounds themselves, or upgrade to newer versions that solve the issue for them.

Following up on this thread, besides the lengthy explanation Kiril gave in the last comment, we have been waiting for a reproducible example/sample project. Not to open up the conversations surrounding that again, but the point remains that we will not be able to effectively look into this until we can reproduce it.

In any case, if you are able to provide an example/sample project we'd be more than happy to take a look and of course to re-open the issue. If you need any further guidance on that, please do let us know.

Kind regards, Zoheil

sonicviz commented 1 year ago

Kiril gave in the last comment, we have been waiting for a reproducible example/sample project. Not to open up the conversations surrounding that again, but the point remains that we will not be able to effectively look into this until we can reproduce it.

This tells me you didn't really listen to what I said. You cannot solve bugs with complex user issues with the only method being "give us a reproducable project or we can't do anything". Software doesn't work like that. It's only under user conditions that certain issues arise.

I've recently solved two extremely hard user issues with my addons using AG-grid that both involved elements of remote debugging, giving the users debug builds in order to gain insight into issues within their context. No way could I ask them for a reproducable context, it just wasn't possible. You need to use other methods to solve issues, and it's not an option to tell them "sorry, can't do anything".

Your approach to demanding reproducable projects is a very limited system for solving hard problems that only show up in production on user systems.

Why doesn't the logging work? Why isn't their more extensive debugging insight?

If there was I might be able to resolve it myself. It would also, ultimately, benefit you to have better debug and logging tools in the application so users are better enabled to solving problems they may encounter in production environments.

SalmanVolvoGit commented 1 month ago

So was any solution related to this provided from Ag-grid side ?

sonicviz commented 1 month ago

No, no solution. Not interested in any of my feedback/suggestions either. 🤷‍♀️