From 59ab24368455cc968e990ea2d35f44a6949e4726 Mon Sep 17 00:00:00 2001 From: JC Franco Date: Mon, 20 Jan 2025 01:08:14 -0800 Subject: [PATCH] fix(combobox, input-time-zone): fix initial item selection delay (#11326) **Related Issue:** #10731 ## Summary Fixes a combobox regression introduced by https://github.com/Esri/calcite-design-system/pull/11265/ without impacting performance. ### Notable changes * drops debouncing of * item update (initial update delay was the main cause of regression) * change event emitting * adds more test coverage for change event emitting * ignores item-initiated updates until combobox is loaded * splits up item update logic into the following sections to minimize re-renders: * item/data * item props * selection/filter * minor tidy up --- .../src/components/combobox/combobox.e2e.ts | 29 ++++++-- .../src/components/combobox/combobox.tsx | 68 ++++++++----------- 2 files changed, 52 insertions(+), 45 deletions(-) diff --git a/packages/calcite-components/src/components/combobox/combobox.e2e.ts b/packages/calcite-components/src/components/combobox/combobox.e2e.ts index d1c7495cc06..10bda42d6ee 100644 --- a/packages/calcite-components/src/components/combobox/combobox.e2e.ts +++ b/packages/calcite-components/src/components/combobox/combobox.e2e.ts @@ -1214,7 +1214,7 @@ describe("calcite-combobox", () => { `, ); - const eventSpy = await page.spyOnEvent("calciteComboboxChipClose", "window"); + const eventSpy = await page.spyOnEvent("calciteComboboxChipClose"); const chip = await page.find("calcite-combobox >>> calcite-chip"); @@ -1235,6 +1235,7 @@ describe("calcite-combobox", () => { `); const input = await page.find("calcite-combobox >>> input"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); await input.click(); await input.press("K"); @@ -1248,6 +1249,7 @@ describe("calcite-combobox", () => { expect((await combobox.getProperty("selectedItems")).length).toBe(1); expect(await item.getProperty("selected")).toBe(true); + expect(eventSpy).toHaveReceivedEventTimes(1); }); it("should replace current value to new custom value in single selection mode", async () => { @@ -1262,6 +1264,7 @@ describe("calcite-combobox", () => { await skipAnimations(page); const combobox = await page.find("calcite-combobox"); const input = await page.find("calcite-combobox >>> input"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); await input.click(); await input.press("K"); @@ -1276,6 +1279,7 @@ describe("calcite-combobox", () => { expect((await combobox.getProperty("selectedItems")).length).toBe(1); expect(await customValue.getProperty("selected")).toBe(true); expect(await item1.getProperty("selected")).toBe(false); + expect(eventSpy).toHaveReceivedEventTimes(1); }); it("should auto-select new custom values in multiple selection mode", async () => { @@ -1289,6 +1293,7 @@ describe("calcite-combobox", () => { `); const combobox = await page.find("calcite-combobox"); const input = await page.find("calcite-combobox >>> input"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); await input.click(); await input.press("K"); @@ -1306,6 +1311,7 @@ describe("calcite-combobox", () => { expect(await customValue.getProperty("selected")).toBe(true); expect(await item1.getProperty("selected")).toBe(true); expect(await item2.getProperty("selected")).toBe(true); + expect(eventSpy).toHaveReceivedEventTimes(1); }); }); @@ -1716,7 +1722,7 @@ describe("calcite-combobox", () => { }); it("should cycle through items on ArrowUp/ArrowDown and toggle selection on/off on Enter", async () => { - const eventSpy = await page.spyOnEvent("calciteComboboxChange", "window"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); const item1 = await page.find("calcite-combobox-item#one"); const item2 = await page.find("calcite-combobox-item#two"); const item3 = await page.find("calcite-combobox-item#three"); @@ -2013,6 +2019,7 @@ describe("calcite-combobox", () => { `); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); let chip = await page.find("calcite-combobox >>> calcite-chip"); expect(chip).toBeNull(); @@ -2021,6 +2028,7 @@ describe("calcite-combobox", () => { await element.press("K"); await element.press("Enter"); + expect(eventSpy).toHaveReceivedEventTimes(1); chip = await page.find("calcite-combobox >>> calcite-chip"); expect(chip).toBeDefined(); @@ -2032,6 +2040,7 @@ describe("calcite-combobox", () => { await element.press("Enter"); const chips = await page.findAll("calcite-combobox >>> calcite-chip"); expect(chips.length).toBe(1); + expect(eventSpy).toHaveReceivedEventTimes(2); }); it("should fire calciteComboboxChange when entering new unknown tag", async () => { @@ -2049,6 +2058,7 @@ describe("calcite-combobox", () => { await input.press("K"); await input.press("Enter"); + await page.waitForChanges(); expect(eventSpy).toHaveReceivedEventTimes(1); }); @@ -2063,6 +2073,7 @@ describe("calcite-combobox", () => { `); const chip = await page.find("calcite-combobox >>> calcite-chip"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); expect(chip).toBeNull(); const input = await page.find("calcite-combobox >>> input"); @@ -2080,6 +2091,7 @@ describe("calcite-combobox", () => { let chips = await page.findAll("calcite-combobox >>> calcite-chip"); expect(chips.length).toBe(1); + expect(eventSpy).toHaveReceivedEventTimes(1); await input.press("j"); await page.waitForChanges(); @@ -2092,6 +2104,7 @@ describe("calcite-combobox", () => { chips = await page.findAll("calcite-combobox >>> calcite-chip"); expect(chips.length).toBe(2); + expect(eventSpy).toHaveReceivedEventTimes(2); }); it("should select known tag when input", async () => { @@ -2104,6 +2117,7 @@ describe("calcite-combobox", () => { `); let chip = await page.find("calcite-combobox >>> calcite-chip"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); expect(chip).toBeNull(); const input = await page.find("calcite-combobox >>> input"); @@ -2119,6 +2133,7 @@ describe("calcite-combobox", () => { expect(await chip.getProperty("value")).toBe("one"); const item1 = await page.find("calcite-combobox-item#one"); expect(await item1.getProperty("selected")).toBe(true); + expect(eventSpy).toHaveReceivedEventTimes(1); }); }); @@ -2398,8 +2413,9 @@ describe("calcite-combobox", () => { const page = await newE2EPage(); await page.setContent(html); await skipAnimations(page); + const openEvent = page.waitForEvent("calciteComboboxOpen"); await page.click("calcite-combobox"); - await page.waitForChanges(); + await openEvent; const activeItem = await page.find("calcite-combobox-item[active]"); expect(await activeItem.getProperty("value")).toBe(expectedActiveItemValue); @@ -2507,12 +2523,12 @@ describe("calcite-combobox", () => { async function assertClickOutside(selectionMode = "multiple", allowCustomValues = false): Promise { const combobox = await page.find("calcite-combobox"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); combobox.setProperty("selectionMode", selectionMode); combobox.setProperty("allowCustomValues", allowCustomValues); + await page.waitForChanges(); const inputEl = await page.find(`#myCombobox >>> input`); - await inputEl.focus(); - await page.waitForChanges(); expect(await page.evaluate(() => document.activeElement.id)).toBe("myCombobox"); const comboboxRect = await page.evaluate(() => { @@ -2530,6 +2546,7 @@ describe("calcite-combobox", () => { expect(await page.evaluate(() => document.activeElement.id)).not.toBe("myCombobox"); expect(await inputEl.getProperty("value")).toBe(""); expect(await combobox.getProperty("value")).toBe(allowCustomValues ? "three" : ""); + expect(eventSpy).toHaveReceivedEventTimes(allowCustomValues ? 1 : 0); } selectionModes.forEach((mode) => { @@ -2558,6 +2575,7 @@ describe("calcite-combobox", () => { async function clearInputValueOnBlur(selectionMode = "multiple", allowCustomValues = false): Promise { const combobox = await page.find("calcite-combobox"); + const eventSpy = await page.spyOnEvent("calciteComboboxChange"); combobox.setProperty("selectionMode", selectionMode); combobox.setProperty("allowCustomValues", allowCustomValues); const inputEl = await page.find(`#myCombobox >>> input`); @@ -2572,6 +2590,7 @@ describe("calcite-combobox", () => { await page.waitForChanges(); expect(await inputEl.getProperty("value")).toBe(""); expect(await combobox.getProperty("value")).toBe(allowCustomValues ? "three" : ""); + expect(eventSpy).toHaveReceivedEventTimes(allowCustomValues ? 1 : 0); } selectionModes.forEach((mode) => { diff --git a/packages/calcite-components/src/components/combobox/combobox.tsx b/packages/calcite-components/src/components/combobox/combobox.tsx index fa4d5740bb8..d74c2fdd8da 100644 --- a/packages/calcite-components/src/components/combobox/combobox.tsx +++ b/packages/calcite-components/src/components/combobox/combobox.tsx @@ -107,10 +107,6 @@ export class Combobox // #region Private Properties - private internalComboboxChangeEvent = (): void => { - this.calciteComboboxChange.emit(); - }; - private allSelectedIndicatorChipEl: Chip["el"]; private chipContainerEl: HTMLDivElement; @@ -119,7 +115,9 @@ export class Combobox defaultValue: Combobox["value"]; - private emitComboboxChange = debounce(this.internalComboboxChangeEvent, 0); + private emitComboboxChange(): void { + this.calciteComboboxChange.emit(); + } private get effectiveFilterProps(): string[] { if (!this.filterProps) { @@ -216,6 +214,8 @@ export class Combobox private groupData: GroupData[]; + private groupItems: HTMLCalciteComboboxItemGroupElement["el"][] = []; + private guid = guid(); private ignoreSelectedEventsFlag = false; @@ -224,6 +224,8 @@ export class Combobox private internalValueChangeFlag = false; + private items: HTMLCalciteComboboxItemElement["el"][] = []; + labelEl: Label["el"]; private listContainerEl: HTMLDivElement; @@ -278,12 +280,6 @@ export class Combobox @state() compactSelectionDisplay = false; - @state() groupItems: HTMLCalciteComboboxItemGroupElement["el"][] = []; - - @state() items: HTMLCalciteComboboxItemElement["el"][] = []; - - @state() needsIcon: boolean; - @state() selectedHiddenChipsCount = 0; @state() selectedVisibleChipsCount = 0; @@ -564,7 +560,6 @@ export class Combobox async load(): Promise { setUpLoadableComponent(this); - this.filterItems(this.filterText, false, false); } override willUpdate(changes: PropertyValues): void { @@ -611,16 +606,15 @@ export class Combobox } updateHostInteraction(this); - - if (!this.hasUpdated) { - this.refreshSelectionDisplay(); - } + this.refreshSelectionDisplay(); } loaded(): void { afterConnectDefaultValueSet(this, this.getValue()); connectFloatingUI(this); setComponentLoaded(this); + this.updateItems(); + this.filterItems(this.filterText, false, false); } override disconnectedCallback(): void { @@ -658,14 +652,10 @@ export class Combobox private valueHandler(value: string | string[]): void { if (!this.internalValueChangeFlag) { - const items = this.getItems(); - if (Array.isArray(value)) { - items.forEach((item) => (item.selected = value.includes(item.value))); - } else if (value) { - items.forEach((item) => (item.selected = value === item.value)); - } else { - items.forEach((item) => (item.selected = false)); - } + this.getItems().forEach((item) => { + item.selected = Array.isArray(value) ? value.includes(item.value) : value === item.value; + }); + this.updateItems(); } } @@ -718,7 +708,9 @@ export class Combobox event: CustomEvent, ): void { event.stopPropagation(); - this.updateItems(); + if (this.hasUpdated) { + this.updateItems(); + } } private clearValue(): void { @@ -931,6 +923,7 @@ export class Combobox listContainerEl.style.inlineSize = `${referenceEl.clientWidth}px`; await this.reposition(true); } + private calciteChipCloseHandler(comboboxItem: HTMLCalciteComboboxItemElement["el"]): void { this.open = false; @@ -1229,15 +1222,20 @@ export class Combobox return this.filterText === "" ? this.items : this.items.filter((item) => !item.hidden); } - private updateItems = debounce((): void => { + private updateItems(): void { this.items = this.getItems(); this.groupItems = this.getGroupItems(); + this.data = this.getData(); this.groupData = this.getGroupData(); + + this.updateItemProps(); + this.selectedItems = this.getSelectedItems(); this.filteredItems = this.getFilteredItems(); - this.needsIcon = this.getNeedsIcon(); + } + private updateItemProps(): void { this.items.forEach((item) => { item.selectionMode = this.selectionMode; item.scale = this.scale; @@ -1260,7 +1258,7 @@ export class Combobox nextGroupItem.afterEmptyGroup = groupItem.children.length === 0; } }); - }, DEBOUNCE.nextTick); + } private getData(): ItemData[] { return this.items.map((item) => ({ @@ -1281,10 +1279,6 @@ export class Combobox })); } - private getNeedsIcon(): boolean { - return isSingleLike(this.selectionMode) && this.items.some((item) => item.icon); - } - private resetText(): void { if (this.textInput.value) { this.textInput.value.value = ""; @@ -1308,9 +1302,6 @@ export class Combobox if (existingItem) { this.toggleSelection(existingItem, true); } else { - if (!this.isMulti()) { - this.toggleSelection(this.selectedItems[this.selectedItems.length - 1], false); - } const item = document.createElement( // TODO: [MIGRATION] If this is dynamically creating a web component, please read the docs: https://qawebgis.esri.com/arcgis-components/?path=/docs/lumina-jsx--docs#rendering-jsx-outside-the-component "calcite-combobox-item", @@ -1319,14 +1310,11 @@ export class Combobox item.heading = value; item.selected = true; this.el.prepend(item); - this.resetText(); + this.toggleSelection(item, true); + this.open = true; if (focus) { this.setFocus(); } - this.updateItems(); - this.filterItems(""); - this.open = true; - this.emitComboboxChange(); } }