Skip to content

Commit

Permalink
fix(combobox, input-time-zone): fix initial item selection delay (#11326
Browse files Browse the repository at this point in the history 
)

**Related Issue:** #10731  

## Summary  

Fixes a combobox regression introduced by
#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
  • Loading branch information
@jcfranco
jcfranco authored Jan 20, 2025
1 parent 969e194 commit 59ab243
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 45 deletions.
29 changes: 24 additions & 5 deletions packages/calcite-components/src/components/combobox/combobox.e2e.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1214,7 +1214,7 @@ describe("calcite-combobox", () => {
</calcite-combobox>`,
);

const eventSpy = await page.spyOnEvent("calciteComboboxChipClose", "window");
const eventSpy = await page.spyOnEvent("calciteComboboxChipClose");

const chip = await page.find("calcite-combobox >>> calcite-chip");

Expand All @@ -1235,6 +1235,7 @@ describe("calcite-combobox", () => {
</calcite-combobox>
`);
const input = await page.find("calcite-combobox >>> input");
const eventSpy = await page.spyOnEvent("calciteComboboxChange");

await input.click();
await input.press("K");
Expand All @@ -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 () => {
Expand All @@ -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");
Expand All @@ -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 () => {
Expand All @@ -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");
Expand All @@ -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);
});
});

Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -2013,6 +2019,7 @@ describe("calcite-combobox", () => {
<calcite-combobox-item id="three" value="three" text-label="three"></calcite-combobox-item>
</calcite-combobox>
`);
const eventSpy = await page.spyOnEvent("calciteComboboxChange");
let chip = await page.find("calcite-combobox >>> calcite-chip");
expect(chip).toBeNull();

Expand All @@ -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();
Expand All @@ -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 () => {
Expand All @@ -2049,6 +2058,7 @@ describe("calcite-combobox", () => {

await input.press("K");
await input.press("Enter");
await page.waitForChanges();
expect(eventSpy).toHaveReceivedEventTimes(1);
});

Expand All @@ -2063,6 +2073,7 @@ describe("calcite-combobox", () => {
<button>OK</button>
`);
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");
Expand All @@ -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();
Expand All @@ -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 () => {
Expand All @@ -2104,6 +2117,7 @@ describe("calcite-combobox", () => {
</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");
Expand All @@ -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);
});
});

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -2507,12 +2523,12 @@ describe("calcite-combobox", () => {

async function assertClickOutside(selectionMode = "multiple", allowCustomValues = false): Promise<void> {
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(() => {
Expand All @@ -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) => {
Expand Down Expand Up @@ -2558,6 +2575,7 @@ describe("calcite-combobox", () => {

async function clearInputValueOnBlur(selectionMode = "multiple", allowCustomValues = false): Promise<void> {
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`);
Expand All @@ -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) => {
Expand Down
68 changes: 28 additions & 40 deletions packages/calcite-components/src/components/combobox/combobox.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,6 @@ export class Combobox

// #region Private Properties

private internalComboboxChangeEvent = (): void => {
this.calciteComboboxChange.emit();
};

private allSelectedIndicatorChipEl: Chip["el"];

private chipContainerEl: HTMLDivElement;
Expand All @@ -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) {
Expand Down Expand Up @@ -216,6 +214,8 @@ export class Combobox

private groupData: GroupData[];

private groupItems: HTMLCalciteComboboxItemGroupElement["el"][] = [];

private guid = guid();

private ignoreSelectedEventsFlag = false;
Expand All @@ -224,6 +224,8 @@ export class Combobox

private internalValueChangeFlag = false;

private items: HTMLCalciteComboboxItemElement["el"][] = [];

labelEl: Label["el"];

private listContainerEl: HTMLDivElement;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -564,7 +560,6 @@ export class Combobox

async load(): Promise<void> {
setUpLoadableComponent(this);
this.filterItems(this.filterText, false, false);
}

override willUpdate(changes: PropertyValues<this>): void {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
}
}
Expand Down Expand Up @@ -718,7 +708,9 @@ export class Combobox
event: CustomEvent<HTMLCalciteComboboxItemElement["el"]>,
): void {
event.stopPropagation();
this.updateItems();
if (this.hasUpdated) {
this.updateItems();
}
}

private clearValue(): void {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -1260,7 +1258,7 @@ export class Combobox
nextGroupItem.afterEmptyGroup = groupItem.children.length === 0;
}
});
}, DEBOUNCE.nextTick);
}

private getData(): ItemData[] {
return this.items.map((item) => ({
Expand All @@ -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 = "";
Expand All @@ -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",
Expand All @@ -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();
}
}

Expand Down

0 comments on commit 59ab243

Please sign in to comment.