Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(combobox, input-time-zone): fix initial item selection delay #11326

Merged
merged 13 commits into from
Jan 20, 2025
+52 −45
Merged

fix(combobox, input-time-zone): fix initial item selection delay #11326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b6b5830
fix(combobox, input-time-zone): fix initial item selection delay
jcfranco Jan 17, 2025
c7db265
tidy up
jcfranco Jan 17, 2025
ffc7ee8
update items w/ willUpdate after loaded item update
jcfranco Jan 17, 2025
f109f8d
Merge branch 'dev' into jcfranco/10731-fix-combobox-initial-item-sele…
jcfranco Jan 18, 2025
ce7054c
remove unused prop and method
jcfranco Jan 18, 2025
6446fb6
remove state from non-rendering props
jcfranco Jan 18, 2025
ae49554
allow providing scope of item updates
jcfranco Jan 18, 2025
d43f04d
tidy up
jcfranco Jan 18, 2025
77db412
tidy up
jcfranco Jan 18, 2025
94f580d
tidy up
jcfranco Jan 18, 2025
7f18667
fix typo
jcfranco Jan 18, 2025
9b1a4d6
restore refreshSelectionDisplay()
jcfranco Jan 18, 2025
f1c146d
revisit item update logic
jcfranco Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions packages/calcite-components/src/components/combobox/combobox.e2e.ts
View file
Open in desktop
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
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

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 => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we still debounce this in some cases? Like the mutation observer case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could keep an eye out for this. The MO callback kicks in once per batch of mutations, so I don't expect a significant increase in updateItem calls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. We may want to have some internal doc/guidelines on when we should debounce a function and under what circumstances.

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
Loading