fix(form-core): registr. complete cb before initial interaction states

ing-bank · Apr 7, 2021 · 38297d0 · 38297d0
1 parent 5354e1a
commit 38297d0
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 65 deletions.
diff --git a/.changeset/cold-spies-press.md b/.changeset/cold-spies-press.md
@@ -0,0 +1,8 @@
+---
+'@lion/form-core': patch
+'@lion/form-integrations': patch
+---
+
+## Bug fixes
+
+**form-core**: registrationComplete callback executed before initial interaction states are computed
diff --git a/packages/form-core/src/choice-group/ChoiceGroupMixin.js b/packages/form-core/src/choice-group/ChoiceGroupMixin.js
@@ -11,6 +11,12 @@ import { InteractionStateMixin } from '../InteractionStateMixin.js';
  */
 
 /**
+ * ChoiceGroupMixin applies on both Fields (listbox/select-rich/combobox)  and FormGroups
+ * (radio-group, checkbox-group)
+ * TODO: Ideally, the ChoiceGroupMixin should not depend on InteractionStateMixin, which is only
+ * designed for usage with Fields, in other words: their interaction states are not derived from
+ * children events, like in FormGroups
+ *
  * @type {ChoiceGroupMixin}
  * @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
  */
@@ -53,8 +59,8 @@ const ChoiceGroupMixinImplementation = superclass =>
       };
 
       if (this.__isInitialModelValue) {
-        this.__isInitialModelValue = false;
         this.registrationComplete.then(() => {
+          this.__isInitialModelValue = false;
           this._setCheckedElements(value, checkCondition);
           this.requestUpdate('modelValue', this.__oldModelValue);
         });
@@ -89,8 +95,8 @@ const ChoiceGroupMixinImplementation = superclass =>
       const checkCondition = (el, val) => el.serializedValue.value === val;
 
       if (this.__isInitialSerializedValue) {
-        this.__isInitialSerializedValue = false;
         this.registrationComplete.then(() => {
+          this.__isInitialSerializedValue = false;
           this._setCheckedElements(value, checkCondition);
           this.requestUpdate('serializedValue');
         });
@@ -116,8 +122,8 @@ const ChoiceGroupMixinImplementation = superclass =>
       const checkCondition = (el, val) => el.formattedValue === val;
 
       if (this.__isInitialFormattedValue) {
-        this.__isInitialFormattedValue = false;
         this.registrationComplete.then(() => {
+          this.__isInitialFormattedValue = false;
           this._setCheckedElements(value, checkCondition);
         });
       } else {
@@ -138,34 +144,10 @@ const ChoiceGroupMixinImplementation = superclass =>
       this.__isInitialSerializedValue = true;
       /** @private */
       this.__isInitialFormattedValue = true;
-      /** @type {Promise<any> & {done?:boolean}} */
-      this.registrationComplete = new Promise((resolve, reject) => {
-        /** @private */
-        this.__resolveRegistrationComplete = resolve;
-        /** @private */
-        this.__rejectRegistrationComplete = reject;
-      });
-      this.registrationComplete.done = false;
-      this.registrationComplete.then(
-        () => {
-          this.registrationComplete.done = true;
-        },
-        () => {
-          this.registrationComplete.done = true;
-          throw new Error(
-            'Registration could not finish. Please use await el.registrationComplete;',
-          );
-        },
-      );
     }
 
     connectedCallback() {
       super.connectedCallback();
-      // Double microtask queue to account for Webkit race condition
-      Promise.resolve().then(() =>
-        // @ts-ignore
-        Promise.resolve().then(() => this.__resolveRegistrationComplete()),
-      );
 
       this.registrationComplete.then(() => {
         this.__isInitialModelValue = false;
@@ -174,6 +156,14 @@ const ChoiceGroupMixinImplementation = superclass =>
       });
     }
 
+    /**
+     * @enhance FormRegistrarMixin
+     */
+    _completeRegistration() {
+      // Double microtask queue to account for Webkit race condition
+      Promise.resolve().then(() => super._completeRegistration());
+    }
+
     /** @param {import('@lion/core').PropertyValues} changedProperties */
     updated(changedProperties) {
       super.updated(changedProperties);
@@ -185,18 +175,6 @@ const ChoiceGroupMixinImplementation = superclass =>
       }
     }
 
-    disconnectedCallback() {
-      super.disconnectedCallback();
-
-      if (this.registrationComplete.done === false) {
-        Promise.resolve().then(() => {
-          Promise.resolve().then(() => {
-            this.__rejectRegistrationComplete();
-          });
-        });
-      }
-    }
-
     /**
      * @override from FormRegistrarMixin
      * @param {FormControl} child

diff --git a/packages/form-core/src/form-group/FormGroupMixin.js b/packages/form-core/src/form-group/FormGroupMixin.js
@@ -146,32 +146,13 @@ const FormGroupMixinImplementation = superclass =>
       this.addEventListener('validate-performed', this.__onChildValidatePerformed);
 
       this.defaultValidators = [new FormElementsHaveNoError()];
-      /** @type {Promise<any> & {done?:boolean}} */
-      this.registrationComplete = new Promise((resolve, reject) => {
-        this.__resolveRegistrationComplete = resolve;
-        this.__rejectRegistrationComplete = reject;
-      });
-      this.registrationComplete.done = false;
-      this.registrationComplete.then(
-        () => {
-          this.registrationComplete.done = true;
-        },
-        () => {
-          this.registrationComplete.done = true;
-          throw new Error(
-            'Registration could not finish. Please use await el.registrationComplete;',
-          );
-        },
-      );
     }
 
     connectedCallback() {
       super.connectedCallback();
       this.setAttribute('role', 'group');
-      // @ts-ignore
-      Promise.resolve().then(() => this.__resolveRegistrationComplete());
 
-      this.registrationComplete.then(() => {
+      this.initComplete.then(() => {
         this.__isInitialModelValue = false;
         this.__isInitialSerializedValue = false;
         this.__initInteractionStates();
@@ -185,11 +166,6 @@ const FormGroupMixinImplementation = superclass =>
         document.removeEventListener('click', this._checkForOutsideClick);
         this.__hasActiveOutsideClickHandling = false;
       }
-      if (this.registrationComplete.done === false) {
-        Promise.resolve().then(() => {
-          this.__rejectRegistrationComplete();
-        });
-      }
     }
 
     __initInteractionStates() {

diff --git a/packages/form-core/src/registration/FormRegistrarMixin.js b/packages/form-core/src/registration/FormRegistrarMixin.js
@@ -60,6 +60,61 @@ const FormRegistrarMixinImplementation = superclass =>
         'form-element-name-changed',
         /** @type {EventListenerOrEventListenerObject} */ (this._onRequestToChangeFormElementName),
       );
+
+      /**
+       * initComplete resolves after all pending initialization logic
+       * (for instance `<form-group .serializedValue=${{ child1: 'a', child2: 'b' }}>`)
+       * is executed
+       * @type {Promise<any>}
+       */
+      this.initComplete = new Promise((resolve, reject) => {
+        this.__resolveInitComplete = resolve;
+        this.__rejectInitComplete = reject;
+      });
+
+      /**
+       * registrationComplete waits for all children formElements to have registered
+       * @type {Promise<any> & {done?:boolean}}
+       */
+      this.registrationComplete = new Promise((resolve, reject) => {
+        this.__resolveRegistrationComplete = resolve;
+        this.__rejectRegistrationComplete = reject;
+      });
+      this.registrationComplete.done = false;
+      this.registrationComplete.then(
+        () => {
+          this.registrationComplete.done = true;
+          this.__resolveInitComplete(undefined);
+        },
+        () => {
+          this.registrationComplete.done = true;
+          this.__rejectInitComplete(undefined);
+          throw new Error(
+            'Registration could not finish. Please use await el.registrationComplete;',
+          );
+        },
+      );
+    }
+
+    connectedCallback() {
+      super.connectedCallback();
+      this._completeRegistration();
+    }
+
+    _completeRegistration() {
+      Promise.resolve().then(() => this.__resolveRegistrationComplete(undefined));
+    }
+
+    disconnectedCallback() {
+      super.disconnectedCallback();
+
+      if (this.registrationComplete.done === false) {
+        Promise.resolve().then(() => {
+          Promise.resolve().then(() => {
+            this.__rejectRegistrationComplete();
+          });
+        });
+      }
     }
 
     /**

diff --git a/packages/form-core/types/registration/FormRegistrarMixinTypes.d.ts b/packages/form-core/types/registration/FormRegistrarMixinTypes.d.ts
@@ -22,6 +22,8 @@ export declare class FormRegistrarHost {
   _onRequestToAddFormElement(e: CustomEvent): void;
   isRegisteredFormElement(el: FormControlHost): boolean;
   registrationComplete: Promise<boolean>;
+  initComplete: Promise<boolean>;
+  protected _completeRegistration(): void;
 }
 
 export declare function FormRegistrarImplementation<T extends Constructor<LitElement>>(

diff --git a/packages/form-integrations/test/form-integrations.test.js b/packages/form-integrations/test/form-integrations.test.js
@@ -81,6 +81,7 @@ describe('Form Integrations', () => {
         ></umbrella-form>`,
       ));
 
+      await el._lionFormNode.initComplete;
       expect(el._lionFormNode.dirty).to.be.false;
     });
   });

diff --git a/packages/form-integrations/test/helpers/umbrella-form.js b/packages/form-integrations/test/helpers/umbrella-form.js
@@ -25,6 +25,9 @@ export class UmbrellaForm extends LitElement {
     ));
   }
 
+  /**
+   * @param {string} v
+   */
   set serializedValue(v) {
     this.__serializedValue = v;
   }
@@ -141,7 +144,8 @@ export class UmbrellaForm extends LitElement {
             <lion-button
               type="button"
               raised
-              @click=${ev =>
+              @click=${(/** @type {Event} */ ev) =>
+                // @ts-ignore
                 ev.currentTarget.parentElement.parentElement.parentElement.resetGroup()}
               >Reset</lion-button
             >