fix(forms): improve select performance #61949
Closed
+578
−29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7d1a59b to
3f81874
Compare
We defer the update until after rendering is complete for two reasons: first, to avoid repeatedly calling `writeValue` on every option element until we find the selected one (could be the very last element). Second, to ensure that we perform the write after the DOM elements have been created (this doesn't happen until the end of change detection when animations are enabled). This is needed to efficiently set the select value when adding/removing options. The previous approach resulted in exponentially more `_compareValue` calls than the number of option elements (issue angular#41330). Finally, this PR fixes an issue with delayed element removal when using the animations module (in all browsers). Previously when a selected option was removed (so no option matched the ngModel anymore), Angular changed the select element value before actually removing the option from the DOM. Then when the option was finally removed from the DOM, the browser would change the select value to that of the first option, even though it didn't match the ngModel (issue angular#18430). Note that this is still somewhat of an application problem when using `ngModel`. The model value still needs to be updated to a valid value when the selected value is deleted or it will be out of sync with the DOM. Fixes angular#41330, fixes angular#18430.
3f81874 to
cd961cb
Compare
3b278f1 to
fb7134d
Compare
| @@ -136,12 +141,66 @@ export class SelectControlValueAccessor | |||
| } | |||
|
|
|||
| private _compareWith: (o1: any, o2: any) => boolean = Object.is; | |||
| // We need this because we might be in the process of destroying the root | |||
| // injector, which is marked as destroyed before running destroy hooks. | |||
| // Attempting to use afterNextRender with the node injector would evntually | |||
AndrewKushnir
approved these changes
Jun 10, 2025
|
This PR was merged into the repository by commit 4f0221e. The changes were merged into the following branches: main |
6 tasks
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We defer the update until after rendering
is complete for two reasons: first, to avoid repeatedly calling
writeValueon every option element until we find the selected one (could be the very last element). Second, to ensure that we perform the write after the DOM elements have been created (this doesn't happen until the end of change detection when animations are enabled).This is needed to efficiently set the select value when adding/removing options. The previous approach resulted in exponentially more
_compareValuecalls than the number of option elements (issue #41330).Finally, this PR fixes an issue with delayed element removal when using the animations module (in all browsers). Previously when a selected option was removed (so no option matched the ngModel anymore), Angular changed the select element value before actually removing the option from the DOM. Then when the option was finally removed from the DOM, the browser would change the select value to that of the first option, even though it didn't match the ngModel (issue #18430). Note that this is still somewhat of an application problem when using
ngModel. The model value still needs to be updated to a valid value when the selected value is deleted or it will be out of sync with the DOM.Fixes #41330, fixes #18430.
TGP
requires an update to the screenshots in one test, with minor shifting in the placement of the text in the select control (see cl/767187673)
Though there is a green TGP, this changes the timing of values being written to the DOM, which still caries some risk. We should merge to
mainonly to give more time for things to be discovered internally prior to external release.