Skip to content

fix: fractional grid height leaves the bottom rows unrendered - #2700

Merged
ghiscoding merged 1 commit into
masterfrom
fix/1262-fractional-height-bottom-render
Aug 6, 2026
Merged

fix: fractional grid height leaves the bottom rows unrendered#2700
ghiscoding merged 1 commit into
masterfrom
fix/1262-fractional-height-bottom-render

Conversation

@ghiscoding

@ghiscoding ghiscoding commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Port bug fix from 6pac/SlickGrid PR 6pac/SlickGrid#1263 into slickgrid-universal

Fixes 6pac/SlickGrid#1262.

Root cause

Two different "maximum scrollTop" values disagree by a sub-pixel amount:

value at the bottom (height: 430.1px, rowHeight 20, 100 rows)
_handleScroll() ceiling — scrollHeight - clientHeight (integer, and equal to the browser's real max) 1611
scrollTo() ceiling — th - getBoundingClientRect().height + scrollbarH (fractional) 1610.90625
clientHeight is rounded, so when the height's decimal part is below .5 it rounds down and the browser's real maximum scrollTop ends up above the ceiling scrollTo() clamps to. At .5 or above — and on integer heights — it rounds up, the gap disappears, and nothing goes wrong. That is exactly the threshold reported in the issue.

A mouse wheel that overshoots the bottom is clamped to 1611 by _handleScroll(), and scrollTo() then lowers it to 1610.90625. Because newScrollTop !== prevScrollTop, scrollTo() takes its "the position moved" branch — which assigned lastRenderedScrollTop as if a render had happened:

this.lastRenderedScrollTop = (this.scrollTop = this.prevScrollTop = newScrollTop);

Back in _handleScroll(), dy = Math.abs(this.lastRenderedScrollTop - this.scrollTop) is therefore 0, the dy > 20 gate fails, and render() is never called. Rows that the previous render had cleaned up stay missing, and the bare viewport shows through at the bottom of the grid. Continued scrolling repeats the same path, which is why the issue reports that scrolling does not restore the grid — only an unrelated re-render (resizing a column, selecting a row) does.

Fix

scrollTo() no longer claims a render it did not do — lastRenderedScrollTop is now set only by render():

-      this.lastRenderedScrollTop = (this.scrollTop = this.prevScrollTop = newScrollTop);
+      this.scrollTop = this.prevScrollTop = newScrollTop;

Every other scrollTo() caller (scrollRowIntoView, scrollRowToTop, scrollPage) calls render() immediately afterwards, so no render is lost. The branch this touches is only entered when scrollTo() genuinely moves the position — the fractional-height case above and virtual paging (offset change) — where a render was needed and was wrongly suppressed in both.

Regression test

cypress/e2e/quirk-fractional-height-bottom-render.cy.ts is self-hosting (harness served via cy.intercept, no page added to examples/), following the quirk-frozen-bottom-cell-cleanup precedent. It reproduces the issue's exact geometry, and asserts two preconditions before the real check so it cannot silently pass for the wrong reason:

  1. the DOM's maximum scrollTop really is greater than the position scrollTo() settles on;
  2. the wheel-up really did clean up the bottom rows.

Then it wheels back down past the bottom in a single event and asserts no visible row is left unrendered.

Verification

  • Fails on the unfixed build (both retry attempts): [94, 95, 96, 97, 98, 99] missing — the Cypress failure screenshot shows the bare viewport band below row 93. Passes with the fix.
  • Height sweep in Chrome (430, 430.1, 430.25, 430.49, 430.5, 430.6, 430.75, 430.9, 429.3, 431.4): 0 missing rows on all; before the fix every sub-.5 height lost 6 rows.
  • Full Cypress suite: 640 tests, 638 passing, 2 pending, 0 failing. tsc and eslint clean.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (57e4705) to head (c59b964).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2700   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         200      200           
  Lines       25465    25465           
  Branches     8999     8999           
=======================================
  Hits        25465    25465           
Flag Coverage Δ
angular 100.0% <ø> (ø)
universal 100.0% <100.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ghiscoding ghiscoding added the AI label Aug 6, 2026
@pkg-pr-new

pkg-pr-new Bot commented Aug 6, 2026

Copy link
Copy Markdown
angular-slickgrid

npm i https://pkg.pr.new/angular-slickgrid@2700

aurelia-slickgrid

npm i https://pkg.pr.new/aurelia-slickgrid@2700

slickgrid-react

npm i https://pkg.pr.new/slickgrid-react@2700

slickgrid-vue

npm i https://pkg.pr.new/slickgrid-vue@2700

@slickgrid-universal/angular-row-detail-plugin

npm i https://pkg.pr.new/@slickgrid-universal/angular-row-detail-plugin@2700

@slickgrid-universal/aurelia-row-detail-plugin

npm i https://pkg.pr.new/@slickgrid-universal/aurelia-row-detail-plugin@2700

@slickgrid-universal/react-row-detail-plugin

npm i https://pkg.pr.new/@slickgrid-universal/react-row-detail-plugin@2700

@slickgrid-universal/vue-row-detail-plugin

npm i https://pkg.pr.new/@slickgrid-universal/vue-row-detail-plugin@2700

@slickgrid-universal/binding

npm i https://pkg.pr.new/@slickgrid-universal/binding@2700

@slickgrid-universal/common

npm i https://pkg.pr.new/@slickgrid-universal/common@2700

@slickgrid-universal/composite-editor-component

npm i https://pkg.pr.new/@slickgrid-universal/composite-editor-component@2700

@slickgrid-universal/custom-footer-component

npm i https://pkg.pr.new/@slickgrid-universal/custom-footer-component@2700

@slickgrid-universal/custom-tooltip-plugin

npm i https://pkg.pr.new/@slickgrid-universal/custom-tooltip-plugin@2700

@slickgrid-universal/empty-warning-component

npm i https://pkg.pr.new/@slickgrid-universal/empty-warning-component@2700

@slickgrid-universal/event-pub-sub

npm i https://pkg.pr.new/@slickgrid-universal/event-pub-sub@2700

@slickgrid-universal/excel-export

npm i https://pkg.pr.new/@slickgrid-universal/excel-export@2700

@slickgrid-universal/graphql

npm i https://pkg.pr.new/@slickgrid-universal/graphql@2700

@slickgrid-universal/odata

npm i https://pkg.pr.new/@slickgrid-universal/odata@2700

@slickgrid-universal/pagination-component

npm i https://pkg.pr.new/@slickgrid-universal/pagination-component@2700

@slickgrid-universal/pdf-export

npm i https://pkg.pr.new/@slickgrid-universal/pdf-export@2700

@slickgrid-universal/row-detail-view-plugin

npm i https://pkg.pr.new/@slickgrid-universal/row-detail-view-plugin@2700

@slickgrid-universal/rxjs-observable

npm i https://pkg.pr.new/@slickgrid-universal/rxjs-observable@2700

@slickgrid-universal/sql

npm i https://pkg.pr.new/@slickgrid-universal/sql@2700

@slickgrid-universal/text-export

npm i https://pkg.pr.new/@slickgrid-universal/text-export@2700

@slickgrid-universal/utils

npm i https://pkg.pr.new/@slickgrid-universal/utils@2700

@slickgrid-universal/vanilla-bundle

npm i https://pkg.pr.new/@slickgrid-universal/vanilla-bundle@2700

@slickgrid-universal/vanilla-force-bundle

npm i https://pkg.pr.new/@slickgrid-universal/vanilla-force-bundle@2700

@slickgrid-universal/web-mcp

npm i https://pkg.pr.new/@slickgrid-universal/web-mcp@2700

commit: c59b964

@ghiscoding
ghiscoding merged commit 36a03d9 into master Aug 6, 2026
14 checks passed
@ghiscoding
ghiscoding deleted the fix/1262-fractional-height-bottom-render branch August 6, 2026 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When scrolling while the grid height is a decimal, rows may not appear at the bottom of the grid.

1 participant