perf(combobox): prevent initial list render and update tests to prove…

… that reduces render time
adobe · Feb 6, 2024 · 3dc5b1f · 3dc5b1f
1 parent 2e00064
commit 3dc5b1f
Show file tree

Hide file tree

Showing 8 changed files with 148 additions and 101 deletions.
diff --git a/packages/combobox/src/Combobox.ts b/packages/combobox/src/Combobox.ts
@@ -76,6 +76,9 @@ export class Combobox extends Textfield {
     @query('slot:not([name])')
     private optionSlot!: HTMLSlotElement;
 
+    @state()
+    overlayOpen = false;
+
     @query('#input')
     private input!: HTMLInputElement;
 
@@ -244,6 +247,7 @@ export class Combobox extends Textfield {
 
     public handleClosed(): void {
         this.open = false;
+        this.overlayOpen = false;
     }
 
     public handleOpened(): void {
@@ -262,8 +266,12 @@ export class Combobox extends Textfield {
     protected override shouldUpdate(
         changed: PropertyValues<this & { optionEls: MenuItem[] }>
     ): boolean {
-        if (changed.has('open') && !this.open) {
-            this.activeDescendant = undefined;
+        if (changed.has('open')) {
+            if (!this.open) {
+                this.activeDescendant = undefined;
+            } else {
+                this.overlayOpen = true;
+            }
         }
         if (changed.has('value')) {
             this.filterAvailableOptions();
@@ -418,26 +426,29 @@ export class Combobox extends Textfield {
                         style="min-width: ${width}px;"
                         size=${this.size}
                     >
-                        ${repeat(
-                            this.availableOptions,
-                            (option) => option.value,
-                            (option) => {
-                                return html`
-                                    <sp-menu-item
-                                        id="${option.value}"
-                                        ?focused=${this.activeDescendant
-                                            ?.value === option.value}
-                                        aria-selected=${this.activeDescendant
-                                            ?.value === option.value
-                                            ? 'true'
-                                            : 'false'}
-                                        .value=${option.value}
-                                    >
-                                        ${option.itemText}
-                                    </sp-menu-item>
-                                `;
-                            }
-                        )}
+                        ${this.overlayOpen
+                            ? repeat(
+                                  this.availableOptions,
+                                  (option) => option.value,
+                                  (option) => {
+                                      return html`
+                                          <sp-menu-item
+                                              id="${option.value}"
+                                              ?focused=${this.activeDescendant
+                                                  ?.value === option.value}
+                                              aria-selected=${this
+                                                  .activeDescendant?.value ===
+                                              option.value
+                                                  ? 'true'
+                                                  : 'false'}
+                                              .value=${option.value}
+                                          >
+                                              ${option.itemText}
+                                          </sp-menu-item>
+                                      `;
+                                  }
+                              )
+                            : html``}
                         <slot
                             hidden
                             @slotchange=${this.handleSlotchange}

diff --git a/packages/combobox/test/benchmark/basic-test.ts b/packages/combobox/test/benchmark/basic-test.ts
@@ -16,6 +16,11 @@ import { html } from '@spectrum-web-components/base';
 import { measureFixtureCreation } from '../../../../test/benchmark/helpers.js';
 import { benchmarkOptions } from '../index.js';
 
-measureFixtureCreation(html`
-    <sp-combobox .options=${benchmarkOptions}></sp-combobox>
-`);
+measureFixtureCreation(
+    html`
+        <sp-combobox .options=${benchmarkOptions}></sp-combobox>
+    `,
+    {
+        numRenders: 10,
+    }
+);
diff --git a/packages/combobox/test/benchmark/light-dom-test.ts b/packages/combobox/test/benchmark/light-dom-test.ts
@@ -16,14 +16,19 @@ import { html } from '@spectrum-web-components/base';
 import { measureFixtureCreation } from '../../../../test/benchmark/helpers.js';
 import { countryList } from '../index.js';
 
-measureFixtureCreation(html`
-    <sp-combobox>
-        ${countryList.map(
-            (option, index) => html`
-                <sp-menu-item id=${index} value=${option}>
-                    ${option}
-                </sp-menu-item>
-            `
-        )}
-    </sp-combobox>
-`);
+measureFixtureCreation(
+    html`
+        <sp-combobox>
+            ${countryList.map(
+                (option, index) => html`
+                    <sp-menu-item id=${index} value=${option}>
+                        ${option}
+                    </sp-menu-item>
+                `
+            )}
+        </sp-combobox>
+    `,
+    {
+        numRenders: 10,
+    }
+);
diff --git a/packages/combobox/test/combobox-a11y.test.ts b/packages/combobox/test/combobox-a11y.test.ts
@@ -10,7 +10,13 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 
-import { elementUpdated, expect, html, oneEvent } from '@open-wc/testing';
+import {
+    elementUpdated,
+    expect,
+    html,
+    nextFrame,
+    oneEvent,
+} from '@open-wc/testing';
 
 import '@spectrum-web-components/combobox/sp-combobox.js';
 import { Combobox } from '@spectrum-web-components/combobox';
@@ -21,7 +27,7 @@ import {
     findAccessibilityNode,
     sendKeys,
 } from '@web/test-runner-commands';
-import { comboboxFixture, isWebKit } from './index.js';
+import { comboboxFixture, isWebKit } from './helpers.js';
 import {
     withFieldLabel,
     withHelpText,
@@ -206,6 +212,11 @@ describe('Combobox accessibility', () => {
             '#apple'
         ) as MenuItem;
 
+        await elementUpdated(activeDescendant);
+        // Menu Item association with a Menu happens outside of the update lifecycle
+        await nextFrame();
+        await nextFrame();
+
         expect(activeDescendant.focused).to.be.true;
         expect(el.focused).to.be.true;
         await expect(el).to.be.accessible();

diff --git a/packages/combobox/test/combobox.data.test.ts b/packages/combobox/test/combobox.data.test.ts
@@ -21,7 +21,7 @@ import {
 import '@spectrum-web-components/combobox/sp-combobox.js';
 import '@spectrum-web-components/menu/sp-menu-item.js';
 import { fixture } from '../../../test/testing-helpers.js';
-import { comboboxFixture, TestableCombobox } from './index.js';
+import { comboboxFixture, TestableCombobox } from './helpers.js';
 import { SpectrumElement, TemplateResult } from '@spectrum-web-components/base';
 import { customElement } from '@spectrum-web-components/base/src/decorators.js';
 import { MenuItem } from '@spectrum-web-components/menu';

diff --git a/packages/combobox/test/combobox.test.ts b/packages/combobox/test/combobox.test.ts
@@ -29,7 +29,7 @@ import {
     comboboxFixture,
     TestableCombobox,
     testActiveElement,
-} from './index.js';
+} from './helpers.js';
 import { sendMouse } from '../../../test/plugins/browser.js';
 import { withTooltip } from '../stories/combobox.stories.js';
 import type { Tooltip } from '@spectrum-web-components/tooltip';
@@ -532,8 +532,6 @@ describe('Combobox', () => {
 
             await elementUpdated(el);
 
-            const item = el.shadowRoot.querySelector('#cherry') as HTMLElement;
-
             expect(el.value).to.equal('');
             expect(el.activeDescendant).to.be.undefined;
             expect(el.open).to.be.false;
@@ -542,6 +540,9 @@ describe('Combobox', () => {
             el.focusElement.click();
             await opened;
 
+            const item = el.shadowRoot.querySelector('#cherry') as HTMLElement;
+            await elementUpdated(item);
+
             expect(el.open).to.be.true;
 
             const itemValue = (item.textContent as string).trim();
@@ -569,10 +570,6 @@ describe('Combobox', () => {
 
             await elementUpdated(el);
 
-            const item = el.shadowRoot.querySelector(
-                '[value="cherry"]'
-            ) as MenuItem;
-
             expect(el.value).to.equal('');
             expect(el.activeDescendant).to.be.undefined;
             expect(el.open).to.be.false;
@@ -581,6 +578,11 @@ describe('Combobox', () => {
             el.focusElement.click();
             await opened;
 
+            const item = el.shadowRoot.querySelector(
+                '[value="cherry"]'
+            ) as MenuItem;
+            await elementUpdated(item);
+
             expect(el.open).to.be.true;
 
             const itemValue = item.itemText;
@@ -679,15 +681,17 @@ describe('Combobox', () => {
             expect(el.open).to.be.false;
             expect(el.availableOptions.length).equal(12);
             expect(el.options?.length).equal(12);
-            let items = [
-                ...el.shadowRoot.querySelectorAll('#listbox sp-menu-item'),
-            ];
-            expect(items.length).to.equal(12);
 
             const opened = oneEvent(el, 'sp-opened');
             el.click();
             await opened;
 
+            let items = [
+                ...el.shadowRoot.querySelectorAll('#listbox sp-menu-item'),
+            ];
+            expect(items.length).to.equal(12);
+            await Promise.all(items.map((item) => elementUpdated(item)));
+
             await executeServerCommand('send-keys', {
                 press: 'C',
             });

diff --git a/packages/combobox/test/helpers.ts b/packages/combobox/test/helpers.ts
@@ -0,0 +1,62 @@
+/*
+Copyright 2020 Adobe. All rights reserved.
+This file is licensed to you under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License. You may obtain a copy
+of the License at http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software distributed under
+the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
+OF ANY KIND, either express or implied. See the License for the specific language
+governing permissions and limitations under the License.
+*/
+
+import { expect, fixture } from '@open-wc/testing';
+import { html } from '@open-wc/testing';
+import { ComboboxOption } from '@spectrum-web-components/combobox';
+import '@spectrum-web-components/combobox/sp-combobox.js';
+import { MenuItem } from '@spectrum-web-components/menu';
+import { fruits } from '../stories/index.js';
+
+export type TestableCombobox = HTMLElement & {
+    activeDescendant: ComboboxOption;
+    autocomplete: 'none' | 'list';
+    availableOptions: ComboboxOption[];
+    focused: boolean;
+    focusElement: HTMLInputElement;
+    open: boolean;
+    optionEls: MenuItem[];
+    options: ComboboxOption[];
+    shadowRoot: ShadowRoot;
+    value: string;
+};
+
+export const comboboxFixture = async (): Promise<TestableCombobox> => {
+    const el = await fixture<TestableCombobox>(
+        html`
+            <sp-combobox
+                .autocomplete=${'list'}
+                label="Combobox"
+                .options=${fruits}
+            >
+                Combobox
+            </sp-combobox>
+        `
+    );
+
+    return el;
+};
+
+export const testActiveElement = (
+    el: TestableCombobox,
+    testId: string
+): void => {
+    expect(el.activeDescendant?.value).to.equal(testId);
+    const activeElement = el.shadowRoot.querySelector(
+        `#${el.activeDescendant.value}`
+    ) as HTMLElement;
+    expect(activeElement.getAttribute('aria-selected')).to.equal('true');
+};
+
+export const isWebKit =
+    /AppleWebKit/.test(window.navigator.userAgent) &&
+    !/Chrome/.test(window.navigator.userAgent);
diff --git a/packages/combobox/test/index.ts b/packages/combobox/test/index.ts
@@ -10,13 +10,6 @@ OF ANY KIND, either express or implied. See the License for the specific languag
 governing permissions and limitations under the License.
 */
 
-import { expect, fixture } from '@open-wc/testing';
-import { html } from '@open-wc/testing';
-import { ComboboxOption } from '@spectrum-web-components/combobox';
-import '@spectrum-web-components/combobox/sp-combobox.js';
-import { MenuItem } from '@spectrum-web-components/menu';
-import { fruits } from '../stories/index.js';
-
 export const countryList = [
     'Afghanistan',
     'Albania',
@@ -273,47 +266,3 @@ export const benchmarkOptions = countryList.map((value, index) => ({
     value: index.toString(),
     itemText: value,
 }));
-
-export type TestableCombobox = HTMLElement & {
-    activeDescendant: ComboboxOption;
-    autocomplete: 'none' | 'list';
-    availableOptions: ComboboxOption[];
-    focused: boolean;
-    focusElement: HTMLInputElement;
-    open: boolean;
-    optionEls: MenuItem[];
-    options: ComboboxOption[];
-    shadowRoot: ShadowRoot;
-    value: string;
-};
-
-export const comboboxFixture = async (): Promise<TestableCombobox> => {
-    const el = await fixture<TestableCombobox>(
-        html`
-            <sp-combobox
-                .autocomplete=${'list'}
-                label="Combobox"
-                .options=${fruits}
-            >
-                Combobox
-            </sp-combobox>
-        `
-    );
-
-    return el;
-};
-
-export const testActiveElement = (
-    el: TestableCombobox,
-    testId: string
-): void => {
-    expect(el.activeDescendant?.value).to.equal(testId);
-    const activeElement = el.shadowRoot.querySelector(
-        `#${el.activeDescendant.value}`
-    ) as HTMLElement;
-    expect(activeElement.getAttribute('aria-selected')).to.equal('true');
-};
-
-export const isWebKit =
-    /AppleWebKit/.test(window.navigator.userAgent) &&
-    !/Chrome/.test(window.navigator.userAgent);