refactor(helper): inicializa listener de focus após abertura do popover#2693
refactor(helper): inicializa listener de focus após abertura do popover#2693luiz-s-vasconcellos wants to merge 2 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the performance of the po-helper component by deferring the initialization of focus event listeners until the popover is actually opened, rather than adding them on component initialization. This reduces the number of active event listeners when helpers are present but not in use.
Changes:
- Introduces a new 'function' trigger type for po-popover that enables programmatic control of opening/closing
- Defers focus listener registration from ngAfterViewInit to popover open event
- Adds p-open event emitter to po-popover for lifecycle coordination
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| po-popover-base.component.ts | Adds 'function' to valid trigger types and introduces p-open event output |
| po-popover.component.ts | Implements 'function' trigger behavior with deferred click listener registration and cleanup |
| po-helper.component.ts | Moves focus listener registration from ngAfterViewInit to handleOpen/handleClose methods |
| po-helper.component.html | Switches to 'function' trigger and wires up p-open/p-close event handlers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Evento disparado ao abrir o popover. */ | ||
| @Output('p-open') openPopover = new EventEmitter<any>(); |
There was a problem hiding this comment.
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.
| protected handleOpen(): void { | ||
| this.boundFocusIn = this.closePopoverOnFocusOut.bind(this); | ||
| window.addEventListener('focusin', this.boundFocusIn, true); | ||
| } | ||
|
|
||
| protected handleClose(): void { | ||
| if (this.boundFocusIn) { | ||
| window.removeEventListener('focusin', this.boundFocusIn, true); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| if (this.trigger === 'function') { | ||
| this.clickoutListener = this.renderer.listen('document', 'click', (event: MouseEvent) => { | ||
| this.togglePopup(event); | ||
| }); | ||
| } |
There was a problem hiding this comment.
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().
| this.cd.detectChanges(); | ||
| }); | ||
|
|
||
| if (this.trigger === 'function') { | ||
| this.clickoutListener = this.renderer.listen('document', 'click', (event: MouseEvent) => { | ||
| this.togglePopup(event); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| 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(); | |
| }); |
| protected handleOpen(): void { | ||
| this.boundFocusIn = this.closePopoverOnFocusOut.bind(this); | ||
| window.addEventListener('focusin', this.boundFocusIn, true); | ||
| } |
There was a problem hiding this comment.
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.
| if (this.trigger === 'function') { | ||
| this.clickoutListener = this.renderer.listen('document', 'click', (event: MouseEvent) => { | ||
| this.togglePopup(event); | ||
| }); | ||
| } |
There was a problem hiding this comment.
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.
Proposta de solução para performance dos listeners.