From 1a13a46096f35574e64aa24e67aec85a785cb42e Mon Sep 17 00:00:00 2001 From: Lukas Maurer Date: Tue, 12 Nov 2024 10:11:04 +0100 Subject: [PATCH] fix(core): prevent memory leaks caused by detached dom nodes (#1522) Co-authored-by: Daniel Leroux Co-authored-by: matthias --- .changeset/serious-kings-explode.md | 5 + packages/core/component-doc.json | 67 +++--- packages/core/src/components.d.ts | 28 ++- .../src/components/breadcrumb/breadcrumb.tsx | 30 ++- .../breadcrumb/test/breadcrumb.ct.ts | 6 +- .../category-filter/category-filter.tsx | 226 ++++++++++++------ .../date-dropdown/date-dropdown.tsx | 45 ++-- .../components/date-picker/date-picker.tsx | 158 ++++++------ .../dropdown-button/dropdown-button.tsx | 17 +- .../dropdown/dropdown-controller.ts | 26 -- .../core/src/components/dropdown/dropdown.tsx | 168 +++++++------ .../core/src/components/select/select.tsx | 120 +++++----- .../src/components/select/test/select.ct.ts | 6 +- .../core/src/components/tooltip/tooltip.tsx | 3 +- .../utils/disposable-event-listener.ts | 36 +++ .../src/components/utils/element-reference.ts | 10 + packages/core/src/components/utils/focus.ts | 47 ++-- .../core/src/components/utils/make-ref.ts | 2 +- packages/core/src/index.ts | 1 + tools/strict-check/package.json | 1 + 20 files changed, 605 insertions(+), 397 deletions(-) create mode 100644 .changeset/serious-kings-explode.md create mode 100644 packages/core/src/components/utils/disposable-event-listener.ts create mode 100644 packages/core/src/components/utils/element-reference.ts diff --git a/.changeset/serious-kings-explode.md b/.changeset/serious-kings-explode.md new file mode 100644 index 00000000000..c06f0531100 --- /dev/null +++ b/.changeset/serious-kings-explode.md @@ -0,0 +1,5 @@ +--- +"@siemens/ix": patch +--- + +fix(core): prevent detached dom nodes diff --git a/packages/core/component-doc.json b/packages/core/component-doc.json index 47f9ee17ae8..eb3cbe2cf3b 100644 --- a/packages/core/component-doc.json +++ b/packages/core/component-doc.json @@ -3504,6 +3504,7 @@ "reflectToAttr": false, "docs": "Picker date. If the picker is in range mode this property is the start date.\nIf set to `null` no default start date will be pre-selected.\n\nFormat is based on `format`", "docsTags": [], + "default": "''", "values": [ { "type": "string" @@ -3591,6 +3592,7 @@ "reflectToAttr": false, "docs": "The latest date that can be selected by the date picker.\nIf not set there will be no restriction.", "docsTags": [], + "default": "''", "values": [ { "type": "string" @@ -3612,6 +3614,7 @@ "reflectToAttr": false, "docs": "The earliest date that can be selected by the date picker.\nIf not set there will be no restriction.", "docsTags": [], + "default": "''", "values": [ { "type": "string" @@ -3655,6 +3658,7 @@ "reflectToAttr": false, "docs": "Picker date. If the picker is in range mode this property is the end date.\nIf the picker is not in range mode leave this value `null`\n\nFormat is based on `format`", "docsTags": [], + "default": "''", "values": [ { "type": "string" @@ -3957,13 +3961,12 @@ "text": "2.1.0" } ], - "default": "undefined", "values": [ { "type": "string" } ], - "optional": false, + "optional": true, "required": false }, { @@ -3984,6 +3987,7 @@ "text": "1.1.0" } ], + "default": "''", "values": [ { "type": "string" @@ -4010,6 +4014,7 @@ "text": "1.1.0" } ], + "default": "''", "values": [ { "type": "string" @@ -4062,6 +4067,7 @@ "text": "since 2.1.0. Use `i18nDone`" } ], + "default": "''", "deprecation": "since 2.1.0. Use `i18nDone`", "values": [ { @@ -5212,14 +5218,15 @@ "props": [ { "name": "anchor", - "type": "HTMLElement | string", + "type": "HTMLElement | Promise | string", "complexType": { - "original": "string | HTMLElement", - "resolved": "HTMLElement | string", + "original": "ElementReference", + "resolved": "HTMLElement | Promise | string", "references": { - "HTMLElement": { - "location": "global", - "id": "global::HTMLElement" + "ElementReference": { + "location": "import", + "path": "src/components/utils/element-reference", + "id": "src/components/utils/element-reference.ts::ElementReference" } } }, @@ -5232,11 +5239,14 @@ { "type": "HTMLElement" }, + { + "type": "Promise" + }, { "type": "string" } ], - "optional": false, + "optional": true, "required": false }, { @@ -5437,16 +5447,13 @@ "name": "trigger", "type": "HTMLElement | Promise | string", "complexType": { - "original": "string | HTMLElement | Promise", + "original": "ElementReference", "resolved": "HTMLElement | Promise | string", "references": { - "HTMLElement": { - "location": "global", - "id": "global::HTMLElement" - }, - "Promise": { - "location": "global", - "id": "global::Promise" + "ElementReference": { + "location": "import", + "path": "src/components/utils/element-reference", + "id": "src/components/utils/element-reference.ts::ElementReference" } } }, @@ -5466,7 +5473,7 @@ "type": "string" } ], - "optional": false, + "optional": true, "required": false } ], @@ -5660,7 +5667,7 @@ "type": "string" } ], - "optional": false, + "optional": true, "required": false }, { @@ -5681,7 +5688,7 @@ "type": "string" } ], - "optional": false, + "optional": true, "required": false }, { @@ -5764,7 +5771,7 @@ "type": "string" } ], - "optional": false, + "optional": true, "required": false }, { @@ -16135,16 +16142,13 @@ "name": "for", "type": "HTMLElement | Promise | string", "complexType": { - "original": "string | HTMLElement | Promise", + "original": "ElementReference", "resolved": "HTMLElement | Promise | string", "references": { - "HTMLElement": { - "location": "global", - "id": "global::HTMLElement" - }, - "Promise": { - "location": "global", - "id": "global::Promise" + "ElementReference": { + "location": "import", + "path": "../utils/element-reference", + "id": "src/components/utils/element-reference.ts::ElementReference" } } }, @@ -17905,6 +17909,11 @@ "docstring": "", "path": "src/components/category-filter/input-state.ts" }, + "src/components/utils/element-reference.ts::ElementReference": { + "declaration": "export type ElementReference = string | HTMLElement | Promise;", + "docstring": "", + "path": "src/components/utils/element-reference.ts" + }, "src/components/flip-tile/flip-tile-state.ts::FlipTileState": { "declaration": "export enum FlipTileState {\n None = 'none',\n Info = 'info',\n Warning = 'warning',\n Alarm = 'alarm',\n Primary = 'primary',\n}", "docstring": "", diff --git a/packages/core/src/components.d.ts b/packages/core/src/components.d.ts index c92cc204b11..b3d310448ac 100644 --- a/packages/core/src/components.d.ts +++ b/packages/core/src/components.d.ts @@ -24,6 +24,7 @@ import { DateTimeCardCorners } from "./components/date-time-card/date-time-card" import { DateChangeEvent } from "./components/date-picker/date-picker"; import { DateTimeCardCorners as DateTimeCardCorners1 } from "./components/date-time-card/date-time-card"; import { DateTimeDateChangeEvent, DateTimeSelectEvent } from "./components/datetime-picker/datetime-picker"; +import { ElementReference } from "./components/utils/element-reference"; import { CloseBehavior } from "./components/dropdown/dropdown-controller"; import { AlignedPlacement, Side } from "./components/dropdown/placement"; import { DropdownButtonVariant } from "./components/dropdown-button/dropdown-button"; @@ -42,6 +43,7 @@ import { TabClickDetail } from "./components/tab-item/tab-item"; import { TimePickerCorners } from "./components/time-picker/time-picker"; import { ToastConfig, ToastType } from "./components/toast/toast-utils"; import { ShowToastResult } from "./components/toast/toast-container"; +import { ElementReference as ElementReference1 } from "./components/utils/element-reference"; import { Element } from "@stencil/core"; import { TreeContext, TreeItemContext, TreeModel, UpdateCallback } from "./components/tree/tree-model"; import { TextDecoration, TypographyColors, TypographyFormat } from "./components/typography/typography"; @@ -65,6 +67,7 @@ export { DateTimeCardCorners } from "./components/date-time-card/date-time-card" export { DateChangeEvent } from "./components/date-picker/date-picker"; export { DateTimeCardCorners as DateTimeCardCorners1 } from "./components/date-time-card/date-time-card"; export { DateTimeDateChangeEvent, DateTimeSelectEvent } from "./components/datetime-picker/datetime-picker"; +export { ElementReference } from "./components/utils/element-reference"; export { CloseBehavior } from "./components/dropdown/dropdown-controller"; export { AlignedPlacement, Side } from "./components/dropdown/placement"; export { DropdownButtonVariant } from "./components/dropdown-button/dropdown-button"; @@ -83,6 +86,7 @@ export { TabClickDetail } from "./components/tab-item/tab-item"; export { TimePickerCorners } from "./components/time-picker/time-picker"; export { ToastConfig, ToastType } from "./components/toast/toast-utils"; export { ShowToastResult } from "./components/toast/toast-container"; +export { ElementReference as ElementReference1 } from "./components/utils/element-reference"; export { Element } from "@stencil/core"; export { TreeContext, TreeItemContext, TreeModel, UpdateCallback } from "./components/tree/tree-model"; export { TextDecoration, TypographyColors, TypographyFormat } from "./components/typography/typography"; @@ -627,7 +631,7 @@ export namespace Components { * Format of time string See {@link "https://moment.github.io/luxon/#/formatting?id=table-of-tokens"} for all available tokens. * @since 2.1.0 */ - "locale": string; + "locale"?: string; /** * The latest date that can be selected by the date picker. If not set there will be no restriction. * @since 1.1.0 @@ -802,7 +806,7 @@ export namespace Components { /** * Define an anchor element */ - "anchor": string | HTMLElement; + "anchor"?: ElementReference; /** * Controls if the dropdown will be closed in response to a click event depending on the position of the event relative to the dropdown. If the dropdown is a child of another one, it will be closed with the parent, regardless of its own close behavior. */ @@ -817,12 +821,12 @@ export namespace Components { /** * Move dropdown along main axis of alignment */ - "offset": { + "offset"?: { mainAxis?: number; crossAxis?: number; alignmentAxis?: number; }; - "overwriteDropdownStyle": (delegate: { + "overwriteDropdownStyle"?: (delegate: { dropdownRef: HTMLElement; triggerRef?: HTMLElement; }) => Promise>; @@ -846,7 +850,7 @@ export namespace Components { /** * Define an element that triggers the dropdown. A trigger can either be a string that will be interpreted as id attribute or a DOM element. */ - "trigger": string | HTMLElement | Promise; + "trigger"?: ElementReference; /** * Update position of dropdown */ @@ -872,11 +876,11 @@ export namespace Components { /** * Button icon */ - "icon": string; + "icon"?: string; /** * Set label */ - "label": string; + "label"?: string; /** * Outline button */ @@ -885,7 +889,7 @@ export namespace Components { * Placement of the dropdown * @since 2.0.0 */ - "placement": AlignedPlacement; + "placement"?: AlignedPlacement; /** * Button variant */ @@ -2269,7 +2273,7 @@ export namespace Components { /** * CSS selector for hover trigger element e.g. `for="[data-my-custom-select]"` */ - "for"?: string | HTMLElement | Promise; + "for"?: ElementReference1; "hideDelay": number; "hideTooltip": () => Promise; /** @@ -4938,7 +4942,7 @@ declare namespace LocalJSX { /** * Define an anchor element */ - "anchor"?: string | HTMLElement; + "anchor"?: ElementReference; /** * Controls if the dropdown will be closed in response to a click event depending on the position of the event relative to the dropdown. If the dropdown is a child of another one, it will be closed with the parent, regardless of its own close behavior. */ @@ -4985,7 +4989,7 @@ declare namespace LocalJSX { /** * Define an element that triggers the dropdown. A trigger can either be a string that will be interpreted as id attribute or a DOM element. */ - "trigger"?: string | HTMLElement | Promise; + "trigger"?: ElementReference; } /** * @since 1.3.0 @@ -6526,7 +6530,7 @@ declare namespace LocalJSX { /** * CSS selector for hover trigger element e.g. `for="[data-my-custom-select]"` */ - "for"?: string | HTMLElement | Promise; + "for"?: ElementReference1; "hideDelay"?: number; /** * Define if the user can access the tooltip via mouse. diff --git a/packages/core/src/components/breadcrumb/breadcrumb.tsx b/packages/core/src/components/breadcrumb/breadcrumb.tsx index 96fe3a35163..f33be328667 100644 --- a/packages/core/src/components/breadcrumb/breadcrumb.tsx +++ b/packages/core/src/components/breadcrumb/breadcrumb.tsx @@ -21,6 +21,7 @@ import { } from '@stencil/core'; import { a11yBoolean, a11yHostAttributes } from '../utils/a11y'; import { createMutationObserver } from '../utils/mutation-observer'; +import { makeRef } from '../utils/make-ref'; let sequenceId = 0; const createId = (prefix: string = 'breadcrumb-') => { @@ -65,19 +66,20 @@ export class Breadcrumb { /** * Crumb item clicked event */ - @Event() itemClick: EventEmitter; + @Event() itemClick!: EventEmitter; /** * Next item clicked event */ - @Event() nextClick: EventEmitter<{ event: UIEvent; item: string }>; + @Event() nextClick!: EventEmitter<{ event: UIEvent; item: string }>; + + private readonly previousButtonRef = makeRef(); + private readonly nextButtonRef = makeRef(); - @State() previousButtonRef: HTMLElement; - @State() nextButtonRef: HTMLElement; @State() items: HTMLIxBreadcrumbItemElement[] = []; @State() isPreviousDropdownExpanded = false; - private mutationObserver: MutationObserver; + private mutationObserver?: MutationObserver; private previousButtonId = createId(); private previousDropdownId = createId(); @@ -117,7 +119,7 @@ export class Breadcrumb { bc.isDropdownTrigger = shouldShowDropdown; if (shouldShowDropdown) { - this.nextButtonRef = bc; + this.nextButtonRef(bc); } if (updatedItems.length < this.visibleItemCount) { @@ -143,23 +145,19 @@ export class Breadcrumb { aria-label={this.ariaLabelPreviousButton} trigger={ this.items?.length > this.visibleItemCount - ? this.previousButtonRef - : null + ? this.previousButtonRef.waitForCurrent() + : undefined } onShowChanged={({ detail }) => { this.isPreviousDropdownExpanded = detail; - const previousButton = this.hostElement.shadowRoot.getElementById( + const previousButton = this.hostElement.shadowRoot!.getElementById( this.previousButtonId ); // Need to force update previous button to change `aria-expanded` if (previousButton) { - forceUpdate( - this.hostElement.shadowRoot.getElementById( - this.previousButtonId - ) - ); + forceUpdate(previousButton); } }} > @@ -182,7 +180,7 @@ export class Breadcrumb { {this.items?.length > this.visibleItemCount ? ( (this.previousButtonRef = ref)} + ref={this.previousButtonRef} label="..." tabIndex={1} onItemClick={(event) => event.stopPropagation()} @@ -200,7 +198,7 @@ export class Breadcrumb { - + {this.nextItems?.map((item) => (