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

Author: devversion

src/lib/checkbox/checkbox.spec.ts

@@ -19,10 +19,10 @@ import {MutationObserverFactory} from '@angular/cdk/observers';
 describe('MatCheckbox', () => {
   let fixture: ComponentFixture<any>;
 
-  function createComponent<T>(componentType: Type<T>): ComponentFixture<T> {
+  function createComponent<T>(componentType: Type<T>, extraDeclarations: Type<any>[] = []) {
     TestBed.configureTestingModule({
       imports: [MatCheckboxModule, FormsModule, ReactiveFormsModule],
-      declarations: [componentType],
+      declarations: [componentType, ...extraDeclarations],
     }).compileComponents();
 
     return TestBed.createComponent<T>(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: `<mat-checkbox tabindex="5"></mat-checkbox>`
 })
 class CheckboxWithTabindexAttr {}
+
+/** Test component that uses another component for its label. */
+@Component({
+  template: `<mat-checkbox><some-text></some-text></mat-checkbox>`
+})
+class CheckboxWithProjectedLabel {}
+
+/** Component that renders some text through a binding. */
+@Component({
+  selector: 'some-text',
+  template: '<span>{{text}}</span>'
+})
+class TextBindingComponent {
+  text: string = 'Some text';
+}

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.

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<SlideToggleWithoutLabel>;
     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<SlideToggleProjectedLabel>;
+    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: `<mat-slide-toggle><some-text></some-text></mat-slide-toggle>`
+})
+class SlideToggleProjectedLabel {}
+
+@Component({
+  selector: 'some-text',
+  template: `<span>{{text}}</span>`
+})
+class TextBindingComponent {
+  text: string = 'Some text';
+}

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();
   }
 }