fix(material/chips): chips form control updating value immediately

[mistrykaran91]

Currently, when we have chips with form control, the value is updated only when its focused out. This fix will update the value of form control immediately

Fixes #28065

[mistrykaran91]

@mmalerba , I have raised a new PR here. The old one seems to be messed up while rebasing. Also, I have added here the extra check for disabled thingy. It might be because of this missing condition those internal google tests were failing ?

[mmalerba]

CARETAKER NOTE: There are still 18 projects with failing tests. I think they're most likely just bad tests, where people expected the current behavior because it was the current behavior, rather than because it actually makes sense. But we will need to go through and fix those before we can land this (nvm, seems to be real failures)

[mmalerba]

@mistrykaran91 I looked into one of the tests and the root of the problem seemed to be that a change event was fired even when the value didn't change. I think this is similar to what you were getting at with adding the disabled check, but it seems that's not the only scenario where it can erroneously emit a change.

[mistrykaran91]

@mistrykaran91 I looked into one of the tests and the root of the problem seemed to be that a change event was fired even when the value didn't change. I think this is similar to what you were getting at with adding the disabled check, but it seems that's not the only scenario where it can erroneously emit a change.

Ok, Not sure how can I reproduce this one but i'll take a look and try to debug where it's failing

[mistrykaran91]

@mmalerba : I have added one empty check before firing _change event, may be that should fix the issue ?

[mmalerba]

I'll give it a try, but I think a more robust solution might be to cache the previously emitted value and actually check if the current value is different than it before emitting.

edit: re-ran it and it did fix some of the tests, but not all. I think the issue is that some people have special logic like "don't add it if its already in the list", "don't add it if its not a valid choice", etc. So I do think you'll need something like the previous value caching

[mistrykaran91]

@mmalerba Got you, Just one question do I need to check the whole list before emitting the event, or should I just check the last value only, i.e., the string or so ? Or the value which is emitted from the propagateChanges method, i.e., the array which i need check to each item if that exists or so ?

[mmalerba]

I would just check the entire list

[mistrykaran91]

@mmalerba : I have done the following changes:

1. It seems previously _propagateChanges event was called two times, one for the change and as soon as I blur the changes. But one thing I wanted to understand a bit before my changes that on every blur it calls the _propagateChanges event and in that case too the same values were emitted so not sure why the internal tests pass in that case.

2. The checks I had added there because this._value was having the old value only so it acted as a cache.

3. I have added one more case to update the formcontrol i.e. in the case of removing chips.

4. If you want me to move that check specifically to _change method ? Just so that it doesn't create issue in case of blur event which works as before. In that case, I can create getter for valueToEmit and use it in the _change method and _this.value is already available. What is your opinion

[mistrykaran91]

@mmalerba : Any update on this or anything I can take a look ?

[mmalerba]

There are still internal test failures. This PR is likely too breaking to land outside of a major version change, and it will need someone with access to google's code to spend some time debugging and fixing the test failures before it can land. It'll have to be prioritized along with the other work the team is doing, so I can't really promise when we'll have time to look at it

[mistrykaran91]

There are still internal test failures. This PR is likely too breaking to land outside of a major version change, and it will need someone with access to google's code to spend some time debugging and fixing the test failures before it can land. It'll have to be prioritized along with the other work the team is doing, so I can't really promise when we'll have time to look at it

Ok, then better I'll close this MR. Just one thing we also same thing for remove chips.
I have created separate MR to include that only, I hope that should not impact. When you get some time you can take a look into it once. Thanks

#31420