Skip to content

Commit

Permalink
fix(checkbox): update MatCheckbox disabled setter to trigger change d…
Browse files Browse the repository at this point in the history
…etection (#11098)

This is related to #11056, which is a similar fix for
MatRadioButton.

The MatCheckbox component instance may be accessed directly by a parent component using
ViewChild/ContentChild. Since MatCheckbox uses OnPush change detection setting the disabled property
directly currently doesn't cause change detection leading to a stale or broken state. Accessing it
through a template binding does correctly trigger change detection on MatCheckbox (there are already
test cases for this method). We need to add a manual call to markForCheck, or else MatCheckbox will
never get kicked if disabled is set directly on the component instance.

Related open Angular bug: angular/angular#20611
  • Loading branch information
stevenyxu authored and jelbourn committed May 8, 2018
1 parent 33b5df4 commit ce1b517
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 4 deletions.
73 changes: 71 additions & 2 deletions src/lib/checkbox/checkbox.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {ComponentFixture, fakeAsync, TestBed, tick, flush} from '@angular/core/testing';
import {FormControl, FormsModule, NgModel, ReactiveFormsModule} from '@angular/forms';
import {Component, DebugElement} from '@angular/core';
import {Component, DebugElement, ViewChild} from '@angular/core';
import {By} from '@angular/platform-browser';
import {dispatchFakeEvent} from '@angular/cdk/testing';
import {MatCheckbox, MatCheckboxChange, MatCheckboxModule} from './index';
Expand Down Expand Up @@ -28,6 +28,7 @@ describe('MatCheckbox', () => {
CheckboxWithFormControl,
CheckboxWithoutLabel,
CheckboxWithTabindexAttr,
CheckboxUsingViewChild,
]
});

Expand Down Expand Up @@ -786,7 +787,6 @@ describe('MatCheckbox', () => {
});

describe('with native tabindex attribute', () => {

it('should properly detect native tabindex attribute', fakeAsync(() => {
fixture = TestBed.createComponent(CheckboxWithTabindexAttr);
fixture.detectChanges();
Expand All @@ -799,6 +799,61 @@ describe('MatCheckbox', () => {
}));
});

describe('using ViewChild', () => {
let checkboxDebugElement: DebugElement;
let checkboxNativeElement: HTMLElement;
let testComponent: CheckboxUsingViewChild;

beforeEach(() => {
fixture = TestBed.createComponent(CheckboxUsingViewChild);
fixture.detectChanges();

checkboxDebugElement = fixture.debugElement.query(By.directive(MatCheckbox));
checkboxNativeElement = checkboxDebugElement.nativeElement;
testComponent = fixture.debugElement.componentInstance;
});

it('should toggle checkbox disabledness correctly', () => {
const checkboxInstance = checkboxDebugElement.componentInstance;
const inputElement = <HTMLInputElement>checkboxNativeElement.querySelector('input');
expect(checkboxInstance.disabled).toBe(false);
expect(checkboxNativeElement.classList).not.toContain('mat-checkbox-disabled');
expect(inputElement.tabIndex).toBe(0);
expect(inputElement.disabled).toBe(false);

testComponent.isDisabled = true;
fixture.detectChanges();

expect(checkboxInstance.disabled).toBe(true);
expect(checkboxNativeElement.classList).toContain('mat-checkbox-disabled');
expect(inputElement.disabled).toBe(true);

testComponent.isDisabled = false;
fixture.detectChanges();

expect(checkboxInstance.disabled).toBe(false);
expect(checkboxNativeElement.classList).not.toContain('mat-checkbox-disabled');
expect(inputElement.tabIndex).toBe(0);
expect(inputElement.disabled).toBe(false);
});

it('should toggle checkbox ripple disabledness correctly', () => {
const labelElement = checkboxNativeElement.querySelector('label') as HTMLLabelElement;

testComponent.isDisabled = true;
fixture.detectChanges();
dispatchFakeEvent(labelElement, 'mousedown');
dispatchFakeEvent(labelElement, 'mouseup');
expect(checkboxNativeElement.querySelectorAll('.mat-ripple-element').length).toBe(0);

testComponent.isDisabled = false;
fixture.detectChanges();
dispatchFakeEvent(labelElement, 'mousedown');
dispatchFakeEvent(labelElement, 'mouseup');
expect(checkboxNativeElement.querySelectorAll('.mat-ripple-element').length).toBe(1);
});
});

describe('with multiple checkboxes', () => {
beforeEach(() => {
fixture = TestBed.createComponent(MultipleCheckboxes);
Expand Down Expand Up @@ -1116,6 +1171,20 @@ class CheckboxWithTabIndex {
isDisabled: boolean = false;
}


/** Simple test component that accesses MatCheckbox using ViewChild. */
@Component({
template: `
<mat-checkbox></mat-checkbox>`,
})
class CheckboxUsingViewChild {
@ViewChild(MatCheckbox) checkbox;

set isDisabled(value: boolean) {
this.checkbox.disabled = value;
}
}

/** Simple test component with an aria-label set. */
@Component({
template: `<mat-checkbox aria-label="Super effective"></mat-checkbox>`
Expand Down
17 changes: 15 additions & 2 deletions src/lib/checkbox/checkbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export const _MatCheckboxMixinBase =
'[class.mat-checkbox-label-before]': 'labelPosition == "before"',
},
providers: [MAT_CHECKBOX_CONTROL_VALUE_ACCESSOR],
inputs: ['disabled', 'disableRipple', 'color', 'tabIndex'],
inputs: ['disableRipple', 'color', 'tabIndex'],
encapsulation: ViewEncapsulation.None,
changeDetection: ChangeDetectionStrategy.OnPush
})
Expand Down Expand Up @@ -213,6 +213,20 @@ export class MatCheckbox extends _MatCheckboxMixinBase implements ControlValueAc
}
private _checked: boolean = false;

/**
* Whether the checkbox is disabled. This fully overrides the implementation provided by
* mixinDisabled, but the mixin is still required because mixinTabIndex requires it.
*/
@Input()
get disabled() { return this._disabled; }
set disabled(value: any) {
if (value != this.disabled) {
this._disabled = value;
this._changeDetectorRef.markForCheck();
}
}
private _disabled: boolean = false;

/**
* Whether the checkbox is indeterminate. This is also known as "mixed" mode and can be used to
* represent a checkbox with three states, e.g. a checkbox that represents a nested list of
Expand Down Expand Up @@ -267,7 +281,6 @@ export class MatCheckbox extends _MatCheckboxMixinBase implements ControlValueAc
// Implemented as part of ControlValueAccessor.
setDisabledState(isDisabled: boolean) {
this.disabled = isDisabled;
this._changeDetectorRef.markForCheck();
}

_getAriaChecked(): 'true' | 'false' | 'mixed' {
Expand Down

0 comments on commit ce1b517

Please sign in to comment.