spectrum-web-components fix: Mitigate Safari memory leak for input element

trafficstars

Description

This is an attempt to reduce the memory leak impact caused by the Safari bug with HTML input elements that aren't properly garbage collected.

Changes in this PR affect textfield, number-field and search:

form and input event listeners are added using addEventListener API instead of the lit declarative way
event listeners and form element are removed on disconnectedCallback
search has some extra logic to add/remove events on clear button, which was not cleaning properly when search has value and the button is rendered
search and combobox extend TextfieldBase instead of Textfield to avoid having an extra form element in search

The PR should be treated as POC to showcase the changes we need in SWC and to confirm the fix works as expected on our side. In case we want to proceed with the changes we can abstract the duplicated logic into a Mixin that could later be removed when the bug is fixed in Safari.

Related issue(s)

https://github.com/adobe/spectrum-web-components/discussions/4197

Motivation and context

How has this been tested?

[ ] Test case 1
1. Go here
2. Do this
[ ] Test case 2
1. Go here
2. Do this

Screenshots (if appropriate)

Here's a screenshot that compares DOM trees that use sp-search; left one is current SWC behavior, right one is patched using the changes in this PR. After detaching child nodes from their parent element, the ones in red are garbage collected, while the remaining ones are retained.

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

[ ] I have signed the Adobe Open Source CLA.
[ ] My code follows the code style of this project.
[ ] If my change required a change to the documentation, I have updated the documentation in this pull request.
[ ] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.
[ ] I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

Apr 08 '24 14:04 loredanaspataru

Lighthouse scores

Category	Latest (report)	Main (report)	Branch (report)
Performance	0.99	0.99	0.98
Accessibility	1	1	1
Best Practices	1	1	1
SEO	1	0.92	0.92
PWA	1	1	1

What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category	Latest	Main	Branch
Total	221.184 kB	210.336 kB 🏆	217.576 kB
Scripts	53.628 kB	47.964 kB 🏆	54.539 kB
Stylesheet	34.659 kB	30.398 kB 🏆	31.072 kB
Document	5.906 kB	5.191 kB 🏆	5.253 kB
Font	126.991 kB	126.783 kB	126.712 kB 🏆

Request Count

Category	Latest	Main	Branch
Total	45	45	43 🏆
Scripts	37	37	35 🏆
Stylesheet	5	5	5
Document	1	1	1
Font	2	2	2

Apr 08 '24 15:04 github-actions[bot]

Branch preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Apr 08 '24 15:04 github-actions[bot]

Tachometer results

Chrome

action-bar permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	484 kB	48.05ms - 50.10ms	-	faster ✔ 1% - 6% 0.54ms - 2.86ms
branch	472 kB	50.24ms - 51.31ms	slower ❌ 1% - 6% 0.54ms - 2.86ms	-

action-menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	647 kB	128.08ms - 131.70ms	-	faster ✔ 6% - 9% 7.91ms - 13.03ms
branch	634 kB	138.55ms - 142.17ms	slower ❌ 6% - 10% 7.91ms - 13.03ms	-

test-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	604 kB	59.22ms - 60.29ms	-	faster ✔ 6% - 9% 3.71ms - 5.67ms
branch	591 kB	63.62ms - 65.27ms	slower ❌ 6% - 10% 3.71ms - 5.67ms	-

test-lazy permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	603 kB	57.57ms - 58.72ms	-	faster ✔ 6% - 9% 3.87ms - 5.90ms
branch	590 kB	62.20ms - 63.87ms	slower ❌ 7% - 10% 3.87ms - 5.90ms	-

test-open-close-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	789 kB	1871.01ms - 1874.10ms	-	unsure 🔍 -0% - +0% -3.56ms - +1.03ms
branch	777 kB	1872.13ms - 1875.51ms	unsure 🔍 -0% - +0% -1.03ms - +3.56ms	-

test-open-close permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	788 kB	1858.16ms - 1861.56ms	-	unsure 🔍 -0% - +0% -0.85ms - +3.51ms
branch	775 kB	1857.18ms - 1859.88ms	unsure 🔍 -0% - +0% -3.51ms - +0.85ms	-

card permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	502 kB	37.73ms - 39.09ms	-	unsure 🔍 -6% - +0% -2.30ms - +0.14ms
branch	489 kB	38.47ms - 40.50ms	unsure 🔍 -0% - +6% -0.14ms - +2.30ms	-

color-field permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	482 kB	39.10ms - 40.28ms	-	faster ✔ 3% - 6% 1.19ms - 2.67ms
branch	475 kB	41.17ms - 42.06ms	slower ❌ 3% - 7% 1.19ms - 2.67ms	-

combobox permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	709 kB	35.61ms - 36.07ms	-	faster ✔ 2% - 4% 0.78ms - 1.45ms
branch	697 kB	36.71ms - 37.20ms	slower ❌ 2% - 4% 0.78ms - 1.45ms	-

light-dom-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	709 kB	377.02ms - 382.28ms	-	faster ✔ 3% - 6% 13.15ms - 22.35ms
branch	697 kB	393.62ms - 401.18ms	slower ❌ 3% - 6% 13.15ms - 22.35ms	-

illustrated-message permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	404 kB	13.40ms - 13.59ms	-	faster ✔ 1% - 5% 0.11ms - 0.65ms
branch	393 kB	13.62ms - 14.13ms	slower ❌ 1% - 5% 0.11ms - 0.65ms	-

menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	475 kB	202.89ms - 207.05ms	-	faster ✔ 1% - 3% 2.03ms - 7.04ms
branch	463 kB	208.11ms - 210.90ms	slower ❌ 1% - 3% 2.03ms - 7.04ms	-

number-field permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	523 kB	65.42ms - 67.19ms	-	faster ✔ 8% - 11% 5.65ms - 8.20ms
branch	511 kB	72.31ms - 74.15ms	slower ❌ 8% - 12% 5.65ms - 8.20ms	-

overlay permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	682 kB	417.05ms - 420.68ms	-	faster ✔ 1% - 2% 2.75ms - 7.21ms
branch	660 kB	422.55ms - 425.14ms	slower ❌ 1% - 2% 2.75ms - 7.21ms	-

directive-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	777 kB	21.73ms - 22.59ms	-	faster ✔ 6% - 10% 1.36ms - 2.41ms
branch	779 kB	23.74ms - 24.34ms	slower ❌ 6% - 11% 1.36ms - 2.41ms	-

element-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	763 kB	340.14ms - 343.47ms	-	faster ✔ 2% - 3% 5.34ms - 10.02ms
branch	750 kB	347.84ms - 351.12ms	slower ❌ 2% - 3% 5.34ms - 10.02ms	-

lazy-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	561 kB	39.61ms - 40.36ms	-	faster ✔ 5% - 8% 2.17ms - 3.44ms
branch	548 kB	42.28ms - 43.30ms	slower ❌ 5% - 9% 2.17ms - 3.44ms	-

picker permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	513 kB	510.01ms - 518.45ms	-	faster ✔ 1% - 3% 4.41ms - 16.89ms
branch	500 kB	520.29ms - 529.48ms	slower ❌ 1% - 3% 4.41ms - 16.89ms	-

popover permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	384 kB	11.40ms - 11.55ms	-	faster ✔ 2% - 4% 0.23ms - 0.44ms
branch	372 kB	11.73ms - 11.89ms	slower ❌ 2% - 4% 0.23ms - 0.44ms	-

search permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	471 kB	37.87ms - 38.75ms	-	faster ✔ 5% - 8% 1.98ms - 3.32ms
branch	460 kB	40.46ms - 41.47ms	slower ❌ 5% - 9% 1.98ms - 3.32ms	-

slider permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	480 kB	73.39ms - 75.31ms	-	faster ✔ 2% - 6% 1.48ms - 4.30ms
branch	467 kB	76.20ms - 78.27ms	slower ❌ 2% - 6% 1.48ms - 4.30ms	-

split-button permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	723 kB	1857.17ms - 1860.31ms	-	unsure 🔍 -0% - +0% -1.64ms - +2.85ms
branch	710 kB	1856.54ms - 1859.74ms	unsure 🔍 -0% - +0% -2.85ms - +1.64ms	-

textfield permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	432 kB	22.98ms - 23.59ms	-	faster ✔ 13% - 17% 3.62ms - 4.64ms
branch	421 kB	27.01ms - 27.82ms	slower ❌ 15% - 20% 3.62ms - 4.64ms	-

tooltip permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	561 kB	33.33ms - 33.97ms	-	faster ✔ 2% - 5% 0.82ms - 1.92ms
branch	538 kB	34.58ms - 35.47ms	slower ❌ 2% - 6% 0.82ms - 1.92ms	-

test-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	545 kB	22.78ms - 23.29ms	-	faster ✔ 8% - 11% 1.89ms - 2.79ms
branch	542 kB	25.00ms - 25.74ms	slower ❌ 8% - 12% 1.89ms - 2.79ms	-

test-element permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	656 kB	51.16ms - 52.28ms	-	faster ✔ 4% - 6% 1.89ms - 3.52ms
branch	643 kB	53.83ms - 55.02ms	slower ❌ 4% - 7% 1.89ms - 3.52ms	-

test-lazy permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	632 kB	40.92ms - 41.86ms	-	faster ✔ 6% - 9% 2.83ms - 4.19ms
branch	619 kB	44.41ms - 45.40ms	slower ❌ 7% - 10% 2.83ms - 4.19ms	-

truncated permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	528 kB	53.97ms - 54.76ms	-	faster ✔ 2% - 5% 1.34ms - 2.63ms
branch	516 kB	55.84ms - 56.85ms	slower ❌ 2% - 5% 1.34ms - 2.63ms	-

Firefox

action-bar permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	484 kB	114.07ms - 120.97ms	-	faster ✔ 1% - 8% 0.56ms - 9.44ms
branch	472 kB	119.72ms - 125.32ms	slower ❌ 0% - 8% 0.56ms - 9.44ms	-

action-menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	647 kB	279.88ms - 283.28ms	-	faster ✔ 12% - 14% 39.76ms - 45.60ms
branch	634 kB	321.89ms - 326.63ms	slower ❌ 14% - 16% 39.76ms - 45.60ms	-

test-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	604 kB	133.89ms - 137.11ms	-	unsure 🔍 -1% - +2% -1.42ms - +2.18ms
branch	591 kB	134.31ms - 135.93ms	unsure 🔍 -2% - +1% -2.18ms - +1.42ms	-

test-lazy permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	603 kB	152.33ms - 160.19ms	-	slower ❌ 8% - 15% 11.81ms - 21.43ms
branch	590 kB	136.88ms - 142.40ms	faster ✔ 8% - 13% 11.81ms - 21.43ms	-

test-open-close-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	789 kB	1901.08ms - 1913.20ms	-	slower ❌ 1% - 1% 11.47ms - 24.33ms
branch	777 kB	1887.10ms - 1891.38ms	faster ✔ 1% - 1% 11.47ms - 24.33ms	-

test-open-close permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	788 kB	1887.37ms - 1891.87ms	-	unsure 🔍 -0% - +0% -4.09ms - +3.97ms
branch	775 kB	1886.34ms - 1893.02ms	unsure 🔍 -0% - +0% -3.97ms - +4.09ms	-

card permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	502 kB	75.40ms - 82.64ms	-	unsure 🔍 -12% - +1% -9.83ms - +1.31ms
branch	489 kB	79.05ms - 87.51ms	unsure 🔍 -2% - +13% -1.31ms - +9.83ms	-

color-field permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	495 kB	78.44ms - 81.24ms	-	faster ✔ 1% - 7% 0.78ms - 5.50ms
branch	483 kB	81.08ms - 84.88ms	slower ❌ 1% - 7% 0.78ms - 5.50ms	-

combobox permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	709 kB	61.33ms - 65.51ms	-	unsure 🔍 -3% - +5% -2.09ms - +2.93ms
branch	697 kB	61.60ms - 64.40ms	unsure 🔍 -5% - +3% -2.93ms - +2.09ms	-

light-dom-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	709 kB	754.38ms - 765.30ms	-	slower ❌ 2% - 5% 14.26ms - 36.74ms
branch	697 kB	724.52ms - 744.16ms	faster ✔ 2% - 5% 14.26ms - 36.74ms	-

illustrated-message permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	404 kB	26.51ms - 28.09ms	-	unsure 🔍 -4% - +4% -0.99ms - +1.07ms
branch	393 kB	26.59ms - 27.93ms	unsure 🔍 -4% - +4% -1.07ms - +0.99ms	-

menu permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	475 kB	433.04ms - 449.24ms	-	faster ✔ 1% - 5% 3.48ms - 25.16ms
branch	463 kB	448.26ms - 462.66ms	slower ❌ 1% - 6% 3.48ms - 25.16ms	-

number-field permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	523 kB	150.27ms - 155.73ms	-	faster ✔ 5% - 11% 8.58ms - 18.10ms
branch	511 kB	162.44ms - 170.24ms	slower ❌ 6% - 12% 8.58ms - 18.10ms	-

overlay permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	770 kB	643.21ms - 669.23ms	-	slower ❌ 3% - 7% 18.31ms - 45.25ms
branch	757 kB	620.93ms - 627.95ms	faster ✔ 3% - 7% 18.31ms - 45.25ms	-

directive-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	773 kB	46.86ms - 47.50ms	-	faster ✔ 3% - 6% 1.67ms - 3.05ms
branch	761 kB	48.92ms - 50.16ms	slower ❌ 4% - 6% 1.67ms - 3.05ms	-

element-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	763 kB	665.87ms - 673.89ms	-	slower ❌ 4% - 6% 23.59ms - 35.57ms
branch	750 kB	635.85ms - 644.75ms	faster ✔ 4% - 5% 23.59ms - 35.57ms	-

lazy-test permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	561 kB	94.37ms - 102.51ms	-	unsure 🔍 -3% - +6% -3.16ms - +5.44ms
branch	548 kB	95.90ms - 98.70ms	unsure 🔍 -5% - +3% -5.44ms - +3.16ms	-

picker permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	513 kB	1022.94ms - 1032.90ms	-	faster ✔ 5% - 8% 49.07ms - 86.53ms
branch	500 kB	1077.66ms - 1113.78ms	slower ❌ 5% - 8% 49.07ms - 86.53ms	-

popover permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	384 kB	29.61ms - 33.19ms	-	unsure 🔍 -11% - +6% -3.61ms - +1.89ms
branch	372 kB	30.17ms - 34.35ms	unsure 🔍 -6% - +12% -1.89ms - +3.61ms	-

search permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	471 kB	69.12ms - 71.96ms	-	faster ✔ 7% - 13% 4.94ms - 9.90ms
branch	460 kB	75.92ms - 80.00ms	slower ❌ 7% - 14% 4.94ms - 9.90ms	-

slider permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	480 kB	164.38ms - 170.82ms	-	faster ✔ 1% - 7% 1.50ms - 11.62ms
branch	467 kB	170.25ms - 178.07ms	slower ❌ 1% - 7% 1.50ms - 11.62ms	-

split-button permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	723 kB	1876.65ms - 1879.91ms	-	unsure 🔍 -0% - -0% -6.13ms - -0.75ms
branch	710 kB	1879.58ms - 1883.86ms	unsure 🔍 +0% - +0% +0.75ms - +6.13ms	-

textfield permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	432 kB	47.57ms - 49.75ms	-	faster ✔ 9% - 18% 4.66ms - 10.42ms
branch	421 kB	53.54ms - 58.86ms	slower ❌ 9% - 22% 4.66ms - 10.42ms	-

tooltip permalink

test-basic

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	656 kB	81.56ms - 85.12ms	-	slower ❌ 9% - 15% 6.77ms - 10.91ms
branch	643 kB	73.45ms - 75.55ms	faster ✔ 8% - 13% 6.77ms - 10.91ms	-

test-directive permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	533 kB	47.42ms - 48.58ms	-	faster ✔ 14% - 21% 7.47ms - 12.69ms
branch	521 kB	55.53ms - 60.63ms	slower ❌ 16% - 26% 7.47ms - 12.69ms	-

test-element permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	656 kB	120.77ms - 129.91ms	-	unsure 🔍 -6% - +3% -7.53ms - +3.25ms
branch	643 kB	124.63ms - 130.33ms	unsure 🔍 -3% - +6% -3.25ms - +7.53ms	-

test-lazy permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	632 kB	93.89ms - 98.59ms	-	faster ✔ 6% - 13% 6.51ms - 13.85ms
branch	619 kB	103.60ms - 109.24ms	slower ❌ 7% - 15% 6.51ms - 13.85ms	-

truncated permalink

basic-test

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	528 kB	107.93ms - 114.35ms	-	unsure 🔍 -8% - +0% -9.80ms - +0.56ms
branch	516 kB	111.70ms - 119.82ms	unsure 🔍 -1% - +9% -0.56ms - +9.80ms	-

Apr 08 '24 15:04 github-actions[bot]

Also can you check if you can better abstract the AbortController to a new controller file so that the rest of the library can also leverage it?

@Rajdeepc Not sure exactly what you mean with this abstraction since AbortController is a browser API. Could you further explain this or offer an example?

Also can you let me know how can I test this?

I will update the test env I used to reflect the latest changes from this PR and get back with an answer. Thanks for taking the time to check this out

Apr 16 '24 14:04 loredanaspataru

spectrum-web-components spectrum-web-components copied to clipboard

fix: Mitigate Safari memory leak for input element

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

Lighthouse scores

Transfer Size

Request Count

Branch preview

Tachometer results

action-bar permalink

basic-test

action-menu permalink

test-basic

test-directive permalink

test-lazy permalink

test-open-close-directive permalink

test-open-close permalink

card permalink

test-basic

color-field permalink

basic-test

combobox permalink

basic-test

light-dom-test permalink

illustrated-message permalink

test-basic

menu permalink

test-basic

number-field permalink

basic-test

overlay permalink

basic-test

directive-test permalink

element-test permalink

lazy-test permalink

picker permalink

basic-test

popover permalink

test-basic

search permalink

test-basic

slider permalink

test-basic

split-button permalink

basic-test

textfield permalink

test-basic

tooltip permalink

test-basic

test-directive permalink

test-element permalink

test-lazy permalink

truncated permalink

basic-test

action-bar permalink

basic-test

action-menu permalink

test-basic

test-directive permalink

test-lazy permalink

test-open-close-directive permalink

test-open-close permalink

card permalink

test-basic

color-field permalink

basic-test

combobox permalink

basic-test

light-dom-test permalink

illustrated-message permalink

test-basic

menu permalink

test-basic

number-field permalink

spectrum-web-components
spectrum-web-components copied to clipboard