From 8d9bbbf008de83f4988d25aedaf1e153e41ad60f Mon Sep 17 00:00:00 2001
From: Paul Gschwendtner <paulgschwendtner@gmail.com>
Date: Thu, 8 Jun 2017 23:32:35 +0200
Subject: [PATCH] fix(checkbox): margin for empty checkboxes incorrectly added
 (#4730)

* fix(checkbox): margin for empty checkboxes incorrectly added

With #2121 the margin will be removed for checkboxes that don't have any label set.

A problem is that the Checkbox uses the OnPush change detection strategy and therefore the checkbox is not able to detect any delayed / async label change.

This means that checkboxes that set their label from an async binding will not have any margin until the users clicks on the checkbox.

Using the `cdkObserveContent` seems to be an elegant approach when using the OnPush strategy.

The `:empty` CSS selector would be more elegant but it's very sensitive about whitespaces and therefore it doesn't work properly.

Fixes #4720

* Updates
---
 src/lib/checkbox/checkbox.html    |  4 +-
 src/lib/checkbox/checkbox.spec.ts | 62 +++++++++++++++++++++++++------
 src/lib/checkbox/checkbox.ts      | 16 ++++----
 src/lib/checkbox/index.ts         |  4 +-
 4 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/src/lib/checkbox/checkbox.html b/src/lib/checkbox/checkbox.html
index e07b211a679..78090d6208b 100644
--- a/src/lib/checkbox/checkbox.html
+++ b/src/lib/checkbox/checkbox.html
@@ -1,6 +1,6 @@
 <label [attr.for]="inputId" class="mat-checkbox-layout" #label>
   <div class="mat-checkbox-inner-container"
-       [class.mat-checkbox-inner-container-no-side-margin]="!_hasLabel()">
+       [class.mat-checkbox-inner-container-no-side-margin]="!checkboxLabel.textContent.trim()">
     <input #input
            class="mat-checkbox-input cdk-visually-hidden" type="checkbox"
            [id]="inputId"
@@ -35,7 +35,7 @@
       <div class="mat-checkbox-mixedmark"></div>
     </div>
   </div>
-  <span class="mat-checkbox-label" #labelWrapper>
+  <span class="mat-checkbox-label" #checkboxLabel (cdkObserveContent)="_onLabelTextChange()">
     <!-- Add an invisible span so JAWS can read the label -->
     <span style="display:none">&nbsp;</span>
     <ng-content></ng-content>
diff --git a/src/lib/checkbox/checkbox.spec.ts b/src/lib/checkbox/checkbox.spec.ts
index 3cfd978c364..af53a375e49 100644
--- a/src/lib/checkbox/checkbox.spec.ts
+++ b/src/lib/checkbox/checkbox.spec.ts
@@ -765,19 +765,57 @@ describe('MdCheckbox', () => {
   });
 
   describe('without label', () => {
-    let checkboxDebugElement: DebugElement;
-    let checkboxNativeElement: HTMLElement;
+    let testComponent: CheckboxWithoutLabel;
+    let checkboxElement: HTMLElement;
+    let checkboxInnerContainer: HTMLElement;
 
-    it('should add a css class to inner-container to remove side margin', () => {
+    beforeEach(() => {
       fixture = TestBed.createComponent(CheckboxWithoutLabel);
-      fixture.detectChanges();
-      checkboxDebugElement = fixture.debugElement.query(By.directive(MdCheckbox));
-      checkboxNativeElement = checkboxDebugElement.nativeElement;
 
-      let checkboxInnerContainerWithoutMarginCount = checkboxNativeElement
-          .querySelectorAll('.mat-checkbox-inner-container-no-side-margin').length;
-      expect(checkboxInnerContainerWithoutMarginCount).toBe(1);
+      const checkboxDebugEl = fixture.debugElement.query(By.directive(MdCheckbox));
+
+      testComponent = fixture.componentInstance;
+      checkboxElement = checkboxDebugEl.nativeElement;
+      checkboxInnerContainer = checkboxDebugEl
+        .query(By.css('.mat-checkbox-inner-container')).nativeElement;
     });
+
+    it('should remove margin for checkbox without a label', () => {
+      fixture.detectChanges();
+
+      expect(checkboxInnerContainer.classList)
+        .toContain('mat-checkbox-inner-container-no-side-margin');
+    });
+
+    it('should not remove margin if initial label is set through binding', async(() => {
+      testComponent.label = 'Some content';
+      fixture.detectChanges();
+
+      expect(checkboxInnerContainer.classList)
+        .not.toContain('mat-checkbox-inner-container-no-side-margin');
+    }));
+
+    it('should re-add margin if label is added asynchronously', async(() => {
+      fixture.detectChanges();
+
+      expect(checkboxInnerContainer.classList)
+        .toContain('mat-checkbox-inner-container-no-side-margin');
+
+      testComponent.label = 'Some content';
+      fixture.detectChanges();
+
+      // Wait for the MutationObserver to detect the content change and for the cdkObserveContent
+      // to emit the change event to the checkbox.
+      setTimeout(() => {
+        // The MutationObserver from the cdkObserveContent directive detected the content change
+        // and notified the checkbox component. The checkbox then marks the component as dirty
+        // by calling `markForCheck()`. This needs to be reflected by the component template then.
+        fixture.detectChanges();
+
+        expect(checkboxInnerContainer.classList)
+          .not.toContain('mat-checkbox-inner-container-no-side-margin');
+      }, 1);
+    }));
   });
 });
 
@@ -888,6 +926,8 @@ class CheckboxWithFormControl {
 
 /** Test component without label */
 @Component({
-  template: `<md-checkbox></md-checkbox>`
+  template: `<md-checkbox>{{ label }}</md-checkbox>`
 })
-class CheckboxWithoutLabel {}
+class CheckboxWithoutLabel {
+  label: string;
+}
diff --git a/src/lib/checkbox/checkbox.ts b/src/lib/checkbox/checkbox.ts
index 279d6efc8fe..a4c764634ce 100644
--- a/src/lib/checkbox/checkbox.ts
+++ b/src/lib/checkbox/checkbox.ts
@@ -161,14 +161,6 @@ export class MdCheckbox extends _MdCheckboxMixinBase
   /** The native `<input type="checkbox"> element */
   @ViewChild('input') _inputElement: ElementRef;
 
-  @ViewChild('labelWrapper') _labelWrapper: ElementRef;
-
-  /** Whether the checkbox has label */
-  _hasLabel(): boolean {
-    const labelText = this._labelWrapper.nativeElement.textContent || '';
-    return !!labelText.trim().length;
-  }
-
   /** Called when the checkbox is blurred. Needed to properly implement ControlValueAccessor. */
   @ViewChild(MdRipple) _ripple: MdRipple;
 
@@ -251,6 +243,14 @@ export class MdCheckbox extends _MdCheckboxMixinBase
     return this.disableRipple || this.disabled;
   }
 
+  /** 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();
+  }
+
   /**
    * Sets the model value. Implemented as part of ControlValueAccessor.
    * @param value Value to be set to the model.
diff --git a/src/lib/checkbox/index.ts b/src/lib/checkbox/index.ts
index 1987bac2863..cfcce8f5d69 100644
--- a/src/lib/checkbox/index.ts
+++ b/src/lib/checkbox/index.ts
@@ -1,11 +1,11 @@
 import {NgModule} from '@angular/core';
 import {CommonModule} from '@angular/common';
-import {MdRippleModule, MdCommonModule, FocusOriginMonitor} from '../core';
+import {MdRippleModule, MdCommonModule, FocusOriginMonitor, ObserveContentModule} from '../core';
 import {MdCheckbox} from './checkbox';
 
 
 @NgModule({
-  imports: [CommonModule, MdRippleModule, MdCommonModule],
+  imports: [CommonModule, MdRippleModule, MdCommonModule, ObserveContentModule],
   exports: [MdCheckbox, MdCommonModule],
   declarations: [MdCheckbox],
   providers: [FocusOriginMonitor]