fix(grid): restore trailing scroll emission so top rows aren't lost after fast scroll#17455
fix(grid): restore trailing scroll emission so top rows aren't lost after fast scroll#17455Zneeky wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Restores the intended RxJS throttle trailing-edge behavior for grid vertical scroll handling in IgxGridBaseDirective, addressing a regression where virtualization could remain at an intermediate startIndex after momentum scrolling back to the top (scrollbar settles at ~0 but top rows remain missing).
Changes:
- Passes an explicit
{ leading: false, trailing: true }configuration to the vertical scrollthrottle(...)operator to ensure the final “settle” scroll position is processed. - Adds an inline code comment documenting the rationale and the user-visible failure mode being prevented.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
projects/igniteui-angular/grids/grid/src/grid-base.directive.ts:3707
- This bug fix changes scroll/virtualization behavior but is only manually tested. Please add a regression unit test that simulates a burst of
scrollNotifyevents within one throttle window and asserts the trailing (final) event is processed (e.g.,verticalScrollHandleris invoked with the last scrollTop near 0 and the rendered start index updates accordingly). This helps prevent future regressions if the throttle logic is changed again.
this.scrollNotify.pipe(
filter(() => !this._init),
throttle(() =>
this.throttleTime$.pipe(
take(1),
switchMap(time => timer(time, this.throttleScheduler))
),
// `trailing: true` ensures the final settle position of a fast momentum
// scroll is processed; otherwise the last scroll events are dropped and the
// rows stay frozen at an intermediate startIndex while the scrollbar is at top.
// `leading: true` keeps the immediate response on scroll start.
{ leading: true, trailing: true }
),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
projects/igniteui-angular/grids/grid/src/grid-base.directive.ts:3706
- The PR description states the throttle config should be restored to
{ leading: false, trailing: true }, but the code sets{ leading: true, trailing: true }. Please confirm which behavior is intended and align the implementation and/or PR description accordingly (enablingleading: truechanges behavior vs the previously documented config and may increase scroll handler invocations).
// `trailing: true` ensures the final settle position of a fast momentum
// scroll is processed; otherwise the last scroll events are dropped and the
// rows stay frozen at an intermediate startIndex while the scrollbar is at top.
// `leading: true` keeps the immediate response on scroll start.
{ leading: true, trailing: true }
projects/igniteui-angular/grids/grid/src/grid-base.directive.ts:3701
- This change fixes a subtle virtualization/scroll regression, but there is no unit test coverage exercising the
scrollNotifythrottling pipeline (no spec referencesscrollNotify). Please add a focused test that simulates a burst of scroll events within a throttle window and asserts that the final scroll position is processed (e.g.,verticalScrollHandlercalled for the last event / startIndex returns to 0). This will prevent future regressions if throttle logic is refactored again.
this.scrollNotify.pipe(
filter(() => !this._init),
throttle(() =>
this.throttleTime$.pipe(
take(1),
switchMap(time => timer(time, this.throttleScheduler))
),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
projects/igniteui-angular/grids/grid/src/grid.component.spec.ts:2189
- Same as above: if the throttle duration reaches the 60ms cap, waiting only 50ms can intermittently assert before the trailing emission is delivered.
grid.scrollNotify.next({ target: { scrollTop: Math.round(maxScroll / 2) } });
grid.scrollNotify.next({ target: { scrollTop: 0 } });
await wait(50);
await fix.whenStable();
projects/igniteui-angular/grids/grid/src/grid-base.directive.ts:3706
- The PR description states the intent is to restore
throttle(..., { leading: false, trailing: true }), but the code sets{ leading: true, trailing: true }. This changes runtime behavior (potentially moreverticalScrollHandlerinvocations) beyond restoring the missing trailing edge. Either align the config with the described behavior or update the PR description/rationale to reflect the intentional leading-edge change and its performance impact.
// `trailing: true` ensures the final settle position of a fast momentum
// scroll is processed; otherwise the last scroll events are dropped and the
// rows stay frozen at an intermediate startIndex while the scrollbar is at top.
// `leading: true` keeps the immediate response on scroll start.
{ leading: true, trailing: true }
Drives the vertical scrollNotify stream with an intermediate (leading-edge) position followed by a scrollTop=0 settle in the same throttle window, so the settle can only be delivered on the trailing edge. Asserts the grid resets to startIndex 0 / renders row 0, guarding against the dropped-trailing throttle config that left the top rows frozen out of view.
ffed17e to
592caaa
Compare
Closes #17287
Description
Fixes a regression where the top rows of
igx-grid/igx-tree-grid/igx-hierarchical-gridcould become invisible after a fast momentum/inertiascroll back to the top. The scrollbar reports
scrollTop ≈ 0, but thevirtualized rows stay frozen at an intermediate
startIndex(~row 10–15).The vertical-scroll
throttleonscrollNotifywas passing no config, so itused RxJS's default
{ leading: true, trailing: false }. Restoring the explicit{ leading: true, trailing: true }config ensures the final settle position ofa scroll is always processed.
Motivation / Context
The throttle originally used
throttleTime(THROTTLE_TIME, animationFrameScheduler, { leading: false, trailing: true }).When it was changed to a data-size–dependent
throttle(durationSelector)(in"feat(grids): Apply different throttle based on the amount of data in display"),
the config object was dropped and the operator fell back to the RxJS default
{ leading: true, trailing: false }.Without the trailing edge, the last scroll events of a momentum flick — including
the
scrollTop ≈ 0settle at the top — fall inside a throttle window (up to 60 msfor large grids) and are discarded.
verticalScrollContainer.onScrollnever runsfor the final position, so virtualization stays at the last leading
startIndexwhile the DOM scrollbar settles at the top → the top rows appear missing.
trailing: truerestores the guarantee that the final resting scroll position isalways handled. Independent of zone/zoneless change detection.
Type of Change (check all that apply):
Component(s) / Area(s) Affected:
Grid, Tree Grid, Hierarchical Grid (shared
IgxGridBaseDirectiveverticalscroll handling / virtualization)
How Has This Been Tested?
Manual: Grid performance sample with 1000 rows — repeatedly wheel-scroll up/down
with momentum and confirm the grid always snaps back to row 0 at the top
(scrollbar at top ⇔ first row visible), where it previously froze around row
10–15.
Test Configuration:
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes