refactor(ESF, combo): use IgxVirtualScrollComponent instead of igxFor - #17579
Open
viktorkombov wants to merge 8 commits into
Open
refactor(ESF, combo): use IgxVirtualScrollComponent instead of igxFor#17579viktorkombov wants to merge 8 commits into
viktorkombov wants to merge 8 commits into
Conversation
viktorkombov
requested review from
ChronosSF and
rkaraivanov
and
a balanced review from Copilot
September 8, 2026 09:24
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The critical test failures and moderate accessibility and navigation issues must be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Migrates Excel-style filtering from igxFor to IgxVirtualScrollComponent; the titled Combo migration is absent.
Changes:
- Replaces ESF virtualization and keyboard-navigation logic.
- Updates virtual-scroll markup and documentation.
File summaries
| File | Summary | Findings |
|---|---|---|
projects/igniteui-angular/virtual-scroll/README.md |
Documents popup viewport initialization. | Documents an unavailable input and behavior inconsistent with the implementation. (nit, 2 votes) |
projects/igniteui-angular/grids/core/src/filtering/excel-style/excel-style-search.component.ts |
Migrates ESF scrolling and focus logic. | Existing tests reference removed APIs. (critical, 2 votes) Off-window active descendants can reference nonexistent elements. (moderate, 2 votes) Estimated item size can select the wrong row. (moderate, 1 vote) Combo migration is absent. (nit, 2 votes) Navigation regression coverage is missing. (nit, 1 vote) |
projects/igniteui-angular/grids/core/src/filtering/excel-style/excel-style-search.component.html |
Renders ESF options through virtual scroll. | Virtualized options lack logical position and set size. (moderate, 2 votes) The virtual-scroll list role disrupts listbox option ownership. (moderate, 2 votes) |
Review details
Suppressed comments (2)
projects/igniteui-angular/grids/core/src/filtering/excel-style/excel-style-search.component.html:43
- The Indigo ESF theme still applies its horizontal padding only to
igx-display-container(themes/shared/_indigo.scss:122), but that element disappears with this replacement. The Indigo list rows will lose their established inset; retarget the style to the new virtual-scroll content/host as part of this migration.
<igx-virtual-scroll
#virtualScroll
[data]="displayedListData"
[estimatedItemSize]="itemSize"
(stateChange)="onVirtualStateChange($event)"
projects/igniteui-angular/grids/core/src/filtering/excel-style/excel-style-search.component.ts:373
- The migration rewrites keyboard scrolling and active-descendant behavior, but the ESF grid specs have no coverage for listbox Arrow/Home/End navigation or off-window targets. Add Jasmine coverage that navigates beyond the rendered range and verifies scrolling, the active row, and a valid
aria-activedescendant; this is where regressions in the new asynchronous path will surface.
protected onVirtualStateChange(state: VirtualScrollState): void {
this._renderedRange = state;
this.refreshActiveDescendant();
- Files reviewed: 3/3 changed files
- Comments generated: 7
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Zero-sized viewports can incorrectly retain stale measurements; the stated consumer migrations and additional API coverage are also missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
projects/igniteui-angular/virtual-scroll/README.md:46
- The claim that this input is never read again after a measurement is not true when
orientationchanges: the orientation effect resets_viewportSizeto zero atvirtual-scroll.component.ts:415-417, so a hidden host falls back to this input again. Either preserve a separate “already measured” state or document that the hint can become active again until the new axis is measured.
| `initialViewportSize` | `number` | `0` | Viewport size in pixels to render the **first** window against, for a list that cannot be measured when it is first rendered. A hint for that render only: once the host measures above zero, the measured size takes over and this input is not read again. Negative, `NaN` and infinite values count as no hint. See [Lists inside a popup](#lists-inside-a-popup). |
projects/igniteui-angular/virtual-scroll/src/virtual-scroll/virtual-scroll.component.ts:562
- The hinted viewport is now used by
scrollToIndex, but the added tests only cover rendered ranges and resizing; none callsscrollToIndexbefore the host has a real measurement. Please add a hidden/unmeasured-host test that verifies alignment (includingnearest) usesinitialViewportSize, since this is newly changed public behavior.
if (
requested === "nearest" &&
this._engine.isIndexInView(index, current, this._effectiveViewportSize())
projects/igniteui-angular/virtual-scroll/src/virtual-scroll/virtual-scroll.component.ts:219
- This new public input is user-visible API, but the current 22.2.0 changelog has no Virtual Scroll entry. Add it under New Features so consumers can discover the new popup-rendering contract.
/**
* Viewport size in pixels to render the first window against, for a list that is hidden
* until the change detection pass that reveals it and so has no size to measure in it.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
A list inside a popup is hidden until the change detection pass that reveals it, so it has no size to measure and renders nothing in that pass. initialViewportSize gives that first render a size to work from; the measured size takes over as soon as the host can be measured.
viktorkombov
force-pushed
the
vkombov/use-virtual-scroll-for-combo-esf
branch
from
September 8, 2026 12:58
733528c to
3220b2d
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Paged mode has unbounded O(totalCount) memory usage, and the scope mismatch, ineffective assertions, and viewport-hint lifecycle issue remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
projects/igniteui-angular/virtual-scroll/src/virtual-scroll/virtual-scroll.component.spec.ts:1163
- These bounds do not verify the documented normalization: an implementation that always used any finite value above 20 would pass every case. Add expected totals to the cases and assert the exact track height (20 items for NaN/infinite/negative and 400 items for 400.7).
const height = Number.parseFloat(vsTrack(windowFixture).style.height);
expect(Number.isFinite(height)).toBeTrue();
expect(height).toBeGreaterThanOrEqual(20 * 50);
projects/igniteui-angular/virtual-scroll/src/virtual-scroll/virtual-scroll.component.ts:248
- The stated ESF/combo refactor is not present in this change: both consumers still import and render
IgxForOfDirective/*igxFor, while this line only adds a new virtual-scroll API. Please migrate those consumers and their tests, or retitle/rescope the pull request so its claimed purpose matches the implementation.
public readonly dataWindow = input<VirtualDataWindow<T> | null>(null);
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
| this._previousItems = loaded; | ||
| this._engine.resize( | ||
| items.length, | ||
| loaded.totalCount, |
Comment on lines
+9
to
+11
| - `IgxVirtualScrollComponent` | ||
| - Added `initialViewportSize`, the viewport size to render the first window against. A list that is hidden until the change detection pass that reveals it has no size to measure in that pass and would render nothing; this gives that first render a size to work from, and the host's own size takes over once it has been laid out. | ||
| - Added `dataWindow`, taking a loaded page of a larger collection as `{ items, startIndex, totalCount }`. The list is as long as `totalCount`, so the scrollbar spans the whole collection while only the page is in memory, and indices the page does not cover render nothing until a page that covers them arrives. `data` is unchanged and is used whenever `dataWindow` is not set. |
Comment on lines
+485
to
487
| // The size of the previous axis says nothing about the new one. | ||
| this._viewportSize.set(null); | ||
| this._measureViewport(); |
The search list is virtualized by IgxVirtualScrollComponent instead of the igxFor directive. Rows are measured in the DOM rather than assumed from a configured item and container size, and initialViewportSize gives the list the height the menu allots it so the first window renders in the pass that opens it. aria-activedescendant now names only a rendered element: navigateItem clears it while the target is off screen and restores it once the scroll has rendered that row, and a scroll that recycles the wrappers no longer leaves it pointing at an element that is gone.
Migrates IgxComboComponent and IgxSimpleComboComponent from the igxFor directive to IgxVirtualScrollComponent, following the Excel style filtering search list. IgxDropDownComponent now also accepts a content-projected igx-virtual-scroll; *igxFor keeps working as documented, and selection and navigation behave the same either way. A row is measured in the DOM once it renders and the measured size replaces the estimate it started from, so the scrollable extent follows the real height of the rows that have rendered.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical asynchronous paging/render-state defects and unresolved navigation and ARIA issues block approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
projects/igniteui-angular/combo/src/combo/combo.pipes.ts:14
- Angular 22 treats pipes as standalone by default, and the project guidance explicitly avoids setting
standalone: trueon new declarations. Remove this redundant flag to keep the new pipe aligned with the current framework convention.
standalone: true
projects/igniteui-angular/drop-down/README.md:87
- The example nests the virtualizer's default
role="list"inside the drop-down'slistbox, but its children arerole="option", notlistitem. Applications following this example inherit an invalid ARIA structure. Showrole="presentation"on the virtual-scroll host (and apply the same requirement to projected virtual-scroll guidance).
<igx-virtual-scroll [data]="localItems" [estimatedItemSize]="28" [initialViewportSize]="200"
style="display: block; height: 200px">
projects/igniteui-angular/grids/core/src/filtering/excel-style/excel-style-search.component.html:39
igx-virtual-scrollsuppliesrole="list"by default, but here it is nested inside alistboxand directly containsrole="option"rows. That creates an invalid ARIA hierarchy because alistrequireslistitemdescendants. Override the virtualizer to a presentational role so the options remain semantically owned by the outer listbox.
<igx-virtual-scroll
- Files reviewed: 27/27 changed files
- Comments generated: 6
- Review effort level: Balanced
Comment on lines
+1105
to
+1106
| this._virtualizationState = { startIndex: state.startIndex, chunkSize }; | ||
| this.dataPreLoad.emit({ ...this._virtualizationState, owner: this }); |
Comment on lines
+118
to
+120
| public isIndexRendered(index: number): boolean { | ||
| return index >= this._rendered.startIndex && index <= this._rendered.endIndex; | ||
| } |
Comment on lines
+81
to
+85
| <igx-virtual-scroll #virtualScroll | ||
| [dataWindow]="itemWindow" | ||
| [estimatedItemSize]="estimatedItemSize" | ||
| [initialViewportSize]="viewportSize" | ||
| (stateChange)="handleVirtualStateChange($event)"> |
Comment on lines
+83
to
+85
| public get length(): number { | ||
| const window = this._scroll.dataWindow(); | ||
| return window ? window.totalCount : (this._scroll.data() ?? []).length; |
Comment on lines
+329
to
333
| if (this.virtualization) { | ||
| newSelection = { | ||
| value: this.virtDir.igxForOf![index], | ||
| value: this.virtualization.itemAt(index), | ||
| index | ||
| } as IgxDropDownItemBaseDirective; |
Comment on lines
+77
to
+81
| <igx-virtual-scroll #virtualScroll | ||
| [dataWindow]="itemWindow" | ||
| [estimatedItemSize]="estimatedItemSize" | ||
| [initialViewportSize]="viewportSize" | ||
| (stateChange)="handleVirtualStateChange($event)"> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #
Description
Motivation / Context
Type of Change (check all that apply):
Component(s) / Area(s) Affected:
How Has This Been Tested?
Test Configuration:
Screenshots / Recordings
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)