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.

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

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

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

@@ -161,8 +161,8 @@ export const ValidateMixinImplementation = superclass =>
        * @type {Validator[]}
        */
       this.__validationResult = [];
-      /** @type {Validator[]} */
+      /** @type {string} */
-      this.__prevValidationResult = [];
+      this.__prevShownValidationFeedback = '';
 
       this.__onValidatorUpdated = this.__onValidatorUpdated.bind(this);
       this._updateFeedbackComponent = this._updateFeedbackComponent.bind(this);
@@ -279,7 +279,6 @@ export const ValidateMixinImplementation = superclass =>
         return;
       }
 
-      this.__storePrevResult();
       if (clearCurrentResult) {
         // Clear ('invalidate') all pending and existing validation results.
         // This is needed because we have async (pending) validators whose results
@@ -289,10 +288,6 @@ export const ValidateMixinImplementation = superclass =>
       await this.__executeValidators();
     }
 
-    __storePrevResult() {
-      this.__prevValidationResult = this.__validationResult;
-    }
-
     /**
      * @desc step A1-3 + B (as explained in 'validate')
      */
@@ -401,7 +396,7 @@ export const ValidateMixinImplementation = superclass =>
       return resultValidators.filter(v =>
         v.executeOnResults({
           regularValidationResult,
-          prevValidationResult: this.__prevValidationResult,
+          prevShownValidationFeedback: this.__prevShownValidationFeedback,
         }),
       );
     }
@@ -582,8 +577,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.__prevShownValidationFeedback = this.__prioritizedResult[0].type;
+          }
+
+          const messageMap = await this.__getFeedbackMessages(this.__prioritizedResult);
           _feedbackNode.feedbackData = messageMap.length ? messageMap : [];
         });
       } else {
@@ -636,10 +635,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

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

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

@@ -548,15 +548,21 @@ export function runValidateMixinSuite(customConfig) {
 
         /**
          *
-         * @param {{ regularValidationResult: Validator[], prevValidationResult: Validator[]}} param0
+         * @param {Object} context
+         * @param {Validator[]} context.regularValidationResult
+         * @param {string} context.prevShownValidationFeedback
+         * @returns {boolean}
          */
         // eslint-disable-next-line class-methods-use-this
-        executeOnResults({ regularValidationResult, prevValidationResult }) {
+        executeOnResults({ regularValidationResult, prevShownValidationFeedback }) {
           const errorOrWarning = /** @param {Validator} v */ v =>
             v.type === 'error' || v.type === 'warning';
           const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length;
-          const prevHadErrorOrWarning = !!prevValidationResult.filter(errorOrWarning).length;
+
-          return !hasErrorOrWarning && prevHadErrorOrWarning;
+          return (
+            !hasErrorOrWarning &&
+            (prevShownValidationFeedback === 'error' || prevShownValidationFeedback === 'warning')
+          );
         }
       };
 
@@ -607,15 +613,15 @@ export function runValidateMixinSuite(customConfig) {
               .modelValue=${'myValue'}
             >${lightDom}</${withSuccessTag}>
           `));
-        const prevValidationResult = el.__prevValidationResult;
+        const prevShownValidationFeedback = el.__prevShownValidationFeedback;
         const regularValidationResult = [
           ...el.__syncValidationResult,
           ...el.__asyncValidationResult,
         ];
 
         expect(resultValidateSpy.args[0][0]).to.eql({
-          prevValidationResult,
           regularValidationResult,
+          prevShownValidationFeedback,
         });
       });
 
@@ -1140,7 +1146,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

@@ -15,18 +15,19 @@ describe('ResultValidator', () => {
        *
        * @param {Object} context
        * @param {Validator[]} context.regularValidationResult
-       * @param {Validator[]} context.prevValidationResult
+       * @param {string} context.prevShownValidationFeedback
        * @returns {boolean}
        */
-      executeOnResults({ regularValidationResult, prevValidationResult }) {
+      executeOnResults({ regularValidationResult, prevShownValidationFeedback }) {
-        const hasSuccess = regularValidationResult.length && !prevValidationResult.length;
+        const hasSuccess =
+          regularValidationResult.length && prevShownValidationFeedback === 'error';
         return !!hasSuccess;
       }
     }
     expect(
       new MyResultValidator().executeOnResults({
         regularValidationResult: [new Required(), new MinLength(3)],
-        prevValidationResult: [],
+        prevShownValidationFeedback: 'error',
       }),
     ).to.be.true;
   });

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

@@ -37,7 +37,7 @@ export declare class ValidateHost {
   __syncValidationResult: Validator[];
   __asyncValidationResult: Validator[];
   __validationResult: Validator[];
-  __prevValidationResult: Validator[];
+  __prevShownValidationFeedback: string;
 
   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;
+    });
+  });
+});