From 4636a987fa7148a479601815902fc3e46e7de15c Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 18 Sep 2018 23:19:49 +0200 Subject: [PATCH] fix(checkbox, slide-toggle): no margin if content is projected (#12973) * fix(checkbox, slide-toggle): no margin if content is projected Usually if the label of the checkbox or slide-toggle is empty, we remove the margin between label container and thumb/check because otherwise there would be too much spacing. Currently if developers use a component inside of the checkbox or slide-toggle in order to render the label, the margin is accidentally removed and the label looks misplaced. This is because the `cdkObserveContent` event runs outside of the `NgZone` and no change detection round _checks_ the checkbox or slide-toggle. Fixes #4720 * Add tests --- src/lib/checkbox/checkbox.spec.ts | 49 ++++++++++++++++++++++- src/lib/checkbox/checkbox.ts | 10 +++-- src/lib/slide-toggle/slide-toggle.spec.ts | 44 ++++++++++++++++++-- src/lib/slide-toggle/slide-toggle.ts | 10 +++-- 4 files changed, 99 insertions(+), 14 deletions(-) diff --git a/src/lib/checkbox/checkbox.spec.ts b/src/lib/checkbox/checkbox.spec.ts index 1ae96a96d582..a7fdb4ca1e80 100644 --- a/src/lib/checkbox/checkbox.spec.ts +++ b/src/lib/checkbox/checkbox.spec.ts @@ -19,10 +19,10 @@ import {MutationObserverFactory} from '@angular/cdk/observers'; describe('MatCheckbox', () => { let fixture: ComponentFixture; - function createComponent(componentType: Type): ComponentFixture { + function createComponent(componentType: Type, extraDeclarations: Type[] = []) { TestBed.configureTestingModule({ imports: [MatCheckboxModule, FormsModule, ReactiveFormsModule], - declarations: [componentType], + declarations: [componentType, ...extraDeclarations], }).compileComponents(); return TestBed.createComponent(componentType); @@ -1104,7 +1104,37 @@ describe('MatCheckbox', () => { fixture.detectChanges(); expect(checkboxInnerContainer.querySelector('input')!.hasAttribute('value')).toBe(false); }); + }); + + describe('label margin', () => { + it('should properly update margin if label content is projected', () => { + const mutationCallbacks: Function[] = []; + + TestBed.configureTestingModule({ + providers: [ + {provide: MutationObserverFactory, useValue: { + create: (callback: Function) => { + mutationCallbacks.push(callback); + return {observe: () => {}, disconnect: () => {}}; + } + }} + ] + }); + fixture = createComponent(CheckboxWithProjectedLabel, [TextBindingComponent]); + fixture.detectChanges(); + + const checkboxInnerContainer = fixture.debugElement + .query(By.css('.mat-checkbox-inner-container')).nativeElement; + + // Do not run the change detection for the fixture manually because we want to verify + // that the checkbox properly toggles the margin class even if the observe content output + // fires outside of the zone. + mutationCallbacks.forEach(callback => callback()); + + expect(checkboxInnerContainer.classList).not + .toContain('mat-checkbox-inner-container-no-side-margin'); + }); }); }); @@ -1238,3 +1268,18 @@ class CheckboxWithoutLabel { template: `` }) class CheckboxWithTabindexAttr {} + +/** Test component that uses another component for its label. */ +@Component({ + template: `` +}) +class CheckboxWithProjectedLabel {} + +/** Component that renders some text through a binding. */ +@Component({ + selector: 'some-text', + template: '{{text}}' +}) +class TextBindingComponent { + text: string = 'Some text'; +} diff --git a/src/lib/checkbox/checkbox.ts b/src/lib/checkbox/checkbox.ts index d36ee8599663..aa7f0676d0d9 100644 --- a/src/lib/checkbox/checkbox.ts +++ b/src/lib/checkbox/checkbox.ts @@ -282,10 +282,12 @@ export class MatCheckbox extends _MatCheckboxMixinBase implements ControlValueAc /** Method being called whenever the label text changes. */ _onLabelTextChange() { - // This method is getting called whenever the label of the checkbox changes. - // Since the checkbox uses the OnPush strategy we need to notify it about the change - // that has been recognized by the cdkObserveContent directive. - this._changeDetectorRef.markForCheck(); + // Since the event of the `cdkObserveContent` directive runs outside of the zone, the checkbox + // component will be only marked for check, but no actual change detection runs automatically. + // Instead of going back into the zone in order to trigger a change detection which causes + // *all* components to be checked (if explicitly marked or not using OnPush), we only trigger + // an explicit change detection for the checkbox view and it's children. + this._changeDetectorRef.detectChanges(); } // Implemented as part of ControlValueAccessor. diff --git a/src/lib/slide-toggle/slide-toggle.spec.ts b/src/lib/slide-toggle/slide-toggle.spec.ts index 24e06416a8cc..e39739bced06 100644 --- a/src/lib/slide-toggle/slide-toggle.spec.ts +++ b/src/lib/slide-toggle/slide-toggle.spec.ts @@ -29,7 +29,9 @@ describe('MatSlideToggle without forms', () => { declarations: [ SlideToggleBasic, SlideToggleWithTabindexAttr, - SlideToggleWithoutLabel + SlideToggleWithoutLabel, + SlideToggleProjectedLabel, + TextBindingComponent, ], providers: [ {provide: HAMMER_GESTURE_CONFIG, useFactory: () => gestureConfig = new TestGestureConfig()}, @@ -657,7 +659,6 @@ describe('MatSlideToggle without forms', () => { describe('without label', () => { let fixture: ComponentFixture; let testComponent: SlideToggleWithoutLabel; - let slideToggleElement: HTMLElement; let slideToggleBarElement: HTMLElement; beforeEach(() => { @@ -666,7 +667,6 @@ describe('MatSlideToggle without forms', () => { const slideToggleDebugEl = fixture.debugElement.query(By.directive(MatSlideToggle)); testComponent = fixture.componentInstance; - slideToggleElement = slideToggleDebugEl.nativeElement; slideToggleBarElement = slideToggleDebugEl .query(By.css('.mat-slide-toggle-bar')).nativeElement; }); @@ -697,10 +697,33 @@ describe('MatSlideToggle without forms', () => { flushMutationObserver(); fixture.detectChanges(); - expect(slideToggleElement.classList) + expect(slideToggleBarElement.classList) .not.toContain('mat-slide-toggle-bar-no-side-margin'); })); }); + + describe('label margin', () => { + let fixture: ComponentFixture; + let slideToggleBarElement: HTMLElement; + + beforeEach(() => { + fixture = TestBed.createComponent(SlideToggleProjectedLabel); + slideToggleBarElement = fixture.debugElement + .query(By.css('.mat-slide-toggle-bar')).nativeElement; + + fixture.detectChanges(); + }); + + it('should properly update margin if label content is projected', () => { + // Do not run the change detection for the fixture manually because we want to verify + // that the slide-toggle properly toggles the margin class even if the observe content + // output fires outside of the zone. + flushMutationObserver(); + + expect(slideToggleBarElement.classList).not + .toContain('mat-slide-toggle-bar-no-side-margin'); + }); + }); }); describe('MatSlideToggle with forms', () => { @@ -1087,3 +1110,16 @@ class SlideToggleWithModelAndChangeEvent { checked: boolean; onChange: () => void = () => {}; } + +@Component({ + template: `` +}) +class SlideToggleProjectedLabel {} + +@Component({ + selector: 'some-text', + template: `{{text}}` +}) +class TextBindingComponent { + text: string = 'Some text'; +} diff --git a/src/lib/slide-toggle/slide-toggle.ts b/src/lib/slide-toggle/slide-toggle.ts index 61dcb6cd2c5d..0bd9f23cc912 100644 --- a/src/lib/slide-toggle/slide-toggle.ts +++ b/src/lib/slide-toggle/slide-toggle.ts @@ -358,9 +358,11 @@ export class MatSlideToggle extends _MatSlideToggleMixinBase implements OnDestro /** Method being called whenever the label text changes. */ _onLabelTextChange() { - // This method is getting called whenever the label of the slide-toggle changes. - // Since the slide-toggle uses the OnPush strategy we need to notify it about the change - // that has been recognized by the cdkObserveContent directive. - this._changeDetectorRef.markForCheck(); + // Since the event of the `cdkObserveContent` directive runs outside of the zone, the + // slide-toggle component will be only marked for check, but no actual change detection runs + // automatically. Instead of going back into the zone in order to trigger a change detection + // which causes *all* components to be checked (if explicitly marked or not using OnPush), + // we only trigger an explicit change detection for the slide-toggle view and it's children. + this._changeDetectorRef.detectChanges(); } }