fix(checkbox): remove adapter.getNativeCb and move property hooks to component (#4073)

BREAKING CHANGE: The component is now responsible for calling MDCCheckboxFoundation#handleChange when the checked and indeterminate properties change.

Author: Matt Goo

packages/mdc-checkbox/adapter.js

@@ -62,9 +62,6 @@ class MDCCheckboxAdapter {
    */
   removeNativeControlAttr(attr) {}
 
-  /** @return {!MDCSelectionControlState} */
-  getNativeControl() {}
-
   forceLayout() {}
 
   /** @return {boolean} */

packages/mdc-checkbox/foundation.js

@@ -28,9 +28,6 @@ import MDCCheckboxAdapter from './adapter';
 /* eslint-enable no-unused-vars */
 import {cssClasses, strings, numbers} from './constants';
 
-/** @const {!Array<string>} */
-const CB_PROTO_PROPS = ['checked', 'indeterminate'];
-
 /**
  * @extends {MDCFoundation<!MDCCheckboxAdapter>}
  */
@@ -57,7 +54,6 @@ class MDCCheckboxFoundation extends MDCFoundation {
       removeClass: (/* className: string */) => {},
       setNativeControlAttr: (/* attr: string, value: string */) => {},
       removeNativeControlAttr: (/* attr: string */) => {},
-      getNativeControl: () => /* !MDCSelectionControlState */ {},
       forceLayout: () => {},
       isAttachedToDOM: () => /* boolean */ {},
       isIndeterminate: () => /* boolean */ {},
@@ -88,12 +84,10 @@ class MDCCheckboxFoundation extends MDCFoundation {
     this.currentCheckState_ = this.determineCheckState_();
     this.updateAriaChecked_();
     this.adapter_.addClass(cssClasses.UPGRADED);
-    this.installPropertyChangeHooks_();
   }
 
   /** @override */
   destroy() {
-    this.uninstallPropertyChangeHooks_();
     clearTimeout(this.animEndLatchTimer_);
   }
 
@@ -128,44 +122,6 @@ class MDCCheckboxFoundation extends MDCFoundation {
     this.transitionCheckState_();
   }
 
-  /** @private */
-  installPropertyChangeHooks_() {
-    const nativeCb = this.getNativeControl_();
-    const cbProto = Object.getPrototypeOf(nativeCb);
-
-    CB_PROTO_PROPS.forEach((controlState) => {
-      const desc = Object.getOwnPropertyDescriptor(cbProto, controlState);
-      // We have to check for this descriptor, since some browsers (Safari) don't support its return.
-      // See: https://bugs.webkit.org/show_bug.cgi?id=49739
-      if (validDescriptor(desc)) {
-        const nativeCbDesc = /** @type {!ObjectPropertyDescriptor} */ ({
-          get: desc.get,
-          set: (state) => {
-            desc.set.call(nativeCb, state);
-            this.transitionCheckState_();
-          },
-          configurable: desc.configurable,
-          enumerable: desc.enumerable,
-        });
-        Object.defineProperty(nativeCb, controlState, nativeCbDesc);
-      }
-    });
-  }
-
-  /** @private */
-  uninstallPropertyChangeHooks_() {
-    const nativeCb = this.getNativeControl_();
-    const cbProto = Object.getPrototypeOf(nativeCb);
-
-    CB_PROTO_PROPS.forEach((controlState) => {
-      const desc = /** @type {!ObjectPropertyDescriptor} */ (
-        Object.getOwnPropertyDescriptor(cbProto, controlState));
-      if (validDescriptor(desc)) {
-        Object.defineProperty(nativeCb, controlState, desc);
-      }
-    });
-  }
-
   /** @private */
   transitionCheckState_() {
     if (!this.adapter_.hasNativeControl()) {
@@ -265,27 +221,6 @@ class MDCCheckboxFoundation extends MDCFoundation {
       this.adapter_.removeNativeControlAttr(strings.ARIA_CHECKED_ATTR);
     }
   }
-
-  /**
-   * @return {!MDCSelectionControlState}
-   * @private
-   */
-  getNativeControl_() {
-    return this.adapter_.getNativeControl() || {
-      checked: false,
-      indeterminate: false,
-      disabled: false,
-      value: null,
-    };
-  }
-}
-
-/**
- * @param {ObjectPropertyDescriptor|undefined} inputPropDesc
- * @return {boolean}
- */
-function validDescriptor(inputPropDesc) {
-  return !!inputPropDesc && typeof inputPropDesc.set === 'function';
 }
 
 export default MDCCheckboxFoundation;

packages/mdc-checkbox/index.js

@@ -30,6 +30,9 @@ import MDCCheckboxFoundation from './foundation';
 import {MDCRipple, MDCRippleFoundation} from '@material/ripple/index';
 import {getMatchesProperty} from '@material/ripple/util';
 
+/** @const {!Array<string>} */
+const CB_PROTO_PROPS = ['checked', 'indeterminate'];
+
 /**
  * @extends MDCComponent<!MDCCheckboxFoundation>
  * @implements {MDCSelectionControl}
@@ -41,12 +44,12 @@ class MDCCheckbox extends MDCComponent {
 
   /**
    * Returns the state of the native control element, or null if the native control element is not present.
-   * @return {?MDCSelectionControlState}
+   * @return {!MDCSelectionControlState}
    * @private
    */
   get nativeCb_() {
     const {NATIVE_CONTROL_SELECTOR} = MDCCheckboxFoundation.strings;
-    const cbEl = /** @type {?MDCSelectionControlState} */ (
+    const cbEl = /** @type {!MDCSelectionControlState} */ (
       this.root_.querySelector(NATIVE_CONTROL_SELECTOR));
     return cbEl;
   }
@@ -67,6 +70,7 @@ class MDCCheckbox extends MDCComponent {
     this.handleAnimationEnd_= () => this.foundation_.handleAnimationEnd();
     this.nativeCb_.addEventListener('change', this.handleChange_);
     this.listen(getCorrectEventName(window, 'animationend'), this.handleAnimationEnd_);
+    this.installPropertyChangeHooks_();
   }
 
   /**
@@ -85,14 +89,51 @@ class MDCCheckbox extends MDCComponent {
     return new MDCRipple(this.root_, foundation);
   }
 
+  /** @private */
+  installPropertyChangeHooks_() {
+    const nativeCb = this.nativeCb_;
+    const cbProto = Object.getPrototypeOf(nativeCb);
+
+    CB_PROTO_PROPS.forEach((controlState) => {
+      const desc = Object.getOwnPropertyDescriptor(cbProto, controlState);
+      // We have to check for this descriptor, since some browsers (Safari) don't support its return.
+      // See: https://bugs.webkit.org/show_bug.cgi?id=49739
+      if (validDescriptor(desc)) {
+        const nativeCbDesc = /** @type {!ObjectPropertyDescriptor} */ ({
+          get: desc.get,
+          set: (state) => {
+            desc.set.call(nativeCb, state);
+            this.foundation_.handleChange();
+          },
+          configurable: desc.configurable,
+          enumerable: desc.enumerable,
+        });
+        Object.defineProperty(nativeCb, controlState, nativeCbDesc);
+      }
+    });
+  }
+
+  /** @private */
+  uninstallPropertyChangeHooks_() {
+    const nativeCb = this.nativeCb_;
+    const cbProto = Object.getPrototypeOf(nativeCb);
+
+    CB_PROTO_PROPS.forEach((controlState) => {
+      const desc = /** @type {!ObjectPropertyDescriptor} */ (
+        Object.getOwnPropertyDescriptor(cbProto, controlState));
+      if (validDescriptor(desc)) {
+        Object.defineProperty(nativeCb, controlState, desc);
+      }
+    });
+  }
+
   /** @return {!MDCCheckboxFoundation} */
   getDefaultFoundation() {
     return new MDCCheckboxFoundation({
       addClass: (className) => this.root_.classList.add(className),
       removeClass: (className) => this.root_.classList.remove(className),
       setNativeControlAttr: (attr, value) => this.nativeCb_.setAttribute(attr, value),
       removeNativeControlAttr: (attr) => this.nativeCb_.removeAttribute(attr),
-      getNativeControl: () => this.nativeCb_,
       isIndeterminate: () => this.indeterminate,
       isChecked: () => this.checked,
       hasNativeControl: () => !!this.nativeCb_,
@@ -151,8 +192,17 @@ class MDCCheckbox extends MDCComponent {
     this.ripple_.destroy();
     this.nativeCb_.removeEventListener('change', this.handleChange_);
     this.unlisten(getCorrectEventName(window, 'animationend'), this.handleAnimationEnd_);
+    this.uninstallPropertyChangeHooks_();
     super.destroy();
   }
 }
 
+/**
+ * @param {ObjectPropertyDescriptor|undefined} inputPropDesc
+ * @return {boolean}
+ */
+function validDescriptor(inputPropDesc) {
+  return !!inputPropDesc && typeof inputPropDesc.set === 'function';
+}
+
 export {MDCCheckboxFoundation, MDCCheckbox};

test/unit/mdc-checkbox/foundation.test.js

@@ -41,7 +41,6 @@ const DESC_UNDEFINED = {
 function setupTest() {
   const {foundation, mockAdapter} = setupFoundationTest(MDCCheckboxFoundation);
   const nativeControl = bel`<input type="checkbox">`;
-  td.when(mockAdapter.getNativeControl()).thenReturn(nativeControl);
   return {foundation, mockAdapter, nativeControl};
 }
 
@@ -80,11 +79,9 @@ function setupChangeHandlerTest() {
   foundation.init();
 
   const change = (newState) => {
-    td.when(mockAdapter.hasNativeControl()).thenReturn(!!newState);
-    if (newState) {
-      td.when(mockAdapter.isChecked()).thenReturn(newState.checked);
-      td.when(mockAdapter.isIndeterminate()).thenReturn(newState.indeterminate);
-    }
+    td.when(mockAdapter.hasNativeControl()).thenReturn(true);
+    td.when(mockAdapter.isChecked()).thenReturn(newState.checked);
+    td.when(mockAdapter.isIndeterminate()).thenReturn(newState.indeterminate);
     foundation.handleChange();
   };
 
@@ -116,7 +113,7 @@ test('exports numbers', () => {
 
 test('defaultAdapter returns a complete adapter implementation', () => {
   verifyDefaultAdapter(MDCCheckboxFoundation, [
-    'addClass', 'removeClass', 'setNativeControlAttr', 'removeNativeControlAttr', 'getNativeControl',
+    'addClass', 'removeClass', 'setNativeControlAttr', 'removeNativeControlAttr',
     'forceLayout', 'isAttachedToDOM', 'isIndeterminate', 'isChecked', 'hasNativeControl', 'setNativeControlDisabled',
   ]);
 });
@@ -136,12 +133,6 @@ test('#init adds aria-checked="mixed" if checkbox is initially indeterminate', (
   td.verify(mockAdapter.setNativeControlAttr('aria-checked', strings.ARIA_CHECKED_INDETERMINATE_VALUE));
 });
 
-test('#init handles case where getNativeControl() does not return anything', () => {
-  const {foundation, mockAdapter} = setupTest();
-  td.when(mockAdapter.getNativeControl()).thenReturn(undefined);
-  assert.doesNotThrow(() => foundation.init());
-});
-
 test('#init handles case when WebIDL attrs cannot be overridden (Safari)', () => {
   const {foundation, nativeControl} = setupTest();
   withMockCheckboxDescriptorReturning(DESC_UNDEFINED, () => {
@@ -159,14 +150,6 @@ test('#init handles case when property descriptors are not returned at all (Andr
   });
 });
 
-test('#destroy handles case where getNativeControl() does not return anything', () => {
-  const {foundation, mockAdapter} = setupTest();
-  foundation.init();
-
-  td.when(mockAdapter.getNativeControl()).thenReturn(undefined);
-  assert.doesNotThrow(() => foundation.destroy());
-});
-
 test('#destroy handles case when WebIDL attrs cannot be overridden (Safari)', () => {
   const {foundation} = setupTest();
   withMockCheckboxDescriptorReturning(DESC_UNDEFINED, () => {
@@ -328,46 +311,10 @@ test('change handler does not add animation classes for bogus changes (init -> u
   td.verify(mockAdapter.addClass(animClassArg), {times: 0});
 });
 
-test('change handler gracefully exits when getNativeControl() returns nothing', () => {
-  const {change} = setupChangeHandlerTest();
-  assert.doesNotThrow(() => change(undefined));
-});
-
-test('"checked" property change hook works correctly', () => {
-  const {foundation, mockAdapter, nativeControl} = setupTest();
-  td.when(mockAdapter.isAttachedToDOM()).thenReturn(true);
-  td.when(mockAdapter.hasNativeControl()).thenReturn(true);
-
-  withMockCheckboxDescriptorReturning({
-    get: () => {},
-    set: () => {},
-    enumerable: false,
-    configurable: true,
-  }, () => {
-    foundation.init();
-    td.when(mockAdapter.isChecked()).thenReturn(true);
-    td.when(mockAdapter.isIndeterminate()).thenReturn(false);
-    nativeControl.checked = !nativeControl.checked;
-    td.verify(mockAdapter.addClass(cssClasses.ANIM_UNCHECKED_CHECKED));
-  });
-});
-
-test('"indeterminate" property change hook works correctly', () => {
-  const {foundation, mockAdapter, nativeControl} = setupTest();
-  td.when(mockAdapter.isAttachedToDOM()).thenReturn(true);
-  td.when(mockAdapter.hasNativeControl()).thenReturn(true);
-
-  withMockCheckboxDescriptorReturning({
-    get: () => {},
-    set: () => {},
-    enumerable: false,
-    configurable: true,
-  }, () => {
-    foundation.init();
-    td.when(mockAdapter.isChecked()).thenReturn(false);
-    td.when(mockAdapter.isIndeterminate()).thenReturn(true);
-
-    nativeControl.indeterminate = !nativeControl.indeterminate;
-    td.verify(mockAdapter.addClass(cssClasses.ANIM_UNCHECKED_INDETERMINATE));
-  });
+test('change handler does not do anything if checkbox element is not found', () => {
+  const {foundation, mockAdapter} = setupTest();
+  td.when(mockAdapter.hasNativeControl()).thenReturn(false);
+  assert.doesNotThrow(() => foundation.handleChange());
+  td.verify(mockAdapter.setNativeControlAttr(td.matchers.isA(String), td.matchers.isA(String)), {times: 0});
+  td.verify(mockAdapter.removeNativeControlAttr(td.matchers.isA(String)), {times: 0});
 });