fix(igxGrid): Fix merge cell offset when using virtual scrollbar with ratio.#16331
fix(igxGrid): Fix merge cell offset when using virtual scrollbar with ratio.#16331
Conversation
|
There has been no recent activity and this PR has been marked inactive. |
|
There has been no recent activity and this PR has been marked inactive. |
There was a problem hiding this comment.
Pull request overview
Fixes igxGrid merged-cell overlay positioning when the virtual scrollbar is scaled using a ratio (due to exceeding the browser’s max scroll size), addressing layout breaks during navigation/search over large datasets.
Changes:
- Use virtualized scroll position (
_virtScrollPosition) when computing merged-cell overlay offsets. - Rework merged-top overlay row detection to use merge roots from the current start row.
- Track and reuse merged-record indices via merge metadata (adds
indexinto merge results).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| projects/igniteui-angular/src/lib/grids/grid/grid.pipes.ts | Changes unmerge logic to use merge-metadata index instead of indexOf. |
| projects/igniteui-angular/src/lib/grids/grid-base.directive.ts | Adjusts merged-cell offset calculation and rewrites merged overlay row detection. |
| projects/igniteui-angular/src/lib/directives/for-of/for_of.directive.ts | Exposes _virtScrollPosition and fixes scrollPosition scaling when using a virtual ratio; updates scroll offset computation. |
| projects/igniteui-angular/src/lib/data-operations/merge-strategy.ts | Adds index to default merge result objects to support faster/consistent lookups. |
Comments suppressed due to low confidence (1)
projects/igniteui-angular/src/lib/grids/grid-base.directive.ts:3726
- This change switches merged-cell positioning logic to use
_virtScrollPosition(virtualized scroll position) and changes how merged-top overlay rows are computed. There are existing cell merging specs, but nothing that covers the scrollbar ratio/virtual-size scenario that triggered issues #16292/#16408. Adding a unit test that forces_virtRatio !== 1(via large total size) and verifies merged overlay row top offset stays aligned during navigation/scroll would help prevent regressions.
protected getMergeCellOffset(rowData) {
const index = rowData.dataIndex;
let offset = this.verticalScrollContainer._virtScrollPosition - this.verticalScrollContainer.getScrollForIndex(index);
if (this.hasPinnedRecords && this.isRowPinningToTop) {
offset -= this.pinnedRowHeight;
}
return -offset;
| let result = cloneArray(collection) as any; | ||
| uniqueRoots.forEach(x => { | ||
| const index = collection.indexOf(x); | ||
| const index = x.index; |
There was a problem hiding this comment.
IgxGridUnmergeActivePipe now relies on x.index being present on merged-record metadata. IGridMergeStrategy.merge() does not document/guarantee an index property on its output objects, so a custom merge strategy that returns cellMergeMeta without index will make index undefined here and can break unmerge (e.g., activeRowIndexes.map(ri => ri - index) becomes NaN). Consider keeping backwards compatibility by falling back to collection.indexOf(x) when x.index is null/undefined, or formalize index as part of the merge-result contract and enforce/populate it for all strategies.
| const index = x.index; | |
| const index = x.index ?? collection.indexOf(x); | |
| if (index < 0 || !x.cellMergeMeta) { | |
| return; | |
| } |
| this.columnsToMerge.forEach((col) => { | ||
| const root = rec.cellMergeMeta?.get(col.field)?.root; | ||
| if (root) { | ||
| data.push({ record: root, index: root.index, dataIndex: root.index }); |
There was a problem hiding this comment.
updateMergedData() now sets rowData.index to root.index, but root.index is the unpinned data index. When row pinning is to top, row components expect a view index that includes the pinned area offset (see getDataViewIndex()), otherwise the overlay row for merged cells can be rendered/treated as the wrong row index. Use a view index here (e.g., this.getDataViewIndex(root.index, /*pinned*/ false) or apply + this.pinnedRecordsCount when isRowPinningToTop).
| data.push({ record: root, index: root.index, dataIndex: root.index }); | |
| data.push({ | |
| record: root, | |
| index: this.getDataViewIndex(root.index, false), | |
| dataIndex: root.index | |
| }); |
| const rec = this.verticalScrollContainer.igxForOf[startIndex]; | ||
| if (rec && rec.cellMergeMeta) { | ||
| this.columnsToMerge.forEach((col) => { | ||
| const root = rec.cellMergeMeta?.get(col.field)?.root; | ||
| if (root) { | ||
| data.push({ record: root, index: root.index, dataIndex: root.index }); | ||
| } | ||
| }) |
There was a problem hiding this comment.
updateMergedData() can push the same root multiple times (once per merged column) when different merged columns share the same merge root. The templates use @for (rowData of mergedDataInView; track rowData.record;), so duplicates can lead to non-unique track keys and incorrect rendering/diffing of the merged-top overlay rows. Consider de-duplicating roots (e.g., by root reference or root.index) before assigning to _mergedDataInView.
| const rec = this.verticalScrollContainer.igxForOf[startIndex]; | |
| if (rec && rec.cellMergeMeta) { | |
| this.columnsToMerge.forEach((col) => { | |
| const root = rec.cellMergeMeta?.get(col.field)?.root; | |
| if (root) { | |
| data.push({ record: root, index: root.index, dataIndex: root.index }); | |
| } | |
| }) | |
| const addedRoots = new Set<any>(); | |
| const rec = this.verticalScrollContainer.igxForOf[startIndex]; | |
| if (rec && rec.cellMergeMeta) { | |
| this.columnsToMerge.forEach((col) => { | |
| const root = rec.cellMergeMeta?.get(col.field)?.root; | |
| if (root && !addedRoots.has(root)) { | |
| addedRoots.add(root); | |
| data.push({ record: root, index: root.index, dataIndex: root.index }); | |
| } | |
| }); |
| /** | ||
| * @hidden | ||
| */ | ||
| /** Internal track for scroll top that is being virtualized */ | ||
| protected _virtScrollPosition = 0; | ||
| public _virtScrollPosition = 0; | ||
| /** If the next onScroll event is triggered due to internal setting of scrollTop */ |
There was a problem hiding this comment.
Making _virtScrollPosition public exposes an underscored internal field as part of the generated typings/API surface. To avoid effectively “publishing” an internal implementation detail, consider keeping the field non-public and exposing a @hidden/@internal getter (e.g., get virtScrollPosition()), which also allows you to keep invariants (like ensuring it stays in sync when resetScrollPosition() is called).
Closes #16292
Closes #16408
In case scrollbar size exceeds max browser allowed and uses ratio to calc. positions, adjust offset of merged cells accordingly.
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)