Skip to content

Commit a85418b

Browse files
devversionandrewseguin
authored andcommitted
refactor(mdc-prototypes): do not mark components dirty in input setters (#16129)
In order to make our component inputs which use boolean coercion more consistent with other inputs which aren't declared through setters, we should no longer call `markForCheck` within input setters as Angular automatically runs change detection if input values are updated. This breaks the programmatic usage of these inputs as Angular in that case won't be able to run change detection (since it can't detect that the value of the input changes).. though this is a more general problem with the `OnPush` strategy as we can't have consistent behavior for programmatic input updates without switching *every* input to a setter. Instead, in order to still support programmatic input updates, we should expose a method that allows developers to mark the MDC components as dirty. This is necessary so that developers which _really_ need to update programmatically (e.g. through `ViewChild`) can still update the componet to reflect the changes. The benefit is that we don't need to convert every input to a setter w/ markForCheck in order to make the OnPush inputs behavior consistent. Related to COMP-170
1 parent bd52ce0 commit a85418b

File tree

4 files changed

+38
-109
lines changed

4 files changed

+38
-109
lines changed

src/material-experimental/mdc-checkbox/checkbox.spec.ts

Lines changed: 28 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import {dispatchFakeEvent} from '@angular/cdk/testing';
2-
import {ChangeDetectionStrategy, Component, DebugElement, Type, ViewChild} from '@angular/core';
2+
import {ChangeDetectionStrategy, Component, DebugElement, Type} from '@angular/core';
33
import {
44
ComponentFixture,
55
fakeAsync,
@@ -68,6 +68,28 @@ describe('MatCheckbox', () => {
6868
expect(inputElement.checked).toBe(false);
6969
}));
7070

71+
72+
it('should toggle checkbox ripple disabledness correctly', fakeAsync(() => {
73+
const rippleSelector = '.mat-ripple-element:not(.mat-checkbox-persistent-ripple)';
74+
75+
testComponent.isDisabled = true;
76+
fixture.detectChanges();
77+
dispatchFakeEvent(labelElement, 'mousedown');
78+
dispatchFakeEvent(labelElement, 'mouseup');
79+
labelElement.click();
80+
expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(0);
81+
82+
flush();
83+
testComponent.isDisabled = false;
84+
fixture.detectChanges();
85+
dispatchFakeEvent(labelElement, 'mousedown');
86+
dispatchFakeEvent(labelElement, 'mouseup');
87+
labelElement.click();
88+
expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(1);
89+
90+
flush();
91+
}));
92+
7193
it('should add and remove indeterminate state', fakeAsync(() => {
7294
expect(inputElement.checked).toBe(false);
7395
expect(inputElement.indeterminate).toBe(false);
@@ -164,15 +186,6 @@ describe('MatCheckbox', () => {
164186
expect(testComponent.isIndeterminate).toBe(true);
165187
}));
166188

167-
it('should change native element checked when check programmatically', fakeAsync(() => {
168-
expect(inputElement.checked).toBe(false);
169-
170-
checkboxInstance.checked = true;
171-
fixture.detectChanges();
172-
173-
expect(inputElement.checked).toBe(true);
174-
}));
175-
176189
it('should toggle checked state on click', fakeAsync(() => {
177190
expect(checkboxInstance.checked).toBe(false);
178191

@@ -698,64 +711,6 @@ describe('MatCheckbox', () => {
698711
}));
699712
});
700713

701-
describe('using ViewChild', () => {
702-
let checkboxDebugElement: DebugElement;
703-
let checkboxNativeElement: HTMLElement;
704-
let testComponent: CheckboxUsingViewChild;
705-
706-
beforeEach(() => {
707-
fixture = createComponent(CheckboxUsingViewChild);
708-
fixture.detectChanges();
709-
710-
checkboxDebugElement = fixture.debugElement.query(By.directive(MatCheckbox));
711-
checkboxNativeElement = checkboxDebugElement.nativeElement;
712-
testComponent = fixture.debugElement.componentInstance;
713-
});
714-
715-
it('should toggle checkbox disabledness correctly', fakeAsync(() => {
716-
const checkboxInstance = checkboxDebugElement.componentInstance;
717-
const inputElement = <HTMLInputElement>checkboxNativeElement.querySelector('input');
718-
expect(checkboxInstance.disabled).toBe(false);
719-
expect(inputElement.tabIndex).toBe(0);
720-
expect(inputElement.disabled).toBe(false);
721-
722-
testComponent.isDisabled = true;
723-
fixture.detectChanges();
724-
725-
expect(checkboxInstance.disabled).toBe(true);
726-
expect(inputElement.disabled).toBe(true);
727-
728-
testComponent.isDisabled = false;
729-
fixture.detectChanges();
730-
731-
expect(checkboxInstance.disabled).toBe(false);
732-
expect(inputElement.tabIndex).toBe(0);
733-
expect(inputElement.disabled).toBe(false);
734-
}));
735-
736-
it('should toggle checkbox ripple disabledness correctly', fakeAsync(() => {
737-
const rippleSelector = '.mat-ripple-element:not(.mat-checkbox-persistent-ripple)';
738-
const labelElement = checkboxNativeElement.querySelector('label') as HTMLLabelElement;
739-
740-
testComponent.isDisabled = true;
741-
fixture.detectChanges();
742-
dispatchFakeEvent(labelElement, 'mousedown');
743-
dispatchFakeEvent(labelElement, 'mouseup');
744-
labelElement.click();
745-
expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(0);
746-
747-
flush();
748-
testComponent.isDisabled = false;
749-
fixture.detectChanges();
750-
dispatchFakeEvent(labelElement, 'mousedown');
751-
dispatchFakeEvent(labelElement, 'mouseup');
752-
labelElement.click();
753-
expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(1);
754-
755-
flush();
756-
}));
757-
});
758-
759714
describe('with multiple checkboxes', () => {
760715
beforeEach(() => {
761716
fixture = createComponent(MultipleCheckboxes);
@@ -855,11 +810,9 @@ describe('MatCheckbox', () => {
855810
// fire and not result in a changed after checked exception. Related: #12323
856811
inputElement.focus();
857812

858-
// Flush the two nested timeouts from the FocusMonitor that are being created on `focus`.
859-
flush();
860-
861-
checkboxInstance.disabled = true;
813+
fixture.componentInstance.isDisabled = true;
862814
fixture.detectChanges();
815+
863816
flushMicrotasks();
864817
}).not.toThrow();
865818
}));
@@ -1005,11 +958,13 @@ class SingleCheckbox {
1005958

1006959
/** Simple component for testing an MatCheckbox with required ngModel. */
1007960
@Component({
1008-
template: `<mat-checkbox [required]="isRequired" [(ngModel)]="isGood">Be good</mat-checkbox>`,
961+
template: `<mat-checkbox [required]="isRequired" [(ngModel)]="isGood"
962+
[disabled]="isDisabled">Be good</mat-checkbox>`,
1009963
})
1010964
class CheckboxWithNgModel {
1011965
isGood: boolean = false;
1012966
isRequired: boolean = true;
967+
isDisabled: boolean = false;
1013968
}
1014969

1015970
@Component({
@@ -1043,20 +998,6 @@ class CheckboxWithTabIndex {
1043998
isDisabled: boolean = false;
1044999
}
10451000

1046-
1047-
/** Simple test component that accesses MatCheckbox using ViewChild. */
1048-
@Component({
1049-
template: `
1050-
<mat-checkbox></mat-checkbox>`,
1051-
})
1052-
class CheckboxUsingViewChild {
1053-
@ViewChild(MatCheckbox, {static: false}) checkbox: MatCheckbox;
1054-
1055-
set isDisabled(value: boolean) {
1056-
this.checkbox.disabled = value;
1057-
}
1058-
}
1059-
10601001
/** Simple test component with an aria-label set. */
10611002
@Component({template: `<mat-checkbox aria-label="Super effective"></mat-checkbox>`})
10621003
class CheckboxWithAriaLabel {

src/material-experimental/mdc-checkbox/checkbox.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess
120120
}
121121
set checked(checked) {
122122
this._checked = coerceBooleanProperty(checked);
123-
this._changeDetectorRef.markForCheck();
124123
}
125124
private _checked = false;
126125

@@ -136,7 +135,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess
136135
}
137136
set indeterminate(indeterminate) {
138137
this._indeterminate = coerceBooleanProperty(indeterminate);
139-
this._changeDetectorRef.markForCheck();
140138
}
141139
private _indeterminate = false;
142140

@@ -147,7 +145,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess
147145
}
148146
set disabled(disabled) {
149147
this._disabled = coerceBooleanProperty(disabled);
150-
this._changeDetectorRef.markForCheck();
151148
}
152149
private _disabled = false;
153150

@@ -158,7 +155,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess
158155
}
159156
set required(required) {
160157
this._required = coerceBooleanProperty(required);
161-
this._changeDetectorRef.markForCheck();
162158
}
163159
private _required = false;
164160

@@ -284,6 +280,7 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess
284280
*/
285281
setDisabledState(isDisabled: boolean) {
286282
this.disabled = isDisabled;
283+
this._changeDetectorRef.markForCheck();
287284
}
288285

289286
/**

src/material-experimental/mdc-slide-toggle/slide-toggle.spec.ts

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,7 @@
11
import {BidiModule, Direction} from '@angular/cdk/bidi';
22
import {dispatchFakeEvent} from '@angular/cdk/testing';
33
import {Component} from '@angular/core';
4-
import {
5-
ComponentFixture,
6-
fakeAsync,
7-
flush,
8-
flushMicrotasks,
9-
TestBed,
10-
tick,
11-
} from '@angular/core/testing';
4+
import {ComponentFixture, fakeAsync, flushMicrotasks, TestBed, tick} from '@angular/core/testing';
125
import {FormControl, FormsModule, NgModel, ReactiveFormsModule} from '@angular/forms';
136
import {By} from '@angular/platform-browser';
147
import {MatSlideToggle, MatSlideToggleChange, MatSlideToggleModule} from './index';
@@ -481,11 +474,9 @@ describe('MatSlideToggle with forms', () => {
481474
// fire and not result in a changed after checked exception. Related: #12323
482475
inputElement.focus();
483476

484-
// Flush the two nested timeouts from the FocusMonitor that are being created on `focus`.
485-
flush();
486-
487-
slideToggle.disabled = true;
477+
fixture.componentInstance.isDisabled = true;
488478
fixture.detectChanges();
479+
489480
flushMicrotasks();
490481
}).not.toThrow();
491482
}));
@@ -496,7 +487,7 @@ describe('MatSlideToggle with forms', () => {
496487
// The control should start off with being untouched.
497488
expect(slideToggleModel.touched).toBe(false);
498489

499-
slideToggle.checked = true;
490+
testComponent.isChecked = true;
500491
fixture.detectChanges();
501492

502493
expect(slideToggleModel.touched).toBe(false);
@@ -736,10 +727,13 @@ class SlideToggleWithForm {
736727
}
737728

738729
@Component({
739-
template: `<mat-slide-toggle [(ngModel)]="modelValue"></mat-slide-toggle>`
730+
template: `<mat-slide-toggle [(ngModel)]="modelValue" [disabled]="isDisabled"
731+
[checked]="isChecked"></mat-slide-toggle>`
740732
})
741733
class SlideToggleWithModel {
742734
modelValue = false;
735+
isChecked = false;
736+
isDisabled = false;
743737
}
744738

745739
@Component({

src/material-experimental/mdc-slide-toggle/slide-toggle.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,6 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe
154154
if (this._foundation) {
155155
this._foundation.setChecked(this._checked);
156156
}
157-
158-
this._changeDetectorRef.markForCheck();
159157
}
160158

161159
/** Whether to disable the ripple on this checkbox. */
@@ -179,8 +177,6 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe
179177
if (this._foundation) {
180178
this._foundation.setDisabled(this._disabled);
181179
}
182-
183-
this._changeDetectorRef.markForCheck();
184180
}
185181
private _disabled = false;
186182

@@ -268,6 +264,7 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe
268264
/** Implemented as part of ControlValueAccessor. */
269265
writeValue(value: any): void {
270266
this.checked = !!value;
267+
this._changeDetectorRef.markForCheck();
271268
}
272269

273270
/** Implemented as part of ControlValueAccessor. */

0 commit comments

Comments
 (0)