Fix/memory leak popup

[Masty88]

Fixes #2264

Description

This pull request fixes an issue where sl-popup would create multiple instances of autoUpdate, leading to event listeners accumulating over time when components using sl-popup are repeatedly added to and removed from the DOM. This accumulation could cause performance issues and potential memory leaks.

Problem

The issue occurs due to the sequence of lifecycle methods and property updates in sl-popup. When the component is added to the DOM, the following sequence happens:

1. The component is added with active: false.
2. connectedCallback() is called, which calls start() regardless of the active state.
3. The active property changes to true.
4. The updated() method detects the change in active and calls start() again.

This results in start() being called twice without properly cleaning up the previous autoUpdate, causing multiple autoUpdate instances to run concurrently.

Changes

To fix this issue, the following changes were made:

• Modified connectedCallback() to only call start() if active is true.

  async connectedCallback() {
    super.connectedCallback();
    await this.updateComplete;
  
    // Start the positioner only if active is true
    if (this.active) {
      this.start();
    }
  }

This solution prevents start() from being called unnecessarily in connectedCallback() when the component is not active, and ensures that start() and stop() are only called in response to changes in the active property.

Testing

Writing an automated test to verify this behavior was challenging due to the nature of the event listener accumulation and limitations in accessing internal properties or intercepting event listeners in a testing environment.

Despite these challenges, I performed manual testing in the browser:

1. Repeatedly added and removed components using sl-popup (e.g., sl-select) from the DOM.
2. Monitored the number of event listeners registered for resize and scroll events.
3. Confirmed that the number of event listeners remained constant after the fix, indicating that event listeners are properly cleaned up.

Additionally, I wrote the following test to ensure that the component handles positioning correctly when the active property changes:

it('should properly handle positioning when active changes', async () => {
const element = await fixture('<sl-popup></sl-popup>');

element.active = true;
await element.updateComplete;

// Simulate a scroll event
const event = new Event('scroll');
window.dispatchEvent(event);

element.active = false;
await element.updateComplete;

// The component should not throw an error when the window is scrolled
expect(() => {
  element.active = true;
  window.dispatchEvent(event);
}).not.to.throw();
});

This test verifies that:

The component can be activated and deactivated without errors.
Event listeners do not cause issues when active changes.
The component remains stable during property changes and event dispatches.

I welcome any suggestions on how to create an effective automated test for this scenario, but given the constraints, manual testing was used to confirm the fix.

Before fix

After fix

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
shoelace	✅ Ready (Inspect)	Visit Preview	Nov 19, 2024 3:53pm