Problem with updating frozenColumn via setOptions

songyuchen415 commented 1 month ago

Describe the bug

I encountered two issues:

If the grid has a horizontal scrollbar and is scrolled to the right, calling setOptions to set 1 column as a frozen column (frozenColumn from -1 to 0) causes the header of the frozen column to display as blank.
Each time setOptions is called, the grid loses its horizontal scroll position.

Based on the observed behavior, I suspect the issue might be within this part of the setOptions implementation:

if (newOptions.frozenColumn) {
  this.getViewports().forEach(vp => vp.scrollLeft = 0);
  this.handleScroll(); // trigger scroll to realign column headers as well
}

It seems there is an issue with the condition in the if statement. If it is changed to the following, the aforementioned issues no longer occur:

if (!this.hasFrozenColumns() && newOptions.frozenColumn! as number >= 0) {
  this.getViewports().forEach(vp => vp.scrollLeft = 0);
  this.handleScroll(); // trigger scroll to realign column headers as well
}

Reproduction

Steps to reproduce issue no.1:

Modify the examples/example-frozen-columns-and-column-group.html in the git repository as follows: Change

<button onclick="setFrozenColumns(-1)">Remove Frozen Columns</button>
<button onclick="setFrozenColumns(2)">Set 3 Frozen Columns</button>

to

<button onclick="setFrozenColumns(-1)">Remove Frozen Columns</button>
<button onclick="setFrozenColumns(0)">Set 1 Frozen Columns</button>

Click the "Remove Frozen Columns" button.
Scroll the grid all the way to the right.
Click the "Set 1 Frozen Columns" button.
Observe that the header of the frozen # column does not display.

Which Framework are you using?

Other

Environment Info

System:
    OS: Windows 10 10.0.19045
    CPU: (8) x64 Intel(R) Core(TM) i7-9700K CPU @ 3.60GHz
    Memory: 17.24 GB / 63.92 GB
  Binaries:
    Node: 20.12.2 - C:\Program Files\nodejs\node.EXE
    npm: 10.6.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 126.0.6478.127
    Edge: Chromium (126.0.2592.113)
    Internet Explorer: 11.0.19041.4355

SlickGrid: 5.9.2

Validations

[X] Follow our Code of Conduct
[X] Read the Wikis.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion.
[X] The provided reproduction is a minimal reproducible example of the bug.

ghiscoding commented 1 month ago

That's not a problem with the library but rather something that must be implemented by the user (you). Anytime something changes that could affect the pre-header columns, you have to re-render the pre-header and it really should be handled by you not by the library itself. For example in Slickgrid-Universal, I have a separate service that subscribes to a bunch of these potential event that affects the pre-header and the onSetOptions is one of them

Slickgrid-Universal lines of code ref

// and finally we need to re-create after user calls the Grid "setOptions" when changing from regular to frozen grid (and vice versa)
        this._eventHandler.subscribe(grid.onSetOptions, (_e, args) => {
          // when user changes frozen columns dynamically (e.g. from header menu), we need to re-render the pre-header of the grouping titles
          if (args?.optionsBefore?.frozenColumn !== args?.optionsAfter?.frozenColumn) {
            this.delayRenderPreHeaderRowGroupingTitles(0);
          }
        });

If we look at the existing example, we can see similar approach but it currently only re-render the pre-header for column resize and/or column reorder, however onSetOptions should also be watched for changes but only when you intend to use Frozen Columns option.

https://github.com/6pac/SlickGrid/blob/8d4a2908798ad0ee1caac01b6e9a4c8c575c3c1f/examples/example-draggable-header-grouping.html#L543-L548

So in summary, I don't think we have a demo that has both Frozen enabled and Header Grouping (pre-header), but if we do then we should add the same kind of code logic in that demo... so again, this must be handled on the userland (you) not the library itself

xinchao-zhang commented 1 month ago

This has nothing to do with additional pre-header row though. Even with a vanilla grid and with no column grouping nor frozen column enabled, calling setOptions alone would make all the viewports lose their horizontal scrolling positions because of this check if (newOptions.frozenColumn) below:

if (newOptions.frozenColumn) {
  this.getViewports().forEach(vp => vp.scrollLeft = 0);
  this.handleScroll(); // trigger scroll to realign column headers as well
}

If I understand it correctly, the default frozenColumn is -1 instead of 0, so that branch is always taken even if there is no frozen column. Actually, the way it is at the moment I think the horizontal scroll position will always be lost whenever there is any change to the options, regardless of if the change is frozenColumn related or not, and regardless of whether the grid already has frozen columns or not (the only exception being that there is exactly one frozen column because in this case the newOptions.frozenColumn is 0).

I guess my question is whether it is reasonable to make the above logic specific to only when there is a material change to the frozenColumn settings?

xinchao-zhang commented 1 month ago

Btw, it is totally fine if something like this should/could be handled in the app space, but for this one the vp.scrollLeft = 0 is called too early - before the onSetOptions event is fired - hence by the time the app is notified there is no longer any way to recover the original scroll position.

ghiscoding commented 1 month ago

the line of code you are referring to came in PR #901 to address a bug mentioned in it. I think I've put if (newOptions.frozenColumn) { because the setOptions argument of newOptions will only run that condition when the frozenColumn is provide in the new options and it's rare that the user will remove the freezing column (-1) and the issue was happening with an index of 0 or higher. If it's causing problem for you and you would rather have newOptions.frozenColumn >= 0 then submit a Pull Request, I think there are Cypress E2E tests to cover this but I'm not 100% sure. This code change was the easiest way (and probably the only way) to handle the bug that is referenced inside PR #901. If you have a better approach without regressing, then submit a PR... maybe we could apply the condition only when pre-header is enabled but that probably won't help you anyway.

Actually, the way it is at the moment I think the horizontal scroll position will always be lost whenever there is any change to the options, regardless of if the change is frozenColumn related or not

no that's incorrect, the condition will only kick in when new options include the frozenColumn property (if you provide something setOptions({ rowHeight: 30 }) then that won't kick in)

This has nothing to do with additional pre-header row though. Even with a vanilla grid and with no column grouping nor frozen column enabled, calling setOptions alone would make all the viewports lose their horizontal scrolling positions because of this check if (newOptions.frozenColumn)

as mentioned above, this was introduced in PR #901 to fix a bug mentioned in the PR, there are real reason behind the fact that it scrolls back to the top/left position.

I guess my question is whether it is reasonable to make the above logic specific to only when there is a material change to the frozenColumn settings?

that is what it does, this condition only kicks in when the frozenColumn option is provided in the new options provided for example setOptions({ frozenColumn: 0 }) will run the condition but setOptions({ rowHeight: 30 }) won't run the condition.

to me the code is totally valid, ~however making sure it's only executed when the pre-header is enabled might be a good to add to the condition though~ (actually reviewed the bug provided and it also happens without the pre-header so the condition is actually correct the way it is)... removing this condition will reintroduce (regress) the bug mentioned in PR #901 so I will not remove this condition. Also for the message I sent earlier to re-render the pre-header, it's still something that needs to be done in most cases for the pre-header to be correct displayed, so even if you say it's not related, it is indirectly related.

For the reason as to why we have to scroll back to the origin (top/left), it seems that SlickGrid has problem to do proper calculation of header/cell positions when toggling frozen column, the best way I've found to avoid this problem was to scroll back to the origin and then SlickGrid calculates positions correctly.

songyuchen415 commented 1 month ago

Hi @ghiscoding,

As you mentioned, we can handle the issue of losing scroll position at the app level by passing delta newOptions. However, we found that the header getting misaligned when pinning the first column is a more severe issue, so I submitted the PR.

Reproduction:

@https://codepen.io/xinchao/pen/GRbqbYb
Scroll the grid to the right
Click "Pin additional column" to pin the first column, and observe the header getting misaligned
Continuing to click "Pin additional column" does not fix the header

ghiscoding commented 1 month ago

closed by PR #1039, I'll push a new release next weekend (you can remind me if I forget)

6pac / SlickGrid