fix(field): format conditionally on user input only

Author: tlouisse

packages/field/src/FormatMixin.js

@@ -6,7 +6,16 @@ import { ObserverMixin } from '@lion/core/src/ObserverMixin.js';
 import { Unparseable } from '@lion/validate';
 
 /**
- * @polymerMixin
+ * @desc Designed to be applied on top of a LionField
+ *
+ * FormatMixin supports these two main flows:
+ * 1) Application Developer sets `.modelValue`:
+ *    Flow: `.modelValue` -> `.formattedValue` -> `.inputElement.value`
+ *                        -> `.serializedValue`
+ * 2) End user interacts with field:
+ *    Flow: `@user-input-changed` -> `.modelValue` -> `.formattedValue` - (debounce till reflect condition (formatOn) is met) -> `.inputElement.value`
+ *                                -> `.serializedValue`
+ *
  * @mixinFunction
  */
 export const FormatMixin = dedupeMixin(
@@ -26,7 +35,8 @@ export const FormatMixin = dedupeMixin(
            *
            * Examples:
            * - For a date input: a String '20/01/1999' will be converted to new Date('1999/01/20')
-           * - For a number input: a formatted String '1.234,56' will be converted to a Number: 1234.56
+           * - For a number input: a formatted String '1.234,56' will be converted to a Number:
+           *   1234.56
            */
           modelValue: {
             type: Object,
@@ -48,13 +58,13 @@ export const FormatMixin = dedupeMixin(
           /**
            * The serialized version of the model value.
            * This value exists for maximal compatibility with the platform API.
-           * The serialized value can be an interface in context where data binding is not supported
-           * and a serialized string needs to be set.
+           * The serialized value can be an interface in context where data binding is not
+           * supported and a serialized string needs to be set.
            *
            * Examples:
            * - For a date input, this would be the iso format of a date, e.g. '1999-01-20'.
-           * - For a number input this would be the String representation of a float ('1234.56' instead
-           *   of 1234.56)
+           * - For a number input this would be the String representation of a float ('1234.56'
+           *   instead of 1234.56)
            *
            * When no parser is available, the value is usually the same as the formattedValue
            * (being inputElement.value)
@@ -139,15 +149,16 @@ export const FormatMixin = dedupeMixin(
       }
 
       /**
-       * Responsible for storing all representations(modelValue, serializedValue, formattedValue and
-       * value) of the input value.
-       * Prevents infinite loops, so all value observers can be treated like they will only be called
-       * once, without indirectly calling other observers.
-       * (in fact, some are called twice, but the __preventRecursiveTrigger lock prevents the second
-       * call from having effect).
+       * Responsible for storing all representations(modelValue, serializedValue, formattedValue
+       * and value) of the input value.
+       * Prevents infinite loops, so all value observers can be treated like they will only be
+       * called once, without indirectly calling other observers.
+       * (in fact, some are called twice, but the __preventRecursiveTrigger lock prevents the
+       * second call from having effect).
        *
-       * @param {string} source - the type of value that triggered this method. It should not be set
-       * again, so that its observer won't be triggered. Can be: 'model'|'formatted'|'serialized'.
+       * @param {string} source - the type of value that triggered this method. It should not be
+       * set again, so that its observer won't be triggered. Can be:
+       * 'model'|'formatted'|'serialized'.
        */
       _calculateValues({ source } = {}) {
         if (this.__preventRecursiveTrigger) return; // prevent infinite loops
@@ -182,7 +193,19 @@ export const FormatMixin = dedupeMixin(
         if (this.modelValue instanceof Unparseable) {
           return this.modelValue.viewValue;
         }
-        if (this.errorState) {
+
+        // - Why check for this.errorState?
+        // We only want to format values that are considered valid. For best UX,
+        // we only 'reward' valid inputs.
+        // - Why check for __isHandlingUserInput?
+        // Downwards sync is prevented whenever we are in a `@user-input-changed` flow.
+        // If we are in a 'imperatively set `.modelValue`' flow, we want to reflect back
+        // the value, no matter what.
+        // This means, whenever we are in errorState, we and modelValue is set
+        // imperatively, we DO want to format a value (it is the only way to get meaningful
+        // input into `.inputElement` with modelValue as input)
+
+        if (this.__isHandlingUserInput && this.errorState) {
           return this.inputElement ? this.value : undefined;
         }
         return this.formatter(this.modelValue, this.formatOptions);
@@ -231,21 +254,19 @@ export const FormatMixin = dedupeMixin(
        */
       _syncValueUpwards() {
         // Downwards syncing should only happen for <lion-field>.value changes from 'above'
-        this.__preventDownwardsSync = true;
         // This triggers _onModelValueChanged and connects user input to the
         // parsing/formatting/serializing loop
         this.modelValue = this.__callParser(this.value);
-        this.__preventDownwardsSync = false;
       }
 
       /**
        * Synchronization from <lion-field>.value to <input>.value
        */
       _reflectBackFormattedValueToUser() {
         // Downwards syncing 'back and forth' prevents change event from being fired in IE.
-        // So only sync when the source of new <lion-field>.value change was not the 'input' event of
-        // inputElement
-        if (!this.__preventDownwardsSync) {
+        // So only sync when the source of new <lion-field>.value change was not the 'input' event
+        // of inputElement
+        if (!this.__isHandlingUserInput) {
           // Text 'undefined' should not end up in <input>
           this.value = typeof this.formattedValue !== 'undefined' ? this.formattedValue : '';
         }
@@ -266,9 +287,12 @@ export const FormatMixin = dedupeMixin(
       }
 
       _onUserInputChanged() {
-        // Upwards syncing. Most properties are delegated right away, value is synced to <lion-field>,
-        // to be able to act on (imperatively set) value changes
+        // Upwards syncing. Most properties are delegated right away, value is synced to
+        // <lion-field>, to be able to act on (imperatively set) value changes
+
+        this.__isHandlingUserInput = true;
         this._syncValueUpwards();
+        this.__isHandlingUserInput = false;
       }
 
       constructor() {
@@ -289,11 +313,11 @@ export const FormatMixin = dedupeMixin(
         this.inputElement.addEventListener(this.formatOn, this._reflectBackFormattedValueDebounced);
         this.inputElement.addEventListener('input', this._proxyInputEvent);
         this.addEventListener('user-input-changed', this._onUserInputChanged);
-        // Connect the value found in <input> to the formatting/parsing/serializing loop as a fallback
-        // mechanism. Assume the user uses the value property of the <lion-field>(recommended api) as
-        // the api (this is a downwards sync).
-        // However, when no value is specified on <lion-field>, have support for sync of the real input
-        // to the <lion-field> (upwards sync).
+        // Connect the value found in <input> to the formatting/parsing/serializing loop as a
+        // fallback mechanism. Assume the user uses the value property of the
+        // <lion-field>(recommended api) as the api (this is a downwards sync).
+        // However, when no value is specified on <lion-field>, have support for sync of the real
+        // input to the <lion-field> (upwards sync).
         if (typeof this.modelValue === 'undefined') {
           this._syncValueUpwards();
         }

packages/field/test/FormatMixin.test.js

@@ -157,6 +157,25 @@ describe('FormatMixin', () => {
     expect(formatEl.inputElement.value).to.equal('foo: test');
   });
 
+  it('reflects back .formattedValue immediately when .modelValue changed imperatively', async () => {
+    const el = await fixture(html`
+      <${elem} .formatter="${value => `foo: ${value}`}">
+        <input slot="input" />
+      </${elem}>
+    `);
+    // The FormatMixin can be used in conjunction with the ValidateMixin, in which case
+    // it can hold errorState (affecting the formatting)
+    el.errorState = true;
+
+    // users types value 'test'
+    mimicUserInput(el, 'test');
+    expect(el.inputElement.value).to.not.equal('foo: test');
+
+    // Now see the difference for an imperative change
+    el.modelValue = 'test2';
+    expect(el.inputElement.value).to.equal('foo: test2');
+  });
+
   describe('parsers/formatters/serializers', () => {
     it('should call the parser|formatter|serializer provided by user', async () => {
       const formatterSpy = sinon.spy(value => `foo: ${value}`);
@@ -206,7 +225,7 @@ describe('FormatMixin', () => {
       expect(parserSpy.callCount).to.equal(1);
     });
 
-    it('will only call the formatter for valid values', async () => {
+    it('will only call the formatter for valid values on `user-input-changed` ', async () => {
       const formatterSpy = sinon.spy(value => `foo: ${value}`);
       const el = await fixture(html`
         <${elem} .formatter=${formatterSpy}>
@@ -216,12 +235,12 @@ describe('FormatMixin', () => {
       expect(formatterSpy.callCount).to.equal(1);
 
       el.errorState = true;
-      el.modelValue = 'bar';
+      mimicUserInput(el, 'bar');
       expect(formatterSpy.callCount).to.equal(1);
-      expect(el.formattedValue).to.equal('foo: init-string');
+      expect(el.formattedValue).to.equal('bar');
 
       el.errorState = false;
-      el.modelValue = 'bar2';
+      mimicUserInput(el, 'bar2');
       expect(formatterSpy.callCount).to.equal(2);
 
       expect(el.formattedValue).to.equal('foo: bar2');

packages/field/test/lion-field.test.js

@@ -19,6 +19,11 @@ const tag = unsafeStatic(tagString);
 const inputSlotString = '<input slot="input" />';
 const inputSlot = unsafeHTML(inputSlotString);
 
+function mimicUserInput(formControl, newViewValue) {
+  formControl.value = newViewValue; // eslint-disable-line no-param-reassign
+  formControl.inputElement.dispatchEvent(new CustomEvent('input', { bubbles: true }));
+}
+
 beforeEach(() => {
   localizeTearDown();
 });
@@ -328,7 +333,7 @@ describe('<lion-field>', () => {
       expect(lionField.error.required).to.be.undefined;
     });
 
-    it('will only update formattedValue the value when valid', async () => {
+    it('will only update formattedValue when valid on `user-input-changed`', async () => {
       const formatterSpy = sinon.spy(value => `foo: ${value}`);
       function isBarValidator(value) {
         return { isBar: value === 'bar' };
@@ -348,9 +353,9 @@ describe('<lion-field>', () => {
       expect(formatterSpy.callCount).to.equal(1);
       expect(lionField.formattedValue).to.equal('foo: bar');
 
-      lionField.modelValue = 'foo';
+      mimicUserInput(lionField, 'foo');
       expect(formatterSpy.callCount).to.equal(1);
-      expect(lionField.formattedValue).to.equal('foo: bar');
+      expect(lionField.formattedValue).to.equal('foo');
     });
   });