Merge pull request #1022 from ing-bank/feat/name-changes

fix(form-core): update child choice fields name attr to parent

.changeset/hungry-files-lay.md

@@ -0,0 +1,5 @@
+---
+'@lion/form-core': patch
+---
+
+Properly update formElements when the name attribute changes, in order to get an updated serializedValue.

.changeset/sour-apes-reply.md

@@ -0,0 +1,5 @@
+---
+'@lion/form-core': patch
+---
+
+Ensure that the name of a child choice field is always synced with the parent choice field(set) when it changes. No longer error when a child is added with a different name than the parent, simply sync it.

packages/form-core/src/FormControlMixin.js

@@ -23,8 +23,8 @@ function uuid(prefix) {
  * @typedef {import('@lion/core').nothing} nothing
  * @typedef {import('@lion/core/types/SlotMixinTypes').SlotsMap} SlotsMap
  * @typedef {import('../types/FormControlMixinTypes.js').FormControlMixin} FormControlMixin
- * @type {FormControlMixin}
  * @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
+ * @type {FormControlMixin}
  */
 const FormControlMixinImplementation = superclass =>
   // eslint-disable-next-line no-shadow, no-unused-vars
@@ -144,8 +144,7 @@ const FormControlMixinImplementation = superclass =>
      * @return {string}
      */
     get fieldName() {
-      // @ts-expect-error
+      return this.__fieldName || this.label || this.name || '';
-      return this.__fieldName || this.label || this.name; // FIXME: when LionField is typed we can inherit this prop
     }
 
     /**
@@ -195,6 +194,8 @@ const FormControlMixinImplementation = superclass =>
 
     constructor() {
       super();
+      /** @type {string | undefined} */
+      this.name = undefined;
       /** @type {string} */
       this._inputId = uuid(this.localName);
       /** @type {HTMLElement[]} */
@@ -257,6 +258,15 @@ const FormControlMixinImplementation = superclass =>
       if (changedProperties.has('helpText') && this._helpTextNode) {
         this._helpTextNode.textContent = this.helpText;
       }
+
+      if (changedProperties.has('name')) {
+        this.dispatchEvent(
+          new CustomEvent('form-element-name-changed', {
+            detail: { oldName: changedProperties.get('name'), newName: this.name },
+            bubbles: true,
+          }),
+        );
+      }
     }
 
     _triggerInitialModelValueChangedEvent() {

packages/form-core/src/choice-group/ChoiceGroupMixin.js

@@ -15,7 +15,6 @@ import { InteractionStateMixin } from '../InteractionStateMixin.js';
  * @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
  */
 const ChoiceGroupMixinImplementation = superclass =>
-  // @ts-expect-error
   class ChoiceGroupMixin extends FormRegistrarMixin(InteractionStateMixin(superclass)) {
     static get properties() {
       return {
@@ -121,6 +120,7 @@ const ChoiceGroupMixinImplementation = superclass =>
     constructor() {
       super();
       this.multipleChoice = false;
+      /** @type {'child'|'choice-group'|'fieldset'} */
       this._repropagationRole = 'choice-group'; // configures event propagation logic of FormControlMixin
 
       this.__isInitialModelValue = true;
@@ -156,6 +156,17 @@ const ChoiceGroupMixinImplementation = superclass =>
       });
     }
 
+    /** @param {import('lit-element').PropertyValues} changedProperties */
+    updated(changedProperties) {
+      super.updated(changedProperties);
+      if (changedProperties.has('name') && this.name !== changedProperties.get('name')) {
+        this.formElements.forEach(child => {
+          // eslint-disable-next-line no-param-reassign
+          child.name = this.name;
+        });
+      }
+    }
+
     disconnectedCallback() {
       super.disconnectedCallback();
 
@@ -171,7 +182,8 @@ const ChoiceGroupMixinImplementation = superclass =>
      */
     addFormElement(child, indexToInsertAt) {
       this._throwWhenInvalidChildModelValue(child);
-      this.__delegateNameAttribute(child);
+      // eslint-disable-next-line no-param-reassign
+      child.name = this.name;
       super.addFormElement(child, indexToInsertAt);
     }
 
@@ -283,24 +295,6 @@ const ChoiceGroupMixinImplementation = superclass =>
       }
     }
 
-    /**
-     * @param {FormControl} child
-     */
-    __delegateNameAttribute(child) {
-      if (!child.name || child.name === this.name) {
-        // eslint-disable-next-line no-param-reassign
-        child.name = this.name;
-      } else {
-        throw new Error(
-          `The ${this.tagName.toLowerCase()} name="${
-            this.name
-          }" does not allow to register ${child.tagName.toLowerCase()} with custom names (name="${
-            child.name
-          }" given)`,
-        );
-      }
-    }
-
     /**
      * @override FormControlMixin
      * @param {CustomEvent} ev

packages/form-core/src/choice-group/ChoiceInputMixin.js

@@ -110,6 +110,17 @@ const ChoiceInputMixinImplementation = superclass =>
       if (changedProperties.has('modelValue')) {
         this.__syncCheckedToInputElement();
       }
+
+      if (
+        changedProperties.has('name') &&
+        // @ts-expect-error not all choice inputs have a parent form group, since this mixin does not have a strict contract with the registration system
+        this.__parentFormGroup &&
+        // @ts-expect-error
+        this.__parentFormGroup.name !== this.name
+      ) {
+        // @ts-expect-error not all choice inputs have a name prop, because this mixin does not have a strict contract with form control mixin
+        this.name = changedProperties.get('name');
+      }
     }
 
     constructor() {

packages/form-core/src/registration/FormRegistrarMixin.js

@@ -50,11 +50,16 @@ const FormRegistrarMixinImplementation = superclass =>
       this._isFormOrFieldset = false;
 
       this._onRequestToAddFormElement = this._onRequestToAddFormElement.bind(this);
+      this._onRequestToChangeFormElementName = this._onRequestToChangeFormElementName.bind(this);
 
       this.addEventListener(
         'form-element-register',
         /** @type {EventListenerOrEventListenerObject} */ (this._onRequestToAddFormElement),
       );
+      this.addEventListener(
+        'form-element-name-changed',
+        /** @type {EventListenerOrEventListenerObject} */ (this._onRequestToChangeFormElementName),
+      );
     }
 
     /**
@@ -163,6 +168,17 @@ const FormRegistrarMixinImplementation = superclass =>
       this.addFormElement(child, indexToInsertAt);
     }
 
+    /**
+     * @param {CustomEvent} ev
+     */
+    _onRequestToChangeFormElementName(ev) {
+      const element = this.formElements[ev.detail.oldName];
+      if (element) {
+        this.formElements[ev.detail.newName] = element;
+        delete this.formElements[ev.detail.oldName];
+      }
+    }
+
     /**
      * @param {CustomEvent} ev
      */

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

@@ -390,7 +390,7 @@ export function runValidateMixinFeedbackPart() {
           params: 4,
           modelValue: 'cat',
           formControl: el,
-          fieldName: undefined,
+          fieldName: '',
           type: 'x',
           name: 'MinLength',
         });
@@ -414,7 +414,7 @@ export function runValidateMixinFeedbackPart() {
           params: 4,
           modelValue: 'cat',
           formControl: el,
-          fieldName: undefined,
+          fieldName: '',
           type: 'error',
           name: 'MinLength',
         });

packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js

@@ -74,7 +74,7 @@ export function runChoiceGroupMixinSuite({ parentTagString, childTagString } = {
       );
     });
 
-    it('automatically sets the name property of child radios to its own name', async () => {
+    it('automatically sets the name property of child fields to its own name', async () => {
       const el = /** @type {ChoiceGroup} */ (await fixture(html`
         <${parentTag} name="gender">
           <${childTag} .choiceValue=${'female'} checked></${childTag}>
@@ -93,7 +93,43 @@ export function runChoiceGroupMixinSuite({ parentTagString, childTagString } = {
       expect(el.formElements[2].name).to.equal('gender');
     });
 
-    it('throws if a child element with a different name than the group tries to register', async () => {
+    it('automatically updates the name property of child fields to its own name', async () => {
+      const el = /** @type {ChoiceGroup} */ (await fixture(html`
+        <${parentTag} name="gender">
+          <${childTag}></${childTag}>
+          <${childTag}></${childTag}>
+        </${parentTag}>
+      `));
+
+      expect(el.formElements[0].name).to.equal('gender');
+      expect(el.formElements[1].name).to.equal('gender');
+
+      el.name = 'gender2';
+
+      await el.updateComplete;
+
+      expect(el.formElements[0].name).to.equal('gender2');
+      expect(el.formElements[1].name).to.equal('gender2');
+    });
+
+    it('prevents updating the name property of a child if it is different from its parent', async () => {
+      const el = /** @type {ChoiceGroup} */ (await fixture(html`
+        <${parentTag} name="gender">
+          <${childTag}></${childTag}>
+          <${childTag}></${childTag}>
+        </${parentTag}>
+      `));
+
+      expect(el.formElements[0].name).to.equal('gender');
+      expect(el.formElements[1].name).to.equal('gender');
+
+      el.formElements[0].name = 'gender2';
+
+      await el.formElements[0].updateComplete;
+      expect(el.formElements[0].name).to.equal('gender');
+    });
+
+    it('adjusts the name of a child element if it has a different name than the group', async () => {
       const el = /** @type {ChoiceGroup} */ (await fixture(html`
         <${parentTag} name="gender">
           <${childTag} .choiceValue=${'female'} checked></${childTag}>
@@ -104,12 +140,9 @@ export function runChoiceGroupMixinSuite({ parentTagString, childTagString } = {
       const invalidChild = /** @type {ChoiceGroup} */ (await fixture(html`
         <${childTag} name="foo" .choiceValue=${'male'}></${childTag}>
       `));
-
-      expect(() => {
       el.addFormElement(invalidChild);
-      }).to.throw(
+      await invalidChild.updateComplete;
-        'The choice-group name="gender" does not allow to register choice-group-input with custom names (name="foo" given)',
+      expect(invalidChild.name).to.equal('gender');
-      );
     });
 
     it('can set initial modelValue on creation', async () => {

packages/form-core/test-suites/form-group/FormGroupMixin.suite.js

@@ -18,26 +18,27 @@ import { FormGroupMixin } from '../../src/form-group/FormGroupMixin.js';
  * @param {{ tagString?: string, childTagString?:string }} [cfg]
  */
 export function runFormGroupMixinSuite(cfg = {}) {
-  const FormChild = class extends LionField {
+  class FormChild extends LionField {
     get slots() {
       return {
         ...super.slots,
         input: () => document.createElement('input'),
       };
     }
-  };
+  }
 
   const childTagString = cfg.childTagString || defineCE(FormChild);
 
-  // @ts-expect-error
+  // @ts-expect-error base constructors same return type
-  const FormGroup = class extends FormGroupMixin(LitElement) {
+  class FormGroup extends FormGroupMixin(LitElement) {
     constructor() {
       super();
       /** @override from FormRegistrarMixin */
       this._isFormOrFieldset = true;
+      /** @type {'child'|'choice-group'|'fieldset'} */
       this._repropagationRole = 'fieldset'; // configures FormControlMixin
     }
-  };
+  }
 
   const tagString = cfg.tagString || defineCE(FormGroup);
   const tag = unsafeStatic(tagString);
@@ -804,6 +805,19 @@ export function runFormGroupMixinSuite(cfg = {}) {
           },
         });
       });
+
+      it('updates the formElements keys when a name attribute changes', async () => {
+        const fieldset = /**  @type {FormGroup} */ (await fixture(html`
+          <${tag}>
+            <${childTag} name="foo" .modelValue=${'qux'}></${childTag}>
+          </${tag}>
+        `));
+        expect(fieldset.serializedValue.foo).to.equal('qux');
+        fieldset.formElements[0].name = 'bar';
+        await fieldset.updateComplete;
+        await fieldset.formElements[0].updateComplete;
+        expect(fieldset.serializedValue.bar).to.equal('qux');
+      });
     });
 
     describe('Reset', () => {

packages/form-core/types/FormControlMixinTypes.d.ts

@@ -10,7 +10,7 @@ declare interface HTMLElementWithValue extends HTMLElement {
   value: string;
 }
 
-export class FormControlHost {
+export declare class FormControlHost {
   static get styles(): CSSResult | CSSResult[];
   /**
    * A Boolean attribute which, if present, indicates that the user should not be able to edit
@@ -129,7 +129,7 @@ export declare function FormControlImplementation<T extends Constructor<LitEleme
   superclass: T,
 ): T &
   Constructor<FormControlHost> &
-  FormControlHost &
+  typeof FormControlHost &
   Constructor<FormRegisteringHost> &
   typeof FormRegisteringHost &
   Constructor<DisabledHost> &

packages/form-core/types/form-group/FormGroupMixinTypes.d.ts

@@ -15,7 +15,7 @@ export declare class FormGroupHost {
   touched: boolean;
   dirty: boolean;
   submitted: boolean;
-  serializedValue: string;
+  serializedValue: { [key: string]: any };
   modelValue: { [x: string]: any };
   formattedValue: string;
   children: Array<HTMLElement & FormControlHost>;

packages/listbox/test-suites/ListboxMixin.suite.js

@@ -96,24 +96,6 @@ export function runListboxMixinSuite(customConfig = {}) {
         );
       });
 
-      it('throws if a child element with a different name than the group tries to register', async () => {
-        const el = await fixture(html`
-        <${tag} name="gender">
-          <${optionTag} .choiceValue=${'female'} checked></${optionTag}>
-          <${optionTag} .choiceValue=${'other'}></${optionTag}>
-        </${tag}>
-      `);
-        const invalidChild = await fixture(html`
-        <${optionTag} name="foo" .choiceValue=${'male'}></${optionTag}>
-      `);
-
-        expect(() => {
-          el.addFormElement(invalidChild);
-        }).to.throw(
-          `The ${cfg.tagString} name="gender" does not allow to register lion-option with custom names (name="foo" given)`,
-        );
-      });
-
       it('can set initial modelValue on creation', async () => {
         const el = await fixture(html`
         <${tag} name="gender" .modelValue=${'other'}>