From fb1522dda58ce7dfcbc719e9dd9ac92289bb59eb Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Thu, 8 Apr 2021 19:54:34 +0200 Subject: [PATCH 1/3] fix(form-core): fieldset label as child label suffix --- .changeset/rotten-dots-argue.md | 5 + packages/form-core/src/FormControlMixin.js | 49 +- .../src/form-group/FormGroupMixin.js | 10 +- .../form-group/FormGroupMixin-input.suite.js | 30 +- .../form-core/test/FormControlMixin.test.js | 442 +++++++++++------- packages/form-core/test/lion-field.test.js | 53 --- .../types/FormControlMixinTypes.d.ts | 12 + 7 files changed, 367 insertions(+), 234 deletions(-) create mode 100644 .changeset/rotten-dots-argue.md diff --git a/.changeset/rotten-dots-argue.md b/.changeset/rotten-dots-argue.md new file mode 100644 index 000000000..03ccbebf3 --- /dev/null +++ b/.changeset/rotten-dots-argue.md @@ -0,0 +1,5 @@ +--- +'@lion/form-core': patch +--- + +**form-core**: fieldset label as child label suffix. Mimics native fieldset a11y diff --git a/packages/form-core/src/FormControlMixin.js b/packages/form-core/src/FormControlMixin.js index 0556a9890..1cf90158e 100644 --- a/packages/form-core/src/FormControlMixin.js +++ b/packages/form-core/src/FormControlMixin.js @@ -321,10 +321,10 @@ const FormControlMixinImplementation = superclass => additionalSlots.forEach(additionalSlot => { const element = this.__getDirectSlotChild(additionalSlot); if (element) { - if (element.hasAttribute('data-label') === true) { + if (element.hasAttribute('data-label')) { this.addToAriaLabelledBy(element, { idPrefix: additionalSlot }); } - if (element.hasAttribute('data-description') === true) { + if (element.hasAttribute('data-description')) { this.addToAriaDescribedBy(element, { idPrefix: additionalSlot }); } } @@ -346,6 +346,7 @@ const FormControlMixinImplementation = superclass => if (reorder) { const insideNodes = nodes.filter(n => this.contains(n)); const outsideNodes = nodes.filter(n => !this.contains(n)); + // eslint-disable-next-line no-param-reassign nodes = [...getAriaElementsInRightDomOrder(insideNodes), ...outsideNodes]; } @@ -704,12 +705,7 @@ const FormControlMixinImplementation = superclass => * @param {HTMLElement} element * @param {{idPrefix?:string; reorder?: boolean}} customConfig */ - addToAriaLabelledBy(element, customConfig = {}) { - const { idPrefix, reorder } = { - reorder: true, - ...customConfig, - }; - + addToAriaLabelledBy(element, { idPrefix = '', reorder = true } = {}) { // eslint-disable-next-line no-param-reassign element.id = element.id || `${idPrefix}-${this._inputId}`; if (!this._ariaLabelledNodes.includes(element)) { @@ -720,18 +716,27 @@ const FormControlMixinImplementation = superclass => } } + /** + * Meant for Application Developers wanting to delete from aria-labelledby attribute. + * @param {HTMLElement} element + */ + removeFromAriaLabelledBy(element) { + if (this._ariaLabelledNodes.includes(element)) { + this._ariaLabelledNodes.splice(this._ariaLabelledNodes.indexOf(element), 1); + this._ariaLabelledNodes = [...this._ariaLabelledNodes]; + + // This value will be read when we need to reflect to attr + /** @type {boolean} */ + this.__reorderAriaLabelledNodes = false; + } + } + /** * Meant for Application Developers wanting to add to aria-describedby attribute. * @param {HTMLElement} element * @param {{idPrefix?:string; reorder?: boolean}} customConfig */ - addToAriaDescribedBy(element, customConfig = {}) { - const { idPrefix, reorder } = { - // chronologically sorts children of host element('this') - reorder: true, - ...customConfig, - }; - + addToAriaDescribedBy(element, { idPrefix = '', reorder = true } = {}) { // eslint-disable-next-line no-param-reassign element.id = element.id || `${idPrefix}-${this._inputId}`; if (!this._ariaDescribedNodes.includes(element)) { @@ -742,6 +747,20 @@ const FormControlMixinImplementation = superclass => } } + /** + * Meant for Application Developers wanting to delete from aria-labelledby attribute. + * @param {HTMLElement} element + */ + removeFromAriaDescribedBy(element) { + if (this._ariaDescribedNodes.includes(element)) { + this._ariaDescribedNodes.splice(this._ariaDescribedNodes.indexOf(element), 1); + this._ariaDescribedNodes = [...this._ariaDescribedNodes]; + // This value will be read when we need to reflect to attr + /** @type {boolean} */ + this.__reorderAriaLabelledNodes = false; + } + } + /** * @param {string} slotName * @return {HTMLElement | undefined} diff --git a/packages/form-core/src/form-group/FormGroupMixin.js b/packages/form-core/src/form-group/FormGroupMixin.js index 69de0baa0..dfbd2a972 100644 --- a/packages/form-core/src/form-group/FormGroupMixin.js +++ b/packages/form-core/src/form-group/FormGroupMixin.js @@ -454,6 +454,10 @@ const FormGroupMixinImplementation = superclass => // TODO: Unlink in removeFormElement this.__linkChildrenMessagesToParent(child); this.validate({ clearCurrentResult: true }); + + if (typeof child.addToAriaLabelledBy === 'function' && this._labelNode) { + child.addToAriaLabelledBy(this._labelNode, { reorder: false }); + } } /** @@ -481,11 +485,15 @@ const FormGroupMixinImplementation = superclass => /** * @override of FormRegistrarMixin. Connects ValidateMixin - * @param {FormRegisteringHost} el + * @param {FormRegisteringHost & FormControlHost} el */ removeFormElement(el) { super.removeFormElement(el); this.validate({ clearCurrentResult: true }); + + if (typeof el.removeFromAriaLabelledBy === 'function' && this._labelNode) { + el.removeFromAriaLabelledBy(this._labelNode, { reorder: false }); + } } }; 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 96c21fbf1..b9ab35d6e 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 @@ -40,7 +40,7 @@ export function runFormGroupMixinInputSuite(cfg = {}) { localizeTearDown(); }); - describe('FormGroupMixin with LionInput', () => { + describe('FormGroupMixin with LionField', () => { it('serializes undefined values as "" (nb radios/checkboxes are always serialized)', async () => { const fieldset = /** @type {FormGroup} */ (await fixture(html` <${tag}> @@ -55,6 +55,34 @@ export function runFormGroupMixinInputSuite(cfg = {}) { 'custom[]': ['custom 1', ''], }); }); + + it('suffixes child labels with group label, just like in
', async () => { + const el = /** @type {FormGroup} */ (await fixture(html` + <${tag} label="set"> + <${childTag} name="A" label="fieldA"> + <${childTag} name="B" label="fieldB"> + + `)); + + /** + * @param {LionInput} formControl + */ + function getLabels(formControl) { + return /** @type {string} */ (formControl._inputNode.getAttribute('aria-labelledby')).split( + ' ', + ); + } + const field1 = el.formElements[0]; + const field2 = el.formElements[1]; + + expect(getLabels(field1)).to.eql([field1._labelNode.id, el._labelNode.id]); + expect(getLabels(field2)).to.eql([field2._labelNode.id, el._labelNode.id]); + + // Test the cleanup on disconnected + el.removeChild(field1); + await field1.updateComplete; + expect(getLabels(field1)).to.eql([field1._labelNode.id]); + }); }); describe('Screen reader relations (aria-describedby) for child inputs and fieldsets', () => { diff --git a/packages/form-core/test/FormControlMixin.test.js b/packages/form-core/test/FormControlMixin.test.js index 6e7686b90..35a2d8180 100644 --- a/packages/form-core/test/FormControlMixin.test.js +++ b/packages/form-core/test/FormControlMixin.test.js @@ -5,35 +5,13 @@ import { FormControlMixin } from '../src/FormControlMixin.js'; import { FormRegistrarMixin } from '../src/registration/FormRegistrarMixin.js'; describe('FormControlMixin', () => { - const inputSlot = ''; + const inputSlot = html``; class FormControlMixinClass extends FormControlMixin(LitElement) {} const tagString = defineCE(FormControlMixinClass); const tag = unsafeStatic(tagString); - it('has a label', async () => { - const elAttr = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag} label="Email address">${inputSlot} - `)); - - expect(elAttr.label).to.equal('Email address', 'as an attribute'); - - const elProp = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag} - .label=${'Email address'} - >${inputSlot} - `)); - expect(elProp.label).to.equal('Email address', 'as a property'); - - const elElem = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag}> - - ${inputSlot} - `)); - expect(elElem.label).to.equal('Email address', 'as an element'); - }); - it('is hidden when attribute hidden is true', async () => { const el = await fixture(html` <${tag} hidden> @@ -43,172 +21,308 @@ describe('FormControlMixin', () => { expect(el).not.to.be.displayed; }); - it('has a label that supports inner html', async () => { - const el = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag}> - - ${inputSlot} - `)); - expect(el.label).to.equal('Email address'); - }); + describe('Label and helpText api', () => { + it('has a label', async () => { + const elAttr = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag} label="Email address">${inputSlot} + `)); - it('only takes label of direct child', async () => { - const el = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag}> - <${tag} label="Email address"> + expect(elAttr.label).to.equal('Email address', 'as an attribute'); + + const elProp = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag} + .label=${'Email address'} + >${inputSlot} + `)); + expect(elProp.label).to.equal('Email address', 'as a property'); + + const elElem = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag}> + ${inputSlot} - - `)); - expect(el.label).to.equal(''); - }); + `)); + expect(elElem.label).to.equal('Email address', 'as an element'); + }); - it('can have a help-text', async () => { - const elAttr = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag} help-text="We will not send you any spam">${inputSlot} - `)); - expect(elAttr.helpText).to.equal('We will not send you any spam', 'as an attribute'); - - const elProp = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag} - .helpText=${'We will not send you any spam'} - >${inputSlot} - `)); - expect(elProp.helpText).to.equal('We will not send you any spam', 'as a property'); - - const elElem = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag}> -
We will not send you any spam
- ${inputSlot} - `)); - expect(elElem.helpText).to.equal('We will not send you any spam', 'as an element'); - }); - - it('can have a help-text that supports inner html', async () => { - const el = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag}> -
We will not send you any spam
- ${inputSlot} - `)); - expect(el.helpText).to.equal('We will not send you any spam'); - }); - - it('only takes help-text of direct child', async () => { - const el = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag}> - <${tag} help-text="We will not send you any spam"> + it('has a label that supports inner html', async () => { + const el = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag}> + ${inputSlot} - - `)); - expect(el.helpText).to.equal(''); - }); + `)); + expect(el.label).to.equal('Email address'); + }); - it('does not duplicate aria-describedby and aria-labelledby ids', async () => { - const el = /** @type {FormControlMixinClass} */ (await fixture(` - <${tagString} help-text="This element will be disconnected/reconnected">${inputSlot} - `)); + it('only takes label of direct child', async () => { + const el = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag}> + <${tag} label="Email address"> + ${inputSlot} + + `)); + expect(el.label).to.equal(''); + }); - const wrapper = /** @type {LitElement} */ (await fixture(`
`)); - el.parentElement?.appendChild(wrapper); - wrapper.appendChild(el); - await wrapper.updateComplete; + it('can have a help-text', async () => { + const elAttr = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag} help-text="We will not send you any spam">${inputSlot} + `)); + expect(elAttr.helpText).to.equal('We will not send you any spam', 'as an attribute'); - ['aria-describedby', 'aria-labelledby'].forEach(ariaAttributeName => { - const ariaAttribute = Array.from(el.children) - .find(child => child.slot === 'input') - ?.getAttribute(ariaAttributeName) - ?.trim() - .split(' '); - const hasDuplicate = !!ariaAttribute?.find((elm, i) => ariaAttribute.indexOf(elm) !== i); - expect(hasDuplicate).to.be.false; + const elProp = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag} + .helpText=${'We will not send you any spam'} + >${inputSlot} + `)); + expect(elProp.helpText).to.equal('We will not send you any spam', 'as a property'); + + const elElem = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag}> +
We will not send you any spam
+ ${inputSlot} + `)); + expect(elElem.helpText).to.equal('We will not send you any spam', 'as an element'); + }); + + it('can have a help-text that supports inner html', async () => { + const el = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag}> +
We will not send you any spam
+ ${inputSlot} + `)); + expect(el.helpText).to.equal('We will not send you any spam'); + }); + + it('only takes help-text of direct child', async () => { + const el = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag}> + <${tag} help-text="We will not send you any spam"> + ${inputSlot} + + `)); + expect(el.helpText).to.equal(''); }); }); - // FIXME: Broken test - it.skip('internally sorts aria-describedby and aria-labelledby ids', async () => { - const wrapper = await fixture(html` -
-
should go after input internals
-
should go after input internals
+ describe('Accessibility', () => { + it('does not duplicate aria-describedby and aria-labelledby ids on reconnect', async () => { + const wrapper = /** @type {HTMLElement} */ (await fixture(html` +
+ <${tag} help-text="This element will be disconnected/reconnected">${inputSlot} +
+ `)); + const el = /** @type {FormControlMixinClass} */ (wrapper.querySelector(tagString)); + const labelIdsBefore = /** @type {string} */ (el._inputNode.getAttribute('aria-labelledby')); + const descriptionIdsBefore = /** @type {string} */ (el._inputNode.getAttribute( + 'aria-describedby', + )); + // Reconnect + wrapper.removeChild(el); + wrapper.appendChild(el); + const labelIdsAfter = /** @type {string} */ (el._inputNode.getAttribute('aria-labelledby')); + const descriptionIdsAfter = /** @type {string} */ (el._inputNode.getAttribute( + 'aria-describedby', + )); + + expect(labelIdsBefore).to.equal(labelIdsAfter); + expect(descriptionIdsBefore).to.equal(descriptionIdsAfter); + }); + + it('adds aria-live="polite" to the feedback slot', async () => { + const el = /** @type {FormControlMixinClass} */ (await fixture(html` <${tag}> - - -
Added to description by default
+ ${inputSlot} +
Added to see attributes
-
should go after input internals
-
should go after input internals
-
`); - const el = /** @type {FormControlMixinClass} */ (wrapper.querySelector(tagString)); - const { _inputNode } = el; + `)); - // 1. addToAriaLabelledBy() - // external inputs should go in order defined by user - const labelA = /** @type {HTMLElement} */ (wrapper.querySelector('#additionalLabelA')); - const labelB = /** @type {HTMLElement} */ (wrapper.querySelector('#additionalLabelB')); - el.addToAriaLabelledBy(labelA); - el.addToAriaLabelledBy(labelB); + expect( + Array.from(el.children) + .find(child => child.slot === 'feedback') + ?.getAttribute('aria-live'), + ).to.equal('polite'); + }); - const ariaLabelId = /** @type {number} */ (_inputNode - .getAttribute('aria-labelledby') - ?.indexOf(`label-${el._inputId}`)); + it('clicking the label should call `_onLabelClick`', async () => { + const spy = sinon.spy(); + const el = /** @type {FormControlMixinClass} */ (await fixture(html` + <${tag} ._onLabelClick="${spy}"> + ${inputSlot} + + `)); + expect(spy).to.not.have.been.called; + el._labelNode.click(); + expect(spy).to.have.been.calledOnce; + }); - const ariaLabelA = /** @type {number} */ (_inputNode - .getAttribute('aria-labelledby') - ?.indexOf('additionalLabelA')); + describe('Adding extra labels and descriptions', () => { + it(`supports centrally orchestrated labels/descriptions via addToAriaLabelledBy() / + removeFromAriaLabelledBy() / addToAriaDescribedBy() / removeFromAriaDescribedBy()`, async () => { + const wrapper = /** @type {HTMLElement} */ (await fixture(html` +
+ <${tag}> + ${inputSlot} + +
Added to description by default
+ +
This also needs to be read whenever the input has focus
+
Same for this
+
`)); + const el = /** @type {FormControlMixinClass} */ (wrapper.querySelector(tagString)); + // wait until the field element is done rendering + await el.updateComplete; + await el.updateComplete; - const ariaLabelB = /** @type {number} */ (_inputNode - .getAttribute('aria-labelledby') - ?.indexOf('additionalLabelB')); + // 1a. addToAriaLabelledBy() + // Check if the aria attr is filled initially + expect(/** @type {string} */ (el._inputNode.getAttribute('aria-labelledby'))).to.contain( + `label-${el._inputId}`, + ); + const additionalLabel = /** @type {HTMLElement} */ (wrapper.querySelector( + '#additionalLabel', + )); + el.addToAriaLabelledBy(additionalLabel); + await el.updateComplete; + let labelledbyAttr = /** @type {string} */ (el._inputNode.getAttribute('aria-labelledby')); + // Now check if ids are added to the end (not overridden) + expect(labelledbyAttr).to.contain(`additionalLabel`); + // Should be placed in the end + expect( + labelledbyAttr.indexOf(`label-${el._inputId}`) < + labelledbyAttr.indexOf('additionalLabel'), + ); - expect(ariaLabelId < ariaLabelB && ariaLabelB < ariaLabelA).to.be.true; + // 1b. removeFromAriaLabelledBy() + el.removeFromAriaLabelledBy(additionalLabel); + await el.updateComplete; + labelledbyAttr = /** @type {string} */ (el._inputNode.getAttribute('aria-labelledby')); + // Now check if ids are added to the end (not overridden) + expect(labelledbyAttr).to.not.contain(`additionalLabel`); - // 2. addToAriaDescribedBy() - // Check if the aria attr is filled initially - const descA = /** @type {HTMLElement} */ (wrapper.querySelector('#additionalDescriptionA')); - const descB = /** @type {HTMLElement} */ (wrapper.querySelector('#additionalDescriptionB')); - el.addToAriaDescribedBy(descB); - el.addToAriaDescribedBy(descA); + // 2a. addToAriaDescribedBy() + // Check if the aria attr is filled initially + 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; - const ariaDescId = /** @type {number} */ (_inputNode - .getAttribute('aria-describedby') - ?.indexOf(`feedback-${el._inputId}`)); + // let describedbyAttr = /** @type {string} */ (el._inputNode.getAttribute( + // 'aria-describedby', + // )); - const ariaDescA = /** @type {number} */ (_inputNode - .getAttribute('aria-describedby') - ?.indexOf('additionalDescriptionA')); + // // 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'), + // ); - const ariaDescB = /** @type {number} */ (_inputNode - .getAttribute('aria-describedby') - ?.indexOf('additionalDescriptionB')); + // // 2b. removeFromAriaDescription() + // el.removeFromAriaDescribedBy(additionalDescription); + // await el.updateComplete; - // Should be placed in the end - expect(ariaDescId < ariaDescB && ariaDescB < ariaDescA).to.be.true; - }); + // 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('adds aria-live="polite" to the feedback slot', async () => { - const el = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag}> - ${inputSlot} -
Added to see attributes
- - `)); + it('sorts internal elements, and allows opt-out', async () => { + const wrapper = await fixture(html` +
+ <${tag}> + + +
+ Added to description by default +
+ +
should go after input internals
+
should go after input internals
+
`); + const el = /** @type {FormControlMixinClass} */ (wrapper.querySelector(tagString)); - expect( - Array.from(el.children) - .find(child => child.slot === 'feedback') - ?.getAttribute('aria-live'), - ).to.equal('polite'); - }); + // N.B. in real life we would never add the input to aria-describedby or -labelledby, + // but this example purely demonstrates dom order is respected. + // A real life scenario would be for instance when + // a Field or FormGroup would be extended and an extra slot would be added in the template + const myInput = /** @type {HTMLElement} */ (wrapper.querySelector('#myInput')); + el.addToAriaLabelledBy(myInput); + await el.updateComplete; + el.addToAriaDescribedBy(myInput); + await el.updateComplete; - it('clicking the label should call `_onLabelClick`', async () => { - const spy = sinon.spy(); - const el = /** @type {FormControlMixinClass} */ (await fixture(html` - <${tag} ._onLabelClick="${spy}"> - ${inputSlot} - - `)); - expect(spy).to.not.have.been.called; - el._labelNode.click(); - expect(spy).to.have.been.calledOnce; + expect( + /** @type {string} */ (el._inputNode.getAttribute('aria-labelledby')).split(' '), + ).to.eql(['myInput', 'internalLabel']); + expect( + /** @type {string} */ (el._inputNode.getAttribute('aria-describedby')).split(' '), + ).to.eql(['myInput', 'internalDescription']); + + // cleanup + el.removeFromAriaLabelledBy(myInput); + await el.updateComplete; + el.removeFromAriaDescribedBy(myInput); + await el.updateComplete; + + // opt-out of reorder + el.addToAriaLabelledBy(myInput, { reorder: false }); + await el.updateComplete; + el.addToAriaDescribedBy(myInput, { reorder: false }); + await el.updateComplete; + + expect( + /** @type {string} */ (el._inputNode.getAttribute('aria-labelledby')).split(' '), + ).to.eql(['internalLabel', 'myInput']); + expect( + /** @type {string} */ (el._inputNode.getAttribute('aria-describedby')).split(' '), + ).to.eql(['internalDescription', 'myInput']); + }); + + it('respects provided order for external elements', async () => { + const wrapper = await fixture(html` +
+
should go after input internals
+
should go after input internals
+ <${tag}> + + +
Added to description by default
+ +
should go after input internals
+
should go after input internals
+
`); + const el = /** @type {FormControlMixinClass} */ (wrapper.querySelector(tagString)); + + // 1. addToAriaLabelledBy() + const labelA = /** @type {HTMLElement} */ (wrapper.querySelector('#externalLabelA')); + const labelB = /** @type {HTMLElement} */ (wrapper.querySelector('#externalLabelB')); + // external inputs should go in order defined by user + el.addToAriaLabelledBy(labelA); + el.addToAriaLabelledBy(labelB); + await el.updateComplete; + + expect( + /** @type {string} */ (el._inputNode.getAttribute('aria-labelledby')).split(' '), + ).to.eql(['internalLabel', 'externalLabelA', 'externalLabelB']); + + // 2. addToAriaDescribedBy() + const descrA = /** @type {HTMLElement} */ (wrapper.querySelector('#externalDescriptionA')); + const descrB = /** @type {HTMLElement} */ (wrapper.querySelector('#externalDescriptionB')); + + el.addToAriaDescribedBy(descrA); + el.addToAriaDescribedBy(descrB); + await el.updateComplete; + + expect( + /** @type {string} */ (el._inputNode.getAttribute('aria-describedby')).split(' '), + ).to.eql(['internalDescription', 'externalDescriptionA', 'externalDescriptionB']); + }); + }); }); describe('Model-value-changed event propagation', () => { diff --git a/packages/form-core/test/lion-field.test.js b/packages/form-core/test/lion-field.test.js index 399ddcf50..d6078fd67 100644 --- a/packages/form-core/test/lion-field.test.js +++ b/packages/form-core/test/lion-field.test.js @@ -193,59 +193,6 @@ describe('', () => { `prefix-${el._inputId} suffix-${el._inputId}`, ); }); - - // TODO: Move test below to FormControlMixin.test.js. - it(`allows to add to aria description or label via addToAriaLabelledBy() and - addToAriaDescribedBy()`, async () => { - const wrapper = /** @type {HTMLElement} */ (await fixture(html` -
- <${tag}> - ${inputSlot} - -
Added to description by default
- -
This also needs to be read whenever the input has focus
-
Same for this
-
`)); - const el = /** @type {LionField} */ (wrapper.querySelector(tagString)); - // wait until the field element is done rendering - await el.updateComplete; - await el.updateComplete; - - const { _inputNode } = el; - - // 1. addToAriaLabel() - // Check if the aria attr is filled initially - expect(_inputNode.getAttribute('aria-labelledby')).to.contain(`label-${el._inputId}`); - const additionalLabel = /** @type {HTMLElement} */ (wrapper.querySelector( - '#additionalLabel', - )); - el.addToAriaLabelledBy(additionalLabel); - const labelledbyAttr = /** @type {string} */ (_inputNode.getAttribute('aria-labelledby')); - // Now check if ids are added to the end (not overridden) - expect(labelledbyAttr).to.contain(`label-${el._inputId}`); - // Should be placed in the end - expect( - labelledbyAttr.indexOf(`label-${el._inputId}`) < labelledbyAttr.indexOf('additionalLabel'), - ); - - // 2. addToAriaDescription() - // Check if the aria attr is filled initially - expect(_inputNode.getAttribute('aria-describedby')).to.contain(`feedback-${el._inputId}`); - const additionalDescription = /** @type {HTMLElement} */ (wrapper.querySelector( - '#additionalDescription', - )); - el.addToAriaDescribedBy(additionalDescription); - const describedbyAttr = /** @type {string} */ (_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'), - ); - }); }); describe(`Validation`, () => { diff --git a/packages/form-core/types/FormControlMixinTypes.d.ts b/packages/form-core/types/FormControlMixinTypes.d.ts index e25aa6a73..7af27b5df 100644 --- a/packages/form-core/types/FormControlMixinTypes.d.ts +++ b/packages/form-core/types/FormControlMixinTypes.d.ts @@ -163,6 +163,18 @@ export declare class FormControlHost { reorder?: boolean | undefined; }, ): void; + public removeFromAriaLabelledBy( + element: HTMLElement, + customConfig?: { + reorder?: boolean | undefined; + }, + ): void; + public removeFromAriaDescribedBy( + element: HTMLElement, + customConfig?: { + reorder?: boolean | undefined; + }, + ): void; __reorderAriaDescribedNodes: boolean | undefined; __getDirectSlotChild(slotName: string): HTMLElement; __dispatchInitialModelValueChangedEvent(): void; From 75af80be3bc159ab3f8ea93fc71ee9ff04cfcd04 Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Fri, 9 Apr 2021 17:57:37 +0200 Subject: [PATCH 2/3] fix(form-core): cleanup group > child descriptions and reenable tests --- .changeset/rude-ducks-hide.md | 8 + packages/form-core/src/FormControlMixin.js | 4 +- .../src/choice-group/ChoiceInputMixin.js | 3 - .../src/form-group/FormGroupMixin.js | 77 ++++--- .../src/registration/FormRegistrarMixin.js | 1 + .../choice-group/ChoiceGroupMixin.suite.js | 1 - .../form-group/FormGroupMixin-input.suite.js | 196 +++++++++++++----- .../form-core/test/FormControlMixin.test.js | 25 --- .../types/FormControlMixinTypes.d.ts | 1 + .../types/form-group/FormGroupMixinTypes.d.ts | 1 + 10 files changed, 207 insertions(+), 110 deletions(-) create mode 100644 .changeset/rude-ducks-hide.md 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>( From 991f1f54f90719dba941f7ace411214bb00813ad Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Fri, 9 Apr 2021 23:17:34 +0200 Subject: [PATCH 3/3] fix(combobox): types --- .changeset/slimy-items-wait.md | 6 +++++ packages/combobox/src/LionCombobox.js | 22 ++++++++++--------- packages/combobox/test/lion-combobox.test.js | 3 --- packages/form-core/src/FormControlMixin.js | 4 +++- .../types/FormControlMixinTypes.d.ts | 1 + .../choice-group/ChoiceGroupMixinTypes.d.ts | 1 + 6 files changed, 23 insertions(+), 14 deletions(-) create mode 100644 .changeset/slimy-items-wait.md diff --git a/.changeset/slimy-items-wait.md b/.changeset/slimy-items-wait.md new file mode 100644 index 000000000..09a5f7d0e --- /dev/null +++ b/.changeset/slimy-items-wait.md @@ -0,0 +1,6 @@ +--- +'@lion/combobox': patch +'@lion/form-core': patch +--- + +**combobox**: enabled and fixed types diff --git a/packages/combobox/src/LionCombobox.js b/packages/combobox/src/LionCombobox.js index 9f150c977..7923786c9 100644 --- a/packages/combobox/src/LionCombobox.js +++ b/packages/combobox/src/LionCombobox.js @@ -1,4 +1,3 @@ -// @ts-nocheck there's an error in cli that cannot be reproduced locally import { html, css, browserDetection } from '@lion/core'; import { OverlayMixin, withDropdownConfig } from '@lion/overlays'; import { LionListbox } from '@lion/listbox'; @@ -12,6 +11,8 @@ import { LionListbox } from '@lion/listbox'; * @typedef {import('@lion/listbox').LionOptions} LionOptions * @typedef {import('@lion/overlays/types/OverlayConfig').OverlayConfig} OverlayConfig * @typedef {import('@lion/core/types/SlotMixinTypes').SlotsMap} SlotsMap + * @typedef {import('@lion/form-core/types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} ChoiceInputHost + * @typedef {import('@lion/form-core/types/FormControlMixinTypes').FormControlHost} FormControlHost * @typedef {import('../types/SelectionDisplay').SelectionDisplay} SelectionDisplay */ @@ -19,6 +20,7 @@ import { LionListbox } from '@lion/listbox'; * LionCombobox: implements the wai-aria combobox design pattern and integrates it as a Lion * FormControl */ +// @ts-expect-error static properties are not compatible export class LionCombobox extends OverlayMixin(LionListbox) { static get properties() { return { @@ -180,7 +182,6 @@ export class LionCombobox extends OverlayMixin(LionListbox) { * @configure FormControlMixin * Will tell FormControlMixin that a11y wrt labels / descriptions / feedback * should be applied here. - * @protected */ get _inputNode() { if (this._ariaVersion === '1.1') { @@ -413,8 +414,9 @@ export class LionCombobox extends OverlayMixin(LionListbox) { * return options.currentValue.length > 4 && super._showOverlayCondition(options); * } * - * @param {{ currentValue: string, lastKey:string }} options + * @param {{ currentValue: string, lastKey:string|undefined }} options * @protected + * @returns {boolean} */ // eslint-disable-next-line class-methods-use-this _showOverlayCondition({ lastKey }) { @@ -423,7 +425,7 @@ export class LionCombobox extends OverlayMixin(LionListbox) { } // when no keyboard action involved (on focused change), return current opened state if (!lastKey) { - return this.opened; + return /** @type {boolean} */ (this.opened); } const doNotShowOn = ['Tab', 'Esc', 'Enter']; return !doNotShowOn.includes(lastKey); @@ -505,8 +507,7 @@ export class LionCombobox extends OverlayMixin(LionListbox) { * should indeed not repropagate as normally. If there is no elements checked, this will be the only * model-value-changed event that gets received, and we should repropagate it. * - * @param {EventTarget & import('../types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} target - * @protected + * @param {FormControlHost} target */ _repropagationCondition(target) { return super._repropagationCondition(target) || this.formElements.every(el => !el.checked); @@ -800,6 +801,7 @@ export class LionCombobox extends OverlayMixin(LionListbox) { * @overridable * @param {string|string[]} modelValue * @param {string|string[]} oldModelValue + * @param {{phase?:string}} config * @protected */ // eslint-disable-next-line no-unused-vars @@ -817,17 +819,17 @@ export class LionCombobox extends OverlayMixin(LionListbox) { * @protected */ _syncToTextboxMultiple(modelValue, oldModelValue = []) { - const diff = modelValue.filter(x => !oldModelValue.includes(x)); + const diff = modelValue.filter(x => !oldModelValue.includes(x)).toString(); this._setTextboxValue(diff); // or last selected value? } /** * @override FormControlMixin - add form-control to [slot=input] instead of _inputNode - * @protected */ _enhanceLightDomClasses() { - if (this.querySelector('[slot=input]')) { - this.querySelector('[slot=input]').classList.add('form-control'); + const formControl = /** @type {HTMLInputElement} */ (this.querySelector('[slot=input]')); + if (formControl) { + formControl.classList.add('form-control'); } } diff --git a/packages/combobox/test/lion-combobox.test.js b/packages/combobox/test/lion-combobox.test.js index 49e39c5af..a78fc2af9 100644 --- a/packages/combobox/test/lion-combobox.test.js +++ b/packages/combobox/test/lion-combobox.test.js @@ -1534,15 +1534,12 @@ describe('lion-combobox', () => { it('synchronizes autocomplete option to textbox', async () => { let el; [el] = await fruitFixture({ autocomplete: 'both' }); - // @ts-expect-error expect(el._inputNode.getAttribute('aria-autocomplete')).to.equal('both'); [el] = await fruitFixture({ autocomplete: 'list' }); - // @ts-expect-error expect(el._inputNode.getAttribute('aria-autocomplete')).to.equal('list'); [el] = await fruitFixture({ autocomplete: 'none' }); - // @ts-expect-error expect(el._inputNode.getAttribute('aria-autocomplete')).to.equal('none'); }); diff --git a/packages/form-core/src/FormControlMixin.js b/packages/form-core/src/FormControlMixin.js index c9eafffdf..cb24df7ee 100644 --- a/packages/form-core/src/FormControlMixin.js +++ b/packages/form-core/src/FormControlMixin.js @@ -9,6 +9,7 @@ import { Unparseable } from './validate/Unparseable.js'; * @typedef {import('@lion/core').CSSResultArray} CSSResultArray * @typedef {import('@lion/core').nothing} nothing * @typedef {import('@lion/core/types/SlotMixinTypes').SlotsMap} SlotsMap + * @typedef {import('../types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} ChoiceInputHost * @typedef {import('../types/FormControlMixinTypes.js').FormControlMixin} FormControlMixin * @typedef {import('../types/FormControlMixinTypes.js').ModelValueEventDetails} ModelValueEventDetails */ @@ -883,8 +884,9 @@ const FormControlMixinImplementation = superclass => /** * TODO: Extend this in choice group so that target is always a choice input and multipleChoice exists. * This will fix the types and reduce the need for ignores/expect-errors - * @param {EventTarget & import('../types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} target + * @param {EventTarget & ChoiceInputHost} target * @protected + * @overridable */ _repropagationCondition(target) { return !( diff --git a/packages/form-core/types/FormControlMixinTypes.d.ts b/packages/form-core/types/FormControlMixinTypes.d.ts index 639014fc8..a0b348112 100644 --- a/packages/form-core/types/FormControlMixinTypes.d.ts +++ b/packages/form-core/types/FormControlMixinTypes.d.ts @@ -182,6 +182,7 @@ export declare class FormControlHost { protected _onBeforeRepropagateChildrenValues(ev: CustomEvent): void; __repropagateChildrenValues(ev: CustomEvent): void; _parentFormGroup: FormControlHost; + _repropagationCondition(target: FormControlHost): boolean; } export declare function FormControlImplementation>( diff --git a/packages/form-core/types/choice-group/ChoiceGroupMixinTypes.d.ts b/packages/form-core/types/choice-group/ChoiceGroupMixinTypes.d.ts index 54c0c3ee1..be27e4ad8 100644 --- a/packages/form-core/types/choice-group/ChoiceGroupMixinTypes.d.ts +++ b/packages/form-core/types/choice-group/ChoiceGroupMixinTypes.d.ts @@ -49,6 +49,7 @@ export declare class ChoiceGroupHost { __delegateNameAttribute(child: FormControlHost): void; protected _onBeforeRepropagateChildrenValues(ev: Event): void; + __oldModelValue: any; } export declare function ChoiceGroupImplementation>(