diff --git a/.changeset/quiet-apricots-sneeze.md b/.changeset/quiet-apricots-sneeze.md new file mode 100644 index 000000000..c243d0e80 --- /dev/null +++ b/.changeset/quiet-apricots-sneeze.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[overlays] Fixed memory leak caused adopted style cache and the `OverlayMixin` not releasing the `OverlayController` on teardown diff --git a/packages/ui/components/overlays/src/OverlayController.js b/packages/ui/components/overlays/src/OverlayController.js index e6a642cf6..fa834fc3e 100644 --- a/packages/ui/components/overlays/src/OverlayController.js +++ b/packages/ui/components/overlays/src/OverlayController.js @@ -192,7 +192,6 @@ export class OverlayController extends EventTarget { zIndex: 9999, }; - this.manager.add(this); /** @protected */ this._contentId = `overlay-content--${Math.random().toString(36).slice(2, 10)}`; /** @private */ @@ -475,6 +474,14 @@ export class OverlayController extends EventTarget { this._init(); /** @private */ this.__elementToFocusAfterHide = undefined; + + if (!this.#isRegisteredOnManager()) { + this.manager.add(this); + } + } + + #isRegisteredOnManager() { + return Boolean(this.manager.list.find(ctrl => this === ctrl)); } /** @@ -1354,6 +1361,10 @@ export class OverlayController extends EventTarget { this.__handleOverlayStyles({ phase: 'teardown' }); this._handleFeatures({ phase: 'teardown' }); this.__wrappingDialogNode?.removeEventListener('cancel', this.__cancelHandler); + + if (this.#isRegisteredOnManager()) { + this.manager.remove(this); + } } /** @private */ diff --git a/packages/ui/components/overlays/src/OverlayMixin.js b/packages/ui/components/overlays/src/OverlayMixin.js index 9afaba49e..39466f83b 100644 --- a/packages/ui/components/overlays/src/OverlayMixin.js +++ b/packages/ui/components/overlays/src/OverlayMixin.js @@ -182,11 +182,13 @@ export const OverlayMixinImplementation = superclass => this._setupOverlayCtrl(); } - disconnectedCallback() { + async disconnectedCallback() { super.disconnectedCallback(); - if (this._overlayCtrl) { - this._teardownOverlayCtrl(); - } + + if (!this._overlayCtrl) return; + if (await this.#isMovingInDom()) return; + + this._teardownOverlayCtrl(); } /** @@ -253,6 +255,7 @@ export const OverlayMixinImplementation = superclass => _teardownOverlayCtrl() { this._teardownOpenCloseListeners(); this.__teardownSyncFromOverlayController(); + /** @type {OverlayController} */ (this._overlayCtrl).teardown(); } @@ -387,5 +390,15 @@ export const OverlayMixinImplementation = superclass => ctrl._popper.update(); } } + + /** + * When we're moving around in dom, disconnectedCallback gets called. + * Before we decide to teardown, let's wait to see if we were not just moving nodes around. + * @return {Promise} + */ + async #isMovingInDom() { + await this.updateComplete; + return this.isConnected; + } }; export const OverlayMixin = dedupeMixin(OverlayMixinImplementation); diff --git a/packages/ui/components/overlays/src/utils/adopt-styles.js b/packages/ui/components/overlays/src/utils/adopt-styles.js index bdececfc4..b5a93b9b4 100644 --- a/packages/ui/components/overlays/src/utils/adopt-styles.js +++ b/packages/ui/components/overlays/src/utils/adopt-styles.js @@ -25,7 +25,7 @@ export const _adoptStyleUtils = { adoptStyles: undefined, }; -const styleCache = new Map(); +const styleCache = new WeakMap(); /** * @param {CSSStyleSheet} cssStyleSheet @@ -81,10 +81,10 @@ function adoptStyleWhenAdoptedStylesheetsNotSupported( */ function handleCache(renderRoot, style, { teardown = false } = {}) { let haltFurtherExecution = false; - if (!styleCache.has(renderRoot)) { + if (renderRoot && !styleCache.has(renderRoot)) { styleCache.set(renderRoot, []); } - const addedStylesForRoot = styleCache.get(renderRoot); + const addedStylesForRoot = styleCache.get(renderRoot) ?? []; const foundStyle = addedStylesForRoot.find( (/** @type {import("lit").CSSResultOrNative} */ addedStyle) => style === addedStyle, ); diff --git a/packages/ui/components/overlays/test/OverlayController.test.js b/packages/ui/components/overlays/test/OverlayController.test.js index d1cfc27f2..7ce54420a 100644 --- a/packages/ui/components/overlays/test/OverlayController.test.js +++ b/packages/ui/components/overlays/test/OverlayController.test.js @@ -36,6 +36,14 @@ async function mimicEscapePress(element) { await aTimeout(0); } +/** + * @param {OverlayController} ctrlToFind + * @returns {boolean} + */ +function isRegisteredOnManager(ctrlToFind) { + return Boolean(ctrlToFind.manager?.list?.find(ctrl => ctrlToFind === ctrl)); +} + /** * Make sure that all browsers serialize html in a similar way * (Firefox tends to output empty style attrs) @@ -275,8 +283,18 @@ describe('OverlayController', () => { }); }); - // TODO: Add teardown feature tests - describe('Teardown', () => {}); + // TODO: Add more teardown feature tests + describe('Teardown', () => { + it('unregisters itself from overlayManager', async () => { + const ctrl = new OverlayController(withGlobalTestConfig()); + + expect(isRegisteredOnManager(ctrl)).to.be.true; + + ctrl.teardown(); + + expect(isRegisteredOnManager(ctrl)).to.be.false; + }); + }); describe('Node Configuration', () => { describe('Content', async () => {