Skip to content

refactor(helper): inicializa listener de focus após abertura do popover #2693

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
luiz-s-vasconcellos wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor(helper): inicializa listener de focus após abertura do popover #2693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72c3a00
refactor(helper): inicializa listener de focus após abertura do popover
luiz-s-vasconcellos Jan 7, 2026
6731fdf
feat(popover): cria trigger 'function' para o popover
luiz-s-vasconcellos Jan 7, 2026
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
7 changes: 4 additions & 3 deletions projects/ui/src/lib/components/po-helper/po-helper.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
<div
(mouseenter)="setPopoverPositionByScreen()"
class="po-helper-container po-helper-target"
[attr.p-size]="this.size()"
#target
Expand All @@ -12,17 +11,19 @@
[attr.aria-describedby]="!popover.isHidden ? 'popover-content-' + id : null"
[class.po-helper-disabled]="disabled()"
(keydown)="onKeyDown($event)"
(click)="emitClick($event); popover.ensurePopoverPosition()"
(click)="openHelperPopover(); emitClick($event); popover.ensurePopoverPosition()"
>
<po-icon [p-icon]="helper()?.type === 'info' ? 'ICON_INFO' : 'ICON_HELP'"></po-icon>
<po-popover
(click)="$event.stopPropagation()"
#popover
[p-position]="popoverPosition"
[p-target]="target"
[p-title]="helper()?.title"
[p-append-in-body]="appendBox()"
p-custom-classes="po-helper-popover"
p-trigger="function"
(p-close)="handleClose()"
(p-open)="handleOpen()"
>
<div [id]="'popover-content-' + id" role="dialog" aria-modal="false" tabindex="-1">
{{ helper()?.content }}
Expand Down
16 changes: 11 additions & 5 deletions projects/ui/src/lib/components/po-helper/po-helper.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ export class PoHelperComponent extends PoHelperBaseComponent implements AfterVie
}
ngAfterViewInit(): void {
PoHelperComponent.instances.push(this);
this.boundFocusIn = this.closePopoverOnFocusOut.bind(this);
window.addEventListener('focusin', this.boundFocusIn, true);
queueMicrotask(() => {
this.setPopoverPositionByScreen();
});
Expand All @@ -99,9 +97,6 @@ export class PoHelperComponent extends PoHelperBaseComponent implements AfterVie

ngOnDestroy(): void {
PoHelperComponent.instances = PoHelperComponent.instances.filter(i => i !== this);
if (this.boundFocusIn) {
window.removeEventListener('focusin', this.boundFocusIn, true);
}
}

openHelperPopover(): void {
Expand Down Expand Up @@ -214,4 +209,15 @@ export class PoHelperComponent extends PoHelperBaseComponent implements AfterVie
const literals = this.poHelperLiterals[lang] || this.poHelperLiterals['en'];
return literals[type];
}

protected handleOpen(): void {
this.boundFocusIn = this.closePopoverOnFocusOut.bind(this);
window.addEventListener('focusin', this.boundFocusIn, true);
}
Comment on lines +213 to +216
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The boundFocusIn property is being reassigned on every popover open without clearing the previous value. If the popover is opened, closed, and reopened multiple times, the boundFocusIn reference will be overwritten, but the old event listener reference will be lost. When handleClose() is called, it will only remove the most recent listener, leaving previous listeners attached. This creates a memory leak. Consider removing the existing listener before reassigning boundFocusIn, or check if it already exists before creating a new one.

Copilot uses AI. Check for mistakes.

protected handleClose(): void {
if (this.boundFocusIn) {
window.removeEventListener('focusin', this.boundFocusIn, true);
}
}
Comment on lines +213 to +222
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The new handleOpen() and handleClose() methods lack test coverage. Add tests to verify that these methods properly add and remove the focusin event listener, and that they are called correctly when the popover opens and closes.

Copilot uses AI. Check for mistakes.
}
5 changes: 4 additions & 1 deletion projects/ui/src/lib/components/po-popover/po-popover-base.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { PO_CONTROL_POSITIONS } from './../../services/po-control-position/po-co

const PO_POPOVER_DEFAULT_POSITION = 'right';
const PO_POPOVER_DEFAULT_TRIGGER = 'click';
const PO_POPOVER_TRIGGERS = ['click', 'hover'];
const PO_POPOVER_TRIGGERS = ['click', 'hover', 'function'];

/**
* @description
Expand Down Expand Up @@ -96,6 +96,9 @@ export class PoPopoverBaseComponent {
/** Evento disparado ao fechar o popover. */
@Output('p-close') closePopover = new EventEmitter<any>();

/** Evento disparado ao abrir o popover. */
@Output('p-open') openPopover = new EventEmitter<any>();
Comment on lines +99 to +100
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The new openPopover event output lacks test coverage. Add tests to verify that the openPopover event is emitted when the popover opens and that it's emitted at the correct time in the open() lifecycle.

Copilot uses AI. Check for mistakes.

// Controla se o popover fica oculto ou visível, por padrão é oculto.
isHidden: boolean = true;

Expand Down
17 changes: 15 additions & 2 deletions projects/ui/src/lib/components/po-popover/po-popover.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ export class PoPopoverComponent extends PoPopoverBaseComponent implements AfterV
close(): void {
this.isHidden = true;
this.closePopover.emit();

if (this.trigger === 'function' && this.clickoutListener) {
this.clickoutListener();
}

this.cd.detectChanges();
}

Expand All @@ -121,8 +126,16 @@ export class PoPopoverComponent extends PoPopoverBaseComponent implements AfterV
this.setElementsControlPosition();
this.setPopoverPosition();
this.setOpacity(1);
this.openPopover.emit();
this.cd.detectChanges();
});

if (this.trigger === 'function') {
this.clickoutListener = this.renderer.listen('document', 'click', (event: MouseEvent) => {
this.togglePopup(event);
});
}
Comment on lines +133 to +137
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The clickoutListener created here for the 'function' trigger is not being properly cleaned up when the popover is reopened multiple times. If a user opens and closes the popover repeatedly, a new listener will be added each time open() is called, but the previous listener from line 107-109 only removes one instance. This creates a memory leak where multiple document click listeners accumulate. Consider checking if clickoutListener already exists before creating a new one, or ensure proper cleanup in the close() method before creating a new listener in open().

Copilot uses AI. Check for mistakes.
Comment on lines +133 to +137
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The new 'function' trigger type lacks test coverage. The existing test suite does not include tests for the behavior when trigger is set to 'function', including the creation and cleanup of the clickoutListener in the open() and close() methods. Add tests to verify that the listener is properly created on open, properly cleaned up on close, and that the togglePopup behavior works correctly with the 'function' trigger.

Copilot uses AI. Check for mistakes.

Comment on lines 130 to +138
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The openPopover event is emitted inside a setTimeout callback, but the clickoutListener is registered immediately after the setTimeout (outside the callback). This creates a race condition where the clickoutListener could be triggered before the openPopover event is emitted and before handleOpen() is called in po-helper component. This means the focus listener might not be set up when needed. Consider moving the clickoutListener registration inside the setTimeout callback after the openPopover.emit() call to ensure proper sequencing.

Suggested change
this.cd.detectChanges();
});
if (this.trigger === 'function') {
this.clickoutListener = this.renderer.listen('document', 'click', (event: MouseEvent) => {
this.togglePopup(event);
});
}
if (this.trigger === 'function') {
this.clickoutListener = this.renderer.listen('document', 'click', (event: MouseEvent) => {
this.togglePopup(event);
});
}
this.cd.detectChanges();
});

Copilot uses AI. Check for mistakes.
this.cd.detectChanges();
}

Expand Down Expand Up @@ -158,7 +171,7 @@ export class PoPopoverComponent extends PoPopoverBaseComponent implements AfterV
this.mouseLeaveListener = this.renderer.listen(this.targetElement, 'mouseleave', (event: MouseEvent) => {
this.close();
});
} else {
} else if (this.trigger === 'click') {
this.clickoutListener = this.renderer.listen('document', 'click', (event: MouseEvent) => {
this.togglePopup(event);
});
Expand Down Expand Up @@ -190,7 +203,7 @@ export class PoPopoverComponent extends PoPopoverBaseComponent implements AfterV
if (!this.appendBox) {
this.close();
}
} else if (this.targetElement?.contains(event.target)) {
} else if (this.targetElement?.contains(event.target) && this.trigger !== 'function') {
this.popoverElement.nativeElement.hidden ? this.open() : this.close();
}
}
Expand Down
Loading