diff --git a/.changeset/rude-ducks-hide.md b/.changeset/rude-ducks-hide.md new file mode 100644 index 000000000..75a8c0124 --- /dev/null +++ b/.changeset/rude-ducks-hide.md @@ -0,0 +1,8 @@ +--- +'@lion/form-core': patch +--- + +**form-core**: + +- cleanup group > child descriptions on disconnectedCallback +- reenable tests diff --git a/packages/form-core/src/FormControlMixin.js b/packages/form-core/src/FormControlMixin.js index 1cf90158e..c9eafffdf 100644 --- a/packages/form-core/src/FormControlMixin.js +++ b/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.} * @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) { diff --git a/packages/form-core/src/choice-group/ChoiceInputMixin.js b/packages/form-core/src/choice-group/ChoiceInputMixin.js index 43f564f9f..3fb7e1ef0 100644 --- a/packages/form-core/src/choice-group/ChoiceInputMixin.js +++ b/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; } } diff --git a/packages/form-core/src/form-group/FormGroupMixin.js b/packages/form-core/src/form-group/FormGroupMixin.js index dfbd2a972..b58fcd967 100644 --- a/packages/form-core/src/form-group/FormGroupMixin.js +++ b/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 + * + * + * ... + *
+ * Park Avenue only has numbers up to 80 + *
+ * */ - __linkChildrenMessagesToParent(child) { - // aria-describedby of (nested) children + __storeAllDescriptionElementsInParentChain() { 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 - this.__linkChildrenMessagesToParent(child); + if (!this.__descriptionElementsInParentChain.size) { + 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); } }; diff --git a/packages/form-core/src/registration/FormRegistrarMixin.js b/packages/form-core/src/registration/FormRegistrarMixin.js index 1e839e951..288b0dbf7 100644 --- a/packages/form-core/src/registration/FormRegistrarMixin.js +++ b/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; diff --git a/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js b/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js index 9ae787d41..5c00fa105 100644 --- a/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js +++ b/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; } } diff --git a/packages/form-core/test-suites/form-group/FormGroupMixin-input.suite.js b/packages/form-core/test-suites/form-group/FormGroupMixin-input.suite.js index b9ab35d6e..ab0ee2368 100644 --- a/packages/form-core/test-suites/form-group/FormGroupMixin-input.suite.js +++ b/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; }; - // eslint-disable-next-line no-shadow - childAriaTest = (/** @type {FormGroup} */ childAriaFixture) => { + childAriaTest = async ( + // eslint-disable-next-line no-shadow + /** @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,75 +159,152 @@ 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]', )); + 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, + 'l1 input(a) refers parent/group', + ); + expect(input_l1_fb.getAttribute('aria-describedby')).to.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.contain( + msg_l1_g.id, + 'l2 input(a) refers grandparent/group.group', + ); + expect(input_l2_fb.getAttribute('aria-describedby')).to.contain( + msg_l1_g.id, + 'l2 input(b) refers grandparent/group.group', + ); + + // Check order: the nearest ('dom wise': so 1. local, 2. parent, 3. grandparent) message + // should be read first by screen reader + const dA = /** @type {string} */ (input_l2_fa.getAttribute('aria-describedby')); + expect( + // @ts-expect-error + dA.indexOf(msg_l2_fa.id) < dA.indexOf(msg_l2_g.id) < dA.indexOf(msg_l1_g.id), + ).to.equal(true, 'order of ids'); + const dB = input_l2_fb.getAttribute('aria-describedby'); + expect( + // @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 */ - - // '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, - 'l1 input(a) refers parent/group', - ); - expect(input_l1_fb.getAttribute('aria-describedby')).to.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.contain( - msg_l1_g.id, - 'l2 input(a) refers grandparent/group.group', - ); - expect(input_l2_fb.getAttribute('aria-describedby')).to.contain( - msg_l1_g.id, - 'l2 input(b) refers grandparent/group.group', - ); - - // Check order: the nearest ('dom wise': so 1. local, 2. parent, 3. grandparent) message - // should be read first by screen reader - const dA = /** @type {string} */ (input_l2_fa.getAttribute('aria-describedby')); - expect( - // @ts-expect-error - dA.indexOf(msg_l2_fa.id) < dA.indexOf(msg_l2_g.id) < dA.indexOf(msg_l1_g.id), - ).to.equal(true, 'order of ids'); - const dB = input_l2_fb.getAttribute('aria-describedby'); - expect( - // @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'); }; + }); - it(`reads feedback message belonging to fieldset when child input is focused + 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 + 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 }); }); }); } diff --git a/packages/form-core/test/FormControlMixin.test.js b/packages/form-core/test/FormControlMixin.test.js index 35a2d8180..483760a23 100644 --- a/packages/form-core/test/FormControlMixin.test.js +++ b/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 () => { diff --git a/packages/form-core/types/FormControlMixinTypes.d.ts b/packages/form-core/types/FormControlMixinTypes.d.ts index 7af27b5df..639014fc8 100644 --- a/packages/form-core/types/FormControlMixinTypes.d.ts +++ b/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>( diff --git a/packages/form-core/types/form-group/FormGroupMixinTypes.d.ts b/packages/form-core/types/form-group/FormGroupMixinTypes.d.ts index b9ca5eed6..748341040 100644 --- a/packages/form-core/types/form-group/FormGroupMixinTypes.d.ts +++ b/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; } export declare function FormGroupImplementation>(