feat(choice-input): api normalization and cleanup

packages/choice-input/package.json

@@ -38,6 +38,7 @@
   "devDependencies": {
     "@lion/input": "^0.1.22",
     "@open-wc/demoing-storybook": "^0.2.0",
-    "@open-wc/testing": "^0.12.5"
+    "@open-wc/testing": "^0.12.5",
+    "sinon": "^7.2.2"
   }
 }

packages/choice-input/src/ChoiceInputMixin.js

@@ -1,70 +1,94 @@
 /* eslint-disable class-methods-use-this */
 
 import { html, css, nothing } from '@lion/core';
-import { ObserverMixin } from '@lion/core/src/ObserverMixin.js';
 import { FormatMixin } from '@lion/field';
 
 export const ChoiceInputMixin = superclass =>
   // eslint-disable-next-line
-  class ChoiceInputMixin extends FormatMixin(ObserverMixin(superclass)) {
+  class ChoiceInputMixin extends FormatMixin(superclass) {
-    get delegations() {
+    static get properties() {
       return {
-        ...super.delegations,
+        ...super.properties,
-        target: () => this.inputElement,
+        /**
-        properties: [...super.delegations.properties, 'checked'],
+         * Boolean indicating whether or not this element is checked by the end user.
-        attributes: [...super.delegations.attributes, 'checked'],
+         */
+        checked: {
+          type: Boolean,
+          reflect: true,
+        },
+        /**
+         * Whereas 'normal' `.modelValue`s usually store a complex/typed version
+         * of a view value, choice inputs have a slightly different approach.
+         * In order to remain their Single Source of Truth characteristic, choice inputs
+         * store both the value and 'checkedness', in the format { value: 'x', checked: true }
+         * Different from the platform, this also allows to serialize the 'non checkedness',
+         * allowing to restore form state easily and inform the server about unchecked options.
+         */
+        modelValue: {
+          type: Object,
+          hasChanged: (nw, old = {}) => nw.value !== old.value || nw.checked !== old.checked,
+        },
+        /**
+         * The value property of the modelValue. It provides an easy inteface for storing
+         * (complex) values in the modelValue
+         */
+        choiceValue: {
+          type: Object,
+        },
       };
     }
 
-    static get syncObservers() {
-      return {
-        ...super.syncObservers,
-        _syncModelValueToChecked: ['modelValue'],
-      };
-    }
-
-    static get asyncObservers() {
-      return {
-        ...super.asyncObservers,
-        _reflectCheckedToCssClass: ['modelValue'],
-      };
-    }
-
-    get choiceChecked() {
-      return this.modelValue.checked;
-    }
-
-    set choiceChecked(checked) {
-      if (this.modelValue.checked !== checked) {
-        this.modelValue = { value: this.modelValue.value, checked };
-      }
-    }
-
     get choiceValue() {
       return this.modelValue.value;
     }
 
     set choiceValue(value) {
+      this.requestUpdate('choiceValue', this.choiceValue);
       if (this.modelValue.value !== value) {
         this.modelValue = { value, checked: this.modelValue.checked };
       }
     }
 
+    _requestUpdate(name, oldValue) {
+      super._requestUpdate(name, oldValue);
+
+      if (name === 'modelValue') {
+        if (this.modelValue.checked !== this.checked) {
+          this.__syncModelCheckedToChecked(this.modelValue.checked);
+        }
+      } else if (name === 'checked') {
+        if (this.modelValue.checked !== this.checked) {
+          this.__syncCheckedToModel(this.checked);
+        }
+      }
+    }
+
+    firstUpdated(c) {
+      super.firstUpdated(c);
+      if (c.has('checked')) {
+        // Here we set the initial value for our [slot=input] content,
+        // which has been set by our SlotMixin
+        this.__syncCheckedToInputElement();
+      }
+    }
+
+    updated(c) {
+      super.updated(c);
+      if (c.has('modelValue')) {
+        this._reflectCheckedToCssClass({ modelValue: this.modelValue });
+        this.__syncCheckedToInputElement();
+      }
+    }
+
     constructor() {
       super();
       this.modelValue = { value: '', checked: false };
     }
 
     /**
-     * @override
+     * Styles for [input=radio] and [input=checkbox] wrappers.
-     * Override InteractionStateMixin
+     * For [role=option] extensions, please override completely
-     * 'prefilled' should be false when modelValue is { checked: false }, which would return
-     * true in original method (since non-empty objects are considered prefilled by default).
      */
-    static _isPrefilled(modelValue) {
-      return modelValue.checked;
-    }
-
     static get styles() {
       return [
         css`
@@ -79,6 +103,10 @@ export const ChoiceInputMixin = superclass =>
       ];
     }
 
+    /**
+     * Template for [input=radio] and [input=checkbox] wrappers.
+     * For [role=option] extensions, please override completely
+     */
     render() {
       return html`
         <slot name="input"></slot>
@@ -96,22 +124,44 @@ export const ChoiceInputMixin = superclass =>
     }
 
     connectedCallback() {
-      if (super.connectedCallback) super.connectedCallback();
+      super.connectedCallback();
-      this.addEventListener('user-input-changed', this._toggleChecked);
+      this.addEventListener('user-input-changed', this.__toggleChecked);
       this._reflectCheckedToCssClass();
     }
 
     disconnectedCallback() {
-      if (super.disconnectedCallback) super.disconnectedCallback();
+      super.disconnectedCallback();
-      this.removeEventListener('user-input-changed', this._toggleChecked);
+      this.removeEventListener('user-input-changed', this.__toggleChecked);
     }
 
-    _toggleChecked() {
+    __toggleChecked() {
-      this.choiceChecked = !this.choiceChecked;
+      this.checked = !this.checked;
     }
 
-    _syncModelValueToChecked({ modelValue }) {
+    __syncModelCheckedToChecked(checked) {
-      this.checked = !!modelValue.checked;
+      this.checked = checked;
+    }
+
+    __syncCheckedToModel(checked) {
+      this.modelValue = { value: this.choiceValue, checked };
+    }
+
+    __syncCheckedToInputElement() {
+      // .inputElement might not be available yet(slot content)
+      // or at all (no reliance on platform construct, in case of [role=option])
+      if (this.inputElement) {
+        this.inputElement.checked = this.checked;
+      }
+    }
+
+    /**
+     * @override
+     * Override InteractionStateMixin
+     * 'prefilled' should be false when modelValue is { checked: false }, which would return
+     * true in original method (since non-empty objects are considered prefilled by default).
+     */
+    static _isPrefilled(modelValue) {
+      return modelValue.checked;
     }
 
     /**
@@ -126,30 +176,13 @@ export const ChoiceInputMixin = superclass =>
 
     /**
      * @override
-     * Override FormatMixin default dispatching of model-value-changed as it only does a simple
+     * hasChanged is designed for async (updated) callback, also check for sync
-     * comparision which is not enough in js because
+     * (_requestUpdate) callback
-     * { value: 'foo', checked: true } !== { value: 'foo', checked: true }
-     * We do our own "deep" comparision.
-     *
-     * @param {object} modelValue
-     * @param {object} modelValue the old one
      */
-    // TODO: consider making a generic option inside FormatMixin for deep object comparisons when
+    _onModelValueChanged({ modelValue }, { modelValue: old }) {
-    // modelValue is an object
+      if (this.constructor._classProperties.get('modelValue').hasChanged(modelValue, old)) {
-    _dispatchModelValueChangedEvent({ modelValue }, { modelValue: old }) {
+        super._onModelValueChanged({ modelValue });
-      let changed = true;
-      if (old) {
-        changed = modelValue.value !== old.value || modelValue.checked !== old.checked;
       }
-      if (changed) {
-        this.dispatchEvent(
-          new CustomEvent('model-value-changed', { bubbles: true, composed: true }),
-        );
-      }
-    }
-
-    _reflectCheckedToCssClass() {
-      this.classList[this.choiceChecked ? 'add' : 'remove']('state-checked');
     }
 
     /**
@@ -171,11 +204,30 @@ export const ChoiceInputMixin = superclass =>
 
     /**
      * @override
-     * Overridden from ValidateMixin, since a different modelValue is used for choice inputs.
+     * Overridden from Field, since a different modelValue is used for choice inputs.
      */
     __isRequired(modelValue) {
       return {
         required: !!modelValue.checked,
       };
     }
+
+    /**
+     * @deprecated use .checked
+     */
+    get choiceChecked() {
+      return this.checked;
+    }
+
+    /**
+     * @deprecated use .checked
+     */
+    set choiceChecked(c) {
+      this.checked = c;
+    }
+
+    /** @deprecated for styling purposes, use [checked] attribute */
+    _reflectCheckedToCssClass() {
+      this.classList[this.checked ? 'add' : 'remove']('state-checked');
+    }
   };

packages/choice-input/test/ChoiceInputMixin.test.js

@@ -1,5 +1,6 @@
 import { expect, fixture } from '@open-wc/testing';
 import { html } from '@lion/core';
+import sinon from 'sinon';
 
 import { LionInput } from '@lion/input';
 import { ChoiceInputMixin } from '../src/ChoiceInputMixin.js';
@@ -50,11 +51,11 @@ describe('ChoiceInputMixin', () => {
     `);
     expect(counter).to.equal(1); // undefined to set value
 
-    el.choiceChecked = true;
+    el.checked = true;
     expect(counter).to.equal(2);
 
     // no change means no event
-    el.choiceChecked = true;
+    el.checked = true;
     el.choiceValue = 'foo';
     el.modelValue = { value: 'foo', checked: true };
     expect(counter).to.equal(2);
@@ -87,58 +88,136 @@ describe('ChoiceInputMixin', () => {
     `);
 
     expect(el.error.required).to.be.true;
-    el.choiceChecked = true;
+    el.checked = true;
     expect(el.error.required).to.be.undefined;
   });
 
   describe('Checked state synchronization', () => {
     it('synchronizes checked state initially (via attribute or property)', async () => {
       const el = await fixture(`<choice-input></choice-input>`);
-      expect(el.choiceChecked).to.equal(false, 'initially unchecked');
+      expect(el.checked).to.equal(false, 'initially unchecked');
 
       const precheckedElementAttr = await fixture(html`
-        <choice-input .choiceChecked=${true}></choice-input>
+        <choice-input .checked=${true}></choice-input>
       `);
-      expect(precheckedElementAttr.choiceChecked).to.equal(true, 'initially checked via attribute');
+      el._syncValueUpwards = () => {}; // We need to disable the method for the test to pass
+
+      expect(precheckedElementAttr.checked).to.equal(true, 'initially checked via attribute');
     });
 
     it('can be checked and unchecked programmatically', async () => {
       const el = await fixture(`<choice-input></choice-input>`);
-      expect(el.choiceChecked).to.be.false;
+      expect(el.checked).to.be.false;
-      el.choiceChecked = true;
+      el.checked = true;
-      expect(el.choiceChecked).to.be.true;
+      expect(el.checked).to.be.true;
+
+      await el.updateComplete;
+      expect(el.inputElement.checked).to.be.true;
     });
 
     it('can be checked and unchecked via user interaction', async () => {
       const el = await fixture(`<choice-input></choice-input>`);
       el.inputElement.click();
-      expect(el.choiceChecked).to.be.true;
+      expect(el.checked).to.be.true;
       el.inputElement.click();
-      expect(el.choiceChecked).to.be.false;
+      expect(el.checked).to.be.false;
     });
 
     it('synchronizes modelValue to checked state and vice versa', async () => {
       const el = await fixture(html`
         <choice-input .choiceValue=${'foo'}></choice-input>
       `);
-      expect(el.choiceChecked).to.be.false;
+      expect(el.checked).to.be.false;
       expect(el.modelValue).to.deep.equal({
         checked: false,
         value: 'foo',
       });
-      el.choiceChecked = true;
+      el.checked = true;
-      expect(el.choiceChecked).to.be.true;
+      expect(el.checked).to.be.true;
       expect(el.modelValue).to.deep.equal({
         checked: true,
         value: 'foo',
       });
     });
 
-    it('synchronizes checked state to class "state-checked" for styling purposes', async () => {
+    it('ensures optimal synchronize performance by preventing redundant computation steps', async () => {
+      /* we are checking private apis here to make sure we do not have cyclical updates
+        which can be quite common for these type of connected data */
+      const el = await fixture(html`
+        <choice-input .choiceValue=${'foo'}></choice-input>
+      `);
+      expect(el.checked).to.be.false;
+
+      const spyModelCheckedToChecked = sinon.spy(el, '__syncModelCheckedToChecked');
+      const spyCheckedToModel = sinon.spy(el, '__syncCheckedToModel');
+      el.checked = true;
+      expect(el.modelValue.checked).to.be.true;
+      expect(spyModelCheckedToChecked.callCount).to.equal(0);
+      expect(spyCheckedToModel.callCount).to.equal(1);
+
+      el.modelValue = { value: 'foo', checked: false };
+      expect(el.checked).to.be.false;
+      expect(spyModelCheckedToChecked.callCount).to.equal(1);
+      expect(spyCheckedToModel.callCount).to.equal(1);
+
+      // not changeing values should not trigger any updates
+      el.checked = false;
+      el.modelValue = { value: 'foo', checked: false };
+      expect(spyModelCheckedToChecked.callCount).to.equal(1);
+      expect(spyCheckedToModel.callCount).to.equal(1);
+    });
+
+    it('synchronizes checked state to [checked] attribute for styling purposes', async () => {
+      const hasAttr = el => el.hasAttribute('checked');
+      const el = await fixture(`<choice-input></choice-input>`);
+      const elChecked = await fixture(html`
+        <choice-input .checked=${true}>
+          <input slot="input" />
+        </choice-input>
+      `);
+
+      // Initial values
+      expect(hasAttr(el)).to.equal(false, 'inital unchecked element');
+      expect(hasAttr(elChecked)).to.equal(true, 'inital checked element');
+
+      // Programmatically via checked
+      el.checked = true;
+      elChecked.checked = false;
+
+      await el.updateComplete;
+      expect(hasAttr(el)).to.equal(true, 'programmatically checked');
+      expect(hasAttr(elChecked)).to.equal(false, 'programmatically unchecked');
+
+      // reset
+      el.checked = false;
+      elChecked.checked = true;
+
+      // Via user interaction
+      el.inputElement.click();
+      elChecked.inputElement.click();
+      await el.updateComplete;
+      expect(hasAttr(el)).to.equal(true, 'user click checked');
+      expect(hasAttr(elChecked)).to.equal(false, 'user click unchecked');
+
+      // reset
+      el.checked = false;
+      elChecked.checked = true;
+
+      // Programmatically via modelValue
+      el.modelValue = { value: '', checked: true };
+      elChecked.modelValue = { value: '', checked: false };
+      await el.updateComplete;
+      expect(hasAttr(el)).to.equal(true, 'modelValue checked');
+      expect(hasAttr(elChecked)).to.equal(false, 'modelValue unchecked');
+    });
+
+    it('[deprecated] synchronizes checked state to class "state-checked" for styling purposes', async () => {
       const hasClass = el => [].slice.call(el.classList).indexOf('state-checked') > -1;
       const el = await fixture(`<choice-input></choice-input>`);
       const elChecked = await fixture(html`
-        <choice-input .choiceChecked=${true}></choice-input>
+        <choice-input .checked=${true}>
+          <input slot="input" />
+        </choice-input>
       `);
 
       // Initial values
@@ -146,15 +225,16 @@ describe('ChoiceInputMixin', () => {
       expect(hasClass(elChecked)).to.equal(true, 'inital checked element');
 
       // Programmatically via checked
-      el.choiceChecked = true;
+      el.checked = true;
-      elChecked.choiceChecked = false;
+      elChecked.checked = false;
+
       await el.updateComplete;
       expect(hasClass(el)).to.equal(true, 'programmatically checked');
       expect(hasClass(elChecked)).to.equal(false, 'programmatically unchecked');
 
       // reset
-      el.choiceChecked = false;
+      el.checked = false;
-      elChecked.choiceChecked = true;
+      elChecked.checked = true;
 
       // Via user interaction
       el.inputElement.click();
@@ -164,8 +244,8 @@ describe('ChoiceInputMixin', () => {
       expect(hasClass(elChecked)).to.equal(false, 'user click unchecked');
 
       // reset
-      el.choiceChecked = false;
+      el.checked = false;
-      elChecked.choiceChecked = true;
+      elChecked.checked = true;
 
       // Programmatically via modelValue
       el.modelValue = { value: '', checked: true };
@@ -174,6 +254,21 @@ describe('ChoiceInputMixin', () => {
       expect(hasClass(el)).to.equal(true, 'modelValue checked');
       expect(hasClass(elChecked)).to.equal(false, 'modelValue unchecked');
     });
+
+    it('[deprecated] uses choiceChecked to set checked state', async () => {
+      const el = await fixture(html`
+        <choice-input .choiceValue=${'foo'}></choice-input>
+      `);
+      expect(el.choiceChecked).to.be.false;
+      el.choiceChecked = true;
+      expect(el.checked).to.be.true;
+      expect(el.modelValue).to.deep.equal({
+        checked: true,
+        value: 'foo',
+      });
+      await el.updateComplete;
+      expect(el.inputElement.checked).to.be.true;
+    });
   });
 
   describe('Format/parse/serialize loop', () => {
@@ -184,7 +279,7 @@ describe('ChoiceInputMixin', () => {
       expect(el.modelValue).deep.equal({ value: 'foo', checked: false });
 
       const elChecked = await fixture(html`
-        <choice-input .choiceValue=${'foo'} .choiceChecked=${true}></choice-input>
+        <choice-input .choiceValue=${'foo'} .checked=${true}></choice-input>
       `);
       expect(elChecked.modelValue).deep.equal({ value: 'foo', checked: true });
     });
@@ -203,7 +298,7 @@ describe('ChoiceInputMixin', () => {
   describe('Interaction states', () => {
     it('is considered prefilled when checked and not considered prefilled when unchecked', async () => {
       const el = await fixture(html`
-        <choice-input .choiceChecked=${true}></choice-input>
+        <choice-input .checked=${true}></choice-input>
       `);
       expect(el.prefilled).equal(true, 'checked element not considered prefilled');