Fix for CSS Rendering Issue at large row counts

[pbower]

Describe the bug

Hi @ghiscoding ,

Hope you're doing well.

Wondering if you have a moment to take a quick look at a bug / suggested fix?

Problem: I recently encountered a CSS rendering issue at large counts:

See correct styles here for comparison:

Solution: I found that it relates to this line of code in appendRowHTML for slick.grid.ts:

   const frozenRowOffset = this.getFrozenRowOffset(row);
    const rowDiv = Utils.createDomElement('div', { className: `ui-widget-content ${rowCss}`, role: 'row', style: { top: `${this.getRowTop(row) - frozenRowOffset}px` } });
    let rowDivR: HTMLElement | undefined;
    divArrayL.push(rowDiv);
    if (this.hasFrozenColumns()) {
      // it has to be a deep copy otherwise we will have issues with pass by reference in js since
      // attempting to add the same element to 2 different arrays will just move 1 item to the other array
      rowDivR = rowDiv.cloneNode(true) as HTMLElement;
      divArrayR.push(rowDivR);
    }

The following fix resolves the issue, and the repo tests still seem ok:

      const frozenRowOffset = this.getFrozenRowOffset(row);
      const topValue = this.getRowTop(row) - frozenRowOffset; // Calculated top value
      const rowDiv = Utils.createDomElement('div', {
        className: `ui-widget-content ${rowCss}`,
        role: 'row',
        style: { transform: `translateY(${topValue}px)` } // Using transform instead of top
      });

If this all looks ok to you, wondering if a special access is required to push a branch with the fix ? As I tried to but it said access denied.

Thanks, Pete

Reproduction

Grid with approx 10m rows with grid line styles . - > close to 10m it will encounter rendering issues.

Which Framework are you using?

React

Environment Info

System:
    OS: Linux 6.8 Ubuntu 24.04 LTS 24.04 LTS (Noble Numbat)
    CPU: (24) x64 12th Gen Intel(R) Core(TM) i9-12900KF
    Memory: 51.57 GB / 62.58 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 20.15.1 - ~/.nvm/versions/node/v20.15.1/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v20.15.1/bin/yarn
    npm: 10.8.2 - ~/.nvm/versions/node/v20.15.1/bin/npm
  Browsers:
    Chrome: 126.0.6478.182

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]

This seems slightly related to issue #297

cc @6pac

So in your code, you seem to completely replace the top style with transform of an Y value? What happens to the next few lines for the frozen column with the clone node? You're not showing them in the bottom portion of the code change, so I'm unsure if you're removing them or what not!?

With this change however, this will most probably break all Cypress E2E tests because a few of these tests rely on the top style to exist and query against it for the test, for example because the SlickGrid Virtual Scroll can replace and shift rows in the DOM (not visually but in the DOM tree), I rely on the top to get whatever rows I want. What I mean is that if you filter some values in SlickGrid and you scroll down and up, you can't rely on let say give me 3rd row of querySelectorAll('.slick-row') because this might give you row 10 instead, however we can use top to get 3rd row (row height * 3 => top: 75px) which is convenient in a Cypress test and I use it often... so changing top to transform might have a lot of implications. I think this is a risky move, other users might also be using the same technique in their own Cypress tests too (or any other kind of testing or even in UI code, who knows), so this might be a Breaking Change (aka requiring a new major version), so it's not a small change

wondering if a special access is required to push a branch with the fix ? As I tried to but it said access denied.

That's the default behavior of GitHub and any other repos using git, no one in the world has access to push directly to a repo. You need to fork the repo and then push to your fork. After pushing to your fork, it will then ask you if you want to create a Pull Request to push to the original repo (here). So it's not a 1 step push, but rather a 3 steps push (unless you're a contributor/maintainer of this repo which are only 6pac and myself).

This Video explains how to contribute to an open source project: Your First GitHub Pull Request (in 10 minutes)

[pbower]

Hey there,

Great, thanks for looking into this so quickly.

Please find responses below:

1. Frozen Column - this is the rest of the method:

   
   protected appendRowHtml(divArrayL: HTMLElement[], divArrayR: HTMLElement[], row: number, range: CellViewportRange, dataLength: number) {
     const d = this.getDataItem(row);
     const dataLoading = row < dataLength && !d;
     let rowCss = 'slick-row' +
       (this.hasFrozenRows && row <= this._options.frozenRow! ? ' frozen' : '') +
       (dataLoading ? ' loading' : '') +
       (row === this.activeRow && this._options.showCellSelection ? ' active' : '') +
       (row % 2 === 1 ? ' odd' : ' even');
   
     if (!d) {
       rowCss += ' ' + this._options.addNewRowCssClass;
     }
   
     const metadata = (this.data as CustomDataView<TData>)?.getItemMetadata?.(row);
   
     if (metadata?.cssClasses) {
       rowCss += ' ' + metadata.cssClasses;
     }
   
     const frozenRowOffset = this.getFrozenRowOffset(row);
     const topValue = this.getRowTop(row) - frozenRowOffset; // Calculated top value
     const rowDiv = Utils.createDomElement('div', {
       className: `ui-widget-content ${rowCss}`,
       role: 'row',
       style: { transform: `translateY(${topValue}px)` } // Using transform instead of top
     });
   
     let rowDivR: HTMLElement | undefined;
     divArrayL.push(rowDiv);
     if (this.hasFrozenColumns()) {
       rowDivR = rowDiv.cloneNode(true) as HTMLElement;
       divArrayR.push(rowDivR);
     }
   
     let colspan: number | string;
     let m: C;
     for (let i = 0, ii = this.columns.length; i < ii; i++) {
       m = this.columns[i];
       if (!m || m.hidden) { continue; }
   
       colspan = 1;
       if (metadata?.columns) {
         const columnData = metadata.columns[m.id] || metadata.columns[i];
         colspan = columnData?.colspan || 1;
         if (colspan === '*') {
           colspan = ii - i;
         }
       }
   
       // Do not render cells outside of the viewport.
       if (this.columnPosRight[Math.min(ii - 1, i + (colspan as number) - 1)] > range.leftPx) {
         if (!m.alwaysRenderColumn && this.columnPosLeft[i] > range.rightPx) {
           // All columns to the right are outside the range.
           break;
         }
   
         if (this.hasFrozenColumns() && (i > this._options.frozenColumn!)) {
           this.appendCellHtml(rowDivR!, row, i, (colspan as number), d);
         } else {
           this.appendCellHtml(rowDiv, row, i, (colspan as number), d);
         }
       } else if (m.alwaysRenderColumn || (this.hasFrozenColumns() && i <= this._options.frozenColumn!)) {
         this.appendCellHtml(rowDiv, row, i, (colspan as number), d);
       }
   
       if ((colspan as number) > 1) {
         i += ((colspan as number) - 1);
       }
     }
   }

2. **Cypress Tests**
I have ran the tests and can confirm that the Cypress tests do not pass (based on the issue mentioned - the grid itself seems to be functioning well). It sounds like whilst the fix seems straightforward that it throws off the tests? I'm not exactly sure what to say in response to this, as I probably don't know enough about the areas you've highlighted there to suggest anything helpful at this stage. 

However, with regards to the bugfix, in terms of motivational sentiment I remember coming across documentation or possibly a github issue when first using Slickgrid about a year back, and it said that '2 billion rows are supported'. This made me very excited, as it was a big promise. I'm praying that can pull through, and at 100m so far and counting!. 

3. **Github issues**
Brilliant, thanks for that! Super helpful.
Happy to fork it and submit it for review?

[ghiscoding]

Yes some people use it with extremely large dataset, I did see someone using that much data but I would never go with that much size myself (it's not recommended).

I already mentioned exactly why the Cypress tests will fail, I'm often using the top with a querySelector to find the x row in the grid and now you want to completely remove top and replace it with transform, so yes of course that will fail the tests and potentially break a few user's code. This really looks like a Breaking Change to me

I don't want to use compare tools to find out what really change, it would be helpful if you could use the ```diff instead and add + for new/change with lines of code and - for deleted/changed previous lines of code with, for example

- hello world
+ Hello World!

I'm not too sure that I want to proceed considering that this is most probably a Breaking Change.

[pbower]

Look I'm just trying to help and be responsive, by contributing what I believe is a code fix to a genuine bug that I encountered when using the library. Above, I've run the tests in response to the comments.

Please find the requested diff details in the attached .diff zipFile changes-diff.zip.

That's completely understandable if it breaks a lot of tests and creates a lot of work. I thought I'd take the time to raise it, so others could benefit from the fix.

Thanks

- 
-        }
+      let rowDivR: HTMLElement | undefined;
+      divArrayL.push(rowDiv);
+      if (this.hasFrozenColumns()) {
+        rowDivR = rowDiv.cloneNode(true) as HTMLElement;
+        divArrayR.push(rowDivR);
       }

-      // Do not render cells outside of the viewport.
-      if (this.columnPosRight[Math.min(ii - 1, i + (colspan as number) - 1)] > range.leftPx) {
-        if (!m.alwaysRenderColumn && this.columnPosLeft[i] > range.rightPx) {
-          // All columns to the right are outside the range.
-          break;
+      let colspan: number | string;
+      let m: C;
+      for (let i = 0, ii = this.columns.length; i < ii; i++) {
+        m = this.columns[i];
+        if (!m || m.hidden) { continue; }
+
+        colspan = 1;
+        if (metadata?.columns) {
+          const columnData = metadata.columns[m.id] || metadata.columns[i];
+          colspan = columnData?.colspan || 1;
+          if (colspan === '*') {
+            colspan = ii - i;
+          }
         }

-        if (this.hasFrozenColumns() && (i > this._options.frozenColumn!)) {
-          this.appendCellHtml(rowDivR!, row, i, (colspan as number), d);
-        } else {
+        // Do not render cells outside of the viewport.
+        if (this.columnPosRight[Math.min(ii - 1, i + (colspan as number) - 1)] > range.leftPx) {
+          if (!m.alwaysRenderColumn && this.columnPosLeft[i] > range.rightPx) {
+            // All columns to the right are outside the range.
+            break;
+          }
+
+          if (this.hasFrozenColumns() && (i > this._options.frozenColumn!)) {
+            this.appendCellHtml(rowDivR!, row, i, (colspan as number), d);
+          } else {
+            this.appendCellHtml(rowDiv, row, i, (colspan as number), d);
+          }
+        } else if (m.alwaysRenderColumn || (this.hasFrozenColumns() && i <= this._options.frozenColumn!)) {
           this.appendCellHtml(rowDiv, row, i, (colspan as number), d);
         }
-      } else if (m.alwaysRenderColumn || (this.hasFrozenColumns() && i <= this._options.frozenColumn!)) {
-        this.appendCellHtml(rowDiv, row, i, (colspan as number), d);
-      }

-      if ((colspan as number) > 1) {
-        i += ((colspan as number) - 1);
+        if ((colspan as number) > 1) {
+          i += ((colspan as number) - 1);
+        }
       }
-    }
   }

   protected appendCellHtml(divRow: HTMLElement, row: number, cell: number, colspan: number, item: TData) { 

[ghiscoding]

Yeah sure I'm open to comments and discussions, what I meant to say is that I just think that we have to consider the potential side effect in doing this kind of changes because that could impact end users which is something that I always try to avoid (even if it fixes a bug, it might still be a Breaking Change because of the behavior change). Taking a look at your new code, it's more than I expected, have you tested frozen grids and all? The Cypress tests would eventually for sure all have to pass (probably by just changing top query selector to transform)

For example with the Cypress test shown below, it ask for top: ${GRID_ROW_HEIGHT * 0}px; which is basically the 1st grid row, then * 1 is 2n row, and so on and so on. I use this approach because of the SlickGrid Virtual Scroll that always change the div order in the DOM, the only constant is the top which is how it works in the UI and is good to use in the Cypress tests

https://github.com/6pac/SlickGrid/blob/87b8c6af4bed91bb646971b96d81185f346e7d2c/cypress/e2e/example-autotooltips.cy.ts#L39-L41

So in the end, I didn't mean that I will refuse the PR, but I meant to say is that if it's really the only way to go, then that would potentially (probably) require a major version v6.0 because of the Breaking Change behavior (changing from top to transform) and that is because of the potential side effect that could break user's code or expectations. However doing a major version for this small change seems a little too much... so my question is, is it really the only way to fix this issue? isn't there a way that we can keep the top somehow and still fix the issue without requiring a major version? If it's really the only way, then I find the list of changes for a major version to be very light.

I wonder why a transform is working well but top isn't always working as well? Do you have articles about this kind of change and issues? How did you come up with such a solution?

[pbower]

Sure, no problem, I appreciate that. Thanks.

Ok, so with regards to translate vs top, I’d been reading a book on front end which suggested that translate gets put on the GPU and accelerates paint times, compared to top which (generally) has longer paint times. The video at this link gets to the crux at 7:48 https://www.paulirish.com/2012/why-moving-elements-with-translate-is-better-than-posabs-topleft/ , and makes a convincing argument using browser tools to measure paint times. Part of me is wondering whether this is still the case in 2024 (?), but it’s interesting for sure. Also, apparently translate handles larger numbers better, which is why I initially went to it as assumed I’d hit some weird overflow issues due to the number sizes.

With regards to the versioning, I understand your concerns. For a personal project, I’m at the point where I need to make a call on Slickgrid, and it’s been very promising in most areas. However a related area that’s causing a few issues is the repainting times, affecting the frame rate after fast scrolling. It’s still very fast data though not 45-60 FPS fast enough that casual users might expect from a spreadsheet-like tool, in that when holding down scroll (medium speed - not the slow arrow scroll, or moving the bar, but the scroll bar space between) the data and grid doesn’t repaint in time for it to look like the data is being scrolled through. It does manage to get 30% of the data in time though (which has the effect of looking like the data is chasing the grid). I should note that this is compounded by the way data is brought in for my specific use case and, but that has already been heavily optimised and therefore I believe any achievable optimisations on the library code could still have potential to benefit the general library.

A related UX drawback I’ve experienced is the white flashing after scrolling which as I understand from one of your stack overflow posts is part of the virtual scrolling, but Ive wondered if there is still maybe some opportunity here? Perhaps not? But it’s still a different UX to say Excel/google sheets, and I felt concerned that a user might get disoriented. Not for jumping from say row 1000 to 30,000, but for scrolling between adjacent numbers (at high speed) I feel it would be awesome if it could be a bit smoother.

Why am I raising this? Well if it’s is ok with you I’d like to have an go at diving deeper on the performance side to see if I can profile it, review the internals in more detail and potentially recommend further adjustments?

Can’t promise it will bear any fruit, as I know you guys have worked with this for a long time, but if I were to break any ground (rather than tests 😅) and bundle a few related changes up, try pass the tests, then would you be happy to take a look at it ? I would have no issue if it sat on the back burner until it made sense to bundle as part of a broader release, or otherwise I can always just run my own patch which I’ve done for a couple of small things, which would be fine. Anyway, keen to have a crack and see if it’s possible to speed up the fast-scroll rendering, if this ok?

Note - would expect up to a 4-8 week turnaround with other commitments, getting familiar with the library and some of the front end optimisations, depending on how difficult it is. Just to help set expectations.

Thanks

[pbower]

Note I should have mentioned if it’s a matter of porting tests I’m happy to check that out at the same time.

[pbower]

Edit: Also noting that the above includes tweaking the library debounce setting from 50ms to 5ms, which helped the data appear partially on the grid during scroll.

[6pac]

From my point of view, the issues you raise are very valid and I'd definitely support working on them.
It probably remains to be seen what the impact would be on the codebase, but if there were very tangible visual improvements, then I'd say it would be worth some degree of pain to implement them.

I'll let @ghiscoding speak for himself, but I suspect he'll say the same thing.

[ghiscoding]

There's a throttle in place which makes the virtual scroll wait before rendering the next batch. It's currently set to 50ms, you can try to decrease it. I think it was added by Ben a long time ago (it doesn't seem to exist in the original SlickGrid repo though). You have a large dataset so you are the best person who could give it a try with maybe 0 and see what happens (there's more info in old PR #810).

https://github.com/6pac/SlickGrid/blob/87b8c6af4bed91bb646971b96d81185f346e7d2c/src/slick.grid.ts#L290

I actually always wondered why that was put in place, so perhaps you could try to disable it completely, something like this maybe. So if you comment out the following line (and where it's being used, which is the enqueue and dequeue). There's a good chance that it would make the rendering experience worst which would help explain why this code was put in place, but without really giving it a try on a large dataset like yours, it's hard to know for sure without testing it.

https://github.com/6pac/SlickGrid/blob/87b8c6af4bed91bb646971b96d81185f346e7d2c/src/slick.grid.ts#L663

https://github.com/6pac/SlickGrid/blob/87b8c6af4bed91bb646971b96d81185f346e7d2c/src/slick.grid.ts#L4918

https://github.com/6pac/SlickGrid/blob/87b8c6af4bed91bb646971b96d81185f346e7d2c/src/slick.grid.ts#L5105

there's also a grid option for how many rows to keep in the buffer when scrolling, which in other words mean how many row do we want to cache and make ready before the scroll happens. I'm not sure if it has any impacts or not but maybe try to increase it a bit?

https://github.com/6pac/SlickGrid/blob/87b8c6af4bed91bb646971b96d81185f346e7d2c/src/slick.grid.ts#L283

Note I should have mentioned if it’s a matter of porting tests I’m happy to check that out at the same time.

it would be part of it, but the question is more about "Do we really want to do a Breaking Change version with just this change? or can we do a bug fix that will not introduce a Breaking Change". Because in my head, I'm really consider the top to transform to be a Breaking Change and I'm trying hard to not push a new major yet again, but if it's really the only solution that works then yeah we'll go ahead with it.

Doing some tests on a large dataset with the area that I mentioned above might be where the perf can be improved. I always wondered what side effect it could have to go without this code. I think there's probably a good (better) number to come up with that will make it look more fluid while not making worst either. I mean without the render throttle, it might make it look worst because the browser can't keep up, but again we won't know until someone is performing local tests of that. Browser are getting better every year so the throttle we needed 5 years ago might not be the same or as worth it today (it's also probably a good idea to test with more than 1 browser).

So anyway, doing more tests and thinking about what else we could include in a Breaking Change version. I mean, if there are other breaking changes, now would be the good time

[6pac]

not aware of ever having added the throttle, but if you have evidence, then I suppose it was me! Way back when this was happening, there was much less standardisation between browsers so it could well have been a cross-browser compromise. I think in the modern context we would be much more confident in the stability of a more optimal solution.

[ghiscoding]

not aware of ever having added the throttle, but if you have evidence, then I suppose it was me! Way back when this was happening, there was much less standardisation between browsers so it could well have been a cross-browser compromise. I think in the modern context we would be much more confident in the stability of a more optimal solution.

haha yeah it's not about pointing fingers, but more about the reason why it was put in place. That might have been copied from another fork too, there was a bunch of different forks that existed at that point in time too so who knows where and why this code was put in place is hard to remember ...

anyway, you can see similar discussions we had about this a while ago in this comment and the commit where you added this throttling is 93f043fa

[6pac]

So I did. I'm pretty sure I didn't write that code though - it must have been either patched in from another repo or contributed in an issue. So in summary, I don't know exactly why it was added. It does definitely make sense to have some throttling for long scrolls (that was probably my logic at the time), but there appears to be some room for improvement.

[pbower]

Great. Thanks for your comments too @6pac. Well this sounds like a great place to start @ghiscoding - I’ll run those preliminary tests over the weekend, and post back with some benchmark comparisons on the parameters listed. If I learn anything else when diving I’ll include it. If there’s anything else specific like that just let me know . Cheers.

[ghiscoding]

anyway, you can see similar discussions we had about this a while ago in this comment and the commit where you added this throttling is 93f043f

@pbower there's a bit more info in that comment (and thread) that I referenced, it seems like 5ms was a good number to use when I tested it at that time.

[ghiscoding]

ohhh wait a second, I just found how to push this into a new version without making a Breaking Change. We could simply add this into a grid option keep top as the default and that's it. You'll be able to change it to a transform while not breaking other expectations and everyone is happy.. woohoo! 🎉 something like this in the GridOption interface topStyleRenderType: 'top' | 'transform' (I'm not too crazy about the name, I'm sure you can find better) would be the default and then just use either or in the code...

 const rowDiv = Utils.createDomElement('div', {
      className: `ui-widget-content ${rowCss}`,
      role: 'row',
-      style: { transform: `translateY(${topValue}px)` } // Using transform instead of top
});

+ if (options.topStyleRenderType === 'transform') {
+   rowDiv.style.transform = `translateY(${topValue}px)`;
+ } else {
+   // else default to `top`
+   rowDiv.style.top = `${topValue}px`;
+ }

and voila! Perf improvements is now doable on your side and it won't break users who are still expecting top because that would remain the default. If we ever decide to do a major version in the future, if it ever happen, then we could simply switch the default to transform if we wanted.

Also for the other portion of code that I reference above (scrollThrottle), it always wraps it in a setTimeout but perhaps what we could do is if the value is 0 then don't use the setTimeout neither the throttle callback but when the value is over 0 then use it the way it currently is (that code should be easy enough to change without breaking anything)

EDIT

I also see this comment in the article you referenced, which was very instructive. I think we are already using an integer for the top value but if not then please make sure it is (with Math.ceil() probably)

[6pac]

@pbower might be woth checking this out too: https://www.thecandidstartup.org/2024/04/29/modern-react-virtual-scroll-grid-9.html

[pbower]

Awesome, thanks for that. Just reaching out with an update - I did have some good success on the weekend both from a profiling and then action-ing perspective. I made a few key updates across :

Apply Html Code Append Cell Html Append Row Html Reorder Rows Render

These changes focused on batching DOM updates.

However, I then a roadblock when I went to run the tests, and unfortunately knocked out a few of the key event listeners in the process. So I will go back and QA it properly next weekend when I get some time and do that before sharing back the code and comparisons, otherwise it's kind of cheating I think.

Thanks

[pbower]

Wow that's a good article. Will definitely review that in detail.

[pbower]

Really curious that repo there - I ran the infinigrid examples - 1 follower / star repo (i.e., like I was the first one to visit it), casually with a 3 trillion row grid sitting there. Seems like he's on a mission.

[6pac]

Yep. From what I can tell, former rockstar programmer gets tired of the stress, makes a well compensated exit and has a lot of time on his hands. My dream.

[ghiscoding]

@pbower we didn't hear from your side, so I went ahead and done couple of small PRs related to this issue

• added a new grid option rowTopOffsetRenderType?: 'top' | 'transform', so that you can start using it without breaking other users since the default remains top (see PR #1050)
  • Note: I modified the Example - Grouping (ESM) to use the transform, so you can also see it live) https://github.com/6pac/SlickGrid/blob/dc08ab801ccd69b4875bcaf22b2c36a80fc192b7/src/slick.grid.ts#L3950-L3957
• decreased scrollRenderThrottling grid option to 10ms instead of previously 50ms, this should reduce the time it shows a blank section when scrolling to some rows that are uncached (for example scroll completely to the bottom of the grid)
• I also found that forceSyncScrolling grid option (defaults to false) exists to completely bypass the scrollRenderThrottling

https://github.com/6pac/SlickGrid/blob/dc08ab801ccd69b4875bcaf22b2c36a80fc192b7/src/slick.grid.ts#L5113-L5118

This is all available in v5.12.0