8000 fix(forms): improve select performance · angular/angular@cd961cb · GitHub
[go: up one dir, main page]
Skip to content

Commit cd961cb

Browse files
theodorejbatscott
theodorejb
authored and
atscott
committed
fix(forms): improve select performance
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 #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.
1 parent 9354efc commit cd961cb

File tree

4 files changed

+1130
-538
lines changed

4 files changed

+1130
-538
lines changed

‎packages/forms/src/directives/select_control_value_accessor.ts

Lines changed: 62 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,15 @@
77
*/
88

99
import {
10+
afterNextRender,
11+
ApplicationRef,
12+
ChangeDetectorRef,
13+
DestroyRef,
1014
Directive,
1115
ElementRef,
1216
forwardRef,
1317
Host,
18+
inject,
1419
Input,
1520
OnDestroy,
1621
Optional,
@@ -136,12 +141,66 @@ export class SelectControlValueAccessor
136141
}
137142

138143
private _compareWith: (o1: any, o2: any) => boolean = Object.is;
144+
// We need this because we might be in the process of destroying the root
145+
// injector, which is marked as destroyed before running destroy hooks.
146+
// Attempting to use afterNextRender with the node injector would evntually
147+
// run into that already destroyed injector.
148+
private readonly appRefInjector = inject(ApplicationRef).injector;
149+
// TODO(atscott): Remove once destroyed is exposed on EnvironmentInjector
150+
private readonly appRefDestroyRef = this.appRefInjector.get(DestroyRef);
151+
private readonly destroyRef = inject(DestroyRef);
152+
private readonly cdr = inject(ChangeDetectorRef);
153+
private _queuedWrite = false;
154+
155+
/**
156+
* This is needed to efficiently set the select value when adding/removing options. If
157+
* writeValue is instead called for every added/removed option, this results in exponentially
158+
* more _compareValue calls than the number of option elements (issue #41330).
159+
*
160+
* Secondly, calling writeValue when rendering individual option elements instead of after they
161+
* are all rendered caused an issue in Safari and IE 11 where the first option element failed
162+
* to be deselected when no option matched the select ngModel. This was because Angular would
163+
* set the select element's value property before appending the option's child text node to the
164+
* DOM (issue #14505).
165+
*
166+
* Finally, this approach is necessary to avoid an issue with delayed element removal when
167+
* using the animations module (in all browsers). Otherwise when a selected option is removed
168+
* (so no option matches the ngModel anymore), Angular would change the select element value
169+
* before actually removing the option from the DOM. Then when the option is finally removed
170+
* from the DOM, the browser would change the select value to that of the first option, even
171+
* though it doesn't match the ngModel (issue #18430).
172+
*
173+
* @internal
174+
*/
175+
_writeValueAfterRender(): void {
176+
if (this._queuedWrite || this.appRefDestroyRef.destroyed) {
177+
return;
178+
}
179+
180+
this._queuedWrite = true;
181+
182+
afterNextRender(
183+
{
184+
write: () => {
185+
if (this.destroyRef.destroyed) {
186+
return;
187+
}
188+
this._queuedWrite = false;
189+
this.writeValue(this.value);
190+
},
191+
},
192+
{injector: this.appRefInjector},
193+
);
194+
}
139195

140196
/**
141197
* Sets the "value" property on the select element.
142198
* @docs-private
143199
*/
144200
writeValue(value: any): void {
201+
// TODO(atscott): This could likely be optimized more by only marking for check if the value is changed
202+
// note that this needs to include both the internal value and the value in the DOM.
203+
this.cdr.markForCheck();
145204
this.value = value;
146205
const id: string | null = this._getOptionId(value);
147206
const valueString = _buildValueString(id, value);
@@ -218,7 +277,7 @@ export class NgSelectOption implements OnDestroy {
218277
if (this._select == null) return;
219278
this._select._optionMap.set(this.id, value);
220279
this._setElementValue(_buildValueString(this.id, value));
221-
this._select.writeValue(this._select.value);
280+
this._select._writeValueAfterRender();
222281
}
223282

224283
/**
@@ -229,7 +288,7 @@ export class NgSelectOption implements OnDestroy {
229288
@Input('value')
230289
set value(value: any) {
231290
this._setElementValue(value);
232-
if (this._select) this._select.writeValue(this._select.value);
291+
if (this._select) this._select._writeValueAfterRender();
233292
}
234293

235294
/** @internal */
@@ -241,7 +300,7 @@ export class NgSelectOption implements OnDestroy {
241300
ngOnDestroy(): void {
242301
if (this._select) {
243302
this._select._optionMap.delete(this.id);
244-
this._select.writeValue(this._select.value);
303+
this._select._writeValueAfterRender();
245304
}
246305
}
247306
}

‎packages/forms/test/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@ ts_project(
66
testonly = True,
77
srcs = glob(["**/*.ts"]),
88
interop_deps = [
9+
"//packages/animations/browser",
910
"//packages/platform-browser",
11+
"//packages/platform-browser/animations",
1012
"//packages/platform-browser/testing",
1113
"//packages/private/testing",
1214
],

0 commit comments

Comments
 (0)
0