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.

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

More robust autocomplete #1860

Merged
merged 14 commits into from
May 3, 2023
Merged
2 changes: 1 addition & 1 deletion e2e/integration/LinkingChildToSchool.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe("Scenario: Linking a child to a school - E2E test", function () {

// choose the school to add
cy.contains("mat-form-field", "School")
.find("[matInput]")
.find("[matInput]:visible")
.type("E2E School{enter}");

// save school in child profile
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
<!--Search-->
<input
[hidden]="!focused"
#inputElement
[formControl]="autocompleteForm"
matInput
style="text-overflow: ellipsis"
[matAutocomplete]="autoSuggestions"
(focusin)="onFocusIn()"
(focusout)="onFocusOut($event)"
/>
<!--Display-->
<input
[hidden]="focused"
[disabled]="_disabled"
matInput
style="text-overflow: ellipsis"
(focusin)="onFocusIn()"
(focusout)="onFocusIn()"
[value]="displayText"
/>
<mat-autocomplete
[disableRipple]="true"
#autoSuggestions="matAutocomplete"
(optionSelected)="select($event.option.value)"
autoActiveFirstOption
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,15 @@ describe("BasicAutocompleteComponent", () => {
expect(options).toHaveSize(3);

await options[2].click();
// When browser is not in foreground, this doesn't happen automatically
component.autocomplete.openPanel();
fixture.detectChanges();
await options[1].click();

expect(component.value).toEqual([0, 2]);
});

it("should clear the input when focusing in multi select mode", fakeAsync(() => {
it("should switch the input when focusing in multi select mode", fakeAsync(() => {
component.multi = true;
component.options = ["some", "values", "and", "other", "options"];
component.value = ["some", "values"];
Expand All @@ -159,11 +162,13 @@ describe("BasicAutocompleteComponent", () => {

component.onFocusIn();
expect(component.autocompleteForm).toHaveValue("");
expect(component.focused).toBeTrue();

component.onFocusOut({} as any);
tick(200);

expect(component.autocompleteForm).toHaveValue("some, values");
expect(component.displayText).toBe("some, values");
expect(component.focused).toBeFalse();
}));

it("should update the error state if the form is invalid", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,17 @@ export class BasicAutocompleteComponent<O, V = O>
startWith([] as SelectableOption<O, V>[])
);
showAddOption = false;
private delayedBlur: any;

get displayText() {
Copy link
Member

Choose a reason for hiding this comment

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

isn't this going against #1087 and we should rather set a variable instead of a getter?

if (this.multi) {
return this._options
.filter((o) => o.selected)
.map((o) => o.asString)
.join(", ");
} else {
return this.autocompleteForm.value;
}
}

get disabled(): boolean {
return this._disabled;
Expand Down Expand Up @@ -212,8 +222,7 @@ export class BasicAutocompleteComponent<O, V = O>
.filter((o) => o.selected)
.map((o) => o.asValue);
// re-open autocomplete to select next option
this.autocompleteForm.setValue("");
setTimeout(() => this.autocomplete.openPanel(), 100);
this.onFocusIn();
} else {
this.autocompleteForm.setValue(option.asString);
this.value = option.asValue;
Expand All @@ -230,48 +239,46 @@ export class BasicAutocompleteComponent<O, V = O>
}

onFocusIn() {
sleidig marked this conversation as resolved.
Show resolved Hide resolved
clearTimeout(this.delayedBlur);
if (!this.focused) {
if (this.multi) {
this.autocompleteForm.setValue("");
} else {
this.showAutocomplete();
}
this.focus();
if (this.multi) {
this.autocompleteForm.setValue("");
} else {
this.showAutocomplete();
sleidig marked this conversation as resolved.
Show resolved Hide resolved
}
setTimeout(() => this.inputElement.focus());
Copy link
Member

Choose a reason for hiding this comment

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

let's comment extensively why we need a timeout here and what are the tradeoffs (as we might face some related bugs again in the future 😉 )

this.focus();
}

onFocusOut(event: FocusEvent) {
if (
!this.elementRef.nativeElement.contains(event.relatedTarget as Element)
) {
// use short timeout in order for creating an option to work
this.delayedBlur = setTimeout(() => this.notifyFocusOut(), 200);
if (!this.multi) {
sleidig marked this conversation as resolved.
Show resolved Hide resolved
this.checkForExactMatch();
}
this.blur();
}
}

private notifyFocusOut() {
if (this.multi) {
this.displaySelectedOptions();
} else {
const inputValue = this.autocompleteForm.value;
const selectedOption = this._options.find(
({ asValue }) => asValue === this._value
private checkForExactMatch() {
const inputValue = this.autocompleteForm.value;
const selectedOption = this._options.find(
({ asValue }) => asValue === this._value
);
if (selectedOption?.asString !== inputValue) {
// try to select the option that matches the input string
const matchingOption = this._options.find(
({ asString }) => asString.toLowerCase() === inputValue.toLowerCase()
);
if (selectedOption?.asString !== inputValue) {
// try to select the option that matches the input string
const matchingOption = this._options.find(
({ asString }) => asString.toLowerCase() === inputValue.toLowerCase()
);
this.select(matchingOption);
}
this.select(matchingOption);
}
this.blur();
}

onContainerClick(event: MouseEvent) {
if ((event.target as Element).tagName.toLowerCase() != "input") {
this.inputElement.focus();
if (
!this._disabled &&
(event.target as Element).tagName.toLowerCase() != "input"
) {
this.onFocusIn();
}
}

Expand Down