From 19af6ec2a84cfc775e7034182375ec92122b9fcc Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 19 Jun 2019 19:28:04 +0200 Subject: [PATCH] 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 --- .../mdc-checkbox/checkbox.spec.ts | 115 +++++------------- .../mdc-checkbox/checkbox.ts | 5 +- .../mdc-slide-toggle/slide-toggle.spec.ts | 22 ++-- .../mdc-slide-toggle/slide-toggle.ts | 5 +- 4 files changed, 38 insertions(+), 109 deletions(-) diff --git a/src/material-experimental/mdc-checkbox/checkbox.spec.ts b/src/material-experimental/mdc-checkbox/checkbox.spec.ts index 88c1ca33daf..896b0cb66d4 100644 --- a/src/material-experimental/mdc-checkbox/checkbox.spec.ts +++ b/src/material-experimental/mdc-checkbox/checkbox.spec.ts @@ -1,5 +1,5 @@ import {dispatchFakeEvent} from '@angular/cdk/testing'; -import {ChangeDetectionStrategy, Component, DebugElement, Type, ViewChild} from '@angular/core'; +import {ChangeDetectionStrategy, Component, DebugElement, Type} from '@angular/core'; import { ComponentFixture, fakeAsync, @@ -68,6 +68,28 @@ describe('MatCheckbox', () => { expect(inputElement.checked).toBe(false); })); + + it('should toggle checkbox ripple disabledness correctly', fakeAsync(() => { + const rippleSelector = '.mat-ripple-element:not(.mat-checkbox-persistent-ripple)'; + + testComponent.isDisabled = true; + fixture.detectChanges(); + dispatchFakeEvent(labelElement, 'mousedown'); + dispatchFakeEvent(labelElement, 'mouseup'); + labelElement.click(); + expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(0); + + flush(); + testComponent.isDisabled = false; + fixture.detectChanges(); + dispatchFakeEvent(labelElement, 'mousedown'); + dispatchFakeEvent(labelElement, 'mouseup'); + labelElement.click(); + expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(1); + + flush(); + })); + it('should add and remove indeterminate state', fakeAsync(() => { expect(inputElement.checked).toBe(false); expect(inputElement.indeterminate).toBe(false); @@ -164,15 +186,6 @@ describe('MatCheckbox', () => { expect(testComponent.isIndeterminate).toBe(true); })); - it('should change native element checked when check programmatically', fakeAsync(() => { - expect(inputElement.checked).toBe(false); - - checkboxInstance.checked = true; - fixture.detectChanges(); - - expect(inputElement.checked).toBe(true); - })); - it('should toggle checked state on click', fakeAsync(() => { expect(checkboxInstance.checked).toBe(false); @@ -698,64 +711,6 @@ describe('MatCheckbox', () => { })); }); - describe('using ViewChild', () => { - let checkboxDebugElement: DebugElement; - let checkboxNativeElement: HTMLElement; - let testComponent: CheckboxUsingViewChild; - - beforeEach(() => { - fixture = createComponent(CheckboxUsingViewChild); - fixture.detectChanges(); - - checkboxDebugElement = fixture.debugElement.query(By.directive(MatCheckbox)); - checkboxNativeElement = checkboxDebugElement.nativeElement; - testComponent = fixture.debugElement.componentInstance; - }); - - it('should toggle checkbox disabledness correctly', fakeAsync(() => { - const checkboxInstance = checkboxDebugElement.componentInstance; - const inputElement = checkboxNativeElement.querySelector('input'); - expect(checkboxInstance.disabled).toBe(false); - expect(inputElement.tabIndex).toBe(0); - expect(inputElement.disabled).toBe(false); - - testComponent.isDisabled = true; - fixture.detectChanges(); - - expect(checkboxInstance.disabled).toBe(true); - expect(inputElement.disabled).toBe(true); - - testComponent.isDisabled = false; - fixture.detectChanges(); - - expect(checkboxInstance.disabled).toBe(false); - expect(inputElement.tabIndex).toBe(0); - expect(inputElement.disabled).toBe(false); - })); - - it('should toggle checkbox ripple disabledness correctly', fakeAsync(() => { - const rippleSelector = '.mat-ripple-element:not(.mat-checkbox-persistent-ripple)'; - const labelElement = checkboxNativeElement.querySelector('label') as HTMLLabelElement; - - testComponent.isDisabled = true; - fixture.detectChanges(); - dispatchFakeEvent(labelElement, 'mousedown'); - dispatchFakeEvent(labelElement, 'mouseup'); - labelElement.click(); - expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(0); - - flush(); - testComponent.isDisabled = false; - fixture.detectChanges(); - dispatchFakeEvent(labelElement, 'mousedown'); - dispatchFakeEvent(labelElement, 'mouseup'); - labelElement.click(); - expect(checkboxNativeElement.querySelectorAll(rippleSelector).length).toBe(1); - - flush(); - })); - }); - describe('with multiple checkboxes', () => { beforeEach(() => { fixture = createComponent(MultipleCheckboxes); @@ -855,11 +810,9 @@ describe('MatCheckbox', () => { // fire and not result in a changed after checked exception. Related: #12323 inputElement.focus(); - // Flush the two nested timeouts from the FocusMonitor that are being created on `focus`. - flush(); - - checkboxInstance.disabled = true; + fixture.componentInstance.isDisabled = true; fixture.detectChanges(); + flushMicrotasks(); }).not.toThrow(); })); @@ -1005,11 +958,13 @@ class SingleCheckbox { /** Simple component for testing an MatCheckbox with required ngModel. */ @Component({ - template: `Be good`, + template: `Be good`, }) class CheckboxWithNgModel { isGood: boolean = false; isRequired: boolean = true; + isDisabled: boolean = false; } @Component({ @@ -1043,20 +998,6 @@ class CheckboxWithTabIndex { isDisabled: boolean = false; } - -/** Simple test component that accesses MatCheckbox using ViewChild. */ -@Component({ - template: ` - `, -}) -class CheckboxUsingViewChild { - @ViewChild(MatCheckbox, {static: false}) checkbox: MatCheckbox; - - set isDisabled(value: boolean) { - this.checkbox.disabled = value; - } -} - /** Simple test component with an aria-label set. */ @Component({template: ``}) class CheckboxWithAriaLabel { diff --git a/src/material-experimental/mdc-checkbox/checkbox.ts b/src/material-experimental/mdc-checkbox/checkbox.ts index 8301db48cbf..eaf91492e29 100644 --- a/src/material-experimental/mdc-checkbox/checkbox.ts +++ b/src/material-experimental/mdc-checkbox/checkbox.ts @@ -120,7 +120,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess } set checked(checked) { this._checked = coerceBooleanProperty(checked); - this._changeDetectorRef.markForCheck(); } private _checked = false; @@ -136,7 +135,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess } set indeterminate(indeterminate) { this._indeterminate = coerceBooleanProperty(indeterminate); - this._changeDetectorRef.markForCheck(); } private _indeterminate = false; @@ -147,7 +145,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess } set disabled(disabled) { this._disabled = coerceBooleanProperty(disabled); - this._changeDetectorRef.markForCheck(); } private _disabled = false; @@ -158,7 +155,6 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess } set required(required) { this._required = coerceBooleanProperty(required); - this._changeDetectorRef.markForCheck(); } private _required = false; @@ -284,6 +280,7 @@ export class MatCheckbox implements AfterViewInit, OnDestroy, ControlValueAccess */ setDisabledState(isDisabled: boolean) { this.disabled = isDisabled; + this._changeDetectorRef.markForCheck(); } /** diff --git a/src/material-experimental/mdc-slide-toggle/slide-toggle.spec.ts b/src/material-experimental/mdc-slide-toggle/slide-toggle.spec.ts index 8dfa7bc6662..b808438eae4 100644 --- a/src/material-experimental/mdc-slide-toggle/slide-toggle.spec.ts +++ b/src/material-experimental/mdc-slide-toggle/slide-toggle.spec.ts @@ -1,14 +1,7 @@ import {BidiModule, Direction} from '@angular/cdk/bidi'; import {dispatchFakeEvent} from '@angular/cdk/testing'; import {Component} from '@angular/core'; -import { - ComponentFixture, - fakeAsync, - flush, - flushMicrotasks, - TestBed, - tick, -} from '@angular/core/testing'; +import {ComponentFixture, fakeAsync, flushMicrotasks, TestBed, tick} from '@angular/core/testing'; import {FormControl, FormsModule, NgModel, ReactiveFormsModule} from '@angular/forms'; import {By} from '@angular/platform-browser'; import {MatSlideToggle, MatSlideToggleChange, MatSlideToggleModule} from './index'; @@ -481,11 +474,9 @@ describe('MatSlideToggle with forms', () => { // fire and not result in a changed after checked exception. Related: #12323 inputElement.focus(); - // Flush the two nested timeouts from the FocusMonitor that are being created on `focus`. - flush(); - - slideToggle.disabled = true; + fixture.componentInstance.isDisabled = true; fixture.detectChanges(); + flushMicrotasks(); }).not.toThrow(); })); @@ -496,7 +487,7 @@ describe('MatSlideToggle with forms', () => { // The control should start off with being untouched. expect(slideToggleModel.touched).toBe(false); - slideToggle.checked = true; + testComponent.isChecked = true; fixture.detectChanges(); expect(slideToggleModel.touched).toBe(false); @@ -736,10 +727,13 @@ class SlideToggleWithForm { } @Component({ - template: `` + template: `` }) class SlideToggleWithModel { modelValue = false; + isChecked = false; + isDisabled = false; } @Component({ diff --git a/src/material-experimental/mdc-slide-toggle/slide-toggle.ts b/src/material-experimental/mdc-slide-toggle/slide-toggle.ts index df7a5ec2731..5653007818f 100644 --- a/src/material-experimental/mdc-slide-toggle/slide-toggle.ts +++ b/src/material-experimental/mdc-slide-toggle/slide-toggle.ts @@ -154,8 +154,6 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe if (this._foundation) { this._foundation.setChecked(this._checked); } - - this._changeDetectorRef.markForCheck(); } /** Whether to disable the ripple on this checkbox. */ @@ -179,8 +177,6 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe if (this._foundation) { this._foundation.setDisabled(this._disabled); } - - this._changeDetectorRef.markForCheck(); } private _disabled = false; @@ -268,6 +264,7 @@ export class MatSlideToggle implements ControlValueAccessor, AfterViewInit, OnDe /** Implemented as part of ControlValueAccessor. */ writeValue(value: any): void { this.checked = !!value; + this._changeDetectorRef.markForCheck(); } /** Implemented as part of ControlValueAccessor. */