fix(form-core): cleanup group > child descriptions and reenable tests

.changeset/rude-ducks-hide.md

@@ -0,0 +1,8 @@
+---
+'@lion/form-core': patch
+---
+
+**form-core**:
+
+- cleanup group > child descriptions on disconnectedCallback
+- reenable tests

packages/form-core/src/FormControlMixin.js

@@ -692,6 +692,8 @@ const FormControlMixinImplementation = superclass =>
     }
 
     /**
+     * This function exposes descripion elements that a FormGroup should expose to its
+     * children. See FormGroupMixin.__getAllDescriptionElementsInParentChain()
      * @return {Array.<HTMLElement|undefined>}
      * @protected
      */
@@ -748,7 +750,7 @@ const FormControlMixinImplementation = superclass =>
     }
 
     /**
-     * Meant for Application Developers wanting to delete from aria-labelledby attribute.
+     * Meant for Application Developers wanting to delete from aria-describedby attribute.
      * @param {HTMLElement} element
      */
     removeFromAriaDescribedBy(element) {

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

@@ -114,9 +114,7 @@ const ChoiceInputMixinImplementation = superclass =>
 
       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
       ) {
         this._syncNameToParentFormGroup();
@@ -251,7 +249,6 @@ const ChoiceInputMixinImplementation = superclass =>
     _syncNameToParentFormGroup() {
       // @ts-expect-error not all choice inputs have a name prop, because this mixin does not have a strict contract with form control mixin
       if (this._parentFormGroup.tagName.includes(this.tagName)) {
-        // @ts-expect-error
         this.name = this._parentFormGroup.name;
       }
     }

packages/form-core/src/form-group/FormGroupMixin.js

@@ -126,7 +126,7 @@ const FormGroupMixinImplementation = superclass =>
 
     constructor() {
       super();
-      // inputNode = this, which always requires a value prop
+      // ._inputNode = this, which always requires a value prop
       this.value = '';
 
       this.disabled = false;
@@ -146,6 +146,8 @@ const FormGroupMixinImplementation = superclass =>
       this.addEventListener('validate-performed', this.__onChildValidatePerformed);
 
       this.defaultValidators = [new FormElementsHaveNoError()];
+
+      this.__descriptionElementsInParentChain = new Set();
     }
 
     connectedCallback() {
@@ -166,6 +168,7 @@ const FormGroupMixinImplementation = superclass =>
         document.removeEventListener('click', this._checkForOutsideClick);
         this.__hasActiveOutsideClickHandling = false;
       }
+      this.__descriptionElementsInParentChain.clear();
     }
 
     __initInteractionStates() {
@@ -425,20 +428,58 @@ const FormGroupMixinImplementation = superclass =>
     }
 
     /**
-     * @param {FormControl} child
+     * Traverses the _parentFormGroup tree, and gathers all aria description elements
+     * (feedback and helptext) that should be provided to children.
+     *
+     * In the example below, when the input for 'street' has focus, a screenreader user
+     * would hear the #group-error.
+     * In case one of the inputs was in error state as well, the SR user would
+     * first hear the local error, followed by #group-error
+     * @example
+     * <lion-fieldset name="address">
+     *   <lion-input name="street" label="Street" .modelValue="${'Park Avenue'}"></lion-input>
+     *   <lion-input name="number" label="Number" .modelValue="${100}">...</lion-input>
+     *   <div slot="feedback" id="group-error">
+     *      Park Avenue only has numbers up to 80
+     *   </div>
+     * </lion-fieldset>
      */
-    __linkChildrenMessagesToParent(child) {
+    __storeAllDescriptionElementsInParentChain() {
-      // aria-describedby of (nested) children
       const unTypedThis = /** @type {unknown} */ (this);
       let parent = /** @type {FormControlHost & { _parentFormGroup:any }} */ (unTypedThis);
-      const ctor = /** @type {typeof FormGroupMixin} */ (this.constructor);
       while (parent) {
-        ctor._addDescriptionElementIdsToField(child, parent._getAriaDescriptionElements());
+        const descriptionElements = parent._getAriaDescriptionElements();
+        const orderedEls = getAriaElementsInRightDomOrder(descriptionElements, { reverse: true });
+        orderedEls.forEach(el => {
+          this.__descriptionElementsInParentChain.add(el);
+        });
         // Also check if the newly added child needs to refer grandparents
         parent = parent._parentFormGroup;
       }
     }
 
+    /**
+     * @param {FormControl} child
+     */
+    __linkParentMessages(child) {
+      this.__descriptionElementsInParentChain.forEach(el => {
+        if (typeof child.addToAriaDescribedBy === 'function') {
+          child.addToAriaDescribedBy(el, { reorder: false });
+        }
+      });
+    }
+
+    /**
+     * @param {FormControl} child
+     */
+    __unlinkParentMessages(child) {
+      this.__descriptionElementsInParentChain.forEach(el => {
+        if (typeof child.removeFromAriaDescribedBy === 'function') {
+          child.removeFromAriaDescribedBy(el);
+        }
+      });
+    }
+
     /**
      * @override of FormRegistrarMixin.
      * @desc Connects ValidateMixin and DisabledMixin
@@ -451,8 +492,10 @@ const FormGroupMixinImplementation = superclass =>
       if (this.disabled) {
         child.makeRequestToBeDisabled();
       }
-      // TODO: Unlink in removeFormElement
+      if (!this.__descriptionElementsInParentChain.size) {
-      this.__linkChildrenMessagesToParent(child);
+        this.__storeAllDescriptionElementsInParentChain();
+      }
+      this.__linkParentMessages(child);
       this.validate({ clearCurrentResult: true });
 
       if (typeof child.addToAriaLabelledBy === 'function' && this._labelNode) {
@@ -468,24 +511,9 @@ const FormGroupMixinImplementation = superclass =>
       return this._getFromAllFormElements('_initialModelValue');
     }
 
-    /**
-     * Add aria-describedby to child element(field), so that it points to feedback/help-text of
-     * parent(fieldset)
-     * @param {FormControl} field - the child: lion-field/lion-input/lion-textarea
-     * @param {HTMLElement[]} descriptionElements  - description elements like feedback and help-text
-     */
-    static _addDescriptionElementIdsToField(field, descriptionElements) {
-      const orderedEls = getAriaElementsInRightDomOrder(descriptionElements, { reverse: true });
-      orderedEls.forEach(el => {
-        if (field.addToAriaDescribedBy) {
-          field.addToAriaDescribedBy(el, { reorder: false });
-        }
-      });
-    }
-
     /**
      * @override of FormRegistrarMixin. Connects ValidateMixin
-     * @param {FormRegisteringHost & FormControlHost} el
+     * @param {FormRegisteringHost & FormControl} el
      */
     removeFormElement(el) {
       super.removeFormElement(el);
@@ -494,6 +522,7 @@ const FormGroupMixinImplementation = superclass =>
       if (typeof el.removeFromAriaLabelledBy === 'function' && this._labelNode) {
         el.removeFromAriaLabelledBy(this._labelNode, { reorder: false });
       }
+      this.__unlinkParentMessages(el);
     }
   };
 

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

@@ -131,6 +131,7 @@ const FormRegistrarMixinImplementation = superclass =>
      */
     addFormElement(child, indexToInsertAt) {
       // This is a way to let the child element (a lion-fieldset or lion-field) know, about its parent
+      // @ts-expect-error FormControl needs to be at the bottom of the hierarchy
       // eslint-disable-next-line no-param-reassign
       child._parentFormGroup = this;
 

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

@@ -11,7 +11,6 @@ customElements.define('choice-input-foo', ChoiceInputFoo);
 class ChoiceInputBar extends ChoiceInputMixin(LionInput) {
   _syncNameToParentFormGroup() {
     // Always sync, without conditions
-    // @ts-expect-error
     this.name = this._parentFormGroup.name;
   }
 }

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

@@ -5,6 +5,10 @@ import { LionInput } from '@lion/input';
 import '@lion/form-core/define';
 import { FormGroupMixin } from '../../src/form-group/FormGroupMixin.js';
 
+/**
+ * @typedef {import('@lion/form-core').LionField} LionField
+ */
+
 /**
  * @param {{ tagString?: string, childTagString?:string }} [cfg]
  */
@@ -140,8 +144,11 @@ export function runFormGroupMixinInputSuite(cfg = {}) {
         return dom;
       };
 
+      childAriaTest = async (
         // eslint-disable-next-line no-shadow
-      childAriaTest = (/** @type {FormGroup} */ childAriaFixture) => {
+        /** @type {FormGroup} */ childAriaFixture,
+        { cleanupPhase = false } = {},
+      ) => {
         /* eslint-disable camelcase */
         // Message elements: all elements pointed at by inputs
         const msg_l1_g = /** @type {FormGroup} */ (childAriaFixture.querySelector('#msg_l1_g'));
@@ -152,21 +159,20 @@ export function runFormGroupMixinInputSuite(cfg = {}) {
         const msg_l2_fb = /** @type {FormChild} */ (childAriaFixture.querySelector('#msg_l2_fb'));
 
         // Field elements: all inputs pointing to message elements
-        const input_l1_fa = /** @type {FormChild} */ (childAriaFixture.querySelector(
+        const input_l1_fa = /** @type {HTMLInputElement} */ (childAriaFixture.querySelector(
           'input[name=l1_fa]',
         ));
-        const input_l1_fb = /** @type {FormChild} */ (childAriaFixture.querySelector(
+        const input_l1_fb = /** @type {HTMLInputElement} */ (childAriaFixture.querySelector(
           'input[name=l1_fb]',
         ));
-        const input_l2_fa = /** @type {FormChild} */ (childAriaFixture.querySelector(
+        const input_l2_fa = /** @type {HTMLInputElement} */ (childAriaFixture.querySelector(
           'input[name=l2_fa]',
         ));
-        const input_l2_fb = /** @type {FormChild} */ (childAriaFixture.querySelector(
+        const input_l2_fb = /** @type {HTMLInputElement} */ (childAriaFixture.querySelector(
           'input[name=l2_fb]',
         ));
 
-        /* eslint-enable camelcase */
+        if (!cleanupPhase) {
-
           // 'L1' fields (inside lion-fieldset[name="l1_g"]) should point to l1(group) msg
           expect(input_l1_fa.getAttribute('aria-describedby')).to.contain(
             msg_l1_g.id,
@@ -210,17 +216,95 @@ export function runFormGroupMixinInputSuite(cfg = {}) {
             // @ts-expect-error
             dB.indexOf(msg_l2_fb.id) < dB.indexOf(msg_l2_g.id) < dB.indexOf(msg_l1_g.id),
           ).to.equal(true, 'order of ids');
+        } else {
+          // cleanupPhase
+          const control_l1_fa = /** @type {LionField} */ (childAriaFixture.querySelector(
+            '[name=l1_fa]',
+          ));
+          const control_l1_fb = /** @type {LionField} */ (childAriaFixture.querySelector(
+            '[name=l1_fb]',
+          ));
+          const control_l2_fa = /** @type {LionField} */ (childAriaFixture.querySelector(
+            '[name=l2_fa]',
+          ));
+          const control_l2_fb = /** @type {LionField} */ (childAriaFixture.querySelector(
+            '[name=l2_fb]',
+          ));
+
+          // @ts-expect-error removeChild should always be inherited via LitElement?
+          control_l1_fa._parentFormGroup.removeChild(control_l1_fa);
+          await control_l1_fa.updateComplete;
+          // @ts-expect-error removeChild should always be inherited via LitElement?
+          control_l1_fb._parentFormGroup.removeChild(control_l1_fb);
+          await control_l1_fb.updateComplete;
+          // @ts-expect-error removeChild should always be inherited via LitElement?
+          control_l2_fa._parentFormGroup.removeChild(control_l2_fa);
+          await control_l2_fa.updateComplete;
+          // @ts-expect-error removeChild should always be inherited via LitElement?
+          control_l2_fb._parentFormGroup.removeChild(control_l2_fb);
+          await control_l2_fb.updateComplete;
+
+          // 'L1' fields (inside lion-fieldset[name="l1_g"]) should point to l1(group) msg
+          expect(input_l1_fa.getAttribute('aria-describedby')).to.not.contain(
+            msg_l1_g.id,
+            'l1 input(a) refers parent/group',
+          );
+          expect(input_l1_fb.getAttribute('aria-describedby')).to.not.contain(
+            msg_l1_g.id,
+            'l1 input(b) refers parent/group',
+          );
+
+          // Also check that aria-describedby of the inputs are not overridden (this relation was
+          // put there in lion-input(using lion-field)).
+          expect(input_l1_fa.getAttribute('aria-describedby')).to.contain(
+            msg_l1_fa.id,
+            'l1 input(a) refers local field',
+          );
+          expect(input_l1_fb.getAttribute('aria-describedby')).to.contain(
+            msg_l1_fb.id,
+            'l1 input(b) refers local field',
+          );
+
+          // Also make feedback element point to nested fieldset inputs
+          expect(input_l2_fa.getAttribute('aria-describedby')).to.not.contain(
+            msg_l1_g.id,
+            'l2 input(a) refers grandparent/group.group',
+          );
+          expect(input_l2_fb.getAttribute('aria-describedby')).to.not.contain(
+            msg_l1_g.id,
+            'l2 input(b) refers grandparent/group.group',
+          );
+
+          // Check cleanup of FormGroup on disconnect
+          const l2_g = /** @type {FormGroup} */ (childAriaFixture.querySelector('[name=l2_g]'));
+          expect(l2_g.__descriptionElementsInParentChain.size).to.not.equal(0);
+          // @ts-expect-error removeChild should always be inherited via LitElement?
+          l2_g._parentFormGroup.removeChild(l2_g);
+          await l2_g.updateComplete;
+          expect(l2_g.__descriptionElementsInParentChain.size).to.equal(0);
+        }
+        /* eslint-enable camelcase */
       };
+    });
 
     it(`reads feedback message belonging to fieldset when child input is focused
         (via aria-describedby)`, async () => {
-        childAriaTest(await childAriaFixture('feedback'));
+      await childAriaTest(await childAriaFixture('feedback'));
     });
 
     it(`reads help-text message belonging to fieldset when child input is focused
         (via aria-describedby)`, async () => {
-        childAriaTest(await childAriaFixture('help-text'));
+      await childAriaTest(await childAriaFixture('help-text'));
     });
+
+    it(`cleans up feedback message belonging to fieldset  on disconnect`, async () => {
+      const el = await childAriaFixture('feedback');
+      await childAriaTest(el, { cleanupPhase: true });
+    });
+
+    it(`cleans up help-text message belonging to fieldset on disconnect`, async () => {
+      const el = await childAriaFixture('help-text');
+      await childAriaTest(el, { cleanupPhase: true });
     });
   });
 }

packages/form-core/test/FormControlMixin.test.js

@@ -204,31 +204,6 @@ describe('FormControlMixin', () => {
         expect(/** @type {string} */ (el._inputNode.getAttribute('aria-describedby'))).to.contain(
           `feedback-${el._inputId}`,
         );
-        // const additionalDescription = /** @type {HTMLElement} */ (wrapper.querySelector(
-        //   '#additionalDescription',
-        // ));
-        // el.addToAriaDescribedBy(additionalDescription);
-        // await el.updateComplete;
-
-        // let describedbyAttr = /** @type {string} */ (el._inputNode.getAttribute(
-        //   'aria-describedby',
-        // ));
-
-        // // Now check if ids are added to the end (not overridden)
-        // expect(describedbyAttr).to.contain(`feedback-${el._inputId}`);
-        // // Should be placed in the end
-        // expect(
-        //   describedbyAttr.indexOf(`feedback-${el._inputId}`) <
-        //     describedbyAttr.indexOf('additionalDescription'),
-        // );
-
-        // // 2b. removeFromAriaDescription()
-        // el.removeFromAriaDescribedBy(additionalDescription);
-        // await el.updateComplete;
-
-        // describedbyAttr = /** @type {string} */ (el._inputNode.getAttribute('aria-describedby'));
-        // // Now check if ids are added to the end (not overridden)
-        // expect(describedbyAttr).to.not.contain(`additionalDescription`);
       });
 
       it('sorts internal elements, and allows opt-out', async () => {

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

@@ -181,6 +181,7 @@ export declare class FormControlHost {
   __repropagateChildrenInitialized: boolean | undefined;
   protected _onBeforeRepropagateChildrenValues(ev: CustomEvent): void;
   __repropagateChildrenValues(ev: CustomEvent): void;
+  _parentFormGroup: FormControlHost;
 }
 
 export declare function FormControlImplementation<T extends Constructor<LitElement>>(

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

@@ -23,6 +23,7 @@ export declare class FormGroupHost {
   _setValueForAllFormElements(property: string, value: any): void;
   resetInteractionState(): void;
   clearGroup(): void;
+  __descriptionElementsInParentChain: Set<HTMLElement>;
 }
 
 export declare function FormGroupImplementation<T extends Constructor<LitElement>>(