fix(button-toggle): value cleared when replacing with list that still contains the selected value (#15508)

In #14627 we added some logic that removes the selected button toggle from the selection when it's destroyed. In an attempt to work around a "changed after checked" error we wrapped the call in a `Promise.resolve`, however that introduced an issue where we might end up overwriting a subsequent value, if it happens immediately after a button is destroyed. These changes move some things around to ensure that we don't overwrite the user's value.

Fixes #15297.

Author: crisbeto

src/material/button-toggle/button-toggle.spec.ts

@@ -206,6 +206,31 @@ describe('MatButtonToggle with forms', () => {
 
       expect(groupElement.querySelectorAll('.mat-ripple-element').length).toBe(0);
     });
+
+    it('should maintain the selected value when swapping out the list of toggles with one ' +
+      'that still contains the value', fakeAsync(() => {
+        expect(buttonToggleInstances[0].checked).toBe(false);
+        expect(fixture.componentInstance.modelValue).toBeFalsy();
+        expect(groupInstance.value).toBeFalsy();
+
+        groupInstance.value = 'red';
+        fixture.detectChanges();
+
+        expect(buttonToggleInstances[0].checked).toBe(true);
+        expect(groupInstance.value).toBe('red');
+
+        fixture.componentInstance.options = [...fixture.componentInstance.options];
+        fixture.detectChanges();
+        tick();
+        fixture.detectChanges();
+
+        buttonToggleDebugElements = fixture.debugElement.queryAll(By.directive(MatButtonToggle));
+        buttonToggleInstances = buttonToggleDebugElements.map(debugEl => debugEl.componentInstance);
+
+        expect(buttonToggleInstances[0].checked).toBe(true);
+        expect(groupInstance.value).toBe('red');
+      }));
+
   });
 });
 

src/material/button-toggle/button-toggle.ts

@@ -264,8 +264,12 @@ export class MatButtonToggleGroup implements ControlValueAccessor, OnInit, After
    * @param toggle Toggle to be synced.
    * @param select Whether the toggle should be selected.
    * @param isUserInput Whether the change was a result of a user interaction.
+   * @param deferEvents Whether to defer emitting the change events.
    */
-  _syncButtonToggle(toggle: MatButtonToggle, select: boolean, isUserInput = false) {
+  _syncButtonToggle(toggle: MatButtonToggle,
+                    select: boolean,
+                    isUserInput = false,
+                    deferEvents = false) {
     // Deselect the currently-selected toggle, if we're in single-selection
     // mode and the button being toggled isn't selected at the moment.
     if (!this.multiple && this.selected && !toggle.checked) {
@@ -278,14 +282,14 @@ export class MatButtonToggleGroup implements ControlValueAccessor, OnInit, After
       this._selectionModel.deselect(toggle);
     }
 
-    // Only emit the change event for user input.
-    if (isUserInput) {
-      this._emitChangeEvent();
+    // We need to defer in some cases in order to avoid "changed after checked errors", however
+    // the side-effect is that we may end up updating the model value out of sequence in others
+    // The `deferEvents` flag allows us to decide whether to do it on a case-by-case basis.
+    if (deferEvents) {
+      Promise.resolve(() => this._updateModelValue(isUserInput));
+    } else {
+      this._updateModelValue(isUserInput);
     }
-
-    // Note: we emit this one no matter whether it was a user interaction, because
-    // it is used by Angular to sync up the two-way data binding.
-    this.valueChange.emit(this.value);
   }
 
   /** Checks whether a button toggle is selected. */
@@ -344,6 +348,18 @@ export class MatButtonToggleGroup implements ControlValueAccessor, OnInit, After
       this._selectionModel.select(correspondingOption);
     }
   }
+
+  /** Syncs up the group's value with the model and emits the change event. */
+  private _updateModelValue(isUserInput: boolean) {
+    // Only emit the change event for user input.
+    if (isUserInput) {
+      this._emitChangeEvent();
+    }
+
+    // Note: we emit this one no matter whether it was a user interaction, because
+    // it is used by Angular to sync up the two-way data binding.
+    this.valueChange.emit(this.value);
+  }
 }
 
 // Boilerplate for applying mixins to the MatButtonToggle class.
@@ -498,7 +514,7 @@ export class MatButtonToggle extends _MatButtonToggleMixinBase implements OnInit
     // Remove the toggle from the selection once it's destroyed. Needs to happen
     // on the next tick in order to avoid "changed after checked" errors.
     if (group && group._isSelected(this)) {
-      Promise.resolve().then(() => group._syncButtonToggle(this, false));
+      group._syncButtonToggle(this, false, false, true);
     }
   }
 

tools/public_api_guard/material/button-toggle.d.ts

@@ -56,7 +56,7 @@ export declare class MatButtonToggleGroup implements ControlValueAccessor, OnIni
     _emitChangeEvent(): void;
     _isPrechecked(toggle: MatButtonToggle): boolean;
     _isSelected(toggle: MatButtonToggle): boolean;
-    _syncButtonToggle(toggle: MatButtonToggle, select: boolean, isUserInput?: boolean): void;
+    _syncButtonToggle(toggle: MatButtonToggle, select: boolean, isUserInput?: boolean, deferEvents?: boolean): void;
     ngAfterContentInit(): void;
     ngOnInit(): void;
     registerOnChange(fn: (value: any) => void): void;