Merge pull request #1254 from ing-bank/fix/success-show

fix(form-core): only show success after recovery from shown feedback

.changeset/orange-walls-sniff.md

@@ -0,0 +1,5 @@
+---
+'@lion/form-core': patch
+---
+
+Only show success feedback if the user is recovering from a shown error/warning.

.eslintignore

@@ -2,6 +2,7 @@ node_modules
 coverage/
 bundlesize/
 .history/
+storybook-static/
 *.d.ts
 _site-dev
 _site

packages/form-core/src/validate/ResultValidator.js

@@ -11,11 +11,19 @@ export class ResultValidator extends Validator {
    * @param {Object} context
    * @param {Validator[]} context.regularValidationResult
    * @param {Validator[]} context.prevValidationResult
+   * @param {Validator[]} context.prevShownValidationResult
    * @param {Validator[]} [context.validators]
    * @returns {boolean}
    */
-  // eslint-disable-next-line no-unused-vars, class-methods-use-this
+  /* eslint-disable no-unused-vars */
-  executeOnResults({ regularValidationResult, prevValidationResult, validators }) {
+  // eslint-disable-next-line class-methods-use-this
+  executeOnResults({
+    regularValidationResult,
+    prevValidationResult,
+    prevShownValidationResult,
+    validators,
+  }) {
+    /* eslint-enable no-unused-vars */
     return true;
   }
 }

packages/form-core/src/validate/ValidateMixin.js

@@ -163,6 +163,8 @@ export const ValidateMixinImplementation = superclass =>
       this.__validationResult = [];
       /** @type {Validator[]} */
       this.__prevValidationResult = [];
+      /** @type {Validator[]} */
+      this.__prevShownValidationResult = [];
 
       this.__onValidatorUpdated = this.__onValidatorUpdated.bind(this);
       this._updateFeedbackComponent = this._updateFeedbackComponent.bind(this);
@@ -279,7 +281,7 @@ export const ValidateMixinImplementation = superclass =>
         return;
       }
 
-      this.__storePrevResult();
+      this.__prevValidationResult = this.__validationResult;
       if (clearCurrentResult) {
         // Clear ('invalidate') all pending and existing validation results.
         // This is needed because we have async (pending) validators whose results
@@ -289,10 +291,6 @@ export const ValidateMixinImplementation = superclass =>
       await this.__executeValidators();
     }
 
-    __storePrevResult() {
-      this.__prevValidationResult = this.__validationResult;
-    }
-
     /**
      * @desc step A1-3 + B (as explained in 'validate')
      */
@@ -402,6 +400,7 @@ export const ValidateMixinImplementation = superclass =>
         v.executeOnResults({
           regularValidationResult,
           prevValidationResult: this.__prevValidationResult,
+          prevShownValidationResult: this.__prevShownValidationResult,
         }),
       );
     }
@@ -582,8 +581,12 @@ export const ValidateMixinImplementation = superclass =>
           this.__prioritizedResult = this._prioritizeAndFilterFeedback({
             validationResult: this.__validationResult,
           });
-          const messageMap = await this.__getFeedbackMessages(this.__prioritizedResult);
 
+          if (this.__prioritizedResult.length > 0) {
+            this.__prevShownValidationResult = this.__prioritizedResult;
+          }
+
+          const messageMap = await this.__getFeedbackMessages(this.__prioritizedResult);
           _feedbackNode.feedbackData = messageMap.length ? messageMap : [];
         });
       } else {
@@ -636,10 +639,15 @@ export const ValidateMixinImplementation = superclass =>
     _updateShouldShowFeedbackFor() {
       const ctor = /** @type {typeof import('../../types/validate/ValidateMixinTypes').ValidateHost} */ (this
         .constructor);
+
       // Necessary typecast because types aren't smart enough to understand that we filter out undefined
-      this.shouldShowFeedbackFor = /** @type {string[]} */ (ctor.validationTypes
+      const newShouldShowFeedbackFor = /** @type {string[]} */ (ctor.validationTypes
         .map(type => (this._showFeedbackConditionFor(type) ? type : undefined))
         .filter(_ => !!_));
+
+      if (JSON.stringify(this.shouldShowFeedbackFor) !== JSON.stringify(newShouldShowFeedbackFor)) {
+        this.shouldShowFeedbackFor = newShouldShowFeedbackFor;
+      }
     }
 
     /**

packages/form-core/src/validate/resultValidators/DefaultSuccess.js

@@ -18,14 +18,16 @@ export class DefaultSuccess extends ResultValidator {
    * @param {Object} context
    * @param {Validator[]} context.regularValidationResult
    * @param {Validator[]} context.prevValidationResult
+   * @param {Validator[]} context.prevShownValidationResult
    * @returns {boolean}
    */
   // eslint-disable-next-line class-methods-use-this
-  executeOnResults({ regularValidationResult, prevValidationResult }) {
+  executeOnResults({ regularValidationResult, prevShownValidationResult }) {
     const errorOrWarning = /** @param {Validator} v */ v =>
       v.type === 'error' || v.type === 'warning';
     const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length;
-    const prevHadErrorOrWarning = !!prevValidationResult.filter(errorOrWarning).length;
+    const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length;
-    return !hasErrorOrWarning && prevHadErrorOrWarning;
+
+    return !hasErrorOrWarning && hasShownErrorOrWarning;
   }
 }

packages/form-core/test-suites/ValidateMixin.suite.js

@@ -548,15 +548,18 @@ export function runValidateMixinSuite(customConfig) {
 
         /**
          *
-         * @param {{ regularValidationResult: Validator[], prevValidationResult: Validator[]}} param0
+         * @param {Object} context
+         * @param {Validator[]} context.regularValidationResult
+         * @param {Validator[]} context.prevShownValidationResult
+         * @returns {boolean}
          */
         // eslint-disable-next-line class-methods-use-this
-        executeOnResults({ regularValidationResult, prevValidationResult }) {
+        executeOnResults({ regularValidationResult, prevShownValidationResult }) {
           const errorOrWarning = /** @param {Validator} v */ v =>
             v.type === 'error' || v.type === 'warning';
           const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length;
-          const prevHadErrorOrWarning = !!prevValidationResult.filter(errorOrWarning).length;
+          const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length;
-          return !hasErrorOrWarning && prevHadErrorOrWarning;
+          return !hasErrorOrWarning && hasShownErrorOrWarning;
         }
       };
 
@@ -608,14 +611,16 @@ export function runValidateMixinSuite(customConfig) {
             >${lightDom}</${withSuccessTag}>
           `));
         const prevValidationResult = el.__prevValidationResult;
+        const prevShownValidationResult = el.__prevShownValidationResult;
         const regularValidationResult = [
           ...el.__syncValidationResult,
           ...el.__asyncValidationResult,
         ];
 
         expect(resultValidateSpy.args[0][0]).to.eql({
-          prevValidationResult,
           regularValidationResult,
+          prevValidationResult,
+          prevShownValidationResult,
         });
       });
 
@@ -1140,7 +1145,7 @@ export function runValidateMixinSuite(customConfig) {
             >${lightDom}</${elTag}>
           `));
 
-          const spy = sinon.spy(el, '_updateFeedbackComponent');
+          const spy = sinon.spy(el, '_updateShouldShowFeedbackFor');
           let counter = 0;
           // for ... of is already allowed we should update eslint...
           // eslint-disable-next-line no-restricted-syntax

packages/form-core/test/validate/ResultValidator.test.js

@@ -1,5 +1,6 @@
 import { expect } from '@open-wc/testing';
 import { ResultValidator } from '../../src/validate/ResultValidator.js';
+import { DefaultSuccess } from '../../src/validate/resultValidators/DefaultSuccess.js';
 import { Required } from '../../src/validate/validators/Required.js';
 import { MinLength } from '../../src/validate/validators/StringValidators.js';
 
@@ -16,17 +17,30 @@ describe('ResultValidator', () => {
        * @param {Object} context
        * @param {Validator[]} context.regularValidationResult
        * @param {Validator[]} context.prevValidationResult
+       * @param {Validator[]} context.prevShownValidationResult
        * @returns {boolean}
        */
-      executeOnResults({ regularValidationResult, prevValidationResult }) {
+      executeOnResults({ regularValidationResult, prevShownValidationResult }) {
-        const hasSuccess = regularValidationResult.length && !prevValidationResult.length;
+        const errorOrWarning = /** @param {Validator} v */ v =>
-        return !!hasSuccess;
+          v.type === 'error' || v.type === 'warning';
+        const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length;
+        const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length;
+
+        return !hasErrorOrWarning && hasShownErrorOrWarning;
       }
     }
     expect(
       new MyResultValidator().executeOnResults({
         regularValidationResult: [new Required(), new MinLength(3)],
         prevValidationResult: [],
+        prevShownValidationResult: [],
+      }),
+    ).to.be.false;
+    expect(
+      new MyResultValidator().executeOnResults({
+        regularValidationResult: [new DefaultSuccess()],
+        prevValidationResult: [new Required()],
+        prevShownValidationResult: [new Required()],
       }),
     ).to.be.true;
   });

packages/form-core/types/validate/ValidateMixinTypes.d.ts

@@ -38,6 +38,7 @@ export declare class ValidateHost {
   __asyncValidationResult: Validator[];
   __validationResult: Validator[];
   __prevValidationResult: Validator[];
+  __prevShownValidationResult: Validator[];
 
   connectedCallback(): void;
   disconnectedCallback(): void;

packages/form-integrations/test/form-validation-integrations.test.js

@@ -0,0 +1,114 @@
+import { expect, fixture, html, defineCE, unsafeStatic } from '@open-wc/testing';
+import { Required, DefaultSuccess, Validator } from '@lion/form-core';
+import { loadDefaultFeedbackMessages } from '@lion/validate-messages';
+import { LionInput } from '@lion/input';
+import sinon from 'sinon';
+
+describe('Form Validation Integrations', () => {
+  const lightDom = '';
+  before(() => {
+    loadDefaultFeedbackMessages();
+  });
+
+  describe('DefaultSuccess', () => {
+    it('does not show success feedback if the user is not recovering from a shown error/warning feedback', async () => {
+      class WarnValidator extends Validator {
+        /**
+         *
+         * @param {?} [param]
+         * @param {Object.<string,?>} [config]
+         */
+        constructor(param, config) {
+          super(param, config);
+          this.type = 'warning';
+        }
+
+        /** @param {string} value */
+        execute(value) {
+          let hasError = false;
+          if (value === 'warn') {
+            hasError = true;
+          }
+          return hasError;
+        }
+      }
+
+      class ValidateElementCustomTypes extends LionInput {
+        static get validationTypes() {
+          return ['error', 'warning', 'success'];
+        }
+      }
+      const elTagString = defineCE(ValidateElementCustomTypes);
+      const elTag = unsafeStatic(elTagString);
+      const el = /** @type {ValidateElementCustomTypes} */ (await fixture(html`
+        <${elTag}
+          .validators=${[
+            new Required(null, { getMessage: () => 'error' }),
+            new WarnValidator(null, { getMessage: () => 'warning' }),
+            new DefaultSuccess(),
+          ]}
+        >${lightDom}</${elTag}>
+      `));
+
+      expect(el._feedbackNode.feedbackData?.length).to.equal(0);
+
+      el.modelValue = 'w';
+      el.touched = true;
+      await el.updateComplete;
+      await el.feedbackComplete;
+      expect(el.showsFeedbackFor).to.eql([]);
+
+      el.modelValue = 'warn';
+      await el.updateComplete;
+      await el.feedbackComplete;
+      expect(el._feedbackNode.feedbackData?.[0].message).to.equal('warning');
+
+      el.modelValue = 'war';
+      await el.updateComplete;
+      await el.feedbackComplete;
+
+      expect([
+        'Okay',
+        'Correct',
+        'Succeeded',
+        'Ok!',
+        'This is right.',
+        'Changed!',
+        'Ok, correct.',
+      ]).to.include(
+        /** @type {{  message: string ;type: string; validator?: Validator | undefined;}[]} */ (el
+          ._feedbackNode.feedbackData)[0].message,
+      );
+
+      el.modelValue = '';
+      await el.updateComplete;
+      await el.feedbackComplete;
+      expect(el._feedbackNode.feedbackData?.[0].message).to.equal('error');
+
+      el.modelValue = 'war';
+      await el.updateComplete;
+      await el.feedbackComplete;
+
+      expect([
+        'Okay',
+        'Correct',
+        'Succeeded',
+        'Ok!',
+        'This is right.',
+        'Changed!',
+        'Ok, correct.',
+      ]).to.include(
+        /** @type {{  message: string ;type: string; validator?: Validator | undefined;}[]} */ (el
+          ._feedbackNode.feedbackData)[0].message,
+      );
+
+      // Check that change in focused or other interaction states does not refresh the success message
+      // without a change in validation results
+      const spy = sinon.spy(el, '_updateFeedbackComponent');
+      el._updateShouldShowFeedbackFor();
+      await el.updateComplete;
+      await el.feedbackComplete;
+      expect(spy.called).to.be.false;
+    });
+  });
+});