search

6pac / SlickGrid

A lightning fast JavaScript grid/spreadsheet
https://stackblitz.com/github/6pac/SlickGrid/tree/master/vite-demo
MIT License
1.85k stars 424 forks source link

fix: ensure DOM alterations during initialization are always cleaned up when there are hidden parents and forceFitColumns=true #1085

Closed jonathanjytang closed 2 days ago

jonathanjytang commented 5 days ago

Fixes Issue #1084

jonathanjytang commented 5 days ago

Since I wasn't sure if anyone else is using the existing protected fields _hiddenParents and oldProps, this change was made in a way that avoids changing the signatures of those fields.

ghiscoding commented 5 days ago

The changes seems ok to me, but like I said I never really worked on these functions, so maybe a review by @6pac might be good too. Let's see if all the Cypress E2E tests are passing or not, that would be a good indication too (edit: all tests are passing)

jonathanjytang commented 4 days ago

I think it should be easy enough to add a flag so that repeat calls to cacheCssForHiddenInit don't have any effect - it should cache the CSS data only once. That's also a good point about a flag to disable this altogether so that the external code can handle it.

@6pac I've changed the PR as suggested so the CSS data is only cached once when the methods are called in a "nested" manner, which fixes the correctness issue in #1084. The change effectively ensures that when there's an nested call to the cacheCssForHiddenInit and restoreCssFromHiddenInit methods in SlickGrid code (only possible in autosizeColumns), the "inner" methods calls aren't run -- it seemed like the most straightforward way to ensure an "inner" nested restoreCssFromHiddenInit call is a no-op (for correctness), ie when forceFitColumns=true the sequence of 1) cacheCss -> 2) cacheCss -> 3) restoreCss -> 4) restoreCss having 2) and 3) be no-ops.

With this change, it seems to me users who want to modify hidden parents externally won't be affected by the cacheCssForHiddenInit / restoreCssFromHiddenInit calls within autosizeColumns, since the modified CSS does not show up outside of SlickGrid methods (unlike the initial request for options.suppressCssChangesOnHiddenInit where explicitInitialization=true was used and the issue was CSS changes observed in-between the two stages of initialization). Though I suppose future contributors can add a flag if they so desire.

6pac commented 3 days ago

Thanks, this works.

ghiscoding commented 3 days ago

@6pac feel free to merge if you think this PR is good, it seems fine from my point of view. Cheers