Skip to content

fix(grid): avoid double overlay detach on destroy with adv. filtering open - #17963

Open
ivanvpetrov wants to merge 2 commits into
masterfrom
ipetrov/fix-17962
Open

ivanvpetrov wants to merge 2 commits into
masterfrom
ipetrov/fix-17962

Conversation

@ivanvpetrov

Copy link
Copy Markdown
Contributor

Closes #17962

Description

Destroying a grid while its advanced filtering dialog is open no longer logs igxOverlay.detach was called with wrong id in the console.

  • In ngOnDestroy, the grid now detaches the advanced filtering dialog only if the overlay is still registered (getOverlayById). The grid already guards its other overlays the same way a few lines below.
  • ngOnDestroy now always resets _advancedFilteringOverlayId.

Motivation / Context

This is the same issue as #17938 (date pickers, fixed in #17958). The advanced filtering dialog is created in the grid's own ViewContainerRef. When the grid is destroyed, Angular destroys that container, and the dialog with it, before it runs the grid's ngOnDestroy. The overlay service registers a destroy callback on the components it attaches, so it detaches the dialog at that point. It doesn't emit closed, so the grid still holds the old overlay id. The grid's ngOnDestroy then calls detach with an id that no longer exists, and the service logs the warning.

The overlay service's own detach-on-destroy behaviour is correct and stays as is.

Type of Change (check all that apply):

  • Bug fix
  • New functionality
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (no functional changes)
  • Documentation
  • Demos
  • CI/CD
  • Tests
  • Changelog
  • Skills/Agents

Component(s) / Area(s) Affected:

Grid, Tree Grid, Hierarchical Grid (advanced filtering dialog)

How Has This Been Tested?

A new spec in grid-filtering-advanced.spec.ts follows the steps from the issue. It renders the grid inside an @if, opens the advanced filtering dialog, removes the grid, and checks that the dialog is gone and that the detach warning wasn't logged. It fails without the fix (igxOverlay.detach was called with wrong id: 0) and passes with it.

  • Unit tests
  • Manual testing
  • Automated e2e tests

Runs:

  • grid-filtering-advanced, grid-toolbar, grid-keyBoardNav-headers, hierarchical-grid.selection and state.* specs: 234 passed
  • npm run lint:lib: no new errors or warnings

Test Configuration:

  • Angular version: 22.2.3
  • Browser(s): Chromium (Vitest browser mode via Playwright)
  • OS: Windows 11

Screenshots / Recordings

N/A: the only visible effect was a console warning.

Checklist:

  • All relevant labels 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
  • Accessibility (ARIA, keyboard navigation, focus management) has been verified

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The review reports no active issues with the advanced-filtering dialog teardown fix.

No open issues 🎉

What changed in this PR

Fixes #17962 by preventing duplicate overlay cleanup when a grid is destroyed with its advanced filtering dialog open. The change applies to the shared base used by Grid, Tree Grid, and Hierarchical Grid.

Changes:

  • Detaches the dialog during grid teardown only if its overlay remains registered, and always clears the stored overlay ID.
  • Adds a regression test that removes an @if-controlled grid and checks dialog removal and absence of the warning.
  • Documents the fix in the changelog.
File Description
projects/​igniteui-angular/​grids/​grid/​src/​grid-filtering-advanced.spec.ts Adds conditional grid teardown coverage for the overlay warning.
projects/​igniteui-angular/​grids/​grid/​src/​grid-base.directive.ts Guards overlay detachment and unconditionally clears the stored ID.
CHANGELOG.md Records the advanced filtering teardown fix.

Review effort: Balanced

@ivanvpetrov
ivanvpetrov marked this pull request as ready for review November 12, 2026 08:09
@viktorkombov viktorkombov added the ✅ status: verified Applies to PRs that have passed manual verification label Nov 12, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Grid] "igxOverlay.detach was called with wrong id" when a grid is destroyed with its advanced filtering dialog open

3 participants