components icon indicating copy to clipboard operation
components copied to clipboard

fix(material/autocomplete): update selection on form control value changes

Open cusher opened this issue 2 years ago • 3 comments

Fixes a bug in Autocomplete where the highlighted selection would not update when the underlying form control had its value updated or was reset.

This goes beyond the changes in https://github.com/angular/components/pull/27653 which does clear the select on reset, but does not update the selection when the control value is modified.

Fixes #27861, #27652

cusher avatar Oct 26 '23 02:10 cusher

@crisbeto Thanks for the review. I rebased off main to resolve the conflicts and made the suggested change. Note the new removal of the added reset code is intentional, since it should be redundant after this change.

I did unfortunately discover an issue while re-testing w/ the demos app that only applies when the options are being asynchronously filtered:

  1. If the value is set to option A, after the filtered options propagate, autocomplete.options will only include option A.
  2. If the value is then changed to option B, option B is not present in autocomplete.options at the time writeValue is called. It also doesn't appear to be present after the promise resolves, or after any arbitrary time delay if the panel is not re-opened.
  3. When the autocomplete panel is re-opened, option B is present (and the only options) but has not been selected.

The only workaround I've been able to figure out is to call _updateMarkedSelection inside _attachOverlay (because then the value of autocomplete.options has updated) but this seems like a pretty hacky solution.

(I'm fine with this being merged as-is and creating a separate issue for that.) LMK if more changes are needed.

cusher avatar Nov 09 '23 21:11 cusher

I ran this against our internal tests which came back with a bunch of failures. They seem to come in a couple of categories:

  1. Screenshot differences because the option is now marked as selected. I think this is fine, I'll just have to approve the screenshots.
  2. There are some tests where the autocomplete was getting closed when it previously wasn't.

I think we should try this out like you had it originally: with the method call outside of the Promise.resolve.

crisbeto avatar Dec 01 '23 08:12 crisbeto

I ran the tests again and they're failing in the same way. I did a bit of investigation:

  1. Some tests are failing because we're calling displayWith much earlier than usual. The call may have to be wrapped in a try/catch.
  2. The additional events we're seeing in our own tests are breaking internal tests as well. We may have to come up with a way to show an option as selected without triggering them.

crisbeto avatar Dec 05 '23 13:12 crisbeto