From 95d553e23994181b827a091b724572678bea3b2f Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Fri, 22 Nov 2019 14:48:19 +0100 Subject: [PATCH] feat(field): order aria attributes based on nodes --- packages/field/src/FormControlMixin.js | 165 +++++++----------- .../utils/getAriaElementsInRightDomOrder.js | 26 +++ packages/field/test/lion-field.test.js | 25 ++- .../getAriaElementsInRightDomOrder.test.js | 46 +++++ packages/fieldset/src/LionFieldset.js | 12 +- packages/fieldset/test/lion-fieldset.test.js | 28 ++- packages/select-rich/src/LionSelectRich.js | 55 +++--- 7 files changed, 192 insertions(+), 165 deletions(-) create mode 100644 packages/field/src/utils/getAriaElementsInRightDomOrder.js create mode 100644 packages/field/test/utils/getAriaElementsInRightDomOrder.test.js diff --git a/packages/field/src/FormControlMixin.js b/packages/field/src/FormControlMixin.js index c48bc12eb..a9bffb133 100644 --- a/packages/field/src/FormControlMixin.js +++ b/packages/field/src/FormControlMixin.js @@ -1,5 +1,16 @@ import { html, css, nothing, dedupeMixin, SlotMixin } from '@lion/core'; import { FormRegisteringMixin } from './FormRegisteringMixin.js'; +import { getAriaElementsInRightDomOrder } from './utils/getAriaElementsInRightDomOrder.js'; + +/** + * Generates random unique identifier (for dom elements) + * @param {string} prefix + */ +function uuid(prefix) { + return `${prefix}-${Math.random() + .toString(36) + .substr(2, 10)}`; +} /** * #FormControlMixin : @@ -17,26 +28,10 @@ export const FormControlMixin = dedupeMixin( class FormControlMixin extends FormRegisteringMixin(SlotMixin(superclass)) { static get properties() { return { - /** - * A list of ids that will be put on the _inputNode as a serialized string - */ - _ariaDescribedby: { - type: String, - }, - - /** - * A list of ids that will be put on the _inputNode as a serialized string - */ - _ariaLabelledby: { - type: String, - }, - /** * When no light dom defined and prop set */ - label: { - type: String, - }, + label: String, /** * When no light dom defined and prop set @@ -45,6 +40,16 @@ export const FormControlMixin = dedupeMixin( type: String, attribute: 'help-text', }, + + /** + * Contains all elements that should end up in aria-labelledby of `._inputNode` + */ + _ariaLabelledNodes: Array, + + /** + * Contains all elements that should end up in aria-describedby of `._inputNode` + */ + _ariaDescribedNodes: Array, }; } @@ -67,12 +72,20 @@ export const FormControlMixin = dedupeMixin( updated(changedProps) { super.updated(changedProps); - if (changedProps.has('_ariaLabelledby')) { - this._onAriaLabelledbyChanged({ _ariaLabelledby: this._ariaLabelledby }); + if (changedProps.has('_ariaLabelledNodes')) { + this.__reflectAriaAttr( + 'aria-labelledby', + this._ariaLabelledNodes, + this.__reorderAriaLabelledNodes, + ); } - if (changedProps.has('_ariaDescribedby')) { - this._onAriaDescribedbyChanged({ _ariaDescribedby: this._ariaDescribedby }); + if (changedProps.has('_ariaDescribedNodes')) { + this.__reflectAriaAttr( + 'aria-describedby', + this._ariaDescribedNodes, + this.__reorderAriaDescribedNodes, + ); } if (changedProps.has('label')) { @@ -102,11 +115,9 @@ export const FormControlMixin = dedupeMixin( constructor() { super(); - this._inputId = `${this.localName}-${Math.random() - .toString(36) - .substr(2, 10)}`; - this._ariaLabelledby = ''; - this._ariaDescribedby = ''; + this._inputId = uuid(this.localName); + this._ariaLabelledNodes = []; + this._ariaDescribedNodes = []; } connectedCallback() { @@ -118,7 +129,6 @@ export const FormControlMixin = dedupeMixin( /** * Public methods */ - _enhanceLightDomClasses() { if (this._inputNode) { this._inputNode.classList.add('form-control'); @@ -133,26 +143,14 @@ export const FormControlMixin = dedupeMixin( } if (_labelNode) { _labelNode.setAttribute('for', this._inputId); - _labelNode.id = _labelNode.id || `label-${this._inputId}`; - const labelledById = ` ${_labelNode.id}`; - if (this._ariaLabelledby.indexOf(labelledById) === -1) { - this._ariaLabelledby += ` ${_labelNode.id}`; - } + this.addToAriaLabelledBy(_labelNode, { idPrefix: 'label' }); } if (_helpTextNode) { - _helpTextNode.id = _helpTextNode.id || `help-text-${this._inputId}`; - const describeIdHelpText = ` ${_helpTextNode.id}`; - if (this._ariaDescribedby.indexOf(describeIdHelpText) === -1) { - this._ariaDescribedby += ` ${_helpTextNode.id}`; - } + this.addToAriaDescribedBy(_helpTextNode, { idPrefix: 'help-text' }); } if (_feedbackNode) { _feedbackNode.setAttribute('aria-live', 'polite'); - _feedbackNode.id = _feedbackNode.id || `feedback-${this._inputId}`; - const describeIdFeedback = ` ${_feedbackNode.id}`; - if (this._ariaDescribedby.indexOf(describeIdFeedback) === -1) { - this._ariaDescribedby += ` ${_feedbackNode.id}`; - } + this.addToAriaDescribedBy(_feedbackNode, { idPrefix: 'feedback' }); } this._enhanceLightDomA11yForAdditionalSlots(); } @@ -169,37 +167,30 @@ export const FormControlMixin = dedupeMixin( additionalSlots.forEach(additionalSlot => { const element = this.__getDirectSlotChild(additionalSlot); if (element) { - element.id = element.id || `${additionalSlot}-${this._inputId}`; if (element.hasAttribute('data-label') === true) { - this._ariaLabelledby += ` ${element.id}`; + this.addToAriaLabelledBy(element, { idPrefix: additionalSlot }); } if (element.hasAttribute('data-description') === true) { - this._ariaDescribedby += ` ${element.id}`; + this.addToAriaDescribedBy(element, { idPrefix: additionalSlot }); } } }); } - /** - * Will handle label, prefix/suffix/before/after (if they contain data-label flag attr). - * Also, contents of id references that will be put in the ._ariaLabelledby property - * from an external context, will be read by a screen reader. - */ - _onAriaLabelledbyChanged({ _ariaLabelledby }) { - if (this._inputNode) { - this._inputNode.setAttribute('aria-labelledby', _ariaLabelledby); - } - } - /** * Will handle help text, validation feedback and character counter, * prefix/suffix/before/after (if they contain data-description flag attr). * Also, contents of id references that will be put in the ._ariaDescribedby property * from an external context, will be read by a screen reader. */ - _onAriaDescribedbyChanged({ _ariaDescribedby }) { + __reflectAriaAttr(attrName, nodes, reorder) { if (this._inputNode) { - this._inputNode.setAttribute('aria-describedby', _ariaDescribedby); + if (reorder) { + // eslint-disable-next-line no-param-reassign + nodes = getAriaElementsInRightDomOrder(nodes); + } + const string = nodes.map(n => n.id).join(' '); + this._inputNode.setAttribute(attrName, string); } } @@ -464,36 +455,6 @@ export const FormControlMixin = dedupeMixin( ]; } - // aria-labelledby and aria-describedby helpers - // TODO: consider extracting to generic ariaLabel helper mixin - - /** - * Let the order of adding ids to aria element by DOM order, so that the screen reader - * respects visual order when reading: - * https://developers.google.com/web/fundamentals/accessibility/focus/dom-order-matters - * @param {array} descriptionElements - holds references to description or label elements whose - * id should be returned - * @returns {array} sorted set of elements based on dom order - * - * TODO: make this method part of a more generic mixin or util and also use for lion-field - */ - static _getAriaElementsInRightDomOrder(descriptionElements) { - const putPrecedingSiblingsAndLocalParentsFirst = (a, b) => { - // https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition - const pos = a.compareDocumentPosition(b); - if ( - pos === Node.DOCUMENT_POSITION_PRECEDING || - pos === Node.DOCUMENT_POSITION_CONTAINED_BY - ) { - return 1; - } - return -1; - }; - - const descriptionEls = descriptionElements.filter(el => el); // filter out null references - return descriptionEls.sort(putPrecedingSiblingsAndLocalParentsFirst); - } - // Returns dom references to all elements that should be referred to by field(s) _getAriaDescriptionElements() { return [this._helpTextNode, this._feedbackNode]; @@ -501,22 +462,30 @@ export const FormControlMixin = dedupeMixin( /** * Meant for Application Developers wanting to add to aria-labelledby attribute. - * @param {string} id - should be the id of an element that contains the label for the - * concerned field or fieldset, living in the same shadow root as the host element of field or - * fieldset. + * @param {Element} element */ - addToAriaLabel(id) { - this._ariaLabelledby += ` ${id}`; + addToAriaLabelledBy(element, { idPrefix, reorder } = { reorder: true }) { + // eslint-disable-next-line no-param-reassign + element.id = element.id || `${idPrefix}-${this._inputId}`; + if (!this._ariaLabelledNodes.includes(element)) { + this._ariaLabelledNodes = [...this._ariaLabelledNodes, element]; + // This value will be read when we need to reflect to attr + this.__reorderAriaLabelledNodes = Boolean(reorder); + } } /** * Meant for Application Developers wanting to add to aria-describedby attribute. - * @param {string} id - should be the id of an element that contains the label for the - * concerned field or fieldset, living in the same shadow root as the host element of field or - * fieldset. + * @param {Element} element */ - addToAriaDescription(id) { - this._ariaDescribedby += ` ${id}`; + addToAriaDescribedBy(element, { idPrefix, reorder } = { reorder: true }) { + // eslint-disable-next-line no-param-reassign + element.id = element.id || `${idPrefix}-${this._inputId}`; + if (!this._ariaDescribedNodes.includes(element)) { + this._ariaDescribedNodes = [...this._ariaDescribedNodes, element]; + // This value will be read when we need to reflect to attr + this.__reorderAriaDescribedNodes = Boolean(reorder); + } } __getDirectSlotChild(slotName) { diff --git a/packages/field/src/utils/getAriaElementsInRightDomOrder.js b/packages/field/src/utils/getAriaElementsInRightDomOrder.js new file mode 100644 index 000000000..7bc3a17da --- /dev/null +++ b/packages/field/src/utils/getAriaElementsInRightDomOrder.js @@ -0,0 +1,26 @@ +/** + * @desc Let the order of adding ids to aria element by DOM order, so that the screen reader + * respects visual order when reading: + * https://developers.google.com/web/fundamentals/accessibility/focus/dom-order-matters + * @param {array} descriptionElements - holds references to description or label elements whose + * id should be returned + * @returns {array} sorted set of elements based on dom order + * + */ +export function getAriaElementsInRightDomOrder(descriptionElements, { reverse } = {}) { + const putPrecedingSiblingsAndLocalParentsFirst = (a, b) => { + // https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition + const pos = a.compareDocumentPosition(b); + if (pos === Node.DOCUMENT_POSITION_PRECEDING || pos === Node.DOCUMENT_POSITION_CONTAINED_BY) { + return 1; + } + return -1; + }; + + const descriptionEls = descriptionElements.filter(el => el); // filter out null references + descriptionEls.sort(putPrecedingSiblingsAndLocalParentsFirst); + if (reverse) { + descriptionEls.reverse(); + } + return descriptionEls; +} diff --git a/packages/field/test/lion-field.test.js b/packages/field/test/lion-field.test.js index c8e5d4d39..e23bad50f 100644 --- a/packages/field/test/lion-field.test.js +++ b/packages/field/test/lion-field.test.js @@ -15,7 +15,6 @@ import { localizeTearDown } from '@lion/localize/test-helpers.js'; import '../lion-field.js'; -const nameSuffix = ''; const tagString = 'lion-field'; const tag = unsafeStatic(tagString); const inputSlotString = ''; @@ -197,7 +196,7 @@ describe('', () => { expect(disabledel._inputNode.hasAttribute('disabled')).to.equal(true); }); - describe(`A11y${nameSuffix}`, () => { + describe('Accessibility', () => { it(`by setting corresponding aria-labelledby (for label) and aria-describedby (for helpText, feedback) ~~~ @@ -221,9 +220,9 @@ describe('', () => { `); const nativeInput = Array.from(el.children).find(child => child.slot === 'input'); - expect(nativeInput.getAttribute('aria-labelledby')).to.equal(` label-${el._inputId}`); - expect(nativeInput.getAttribute('aria-describedby')).to.contain(` help-text-${el._inputId}`); - expect(nativeInput.getAttribute('aria-describedby')).to.contain(` feedback-${el._inputId}`); + expect(nativeInput.getAttribute('aria-labelledby')).to.equal(`label-${el._inputId}`); + expect(nativeInput.getAttribute('aria-describedby')).to.contain(`help-text-${el._inputId}`); + expect(nativeInput.getAttribute('aria-describedby')).to.contain(`feedback-${el._inputId}`); }); it(`allows additional slots (prefix, suffix, before, after) to be included in labelledby @@ -239,16 +238,16 @@ describe('', () => { const nativeInput = Array.from(el.children).find(child => child.slot === 'input'); expect(nativeInput.getAttribute('aria-labelledby')).to.contain( - ` before-${el._inputId} after-${el._inputId}`, + `before-${el._inputId} after-${el._inputId}`, ); expect(nativeInput.getAttribute('aria-describedby')).to.contain( - ` prefix-${el._inputId} suffix-${el._inputId}`, + `prefix-${el._inputId} suffix-${el._inputId}`, ); }); // TODO: put this test on FormControlMixin test once there - it(`allows to add to aria description or label via addToAriaLabel() and - addToAriaDescription()`, async () => { + it(`allows to add to aria description or label via addToAriaLabelledBy() and + addToAriaDescribedBy()`, async () => { const wrapper = await fixture(html`
<${tag}> @@ -269,7 +268,7 @@ describe('', () => { // 1. addToAriaLabel() // Check if the aria attr is filled initially expect(_inputNode.getAttribute('aria-labelledby')).to.contain(`label-${el._inputId}`); - el.addToAriaLabel('additionalLabel'); + el.addToAriaLabelledBy(wrapper.querySelector('#additionalLabel')); // Now check if ids are added to the end (not overridden) expect(_inputNode.getAttribute('aria-labelledby')).to.contain(`label-${el._inputId}`); // Should be placed in the end @@ -281,7 +280,7 @@ describe('', () => { // 2. addToAriaDescription() // Check if the aria attr is filled initially expect(_inputNode.getAttribute('aria-describedby')).to.contain(`feedback-${el._inputId}`); - el.addToAriaDescription('additionalDescription'); + el.addToAriaDescribedBy(wrapper.querySelector('#additionalDescription')); // Now check if ids are added to the end (not overridden) expect(_inputNode.getAttribute('aria-describedby')).to.contain(`feedback-${el._inputId}`); // Should be placed in the end @@ -292,7 +291,7 @@ describe('', () => { }); }); - describe(`Validation${nameSuffix}`, () => { + describe(`Validation`, () => { beforeEach(() => { // Reset and preload validation translations localizeTearDown(); @@ -414,7 +413,7 @@ describe('', () => { }); }); - describe(`Content projection${nameSuffix}`, () => { + describe(`Content projection`, () => { it('renders correctly all slot elements in light DOM', async () => { const el = await fixture(html` <${tag}> diff --git a/packages/field/test/utils/getAriaElementsInRightDomOrder.test.js b/packages/field/test/utils/getAriaElementsInRightDomOrder.test.js new file mode 100644 index 000000000..00187a7b1 --- /dev/null +++ b/packages/field/test/utils/getAriaElementsInRightDomOrder.test.js @@ -0,0 +1,46 @@ +import { expect, fixture, html } from '@open-wc/testing'; +import { getAriaElementsInRightDomOrder } from '../../src/utils/getAriaElementsInRightDomOrder.js'; + +describe('getAriaElementsInRightDomOrder', () => { + it('orders by putting preceding siblings and local parents first', async () => { + const el = await fixture(html` +
+
+
+
+
+
+
+
+
+
+
+ `); + // eslint-disable-next-line no-unused-vars + const [a, _1, b, _2, bChild, _3, c, _4] = el.querySelectorAll('div'); + const unorderedNodes = [bChild, c, a, b]; + const result = getAriaElementsInRightDomOrder(unorderedNodes); + expect(result).to.eql([a, b, bChild, c]); + }); + + it('can order reversely', async () => { + const el = await fixture(html` +
+
+
+
+
+
+
+
+
+
+
+ `); + // eslint-disable-next-line no-unused-vars + const [a, _1, b, _2, bChild, _3, c, _4] = el.querySelectorAll('div'); + const unorderedNodes = [bChild, c, a, b]; + const result = getAriaElementsInRightDomOrder(unorderedNodes, { reverse: true }); + expect(result).to.eql([c, bChild, b, a]); + }); +}); diff --git a/packages/fieldset/src/LionFieldset.js b/packages/fieldset/src/LionFieldset.js index be6dfc1c2..cfccbd53a 100644 --- a/packages/fieldset/src/LionFieldset.js +++ b/packages/fieldset/src/LionFieldset.js @@ -2,6 +2,7 @@ import { SlotMixin, html, LitElement } from '@lion/core'; import { DisabledMixin } from '@lion/core/src/DisabledMixin.js'; import { ValidateMixin } from '@lion/validate'; import { FormControlMixin, FormRegistrarMixin } from '@lion/field'; +import { getAriaElementsInRightDomOrder } from '@lion/field/src/utils/getAriaElementsInRightDomOrder.js'; import { FormElementsHaveNoError } from './FormElementsHaveNoError.js'; /** @@ -427,15 +428,10 @@ export class LionFieldset extends FormRegistrarMixin( * @param {array} descriptionElements - description elements like feedback and help-text */ static _addDescriptionElementIdsToField(field, descriptionElements) { - // TODO: make clear in documentation that help-text and feedback slot should be appended by now - // and dynamically appending (or dom-ifs etc) doesn't work - // TODO: we can cache this on constructor level for perf, but changing template via providers - // might go wrong then when dom order changes per instance. Although we could check if - // 'provision' has taken place or not - const orderedEls = this._getAriaElementsInRightDomOrder(descriptionElements); + const orderedEls = getAriaElementsInRightDomOrder(descriptionElements, { reverse: true }); orderedEls.forEach(el => { - if (field.addToAriaDescription) { - field.addToAriaDescription(el.id); + if (field.addToAriaDescribedBy) { + field.addToAriaDescribedBy(el, { reorder: false }); } }); } diff --git a/packages/fieldset/test/lion-fieldset.test.js b/packages/fieldset/test/lion-fieldset.test.js index 7baa8064b..fa71b2b0d 100644 --- a/packages/fieldset/test/lion-fieldset.test.js +++ b/packages/fieldset/test/lion-fieldset.test.js @@ -884,11 +884,7 @@ describe('', () => { }); }); - describe('a11y', () => { - // beforeEach(() => { - // localizeTearDown(); - // }); - + describe('Accessibility', () => { it('has role="group" set', async () => { const fieldset = await fixture(html`<${tag}>${inputSlots}`); await nextFrame(); @@ -985,48 +981,46 @@ describe('', () => { /* eslint-enable camelcase */ - const ariaDescribedBy = el => el.getAttribute('aria-describedby'); - // 'L1' fields (inside lion-fieldset[name="l1_g"]) should point to l1(group) msg - expect(ariaDescribedBy(input_l1_fa)).to.contain( + expect(input_l1_fa.getAttribute('aria-describedby')).to.contain( msg_l1_g.id, 'l1 input(a) refers parent/group', ); - expect(ariaDescribedBy(input_l1_fb)).to.contain( + 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(ariaDescribedBy(input_l1_fa)).to.contain( + expect(input_l1_fa.getAttribute('aria-describedby')).to.contain( msg_l1_fa.id, 'l1 input(a) refers local field', ); - expect(ariaDescribedBy(input_l1_fb)).to.contain( + 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(ariaDescribedBy(input_l2_fa)).to.contain( + expect(input_l2_fa.getAttribute('aria-describedby')).to.contain( msg_l1_g.id, 'l2 input(a) refers grandparent/group.group', ); - expect(ariaDescribedBy(input_l2_fb)).to.contain( + 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 - let d = ariaDescribedBy(input_l2_fa); + const dA = input_l2_fa.getAttribute('aria-describedby'); expect( - d.indexOf(msg_l1_g.id) < d.indexOf(msg_l2_g.id) < d.indexOf(msg_l2_fa.id), + dA.indexOf(msg_l2_fa.id) < dA.indexOf(msg_l2_g.id) < dA.indexOf(msg_l1_g.id), ).to.equal(true, 'order of ids'); - d = ariaDescribedBy(input_l2_fb); + const dB = input_l2_fb.getAttribute('aria-describedby'); expect( - d.indexOf(msg_l1_g.id) < d.indexOf(msg_l2_g.id) < d.indexOf(msg_l2_fb.id), + dB.indexOf(msg_l2_fb.id) < dB.indexOf(msg_l2_g.id) < dB.indexOf(msg_l1_g.id), ).to.equal(true, 'order of ids'); }; }); diff --git a/packages/select-rich/src/LionSelectRich.js b/packages/select-rich/src/LionSelectRich.js index bb211223f..eb5938e0f 100644 --- a/packages/select-rich/src/LionSelectRich.js +++ b/packages/select-rich/src/LionSelectRich.js @@ -74,6 +74,9 @@ export class LionSelectRich extends OverlayMixin( ]; } + /** + * @override + */ static _isPrefilled(modelValue) { if (!modelValue) { return false; @@ -202,6 +205,13 @@ export class LionSelectRich extends OverlayMixin( } } + get _inputNode() { + // In FormControl, we get direct child [slot="input"]. This doesn't work, because the overlay + // system wraps it in [slot="_overlay-shadow-outlet"] + // TODO: find a way to solve this by putting the wrapping part in shadow dom... + return this.querySelector('[slot="input"]'); + } + updated(changedProps) { super.updated(changedProps); @@ -214,6 +224,22 @@ export class LionSelectRich extends OverlayMixin( this.__retractRequestOptionsToBeDisabled(); } } + + if (this._inputNode && this._invokerNode) { + if (changedProps.has('_ariaLabelledNodes')) { + this._invokerNode.setAttribute( + 'aria-labelledby', + `${this._inputNode.getAttribute('aria-labelledby')} ${this._invokerNode.id}`, + ); + } + + if (changedProps.has('_ariaDescribedNodes')) { + this._invokerNode.setAttribute( + 'aria-describedby', + this._inputNode.getAttribute('aria-describedby'), + ); + } + } } toggle() { @@ -272,35 +298,6 @@ export class LionSelectRich extends OverlayMixin( return this.formElements.map(e => e[property]); } - /** - * add same aria-label to invokerNode as _inputNode - * @override - */ - _onAriaLabelledbyChanged({ _ariaLabelledby }) { - if (this._inputNode) { - this._inputNode.setAttribute('aria-labelledby', _ariaLabelledby); - } - if (this._invokerNode) { - this._invokerNode.setAttribute( - 'aria-labelledby', - `${_ariaLabelledby} ${this._invokerNode.id}`, - ); - } - } - - /** - * add same aria-label to invokerNode as _inputNode - * @override - */ - _onAriaDescribedbyChanged({ _ariaDescribedby }) { - if (this._inputNode) { - this._inputNode.setAttribute('aria-describedby', _ariaDescribedby); - } - if (this._invokerNode) { - this._invokerNode.setAttribute('aria-describedby', _ariaDescribedby); - } - } - __setupEventListeners() { this.__onChildActiveChanged = this.__onChildActiveChanged.bind(this); this.__onChildModelValueChanged = this.__onChildModelValueChanged.bind(this);