Skip to content

fix(ui): prevent ng-event listener accumulation on iframe reload #37113

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
gortiz-dotcms wants to merge 1 commit into
base: main
Choose a base branch
from
+24 −16
Open

fix(ui): prevent ng-event listener accumulation on iframe reload #37113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 12 additions & 5 deletions ...otcms-ui/src/app/view/components/_common/iframe/iframe-component/iframe.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ describe('IframeComponent', () => {
};
});

it('should remove and add listener on load', () => {
it('should remove and add listener on load using the same stable function references', () => {

@oidacra oidacra Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be worth adding a test that exercises the symptom directly? Right now the assertions stop at which references were handed to add/removeEventListener after a single load, so nothing covers the property that actually matters here: one dispatched ng-event producing exactly one emission after several reloads.

Something like triggering load two or three times, grabbing the handler from contentWindow.document.addEventListener.mock.calls, invoking it once, and asserting custom.emit fired once would pin the regression down. As it stands, a refactor could bring the duplicate back without turning the suite red.

iframeEl.triggerEventHandler('load', {
target: {
contentDocument: {
Expand All @@ -228,19 +228,26 @@ describe('IframeComponent', () => {
}
});

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const keyDownRef = (comp as any).boundEmitKeyDown;

@oidacra oidacra Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would deriving these from the mock call history work just as well here? Something like:

const [, keyDownRef] = comp.iframeElement.nativeElement.contentWindow.addEventListener.mock.calls[0];

That asserts the same invariant without depending on the private field names, so a later rename wouldn't break the test. It would also leave the door open to switching these to # private fields per TYPESCRIPT_STANDARDS, which the (comp as any) access would otherwise block.

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const customEventRef = (comp as any).boundEmitCustomEvent;

// remove must receive the exact same reference as add so the browser
// can actually deregister the handler instead of silently failing
expect(
comp.iframeElement.nativeElement.contentWindow.removeEventListener
).toHaveBeenCalledWith('keydown', expect.any(Function));
).toHaveBeenCalledWith('keydown', keyDownRef);
expect(
comp.iframeElement.nativeElement.contentWindow.document.removeEventListener
).toHaveBeenCalledWith('ng-event', expect.any(Function));
).toHaveBeenCalledWith('ng-event', customEventRef);

expect(
comp.iframeElement.nativeElement.contentWindow.addEventListener
).toHaveBeenCalledWith('keydown', expect.any(Function));
).toHaveBeenCalledWith('keydown', keyDownRef);
expect(
comp.iframeElement.nativeElement.contentWindow.document.addEventListener
).toHaveBeenCalledWith('ng-event', expect.any(Function));
).toHaveBeenCalledWith('ng-event', customEventRef);
});

it('should set the colors to the jsp on load', () => {
Expand Down
23 changes: 12 additions & 11 deletions ...pps/dotcms-ui/src/app/view/components/_common/iframe/iframe-component/iframe.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@ export class IframeComponent implements OnInit, OnDestroy {

private destroy$: Subject<boolean> = new Subject<boolean>();

// Stable bound references required so removeEventListener can match the
// exact function object that was passed to addEventListener. Using
// .bind(this) inline creates a new object each call, making removal a
// no-op and causing listeners to accumulate on every iframe load event.

@oidacra oidacra Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small wording question on this comment. The first half is exactly right — .bind(this) returns a new object each call, so removeEventListener never matched. But the last clause, "causing listeners to accumulate on every iframe load event," reads as if it applies to both fields below it.

As far as I can tell it only holds for keydown, which is attached to getIframeWindow() (the WindowProxy keeps its identity across navigations). ng-event is attached to getIframeWindow().document, and each navigation brings a fresh Document, so the old listener goes away with the old document rather than piling up.

Would it be worth splitting that out so the next person reading this doesn't assume both listeners leak the same way?

private readonly boundEmitKeyDown = this.emitKeyDown.bind(this);
private readonly boundEmitCustomEvent = this.emitCustonEvent.bind(this);

ngOnInit(): void {
this.iframeOverlayService.overlay
.pipe(takeUntil(this.destroy$))
Expand Down Expand Up @@ -264,17 +271,11 @@ export class IframeComponent implements OnInit, OnDestroy {
}

private handleIframeEvents($event): void {
this.getIframeWindow().removeEventListener('keydown', this.emitKeyDown.bind(this));
this.getIframeWindow().document.removeEventListener(
'ng-event',
this.emitCustonEvent.bind(this)
);

this.getIframeWindow().addEventListener('keydown', this.emitKeyDown.bind(this));
this.getIframeWindow().document.addEventListener(
'ng-event',
this.emitCustonEvent.bind(this)
);
this.getIframeWindow().removeEventListener('keydown', this.boundEmitKeyDown);
this.getIframeWindow().document.removeEventListener('ng-event', this.boundEmitCustomEvent);

this.getIframeWindow().addEventListener('keydown', this.boundEmitKeyDown);
this.getIframeWindow().document.addEventListener('ng-event', this.boundEmitCustomEvent);

@oidacra oidacra Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I traced the save path for this one and wanted to check the root cause with you. keydown is registered on getIframeWindow() (the WindowProxy, which persists across navigations, so it can genuinely accumulate), but ng-event is registered on getIframeWindow().document — and a navigation always creates a fresh Document. iframe-porlet-legacy.component.ts:182-190 goes further and destroys/recreates the whole <iframe> via the isLoading gate whenever the URL changes, so the document listener starts from zero each load.

Since the duplicate save runs through ng-event (RemotePublisherDialog.js:271-276 -> this listener -> DotCustomEventHandlerService -> openWizard), and DotWizardService.open() completes any pending subject without emitting before creating a new one, would two stacked listeners actually produce two saveContent calls here?

Have you been able to confirm in the Network tab that the two ContentletAjax.saveContent.dwr calls collapse to one? That's the unchecked item in your own test plan, and it would settle it. If it turns out the duplicate persists, would it be worth dropping the Fixes #34534 link so the issue isn't auto-closed?

this.charge.emit($event);

const doc = this.getIframeDocument();
Expand Down
Loading