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 29, 2024 2:18pm

[maxleiko]

Many thanks @Masty88 for taking the time to look at this. I've had a glance at your fix, sounds good, wouldn't it be better to leave the connectedCallback as is and move the check in start directly next to !this.anchorEl ?

Eg.

  private start() {
    // We can't start the positioner without an anchor
    if (!this.anchorEl || !this.active) { // MOVE THE CHECK HERE
      return;
    }

    this.cleanup = autoUpdate(this.anchorEl, this.popup, () => {
      this.reposition();
    });
  }

By doing this you get the proper behavior for any call-site of start not just connectedCallback.

[Masty88]

@maxleiko Thank you for the review! Yes, moving the check to start() is definitely better - it centralizes the validation logic and ensures proper behavior regardless of where start() is called from.
I'have updated the PR with this change.

[claviska]

Really good catch. Thanks for tracking this down and submitting a test! 🙌