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

Jump to bottom

fix(combo): update touched property and apply invalid color on blur when required - master #14922

Merged
merged 9 commits into from
Oct 23, 2024
Merged

fix(combo): update touched property and apply invalid color on blur when required - master #14922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6bb2950
fix(combo): created a separate method for the validation logic
Zneeky Oct 17, 2024
951e9bd
fix(combo): renamed function
Zneeky Oct 18, 2024
e53d267
fix(combo): test for clicking away when the combo is opened
Zneeky Oct 18, 2024
bb0651f
fix(combo): edited combo component test
Zneeky Oct 18, 2024
ec84ebb
fix(combo): onBlur and onFocus event handling this._onTouchedCallback();
Zneeky Oct 22, 2024
c954840
Merge branch 'master' into aahmedov/fix-touched-ivalid-color-not-appl…
Zneeky Oct 22, 2024
a0e31a7
fix(combo): edited onBlur to trigger touched only in a specific scenario
Zneeky Oct 23, 2024
56c0876
Merge branch 'master' into aahmedov/fix-touched-ivalid-color-not-appl…
georgianastasov Oct 23, 2024
63aed4a
Merge branch 'master' into aahmedov/fix-touched-ivalid-color-not-appl…
kacheshmarova Oct 23, 2024
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
22 changes: 17 additions & 5 deletions projects/igniteui-angular/src/lib/combo/combo.common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,9 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
/** @hidden @internal */
public handleClosed() {
this.closed.emit({ owner: this });
if(this.comboInput.nativeElement !== this.document.activeElement){
this.validateComboState();
}
}

/** @hidden @internal */
Expand Down Expand Up @@ -1253,14 +1256,15 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
public onBlur() {
if (this.collapsed) {
this._onTouchedCallback();
if (this.ngControl && this.ngControl.invalid) {
this.valid = IgxInputState.INVALID;
} else {
this.valid = IgxInputState.INITIAL;
}
this.validateComboState();
}
}

/** @hidden @internal */
public onFocus(): void {
this._onTouchedCallback();
}
Comment on lines +1264 to +1266
Copy link
Member

@damyanpetev damyanpetev Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Zneeky @IvayloG @kacheshmarova @Lipata
I guess I'll be the bearer of bad news again, since this cannot be right :) The control cannot be in touched state on first focus, that will cause initially invalid fields to turn red on focus, which is not intended at all.
See https://angular.dev/api/forms/ControlValueAccessor#registerOnTouched

your class calls it when the control should be considered blurred or "touched".

Or just the simplest of examples with the built-in ngModel and simple input where the touched state remains false until first blur:
image
https://stackblitz.com/edit/d5xmg9?file=src%2Fmain.ts

PS: Oh wow, just realized that fix is based on a really old change in the Select and I do remember we had the ValueAccessor implemented wrong.. Hey it's even still in there 😁 That's wrong too lol

Copy link
Member

Choose a reason for hiding this comment

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

The actual issue from what I can tell is touched isn't fired on blur when clicking outside - which is fair enough, and that's likely in the close handling - there's a distinction between closing with selection/KB, which restores the focus back to the input (thus focus as a whole has not yet left the component) and outside click which does mean the component is 'blurred' and should use the touched callback.

Copy link
Member

Choose a reason for hiding this comment

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

See pretty similar handling in the Date Picker for example that does just that and the else case can be replicated for this fix:

igniteui-angular/projects/igniteui-angular/src/lib/date-picker/date-picker.component.ts

Lines 927 to 933 in d5df92a

// do not focus the input if clicking outside in dropdown mode
if (this.getEditElement() && !(args.event && this.isDropdown)) {
this.inputDirective.focus();
} else {
this._onTouchedCallback();
this.updateValidity();
}

Copy link
Contributor

@IvayloG IvayloG Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@damyanpetev @Lipata
Adding some more details:
The control cannot be in a touched state on the first focus, that will cause initially invalid fields to turn red on focus, which is not intended at all.
I was thinking the same, yet after the currently implemented fix the red color is updated on the outside click as it should.
BUG 14900 IG Combo Touched onFocus

Copy link
Member

@damyanpetev damyanpetev Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Possibly, because the error color is applied either if the underlying form control changes state (but it's invalid to begin with) or the check we evaluate on blur. Perhaps wasn't the best example, I suppose a better one would be if you have error message based on model.error.required && model.touched it should be visible before that :)

On second thought, I believe this will also mess with the updateOn: 'blur' on form controls since that relies on the touched callback to initiate the value update. I think we had a bunch of issues for that some time ago that showed us we weren't implementing the value accessor right and I thought we hunted those down across all components.

Copy link
Member

Choose a reason for hiding this comment

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

Oh more fun, the combo had the same fix as the select... except we reverted that one quick enough lol 😁
See #8123

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@damyanpetev @IvayloG
I've created a new PR to address the issues that this PR introduced, and I'd appreciate your feedback on the proposed solution.

In the new PR I've made a change to the handleClosing event:

    if (!e.event) {
        this.comboInput?.nativeElement.focus();
    } else if (this.comboInput?.nativeElement !== this.document.activeElement) {
        this._onTouchedCallback();
        this.updateValidity();
    }

I've added an else if condition to check if the currently active element is not this.comboInput. Initially, I expected this to work with a simple else, but the touched and validation were still somehow being updated unnecessarily. This extra check for the active element seems to prevent those unnecessary updates effectively.

I am sticking to what's written in the Angular's docs => When the user blurs the form control element, the control is marked as "touched".

Let me know if you have any suggestions or concerns about this approach!


/** @hidden @internal */
public setActiveDescendant(): void {
this.activeDescendant = this.dropdown.focusedItem?.id || '';
Expand All @@ -1285,6 +1289,14 @@ export abstract class IgxComboBaseDirective implements IgxComboBase, AfterViewCh
this.manageRequiredAsterisk();
};

private validateComboState() {
if (this.ngControl && this.ngControl.invalid) {
this.valid = IgxInputState.INVALID;
} else {
this.valid = IgxInputState.INITIAL;
}
}

private get isTouchedOrDirty(): boolean {
return (this.ngControl.control.touched || this.ngControl.control.dirty);
}
Expand Down
3 changes: 2 additions & 1 deletion projects/igniteui-angular/src/lib/combo/combo.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
role="combobox" aria-haspopup="listbox"
[attr.aria-expanded]="!dropdown.collapsed" [attr.aria-controls]="dropdown.listId"
[attr.aria-labelledby]="ariaLabelledBy || label?.id || placeholder"
(blur)="onBlur()" />
(blur)="onBlur()"
(focus)="onFocus()" />
<ng-container ngProjectAs="igx-suffix">
<ng-content select="igx-suffix"></ng-content>
</ng-container>
Expand Down
25 changes: 24 additions & 1 deletion projects/igniteui-angular/src/lib/combo/combo.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3606,6 +3606,29 @@ describe('igxCombo', () => {
expect(combo.valid).toEqual(IgxInputState.INITIAL);
expect(combo.comboInput.valid).toEqual(IgxInputState.INITIAL);
}));
it('should mark as touched and invalid when combo is focused, dropdown appears, and user clicks away without selection', fakeAsync(() => {
const ngModel = fixture.debugElement.query(By.directive(NgModel)).injector.get(NgModel);
expect(combo.valid).toEqual(IgxInputState.INITIAL);
expect(combo.comboInput.valid).toEqual(IgxInputState.INITIAL);
expect(ngModel.touched).toBeFalse();

combo.open();
input.triggerEventHandler('focus', {});
fixture.detectChanges();
expect(ngModel.touched).toBeTrue();
combo.searchInput.nativeElement.focus();
fixture.detectChanges();
const documentClickEvent = new MouseEvent('click', { bubbles: true });
document.body.dispatchEvent(documentClickEvent);
fixture.detectChanges();
tick();
document.body.focus();
fixture.detectChanges();
tick();
expect(combo.valid).toEqual(IgxInputState.INVALID);
expect(combo.comboInput.valid).toEqual(IgxInputState.INVALID);
expect(ngModel.touched).toBeTrue();
}));
});
});
describe('Display density', () => {
Expand Down Expand Up @@ -3787,7 +3810,7 @@ class IgxComboFormComponent {
@Component({
template: `
<form #form="ngForm">
<igx-combo #testCombo class="input-container" [placeholder]="'Locations'"
<igx-combo #testCombo #testComboNgModel="ngModel" class="input-container" [placeholder]="'Locations'"
name="anyName" required [(ngModel)]="values"
[data]="items" [disableFiltering]="disableFilteringFlag"
[displayKey]="'field'" [valueKey]="'field'"
Expand Down
Loading