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

[Zneeky]

Closes #14900

Additional information (check all that apply):

• Bug fix
• New functionality
• Documentation
• Demos
• CI/CD

Checklist:

• All relevant tags have been applied to this PR
• This PR includes unit tests covering all the new code (test guidelines)
• This PR includes API docs for newly added methods/properties (api docs guidelines)
• This PR includes feature/README.MD updates for the feature docs
• This PR includes general feature table updates in the root README.MD
• This PR includes CHANGELOG.MD updates for newly added functionality
• This PR contains breaking changes
• This PR includes ng update migrations for the breaking changes (migrations guidelines)
• This PR includes behavioral changes and the feature specification has been updated with them

[IvayloG]

@Zneeky
Have you checked if using an onFocus handler is fixing the issue? Here is a similar issue fixed for the select
#6370 and the PR. It might be preferable to use the same approach.

    public onFocus(): void {
        this._onTouchedCallback();
    }

[Zneeky]

@Zneeky Have you checked if using an onFocus handler is fixing the issue? Here is a similar issue fixed for the select #6370 and the PR. It might be preferable to use the same approach.

    public onFocus(): void {
        this._onTouchedCallback();
    }

Thank you for the recommendation! I tested it, and your suggestion works as expected. As in the reference PR, I will proceed by updating ng-touched both on onFocus and onBlur.

[IvayloG]

I prefer this approach and like the separation of concern as on the one side - _onTouchedCallback is not coupled with the validateComboState() AND on the other handling onFocus ensures the component should be more stable and less prone to bugs as this is handled as soon as the user interacts with the component. This may seem slightly redundant, yet should be more stable.

[damyanpetev]

@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:

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

[damyanpetev]

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.

[damyanpetev]

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();
	}

[IvayloG]

@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.

[damyanpetev]

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.

[damyanpetev]

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

[Zneeky]

@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!