tabulator icon indicating copy to clipboard operation
tabulator copied to clipboard

Fix errors when a grouped table is shown

Open janhaertel opened this issue 11 months ago • 4 comments

If a table has groups which are all collapsed and the VirtualDomVertical renderer is used, there are some issues scrolling the table:

Example https://jsfiddle.net/isaraystanz/tpgh5vze/11/

  • On initial display there is a horizontal scrollbar even though fitColumns is enabled and the colums do fit the view without a scrollbar being needed.

I noticed that RowManager.renderTable() will call VirtualDomVertical.renderRows() => _virtualRenderFill() which triggers GroupRows.virtualRenderFill(). In this function the minWidth is adjusted if there are only group rows causing a horizontal scrollbar to be displayed. Later in RowManager.renderTable(), the call to RowManager.layoutRefresh() will lead to filtColumns.scaleColumns() to be invoked, which changes the width of the columns. After this, the columns are thin enough to fit the view but the horizontal scrollbar is still being shown, due to the minWidth that was set. I added a call to GroupRows.virtualRenderFill() after this, so the minWidth will be adjusted and the horizontal scrollbar disappears.

  • The table cannot be scrolled to the bottom, because the view always jumps back to the top.

RowManager._virtualRenderFill() will call row.calcHeight() after rows have been rendered causing Group.outerHeight to be set internally. This is always 0 because the implementation for Group.calcHeight() is empty. The rowsHeight calculated later will then also be 0 since all groups are collapsed. When the view is scrolled so far, that the next rows have to be added at the bottom and some have to be removed from the top, VirtualDomVertical._removeTopRow() tries to sum up row heights until none have to be removed anymore. Since the height is always 0, all rows are added to a list of removable rows then all are removed. I added a minimal implementation to Group.calcHeight(), so height will not be 0.

  • The table scrolls to the top every time a group is collapsed/expanded.

Collapsing/Expanding a group will lead to_virtualRenderFill(), (GroupRows.updateGroupRows () ... => RowManager.reRenderInPosition() ...=> _virtualRenderFill()) which sets scrollTop after rendering. Here the position is not correct due to the groupRow height being empty. This is fixed by the code from the previous issue.

Is the implementation for GroupRow.calcHeight() empty on purpose? If so the height may have to be calculated differently.

These problems are also addressed in the github issues #4219 and #4525.

fixes #4219 fixes #4525

janhaertel avatar Jan 23 '25 09:01 janhaertel

I have tried these fixes and I no longer have scroll problems on my closed grouped table. Thanks @janhaertel and looking forward to seeing this merged!

pspsdev avatar Feb 12 '25 16:02 pspsdev

This pull request fixes many of my scrolling issues with the vertical virtual renderer and row groups, but not all. Scrolling down with these code changes now works smoothly: if you hold down the thumb of the scrollbar and move it down, it follows the mouse pointer and maintains its vertical distance from the mouse pointer. However, if you reach the bottom of the scroll area, hold down the scroll bar thumb and move it up again, you will notice jumps and an increasing distance between the scroll bar thumb and the mouse pointer.

The issue seems to arise from the line... table.insertBefore(row.getElement(), table.firstChild); ... in VirtualDomVertical.js in _addTopRow:

Adding an element to the top changes the position of the vertical scrollbar, and this triggers a scroll event with an increased scroll position value compared to the last scroll action.

The event listener attached in initialize() in RowManager.js recognizes this as a “scrolldown” action (even if the user has scrolled up).

This leads to a "_removeTopRow" and "_addBottomRow" call in VirtualDomVertical.js.

Kallius74 avatar Feb 21 '25 06:02 Kallius74

This is great! I'm trying to understand the whole situation carefully as well as looking into the issue @Kallius74 mentioned. Hopefully we can merge this soon!

azmy60 avatar Mar 24 '25 05:03 azmy60

@janhaertel Just checking in to see if you can make the change Azmi requested above? If so, we can merge once the change is made

rathboma avatar Apr 14 '25 19:04 rathboma