ayo/lion
1
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source
This commit is contained in:
Thijs Louisse 2021-04-09 17:57:37 +02:00
parent fb1522dda5
commit 75af80be3b
10 changed files with 207 additions and 110 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
.changeset/rude-ducks-hide.md Normal file
View file

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

4
packages/form-core/src/FormControlMixin.js
View file

@ -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) {

3
packages/form-core/src/choice-group/ChoiceInputMixin.js
View file

@ -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;
}
}

77
packages/form-core/src/form-group/FormGroupMixin.js
View file

@ -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) {
// 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);
}
};

1
packages/form-core/src/registration/FormRegistrarMixin.js
View file

@ -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;

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

@ -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;
}
}

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

@ -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 });
});
});
}

25
packages/form-core/test/FormControlMixin.test.js
View file

@ -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 () => {

1
packages/form-core/types/FormControlMixinTypes.d.ts vendored
View file

@ -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>>(

1
packages/form-core/types/form-group/FormGroupMixinTypes.d.ts vendored
View file

@ -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>>(