fix(input-amount): parse amount always on locale once the amount is f…

…ormatted (#2439) * fix(input-amount): parse amount always on locale once the amount is formatted * chore: add unit test * chore: add some description * chore: update playwright script to install dependencies * Update .github/workflows/verify-pr.yml * chore: set formatOptions temp and cleanup for programmatic api * feat(form-core): add "user-edit" mode to formatOptions while editing existing value of a form control * chore: enhance code readability and robustness --------- Co-authored-by: Thijs Louisse <[email protected]>
ing-bank · Jan 15, 2025 · 35e6605 · 35e6605
1 parent a992da4
commit 35e6605
Show file tree

Hide file tree

Showing 11 changed files with 162 additions and 36 deletions.
diff --git a/.changeset/bright-snakes-smash.md b/.changeset/bright-snakes-smash.md
@@ -0,0 +1,5 @@
+---
+'@lion/ui': minor
+---
+
+[form-core] add "user-edit" mode to formatOptions while editing existing value of a form control
diff --git a/packages/ui/components/form-core/src/FormatMixin.js b/packages/ui/components/form-core/src/FormatMixin.js
@@ -249,7 +249,7 @@ const FormatMixinImplementation = superclass =>
       // Apparently, the parser was not able to produce a satisfactory output for the desired
       // modelValue type, based on the current viewValue. Unparseable allows to restore all
       // states (for instance from a lost user session), since it saves the current viewValue.
-      const result = this.parser(value, this.formatOptions);
+      const result = this.parser(value, { ...this.formatOptions, mode: this.#getFormatMode() });
       return result !== undefined ? result : new Unparseable(value);
     }
 
@@ -269,13 +269,8 @@ const FormatMixinImplementation = superclass =>
       // imperatively, we DO want to format a value (it is the only way to get meaningful
       // input into `._inputNode` with modelValue as input)
 
-      if (
-        this._isHandlingUserInput &&
-        this.hasFeedbackFor?.length &&
-        this.hasFeedbackFor.includes('error') &&
-        this._inputNode
-      ) {
-        return this._inputNode ? this.value : undefined;
+      if (this._isHandlingUserInput && this.hasFeedbackFor?.includes('error') && this._inputNode) {
+        return this.value;
       }
 
       if (this.modelValue instanceof Unparseable) {
@@ -285,7 +280,10 @@ const FormatMixinImplementation = superclass =>
         return this.modelValue.viewValue;
       }
 
-      return this.formatter(this.modelValue, this.formatOptions);
+      return this.formatter(this.modelValue, {
+        ...this.formatOptions,
+        mode: this.#getFormatMode(),
+      });
     }
 
     /**
@@ -348,7 +346,6 @@ const FormatMixinImplementation = superclass =>
      * @private
      */
     __handlePreprocessor() {
-      const unprocessedValue = this.value;
       let currentCaretIndex = this.value.length;
       // Be gentle with Safari
       if (
@@ -364,7 +361,6 @@ const FormatMixinImplementation = superclass =>
         prevViewValue: this.__prevViewValue,
       });
 
-      this.__prevViewValue = unprocessedValue;
       if (preprocessedValue === undefined) {
         // Make sure we do no set back original value, so we preserve
         // caret index (== selectionStart/selectionEnd)
@@ -459,7 +455,7 @@ const FormatMixinImplementation = superclass =>
       /**
        * Configuration object that will be available inside the formatter function
        */
-      this.formatOptions = /** @type {FormatOptions} */ ({});
+      this.formatOptions = /** @type {FormatOptions} */ ({ mode: 'auto' });
 
       /**
        * The view value is the result of the formatter function (when available).
@@ -543,10 +539,8 @@ const FormatMixinImplementation = superclass =>
      */
     __onPaste() {
       this._isPasting = true;
-      this.formatOptions.mode = 'pasted';
-      setTimeout(() => {
+      queueMicrotask(() => {
         this._isPasting = false;
-        this.formatOptions.mode = 'auto';
       });
     }
 
@@ -588,6 +582,17 @@ const FormatMixinImplementation = superclass =>
         this._inputNode.removeEventListener('compositionend', this.__onCompositionEvent);
       }
     }
+
+    #getFormatMode() {
+      if (this._isPasting) {
+        return 'pasted';
+      }
+      const isUserEditing = this._isHandlingUserInput && this.__prevViewValue;
+      if (isUserEditing) {
+        return 'user-edit';
+      }
+      return 'auto';
+    }
   };
 
 export const FormatMixin = dedupeMixin(FormatMixinImplementation);
diff --git a/packages/ui/components/form-core/test-suites/FormatMixin.suite.js b/packages/ui/components/form-core/test-suites/FormatMixin.suite.js
@@ -5,6 +5,8 @@ import sinon from 'sinon';
 import { Unparseable, Validator, FormatMixin } from '@lion/ui/form-core.js';
 import { getFormControlMembers, mimicUserInput } from '@lion/ui/form-core-test-helpers.js';
 
+const isLionInputStepper = (/** @type {FormatClass} */ el) => 'valueTextMapping' in el;
+
 /**
  * @typedef {import('../types/FormControlMixinTypes.js').FormControlHost} FormControlHost
  * @typedef {ArrayConstructor | ObjectConstructor | NumberConstructor | BooleanConstructor | StringConstructor | DateConstructor | 'iban' | 'email'} modelValueType
@@ -480,7 +482,7 @@ export function runFormatMixinSuite(customConfig) {
           /**
            * @param {FormatClass} el
            */
-          function paste(el, val = 'lorem') {
+          function mimicPaste(el, val = 'lorem') {
             const { _inputNode } = getFormControlMembers(el);
             _inputNode.value = val;
             _inputNode.dispatchEvent(new ClipboardEvent('paste', { bubbles: true }));
@@ -494,12 +496,17 @@ export function runFormatMixinSuite(customConfig) {
           `)
             );
             const formatterSpy = sinon.spy(el, 'formatter');
-            paste(el);
+            mimicPaste(el);
             expect(formatterSpy).to.be.called;
-            expect(/** @type {{mode: string}} */ (formatterSpy.args[0][1]).mode).to.equal('pasted');
+            expect(/** @type {{mode: string}} */ (formatterSpy.lastCall.args[1]).mode).to.equal(
+              'pasted',
+            );
+            // give microtask of _isPasting chance to reset
             await aTimeout(0);
-            mimicUserInput(el, '');
-            expect(/** @type {{mode: string}} */ (formatterSpy.args[0][1]).mode).to.equal('auto');
+            el.modelValue = 'foo';
+            expect(/** @type {{mode: string}} */ (formatterSpy.lastCall.args[1]).mode).to.equal(
+              'auto',
+            );
           });
 
           it('sets protected value "_isPasting" for Subclassers', async () => {
@@ -509,7 +516,7 @@ export function runFormatMixinSuite(customConfig) {
           `)
             );
             const formatterSpy = sinon.spy(el, 'formatter');
-            paste(el);
+            mimicPaste(el);
             expect(formatterSpy).to.have.been.called;
             // @ts-ignore [allow-protected] in test
             expect(el._isPasting).to.be.true;
@@ -526,7 +533,7 @@ export function runFormatMixinSuite(customConfig) {
             );
             // @ts-ignore [allow-protected] in test
             const reflectBackSpy = sinon.spy(el, '_reflectBackOn');
-            paste(el);
+            mimicPaste(el);
             expect(reflectBackSpy).to.have.been.called;
           });
 
@@ -538,10 +545,33 @@ export function runFormatMixinSuite(customConfig) {
             );
             // @ts-ignore [allow-protected] in test
             const reflectBackSpy = sinon.spy(el, '_reflectBackOn');
-            paste(el);
+            mimicPaste(el);
             expect(reflectBackSpy).to.have.been.called;
           });
         });
+
+        describe('On user input', () => {
+          it('adjusts formatOptions.mode to "user-edit" for parser when user changes value', async () => {
+            const el = /** @type {FormatClass} */ (
+              await fixture(html`<${tag}><input slot="input"></${tag}>`)
+            );
+
+            const parserSpy = sinon.spy(el, 'parser');
+
+            // Here we get auto as we start from '' (so there was no value to edit)
+            mimicUserInput(el, 'some val');
+            expect(/** @type {{mode: string}} */ (parserSpy.lastCall.args[1]).mode).to.equal(
+              'auto',
+            );
+            await el.updateComplete;
+
+            mimicUserInput(el, 'some other val');
+            expect(/** @type {{mode: string}} */ (parserSpy.lastCall.args[1]).mode).to.equal(
+              'user-edit',
+            );
+            await el.updateComplete;
+          });
+        });
       });
 
       describe('Parser', () => {
@@ -597,6 +627,43 @@ export function runFormatMixinSuite(customConfig) {
           expect(_inputNode.value).to.equal(val);
         });
 
+        it('does only calculate derived values as consequence of user input when preprocessed value is different from previous view value', async () => {
+          const val = generateValueBasedOnType({ viewValue: true }) || 'init-value';
+          if (typeof val !== 'string') return;
+
+          const preprocessorSpy = sinon.spy(v => v.replace(/\$$/g, ''));
+          const el = /** @type {FormatClass} */ (
+            await fixture(html`
+          <${tag} .preprocessor=${preprocessorSpy}>
+            <input slot="input" .value="${val}">
+          </${tag}>
+        `)
+          );
+
+          // TODO: find out why we need to skip this for lion-input-stepper
+          if (isLionInputStepper(el)) return;
+
+          /**
+           * The _calculateValues method is called inside _onUserInputChanged w/o providing args
+           * @param {sinon.SinonSpyCall} call
+           * @returns {boolean}
+           */
+          const isCalculateCallAfterUserInput = call => call.args[0]?.length === 0;
+
+          const didRecalculateAfterUserInput = (/** @type {sinon.SinonSpy<any[], any>} */ spy) =>
+            spy.callCount > 1 && !spy.getCalls().find(isCalculateCallAfterUserInput);
+
+          // @ts-expect-error [allow-protected] in test
+          const calcValuesSpy = sinon.spy(el, '_calculateValues');
+          // this value gets preprocessed to 'val'
+          mimicUserInput(el, `${val}$`);
+          expect(didRecalculateAfterUserInput(calcValuesSpy)).to.be.false;
+
+          // this value gets preprocessed to 'value' (and thus differs from previous)
+          mimicUserInput(el, `${val}ue$`);
+          expect(didRecalculateAfterUserInput(calcValuesSpy)).to.be.true;
+        });
+
         it('does not preprocess during composition', async () => {
           const el = /** @type {FormatClass} */ (
             await fixture(html`

diff --git a/packages/ui/components/form-core/types/FormatMixinTypes.ts b/packages/ui/components/form-core/types/FormatMixinTypes.ts
@@ -3,7 +3,7 @@ import { LitElement } from 'lit';
 import { ValidateHost } from './validate/ValidateMixinTypes.js';
 import { FormControlHost } from './FormControlMixinTypes.js';
 
-export type FormatOptions = { mode: 'pasted' | 'auto' } & object;
+export type FormatOptions = { mode: 'pasted' | 'auto' | 'user-edit'} & object;
 export declare class FormatHost {
   /**
    * Converts viewValue to modelValue

diff --git a/packages/ui/components/input-amount/src/parsers.js b/packages/ui/components/input-amount/src/parsers.js
@@ -1,7 +1,7 @@
 import { parseNumber, getFractionDigits } from '@lion/ui/localize-no-side-effects.js';
 
 /**
- * @typedef {import('../../localize/types/LocalizeMixinTypes.js').FormatNumberOptions} FormatOptions
+ * @typedef {import('../../localize/types/LocalizeMixinTypes.js').FormatNumberOptions} FormatNumberOptions
  */
 
 /**
@@ -28,7 +28,7 @@ function round(value, decimals) {
  * parseAmount('1,234.56', {currency: 'JOD'}); => 1234.560
  *
  * @param {string} value Number to be parsed
- * @param {FormatOptions} [givenOptions] Locale Options
+ * @param {FormatNumberOptions} [givenOptions] Locale Options
  */
 export function parseAmount(value, givenOptions) {
   const unmatchedInput = value.match(/[^0-9,.\- ]/g);
@@ -44,7 +44,7 @@ export function parseAmount(value, givenOptions) {
     return undefined;
   }
 
-  /** @type {FormatOptions} */
+  /** @type {FormatNumberOptions} */
   const options = {
     ...givenOptions,
   };

diff --git a/packages/ui/components/input-amount/test/lion-input-amount.test.js b/packages/ui/components/input-amount/test/lion-input-amount.test.js
@@ -1,17 +1,20 @@
 import { aTimeout, expect, fixture } from '@open-wc/testing';
 import { html } from 'lit';
-import { localize } from '@lion/ui/localize.js';
+import { getLocalizeManager } from '@lion/ui/localize-no-side-effects.js';
 import { localizeTearDown } from '@lion/ui/localize-test-helpers.js';
 import { getInputMembers } from '@lion/ui/input-test-helpers.js';
 import { LionInputAmount, formatAmount, parseAmount } from '@lion/ui/input-amount.js';
-
+import { mimicUserInput } from '@lion/ui/form-core-test-helpers.js';
+import sinon from 'sinon';
 import '@lion/ui/define/lion-input-amount.js';
 
 /**
  * @typedef {import('../../input/src/LionInput.js').LionInput} LionInput
  */
 
 describe('<lion-input-amount>', () => {
+  const localize = getLocalizeManager();
+
   beforeEach(() => {
     localizeTearDown();
   });
@@ -128,6 +131,37 @@ describe('<lion-input-amount>', () => {
     expect(_inputNode.value).to.equal('100.12');
   });
 
+  it('adjusts formats with locale when formatOptions.mode is "user-edit"', async () => {
+    const el = /** @type {LionInputAmount} */ (
+      await fixture(
+        html`<lion-input-amount
+          .modelValue=${123456.78}
+          currency="EUR"
+          .formatOptions="${{ locale: 'nl-NL' }}"
+        ></lion-input-amount>`,
+      )
+    );
+    const parserSpy = sinon.spy(el, 'parser');
+    const formatterSpy = sinon.spy(el, 'formatter');
+
+    // @ts-expect-error [allow-protected] in test
+    expect(el._inputNode.value).to.equal('123.456,78');
+
+    // When editing an already existing value, we interpet the separators as they are
+    mimicUserInput(el, '123.456');
+    expect(parserSpy.lastCall.args[1]?.mode).to.equal('user-edit');
+    expect(formatterSpy.lastCall.args[1]?.mode).to.equal('user-edit');
+    expect(el.modelValue).to.equal(123456);
+    expect(el.formattedValue).to.equal('123.456,00');
+
+    // Formatting should only affect values that should be formatted / parsed as a consequence of user input.
+    // When a user finished editing, the default should be restored.
+    // (think of a programmatically set modelValue, that should behave idempotent, regardless of when it is set)
+    el.modelValue = 1234;
+    expect(el.formattedValue).to.equal('1.234,00');
+    expect(formatterSpy.lastCall.args[1]?.mode).to.equal('auto');
+  });
+
   it('sets inputmode attribute to decimal', async () => {
     const el = /** @type {LionInputAmount} */ (
       await fixture(`<lion-input-amount></lion-input-amount>`)

diff --git a/packages/ui/components/input-amount/test/parsers.test.js b/packages/ui/components/input-amount/test/parsers.test.js
@@ -60,4 +60,11 @@ describe('parseAmount()', async () => {
     expect(parseAmount('foo1', { mode: 'pasted' })).to.equal(1);
     expect(parseAmount('EUR 1,50', { mode: 'pasted' })).to.equal(1.5);
   });
+
+  it('parses based on locale when "user-edit" mode used', async () => {
+    expect(parseAmount('123,456.78', { mode: 'auto' })).to.equal(123456.78);
+    expect(parseAmount('123,456.78', { mode: 'user-edit' })).to.equal(123456.78);
+    expect(parseAmount('123.456,78', { mode: 'auto' })).to.equal(123456.78);
+    expect(parseAmount('123.456,78', { mode: 'user-edit' })).to.equal(123.45678);
+  });
 });
diff --git a/packages/ui/components/localize/src/number/getDecimalSeparator.js b/packages/ui/components/localize/src/number/getDecimalSeparator.js
@@ -8,7 +8,7 @@ import { getLocale } from '../utils/getLocale.js';
  * @returns {string} The separator
  */
 export function getDecimalSeparator(locale, options) {
-  if (options && options.decimalSeparator) {
+  if (options?.decimalSeparator) {
     return options.decimalSeparator;
   }
   const computedLocale = getLocale(locale);

diff --git a/packages/ui/components/localize/src/number/parseNumber.js b/packages/ui/components/localize/src/number/parseNumber.js
@@ -17,13 +17,15 @@ import { getDecimalSeparator } from './getDecimalSeparator.js';
  *
  * @param {string} value Clean number (only [0-9 ,.]) to be parsed
  * @param {object} options
- * @param {string?} [options.mode] auto|pasted
+ * @param {string?} [options.mode] auto|pasted|user-edit
  * @return {string} unparseable|withLocale|heuristic
  */
 function getParseMode(value, { mode = 'auto' } = {}) {
   const separators = value.match(/[., ]/g);
 
-  if (!separators) {
+  // When a user edits an existin value, we already formatted it with a certain locale.
+  // For best UX, we stick with this locale
+  if (!separators || mode === 'user-edit') {
     return 'withLocale';
   }
   if (mode === 'auto' && separators.length === 1) {
@@ -52,8 +54,7 @@ function getParseMode(value, { mode = 'auto' } = {}) {
  * @param {import('../../types/LocalizeMixinTypes.js').FormatNumberOptions} options Locale Options
  */
 function parseWithLocale(value, options) {
-  const locale = options && options.locale ? options.locale : undefined;
-  const separator = getDecimalSeparator(locale, options);
+  const separator = getDecimalSeparator(options?.locale, options);
   const regexNumberAndLocaleSeparator = new RegExp(`[0-9${separator}-]`, 'g');
   let numberAndLocaleSeparator = value.match(regexNumberAndLocaleSeparator)?.join('');
   if (separator === ',') {

diff --git a/packages/ui/components/localize/test/number/parseNumber.test.js b/packages/ui/components/localize/test/number/parseNumber.test.js
@@ -74,6 +74,13 @@ describe('parseNumber()', () => {
     expect(parseNumber('123456.78', { mode: 'pasted' })).to.equal(123456.78);
   });
 
+  it('detects separators withLocale when "user-edit" mode used e.g. 123.456,78', async () => {
+    expect(parseNumber('123,456.78', { mode: 'auto' })).to.equal(123456.78);
+    expect(parseNumber('123,456.78', { mode: 'user-edit' })).to.equal(123456.78);
+    expect(parseNumber('123.456,78', { mode: 'auto' })).to.equal(123456.78);
+    expect(parseNumber('123.456,78', { mode: 'user-edit' })).to.equal(123.45678);
+  });
+
   it('detects separators unparseable when there are 2 same ones e.g. 1.234.56', () => {
     expect(parseNumber('1.234.56')).to.equal(123456);
     expect(parseNumber('1,234,56')).to.equal(123456);