Skip to content

Commit 97a3f25

Browse files
committed
fix(core): memory leaks from connected callback states
- Added tests to ensure that the checked state of checkboxes and radio buttons resets to their native defaults upon calling the reset method. - Improved the handling of observers in form controls to prevent duplication after reconnecting. - Enhanced the Select component to stop observing removed options and to reset selected options to their native defaults. - Added accessibility tests for the PagePanel component to ensure compliance with axe standards. Signed-off-by: Cory Rylan <crylan@nvidia.com>
1 parent 686ff96 commit 97a3f25

26 files changed

Lines changed: 686 additions & 189 deletions

projects/core/src/checkbox/checkbox.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,17 @@ describe(`${Checkbox.metadata.tag} - control base behavior`, () => {
8989
expect((e as Event).composed).toBe(true);
9090
});
9191

92+
it('should reset checked state to the native default', async () => {
93+
const input = fixture.querySelector('input');
94+
input.defaultChecked = true;
95+
input.checked = false;
96+
97+
element.reset();
98+
await elementIsStable(element);
99+
100+
expect(input.checked).toBe(true);
101+
});
102+
92103
it('should disconnect observers when removed from DOM', async () => {
93104
const input = fixture.querySelector('input');
94105
expect(element.matches(':state(checked)')).toBe(false);

projects/core/src/format-relative-time/format-relative-time.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,5 +408,19 @@ describe(FormatRelativeTime.metadata.tag, () => {
408408
const time = element.shadowRoot!.querySelector('time');
409409
expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z');
410410
});
411+
412+
it('should fall back to raw string for relative values outside Intl range', async () => {
413+
vi.useRealTimers();
414+
const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY);
415+
416+
element.date = '2023-07-27T12:00:00.000Z';
417+
element.requestUpdate();
418+
await elementIsStable(element);
419+
420+
const time = element.shadowRoot!.querySelector('time');
421+
expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z');
422+
423+
now.mockRestore();
424+
});
411425
});
412426
});

projects/core/src/format-relative-time/format-relative-time.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ export class FormatRelativeTime extends LitElement {
9595
const value = Math.round(absDiff / UNIT_DIVISORS[unit]);
9696
if (value < max) return { value: sign * value, unit };
9797
}
98-
throw new Error('format-relative-time: no relative time threshold matched');
98+
return { value: sign * Math.round(absDiff / UNIT_DIVISORS.year), unit: 'year' };
9999
}
100100

101101
#computeExplicitUnit(diffMs: number, unit: TimeUnitOption): number {

projects/core/src/forms/control-group/control-group.css

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
--color: var(--nve-sys-interaction-field-color);
66
--opacity: 1;
77
--label-color: var(--nve-sys-interaction-field-color);
8-
--label-text-transform: capitalize;
8+
--label-text-transform: none;
99
--label-font-weight: var(--nve-ref-font-weight-medium);
1010
--label-font-size: var(--nve-ref-font-size-100);
1111
--_label-width: var(--label-width, 180px);

projects/core/src/forms/control-group/control-group.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ import {
1212
associateControlGroup
1313
} from '@nvidia-elements/core/internal';
1414
import type { ControlMessage } from '../control-message/control-message.js';
15-
import { setupControlStatusStates, setupControlGroupStates, inputQuery } from '../utils/states.js';
15+
import {
16+
setupControlStatusStates,
17+
setupControlGroupStates,
18+
inputQuery,
19+
type ControlStateCleanup
20+
} from '../utils/states.js';
1621
import { setupControlLayoutStates } from '../utils/layout.js';
1722
import styles from './control-group.css?inline';
1823

@@ -53,7 +58,7 @@ export class ControlGroup extends LitElement {
5358
return this.querySelectorAll ? Array.from(this.querySelectorAll<ControlMessage>('nve-control-message')) : [];
5459
}
5560

56-
#observers: (MutationObserver | ResizeObserver)[] = [];
61+
#observers: (MutationObserver | ResizeObserver | ControlStateCleanup)[] = [];
5762

5863
static styles = useStyles([styles]);
5964

projects/core/src/forms/control/control.css

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
--accent-color: var(--nve-sys-accent-secondary-background);
77
--color: var(--nve-sys-interaction-field-color);
88
--label-color: var(--nve-sys-interaction-field-color);
9-
--label-text-transform: capitalize;
9+
--label-text-transform: none;
1010
--label-font-weight: var(--nve-ref-font-weight-medium);
1111
--label-font-size: var(--nve-ref-font-size-100);
1212
--control-width: 100%;
@@ -38,10 +38,6 @@
3838
align-self: center !important;
3939
}
4040

41-
::slotted(label::first-letter) {
42-
text-transform: capitalize;
43-
}
44-
4541
:host(:state(disabled)),
4642
:host(:state(disabled)) ::slotted(nve-control-message:not([status])) {
4743
--cursor: not-allowed;

projects/core/src/forms/control/control.test.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// SPDX-License-Identifier: Apache-2.0
33

44
import { html } from 'lit';
5-
import { describe, expect, it, beforeEach, afterEach } from 'vitest';
5+
import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest';
66
import { createFixture, removeFixture, elementIsStable, untilEvent } from '@internals/testing';
77
import { Control, ControlMessage } from '@nvidia-elements/core/forms';
88
import '@nvidia-elements/core/forms/define.js';
@@ -167,6 +167,33 @@ describe(Control.metadata.tag, () => {
167167
expect(getComputedStyle(message).display).toBe('block');
168168
expect(validationMessage.hidden).toBe(true);
169169
});
170+
171+
it('should not duplicate form reset listeners after reconnect', async () => {
172+
removeFixture(fixture);
173+
fixture = await createFixture(html`
174+
<form>
175+
<nve-control>
176+
<label>label</label>
177+
<input type="text" required />
178+
<nve-control-message>message</nve-control-message>
179+
</nve-control>
180+
</form>
181+
`);
182+
const form = fixture.querySelector('form');
183+
element = fixture.querySelector(Control.metadata.tag);
184+
await elementIsStable(element);
185+
186+
element.remove();
187+
form.appendChild(element);
188+
await elementIsStable(element);
189+
element.shadowRoot!.dispatchEvent(new Event('slotchange'));
190+
await elementIsStable(element);
191+
192+
const requestUpdate = vi.spyOn(element, 'requestUpdate');
193+
form.dispatchEvent(new Event('reset'));
194+
195+
expect(requestUpdate).toHaveBeenCalledTimes(2);
196+
});
170197
});
171198

172199
describe(`${Control.metadata.tag}: custom`, () => {

projects/core/src/forms/control/control.ts

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,20 @@ import {
2121
setupControlValidationStates,
2222
setupControlStates,
2323
setupControlStatusStates,
24-
inputQuery
24+
inputQuery,
25+
type ControlStateCleanup
2526
} from '../utils/states.js';
2627
import { setupControlLayoutStates } from '../utils/layout.js';
2728
import globalStyles from './control.global.css?inline';
2829
import styles from './control.css?inline';
2930

31+
interface ResettableControl {
32+
getAttribute: Element['getAttribute'];
33+
value: string;
34+
}
35+
36+
type ControlInput = HTMLInputElement | HTMLSelectElement | HTMLTextAreaElement | ResettableControl;
37+
3038
/**
3139
* @element nve-control
3240
* @description Wraps a form input with its associated label and validation messages, managing layout and accessibility associations.
@@ -118,7 +126,7 @@ export class Control extends LitElement {
118126
/** @private */
119127
declare _internals: ElementInternals;
120128

121-
#observers: (MutationObserver | ResizeObserver)[] = [];
129+
#observers: (MutationObserver | ResizeObserver | ControlStateCleanup)[] = [];
122130

123131
protected _associateDatalist = true;
124132

@@ -181,15 +189,42 @@ export class Control extends LitElement {
181189

182190
/** Resets control value to initial attribute value and clears any active validation rules. */
183191
reset() {
184-
this.input.value = this.input.getAttribute('value') ?? '';
192+
this.#resetInputValue(this.input as ControlInput);
185193
this.requestUpdate();
186194
this.dispatchEvent(new CustomEvent('reset', { bubbles: true, composed: true }));
187195
}
188196

189-
#setupInput() {
190-
setupControlValidationStates(this, this.#messages);
197+
#resetInputValue(input: ControlInput) {
198+
if (input instanceof HTMLSelectElement) {
199+
this.#resetSelectValue(input);
200+
return;
201+
}
202+
203+
if (this.#isCheckedInput(input)) {
204+
input.checked = input.defaultChecked;
205+
input.indeterminate = false;
206+
return;
207+
}
191208

209+
input.value =
210+
input instanceof HTMLInputElement || input instanceof HTMLTextAreaElement
211+
? input.defaultValue
212+
: (input.getAttribute('value') ?? '');
213+
}
214+
215+
#resetSelectValue(input: HTMLSelectElement) {
216+
const hasDefaultSelection = Array.from(input.options).some(option => option.defaultSelected);
217+
Array.from(input.options).forEach(option => (option.selected = option.defaultSelected));
218+
if (!input.multiple && !hasDefaultSelection) input.selectedIndex = 0;
219+
}
220+
221+
#isCheckedInput(input: ControlInput): input is HTMLInputElement {
222+
return input instanceof HTMLInputElement && (input.type === 'checkbox' || input.type === 'radio');
223+
}
224+
225+
#setupInput() {
192226
this.#observers.push(
227+
...setupControlValidationStates(this, this.#messages),
193228
...setupControlStates(this),
194229
...setupControlStatusStates(this, this.#messages),
195230
setupControlLayoutStates(this),

0 commit comments

Comments
 (0)