Skip to content

fix(action-strip): Assign parent to not destroy on DOM move. #16270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: 20.1.x
Choose a base branch
from
Open

fix(action-strip): Assign parent to not destroy on DOM move. #16270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5ac7a0d
fix(action-strip): Assign parent to not destroy on DOM move.
Jun 16, 2025
01683e7
chore(*): Move strip handling after parentComponentRef is populated.
Jun 16, 2025
fbd1785
fix(action-strip): Fix timing issues.
Jun 20, 2025
a8642fd
fix(igxGridRow): On destroy check for still shown action strip.
Oct 9, 2025
ec11d6d
chore(action-strip,elements): minor new lines formatting
damyanpetev Oct 10, 2025
7201841
fix(igxGrid): On detach also check for still shown action strip.
Oct 13, 2025
89b1eb8
Merge branch 'mkirova/fix-15768-20.1.x' of https://github.com/IgniteU…
Oct 13, 2025
f18b2f4
chore(*): Add null check.
Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions projects/igniteui-angular/src/lib/grids/grid-base.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6438,6 +6438,12 @@ export abstract class IgxGridBaseDirective implements GridType,
}
}

protected viewDetachHandler(args) {
if (this.actionStrip && args.view.rootNodes.find(x => x === this.actionStrip.context?.element.nativeElement)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd flip that to:

Suggested change
if (this.actionStrip && args.view.rootNodes.find(x => x === this.actionStrip.context?.element.nativeElement)) {
if (this.actionStrip?.context?.element.nativeElement && args.view.rootNodes.includes(this.actionStrip.context.element.nativeElement)) {

just so the check doesn't run if there's no context on the strip set at all.

In any case, this is getting to be a bit of a rabbit hole and I got GroupBy and possibly paging as extra cases. So far I've been able to find the grouping scenario with deleting the last row of a group that does the trick and once again there's a rogue show() in there (two actually) before the remove:
image
The paging one haven't caught yet.

To that end:

  • Do add some explanation as to what's not working and what/why that code is doing there, beforeViewDetach is a very chatty event to just take in general and connect to the specific case.
  • This is becoming more and more involved of a fix and at this stage I don't think we can do without test coverage - this should be automatable both in Angular and Elements-specific tests and we should have the scenarios and checks. If we do allow the fix in without tests, those should be the next thing to add immediately after.
  • And lastly, as the screenshot above shows and our previous debug logs from the initial case - there's always a show after the action strip has already once been hidden correctly. Both of those fixes are merely covering up the fact that the show is likely not managed correctly and the strip can't properly close itself as it deletes rows and we might need to address the actual cause instead.

Copy link
Contributor Author

@MayaKirova MayaKirova Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@damyanpetev

  1. The specific scenario shown to me by @MarielaTihova was a grid with paging and groupby (the one in the elements grid sample) where if you keep deleting the last data row until it's replaced by group by row, the action strip gets destroyed.

    In that specific scenario the row is not destroyed but instead cached (see the igxTemplateOutlet), which means it gets detached and it detaches together with the action strip inside. And since it's removed from the DOM, elements destroys it. We already have beforeViewDetach event, which fires before the row detaches, so I just added the same logic there as for the row's destroy handler.

    Hope it makes more sense now.

  2. Sure I can work on automation either as part of this PR or a separate one, depending on whether @dkamburov wants the fix to be included quicker with the next patch or if it can wait.

  3. I'm open to your suggestions on a better fix if you have one.

Copy link
Member

@damyanpetev damyanpetev Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I got the grouping case right, but okay.
As for the alternative suggestion, it's been the same as the last two times - try to avoid the extra show() when the strip closed itself. That seems to revolve around the mouseenter handeler (should it be pointer instead?), so I tried a check for the related target, excluding children of the row (possibly redundant) and children of the action strip itself:
image

@HostListener('mouseenter', ['$event'])
public showActionStrip(e: MouseEvent) {
    const from = e.relatedTarget as HTMLElement | null;
    if (this.grid.actionStrip && !(!from || this.nativeElement.contains(from) || this.grid.actionStrip.el.nativeElement.contains(from))) {
        this.grid.actionStrip.show(this);
    }
    this.grid.hoverIndex = this.index;
}

That handles the initial bug just fine from what I saw (delete the single root of the Tree Grid in Elements demo) and the Grouping scenario (delete the last row of a group). Still something iffy on open from when another row takes the place under the cursor after delete and I have to move to show the strip so the check might need smth extra, but it doesn't break in any of the cases at least.

Copy link
Member

Choose a reason for hiding this comment

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

Could also track enter/leave as state, don't believe there's a leave in-between those multiple enters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@damyanpetev Since you already have the changes locally, might as well commit and make a PR, if that's ok with you. I'll close mine as redundant.

Copy link
Member

@damyanpetev damyanpetev Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Eh, what I posted on top was what I have. However, as I said it doesn't quite work since there's a secondary show() on the "deleted" row that's ignored, but that row is 'replaced' by the next in context and that's how it works in main right now - the strip self-closes and the row opens it back up immediately, the delete overrides the context and the strip somehow remains open on the same DOM row and works fine. So attempting to avoid the open.. is odd. We might need to discuss further.
TLDR: The behavior I'm trying to avoid is what we apparently rely on to handle normal delete scenarios, which is a bit baffling.

this.actionStrip.hide();
}
}

/**
* @hidden @internal
*/
Expand Down
1 change: 1 addition & 0 deletions projects/igniteui-angular/src/lib/grids/grid/grid.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
[igxTemplateOutletContext]="getContext(rowData, rowIndex)"
(cachedViewLoaded)="cachedViewLoaded($event)"
(viewCreated)="viewCreatedHandler($event)"
(beforeViewDetach)="viewDetachHandler($event)"
(viewMoved)="viewMovedHandler($event)">
</ng-template>
</ng-template>
Expand Down
2 changes: 1 addition & 1 deletion projects/igniteui-angular/src/lib/grids/hierarchical-grid/hierarchical-grid.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
<ng-template
[igxTemplateOutlet]="(isHierarchicalRecord(rowData) ? hierarchical_record_template : (isChildGridRecord(rowData) ? child_record_template : hierarchical_record_template))"
[igxTemplateOutletContext]="getContext(rowData, rowIndex, false)" (viewCreated)="viewCreatedHandler($event)"
(viewMoved)="viewMovedHandler($event)" (cachedViewLoaded)="cachedViewLoaded($event)">
(viewMoved)="viewMovedHandler($event)" (cachedViewLoaded)="cachedViewLoaded($event)" (beforeViewDetach)="viewDetachHandler($event)">
</ng-template>
<!-- <ng-container *igxTemplateOutlet="(isHierarchicalRecord(rowData) ? hierarchical_record_template : (isChildGridRecord(rowData) && isExpanded(rowData) ? child_record_template : hierarchical_record_template)); context: getContext(rowData)"></ng-container> -->
</ng-template>
Expand Down
4 changes: 4 additions & 0 deletions projects/igniteui-angular/src/lib/grids/row.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,10 @@ export class IgxRowDirective implements DoCheck, AfterViewInit, OnDestroy {
* @internal
*/
public ngOnDestroy() {
// if action strip is shown here but row is about to be destroyed, hide it.
if (this.grid.actionStrip && this.grid.actionStrip.context === this) {
this.grid.actionStrip.hide();
}
this.destroy$.next(true);
this.destroy$.complete();
}
Expand Down
1 change: 1 addition & 0 deletions projects/igniteui-angular/src/lib/grids/tree-grid/tree-grid.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
(dataChanging)="dataRebinding($event)" (dataChanged)="dataRebound($event)">
<ng-template [igxTemplateOutlet]='isSummaryRow(rowData) ? summary_template : record_template'
[igxTemplateOutletContext]='getContext(rowData, rowIndex, false)'
(beforeViewDetach)="viewDetachHandler($event)"
(cachedViewLoaded)='cachedViewLoaded($event)'>
</ng-template>
</ng-template>
Expand Down
Loading