Skip to content

fix(igxGrid): Fix merge cell offset when using virtual scrollbar with ratio.#16331

Merged
dkamburov merged 7 commits into20.1.xfrom
mkirova/fix-16292-20.1.x
Apr 6, 2026
Merged

fix(igxGrid): Fix merge cell offset when using virtual scrollbar with ratio.#16331
dkamburov merged 7 commits into20.1.xfrom
mkirova/fix-16292-20.1.x

Conversation

@MayaKirova
Copy link
Copy Markdown
Contributor

@MayaKirova MayaKirova commented Oct 20, 2025

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):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@github-actions
Copy link
Copy Markdown

There has been no recent activity and this PR has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label Jan 26, 2026
@dkamburov dkamburov removed the status: inactive Used to stale issues and pull requests label Jan 26, 2026
@github-actions
Copy link
Copy Markdown

There has been no recent activity and this PR has been marked inactive.

@github-actions github-actions bot added the status: inactive Used to stale issues and pull requests label Mar 28, 2026
@dkamburov dkamburov requested a review from mddragnev April 2, 2026 12:21
mddragnev
mddragnev previously approved these changes Apr 2, 2026
@dkamburov dkamburov removed the status: inactive Used to stale issues and pull requests label Apr 2, 2026
@IMinchev64 IMinchev64 added 💥 status: in-test PRs currently being tested ✅ status: verified Applies to PRs that have passed manual verification and removed ❌ status: awaiting-test PRs awaiting manual verification 💥 status: in-test PRs currently being tested labels Apr 3, 2026
Copilot AI review requested due to automatic review settings April 6, 2026 11:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 index into 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;
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
const index = x.index;
const index = x.index ?? collection.indexOf(x);
if (index < 0 || !x.cellMergeMeta) {
return;
}

Copilot uses AI. Check for mistakes.
this.columnsToMerge.forEach((col) => {
const root = rec.cellMergeMeta?.get(col.field)?.root;
if (root) {
data.push({ record: root, index: root.index, dataIndex: root.index });
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
data.push({ record: root, index: root.index, dataIndex: root.index });
data.push({
record: root,
index: this.getDataViewIndex(root.index, false),
dataIndex: root.index
});

Copilot uses AI. Check for mistakes.
Comment on lines +8255 to +8262
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 });
}
})
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 });
}
});

Copilot uses AI. Check for mistakes.
Comment on lines +274 to 279
/**
* @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 */
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
@dkamburov dkamburov requested a review from mddragnev April 6, 2026 14:27
@dkamburov dkamburov merged commit 325b5a3 into 20.1.x Apr 6, 2026
9 checks passed
@dkamburov dkamburov deleted the mkirova/fix-16292-20.1.x branch April 6, 2026 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

grid: cell-merging version: 20.1.x ✅ status: verified Applies to PRs that have passed manual verification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants